Re: + edac-cpc925-mc-platform-device-setup.patch added to -mm tree

Re: + edac-cpc925-mc-platform-device-setup.patch added to -mm tree

[Kumar Gala]

On Apr 15, 2009, at 5:27 PM, akpm@linux-foundation.org wrote:

>
> The patch titled
>     edac: cpc925 MC platform device setup
> has been added to the -mm tree.  Its filename is
>     edac-cpc925-mc-platform-device-setup.patch
>
> Before you just go and hit "reply", please:
>   a) Consider who else should be cc'ed
>   b) Prefer to cc a suitable mailing list as well
>   c) Ideally: find the original patch on the mailing list and do a
>      reply-to-all to that, adding suitable additional cc's
>
> *** Remember to use Documentation/SubmitChecklist when testing your  
> code ***
>
> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> out what to do about this
>
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
>
> ------------------------------------------------------
> Subject: edac: cpc925 MC platform device setup
> From: Harry Ciao <qingtao.cao@windriver.com>
>
> Fix up the number of cells for the values of CPC925 Memory Controller,
> and setup related platform device during system booting up, against
> which CPC925 Memory Controller EDAC driver would be matched.
>
> Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
> Cc: Doug Thompson <norsk5@yahoo.com>
> Cc: Michael Ellerman <michael@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Kumar Gala <galak@gate.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> arch/powerpc/kernel/prom_init.c      |   33 +++++++++++++
> arch/powerpc/platforms/maple/setup.c |   59 +++++++++++++++++++++++++
> 2 files changed, 92 insertions(+)
>
> diff -puN arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform- 
> device-setup arch/powerpc/kernel/prom_init.c
> --- a/arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform-device- 
> setup
> +++ a/arch/powerpc/kernel/prom_init.c
> @@ -1947,8 +1947,40 @@ static void __init fixup_device_tree_map
> 	prom_setprop(isa, name, "ranges",
> 			isa_ranges, sizeof(isa_ranges));
> }
> +
> +#define CPC925_MC_START		0xf8000000
> +#define CPC925_MC_LENGTH	0x1000000
> +/* The values for memory-controller don't have right number of  
> cells */
> +static void __init fixup_device_tree_maple_memory_controller(void)
> +{

I don't see why this cant be part of the existing  
fixup_device_tree_maple().

I also find it odd we don't ensure we are running on a maple before we  
apply this fixup.

- k

>
> +	phandle mc;
> +	u32 mc_reg[4];
> +	char *name = "/hostbridge@f8000000";
> +
> +	mc = call_prom("finddevice", 1, 1, ADDR(name));
> +	if (!PHANDLE_VALID(mc))
> +		return;
> +
> +	if (prom_getproplen(mc, "reg") != 8)
> +		return;
> +
> +	if (prom_getprop(mc, "reg", mc_reg, sizeof(mc_reg)) == PROM_ERROR)
> +		return;
> +
> +	if (mc_reg[0] != CPC925_MC_START || mc_reg[1] != CPC925_MC_LENGTH)
> +		return;
> +
> +	prom_printf("Fixing up bogus hostbridge on Maple...\n");
> +
> +	mc_reg[0] = 0x0;
> +	mc_reg[1] = CPC925_MC_START;
> +	mc_reg[2] = 0x0;
> +	mc_reg[3] = CPC925_MC_LENGTH;
> +	prom_setprop(mc, name, "reg", mc_reg, sizeof(mc_reg));
> +}
> #else
> #define fixup_device_tree_maple()
> +#define fixup_device_tree_maple_memory_controller()
> #endif
>
> #ifdef CONFIG_PPC_CHRP
> @@ -2189,6 +2221,7 @@ static void __init fixup_device_tree_efi
> static void __init fixup_device_tree(void)
> {
> 	fixup_device_tree_maple();
> +	fixup_device_tree_maple_memory_controller();
> 	fixup_device_tree_chrp();
> 	fixup_device_tree_pmac();
> 	fixup_device_tree_efika();
> diff -puN arch/powerpc/platforms/maple/setup.c~edac-cpc925-mc- 
> platform-device-setup arch/powerpc/platforms/maple/setup.c
> --- a/arch/powerpc/platforms/maple/setup.c~edac-cpc925-mc-platform- 
> device-setup
> +++ a/arch/powerpc/platforms/maple/setup.c
> @@ -335,3 +335,62 @@ define_machine(maple) {
> 	.progress		= maple_progress,
> 	.power_save		= power4_idle,
> };
> +
> +#ifdef CONFIG_EDAC
> +/*
> + * Register a platform device for CPC925 memory controller on
> + * Motorola ATCA-6101 blade.
> + */
> +#define MAPLE_CPC925_MODEL	"Motorola,ATCA-6101"
> +static int __init maple_cpc925_edac_setup(void)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *np = NULL;
> +	struct resource r;
> +	const unsigned char *model;
> +	int ret;
> +
> +	np = of_find_node_by_path("/");
> +	if (!np) {
> +		printk(KERN_ERR "%s: Unable to get root node\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	model = (const unsigned char *)of_get_property(np, "model", NULL);
> +	if (!model) {
> +		printk(KERN_ERR "%s: Unabel to get model info\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	ret = strcmp(model, MAPLE_CPC925_MODEL);
> +	of_node_put(np);
> +
> +	if (ret != 0)
> +		return 0;
> +
> +	np = of_find_node_by_type(NULL, "memory-controller");
> +	if (!np) {
> +		printk(KERN_ERR "%s: Unable to find memory-controller node\n",
> +			__func__);
> +		return -ENODEV;
> +	}
> +
> +	ret = of_address_to_resource(np, 0, &r);
> +	of_node_put(np);
> +
> +	if (ret < 0) {
> +		printk(KERN_ERR "%s: Unable to get memory-controller reg\n",
> +			__func__);
> +		return -ENODEV;
> +	}
> +
> +	pdev = platform_device_register_simple("cpc925_edac", 0, &r, 1);
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +
> +	printk(KERN_INFO "%s: CPC925 platform device created\n", __func__);
> +
> +	return 0;
> +}
> +machine_device_initcall(maple, maple_cpc925_edac_setup);
> +#endif
> _
>
> Patches currently in -mm which might be from  
> qingtao.cao@windriver.com are
>
> edac-add-cpc925-memory-controller-driver.patch
> edac-add-cpc925-memory-controller-driver-cleanup.patch
> edac-add-edac_device_alloc_index.patch
> edac-add-edac_device_alloc_index-cleanup.patch
> edac-cpc925-mc-platform-device-setup.patch

Re: + edac-cpc925-mc-platform-device-setup.patch added to -mm tree

[Harry Ciao]

Kumar Gala wrote:
>
> On Apr 15, 2009, at 5:27 PM, akpm@linux-foundation.org wrote:
>
>>
>> The patch titled
>>     edac: cpc925 MC platform device setup
>> has been added to the -mm tree.  Its filename is
>>     edac-cpc925-mc-platform-device-setup.patch
>>
>> Before you just go and hit "reply", please:
>>   a) Consider who else should be cc'ed
>>   b) Prefer to cc a suitable mailing list as well
>>   c) Ideally: find the original patch on the mailing list and do a
>>      reply-to-all to that, adding suitable additional cc's
>>
>> *** Remember to use Documentation/SubmitChecklist when testing your 
>> code ***
>>
>> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
>> out what to do about this
>>
>> The current -mm tree may be found at 
>> http://userweb.kernel.org/~akpm/mmotm/
>>
>> ------------------------------------------------------
>> Subject: edac: cpc925 MC platform device setup
>> From: Harry Ciao <qingtao.cao@windriver.com>
>>
>> Fix up the number of cells for the values of CPC925 Memory Controller,
>> and setup related platform device during system booting up, against
>> which CPC925 Memory Controller EDAC driver would be matched.
>>
>> Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
>> Cc: Doug Thompson <norsk5@yahoo.com>
>> Cc: Michael Ellerman <michael@ellerman.id.au>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Kumar Gala <galak@gate.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>
>> arch/powerpc/kernel/prom_init.c      |   33 +++++++++++++
>> arch/powerpc/platforms/maple/setup.c |   59 +++++++++++++++++++++++++
>> 2 files changed, 92 insertions(+)
>>
>> diff -puN 
>> arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform-device-setup 
>> arch/powerpc/kernel/prom_init.c
>> --- 
>> a/arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform-device-setup
>> +++ a/arch/powerpc/kernel/prom_init.c
>> @@ -1947,8 +1947,40 @@ static void __init fixup_device_tree_map
>>     prom_setprop(isa, name, "ranges",
>>             isa_ranges, sizeof(isa_ranges));
>> }
>> +
>> +#define CPC925_MC_START        0xf8000000
>> +#define CPC925_MC_LENGTH    0x1000000
>> +/* The values for memory-controller don't have right number of cells */
>> +static void __init fixup_device_tree_maple_memory_controller(void)
>> +{
>
> I don't see why this cant be part of the existing 
> fixup_device_tree_maple().
>
> I also find it odd we don't ensure we are running on a maple before we 
> apply this fixup.
>
> - k
>
Hi Kumar,

Thanks a lot for your concern.

This newly added fixup for memory controller on Maple will be placed 
right after fixup_device_tree_maple(), both of them will be controlled 
by CONFIG_PPC_MAPLE, so there is no worry that it will be applied 
against anything other than Maple.

Meanwhile, it aims at fixup bad cell numbers for the memory controller, 
whereas the original fixup_device_tree_maple() aiming at fixing up the 
ISA controller on HT channel, we'd better separate them in different 
function IMHO.

Best regards,

Harry


>>
>> +    phandle mc;
>> +    u32 mc_reg[4];
>> +    char *name = "/hostbridge@f8000000";
>> +
>> +    mc = call_prom("finddevice", 1, 1, ADDR(name));
>> +    if (!PHANDLE_VALID(mc))
>> +        return;
>> +
>> +    if (prom_getproplen(mc, "reg") != 8)
>> +        return;
>> +
>> +    if (prom_getprop(mc, "reg", mc_reg, sizeof(mc_reg)) == PROM_ERROR)
>> +        return;
>> +
>> +    if (mc_reg[0] != CPC925_MC_START || mc_reg[1] != CPC925_MC_LENGTH)
>> +        return;
>> +
>> +    prom_printf("Fixing up bogus hostbridge on Maple...\n");
>> +
>> +    mc_reg[0] = 0x0;
>> +    mc_reg[1] = CPC925_MC_START;
>> +    mc_reg[2] = 0x0;
>> +    mc_reg[3] = CPC925_MC_LENGTH;
>> +    prom_setprop(mc, name, "reg", mc_reg, sizeof(mc_reg));
>> +}
>> #else
>> #define fixup_device_tree_maple()
>> +#define fixup_device_tree_maple_memory_controller()
>> #endif
>>
>> #ifdef CONFIG_PPC_CHRP
>> @@ -2189,6 +2221,7 @@ static void __init fixup_device_tree_efi
>> static void __init fixup_device_tree(void)
>> {
>>     fixup_device_tree_maple();
>> +    fixup_device_tree_maple_memory_controller();
>>     fixup_device_tree_chrp();
>>     fixup_device_tree_pmac();
>>     fixup_device_tree_efika();
>> diff -puN 
>> arch/powerpc/platforms/maple/setup.c~edac-cpc925-mc-platform-device-setup 
>> arch/powerpc/platforms/maple/setup.c
>> --- 
>> a/arch/powerpc/platforms/maple/setup.c~edac-cpc925-mc-platform-device-setup 
>>
>> +++ a/arch/powerpc/platforms/maple/setup.c
>> @@ -335,3 +335,62 @@ define_machine(maple) {
>>     .progress        = maple_progress,
>>     .power_save        = power4_idle,
>> };
>> +
>> +#ifdef CONFIG_EDAC
>> +/*
>> + * Register a platform device for CPC925 memory controller on
>> + * Motorola ATCA-6101 blade.
>> + */
>> +#define MAPLE_CPC925_MODEL    "Motorola,ATCA-6101"
>> +static int __init maple_cpc925_edac_setup(void)
>> +{
>> +    struct platform_device *pdev;
>> +    struct device_node *np = NULL;
>> +    struct resource r;
>> +    const unsigned char *model;
>> +    int ret;
>> +
>> +    np = of_find_node_by_path("/");
>> +    if (!np) {
>> +        printk(KERN_ERR "%s: Unable to get root node\n", __func__);
>> +        return -ENODEV;
>> +    }
>> +
>> +    model = (const unsigned char *)of_get_property(np, "model", NULL);
>> +    if (!model) {
>> +        printk(KERN_ERR "%s: Unabel to get model info\n", __func__);
>> +        return -ENODEV;
>> +    }
>> +
>> +    ret = strcmp(model, MAPLE_CPC925_MODEL);
>> +    of_node_put(np);
>> +
>> +    if (ret != 0)
>> +        return 0;
>> +
>> +    np = of_find_node_by_type(NULL, "memory-controller");
>> +    if (!np) {
>> +        printk(KERN_ERR "%s: Unable to find memory-controller node\n",
>> +            __func__);
>> +        return -ENODEV;
>> +    }
>> +
>> +    ret = of_address_to_resource(np, 0, &r);
>> +    of_node_put(np);
>> +
>> +    if (ret < 0) {
>> +        printk(KERN_ERR "%s: Unable to get memory-controller reg\n",
>> +            __func__);
>> +        return -ENODEV;
>> +    }
>> +
>> +    pdev = platform_device_register_simple("cpc925_edac", 0, &r, 1);
>> +    if (IS_ERR(pdev))
>> +        return PTR_ERR(pdev);
>> +
>> +    printk(KERN_INFO "%s: CPC925 platform device created\n", __func__);
>> +
>> +    return 0;
>> +}
>> +machine_device_initcall(maple, maple_cpc925_edac_setup);
>> +#endif
>> _
>>
>> Patches currently in -mm which might be from 
>> qingtao.cao@windriver.com are
>>
>> edac-add-cpc925-memory-controller-driver.patch
>> edac-add-cpc925-memory-controller-driver-cleanup.patch
>> edac-add-edac_device_alloc_index.patch
>> edac-add-edac_device_alloc_index-cleanup.patch
>> edac-cpc925-mc-platform-device-setup.patch
>

Re: + edac-cpc925-mc-platform-device-setup.patch added to -mm tree

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]

On Thu, 2009-04-16 at 09:57 +0800, Harry Ciao wrote:
> Kumar Gala wrote:
> > On Apr 15, 2009, at 5:27 PM, akpm@linux-foundation.org wrote:
> >> arch/powerpc/kernel/prom_init.c      |   33 +++++++++++++
> >> arch/powerpc/platforms/maple/setup.c |   59 +++++++++++++++++++++++++
> >> 2 files changed, 92 insertions(+)
> >>
> >> diff -puN 
> >> arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform-device-setup 
> >> arch/powerpc/kernel/prom_init.c
> >> --- 
> >> a/arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform-device-setup
> >> +++ a/arch/powerpc/kernel/prom_init.c
> >> @@ -1947,8 +1947,40 @@ static void __init fixup_device_tree_map
> >>     prom_setprop(isa, name, "ranges",
> >>             isa_ranges, sizeof(isa_ranges));
> >> }
> >> +
> >> +#define CPC925_MC_START        0xf8000000
> >> +#define CPC925_MC_LENGTH    0x1000000
> >> +/* The values for memory-controller don't have right number of cells */
> >> +static void __init fixup_device_tree_maple_memory_controller(void)
> >> +{
> >
> > I don't see why this cant be part of the existing 
> > fixup_device_tree_maple().
> >
> > I also find it odd we don't ensure we are running on a maple before we 
> > apply this fixup.

> Hi Kumar,
> 
> Thanks a lot for your concern.
> 
> This newly added fixup for memory controller on Maple will be placed 
> right after fixup_device_tree_maple(), both of them will be controlled 
> by CONFIG_PPC_MAPLE, so there is no worry that it will be applied 
> against anything other than Maple.

Hi Harry,

We regularly build a single kernel with multiple platforms enabled, so
just having it controlled by a CONFIG symbol is not sufficient. Someone
might build a kernel for MAPLE & PSERIES & ISERIES & CELL, so the maple
fixup needs to be careful it doesn't break the other platforms.

The existing maple fixup doesn't check if it's on a maple either, but it
is a bit more discerning about what it finds before it fixes things up.

Your code already checks that "reg" is 8 bytes long to start with, I
think if it also checks that #address-cells and #size-cells are == 2,
then it's pretty safe. Because at that point we know we have a node with
the right name, the reg property has a known value, and reg is short WRT
#cells.

> Meanwhile, it aims at fixup bad cell numbers for the memory controller, 
> whereas the original fixup_device_tree_maple() aiming at fixing up the 
> ISA controller on HT channel, we'd better separate them in different 
> function IMHO.

I think I agree it's better as a separate routine. We could have a
firmware that doesn't need the original maple fixup (and so exits from
that routine early) but does need this one.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: + edac-cpc925-mc-platform-device-setup.patch added to -mm tree

[Harry Ciao]

Michael Ellerman wrote:
> On Thu, 2009-04-16 at 09:57 +0800, Harry Ciao wrote:
>   
>> Kumar Gala wrote:
>>     
>>> On Apr 15, 2009, at 5:27 PM, akpm@linux-foundation.org wrote:
>>>       
>>>> arch/powerpc/kernel/prom_init.c      |   33 +++++++++++++
>>>> arch/powerpc/platforms/maple/setup.c |   59 +++++++++++++++++++++++++
>>>> 2 files changed, 92 insertions(+)
>>>>
>>>> diff -puN 
>>>> arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform-device-setup 
>>>> arch/powerpc/kernel/prom_init.c
>>>> --- 
>>>> a/arch/powerpc/kernel/prom_init.c~edac-cpc925-mc-platform-device-setup
>>>> +++ a/arch/powerpc/kernel/prom_init.c
>>>> @@ -1947,8 +1947,40 @@ static void __init fixup_device_tree_map
>>>>     prom_setprop(isa, name, "ranges",
>>>>             isa_ranges, sizeof(isa_ranges));
>>>> }
>>>> +
>>>> +#define CPC925_MC_START        0xf8000000
>>>> +#define CPC925_MC_LENGTH    0x1000000
>>>> +/* The values for memory-controller don't have right number of cells */
>>>> +static void __init fixup_device_tree_maple_memory_controller(void)
>>>> +{
>>>>         
>>> I don't see why this cant be part of the existing 
>>> fixup_device_tree_maple().
>>>
>>> I also find it odd we don't ensure we are running on a maple before we 
>>> apply this fixup.
>>>       
>
>   
>> Hi Kumar,
>>
>> Thanks a lot for your concern.
>>
>> This newly added fixup for memory controller on Maple will be placed 
>> right after fixup_device_tree_maple(), both of them will be controlled 
>> by CONFIG_PPC_MAPLE, so there is no worry that it will be applied 
>> against anything other than Maple.
>>     
>
> Hi Harry,
>
> We regularly build a single kernel with multiple platforms enabled, so
> just having it controlled by a CONFIG symbol is not sufficient. Someone
> might build a kernel for MAPLE & PSERIES & ISERIES & CELL, so the maple
> fixup needs to be careful it doesn't break the other platforms.
>
> The existing maple fixup doesn't check if it's on a maple either, but it
> is a bit more discerning about what it finds before it fixes things up.
>
> Your code already checks that "reg" is 8 bytes long to start with, I
> think if it also checks that #address-cells and #size-cells are == 2,
> then it's pretty safe. Because at that point we know we have a node with
> the right name, the reg property has a known value, and reg is short WRT
> #cells.
>
>   
Many thanks Michael!

That makes a lot of sense to me :) I will integrate the check if both 
its parent #address-cells and #size-cells equal to 2 before fixing 
anything up. The fact that the reg length == 8 bytes whereas parent's 
cells are 2 validate our fixup is necessary.

Best regards,

Harry


>> Meanwhile, it aims at fixup bad cell numbers for the memory controller, 
>> whereas the original fixup_device_tree_maple() aiming at fixing up the 
>> ISA controller on HT channel, we'd better separate them in different 
>> function IMHO.
>>     
>
> I think I agree it's better as a separate routine. We could have a
> firmware that doesn't need the original maple fixup (and so exits from
> that routine early) but does need this one.
>
> cheers
>
>