Re: [coreboot] v3 CS5536 SMBus bug

[ron minnich]

On Wed, Aug 20, 2008 at 9:30 AM, Carl-Daniel Hailfinger
 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
> 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