[coreboot-gerrit] [XL] Change in coreboot[main]: nb/via/cx700: Implement raminit

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.

Angel Pons has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/82770?usp=email )

Change subject: nb/via/cx700: Implement raminit
......................................................................

Patch Set 2: Code-Review+2

(7 comments)

File src/northbridge/via/cx700/chip.h:

https://review.coreboot.org/c/coreboot/+/82770/comment/75b5bf43_9166ea06?usp... :
PS2, Line 15: mem_clocks
At first, I read this as "mem**e**_clocks"...

File src/northbridge/via/cx700/raminit.h:

https://review.coreboot.org/c/coreboot/+/82770/comment/73b04f18_8838ff72?usp... :
PS2, Line 7: void sdram_enable(const struct dram_cfg *);
Should be easy to fix.

File src/northbridge/via/cx700/raminit.c:

https://review.coreboot.org/c/coreboot/+/82770/comment/c2a3dc95_fe3a2980?usp... :
PS2, Line 79: #define	GET_SPD(i, val, tmp, reg)							\
Why is this not a function?

https://review.coreboot.org/c/coreboot/+/82770/comment/afe4620a_dd711f7f?usp... :
PS2, Line 213: ((NA_ODT << 6)    | (NA_ODT << 4)    | (NA_ODT << 2)    | Rank0_ODT)
A helper macro to factor out the shifts would probably make this table a bit easier to read. No big deal though.

https://review.coreboot.org/c/coreboot/+/82770/comment/677b1fb1_3676bf15?usp... :
PS2, Line 276: 	
Is this a tab?
```suggestion
/* Chipset Performance UP and other setting after DRAM Sizing Registers */
```

https://review.coreboot.org/c/coreboot/+/82770/comment/d7ed441c_c03119c0?usp... :
PS2, Line 361: 	/* FIXME: Only supports 2 ranks per DIMM */
Can there be more?

https://review.coreboot.org/c/coreboot/+/82770/comment/0b958aa0_34c76a64?usp... :
PS2, Line 1274: 
              : 				write32p(0, 0x55555555);
              : 				write32p(4, 0x55555555);
              : 				udelay(15);
              : 				if (read32p(0) != 0x55555555)
              : 					break;
              : 				if (read32p(4) != 0x55555555)
              : 					break;
              : 				write32p(0, 0xaaaaaaaa);
              : 				write32p(4, 0xaaaaaaaa);
              : 				udelay(15);
              : 				if (read32p(0) != 0xaaaaaaaa)
              : 					break;
              : 				if (read32p(4) != 0xaaaaaaaa)
              : 					break;
Could be factored out into a helper function:

```
bool test_ram_rw(u32 val)
{
    write32p(0, val);
    write32p(4, val);
    udelay(15);
    return read32p(0) == val && read32p(4) == val;
}
```