next set of patches: modify the dtc and device code to support IDs

[ron minnich]

This is the next set of changes for the dtc and device code to support
device ids.

They are minor but will make our lives easier. coreboot builds, I will
try to test tonight.

ron

next set of patches: modify the dtc and device code to support IDs

[Peter Stuge]

On Thu, Jan 17, 2008 at 12:55:22PM -0800, ron minnich wrote:
> +#define TYPENAME(a,b,c,d) ((a<<24)|(b<<16)|(c<<8)|(d))

Does each parameter need () in the macro definition here? And maybe
mask away overflowing bits?

#define TYPENAME(a,b,c,d) ((((a)&0xff)<<24)|(((b)&0xff)<<16)|(((c)&0xff)<<8)|((d)&0xff))

?
> DEVICE_ID_ROOT	= TYPENAME('R','O','O','T'),
> DEVICE_ID_PCI	= TYPENAME(' ','P','C','I'),
> DEVICE_ID_PNP	= TYPENAME(' ','P','N','P'),
> DEVICE_ID_I2C	= TYPENAME(' ','I','2','C'),
> DEVICE_ID_APIC	= TYPENAME('A','P','I','C'),
> DEVICE_ID_PCI_DOMAIN = TYPENAME('P','C','I','D'),
> DEVICE_ID_APIC_CLUSTER = TYPENAME('A','P','C','C'),
> DEVICE_ID_CPU = TYPENAME(' ','C','P','U'),
> DEVICE_ID_CPU_BUS =  TYPENAME(' ','B','U','S'),

Dunno..
> +{
>    constructor = "geodelx_north_constructors";
> domainid = "PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_LXBRIDGE";
>
> +};

Different whitespace?
> /* note the flaw here: we're just taking the %s as an initialization string.
> Someone who is good at yacc might be able to put this stuff into the dts language.
> this streq stuff is a bit of a hack
> */
>
> if (streq(prop->name, "pcidomain")){
>    fprintf(f, "\t.path = {.type=DEVICE_PATH_PCI_DOMAIN,.u={.pci_domain={ .domain = %s }}},\n", 
>      prop->val.val);

uhm, what? Can you explain this a little further?

I like the general idea, but no ack just yet.

//Peter

next set of patches: modify the dtc and device code to support IDs

[ron minnich]

On Jan 17, 2008 3:35 PM, Peter Stuge  wrote:
> On Thu, Jan 17, 2008 at 12:55:22PM -0800, ron minnich wrote:
> > +#define TYPENAME(a,b,c,d) ((a<<24)|(b<<16)|(c<<8)|(d))
>
> Does each parameter need () in the macro definition here? And maybe
> mask away overflowing bits?
>
> #define TYPENAME(a,b,c,d) ((((a)&0xff)<<24)|(((b)&0xff)<<16)|(((c)&0xff)<<8)|((d)&0xff))

I'll fix this if we keep the change (see below).
> ?
> > DEVICE_ID_ROOT  = TYPENAME('R','O','O','T'),
> > DEVICE_ID_PCI   = TYPENAME(' ','P','C','I'),
> > DEVICE_ID_PNP   = TYPENAME(' ','P','N','P'),
> > DEVICE_ID_I2C   = TYPENAME(' ','I','2','C'),
> > DEVICE_ID_APIC  = TYPENAME('A','P','I','C'),
> > DEVICE_ID_PCI_DOMAIN = TYPENAME('P','C','I','D'),
> > DEVICE_ID_APIC_CLUSTER = TYPENAME('A','P','C','C'),
> > DEVICE_ID_CPU = TYPENAME(' ','C','P','U'),
> > DEVICE_ID_CPU_BUS =  TYPENAME(' ','B','U','S'),
>
> Dunno..

You mean you don't like it? I can always skip this patch. It is a
suggestion. I don't
know if I like it either, but on the other hand, it might be handy for
jtag debuggers.
> > +{
> >    constructor = "geodelx_north_constructors";
> > domainid = "PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_LXBRIDGE";
> >
> > +};
>
> Different whitespace?

fixed.
> > /* note the flaw here: we're just taking the %s as an initialization string.
> > Someone who is good at yacc might be able to put this stuff into the dts language.
> > this streq stuff is a bit of a hack
> > */
> >
> > if (streq(prop->name, "pcidomain")){
> >    fprintf(f, "\t.path = {.type=DEVICE_PATH_PCI_DOMAIN,.u={.pci_domain={ .domain = %s }}},\n",
> >      prop->val.val);
>
> uhm, what? Can you explain this a little further?

Note that this code is not changed from a year ago. So I won't yank the code :-)

The comment is noteing that we're generating C code from property
names. So, for example, in
 mainboard/pcengines/alix1c/dts , we have this:
                pcidomain = "0";
That property name is picked up by the dtc code shown above and turned
into C code. The property name (pcidomain) matches in the streq; the
property value ("0") is taken as a domain name.

My comment represents some uncertainty on the design, but, in painful,
long, discussions on the v3 list we agreed to this -- so I should
probably just drop the comment. I had forgotten how long it took us to
iterate to this design, so I should not stir up discussion on it ...

That said, I don't see a good way to move forward absent this patch,
but I'm open to suggestion.

Thanks

ron

next set of patches: modify the dtc and device code to support IDs

[Uwe Hermann]

On Thu, Jan 17, 2008 at 12:55:22PM -0800, ron minnich wrote:
> +/* here is a simple macro for creating 32-bit int values from
> 4 chars. I am putting it here as I am not yet sure where to
> put it!
> */

Here's a slighly better comment, IMHO:

/**
 * Create a 32-bit value from four characters. This is better
 * than the usual enum values when using (JTAG) debuggers.
 */
#define TYPENAME(a,b,c,d) ((a<<24)|(b<<16)|(c<<8)|(d))
> /* get the properties out that are generic device props */

Please try to write all code comments (at least full sentences)
correctly, i.e. starting with capital letter and ending in a full stop.
I'm fixing this from time to time in the code, but it keeps coming back...
> @@ -1349,7 +1364,7 @@
>
> fix_next(bi->dt);
>    /* emit any includes that we need  -- TODO: ONLY ONCE PER TYPE*/
> fprintf(f, "#include <device/device.h>\n#include <device/pci.h>\n");
> fprintf(f, "#include <device/device.h>\n#include <device/pci.h>\n#include <device/pci_ids.h>\n");

Shouldn't we just #include pci_ids.h from pci.h? It's tedious and
useless to #include pci_ids.h manually all over the place. We need it
in pretty much every file in v3 (at least in all files which also need pci.h).

Uwe.

next set of patches: modify the dtc and device code to support IDs

[ron minnich]

Uwe, try this version. I took your good comments into account.

ron