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.
Thoughts? Volunteers?
Marc
On Fri, Feb 22, 2008 at 1:02 PM, Marc Jones marc.jones@amd.com 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.
yes. I agree. Until there is some demonstration that we need smbus outside of initram, we should go with simple. I will accept it if we want to yank this and go with simple and low level.
ron
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.
Regards, Carl-Daniel
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? Marc
On Fri, Feb 22, 2008 at 3:34 PM, Marc Jones marc.jones@amd.com wrote:
That would work, but why have the device tree version if it is never used?
yep, that's the big question. My inclination is to rip it out, record we ripped it out. If we ever need it, we know where to find it.
ron
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@gmx.net
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); }
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@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);
}
-- coreboot mailing list coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
-
Carl-Daniel Hailfinger -
Jordan Crouse -
Marc Jones -
ron minnich