[v1 PATCH 3/3] EDAC: CPC925 MC platform device setup

[v1 PATCH 3/3] EDAC: CPC925 MC platform device setup

[Harry Ciao]

Hi,

Sorry, my git-send-mail died and failed to send out 3/3 patch.

Harry

[v1 PATCH 3/3] EDAC: CPC925 MC platform device setup

[Harry Ciao]

Setup a platform device for the CPC925 Memory Controller during system
booting up, against which CPC925 MC EDAC driver would be matched.

Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
---
 arch/powerpc/platforms/maple/setup.c |   47 ++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
index bfd60e4..ca8a3ff 100644
--- a/arch/powerpc/platforms/maple/setup.c
+++ b/arch/powerpc/platforms/maple/setup.c
@@ -335,3 +335,50 @@ define_machine(maple) {
 	.progress		= maple_progress,
 	.power_save		= power4_idle,
 };
+
+#ifdef CONFIG_EDAC
+#define CPC925_MC_START		0xf8000000
+#define CPC925_MC_END		0xf8ffffff /* sizeof 16MB */
+/* Register a platform device for CPC925 memory controller */
+static int __init maple_cpc925_edac_setup(void)
+{
+	struct platform_device *pdev;
+	struct device_node *np = NULL;
+	struct resource r;
+
+	np = of_find_node_by_name(NULL, "hostbridge");
+	if (!np) {
+		printk(KERN_ERR "%s: Unable to find hostbridge node\n",
+			__func__);
+		return -ENODEV;
+	}
+
+	if (of_address_to_resource(np, 0, &r)) {
+		/*
+		 * of_address_to_resource() would get the #address-cells
+		 * and #size-cells properties for a node from its parent.
+		 * On Maple 64bit target hostbridge parent node has specified
+		 * both of these two properties to be 2. However, the actual
+		 * "cell" value for host bridge node is 1. Since we can't
+		 * fix this firmware-generated DTB, we have to setup a
+		 * resource structure manually.
+		 */
+		memset(&r, 0, sizeof(r));
+		r.start = CPC925_MC_START;
+		r.end = CPC925_MC_END;
+		r.name = "hostbridge";
+		r.flags = IORESOURCE_MEM;
+	}
+
+	of_node_put(np);
+
+	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;
+}
+arch_initcall(maple_cpc925_edac_setup);
+#endif
-- 
1.5.6.2

Re: [v1 PATCH 3/3] EDAC: CPC925 MC platform device setup

[Michael Ellerman]

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

On Fri, 2009-04-03 at 14:10 +0800, Harry Ciao wrote:
> Setup a platform device for the CPC925 Memory Controller during system
> booting up, against which CPC925 MC EDAC driver would be matched.
> 
> Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
> ---
>  arch/powerpc/platforms/maple/setup.c |   47 ++++++++++++++++++++++++++++++++++
>  1 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
> index bfd60e4..ca8a3ff 100644
> --- a/arch/powerpc/platforms/maple/setup.c
> +++ b/arch/powerpc/platforms/maple/setup.c
> @@ -335,3 +335,50 @@ define_machine(maple) {
>  	.progress		= maple_progress,
>  	.power_save		= power4_idle,
>  };
> +
> +#ifdef CONFIG_EDAC
> +#define CPC925_MC_START		0xf8000000
> +#define CPC925_MC_END		0xf8ffffff /* sizeof 16MB */
> +/* Register a platform device for CPC925 memory controller */
> +static int __init maple_cpc925_edac_setup(void)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *np = NULL;
> +	struct resource r;
> +
> +	np = of_find_node_by_name(NULL, "hostbridge");

This is way too generic IMHO. You're going to match on any node called
"hostbridge" on any 64-bit powerpc machine, there's a good chance there
are other firmwares out there with such a node, or will be in the
future.

Is there no compatible or model property you can look for, so you're at
least somewhat confident you're actually looking at a CPC925?

If not this should be a machine_initcall() for Maple only.

> +	if (of_address_to_resource(np, 0, &r)) {
> +		/*
> +		 * of_address_to_resource() would get the #address-cells
> +		 * and #size-cells properties for a node from its parent.
> +		 * On Maple 64bit target hostbridge parent node has specified
> +		 * both of these two properties to be 2. However, the actual
> +		 * "cell" value for host bridge node is 1. Since we can't
> +		 * fix this firmware-generated DTB, we have to setup a
> +		 * resource structure manually.
> +		 */
> +		memset(&r, 0, sizeof(r));
> +		r.start = CPC925_MC_START;
> +		r.end = CPC925_MC_END;
> +		r.name = "hostbridge";
> +		r.flags = IORESOURCE_MEM;

We do have fixups for bogus firmware including maple, look in
prom_init.c:fixup_device_tree_maple(). Although that's for actual
firmware, not a DTB.

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: [v1 PATCH 3/3] EDAC: CPC925 MC platform device setup

[Harry Ciao]

Michael Ellerman 写道:
> On Fri, 2009-04-03 at 14:10 +0800, Harry Ciao wrote:
>   
>> Setup a platform device for the CPC925 Memory Controller during system
>> booting up, against which CPC925 MC EDAC driver would be matched.
>>
>> Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
>> ---
>>  arch/powerpc/platforms/maple/setup.c |   47 ++++++++++++++++++++++++++++++++++
>>  1 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
>> index bfd60e4..ca8a3ff 100644
>> --- a/arch/powerpc/platforms/maple/setup.c
>> +++ b/arch/powerpc/platforms/maple/setup.c
>> @@ -335,3 +335,50 @@ define_machine(maple) {
>>  	.progress		= maple_progress,
>>  	.power_save		= power4_idle,
>>  };
>> +
>> +#ifdef CONFIG_EDAC
>> +#define CPC925_MC_START		0xf8000000
>> +#define CPC925_MC_END		0xf8ffffff /* sizeof 16MB */
>> +/* Register a platform device for CPC925 memory controller */
>> +static int __init maple_cpc925_edac_setup(void)
>> +{
>> +	struct platform_device *pdev;
>> +	struct device_node *np = NULL;
>> +	struct resource r;
>> +
>> +	np = of_find_node_by_name(NULL, "hostbridge");
>>     
>
> This is way too generic IMHO. You're going to match on any node called
> "hostbridge" on any 64-bit powerpc machine, there's a good chance there
> are other firmwares out there with such a node, or will be in the
> future.
>
> Is there no compatible or model property you can look for, so you're at
> least somewhat confident you're actually looking at a CPC925?
>
> If not this should be a machine_initcall() for Maple only.
>
>   
>> +	if (of_address_to_resource(np, 0, &r)) {
>> +		/*
>> +		 * of_address_to_resource() would get the #address-cells
>> +		 * and #size-cells properties for a node from its parent.
>> +		 * On Maple 64bit target hostbridge parent node has specified
>> +		 * both of these two properties to be 2. However, the actual
>> +		 * "cell" value for host bridge node is 1. Since we can't
>> +		 * fix this firmware-generated DTB, we have to setup a
>> +		 * resource structure manually.
>> +		 */
>> +		memset(&r, 0, sizeof(r));
>> +		r.start = CPC925_MC_START;
>> +		r.end = CPC925_MC_END;
>> +		r.name = "hostbridge";
>> +		r.flags = IORESOURCE_MEM;
>>     
>
> We do have fixups for bogus firmware including maple, look in
> prom_init.c:fixup_device_tree_maple(). Although that's for actual
> firmware, not a DTB.
>
> cheers
>
>   
Hi Michael,

Thanks a lot for your comments! I can see it would be much better than
mine. Next whole week I will be on vacation, I will give some serious
thoughts about your ideas and improve my patch in v2 after I come back.

Best regards,

Harry