On 23/02/08 01:15 +0100, Carl-Daniel Hailfinger wrote:
> On 23.02.2008 00:34, Marc Jones wrote:
> >
> >
> > Carl-Daniel Hailfinger wrote:
> >> On 22.02.2008 22:02, Marc Jones wrote:
> >>> We need to discuss v3 smbus operations. Someone has done a lot of
> >>> work to make smbus_ops.c and smbus.h. The code treats smbus as a
> >>> bus, like the pci bus, with a device structure and all.
> >>>
> >>> This approach seems nice and maybe the right way to do it, but it is
> >>> somewhat overkill. I think that the complexity is one reason why the
> >>> structure is in place in v2 but never used. Instead, simpler
> >>> chipset/mainboard specific functions are used. The other reason is
> >>> that the smbus is accessed in ROMCC/CAR code and not in the main
> >>> coreboot bus enumeration code. My observation is that the SPD is the
> >>> only device on smbus used by most mainboards in coreboot.
> >>>
> >>> So, what do we want to do for v3? If we go with the bus/device
> >>> structure every mainboard in v2 will need to have the smbus
> >>> functions ported. Also, someone will have to figure out how to
> >>> describe the smbus devices in the dts and the entire thing might
> >>> need to use a simpler bus/device structure. Or, we can do as was
> >>> done in v2 and leave it to the chipset/mainboard code.
> >>>
> >>
> >> This is a bit more complicated than visible at first glance. We have
> >> two different smbus_read_byte() functions in v3:
> >> int smbus_read_byte(u16 device, u8 address);
> >> int smbus_read_byte(struct device *dev, u8 addr);
> >>
> >> The confusion arises because these two clearly different functions
> >> have the same name. I'd suggest to rename the first smbus_read_byte
> >> to smbus_read_byte_early to make it clear that it does not care about
> >> the device tree. All initram stuff just uses the first function.
> >
> > That would work, but why have the device tree version if it is never
> > used?
>> That may change in the future. We keep lots of code in v3 which is not
> used yet. AGP, PCIE, CardBus etc.
>>> Rename the non-devicetree version of smbus_read_byte() to
> smbus_read_byte_early() to resolve a naming conflict.
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Nak. I agree with the "rip it out" crowd - pull out any mention of
smbus, stuff the read byte code all under spd_read_byte() and call it good.
Jordan
> Index: LinuxBIOSv3-smbusreadbyte-rename/southbridge/amd/cs5536/smbus_initram.c
> ===================================================================
> --- LinuxBIOSv3-smbusreadbyte-rename/southbridge/amd/cs5536/smbus_initram.c (Revision 616)
> +++ LinuxBIOSv3-smbusreadbyte-rename/southbridge/amd/cs5536/smbus_initram.c (Arbeitskopie)
> @@ -319,7 +319,7 @@
> * @param address The address.
> * @return The data from the SMBus packet area or an error of 0xff (i.e. -1).
> */
> -int smbus_read_byte(u16 device, u8 address)
> +int smbus_read_byte_early(u16 device, u8 address)
> {
> static int first_time = 1;
>> @@ -335,7 +335,7 @@
> * Read a byte from the SPD.
> *
> * For this chip, that is really just saying 'read a byte from SMBus'.
> - * So we use smbus_read_byte(). Nota Bene: leave this here as a function
> + * So we use smbus_read_byte_early(). Nota Bene: leave this here as a function
> * rather than a #define in an obscure location. This function is called
> * only a few dozen times, and it's not performance critical.
> *
> @@ -345,5 +345,5 @@
> */
> u8 spd_read_byte(u16 device, u8 address)
> {
> - return smbus_read_byte(device, address);
> + return smbus_read_byte_early(device, address);
> }
>>>> --
>http://www.hailfinger.org/>>> --
> coreboot mailing list
>coreboot at coreboot.org>http://www.coreboot.org/mailman/listinfo/coreboot>>
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.