[coreboot] v3 CS5536 SMBus bug
ron minnich
rminnich at gmail.com
Mon Sep 15 19:33:10 CEST 2008
On Wed, Aug 20, 2008 at 9:30 AM, Carl-Daniel Hailfinger
<c-d.hailfinger.devel.2006 at gmx.net> wrote:
> On 20.08.2008 16:37, Stefan Reinauer wrote:
>> Carl-Daniel Hailfinger wrote:
>>
>>> v3 can't use global variables in stage1 or initram. Same applies to
>>> static local variables. See the bug below.
>>>
>>> Ideas for fixes? The generic variable infrastructure would be one option.
>>>
>>>
>>
>> Just call smbus_init() prior to calling smbus_read_byte() the first
>> time. The variable infrastructure might be a nice idea for some things,
>> but I think in cases as simple as this, we should not rely on it.
>>
>> Is there a method to change variables in your "variable infrastructure"
>> across cpus? If so, it could be useful for semaphores / locking. But I
>> don't think that's possible since the stuff is in cache, right?
>>
>
> Thanks to Marc for answering a few questions about that code. Proposed
> patch follows.
> Please note that this patch will break compilation because stage1 code
> tries to call initram code. SMBus init functions have to be moved from
> initram to stage1.
>
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>
> Index: southbridge/amd/cs5536/stage1.c
> ===================================================================
> --- southbridge/amd/cs5536/stage1.c (Revision 790)
> +++ southbridge/amd/cs5536/stage1.c (Arbeitskopie)
> @@ -309,5 +309,5 @@
> cs5536_usb_swapsif();
> cs5536_setup_iobase();
> cs5536_setup_smbus_gpio();
> - /* cs5536_enable_smbus(); -- Leave this out for now. */
> + cs5536_enable_smbus();
> }
> Index: southbridge/amd/cs5536/smbus_initram.c
> ===================================================================
> --- southbridge/amd/cs5536/smbus_initram.c (Revision 790)
> +++ southbridge/amd/cs5536/smbus_initram.c (Arbeitskopie)
> @@ -50,7 +50,7 @@
> * controller address. Code can call this more than once, but the effect of
> * doing so is uncertain due to SMBus address set.
> */
> -static void smbus_init(void)
> +static void cs5536_enable_smbus(void)
> {
> smbus_enable();
>
> @@ -321,13 +321,6 @@
> */
> int smbus_read_byte(u16 device, u8 address)
> {
> - static int first_time = 1;
> -
> - if (first_time) {
> - smbus_init();
> - first_time = 0;
> - }
> -
> return do_smbus_read_byte(SMBUS_IO_BASE, device, address);
> }
>
>
I think we don't want to do it this way, since we can not guarantee that
- all platforms have/need smbus (many do not)
- those platforms that have/need smbus use the cs5536 smbus.
I will try another patch later today. I see no reason that initram
code can't just call smbus_init. It's a simple fix that should solve
the problem.
Thanks
ron
More information about the coreboot
mailing list