Re: [v0 PATCH 5/5] EDAC: CPC925 MC platform device setup

Re: [v0 PATCH 5/5] EDAC: CPC925 MC platform device setup

[Arnd Bergmann]

On Thursday 02 April 2009, Harry Ciao wrote:
> +#ifdef CONFIG_EDAC
> +#define CPC925_MC_START=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A00=
xf8000000
> +#define CPC925_MC_END=A0=A0=A0=A0=A0=A0=A0=A0=A0=A00xf8ffffff /* sizeof =
16MB */
> +/* Register a platform device for CPC925 memory controller */
> +static int __init maple_cpc925_edac_setup(void)

It's not good to have these encoded as magic numbers.
Can't you find the addresses in the device tree? Maybe it's
even possible to make this an of_platform_driver if you
find a good node to bind to.

Does the driver also work on a G5 Mac, or is it limited
to the maple platform?

	Arnd <><

Re: [v0 PATCH 2/5] EDAC: Add CPC925 driver source file

[Arnd Bergmann]

On Thursday 02 April 2009, Harry Ciao wrote:
> Introduce IBM CPC925 EDAC driver source file, which makes use of error
> detections on the IBM CPC925 Bridge and Memory Controller.
> 
> Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>

On a very brief review, the driver looks good to me, but please
post the full series to the linuxppc-dev@ozlabs.org mailing list
as well, because that is likely to have more people that will
comment on it.

The first three patches of your series should be merged into
a single one, because none of them has any signficance by itself.
You may also consider moving the contents of the header file
into the main driver, because it does not actually act as an
interface.

	Arnd <><

Re: [v0 PATCH 5/5] EDAC: CPC925 MC platform device setup

[Harry Ciao]

Arnd Bergmann 写道:
> On Thursday 02 April 2009, Harry Ciao wrote:
>   
>> +#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)
>>     
>
> It's not good to have these encoded as magic numbers.
> Can't you find the addresses in the device tree? Maybe it's
> even possible to make this an of_platform_driver if you
> find a good node to bind to.
>
> Does the driver also work on a G5 Mac, or is it limited
> to the maple platform?
>
> 	Arnd <><
>
>  
Hi Arnd,

I did try to get resource information from its DTB node first, the 
"hostbridge" node do can be found successfully, however, unfortunately, 
as I put in the notes in that function, the #address-cells and 
#size-cells specified by its parent node are both 2, but the cell number 
used in its "reg" property is actually 1, as

reg = <0xf8000000 0x1000000>;

which will make of_address_to_resource() failed with -EINVAL; that's why 
I have to set it up from scratch manually.

Cheers,

Harry

Re: [v0 PATCH 5/5] EDAC: CPC925 MC platform device setup

[Harry Ciao]

Harry Ciao 写道:
> Arnd Bergmann 写道:
>> On Thursday 02 April 2009, Harry Ciao wrote:
>>  
>>> +#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)
>>>     
>>
>> It's not good to have these encoded as magic numbers.
>> Can't you find the addresses in the device tree? Maybe it's
>> even possible to make this an of_platform_driver if you
>> find a good node to bind to.
>>
>> Does the driver also work on a G5 Mac, or is it limited
>> to the maple platform?
>>
>>     Arnd <><
>>
>>  
Hi Arnd,

I forgot to mention that so far I have tested this CPC925 driver on the 
Maple platform, I have no G5 Mac to test with.
CPC925 is specific to the PowerPC 970 family of processors, but I don't 
think Maple is the only platform that ever use PowerPC 970 
processors(that is why this driver should depend on PP64, rather than 
PPC_MAPLE).

Thanks,

Harry

> Hi Arnd,
>
> I did try to get resource information from its DTB node first, the 
> "hostbridge" node do can be found successfully, however, 
> unfortunately, as I put in the notes in that function, the 
> #address-cells and #size-cells specified by its parent node are both 
> 2, but the cell number used in its "reg" property is actually 1, as
>
> reg = <0xf8000000 0x1000000>;
>
> which will make of_address_to_resource() failed with -EINVAL; that's 
> why I have to set it up from scratch manually.
>
> Cheers,
>
> Harry
>
>
>