[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.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071102/8205f508/attachment.sig>