[coreboot] v3 CS5536 SMBus bug

[ron minnich]

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