v3 compilation failures

[Carl-Daniel Hailfinger]

3 of 5 targets in v3 fail to compile:
- adl/msm800sev
- amd/norwich
- artecgroup/dbe61

While none of these targets worked on real hardware, we were at least
able to compile them and could keep the code mostly warning-free to give
later porters a good start. Right now the accumulated mess is rather
difficult to clean up.

One of the contributing factors is the variety of smbus_read_byte
prototypes. Same problem for spd_read_byte. Which one is the "right"
prototype?

./include/device/smbus.h:int smbus_read_byte(struct device *dev, u8 addr);
./device/smbus_ops.c:int smbus_read_byte(struct device *dev, u8 addr)
(introduced in r307)

./include/lib.h:int smbus_read_byte(unsigned device, unsigned address);
./mainboard/artecgroup/dbe61/initram.c:int smbus_read_byte(unsigned
device, unsigned address)
(introduced in r349)

./southbridge/amd/cs5536/smbus_initram.c:int smbus_read_byte(u16 device,
u8 address)
(introduced in r344)

Patches appreciated.
Hints:
- artecgroup/dbe61 first failed in r527 with "undefined reference to
'spd_read_byte'"
- adl/msm800sev and amd/norwich first failed in r537 with "conflicting
types for 'spd_read_byte'", which was uncovered by the new combined
compilation, but the root cause (conflicting types) had been there for
dozens of revisions.

Regards,
Carl-Daniel

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> 3 of 5 targets in v3 fail to compile:
> adl/msm800sev
> amd/norwich
> artecgroup/dbe61
>
> While none of these targets worked on real hardware, we were at least
> able to compile them and could keep the code mostly warning-free to give
> later porters a good start. Right now the accumulated mess is rather
> difficult to clean up.
>
> One of the contributing factors is the variety of smbus_read_byte
> prototypes. Same problem for spd_read_byte. Which one is the "right"
> prototype?
>
> ./include/device/smbus.h:int smbus_read_byte(struct device *dev, u8 addr);
> ./device/smbus_ops.c:int smbus_read_byte(struct device *dev, u8 addr)
> (introduced in r307)
>
> ./include/lib.h:int smbus_read_byte(unsigned device, unsigned address);
> ./mainboard/artecgroup/dbe61/initram.c:int smbus_read_byte(unsigned
> device, unsigned address)
> (introduced in r349)
>
> ./southbridge/amd/cs5536/smbus_initram.c:int smbus_read_byte(u16 device,
> u8 address)
> (introduced in r344)
>
> Patches appreciated.
> Hints:
> artecgroup/dbe61 first failed in r527 with "undefined reference to
>
> 'spd_read_byte'"
> adl/msm800sev and amd/norwich first failed in r537 with "conflicting
>
> types for 'spd_read_byte'", which was uncovered by the new combined
> compilation, but the root cause (conflicting types) had been there for
> dozens of revisions.
>
> Regards,
> Carl-Daniel

I have patches for norwich.... give me a few more hours.
Marc

[ron minnich]

On Jan 21, 2008 10:12 AM, Marc Jones  wrote:
> > ./include/device/smbus.h:int smbus_read_byte(struct device *dev, u8 addr);
> > ./device/smbus_ops.c:int smbus_read_byte(struct device *dev, u8 addr)
> > (introduced in r307)
> >
> > ./include/lib.h:int smbus_read_byte(unsigned device, unsigned address);
> > ./mainboard/artecgroup/dbe61/initram.c:int smbus_read_byte(unsigned
> > device, unsigned address)
> > (introduced in r349)
> >
> > ./southbridge/amd/cs5536/smbus_initram.c:int smbus_read_byte(u16 device,
> > u8 address)
> > (introduced in r344)

I recommend u16 device, u16 address. The address can be up to 10 bits
as I understand it
on some versions of smbus. Am I wrong on this however?
> > Patches appreciated.
> > Hints:
> > artecgroup/dbe61 first failed in r527 with "undefined reference to
> >
> > 'spd_read_byte'"
> > adl/msm800sev and amd/norwich first failed in r537 with "conflicting
> >
> > types for 'spd_read_byte'", which was uncovered by the new combined
> > compilation, but the root cause (conflicting types) had been there for
> > dozens of revisions.

This is probably mostly my fault, I will try to fix.

[Stefan Reinauer]

ron minnich wrote:
> On Jan 21, 2008 10:12 AM, Marc Jones  wrote:
> > > ./include/device/smbus.h:int smbus_read_byte(struct device *dev, u8 addr);
> > > ./device/smbus_ops.c:int smbus_read_byte(struct device *dev, u8 addr)
> > > (introduced in r307)
> > >
> > > ./include/lib.h:int smbus_read_byte(unsigned device, unsigned address);
> > > ./mainboard/artecgroup/dbe61/initram.c:int smbus_read_byte(unsigned
> > > device, unsigned address)
> > > (introduced in r349)
> > >
> > > ./southbridge/amd/cs5536/smbus_initram.c:int smbus_read_byte(u16 device,
> > > u8 address)
> > > (introduced in r344)
>
> I recommend u16 device, u16 address. The address can be up to 10 bits
> as I understand it
> on some versions of smbus. Am I wrong on this however?

I thought a device always has to be struct device? No?

Stefan

[ron minnich]

On Jan 21, 2008 12:34 PM, Stefan Reinauer  wrote:
> > I recommend u16 device, u16 address. The address can be up to 10 bits
> > as I understand it
> > on some versions of smbus. Am I wrong on this however?
>
> I thought a device always has to be struct device? No?

I'm having a slow day. Sorry. yes you are right. But the addr should
probably be u16

ron

[Marc Jones]

ron minnich wrote:
> On Jan 21, 2008 12:34 PM, Stefan Reinauer  wrote:
> > > I recommend u16 device, u16 address. The address can be up to 10 bits
> > > as I understand it
> > > on some versions of smbus. Am I wrong on this however?
> >
> > I thought a device always has to be struct device? No?
>
> I'm having a slow day. Sorry. yes you are right. But the addr should
> probably be u16
>
> ron

I looked at the smbus spec (it is public) and didn't see anything about 
10bit address.
http://smbus.org/specs/

"SMBus addresses are 7 binary
bits long and are conventionally expressed as 4 bits followed by 3 bits 
followed by the letter ‘b’, for
example, 0001 110b. These addresses occupy the high seven bits of an 
eight-bit field on the bus. The low
bit of this field, however, has other semantic meaning that is not part 
of an SMBus address"

Marc

[ron minnich]

On Jan 21, 2008 12:42 PM, Marc Jones  wrote:
> I looked at the smbus spec (it is public) and didn't see anything about
> 10bit address.
> http://smbus.org/specs/
>
> "SMBus addresses are 7 binary
> bits long and are conventionally expressed as 4 bits followed by 3 bits
> followed by the letter 'b', for
> example, 0001 110b. These addresses occupy the high seven bits of an
> eight-bit field on the bus. The low
> bit of this field, however, has other semantic meaning that is not part
> of an SMBus address"

http://www.robot-electronics.co.uk/htm/using_the_i2c_bus.htm

"All I2C addresses are either 7 bits or 10 bits. The use of 10 bit
addresses is rare and is not covered here. "

Maybe we should not worry about it, but there was at one point the
possibility of 10 bit addresses. I'm fine either way.

thanks

ron

[Uwe Hermann]

On Mon, Jan 21, 2008 at 01:42:21PM -0700, Marc Jones wrote:
> I looked at the smbus spec (it is public) and didn't see anything about 
> 10bit address.
> http://smbus.org/specs/
>
> "SMBus addresses are 7 binary
> bits long and are conventionally expressed as 4 bits followed by 3 bits 
> followed by the letter ‘b’, for
> example, 0001 110b. These addresses occupy the high seven bits of an 
> eight-bit field on the bus. The low
> bit of this field, however, has other semantic meaning that is not part 
> of an SMBus address"

The I2C spec has an optional 10bit addresing feature. I2C is similar
(and mostly compatible) but not identical to SMBus; I'm not sure if the
10 bit addressing is available in SMBus.

Uwe.

[Marc Jones]

ron minnich wrote:
> On Jan 21, 2008 10:12 AM, Marc Jones  wrote:
> > > Patches appreciated.
> > > Hints:
> > > artecgroup/dbe61 first failed in r527 with "undefined reference to
> > >
> > > 'spd_read_byte'"
> > > adl/msm800sev and amd/norwich first failed in r537 with "conflicting
> > >
> > > types for 'spd_read_byte'", which was uncovered by the new combined
> > > compilation, but the root cause (conflicting types) had been there for
> > > dozens of revisions.
>
> This is probably mostly my fault, I will try to fix.

Here is what I had done to get norwich building.

Marc

[ron minnich]

On Jan 21, 2008 1:14 PM, Marc Jones  wrote:
===================================================================
> --- LinuxBIOSv3.orig/include/lib.h      2008-01-11 15:52:52.000000000 -0700
> +++ LinuxBIOSv3/include/lib.h   2008-01-11 16:04:12.000000000 -0700
> @@ -36,11 +36,4 @@
>  void beep_short(void);
>  void beep_long(void);
>
> -/* smbus functions */
> -int smbus_read_byte(unsigned device, unsigned address);

hmm. So where does the prototype live then?
> -/* dram functions */
> -void ram_failure(const char *why);
> -void ram_initialize(int controllers, void *ctrl);

These are in lib/ram.c and even defined in the documentation ;-)

What should we do about that?

ron

[Marc Jones]

ron minnich wrote:
> On Jan 21, 2008 1:14 PM, Marc Jones  wrote:
> > --- LinuxBIOSv3.orig/include/lib.h      2008-01-11 15:52:52.000000000 -0700
> > +++ LinuxBIOSv3/include/lib.h   2008-01-11 16:04:12.000000000 -0700
> > @@ -36,11 +36,4 @@
> >  void beep_short(void);
> >  void beep_long(void);
> >
> > -/* smbus functions */
> > -int smbus_read_byte(unsigned device, unsigned address);
>
> hmm. So where does the prototype live then?
> > -/* dram functions */
> > -void ram_failure(const char *why);
> > -void ram_initialize(int controllers, void *ctrl);
>
> These are in lib/ram.c and even defined in the documentation ;-)
>
> What should we do about that?

You have a point. There should be a standard prototype but lib.h seemed 
to be a random place for them.

Marc

[Uwe Hermann]

On Mon, Jan 21, 2008 at 04:01:57PM -0700, Marc Jones wrote:
> > > -/* smbus functions */
> > > -int smbus_read_byte(unsigned device, unsigned address);
> >
> > hmm. So where does the prototype live then?

There's include/device/smbus.h with
  int smbus_read_byte(struct device *dev, u8 addr);
and also include/lib.h with
  int smbus_read_byte(unsigned device, unsigned address);
at the moment.

The latter should be dropped IMO, include/device/smbus.h sounds like
the correct place to have the prototype.
> > > -/* dram functions */
> > > -void ram_failure(const char *why);
> > > -void ram_initialize(int controllers, void *ctrl);
> >
> > These are in lib/ram.c and even defined in the documentation ;-)
> >
> > What should we do about that?
>
> You have a point. There should be a standard prototype but lib.h seemed 
> to be a random place for them.

I think that's ok, IIRC we did this in order to not have yet another
almost-empty header file (ram.h). We should try to keep the number of files
(especially useless *.h files) down as much as possible/reasonable.

Uwe.

[Stefan Reinauer]

* Marc Jones  [080122 00:01]:
> > > -/* smbus functions */
> > > -int smbus_read_byte(unsigned device, unsigned address);
> >
> > hmm. So where does the prototype live then?

They should not necessarily be inter-stage prototypes. I think they can
be part of the "initram" stage and stay in there. No?
> > > -/* dram functions */
> > > -void ram_failure(const char *why);
> > > -void ram_initialize(int controllers, void *ctrl);
> >
> > These are in lib/ram.c and even defined in the documentation ;-)
> >
> > What should we do about that?
>
> You have a point. There should be a standard prototype but lib.h seemed 
> to be a random place for them.

Don't think we want them in the bootblock. It's too big already anyways.

[Carl-Daniel Hailfinger]

On 28.01.2008 14:28, Stefan Reinauer wrote:
> Marc Jones  [080122 00:01]:
> > > > -/* smbus functions */
> > > > -int smbus_read_byte(unsigned device, unsigned address);
> > >
> > > hmm. So where does the prototype live then?
>
> They should not necessarily be inter-stage prototypes. I think they can
> be part of the "initram" stage and stay in there. No?

We may need access to SMBUS for hardware other than SPD.
> > > > -/* dram functions */
> > > > -void ram_failure(const char *why);
> > > > -void ram_initialize(int controllers, void *ctrl);
> > >
> > > These are in lib/ram.c and even defined in the documentation ;-)
> > >
> > > What should we do about that?
> >
> > You have a point. There should be a standard prototype but lib.h seemed 
> > to be a random place for them.
>
> Don't think we want them in the bootblock. It's too big already anyways.

I always wondered why the bootblock was so large in v3 although not that
much code was linked into it. Maybe I underestimated the amount of code
linked into the bootblock.

Regards,
Carl-Daniel

[Jordan Crouse]

On 28/01/08 19:56 +0100, Carl-Daniel Hailfinger wrote:
> On 28.01.2008 14:28, Stefan Reinauer wrote:
> > Marc Jones  [080122 00:01]:
> > > > > -/* smbus functions */
> > > > > -int smbus_read_byte(unsigned device, unsigned address);
> > > >
> > > > hmm. So where does the prototype live then?
> >
> > They should not necessarily be inter-stage prototypes. I think they can
> > be part of the "initram" stage and stay in there. No?
>
> We may need access to SMBUS for hardware other than SPD.

+1.  Thermal sensors and GPIO extenders most often live in SMB land.

Jordan

[Stefan Reinauer]

* Marc Jones  [080121 22:14]:
> Signed-off-by: Marc Jones

Acked-by: Stefan Reinauer
> Index: LinuxBIOSv3/southbridge/amd/cs5536/smbus_initram.c
>
> --- LinuxBIOSv3.orig/southbridge/amd/cs5536/smbus_initram.c	2008-01-11 15:35:37.000000000 -0700
> +++ LinuxBIOSv3/southbridge/amd/cs5536/smbus_initram.c	2008-01-11 15:57:06.000000000 -0700
> @@ -343,7 +343,7 @@
> @param address The address.
> @return The data from the SMBus packet area or an error of 0xff (i.e. -1).
>
> */
> -int spd_read_byte(u16 device, u8 address)
> +u8 spd_read_byte(u16 device, u8 address)
>  {
>    return smbus_read_byte(device, address);
>  }
> Signed-off-by: Marc Jones

Acked-by: Stefan Reinauer
> Index: LinuxBIOSv3/include/lib.h
>
> --- LinuxBIOSv3.orig/include/lib.h	2008-01-11 15:52:52.000000000 -0700
> +++ LinuxBIOSv3/include/lib.h	2008-01-11 16:04:12.000000000 -0700
> @@ -36,11 +36,4 @@
>  void beep_short(void);
>  void beep_long(void);
>
> -/* smbus functions */
> -int smbus_read_byte(unsigned device, unsigned address);
>
> -/* dram functions */
> -void ram_failure(const char *why);
> -void ram_initialize(int controllers, void *ctrl);
>
> #endif /* LIB_H */

[Marc Jones]

Stefan Reinauer wrote:
> Marc Jones  [080121 22:14]:
> > Signed-off-by: Marc Jones
>
> Acked-by: Stefan Reinauer

r 576
> > Signed-off-by: Marc Jones
>
> Acked-by: Stefan Reinauer

r 568

Thanks,
Marc