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

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

[akpm]

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)
+{
+	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

[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
>
>