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

[Ira W. Snyder]

On Wed, Jul 08, 2009 at 03:58:02PM -0500, Kumar Gala wrote:
>
> On Jul 8, 2009, at 2:33 PM, Ira W. Snyder wrote:
>
>> On Wed, Jul 08, 2009 at 01:09:39PM -0500, Kumar Gala wrote:
>>>
>>> On Jul 8, 2009, at 11:19 AM, Ira W. Snyder wrote:
>>>
>>>> Add support for the Freescale MPC83xx memory controller to the
>>>> existing
>>>> driver for the Freescale MPC85xx memory controller. The only
>>>> difference
>>>> between the two processors are in the CS_BNDS register parsing code.
>>>>
>>>> The L2 cache controller does not exist on the MPC83xx, but the OF
>>>> subsystem
>>>> will not use the driver if the device is not present in the OF  
>>>> device
>>>> tree.
>>>>
>>>> Signed-off-by: Ira W. Snyder <iws at ovro.caltech.edu>
>>>> ---
>>>>
>>>> This was tested on an MPC8349EMDS-based board.
>>>>
>>>> I don't really like how the MCE disable works on mpc85xx (clearing  
>>>> the
>>>> HID1 register), but this can be revisited when mpc86xx support gets
>>>> added. It sucks to have this happen before the probe() routine is
>>>> called
>>>> and we know what kind of hardware we're actually running on.
>>>>
>>>> drivers/edac/Kconfig        |    6 +++---
>>>> drivers/edac/mpc85xx_edac.c |   38 +++++++++++++++++++++++++++
>>>> +----------
>>>> drivers/edac/mpc85xx_edac.h |    3 +++
>>>> 3 files changed, 34 insertions(+), 13 deletions(-)
>>>
>>> [snip]
>>>
>>>> /************************ MC SYSFS parts
>>>> ***********************************/
>>>>
>>>> @@ -738,7 +740,8 @@ static irqreturn_t mpc85xx_mc_isr(int irq, void
>>>> *dev_id)
>>>> 	return IRQ_HANDLED;
>>>> }
>>>>
>>>> -static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci)
>>>> +static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci,
>>>> +					  const struct of_device_id *match)
>>>> {
>>>> 	struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
>>>> 	struct csrow_info *csrow;
>>>> @@ -784,18 +787,26 @@ static void __devinit  
>>>> mpc85xx_init_csrows(struct
>>>> mem_ctl_info *mci)
>>>> 	}
>>>>
>>>> 	for (index = 0; index < mci->nr_csrows; index++) {
>>>> -		u32 start;
>>>> -		u32 end;
>>>> +		u32 start, end, extra;
>>>>
>>>> 		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 (match->data && match->data == MPC83xx) {
>>>> +			start = (cs_bnds & 0x00ff0000) << 8;
>>>> +			end   = (cs_bnds & 0x000000ff) << 24;
>>>> +			extra = 0x00ffffff;
>>>> +		} else {
>>>> +			start = (cs_bnds & 0x0fff0000) << 4;
>>>> +			end   = (cs_bnds & 0x00000fff) << 20;
>>>> +			extra = 0x000fffff;
>>>> +		}
>>>
>>> I don't understand this at all.. The only difference between 83xx &  
>>> 85xx
>>> is that we should have an extra 4-bits for 36-bit physical addresses.
>>>
>>> We should be able to write this code in such a way that it works for
>>> both 83xx & 85xx.
>>>
>>>                start = (cs_bnds & 0xffff0000) >> 16;
>>>                end = (cs_bnds & 0xffff);
>>>
>>> 		if (start == end)
>>> 			continue;
>>> 		start <<= (20 - PAGE_SHIFT);
>>> 		end <<= (20 - PAGE_SHIFT);
>>> 		end |= (1 << (20 - PAGE_SHIFT)) - 1;
>>>
>>
>> Sorry, I don't know a thing about PAGE_SHIFT. Looking in
>> arch/powerpc/platforms/Kconfig.cputype, PPC_85xx doesn't seem to imply 
>> a
>> change in PAGE_SIZE, which changes the PAGE_SHIFT in
>> arch/powerpc/include/asm/page.h.
>>
>> Also, are you sure you cannot use 4K pages on an 85xx? If you can use 
>> 4K
>> pages on 85xx, then PAGE_SHIFT == 12, and your solution breaks.
>>
>> I admit I don't know much about all of the different powerpc  
>> platforms.
>
> PAGE_SHIFT is the same.. I was folding in the code from below the diff:
>
>                 csrow->first_page = start >> PAGE_SHIFT;
>                 csrow->last_page = end >> PAGE_SHIFT;
>
> into the calculation of start/end.  I know PAGE_SHIFT is a minimum 12 on 
> any PPC based system (and at this point its always 12 on 85xx/83xx).  The 
> idea is to ensure that when shift start/end up that it still fits in 
> 32-bits.  This is to handle the case of >4G of memory.
>

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.

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