Questions about more AMD related flags

[Patrick Georgi]

Hi,

while I opened that can of worms over with QRANK_DIMM_SUPPORT, and
people are listening ;-), let me widen the debate some more:

SET_NB_CFG_54:
1. It's used in two places (dualcore and quadcore AMD code)
2. There, it's set active if undeclared before
3. All other declarations set this active

Any reason to keep this variable at all? If yes, I'll move it to
Kconfig, otherwise I'll just drop it.

and
SET_FIDVID*:
These have _very_ weird behaviour, being set to some defaults in the two
init_cpus.c (and fidvid.c seems to expect to be included after that
one?), and some other settings somewhere else.
I tried to untangle that while moving to Kconfig, but didn't quite
succeed, so is here anyone who knows which defaults (or dependencies)
are correct for each of the following? So far I have:

For K8:
+config SET_FIDVID
+       bool
+       default y if K8_REV_F_SUPPORT
+       default n
+
+config SET_FIDVID_CORE0_ONLY
+       bool
+       default y
+       depends on SET_FIDVID
+
+config SET_FIDVID_CORE_RANGE
+       bool
+       default n
+       depends on SET_FIDVID
+
+config SET_FIDVID_ONE_BY_ONE
+       bool
+       default y
+       depends on SET_FIDVID
+
+config SET_FIDVID_DEBUG
+       bool
+       default n
+       depends on SET_FIDVID
+
+config SET_FIDVID_STORE_AP_APICID_AT_FIRST
+       bool
+       default y
+       depends on SET_FIDVID

For Fam10h:
+config SET_FIDVID
+       bool
+       default y
+
+config SET_FIDVID_CORE0_ONLY
+       bool
+       default n
+       depends on SET_FIDVID
+
+config SET_FIDVID_CORE_RANGE
+       bool
+       default n
+       depends on SET_FIDVID
+
+config SET_FIDVID_DEBUG
+       bool
+       default y
+       depends on SET_FIDVID
+
+config SET_FIDVID_STORE_AP_APICID_AT_FIRST
+       bool
+       default y
+       depends on SET_FIDVID

Corrections welcome

Thanks,
Patrick

[Scott Duplichan]

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Patrick Georgi
Sent: Friday, November 05, 2010 02:11 PM
To: [e-mail]
Subject: [coreboot] Questions about more AMD related flags

]Hi,
]
]while I opened that can of worms over with QRANK_DIMM_SUPPORT, and
]people are listening ;-), let me widen the debate some more:
]
]SET_NB_CFG_54:
]1. It's used in two places (dualcore and quadcore AMD code)
]2. There, it's set active if undeclared before
]3. All other declarations set this active
]
]Any reason to keep this variable at all? If yes, I'll move it to
]Kconfig, otherwise I'll just drop it.

I am familiar with the recent history of this bit. It defaults to
zero, resulting in the 'weird' local apic id numbering. BIOS is 
supposed to set it early to get the normal apic id numbering. 
While I do not know the origin of this strange bit, I do know
it was scheduled for removal in future AMD processors. Unfortunately
the removal plan was rejected. I would certainly get rid of
SET_NB_CFG_54. Just let coreboot set this bit early so that there
is no need to deal with two different local apic id formats.

Thanks,
Scott

]Thanks,
]Patrick

[Patrick Georgi]

Am 06.11.2010 05:32, schrieb Scott Duplichan:
> I am familiar with the recent history of this bit. It defaults to
> zero, resulting in the 'weird' local apic id numbering. BIOS is 
> supposed to set it early to get the normal apic id numbering. 
> While I do not know the origin of this strange bit, I do know
> it was scheduled for removal in future AMD processors. Unfortunately
> the removal plan was rejected. I would certainly get rid of
> SET_NB_CFG_54. Just let coreboot set this bit early so that there
> is no need to deal with two different local apic id formats.

This patch does away with SET_NB_CFG_54, keeping the code that was
selected by this option for all K8+ multicore CPUs.

Signed-off-by: Patrick Georgi

[Scott Duplichan]

-----Original Message-----
From: Patrick Georgi [mailto:[e-mail]] 
Sent: Wednesday, November 17, 2010 04:32 PM
To: Scott Duplichan
Cc: [e-mail]
Subject: Re: [coreboot] Questions about more AMD related flags

]Am 06.11.2010 05:32, schrieb Scott Duplichan:
]> I am familiar with the recent history of this bit. It defaults to
]> zero, resulting in the 'weird' local apic id numbering. BIOS is 
]> supposed to set it early to get the normal apic id numbering. 
]> While I do not know the origin of this strange bit, I do know
]> it was scheduled for removal in future AMD processors. Unfortunately
]> the removal plan was rejected. I would certainly get rid of
]> SET_NB_CFG_54. Just let coreboot set this bit early so that there
]> is no need to deal with two different local apic id formats.
]This patch does away with SET_NB_CFG_54, keeping the code that was
]selected by this option for all K8+ multicore CPUs.
]
]
]Signed-off-by: Patrick Georgi

Tested on mahogany and mahogany_fam10
Acked-by: Scott Duplichan

Thanks,
Scott

[xdrudis]

On Fri, Nov 05, 2010 at 08:11:28PM +0100, Patrick Georgi wrote:
> SET_FIDVID*:
> These have _very_ weird behaviour, being set to some defaults in the two
> init_cpus.c (and fidvid.c seems to expect to be included after that
> one?), and some other settings somewhere else.
> I tried to untangle that while moving to Kconfig, but didn't quite
> succeed, so is here anyone who knows which defaults (or dependencies)
> are correct for each of the following? So far I have:

I've been playing with fidvid.c a little for my single FAM10 cpu (4
cores) slowly at my scarce free time.

I'm quite ignorant and the only result I got has been a consistent
hang in prep_fid_vid after warm reset with my changes, and without 
them it sometimes hanged around there and often later.

So don't quite believe me but
> For Fam10h:
> +config SET_FIDVID
>   bool
>   default y

This enables or disables all cpu and northbridge frecuency and voltage
settings required by BKDG . In order to comply with BKDG it should be
on for all FAM10.  I believe it is set to 1 in all FAM10 boards ?
> +config SET_FIDVID_CORE0_ONLY
>   bool
>   default n
>   depends on SET_FIDVID

I remember I didn't realise for a while that SET_FIDVID_CORE0_ONLY
was off but it was, so I agree it was weird to me, but I thought it 
was just me. There are things in the BKDG that need to be done 
for each core, not just core0, so I decided it 
was rightly off. I'd say FAM10 needs  SET_FIDVID_CORE0_ONLY
to off irrespective of board.

For example, 
fixPsNbVidBeforeWR() in src/cpu/amd/model_10xxx/fidvid.c 
implements steps 1-6 of BKDG 2.4.2.9.1 in case of 
needing nb vid update. The BKDG #31116 rev 3.48 April 2010
says the pstate MSRs need writes for all cores.
The BSP core 0 will always do it, because mainboard/.../romstage.c
calls init_fidvid_bsp(). But the other cores wouldn't 
do it if SET_FIDVID_CORE0_ONLY was on (init_cpus() would call
init_fidvid_ap for core0 of nodes other than BSP but not 
for cores 1,2,3,... of any node)

Is this what you were asking ?
> +config SET_FIDVID_CORE_RANGE
>   bool
>   default n
>   depends on SET_FIDVID

bool ? I Only saw it used in init_fidvid_bsp as the second 
parameter to for_each_ap, and it does:

if 1 iterates over core0 in nodes >0
if 2 iterates over cores 1 .. max (skips core0 of each node)
else iterates over all cores in all nodes (except bsp core 0)

this iterations wait for the cores to give their results and 
then calculates the common fid (frequency Id for the
northbridge, wich must be the minimum i.e. slowest required for
all processors). I guess one can check for each core or for
one core in each processor (node). BKDG says

"F3xD4[NbFid] must be matched between all processors in the coherent
   fabric of a multi-socket sys- tem. The lowest setting from all
   processors in a multi-socket system (determined by using the following
   equa- tions on each processor and selecting the lowest value) is used
   as the common NbFid. "

So I would suppose you don't really need to check all cores in 
a processor. If they share the northbridge how could they
require different frequencies for it ?. But this is only with
respect to the frecuency. Maybe you want to wait for all simply
to make sure you already set the NBVid before you set the NbFid?
I believe for tilapia_fam10 you already have waited 
at wait_all_other_cores_started  before calling init_fidvid_bsp,
so you could have SET_FIDVID_CORE_RANGE to 1 and save a little.
I think it's always 0, though.
> +config SET_FIDVID_DEBUG
>   bool
>   default y
>   depends on SET_FIDVID

For me it's ok at 1 . For FAM10 at least it is only use to enable
traces for fidvid code (frequency and voltage setting).
> +config SET_FIDVID_STORE_AP_APICID_AT_FIRST
>   bool
>   default y
>   depends on SET_FIDVID

model_10xx/fidvid.c 
// if we are tight of CAR stack, disable it
#define SET_FIDVID_STORE_AP_APICID_AT_FIRST 1

I don't fully understand the advantage of enabling it.
Apparently it only enables a table of core apicids 
that is first built and then read to wait for each core
to tell you its nbfid. Why can't you directly iterate
once and wait and calculate the nbfid (which is what
would happen if this was 0) ?.
The other difference is that setting it to 0 would
make it not use SET_FIDVID_CORE_RANGE, and iterate 
always for all cores that have calculated fid, 
(in fact iterate for all cores
if SET_FIDVID_CORE0_ONLY is 0 as it should, or only
cores 0 if SET_FIDVID_CORE0_ONLY is 1 against BKDG).

I don't know about fam f, it uses a different fidvid.c .

[xdrudis]

On Sat, Nov 06, 2010 at 03:32:30PM +0100, xdrudis wrote:
> So I would suppose you don't really need to check all cores in 
> a processor. If they share the northbridge how could they
> require different frequencies for it ?. But this is only with
> respect to the frecuency. Maybe you want to wait for all simply
> to make sure you already set the NBVid before you set the NbFid?
> I believe for tilapia_fam10 you already have waited 
> at wait_all_other_cores_started  before calling init_fidvid_bsp,
> so you could have SET_FIDVID_CORE_RANGE to 1 and save a little.

Sorry, I was mistaken. wait_all_other_cores_started does not mean
wait for all cores that have started (until they're done) 
but wait for all cores to start. I mean that 
wait_all_other_cores_started waits
until cores have written  F10_APSTATE_STARTED to the lapic and 
init_fidvid_bsp waits until they have written F10_APSTATE_RESET,
just after seting nbvid an changing to pstate 1 and then core0 to pstate 0

I'm not 100% that BKDG 2.4.2.9.1 requires synchronization
after each step, which we don't do, but if it is required between
steps 6 and 8 then SET_FIDVID_CORE_RANGE can't be set to 1
because we wouldn't be waiting for all cores to change to p1.