Re: [LinuxBIOS] [PATCH] vt8237r fixes/improvements (LBv2)

[Uwe Hermann]

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.