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

[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@xxxxxxxxxxxxx>
> ---
>  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: signature.asc
Description: This is a digitally signed message part