Re: [PATCH] edac: mpc85xx: add support for mpc83xx memory controller

On Thu, Jul 09, 2009 at 03:15:29PM -0500, Kumar Gala wrote:

On Jul 9, 2009, at 2:35 PM, Ira W. Snyder wrote:

quoted

On Thu, Jul 09, 2009 at 01:25:43PM -0500, Kumar Gala wrote:

quoted

On Jul 9, 2009, at 1:17 PM, Ira W. Snyder wrote:

quoted

On Thu, Jul 09, 2009 at 12:58:53PM -0500, Kumar Gala wrote:

quoted

quoted

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out  
correctly.

What does it come out as?  How much memory do you have in the  
system?

The size_mb shows as 0 with your code. See the explanation below.  
With
my code it shows as 256MB, as expected.

I have 256MB memory in the system.

quoted

quoted

The attached patch DOES work on my board, but I'm confident 
that it
does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did
wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the
size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the
pfn,
then I think the original code got it wrong, and the 
calculation for
csrow->nr_pages needs to be changed.

Thanks,
Ira

[snip]

quoted

/************************ MC SYSFS parts
***********************************/

@@ -790,18 +792,19 @@ static void __devinit

mpc85xx_init_csrows(struct
mem_ctl_info *mci)
		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
-		if (start)
-			start |= 0xfffff;
-		if (end)
-			end |= 0xfffff;

can you printk what cs_bnds values are in your setup.

I am only using a single chip select. CS0_BNDS (register 0xe0002000)
is
0x0000000F.

quoted

quoted

+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);

This is the same in both our versions.

start == 0x0
end   == 0xF

quoted

quoted

		if (start == end)
			continue;	/* not populated */

-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+

MY VERSION

start == 0x0
end   == 0xffff

first_page == 0x0
last_page  == 0xffff

YOUR VERSION (<<= (20 - PAGE_SHIFT), etc.)

My math was wrong it should be ( <<= (24 - PAGE_SHIFT) )

With that I think things work out.

Yep, that works out great. This solution is much better than my  
original
code. The 83xx doesn't need to be special-cased anymore.

I checked the math for a 85xx with 64GB of memory. Assuming it uses  
64K
pages (PAGE_SHIFT == 16), then everything works out.

Does the math work for a 64GB system w/4K pages?

Yes, the math works there too. I just double-checked by hand. The v2
patch should work fine on both the 83xx and 85xx (both 4K and 64K
pages).

Thanks again for the help on this one.
Ira