This patch is some small changes to the vt8237r to prepare it for the Jetway J7F2 patch that should be coming soon, and also moves most defines into vt8237r.h. I've changed some of the values from u32 to u8, because that's all they should ever need to be. Also includes doxygenized comments!
Signed-off-by: Corey Osgood corey.osgood@gmail.com
Corey Osgood wrote:
This patch is some small changes to the vt8237r to prepare it for the Jetway J7F2 patch that should be coming soon, and also moves most defines into vt8237r.h. I've changed some of the values from u32 to u8, because that's all they should ever need to be. Also includes doxygenized comments!
Signed-off-by: Corey Osgood corey.osgood@gmail.com
And here's the patch, for real this time!
-Corey
Hi just a quick question, Did you check with SVN version that it still needs warming up?
Thanks,
R.
On Fri, Nov 02, 2007 at 02:03:18AM -0400, Corey Osgood wrote:
@@ -79,11 +62,14 @@
loops = 0; /* Yes, this is a mess, but it's the easiest way to do it. */
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops < SMBUS_TIMEOUT)
Rationale? Does it make a big difference?
-u8 smbus_read_byte(u32 dimm, u32 offset) +/**
- Read a byte from the smbus
- @param dimm The address location of the dimm on the smbus
- @param offset The offset the data is located at
- */
+u8 smbus_read_byte(u8 dimm, u8 offset)
I'm still not entirely sure they're always only 8 bit. Do you have a pointer to a datasheet or spec or standard where it's explicitly defined as 8 bit? Yes, it _is_ 8 bits in most cases, but can we be sure that it'll be 8 bit in _all_ of them? On all chipsets and controllers?
- /* Can I just "return inb(SMBHSTDAT0)"? */
- /* We could probably return inb(SMBHSTDAT0), but we'd lose the ability
* to debug the transaction */
OK, if that's the only issue, just drop the comment.
+/**
- This is provided for compatibility, should it be needed
- */
+inline u8 spd_read_byte(u32 address, u8 offset) +{
- return smbus_read_byte(address, offset);
+}
Hm, this is usually done in auto.c per-mainboard, I think. Either you make spd_read_byte() a wrapper for smbus_read_byte(), or you use the "fake spd" method to return hard-coded settings if there's no real SPD data to be read.
Not sure this function makes sense in vt8237r_early_smbus.c, because of the above and also because it's not SMBus-related per se. I'd say drop it.
Also, address is 32bit here but 8bit in smbus_read_byte()?
+/**
- A fixup for some systems that need time for the smbus to "warm up"
- It reads the ID byte from SMBus, looking for good data from a slot/address
- Exits on either good data or a timeout
Yep, but please extend the comment a bit to contain more information, rationale, example use case where this issue came up, how the problem shows, how it's fixed etc. The comment is good, but a bit too short for describing this non-trivial issue at hand.
- @param mem_controller The memory controller and smbus addresses
- */
+void smbus_fixup(const struct mem_controller *ctrl) +{
- int i, ram_slots, current_slot = 0;
- u8 result = 0;
+#ifdef DIMM_SOCKETS
- ram_slots = DIMM_SOCKETS;
+#else
- ram_slots = sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0]);
+#endif
Can you explain? Shouldn't DIMM_SOCKETS always match sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0])? When does it happen that they do not match (and why?).
Also, we now have ARRAY_SIZE(), please use it here.
- if (!ram_slots) {
print_err("smbus_fixup thinks there are no ram slots!\r\n");return;- }
- PRINT_DEBUG("Waiting for smbus to warm up");
- /* Bad SPD data should be either 0 or 0xff, so really the values we look
* for are arbitrary, as long as they're between 1 and 0xfe */- for(i = 0; (i < SMBUS_TIMEOUT && ((result < SPD_MEMORY_TYPE_SDRAM) ||
(result > SPD_MEMORY_TYPE_SDRAM_DDR2))); i++)
Please explain the SPD_MEMORY_TYPE_SDRAM/SPD_MEMORY_TYPE_SDRAM_DDR2 check in the comment here.
If all you want is to know whether some sensible RAM type is returned wouldn't "> 0" and "< 0xff" be enough (as per your comment)? You don't really care about the exact type, you only want to know _if_ there's a DIMM here, correct?
If I read this correctly you're checking whether you get one of these?
#define SPD_MEMORY_TYPE_SDRAM 4 #define SPD_MEMORY_TYPE_MULTIPLEXED_ROM 5 #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8
If we make this "> 0" and "< 0xff" ("< 10" or so should be enough, too) then this function might be usable on non-vt8237r chipsets and can go in some global SMBus file to be used by others?
- {
if (current_slot > ram_slots) j = 0;result = spd_read_byte(ctrl->channel0[current_slot],SPD_MEMORY_TYPE);current_slot++;PRINT_DEBUG(".");- }
- if (i >= SMBUS_TIMEOUT) print_err("SMBus timed out while warming up\r\n");
- else PRINT_DEBUG("Done\r\n");
+}
Looks good otherwise. Does this contain all of the changes required to make it work on your board _and_ Rudolf's board?
Thanks, Uwe.
Uwe Hermann wrote:
On Fri, Nov 02, 2007 at 02:03:18AM -0400, Corey Osgood wrote:
@@ -79,11 +62,14 @@
loops = 0; /* Yes, this is a mess, but it's the easiest way to do it. */
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops < SMBUS_TIMEOUT)
Rationale? Does it make a big difference?
No, not really. Just that now the loop runs SMBUS_TIMEOUT times, instead if SMBUS_TIMEOUT + 1.
-u8 smbus_read_byte(u32 dimm, u32 offset) +/**
- Read a byte from the smbus
- @param dimm The address location of the dimm on the smbus
- @param offset The offset the data is located at
- */
+u8 smbus_read_byte(u8 dimm, u8 offset)
I'm still not entirely sure they're always only 8 bit. Do you have a pointer to a datasheet or spec or standard where it's explicitly defined as 8 bit? Yes, it _is_ 8 bits in most cases, but can we be sure that it'll be 8 bit in _all_ of them? On all chipsets and controllers?
The offset will always be 8 bit, since there are 256 offsets (and >half of them we'll never touch anyways). The address also has to be 8 bit, since it's programmed into an 8 bit register (per vt8237r datasheet, p125).
- /* Can I just "return inb(SMBHSTDAT0)"? */
- /* We could probably return inb(SMBHSTDAT0), but we'd lose the ability
* to debug the transaction */OK, if that's the only issue, just drop the comment.
+/**
- This is provided for compatibility, should it be needed
- */
+inline u8 spd_read_byte(u32 address, u8 offset) +{
- return smbus_read_byte(address, offset);
+}
Hm, this is usually done in auto.c per-mainboard, I think. Either you make spd_read_byte() a wrapper for smbus_read_byte(), or you use the "fake spd" method to return hard-coded settings if there's no real SPD data to be read.
Not sure this function makes sense in vt8237r_early_smbus.c, because of the above and also because it's not SMBus-related per se. I'd say drop it.
ok
Also, address is 32bit here but 8bit in smbus_read_byte()?
oops!
+/**
- A fixup for some systems that need time for the smbus to "warm up"
- It reads the ID byte from SMBus, looking for good data from a slot/address
- Exits on either good data or a timeout
Yep, but please extend the comment a bit to contain more information, rationale, example use case where this issue came up, how the problem shows, how it's fixed etc. The comment is good, but a bit too short for describing this non-trivial issue at hand.
ok
- @param mem_controller The memory controller and smbus addresses
- */
+void smbus_fixup(const struct mem_controller *ctrl) +{
- int i, ram_slots, current_slot = 0;
- u8 result = 0;
+#ifdef DIMM_SOCKETS
- ram_slots = DIMM_SOCKETS;
+#else
- ram_slots = sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0]);
+#endif
Can you explain? Shouldn't DIMM_SOCKETS always match sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0])? When does it happen that they do not match (and why?).
Unless someone doesn't define DIMM_SOCKETS, or uses some other name. I suppose that could be just dropped in favor of ARRAY_SIZE().
Also, we now have ARRAY_SIZE(), please use it here.
- if (!ram_slots) {
print_err("smbus_fixup thinks there are no ram slots!\r\n");return;- }
- PRINT_DEBUG("Waiting for smbus to warm up");
- /* Bad SPD data should be either 0 or 0xff, so really the values we look
* for are arbitrary, as long as they're between 1 and 0xfe */- for(i = 0; (i < SMBUS_TIMEOUT && ((result < SPD_MEMORY_TYPE_SDRAM) ||
(result > SPD_MEMORY_TYPE_SDRAM_DDR2))); i++)Please explain the SPD_MEMORY_TYPE_SDRAM/SPD_MEMORY_TYPE_SDRAM_DDR2 check in the comment here.
If all you want is to know whether some sensible RAM type is returned wouldn't "> 0" and "< 0xff" be enough (as per your comment)? You don't really care about the exact type, you only want to know _if_ there's a DIMM here, correct?
Safety's sake. If the smbus happens to spurt out some odd value (I've seen 0x30 once) while this is going on, we want to be sure it's really valid data. Originally it only sought DDR2, but that's bad since this southbridge can be used on DDR systems as well. Looking further though, it's only DDR/DDR2, so the SDRAM bit could be dropped.
If I read this correctly you're checking whether you get one of these?
#define SPD_MEMORY_TYPE_SDRAM 4 #define SPD_MEMORY_TYPE_MULTIPLEXED_ROM 5 #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8
If we make this "> 0" and "< 0xff" ("< 10" or so should be enough, too) then this function might be usable on non-vt8237r chipsets and can go in some global SMBus file to be used by others?
Perhaps. vt8231/8235 could use something similar, they just use a big delay as of right now.
- {
if (current_slot > ram_slots) j = 0;result = spd_read_byte(ctrl->channel0[current_slot],SPD_MEMORY_TYPE);current_slot++;PRINT_DEBUG(".");- }
- if (i >= SMBUS_TIMEOUT) print_err("SMBus timed out while warming up\r\n");
- else PRINT_DEBUG("Done\r\n");
+}
Looks good otherwise. Does this contain all of the changes required to make it work on your board _and_ Rudolf's board?
These are only required for my board, Rudolf's works fine without it. I don't think it will break Rudolf's.
Thanks, Uwe.
Updated patch soon :)
-Corey
* Corey Osgood corey.osgood@gmail.com [071102 17:39]:
+/**
- This is provided for compatibility, should it be needed
- */
+inline u8 spd_read_byte(u32 address, u8 offset) +{
- return smbus_read_byte(address, offset);
+}
Also, address is 32bit here but 8bit in smbus_read_byte()?
oops!
That's the whole point behind the abstraction.
Stefan
On Fri, Nov 02, 2007 at 12:39:16PM -0400, Corey Osgood wrote:
/* Yes, this is a mess, but it's the easiest way to do it. */
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops < SMBUS_TIMEOUT)
Rationale? Does it make a big difference?
No, not really. Just that now the loop runs SMBUS_TIMEOUT times, instead if SMBUS_TIMEOUT + 1.
OK, so it's more a cosmetical issue, no real code/functionality changes?
The offset will always be 8 bit, since there are 256 offsets (and >half of them we'll never touch anyways). The address also has to be 8 bit, since it's programmed into an 8 bit register (per vt8237r datasheet, p125).
Yep, u8 for both is fine then.
+#ifdef DIMM_SOCKETS
- ram_slots = DIMM_SOCKETS;
+#else
- ram_slots = sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0]);
+#endif
Can you explain? Shouldn't DIMM_SOCKETS always match sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0])? When does it happen that they do not match (and why?).
Unless someone doesn't define DIMM_SOCKETS, or uses some other name. I suppose that could be just dropped in favor of ARRAY_SIZE().
Yes, drop the DIMM_SOCKETS part and dependency IMO.
ARRAY_SIZE(ctrl->channel0);
should do (if there's only one channel).
Please explain the SPD_MEMORY_TYPE_SDRAM/SPD_MEMORY_TYPE_SDRAM_DDR2 check in the comment here.
If all you want is to know whether some sensible RAM type is returned wouldn't "> 0" and "< 0xff" be enough (as per your comment)? You don't really care about the exact type, you only want to know _if_ there's a DIMM here, correct?
Safety's sake. If the smbus happens to spurt out some odd value (I've seen 0x30 once) while this is going on, we want to be sure it's really
OK, but how do we know the odd values can never be e.g. 8 (which is valid) in some cases? In such a scenario this code wouldn't work?
valid data. Originally it only sought DDR2, but that's bad since this southbridge can be used on DDR systems as well. Looking further though, it's only DDR/DDR2, so the SDRAM bit could be dropped.
If I read this correctly you're checking whether you get one of these?
#define SPD_MEMORY_TYPE_SDRAM 4 #define SPD_MEMORY_TYPE_MULTIPLEXED_ROM 5 #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8
If we make this "> 0" and "< 0xff" ("< 10" or so should be enough, too) then this function might be usable on non-vt8237r chipsets and can go in some global SMBus file to be used by others?
Perhaps. vt8231/8235 could use something similar, they just use a big delay as of right now.
I'd rather match all legit RAM types (1-8 or so) and make it a function which can be used by every chip (not only vt*). Think EDO, DDR3, whatever.
Uwe.
Updated patch attached.
Signed-off-by: Corey Osgood corey.osgood@gmail.com
Comments inline below.
Uwe Hermann wrote:
On Fri, Nov 02, 2007 at 12:39:16PM -0400, Corey Osgood wrote:
Please explain the SPD_MEMORY_TYPE_SDRAM/SPD_MEMORY_TYPE_SDRAM_DDR2 check in the comment here.
If all you want is to know whether some sensible RAM type is returned wouldn't "> 0" and "< 0xff" be enough (as per your comment)? You don't really care about the exact type, you only want to know _if_ there's a DIMM here, correct?
Safety's sake. If the smbus happens to spurt out some odd value (I've seen 0x30 once) while this is going on, we want to be sure it's really
OK, but how do we know the odd values can never be e.g. 8 (which is valid) in some cases? In such a scenario this code wouldn't work?
Yes, but as I said, once, and this has run a lot of times. And IIRC it was the last cycle before valid data was returned. It would be very rare for this to fail, IMO (although the more dram types we add, the more likely it is to fail).
valid data. Originally it only sought DDR2, but that's bad since this southbridge can be used on DDR systems as well. Looking further though, it's only DDR/DDR2, so the SDRAM bit could be dropped.
If I read this correctly you're checking whether you get one of these?
#define SPD_MEMORY_TYPE_SDRAM 4 #define SPD_MEMORY_TYPE_MULTIPLEXED_ROM 5 #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8
If we make this "> 0" and "< 0xff" ("< 10" or so should be enough, too) then this function might be usable on non-vt8237r chipsets and can go in some global SMBus file to be used by others?
Perhaps. vt8231/8235 could use something similar, they just use a big delay as of right now.
I'd rather match all legit RAM types (1-8 or so) and make it a function which can be used by every chip (not only vt*). Think EDO, DDR3, whatever.
EDO I don't think we really need to support, but if someone needs it in the future, they can change it easily enough. I've added DDR3 (0xb according to micron), just in case. I've left the function in vt8237r because there's no file that it would really fit into right now. If someone else wants/needs to use it in the future, it should be moved.
On Fri, Nov 02, 2007 at 01:30:25PM -0400, Corey Osgood wrote:
+void smbus_fixup(const struct mem_controller *ctrl) +{
- int i, ram_slots, current_slot = 0;
- u8 result = 0;
- ram_slots = ARRAY_SIZE(ctrl->channel0);
- if (!ram_slots) {
print_err("smbus_fixup thinks there are no ram slots!\r\n");return;- }
- PRINT_DEBUG("Waiting for smbus to warm up");
- /* Bad SPD data should be either 0 or 0xff, but YMMV. So we look for the
* ID bytes of SDRAM, DDR, DDR2, and DDR3 (and anything in between).* vt8237r has only been seen on DDR and DDR2 based systems, so far */- for(i = 0; (i < SMBUS_TIMEOUT && ((result < SPD_MEMORY_TYPE_SDRAM) ||
(result > SPD_MEMORY_TYPE_SDRAM_DDR3))); i++)- {
if (current_slot > ram_slots) j = 0;
What is j here?
+++ src/include/spd.h (working copy) @@ -105,6 +105,7 @@ #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8 +#define SPD_MEMORY_TYPE_SDRAM_DDR3 0xb
Looks like bad whitespace, but that's a separate patch.
Otherwise I'll ack. Can you commit?
//Peter
Peter Stuge wrote:
On Fri, Nov 02, 2007 at 01:30:25PM -0400, Corey Osgood wrote:
+void smbus_fixup(const struct mem_controller *ctrl) +{
- int i, ram_slots, current_slot = 0;
- u8 result = 0;
- ram_slots = ARRAY_SIZE(ctrl->channel0);
- if (!ram_slots) {
print_err("smbus_fixup thinks there are no ram slots!\r\n");return;- }
- PRINT_DEBUG("Waiting for smbus to warm up");
- /* Bad SPD data should be either 0 or 0xff, but YMMV. So we look for the
* ID bytes of SDRAM, DDR, DDR2, and DDR3 (and anything in between).* vt8237r has only been seen on DDR and DDR2 based systems, so far */- for(i = 0; (i < SMBUS_TIMEOUT && ((result < SPD_MEMORY_TYPE_SDRAM) ||
(result > SPD_MEMORY_TYPE_SDRAM_DDR3))); i++)- {
if (current_slot > ram_slots) j = 0;What is j here?
Oops, should have been current_slot. I can fix it before commit.
+++ src/include/spd.h (working copy) @@ -105,6 +105,7 @@ #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8 +#define SPD_MEMORY_TYPE_SDRAM_DDR3 0xb
Looks like bad whitespace, but that's a separate patch.
Weird, it looks fine in the file.
Otherwise I'll ack. Can you commit?
Yep, just gotta figure out how. I'll hop on IRC if I have a problem.
-Corey
On Fri, 2 Nov 2007, Corey Osgood wrote:
Peter Stuge wrote:
On Fri, Nov 02, 2007 at 01:30:25PM -0400, Corey Osgood wrote:
+++ src/include/spd.h (working copy) @@ -105,6 +105,7 @@ #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8 +#define SPD_MEMORY_TYPE_SDRAM_DDR3 0xb
Looks like bad whitespace, but that's a separate patch.
Weird, it looks fine in the file.
The 4th #define line has a tab chr right after the DDR3 while the first 3 #define lines have only space chrs.
Russ
Russell Whitaker wrote:
On Fri, 2 Nov 2007, Corey Osgood wrote:
Peter Stuge wrote:
On Fri, Nov 02, 2007 at 01:30:25PM -0400, Corey Osgood wrote:
+++ src/include/spd.h (working copy) @@ -105,6 +105,7 @@ #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8 +#define SPD_MEMORY_TYPE_SDRAM_DDR3 0xb
Looks like bad whitespace, but that's a separate patch.
Weird, it looks fine in the file.
The 4th #define line has a tab chr right after the DDR3 while the first 3 #define lines have only space chrs.
Russ
Which explains it perfectly. Fixed now, thanks.
-Corey
On Fri, Nov 02, 2007 at 01:30:25PM -0400, Corey Osgood wrote:
Signed-off-by: Corey Osgood corey.osgood@gmail.com
according to micron), just in case. I've left the function in vt8237r because there's no file that it would really fit into right now. If someone else wants/needs to use it in the future, it should be moved.
Yeah, we can move it later.
For the latest version with Peter's suggestions: Acked-by: Uwe Hermann uwe@hermann-uwe.de
There are some cosmetics to be fixed still, but I'll do than in a follow-up commit.
Uwe.
Uwe Hermann wrote:
On Fri, Nov 02, 2007 at 01:30:25PM -0400, Corey Osgood wrote:
Signed-off-by: Corey Osgood corey.osgood@gmail.com
according to micron), just in case. I've left the function in vt8237r because there's no file that it would really fit into right now. If someone else wants/needs to use it in the future, it should be moved.
Yeah, we can move it later.
For the latest version with Peter's suggestions: Acked-by: Uwe Hermann uwe@hermann-uwe.de
Thanks, r2937, and I didn't even break anything!
-Corey
* Uwe Hermann uwe@hermann-uwe.de [071102 16:56]:
loops = 0; /* Yes, this is a mess, but it's the easiest way to do it. */
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops < SMBUS_TIMEOUT)
Rationale? Does it make a big difference?
Yes, the one is correct, the other is not because it counts SMBUS_TIMEOUT+1 loops.
-u8 smbus_read_byte(u32 dimm, u32 offset) +/**
- Read a byte from the smbus
- @param dimm The address location of the dimm on the smbus
- @param offset The offset the data is located at
- */
+u8 smbus_read_byte(u8 dimm, u8 offset)
I'm still not entirely sure they're always only 8 bit. Do you have a pointer to a datasheet or spec or standard where it's explicitly defined as 8 bit? Yes, it _is_ 8 bits in most cases, but can we be sure that it'll be 8 bit in _all_ of them? On all chipsets and controllers?
smbus address space is 8 bits. Let's reflect this in the code. We can think about this when we find the first machine where it is not.
+inline u8 spd_read_byte(u32 address, u8 offset) +{
- return smbus_read_byte(address, offset);
+}
Hm, this is usually done in auto.c per-mainboard, I think. Either you make spd_read_byte() a wrapper for smbus_read_byte(), or you use the "fake spd" method to return hard-coded settings if there's no real SPD data to be read.
Yes, this is a mainboard specific abstraction. It might have to circumvent smbus switches on the bus.
Not sure this function makes sense in vt8237r_early_smbus.c, because of the above and also because it's not SMBus-related per se. I'd say drop it.
Also, address is 32bit here but 8bit in smbus_read_byte()?
That's fine. The i2c has 8 (actually 7) bits address space. spd might not necessarily be behind the i2c bus. Or it might have a number of switches. So you might have an address of 0xaabbccdd saying that aa, bb, cc, dd are the settings of 3 i2c switches, while dd is the actual address of the spd on the bus _after_ doing the other settings. This is done on the agámi aruma in a similar way.
Stefan
On Fri, Nov 02, 2007 at 05:44:03PM +0100, Stefan Reinauer wrote:
-u8 smbus_read_byte(u32 dimm, u32 offset) +/**
- Read a byte from the smbus
- @param dimm The address location of the dimm on the smbus
- @param offset The offset the data is located at
- */
+u8 smbus_read_byte(u8 dimm, u8 offset)
I'm still not entirely sure they're always only 8 bit. Do you have a pointer to a datasheet or spec or standard where it's explicitly defined as 8 bit? Yes, it _is_ 8 bits in most cases, but can we be sure that it'll be 8 bit in _all_ of them? On all chipsets and controllers?
smbus address space is 8 bits. Let's reflect this in the code. We can
ACK.
think about this when we find the first machine where it is not.
I don't know if there are any, I was just speculating. If SMBus address space is specified as 8 bits u8 is the way to go.
+inline u8 spd_read_byte(u32 address, u8 offset) +{
- return smbus_read_byte(address, offset);
+}
Hm, this is usually done in auto.c per-mainboard, I think. Either you make spd_read_byte() a wrapper for smbus_read_byte(), or you use the "fake spd" method to return hard-coded settings if there's no real SPD data to be read.
Yes, this is a mainboard specific abstraction. It might have to circumvent smbus switches on the bus.
OK, should be dropped here then.
Not sure this function makes sense in vt8237r_early_smbus.c, because of the above and also because it's not SMBus-related per se. I'd say drop it.
Also, address is 32bit here but 8bit in smbus_read_byte()?
That's fine. The i2c has 8 (actually 7) bits address space. spd might not necessarily be behind the i2c bus. Or it might have a number of switches. So you might have an address of 0xaabbccdd saying that aa, bb, cc, dd are the settings of 3 i2c switches, while dd is the actual address of the spd on the bus _after_ doing the other settings. This is done on the agámi aruma in a similar way.
Thanks for the clarifications! For this patch that's not relevant though as spd_read_byte() should be dropped anyway then as it's board specific.
But maybe we should have this info somewhere in the wiki? http://linuxbios.org/Developer_Manual ?
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [071102 18:08]:
That's fine. The i2c has 8 (actually 7) bits address space. spd might not necessarily be behind the i2c bus. Or it might have a number of switches. So you might have an address of 0xaabbccdd saying that aa, bb, cc, dd are the settings of 3 i2c switches, while dd is the actual address of the spd on the bus _after_ doing the other settings. This is done on the agámi aruma in a similar way.
Thanks for the clarifications! For this patch that's not relevant though as spd_read_byte() should be dropped anyway then as it's board specific.
But maybe we should have this info somewhere in the wiki? http://linuxbios.org/Developer_Manual ?
Sounds reasonable
-
Corey Osgood -
Peter Stuge -
Rudolf Marek -
Russell Whitaker -
Stefan Reinauer -
Uwe Hermann