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

["Ira W. Snyder" <iws@ovro.caltech.edu>]

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.

I'll submit a new patch now.

Thanks for the help,
Ira