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