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

[Kumar Gala]

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.

- k

>
>
> start == 0x0
> end   == 0xfff
>
> first_page == 0x0
> last_page  == 0x0
>
>>> +		csrow->first_page = start;
>>> +		csrow->last_page = end;
>>
>> This seems odd to me... I can't believe this is working out properly.
>>
>>>
>>> 		csrow->nr_pages = csrow->last_page + 1 - csrow->first_page;
>
> The calculation is unchanged here from the original code. Due to the
> ">> PAGE_SHIFT", nr_pages ends up as 1 in your version.
>
> MY VERSION
>
> nr_pages == 0xffff + 1 - 0 == 0x10000
>
> 0x10000 * 4096 / 1024 / 1024 == 256 MB
>
>
> YOUR VERSION
>
> nr_pages == 0x0 + 1 - 0x0 == 1
>
> 0x1 * 4096 / 1024 / 1024 == 0MB
>
>>> 		csrow->grain = 8;
>>> 		csrow->mtype = mtype;
>>>
>>
>> Lets get some real values on the table for your system so I can get a
>> sense of what's really going on.
>>
>
> Thanks for the help.
> Ira