Re: [coreboot] r784 - in coreboot-v3: arch/x86 device mainboard/emulation/qemu-x86 mainboard/pcengines/alix1c mainboard/pcengines/alix2c3

[Stefan Reinauer]

This patch is bad. Please back it out.

[e-mail] wrote:
> Modified: coreboot-v3/arch/x86/Kconfig
>
> --- coreboot-v3/arch/x86/Kconfig	2008-08-18 13:03:43 UTC (rev 783)
> +++ coreboot-v3/arch/x86/Kconfig	2008-08-18 16:48:27 UTC (rev 784)
> @@ -93,12 +93,14 @@
>
> config LOGICAL_CPUS
>    hex
> depends CPU_AMD_K8

This is a wrong dependency. It does not reflect any physical dependency,
nor a real dependency in the code. The opposite. It is the purpose that
these variables are used throughout the code to verify the availability
of certain features.

The correct way of describing this would be to set the above variable to
a certain value in CPU_AMD_K8, or any related option.
> default 1
>    help
>    	How many logical CPUs there are. Fix me.
>
> config MAX_PHYSICAL_CPUS
>    hex
> depends CPU_AMD_K8

dito
> default 1
>    help
>    	Max number of physical CPUs (sockets)
> @@ -138,6 +140,7 @@
>
> config SMP
>    boolean
> depends CPU_I586 || CPU_AMD_K8

dito
> default 0
>    help
>      This option is used to enable certain functions to make 
> @@ -147,7 +150,7 @@
>
> config IOAPIC
>    boolean
> depends ARCH_X86
> depends ARCH_X86 && CPU_AMD_K8

dito
> default 0
>    help
>      If you want to configure an IOAPIC, set this. 
> @@ -216,6 +219,7 @@
>
> config USBDEBUG_DIRECT
>    boolean
> depends SOUTHBRIDGE_NVIDIA_MCP55

dito
> default 0
>    help
>    	Determines if we enable USB Direct debugging. If you don't have a dongle,
> Modified: coreboot-v3/device/Kconfig
>
> --- coreboot-v3/device/Kconfig	2008-08-18 13:03:43 UTC (rev 783)
> +++ coreboot-v3/device/Kconfig	2008-08-18 16:48:27 UTC (rev 784)
> @@ -97,11 +97,13 @@
>
> config PCI_64BIT_PREF_MEM
>    bool "64 bit prefetchable memory addresses"
> depends CPU_AMD_K8

dito

r784 - in coreboot-v3: arch/x86 device mainboard/emulation/qemu-x86 mainboard/pcengines/alix1c mainboard/pcengines/alix2c3

[Carl-Daniel Hailfinger]

On 18.08.2008 18:55, Stefan Reinauer wrote:
> This patch is bad. Please back it out.

Can we at least establish a commit message stating the reason for the
backout?
> [e-mail] wrote:
> > Modified: coreboot-v3/arch/x86/Kconfig
> >
> > --- coreboot-v3/arch/x86/Kconfig	2008-08-18 13:03:43 UTC (rev 783)
> > +++ coreboot-v3/arch/x86/Kconfig	2008-08-18 16:48:27 UTC (rev 784)
> > @@ -93,12 +93,14 @@
> >
> > config LOGICAL_CPUS
> >    hex
> > depends CPU_AMD_K8
>
> This is a wrong dependency. It does not reflect any physical dependency,
> nor a real dependency in the code. The opposite.

So should LOGICAL_CPUS be selectable for GeodeLX?
> It is the purpose that
> these variables are used throughout the code to verify the availability
> of certain features.

I have trouble understanding that sentence.
> The correct way of describing this would be to set the above variable to
> a certain value in CPU_AMD_K8, or any related option.

Ah, so you want forward dependencies with select? By the way, at least
our version of Kconfig can't set any variable except boolean, so you're
back to "depends" clauses. If you don't believe me, here is the error
message:

mainboard/amd/Kconfig:57:warning: 'APIC_ID_OFFSET' has wrong type.
'select' only accept arguments of boolean and tristate type

And all this forward-select was frowned upon by Kconfig maintainers
because it is easy to create impossible loops:
A selects B
B depends on !A
Boom.
> > default 1
> >    help
> >    	How many logical CPUs there are. Fix me.
> >
> > config MAX_PHYSICAL_CPUS
> >    hex
> > depends CPU_AMD_K8
>
> dito

dito
> > default 1
> >    help
> >    	Max number of physical CPUs (sockets)
> > @@ -138,6 +140,7 @@
> >
> > config SMP
> >    boolean
> > depends CPU_I586 || CPU_AMD_K8
>
> dito

No, that is a physical dependency. Show me a SMP Geode. It can't be done.
> > default 0
> >    help
> >      This option is used to enable certain functions to make 
> > @@ -147,7 +150,7 @@
> >
> > config IOAPIC
> >    boolean
> > depends ARCH_X86
> > depends ARCH_X86 && CPU_AMD_K8
>
> dito
> > default 0
> >    help
> >      If you want to configure an IOAPIC, set this. 
> > @@ -216,6 +219,7 @@
> >
> > config USBDEBUG_DIRECT
> >    boolean
> > depends SOUTHBRIDGE_NVIDIA_MCP55
>
> dito
> > default 0
> >    help
> >    	Determines if we enable USB Direct debugging. If you don't have a dongle,
> > Modified: coreboot-v3/device/Kconfig
> >
> > --- coreboot-v3/device/Kconfig	2008-08-18 13:03:43 UTC (rev 783)
> > +++ coreboot-v3/device/Kconfig	2008-08-18 16:48:27 UTC (rev 784)
> > @@ -97,11 +97,13 @@
> >
> > config PCI_64BIT_PREF_MEM
> >    bool "64 bit prefetchable memory addresses"
> > depends CPU_AMD_K8
>
> dito

That one should depend on the address space of the CPU.

Regards,
Carl-Daniel