Re: [coreboot] v2[PATCH]RCA RM4100 i82830 support

Hello,
I would like to submit my code for the RCA RM4100. Also code for the  
Intel i82830
Nothbridge, that works great. Everything works great except the  
ethernet and tv-out. By submitting the code maybe someone could help  
me figure out the ethernet problem? Anyways here it is:

Oops I forgot this:

Signed-off-by: Joseph Smith

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

Quoting [e-mail]:
> Hello,
> I would like to submit my code for the RCA RM4100. Also code for the
> Intel i82830 Nothbridge, that works great. Everything works great except the
> and tv-out.

Dis-reguard the last patch, this is the real one. A e100 kernel driver  
developer helped me figure out that the ethernet was an irq routing  
issue, fixed! It is working great now:-)))

Signed-off-by: Joseph Smith

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Peter Stuge]

On Mon, Feb 25, 2008 at 04:31:15PM -0500, [e-mail] wrote:
..
> > tv-out
>
> Dis-reguard the last patch, this is the real one. A e100 kernel driver  
> developer helped me figure out that the ethernet was an irq routing  
> issue, fixed! It is working great now:-)))

Good news!

Do you have any idea about the TV out?

Also see the comments below:
> +++ src/northbridge/intel/i82830/raminit.c	(revision 0)

..
> +static void set_dram_timing(const struct mem_controller *ctrl)
> +{
> /* Set the value for DRAM Timing Register */
> /* TODO: Configure the value according to SPD values. */
> pci_write_config32(ctrl->d0, DRT, 0x00000010);
>
> +}
>
> +static void set_dram_buffer_strength(const struct mem_controller *ctrl)
> +{
> /* TODO: This needs to be set according to the DRAM tech
> * (x8, x16, or x32). Argh, Intel provides no docs on this!
> * Currently, it needs to be pulled from the output of
> * lspci -xxx Rx92
> */
> /* Set the value for System Memory Buffer Strength Control Registers */
> pci_write_config32(ctrl->d0, BUFF_SC, 0xFC9B491B);
>
> +}

How about these two TODOs? In the second case I think the comment
refers to running the command under the factory BIOS - in that case
it would be good to clarify that.
> +static void sdram_enable(int controllers, const struct mem_controller *ctrl)
> +{

..
> /* 4. Mode register set. Wait two memory cycles. */
> /* TODO: Set offset according to DRT values */
> PRINT_DEBUG("RAM Enable 4: Mode register set\r\n");
> do_ram_command(ctrl, RAM_COMMAND_MRS, 0x1d0);

Can this be done?
> +++ targets/rca/rm4100/Config.lb	(revision 0)

..
> +romimage "fallback"
> option USE_FALLBACK_IMAGE = 1
> option FALLBACK_SIZE = ROM_SIZE
> option COREBOOT_EXTRA_VERSION = "_RM4100"
>
> +#	payload /etc/hosts

This last line makes no sense so I would remove it before the commit.

//Peter

v2[PATCH]RCA RM4100 i82830 support

[Corey Osgood]

On Tue, 2008-02-26 at 02:27 +0100, Peter Stuge wrote:
> On Mon, Feb 25, 2008 at 04:31:15PM -0500, [e-mail] wrote:
> ..
> > > tv-out
> >
> > Dis-reguard the last patch, this is the real one. A e100 kernel driver  
> > developer helped me figure out that the ethernet was an irq routing  
> > issue, fixed! It is working great now:-)))
>
> Good news!
>
> Do you have any idea about the TV out?
>
> Also see the comments below:
> > +++ src/northbridge/intel/i82830/raminit.c	(revision 0)
>
> ..
> > +static void set_dram_timing(const struct mem_controller *ctrl)
> > +{
> > /* Set the value for DRAM Timing Register */
> > /* TODO: Configure the value according to SPD values. */
> > pci_write_config32(ctrl->d0, DRT, 0x00000010);
> >
> > +}
> >
> > +static void set_dram_buffer_strength(const struct mem_controller *ctrl)
> > +{
> > /* TODO: This needs to be set according to the DRAM tech
> > * (x8, x16, or x32). Argh, Intel provides no docs on this!
> > * Currently, it needs to be pulled from the output of
> > * lspci -xxx Rx92
> > */
> > /* Set the value for System Memory Buffer Strength Control Registers */
> > pci_write_config32(ctrl->d0, BUFF_SC, 0xFC9B491B);
> >
> > +}
>
> How about these two TODOs? In the second case I think the comment
> refers to running the command under the factory BIOS - in that case
> it would be good to clarify that.

The memory on the rm4100 is onboard. There's a header for a memory slot,
but there's more unknown hardware missing to hook that up. The only
version with memory slots is the Thomson IP1001 (somewhat rare,
essentially the same board with a different branding), and perhaps some
internal RCA prototypes. This info is courtesy of Joe and his site ;)

That said, the BUFF_SC registers are the same deal as on the i810, the
explanation in the datasheet isn't specific or clear about how to set
them. The rest of the registers are fairly simple to set, but without
the ability to try other memory sticks or with real spd data, it might
look right on the rm4100, but fail in practice.
> > +static void sdram_enable(int controllers, const struct mem_controller *ctrl)
> > +{
>
> ..
> > /* 4. Mode register set. Wait two memory cycles. */
> > /* TODO: Set offset according to DRT values */
> > PRINT_DEBUG("RAM Enable 4: Mode register set\r\n");
> > do_ram_command(ctrl, RAM_COMMAND_MRS, 0x1d0);
>
> Can this be done?

Same deal as above. I can put together another patch later, and put the
code in ifdefs with a note to that effect.

-Corey
> > +++ targets/rca/rm4100/Config.lb	(revision 0)
>
> ..
> > +romimage "fallback"
> > option USE_FALLBACK_IMAGE = 1
> > option FALLBACK_SIZE = ROM_SIZE
> > option COREBOOT_EXTRA_VERSION = "_RM4100"
> >
> > +#	payload /etc/hosts
>
> This last line makes no sense so I would remove it before the commit.
>
> //Peter

v2[PATCH]RCA RM4100 i82830 support

[Peter Stuge]

On Mon, Feb 25, 2008 at 08:51:00PM -0500, Corey Osgood wrote:
> > > /* Set the value for DRAM Timing Register */
> > > /* TODO: Configure the value according to SPD values. */
> > > pci_write_config32(ctrl->d0, DRT, 0x00000010);
> >
> > How about these two TODOs?
>
> The memory on the rm4100 is onboard.

I guess this port could use the SPD emulation a la ALIX then?
> The only version with memory slots is the Thomson IP1001

Perhaps another port for that, then?
> 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.
> > > /* TODO: Set offset according to DRT values */
> > > PRINT_DEBUG("RAM Enable 4: Mode register set\r\n");
> > > do_ram_command(ctrl, RAM_COMMAND_MRS, 0x1d0);
> >
> > Can this be done?
>
> Same deal as above.

Yes, exactly.
> I can put together another patch later, and put the code in ifdefs
> with a note to that effect.

Hm? ifdef what?

//Peter

v2[PATCH]RCA RM4100 i82830 support

Quoting Peter Stuge :
> On Mon, Feb 25, 2008 at 08:51:00PM -0500, Corey Osgood wrote:
> > > > /* Set the value for DRAM Timing Register */
> > > > /* TODO: Configure the value according to SPD values. */
> > > > pci_write_config32(ctrl->d0, DRT, 0x00000010);
> > >
> > > How about these two TODOs?
> >
> > The memory on the rm4100 is onboard.
>
> I guess this port could use the SPD emulation a la ALIX then?

possible? Just a fake spd_table setup for now.
> > The only version with memory slots is the Thomson IP1001
>
> Perhaps another port for that, then?

Possible if you could find one. And trust me the people that do have  
them are not willing to share:-(
> > 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.
> > > > /* TODO: Set offset according to DRT values */
> > > > PRINT_DEBUG("RAM Enable 4: Mode register set\r\n");
> > > > do_ram_command(ctrl, RAM_COMMAND_MRS, 0x1d0);
> > >
> > > Can this be done?
> >
> > Same deal as above.
>
> Yes, exactly.
> > I can put together another patch later, and put the code in ifdefs
> > with a note to that effect.
>
> Hm? ifdef what?
>
> //Peter

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Corey Osgood]

On Mon, 2008-02-25 at 21:20 -0500, [e-mail] wrote:
> Quoting Peter Stuge :
> > 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

v2[PATCH]RCA RM4100 i82830 support

[Peter Stuge]

On Mon, Feb 25, 2008 at 10:18:01PM -0500, Corey Osgood wrote:
> > 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.

Degrading like this with a very loud message would be a very cool
feature.
> > 		drb1 = sz.side1 >> 5;
>
> Please don't use bitwise shifts for multiplication/division, same
> applies for other places.

+1

//Peter

v2[PATCH]RCA RM4100 i82830 support

Quoting Corey Osgood :
> On Mon, 2008-02-25 at 21:20 -0500, [e-mail] wrote:
> > Quoting Peter Stuge :
> > > 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...

That would be cool.
> > 	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.

Not sure what you mean here.
> > 		die("HALT\r\n");
>
> Why die?

Die becaause the memory is not supported, right?
> > 	}
> > 	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.

ok will change
> > 		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.

Hmm above is saying that IF THERE IS A SECOND SIDE and it does not  
equal the first side than die, otherwise it is assumed it is single  
sided and moves on.

"if sz.side1 |= sz.side2" does not work for single sided so-dimms, correct?
> > /* 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.

ok will change
> > 		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:

ok will change, but there was quite a discussion about this a while  
ago and half thought it was ok and half didn't. So which is correct in  
"C" standards?
> > 
> > +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).

ok will do.
> I think that's everything, I'm hitting the sack.
>
> -Coreu

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Peter Stuge]

On Mon, Feb 25, 2008 at 11:12:58PM -0500, [e-mail] wrote:
> > > print_err("are not supported on this board\r\n");
> >
> > board -> northbridge or chipset, in all of these.
>
> Not sure what you mean here.

Change the word "board" to either "northbridge" or "chipset" because
this isn't board-specific.
> > > 		die("HALT\r\n");
> >
> > Why die?
>
> Die becaause the memory is not supported, right?

Maybe it is possible to degrade and carry on?
> > > 	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.
>
> Hmm above is saying that IF THERE IS A SECOND SIDE and it does not  
> equal the first side than die, otherwise it is assumed it is single  
> sided and moves on.
>
> "if sz.side1 |= sz.side2" does not work for single sided so-dimms,
> correct?

Yes. I think that was a typo, it should probably have been != instead.
> > > 		drb1 = sz.side1 >> 5;
> >
> > Please don't use bitwise shifts for multiplication/division,
>
> ok will change, but there was quite a discussion about this a while
> ago and half thought it was ok and half didn't. So which is correct
> in "C" standards?

I say depends on the case. If the algorithm is mathematical I say use
/ and * but if it is primarily about moving bits then shift is fine.

//Peter

v2[PATCH]RCA RM4100 i82830 support

> > > 	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.

ok how about this. Below is saying that IF THERE IS A SECOND SIDE and  
it does not equal the first side than give a message, use as a single  
sided dimm, and move on.

/* The i82830 only supports a symmetrical dual-sided dimm
    		 * and can't handle DIMMs smaller than 32MB per
    		 * side or larger than 256MB per side.
    		 */
    		if ((sz.side2 != 0) && (sz.side1 != sz.side2)) {
    			print_err("This northbridge only supports\r\n");
    			print_err("symmetrical dual-sided DIMMs\r\n");
    			print_err("booting as a single-sided DIMM\r\n");
    			sz.side2 = 0;
    		}
    		if ((sz.side1 < 32)) {
    			print_err("DIMMs smaller than 32MB per side\r\n");
    			print_err("are not supported on this northbridge\r\n");
    			die("HALT\r\n");
    		}

if ((sz.side1 > 256)) {
    			print_err("DIMMs larger than 256MB per side\r\n");
    			print_err("are not supported on this northbridge\r\n");
    			die("HALT\r\n");
    		}

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Peter Stuge]

On Tue, Feb 26, 2008 at 10:04:59AM -0500, [e-mail] wrote:
> ok how about this.

I like it!

//Peter

v2[PATCH]RCA RM4100 i82830 support

> > +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).

Oh you mean this? What do I need to do to adapt it for the i82830?

+	/* NOTE: Dual-sided ready */
    read32(0 + addr_offset);
+	for(i = 0; i < (ARRAY_SIZE(ctrl->channel0) * 2); i++) {
+		reg8 = pci_read_config8(ctrl->d0f3, 0x40 + i);
+		if(reg8) read32((reg8 << 26) + addr_offset);
+	}

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Corey Osgood]

On Tue, 2008-02-26 at 16:19 -0500, [e-mail] wrote:
> > > +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).
>
> Oh you mean this? What do I need to do to adapt it for the i82830?
> /* NOTE: Dual-sided ready */
> read32(0 + addr_offset);
> for(i = 0; i < (ARRAY_SIZE(ctrl->channel0) * 2); i++) {
> reg8 = pci_read_config8(ctrl->d0f3, 0x40 + i);
> if(reg8) read32((reg8 << 26) + addr_offset);
> }

This reads from register 0x40 + i, where 0x40 is the top of the first
dimm, and i counts to the max number of dimms. Reg 0x40 contains bits
33:26 of the top address, hence the << 26. So you'd need to adjust both
of these to fit the i830's DRB mechanism. If the i830 uses an ugly
format like the i810 does, then it may be easier to store the size in
scratch registers somewhere after it's calculated, and then use those
values instead. Sorry I can't get more specific, I've got to head out to
my next service call.

-Corey

v2[PATCH]RCA RM4100 i82830 support

Quoting Corey Osgood :
> On Tue, 2008-02-26 at 16:19 -0500, [e-mail] wrote:
> > > > +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).
> >
> > Oh you mean this? What do I need to do to adapt it for the i82830?
> > /* NOTE: Dual-sided ready */
> > read32(0 + addr_offset);
> > for(i = 0; i < (ARRAY_SIZE(ctrl->channel0) * 2); i++) {
> > reg8 = pci_read_config8(ctrl->d0f3, 0x40 + i);
> > if(reg8) read32((reg8 << 26) + addr_offset);
> > }
>
> This reads from register 0x40 + i, where 0x40 is the top of the first
> dimm, and i counts to the max number of dimms.

Why read this from a register?? Why not just use (i = 0; i <  
DIMM_SOCKETS; i++)
> Reg 0x40 contains bits
> 33:26 of the top address, hence the << 26. So you'd need to adjust both
> of these to fit the i830's DRB mechanism. If the i830 uses an ugly
> format like the i810 does, then it may be easier to store the size in
> scratch registers somewhere after it's calculated, and then use those
> values instead. Sorry I can't get more specific, I've got to head out to
> my next service call.

The DRB i830 is not ugly it is very simple, simple, simple. So simple  
that it just calculates the memory size in ticks of 32 per side * the  
next side up to 4 sides. Like this:

Example: 128MB SINGLE sided so-dimm in slot 1
          128MB DOUBLE sided so-dimm in slot 2

There are 4 DRB registers(actually 6 but but only 4 that are usable -  
Intel design glitch), 1 for each side. So in this example they would  
look like this:

DRB1 = 0x04
DRB2 = 0x04
DRB3 = 0x06
DRB4 = 0x08

So if we needed to convert this we would do something like this (DRB *  
32) * 1024 correct? But if we just need to calculate the end of  
so-dimm #1 and the start of so-dimm #2 the only important register  
would be DRB2 (side 2 of so-dimm #1) and DRB3(side 1 of so-dimm #2),  
correct?

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

Quoting [e-mail]:
> Quoting Corey Osgood :
> > On Tue, 2008-02-26 at 16:19 -0500, [e-mail] wrote:
> > > > > +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).
> > >
> > > Oh you mean this? What do I need to do to adapt it for the i82830?
> > > /* NOTE: Dual-sided ready */
> > > read32(0 + addr_offset);
> > > for(i = 0; i < (ARRAY_SIZE(ctrl->channel0) * 2); i++) {
> > > reg8 = pci_read_config8(ctrl->d0f3, 0x40 + i);
> > > if(reg8) read32((reg8 << 26) + addr_offset);
> > > }
> >
> > This reads from register 0x40 + i, where 0x40 is the top of the first
> > dimm, and i counts to the max number of dimms.
>
> Why read this from a register?? Why not just use (i = 0; i <
> DIMM_SOCKETS; i++)
> > Reg 0x40 contains bits
> > 33:26 of the top address, hence the << 26. So you'd need to adjust both
> > of these to fit the i830's DRB mechanism. If the i830 uses an ugly
> > format like the i810 does, then it may be easier to store the size in
> > scratch registers somewhere after it's calculated, and then use those
> > values instead. Sorry I can't get more specific, I've got to head out to
> > my next service call.
>
> The DRB i830 is not ugly it is very simple, simple, simple. So simple
> that it just calculates the memory size in ticks of 32 per side

(Sorry typo I meant) +
> the next side up to 4 sides. Like this:
>
> Example: 128MB SINGLE sided so-dimm in slot 1
>           128MB DOUBLE sided so-dimm in slot 2
>
> There are 4 DRB registers(actually 6 but but only 4 that are usable -
> Intel design glitch), 1 for each side. So in this example they would
> look like this:
>
> DRB1 = 0x04
> DRB2 = 0x04
> DRB3 = 0x06
> DRB4 = 0x08
>
> So if we needed to convert this we would do something like this (DRB *
> 32) * 1024 correct? But if we just need to calculate the end of
> so-dimm #1 and the start of so-dimm #2 the only important register
> would be DRB2 (side 2 of so-dimm #1) and DRB3(side 1 of so-dimm #2),
> correct?
>
> Thanks - Joe

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Corey Osgood]

On Tue, 2008-02-26 at 21:33 -0500, [e-mail] wrote:
> Quoting [e-mail]:
> > Quoting Corey Osgood :
> > > On Tue, 2008-02-26 at 16:19 -0500, [e-mail] wrote:
> > > > > > +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).
> > > >
> > > > Oh you mean this? What do I need to do to adapt it for the i82830?
> > > > /* NOTE: Dual-sided ready */
> > > > read32(0 + addr_offset);
> > > > for(i = 0; i < (ARRAY_SIZE(ctrl->channel0) * 2); i++) {
> > > > reg8 = pci_read_config8(ctrl->d0f3, 0x40 + i);
> > > > if(reg8) read32((reg8 << 26) + addr_offset);
> > > > }
> > >
> > > This reads from register 0x40 + i, where 0x40 is the top of the first
> > > dimm, and i counts to the max number of dimms.
> >
> > Why read this from a register?? Why not just use (i = 0; i <
> > DIMM_SOCKETS; i++)

ARRAY_SIZE(ctrl->channel0) is the number of spd addresses, which can be
less than the max number of dimms.
> > > Reg 0x40 contains bits
> > > 33:26 of the top address, hence the << 26. So you'd need to adjust both
> > > of these to fit the i830's DRB mechanism. If the i830 uses an ugly
> > > format like the i810 does, then it may be easier to store the size in
> > > scratch registers somewhere after it's calculated, and then use those
> > > values instead. Sorry I can't get more specific, I've got to head out to
> > > my next service call.
> >
> > The DRB i830 is not ugly it is very simple, simple, simple. So simple
> > that it just calculates the memory size in ticks of 32 per side
>
> (Sorry typo I meant) +
> > the next side up to 4 sides. Like this:
> >
> > Example: 128MB SINGLE sided so-dimm in slot 1
> >           128MB DOUBLE sided so-dimm in slot 2
> >
> > There are 4 DRB registers(actually 6 but but only 4 that are usable -
> > Intel design glitch), 1 for each side. So in this example they would
> > look like this:
> >
> > DRB1 = 0x04
> > DRB2 = 0x04
> > DRB3 = 0x06
> > DRB4 = 0x08
> >
> > So if we needed to convert this we would do something like this (DRB *
> > 32) * 1024 correct? But if we just need to calculate the end of
> > so-dimm #1 and the start of so-dimm #2 the only important register
> > would be DRB2 (side 2 of so-dimm #1) and DRB3(side 1 of so-dimm #2),
> > correct?

The modified version would be something like
read32(0 + addr_offset);
for(i = 0; i < ARRAY_SIZE(ctrl->channel0); i++) {
    reg8 = pci_read_config8(ctrl->d0/*?*/, DRB1 + i);
    read32(reg8 * 32);
}

I've dropped the if(reg8) because cn700 doesn't program unused DRBs,
hence they're 0, but that obviously wouldn't work here. And I just
realized I didn't do the cn700 the way I meant to, should have read the
start addresses, not the end ones (simple fix).

-Corey

v2[PATCH]RCA RM4100 i82830 support

Quoting Corey Osgood :
> On Tue, 2008-02-26 at 21:33 -0500, [e-mail] wrote:
> > Quoting [e-mail]:
> > > Quoting Corey Osgood :
> > > > On Tue, 2008-02-26 at 16:19 -0500, [e-mail] wrote:
> > > > > > > +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).
> > > > >
> > > > > Oh you mean this? What do I need to do to adapt it for the i82830?
> > > > > /* NOTE: Dual-sided ready */
> > > > > read32(0 + addr_offset);
> > > > > for(i = 0; i < (ARRAY_SIZE(ctrl->channel0) * 2); i++) {
> > > > > reg8 = pci_read_config8(ctrl->d0f3, 0x40 + i);
> > > > > if(reg8) read32((reg8 << 26) + addr_offset);
> > > > > }
> > > >
> > > > This reads from register 0x40 + i, where 0x40 is the top of the first
> > > > dimm, and i counts to the max number of dimms.
> > >
> > > Why read this from a register?? Why not just use (i = 0; i <
> > > DIMM_SOCKETS; i++)
>
> ARRAY_SIZE(ctrl->channel0) is the number of spd addresses, which can be
> less than the max number of dimms.
> > > > Reg 0x40 contains bits
> > > > 33:26 of the top address, hence the << 26. So you'd need to adjust both
> > > > of these to fit the i830's DRB mechanism. If the i830 uses an ugly
> > > > format like the i810 does, then it may be easier to store the size in
> > > > scratch registers somewhere after it's calculated, and then use those
> > > > values instead. Sorry I can't get more specific, I've got to head out to
> > > > my next service call.
> > >
> > > The DRB i830 is not ugly it is very simple, simple, simple. So simple
> > > that it just calculates the memory size in ticks of 32 per side
> >
> > (Sorry typo I meant) +
> > > the next side up to 4 sides. Like this:
> > >
> > > Example: 128MB SINGLE sided so-dimm in slot 1
> > >           128MB DOUBLE sided so-dimm in slot 2
> > >
> > > There are 4 DRB registers(actually 6 but but only 4 that are usable -
> > > Intel design glitch), 1 for each side. So in this example they would
> > > look like this:
> > >
> > > DRB1 = 0x04
> > > DRB2 = 0x04
> > > DRB3 = 0x06
> > > DRB4 = 0x08
> > >
> > > So if we needed to convert this we would do something like this (DRB *
> > > 32) * 1024 correct? But if we just need to calculate the end of
> > > so-dimm #1 and the start of so-dimm #2 the only important register
> > > would be DRB2 (side 2 of so-dimm #1) and DRB3(side 1 of so-dimm #2),
> > > correct?
>
> The modified version would be something like
> read32(0 + addr_offset);
> for(i = 0; i < ARRAY_SIZE(ctrl->channel0); i++) {
>    reg8 = pci_read_config8(ctrl->d0/*?*/, DRB1 + i);
>    read32(reg8 * 32);
> }

Ok starting to make sense but what is all this about? /*?*/ wouldn't  
it just be reg8 = pci_read_config8(ctrl->d0, DRB1 = i);
This is going to show our results in MB, is that what we want? For  
accuracy shouldn't it be in bytes? Sorry for so many questions, I just  
want to get this right.

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Corey Osgood]

On Wed, Feb 27, 2008 at 7:16 AM,  wrote:
> Quoting Corey Osgood :
> > On Tue, 2008-02-26 at 21:33 -0500, [e-mail] wrote:
> > > Quoting [e-mail]:
> > > > Quoting Corey Osgood :
> > > > > On Tue, 2008-02-26 at 16:19 -0500, [e-mail] wrote:
> > > > > > >> +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).
> > > > > > >
> > > > > > Oh you mean this? What do I need to do to adapt it for the i82830?
> > > > > >    /* NOTE: Dual-sided ready */
> > > > > >    read32(0 + addr_offset);
> > > > > >    for(i = 0; i < (ARRAY_SIZE(ctrl->channel0) * 2); i++) {
> > > > > >            reg8 = pci_read_config8(ctrl->d0f3, 0x40 + i);
> > > > > >    if(reg8) read32((reg8 << 26) + addr_offset);
> > > > > > }
> > > > >
> > > > > This reads from register 0x40 + i, where 0x40 is the top of the
>
> first
> > > > > dimm, and i counts to the max number of dimms.
> > > >
> > > > Why read this from a register?? Why not just use (i = 0; i <
> > > > DIMM_SOCKETS; i++)
> >
> > ARRAY_SIZE(ctrl->channel0) is the number of spd addresses, which can be
> > less than the max number of dimms.
> > > > > Reg 0x40 contains bits
> > > > > 33:26 of the top address, hence the << 26. So you'd need to adjust
>
> both
> > > > > of these to fit the i830's DRB mechanism. If the i830 uses an ugly
> > > > > format like the i810 does, then it may be easier to store the size
>
> in
> > > > > scratch registers somewhere after it's calculated, and then use
>
> those
> > > > > values instead. Sorry I can't get more specific, I've got to head
>
> out to
> > > > > my next service call.
> > > >
> > > > The DRB i830 is not ugly it is very simple, simple, simple. So simple
> > > > that it just calculates the memory size in ticks of 32 per side
> > >
> > > (Sorry typo I meant) +
> > > > the next side up to 4 sides. Like this:
> > > >
> > > > Example: 128MB SINGLE sided so-dimm in slot 1
> > > >           128MB DOUBLE sided so-dimm in slot 2
> > > >
> > > > There are 4 DRB registers(actually 6 but but only 4 that are usable -
> > > > Intel design glitch), 1 for each side. So in this example they would
> > > > look like this:
> > > >
> > > > DRB1 = 0x04
> > > > DRB2 = 0x04
> > > > DRB3 = 0x06
> > > > DRB4 = 0x08
> > > >
> > > > So if we needed to convert this we would do something like this (DRB
> > > > 1024 correct? But if we just need to calculate the end of
> > > >
> > > > so-dimm #1 and the start of so-dimm #2 the only important register
> > > > would be DRB2 (side 2 of so-dimm #1) and DRB3(side 1 of so-dimm #2),
> > > > correct?
> >
> > The modified version would be something like
> > read32(0 + addr_offset);
> > for(i = 0; i < ARRAY_SIZE(ctrl->channel0); i++) {
> >       reg8 = pci_read_config8(ctrl->d0/*?*/, DRB1 + i);
> >       read32(reg8 * 32);
> > }
>
> Ok starting to make sense but what is all this about? /*?*/ wouldn't
> it just be reg8 = pci_read_config8(ctrl->d0, DRB1 = i);

DRB1 + i, but yes. The /*?*/ is an inline comment, because I wasn't sure if
it was actually device 0 function 0 that had the memory registers.
> This is going to show our results in MB, is that what we want? For
> accuracy shouldn't it be in bytes? Sorry for so many questions, I just
> want to get this right.

Right again, it would need to be read32(reg8 * 32 * 1024 * 1024) (If I'm
thinking correctly).

-Corey
> Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

I hate to say it Corey, but this just sends it into an infinite loop.  
Does it make a difference that the onboard memory is located in slot2  
and slot1 is empty? Here is what I have:

Thanks - Joe

static void do_ram_command(const struct mem_controller *ctrl, uint32_t  
command, uint32_t addr_offset)
{
    int i;
    uint8_t reg8;
    uint32_t reg32;

/* Configure the RAM command. */
    reg32 = pci_read_config32(ctrl->d0, DRC);
    /* Clear bits 29, 10-8, 6-4. */
    reg32 &= 0xdffff88f;
    reg32 |= command << 4;
       /* If RAM_COMMAND_NORMAL set the refresh mode and IC bit. */
    if (command == RAM_COMMAND_NORMAL) {
    reg32 |= ((RAM_COMMAND_REFRESH << 8) | (RAM_COMMAND_IC << 29));
       }
    pci_write_config32(ctrl->d0, DRC, reg32);

/* RAM_COMMAND_NORMAL affects only the memory controller and
       doesn't need to be "sent" to the DIMMs. */
    /* if (command == RAM_COMMAND_NORMAL) return; */

PRINT_DEBUG("    Sending RAM command 0x");
    PRINT_DEBUG_HEX32(reg32);
    PRINT_DEBUG(" to 0x");
    PRINT_DEBUG_HEX32(0 + addr_offset);
    PRINT_DEBUG("\r\n");

/* NOTE: Dual-sided ready */
    read32(0 + addr_offset);
    for(i = 0; i < ARRAY_SIZE(ctrl->channel0); i++) {
    	reg8 = pci_read_config8(ctrl->d0, DRB + i);
    	read32(((reg8 * 32) * 1024) * 1024);
    }
}

v2[PATCH]RCA RM4100 i82830 support

[Corey Osgood]

On Tue, Mar 4, 2008 at 11:37 PM,  wrote:
> I hate to say it Corey, but this just sends it into an infinite loop.
> Does it make a difference that the onboard memory is located in slot2
> and slot1 is empty? Here is what I have:
>
> Thanks - Joe

Try this instead, IIRC there are 4 DRBs. If it fails to build with too few
registers, I think you can nuke the extra variable and just do multiple
reads to the same location, or else reuse reg32 in place of reg8_2. I'm not
really sure why the previous version failed, it looks sound to me. It might
be interesting to put in some debugging output, to see what's going wrong.

static void do_ram_command(const struct mem_controller *ctrl, uint32_t
command, uint32_t addr_offset)
{
       int i;
       uint8_t reg8, reg8_2 = 0;
       uint32_t reg32;

/* Configure the RAM command. */
       reg32 = pci_read_config32(ctrl->d0, DRC);
       /* Clear bits 29, 10-8, 6-4. */
       reg32 &= 0xdffff88f;
       reg32 |= command << 4;
      /* If RAM_COMMAND_NORMAL set the refresh mode and IC bit. */
       if (command == RAM_COMMAND_NORMAL) {
       reg32 |= ((RAM_COMMAND_REFRESH << 8) | (RAM_COMMAND_IC << 29));
      }
       pci_write_config32(ctrl->d0, DRC, reg32);

/* RAM_COMMAND_NORMAL affects only the memory controller and
          doesn't need to be "sent" to the DIMMs. */
       /* if (command == RAM_COMMAND_NORMAL) return; */

PRINT_DEBUG("    Sending RAM command 0x");
       PRINT_DEBUG_HEX32(reg32);
       PRINT_DEBUG(" to 0x");
       PRINT_DEBUG_HEX32(0 + addr_offset);
       PRINT_DEBUG("\r\n");

/* NOTE: Dual-sided ready */
       read32(0 + addr_offset);
        for(i = 0; i < 4; i++) {
                reg8 = pci_read_config8(ctrl->d0, DRB + i);
               if(reg8 != reg8_2) read32(reg8 * 32 * 1024 * 1024); //extra
parentheses aren't necessary
               reg8_2 = reg8;
       }
}

v2[PATCH]RCA RM4100 i82830 support

Quoting Corey Osgood :
> Try this instead, IIRC there are 4 DRBs. If it fails to build with too few
> registers, I think you can nuke the extra variable and just do multiple
> reads to the same location, or else reuse reg32 in place of reg8_2. I'm not
> really sure why the previous version failed, it looks sound to me. It might
> be interesting to put in some debugging output, to see what's going wrong.

Yep that worked great:-)
Here is a revised patch with all the suggested changes.

Signed-off-by: Joseph Smith

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

[Uwe Hermann]

On Sat, Mar 08, 2008 at 11:17:24PM -0500, [e-mail] wrote:
> Quoting Corey Osgood :
> > Try this instead, IIRC there are 4 DRBs. If it fails to build with too few
> > registers, I think you can nuke the extra variable and just do multiple
> > reads to the same location, or else reuse reg32 in place of reg8_2. I'm not
> > really sure why the previous version failed, it looks sound to me. It might
> > be interesting to put in some debugging output, to see what's going wrong.
>
> Yep that worked great:-)
> Here is a revised patch with all the suggested changes.
>
> Signed-off-by: Joseph Smith

Thanks, committed in r3129 with variour cleanups for now. Will post further
suggestions and/or patches later. Please add a wiki status page for
the board which says which parts already work and which don't.

Uwe.

v2[PATCH]RCA RM4100 i82830 support

Quoting Uwe Hermann :
> On Sat, Mar 08, 2008 at 11:17:24PM -0500, [e-mail] wrote:
> > Quoting Corey Osgood :
> > > Try this instead, IIRC there are 4 DRBs. If it fails to build with too few
> > > registers, I think you can nuke the extra variable and just do multiple
> > > reads to the same location, or else reuse reg32 in place of reg8_2. I'm not
> > > really sure why the previous version failed, it looks sound to me. It might
> > > be interesting to put in some debugging output, to see what's going wrong.
> >
> > Yep that worked great:-)
> > Here is a revised patch with all the suggested changes.
> >
> > Signed-off-by: Joseph Smith
>
> Thanks, committed in r3129 with variour cleanups for now. Will post further
> suggestions and/or patches later. Please add a wiki status page for
> the board which says which parts already work and which don't.

Thanks Uwe. Any suggestions, comments and/or patches are welcome. At  
this point the only thing that isn't working is the tv-out. But that  
is soon to come. Oh there is the flashrom issue too......but that will  
be solved soon also (hopefully). Who do I need to talk to about  
getting Wiki access, Ron??

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

Quoting Corey Osgood :
> On Tue, 2008-02-26 at 02:27 +0100, Peter Stuge wrote:
> > On Mon, Feb 25, 2008 at 04:31:15PM -0500, [e-mail] wrote:
> > ..
> > > > tv-out
> > >
> > > Dis-reguard the last patch, this is the real one. A e100 kernel driver
> > > developer helped me figure out that the ethernet was an irq routing
> > > issue, fixed! It is working great now:-)))
> >
> > Good news!
> >
> > Do you have any idea about the TV out?
> >
> > Also see the comments below:
> > > +++ src/northbridge/intel/i82830/raminit.c	(revision 0)
> >
> > ..
> > > +static void set_dram_timing(const struct mem_controller *ctrl)
> > > +{
> > > /* Set the value for DRAM Timing Register */
> > > /* TODO: Configure the value according to SPD values. */
> > > pci_write_config32(ctrl->d0, DRT, 0x00000010);
> > >
> > > +}
> > >
> > > +static void set_dram_buffer_strength(const struct mem_controller *ctrl)
> > > +{
> > > /* TODO: This needs to be set according to the DRAM tech
> > > * (x8, x16, or x32). Argh, Intel provides no docs on this!
> > > * Currently, it needs to be pulled from the output of
> > > * lspci -xxx Rx92
> > > */
> > > /* Set the value for System Memory Buffer Strength Control Registers */
> > > pci_write_config32(ctrl->d0, BUFF_SC, 0xFC9B491B);
> > >
> > > +}
> >
> > How about these two TODOs? In the second case I think the comment
> > refers to running the command under the factory BIOS - in that case
> > it would be good to clarify that.
>
> The memory on the rm4100 is onboard. There's a header for a memory slot,
> but there's more unknown hardware missing to hook that up. The only
> version with memory slots is the Thomson IP1001 (somewhat rare,
> essentially the same board with a different branding), and perhaps some
> internal RCA prototypes. This info is courtesy of Joe and his site ;)
>
> That said, the BUFF_SC registers are the same deal as on the i810, the
> explanation in the datasheet isn't specific or clear about how to set
> them. The rest of the registers are fairly simple to set, but without
> the ability to try other memory sticks or with real spd data, it might
> look right on the rm4100, but fail in practice.

This is not necessarily true. It is setup so the controller actually  
reads the fake spd table just like it was real (kind of tricks the  
controller). You can change the values in the spd table around to get  
different results.  That's how I tested it. The only thing I couldn't  
test is the second so-dimm slot. Like Corey said above the header is  
missing....
> > > +static void sdram_enable(int controllers, const struct
> >
> > mem_controller *ctrl)
> > > +{
> >
> > ..
> > > /* 4. Mode register set. Wait two memory cycles. */
> > > /* TODO: Set offset according to DRT values */
> > > PRINT_DEBUG("RAM Enable 4: Mode register set\r\n");
> > > do_ram_command(ctrl, RAM_COMMAND_MRS, 0x1d0);
> >
> > Can this be done?
>
> Same deal as above. I can put together another patch later, and put the
> code in ifdefs with a note to that effect.
>
> -Corey
> > > +++ targets/rca/rm4100/Config.lb	(revision 0)
> >
> > ..
> > > +romimage "fallback"
> > > option USE_FALLBACK_IMAGE = 1
> > > option FALLBACK_SIZE = ROM_SIZE
> > > option COREBOOT_EXTRA_VERSION = "_RM4100"
> > >
> > > +#	payload /etc/hosts
> >
> > This last line makes no sense so I would remove it before the commit.
> >
> > //Peter

Thanks - Joe

v2[PATCH]RCA RM4100 i82830 support

Quoting Peter Stuge :
> On Mon, Feb 25, 2008 at 04:31:15PM -0500, [e-mail] wrote:
> ..
> > > tv-out
> >
> > Dis-reguard the last patch, this is the real one. A e100 kernel driver
> > developer helped me figure out that the ethernet was an irq routing
> > issue, fixed! It is working great now:-)))
>
> Good news!

Yeh, over a year on this baby......
> Do you have any idea about the TV out?

Yes ..ideas. I have several others that are going to work on the  
tv-out with me,  that's why I would like to get the source posted.
> Also see the comments below:
> > +++ src/northbridge/intel/i82830/raminit.c	(revision 0)
>
> ..
> > +static void set_dram_timing(const struct mem_controller *ctrl)
> > +{
> > /* Set the value for DRAM Timing Register */
> > /* TODO: Configure the value according to SPD values. */
> > pci_write_config32(ctrl->d0, DRT, 0x00000010);
> >
> > +}
> >
> > +static void set_dram_buffer_strength(const struct mem_controller *ctrl)
> > +{
> > /* TODO: This needs to be set according to the DRAM tech
> > * (x8, x16, or x32). Argh, Intel provides no docs on this!
> > * Currently, it needs to be pulled from the output of
> > * lspci -xxx Rx92
> > */
> > /* Set the value for System Memory Buffer Strength Control Registers */
> > pci_write_config32(ctrl->d0, BUFF_SC, 0xFC9B491B);
> >
> > +}
>
> How about these two TODOs? In the second case I think the comment
> refers to running the command under the factory BIOS - in that case
> it would be good to clarify that.

TODO #1 - The onboard memory has CAS 3 so this is manually set for now.
TODO #2 - That came from Corey's i810 code. And he is right, I  
couldn't find any docs on this so it just got set manually.
> > +static void sdram_enable(int controllers, const struct   
> > mem_controller *ctrl)
> > +{
>
> ..
> > /* 4. Mode register set. Wait two memory cycles. */
> > /* TODO: Set offset according to DRT values */
> > PRINT_DEBUG("RAM Enable 4: Mode register set\r\n");
> > do_ram_command(ctrl, RAM_COMMAND_MRS, 0x1d0);
>
> Can this be done?

Sure it can. The addr_offset (0x1d0) is based on the memory's CAS#  
Latency. The onboard memory has CAS 3 so this is manually set for now.  
I think some of the other Intel chips already have this setup, but  
they are a little different than this one.
> > +++ targets/rca/rm4100/Config.lb	(revision 0)
>
> ..
> > +romimage "fallback"
> > option USE_FALLBACK_IMAGE = 1
> > option FALLBACK_SIZE = ROM_SIZE
> > option COREBOOT_EXTRA_VERSION = "_RM4100"
> >
> > +#	payload /etc/hosts
>
> This last line makes no sense so I would remove it before the commit.

Sure.
> //Peter

Thanks - Joe