[coreboot] [commit] r5609 - in trunk/src: drivers drivers/dec drivers/dec/21143 drivers/ti drivers/ti/pcmcia-cardbus include/device mainboard/nokia/ip530

Peter Stuge peter at stuge.se
Sat Jun 5 23:11:29 CEST 2010


I have several issues with this commit. I pointed out at least one of
them already very long ago!


repository service wrote:
> +++ trunk/src/drivers/Kconfig	Fri Jun  4 21:53:55 2010	(r5609)
> @@ -23,3 +23,21 @@
>  	help
>  	It sets PCI class to IDE compatible native mode, allowing
>  	SeaBIOS, FILO etc... to boot from it.
> +
> +config DRIVERS_TI
> +	bool
> +
> +config DRIVERS_TI_PCI1225
> +	select DRIVERS_TI
> +	bool
> +
> +config DRIVERS_TI_PCI1420
> +	select DRIVERS_TI
> +	bool
> +
> +config DRIVERS_TI_PCI1520
> +	select DRIVERS_TI
> +	bool

DRIVERS_TI is not a great name! And is the code for these three
devices really so different that it warrants three different config
variables? I doubt that.


> +++ trunk/src/drivers/Makefile.inc	Fri Jun  4 21:53:55 2010	(r5609)
> @@ -1,3 +1,5 @@
>  subdirs-y += generic/debug
>  subdirs-y += ati/ragexl
>  subdirs-y += sil/3114
> +subdirs-y += ti/pcmcia-cardbus

The way this is done there is now a big disconnect between those
CONFIG_TI_* and the filenames that they relate to. That really hurts
understandability of our code base and I don't like that at all.


> Added: trunk/src/drivers/dec/21143/21143pd.c

This path is really redundant. Is it correct? I am not sure. "Others
are like that" is not a good reason. And why is there both 21143 and
21143pd? Does the code apply to ALL 21143 or strictly to 21143PD? The
CONFIG_ name implies strictly 21143PD, but the directory name implies
the opposite. This ambiguity is not good!


> +++ trunk/src/drivers/dec/21143/21143pd.c	Fri Jun  4 21:53:55 2010	(r5609)
..
> +/**
> + * The following should be set in the mainboard-specific Kconfig file.
> + */
> +#if (!defined(CONFIG_DEC21143_CACHE_LINE_SIZE) || \
> +     !defined(CONFIG_DEC21143_EXPANSION_ROM_BASE_ADDRESS) || \
> +     !defined(CONFIG_DEC21143_COMMAND_AND_STATUS_CONFIGURATION))
> +#error "you must supply these values in your mainboard-specific Kconfig file"
> +#endif
> +
> +/* CONFIG_DEC21143_CACHE_LINE_SIZE try 0x00000000 if unsure */
> +/* CONFIG_DEC21143_EXPANSION_ROM_BASE_ADDRESS try 0x00000000 if unsure */
> +/* CONFIG_DEC21143_COMMAND_AND_STATUS_CONFIGURATION try 0x02800107 or 0x02800007 if unsure */

Please explain why they should be set by the mainboard if there are
default values?

I very strongly dislike adding any configuration whatsoever to
mainboards, I think it is important to try to always avoid it.
Can you explain why this is absolutely neccessary?


> +++ trunk/src/drivers/ti/pcmcia-cardbus/ti-pcmcia-cardbus.c	Fri Jun  4 21:53:55 2010	(r5609)
..
> +#if (	!defined( CONFIG_TI_PCMCIA_CARDBUS_CMDR ) || \
> +	!defined( CONFIG_TI_PCMCIA_CARDBUS_CLSR ) || \
> +	!defined( CONFIG_TI_PCMCIA_CARDBUS_CLTR	) || \
> +	!defined( CONFIG_TI_PCMCIA_CARDBUS_BCR ) || \
> +	!defined( CONFIG_TI_PCMCIA_CARDBUS_SCR ) || \
> +	!defined( CONFIG_TI_PCMCIA_CARDBUS_MRR ) )
> +#error "you must supply these values in your mainboard-specific Kconfig file"
> +#endif

Same thing here. Please explain more about how these values are
mainboard-specific?


> +static void ti_pci1x2y_init(struct device *dev)
> +{
> +	printk(BIOS_INFO, "Init of Texas Instruments PCI1x2x PCMCIA/CardBus controller\n");

Here is confirmation that the CONFIG_ names are not very good for
this piece of code. The function name and printk suggests that the
exact same code is used for all controllers.


> +#ifdef CONFIG_DRIVERS_TI_PCI1225
> +static const struct pci_driver ti_pci1225_driver __pci_driver = {
> +        .ops    = &ti_pci1x2y_ops,
> +        .vendor = PCI_VENDOR_ID_TI,
> +        .device = PCI_DEVICE_ID_TI_1225,
> +};
> +
> +#endif
> +#ifdef CONFIG_DRIVERS_TI_PCI1420
> +static const struct pci_driver ti_pci1420_driver __pci_driver = {
> +        .ops    = &ti_pci1x2y_ops,
> +        .vendor = PCI_VENDOR_ID_TI,
> +        .device = PCI_DEVICE_ID_TI_1420,
> +};
> +#endif
> +#ifdef CONFIG_DRIVERS_TI_PCI1520
> +static const struct pci_driver ti_pci1520_driver __pci_driver = {
> +        .ops    = &ti_pci1x2y_ops,
> +        .vendor = PCI_VENDOR_ID_TI,
> +        .device = PCI_DEVICE_ID_TI_1420,
> +};
> +#endif

And yes indeed - the exact same code is used for all devices. Then I
think there must also only be a single CONFIG_ variable.


> +++ trunk/src/include/device/pci_ids.h	Fri Jun  4 21:53:55 2010	(r5609)
> @@ -696,6 +696,7 @@
>  #define PCI_DEVICE_ID_TI_4410		0xac41
>  #define PCI_DEVICE_ID_TI_4451		0xac42
>  #define PCI_DEVICE_ID_TI_1420		0xac51
> +#define PCI_DEVICE_ID_TI_1520		0xAC55

Somewhat minor, but please use lower case for a-f to look like other
values in the file.


> @@ -1741,6 +1742,10 @@
>  #define PCI_DEVICE_ID_CCD_B00C		0xb00c
>  #define PCI_DEVICE_ID_CCD_B100		0xb100
>  
> +#define PCI_VENDOR_ID_NOKIA		0x13B8	// Nokia Telecommunications oy
> +#define PCI_VENDOR_ID_NOKIA_WIRELESS	0x1603	// Nokia Wireless Communications
> +#define PCI_VENDOR_ID_NOKIA_HOME	0x1622	// Nokia Home Communications

And please don't add such comments. The name of the define should be
good enough to provide sufficiently meaningful information about what
this number represents. If a comment is neccessary then the name is
bad. I think the names are great, so there should be no comments.
(Also, please avoid // comments, they're allowed in C99 but not in
older C standards.)


> +++ trunk/src/mainboard/nokia/ip530/Kconfig	Fri Jun  4 21:53:55 2010	(r5609)
> @@ -24,10 +24,12 @@
>  	select NORTHBRIDGE_INTEL_I440BX
>  	select SOUTHBRIDGE_INTEL_I82371EB
>  	select SUPERIO_SMSC_SMSCSUPERIO
> +	select DRIVERS_TI_PCI1225
> +	select DRIVERS_DEC_21143PD
>  	select ROMCC
> +	select PIRQ_ROUTE
>  	select HAVE_PIRQ_TABLE
>  	select UDELAY_TSC
> -	select BOARD_ROMSIZE_KB_256

I already asked many times why the romsize was removed, but Myles has
already fixed it.


> +## Configuration items for the ethernet adaptors
> +config DEC21143_CACHE_LINE_SIZE

If these are absolutely neccessary then I think they should go into
devicetree.cb rather than Kconfig.


> +## Configuration for the PCMCIA-Cardbus controller.
> +config TI_PCMCIA_CARDBUS_CMDR

Same for these.


//Peter




More information about the coreboot mailing list