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

[Kumar Gala]

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

> On Thu, Jul 09, 2009 at 01:25:43PM -0500, Kumar Gala wrote:
>>
>> On Jul 9, 2009, at 1:17 PM, Ira W. Snyder wrote:
>>
>>> On Thu, Jul 09, 2009 at 12:58:53PM -0500, Kumar Gala wrote:
>>>>> 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.
>>>
>>>>> 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]
>>>>
>>>>> /************************ 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.
>>>
>>>>> +
>>>>> +		start = (cs_bnds & 0xffff0000) >> 16;
>>>>> +		end   = (cs_bnds & 0x0000ffff);
>>>>>
>>>
>>> This is the same in both our versions.
>>>
>>> start == 0x0
>>> end   == 0xF
>>>
>>>>> 		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?

- k