[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