[coreboot] proposed change to dtc to allow multiple dts files per single chip

[Carl-Daniel Hailfinger]

On 29.01.2008 17:44, ron minnich wrote:
> Second try.
>   

> In the current version of dtc, if one has the line:
> /config/ = "northbridge/amd/geodelx";
>
> Then the file northbridge/amd/geodelx/dts is read in and processed. 
> Magic(TM) appends the name "/dts" to the path. 
>
> This hack is fine with chips that only do one thing. 
> But some (all) northbridge parts play several roles: APIC cluster, PCI domain
> device, and PCI device. The result is a need for more than one dts, since
> there are three possible devices, with three types of IDs, and so on. 
>
> To keep things sane, I am proposing to enable multiple dts files in a
> directory, names (e.g., nothing required here):
> domaindts
> pcidts
> apicdts
>
> (of course these names can be anything, this is just an example).
> This change will require a change to the dtc, since we can no longer
> assume just one dts file, and hence need a way to name these different 
> files. 
>
> The proposed change is very simple. We now require the full path name 
> for the file, and eliminate the Magic(TM).
>
> So, 
> /config/ = "northbridge/amd/geodelx/pcidts";
>
> will open the pcidts file. 
> /config/ = "northbridge/amd/geodelx/domaindts";
> will open the domain dts. 
>
> Maybe we should just call it domain and pci and apic? works for me.
> /config/ = "northbridge/amd/geodelx/domain";
> /config/ = "northbridge/amd/geodelx/pcibridge";
> /config/ = "northbridge/amd/geodelx/apic";
>
> Changes: 
> dtc.c: create a new function, fopenfile, that will only open a path if it 
> really is a file. Modify dtc_open_file to use this function. fopenfile
> assumes "-" means stdin; should it, or should I move that assumption back
> to dtc_open_file?
> dtc.h: add prototypes
> dtc-parser.y: Given a config path, open the path.
> southbridge/amd/cs5536/cs5536.c: example of how C code changes
>
>
> Signed-off-by: Ronald G. Minnich <rminnich at gmail.com>
>   

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Please see the comments below, but they do not have to be addressed for
this commit, just keep them in mind for future commits in that area.

> Index: util/dtc/dtc.c
> ===================================================================
> --- util/dtc/dtc.c	(revision 565)
> +++ util/dtc/dtc.c	(working copy)
> @@ -61,21 +61,47 @@
>  		fill_fullpaths(child, tree->fullpath);
>  }
>  
> -static FILE *dtc_open_file(char *fname)
> +/* How stupid is POSIX? This stupid: you can fopen a directory if mode
> + * is "r", but then there is very little you can do with it except get errors. 
> + * This function now makes sure that the thing you open is a file. 
> + */
> +FILE *fopenfile(char *fname)
>  {
> -	FILE *f;
> +	FILE *f = NULL;
>  
> -	if (streq(fname, "-"))
> +	if (streq(fname, "-")){
>  		f = stdin;
> -	else
> +	} else {
> +		struct stat buf;
> +		if (stat(fname, &buf) < 0) {
> +			errno = EISDIR;
> +			goto no;
> +		}
> +
> +		if (! S_ISREG(buf.st_mode)){
> +			errno = EISDIR;
> +			goto no;
> +		}
> +
>  		f = fopen(fname, "r");
> +	}
>  
> +no:
> +
> +	return f;
> +}
> +
> +FILE *dtc_open_file(char *fname)
> +{
> +	FILE *f = fopenfile(fname);
> +
>  	if (! f)
>  		die("Couldn't open \"%s\": %s\n", fname, strerror(errno));
>  
>  	return f;
>  }
>  
> +
>  static void usage(void)
>  {
>  	fprintf(stderr, "Usage:\n");
> Index: util/dtc/dtc.h
> ===================================================================
> --- util/dtc/dtc.h	(revision 565)
> +++ util/dtc/dtc.h	(working copy)
> @@ -31,6 +31,8 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <netinet/in.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
>  
>  /* also covers (part of?) linux's byteswap.h functionality */
>  #include "endian.h"
> @@ -236,5 +238,6 @@
>  
>  char *join_path(char *path, char *name);
>  void fill_fullpaths(struct node *tree, char *prefix);
> -
> +FILE *fopenfile(char *fname);
> +FILE *dtc_open_file(char *fname);
>  #endif /* _DTC_H */
> Index: util/dtc/dtc-parser.y
> ===================================================================
> --- util/dtc/dtc-parser.y	(revision 565)
> +++ util/dtc/dtc-parser.y	(working copy)
> @@ -121,17 +121,16 @@
>  config:        DT_CONFIG '(' 
>  		includepath {
>  			void switchin(FILE *f);
> -
> -			/* switch ... */
> -			char path[1024];
>  			FILE *f;
> +			/* The need for a cast here is silly */
> +			char *name = (char *)$3.val;
> +
>  			/* TODO: keep track of which of these we have read in. If we have already done it, then 
>  			  * don't do it twice. 
>  			  */
> -			sprintf(path, "%s/dts", $3.val);
> -			f  = fopen(path, "r");
> +			f  = fopenfile(name);
>  			if (! f){
> -				perror(path);
> +				perror(name);
>  				exit(1);
>  			}
>  			switchin(f);
> Index: southbridge/amd/cs5536/cs5536.c
> ===================================================================
> --- southbridge/amd/cs5536/cs5536.c	(revision 565)
> +++ southbridge/amd/cs5536/cs5536.c	(working copy)
> @@ -172,7 +172,7 @@
>   *
>   * @param sb Southbridge config structure.
>   */
> -static void lpc_init(struct southbridge_amd_cs5536_config *sb)
> +static void lpc_init(struct southbridge_amd_cs5536_dts_config *sb)
>  {
>  	struct msr msr;
>  
> @@ -220,7 +220,7 @@
>   *
>   * @param sb Southbridge config structure.
>   */
> -static void uarts_init(struct southbridge_amd_cs5536_config *sb)
> +static void uarts_init(struct southbridge_amd_cs5536_dts_config *sb)
>  {
>  	struct msr msr;
>  	u16 addr = 0;
> @@ -383,7 +383,7 @@
>   *
>   * @param sb Southbridge config structure.
>   */
> -static void enable_USB_port4(struct southbridge_amd_cs5536_config *sb)
> +static void enable_USB_port4(struct southbridge_amd_cs5536_dts_config *sb)
>  {
>  	u32 *bar;
>  	struct msr msr;
> @@ -475,7 +475,7 @@
>  {
>  	struct device *dev;
>  	struct msr msr;
> -	struct southbridge_amd_cs5536_config *sb;
> +	struct southbridge_amd_cs5536_dts_config *sb;
>  	const struct msrinit *csi;
>  
>  	post_code(P80_CHIPSET_INIT);
> @@ -486,7 +486,7 @@
>  		       __FUNCTION__);
>  		return;
>  	}
> -	sb = (struct southbridge_amd_cs5536_config *)dev->device_configuration;
> +	sb = (struct southbridge_amd_cs5536_dts_config *)dev->device_configuration;
>  
>  #if 0
>  	if (!IsS3Resume())
> @@ -548,8 +548,8 @@
>   */
>  static void southbridge_init(struct device *dev)
>  {
> -	struct southbridge_amd_cs5536_config *sb =
> -	    (struct southbridge_amd_cs5536_config *)dev->device_configuration;
> +	struct southbridge_amd_cs5536_dts_config *sb =
> +	    (struct southbridge_amd_cs5536_dts_config *)dev->device_configuration;
>  
>  	/*
>  	 * struct device *gpiodev;
>   

struct southbridge_amd_cs5536_dts_config is a rather ling name.
While I agree that we want to reflect the name of the dts in the name of
the struct, we probably want to keep the name as short as possible.
Since _dts is already part of the struct name, maybe we can remove the
_config from the end.

Regards,
Carl-Daniel