[coreboot] v2[PATCH]RCA RM4100 i82830 support
Corey Osgood
corey.osgood at gmail.com
Tue Feb 26 04:18:01 CET 2008
On Mon, 2008-02-25 at 21:20 -0500, joe at smittys.pointclark.net wrote:
> Quoting Peter Stuge <peter at stuge.se>:
>
> > On Mon, Feb 25, 2008 at 08:51:00PM -0500, Corey Osgood wrote:
> >> without the ability to try other memory sticks or with real spd
> >> data, it might look right on the rm4100, but fail in practice.
> >
> > The problem is that this is in (supposedly) generic 830 code.
> >
> It pretty much is. First of all PC133 SO-DIMMS only come in cas3 or
> cas2 but setting DRT to cas3 is not hurting anything. So your memory
> only runs a tiny bit slower, an average user probably wouldn't notice
> the difference. But it does need to be done eventually (DRT). Besides
> DRT everything else is spd detectable. Did you check out function
> Corey and I came up with to get the memory size using spd 31. I still
> think that is pretty cool.
What he said ;)
Moving on:
> +static void spd_set_dram_size(const struct mem_controller *ctrl)
> +{
> + int i, value, drb1, drb2;
> +
> + for (i = 0; i < DIMM_SOCKETS; i++) {
> + struct dimm_size sz;
> + unsigned device;
> + device = ctrl->channel0[i];
> + drb1 = 0;
> + drb2 = 0;
> +
> + /* First check if a DIMM is actually present. */
> + if (spd_read_byte(device, 2) == 0x4) {
> + print_debug("Found DIMM in slot ");
> + print_debug_hex8(i);
> + print_debug("\r\n");
> +
> + sz = spd_get_dimm_size(device);
> +
> + /* WISHLIST: would be nice to display it as decimal? */
I saw a small bit of code to do this just a little while ago, if only I could remember where...
> + print_debug("DIMM is 0x");
> + print_debug_hex16(sz.side1);
> + print_debug(" on side 1\r\n");
> + print_debug("DIMM is 0x");
> + print_debug_hex16(sz.side2);
> + print_debug(" on side 2\r\n");
> +
> + /* The i82830 can't handle DIMMs smaller than 32MB per
> + * side or larger than 256MB per side. It also can
> + * only support a symmetrical dual-sided dimm.
> + */
> + if ((sz.side1 < 32)) {
> + print_err("DIMMs smaller than 32MB per side\r\n");
> + print_err("are not supported on this board\r\n");
board -> northbridge or chipset, in all of these.
> + die("HALT\r\n");
Why die?
> + }
> +
> + if ((sz.side2 != 0) && (sz.side2 < 32)) {
> + print_err("DIMMs smaller than 32MB per side\r\n");
> + print_err("are not supported on this board\r\n");
> + die("HALT\r\n");
> + }
> +
> + if ((sz.side1 > 256) || (sz.side2 > 256)) {
side1 should always be the larger side, and i830 doesn't support asymetrical dimms, so you can drop the side2 check.
> + print_err("DIMMs larger than 256MB per side\r\n");
> + print_err("are not supported on this board\r\n");
> + die("HALT\r\n");
> + }
> +
> + if ((sz.side2 != 0) && (sz.side1 != sz.side2)) {
> + print_err("This board only supports\r\n");
> + print_err("symmetrical dual-sided DIMMs\r\n");
> + die("HALT\r\n");
> + }
This should be moved right into the detection routine. If the northbridge can't handle it, then just die if sz.side1 |= sz.side2. The other option is to set sz.side2 to 0, and try to use it like a single-sided dimm.
> /* We need to divide size by 32 to set up the
> + * DRB registers.
> + */
> + if (sz.side1 > 0) {
if(sz.side1) works as well. unsigned can never be negative.
> + drb1 = sz.side1 >> 5;
Please don't use bitwise shifts for multiplication/division, same applies for other places.
> + }
> + if (sz.side2 > 0) {
> + drb2 = sz.side2 >> 5;
> + }
Please excuse me for jumping around, trying to keep this short:
>
> +static void do_ram_command(const struct mem_controller *ctrl, uint32_t command, uint32_t addr_offset)
> +{
> <snip>
> +
> + /* Read from (DIMM start address + addr_offset). */
> + read32(0 + addr_offset); //first offset is always 0
> +}
This isn't ready for multiple dimms yet. See the cn700 patch I recently sent (but haven't committed yet, I think it was acked).
I think that's everything, I'm hitting the sack.
-Coreu
More information about the coreboot
mailing list