[coreboot] r1009 - in coreboot-v3: mainboard/kontron/986lcd-m northbridge/intel/i945 southbridge/intel/i82801gx

[Peter Stuge]

svn at coreboot.org wrote:
> +++ coreboot-v3/mainboard/kontron/986lcd-m/Makefile	2008-11-12 23:09:42 UTC (rev 1009)
> @@ -28,6 +28,18 @@
>  INITRAM_SRC= $(src)/mainboard/$(MAINBOARDDIR)/initram.c \
>  			$(src)/northbridge/intel/i945/raminit.c \
>  
> +STAGE2_CHIPSET_SRC=\
> +					$(src)/southbridge/intel/i82801gx/ac97.c \
> + 					$(src)/southbridge/intel/i82801gx/lpc.c \
> + 					$(src)/southbridge/intel/i82801gx/nic.c \
> +					$(src)/southbridge/intel/i82801gx/pci.c \
> +					$(src)/southbridge/intel/i82801gx/pcie.c \
> +					$(src)/southbridge/intel/i82801gx/sata.c \
> +					$(src)/southbridge/intel/i82801gx/smbus.c \
> +					$(src)/southbridge/intel/i82801gx/usb_ehci.c \
> +  					$(src)/southbridge/intel/i82801gx/usb.c	\
> +  					$(src)/southbridge/intel/i82801gx/watchdog.c
> +#					$(src)/southbridge/intel/i82801gx/libsmbus.c \

We must fix this design. This list of files has to go in
southbridge/intel/i82801gx/ somewhere and mainboard/ Makefiles should
just reference the right south Makefile per Kconfig settings,
preferably using one common rule.

Did we consider using the scheme that Linux uses to pick which
objects to build? STAGE2_CHIPSET_SRC-$(CONFIG_INTEL_I82801GX) += srcfiles
in south/ and always include every Makefile in every build. (Maybe
that's done anyway?)


> +++ coreboot-v3/mainboard/kontron/986lcd-m/dts	2008-11-12 23:09:42 UTC (rev 1009)
> @@ -181,6 +181,13 @@
>   				pci at 1f,0{/* which ich? */
>  					/config/("southbridge/intel/i82801gx/ich7m_dh_lpc.dts");
>  				};
> +      				pci at 1f,2{
> +					/config/("southbridge/intel/i82801gx/sata.dts");
> +				};
> +        				pci at 1f,3{
> +					/config/("southbridge/intel/i82801gx/smbus.dts");
> +				};

I'd like the same principle for dts. More reuse and one single
include line pulling in whatever is needed for one complex device.


> -static void pci_domain_set_resources(struct device * dev)
> +static void I945_pci_domain_set_resources(struct device * dev)

This is just one example, there are many cases of copypaste like
this. Is it really neccessary? I think the design has failed if we
can't reuse this kind of code.


>  	/* Report the memory regions */
> -	ram_resource(dev, 3, 0, 640);
> -	ram_resource(dev, 4, 768, (tolmk - 768));
> +	i945_ram_resource(dev, 3, 0, 640);
> +	i945_ram_resource(dev, 4, 768, (tolmk - 768));
>  	if (tomk > 4 * 1024 * 1024) {
> -		ram_resource(dev, 5, 4096 * 1024, tomk - 4 * 1024 * 1024);
> +		i945_ram_resource(dev, 5, 4096 * 1024, tomk - 4 * 1024 * 1024);

So what's the difference? I just cut the i945_ram_resource() changes
out, but..


> Modified: coreboot-v3/southbridge/intel/i82801gx/Makefile
> ===================================================================
> --- coreboot-v3/southbridge/intel/i82801gx/Makefile	2008-11-12 22:43:50 UTC (rev 1008)
> +++ coreboot-v3/southbridge/intel/i82801gx/Makefile	2008-11-12 23:09:42 UTC (rev 1009)
> @@ -24,18 +24,6 @@
>  STAGE2_CHIPSET_SRC += $(src)/southbridge/intel/i82801gx/i82801gx.c
>  
>  STAGE2_CHIPSET_SRC += \
> -					$(src)/southbridge/intel/i82801gx/ac97.c \
> - 					$(src)/southbridge/intel/i82801gx/ide.c \
> - 					$(src)/southbridge/intel/i82801gx/lpc.c \
> - 					$(src)/southbridge/intel/i82801gx/nic.c \
> -					$(src)/southbridge/intel/i82801gx/pci.c \
> -					$(src)/southbridge/intel/i82801gx/pcie.c \
> -					$(src)/southbridge/intel/i82801gx/sata.c \
> -					$(src)/southbridge/intel/i82801gx/smbus.c \
> -					$(src)/southbridge/intel/i82801gx/usb_ehci.c \
> -  					$(src)/southbridge/intel/i82801gx/usb.c	\
> -  					$(src)/southbridge/intel/i82801gx/watchdog.c
> -#					$(src)/southbridge/intel/i82801gx/libsmbus.c \

This has to change back. Any solution that does not keep this list of
files in southbridge/ is plain wrong. Sorry.


> +++ coreboot-v3/southbridge/intel/i82801gx/ac97.c	2008-11-12 23:09:42 UTC (rev 1009)
> +void i82801gx_enable(struct device * dev);

What? This function has to get a better name.


> +void i82801gx_enable(struct device * dev);

Here it is again. It appears a few more times.


//Peter