v3 CS5536 SMBus bug

[Carl-Daniel Hailfinger]

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.

int smbus_read_byte(u16 device, u8 address)
{
    //BUG here!
    static int first_time = 1;

if (first_time) {
    	smbus_init();
    	first_time = 0;
    }

return do_smbus_read_byte(SMBUS_IO_BASE, device, address);
}

Regards,
Carl-Daniel

[Stefan Reinauer]

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?
> int smbus_read_byte(u16 device, u8 address)
> {
>    //BUG here!
>    static int first_time = 1;
>
> if (first_time) {
>    	smbus_init();
>    	first_time = 0;
>    }
>
> return do_smbus_read_byte(SMBUS_IO_BASE, device, address);
> }
>
> Regards,
> Carl-Daniel

[Carl-Daniel Hailfinger]

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.

Agreed.

By the way, the new section checker has better output:
  CHECK   initram (non-empty .data sections)
/sources/tmptrees/corebootv3-check_illegal_global_vars/build/coreboot.initram_partiallylinked.o:
first_time.3526
make: ***
[/sources/tmptrees/corebootv3-check_illegal_global_vars/build/coreboot.initram]
Error 1
> 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?

It strongly depends on the processor. Some processors share their CAR
area contents, some don't. If CAR is shared, locking and updating is
possible. If CAR is not shared, we need a global register for locking
and a sort of syscall mechanism which allows APs to tell the BSP about
memory writes/reads.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

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);
 }

[ron minnich]

OK let's not commit this one until I can test. We're doing a lot of
shuffling here and I don't want to break geode. I don't have a test
platform yet.

ron

[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

[Stefan Reinauer]

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

bringing the smbus controller in a known state may not be a bad idea.
> those platforms that have/need smbus use the cs5536 smbus.

on a system equipped with a CS5536? Why would anyone build another smbus
controller on the mainboard if the southbridge already has one?
> 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

[ron minnich]

On Mon, Sep 15, 2008 at 12:16 PM, Stefan Reinauer  wrote:
> on a system equipped with a CS5536? Why would anyone build another smbus
> controller on the mainboard if the southbridge already has one?

hardware designers to strange things. I am never willing to rule out
this kind of silly design.

ron