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.
--
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
signature.asc
Description: Digital signature
-- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios
