Re: [PATCH 2/2] powerpc: MPC85xx EDAC device driver

[Dave Jiang <djiang@mvista.com>]

Arnd Bergmann wrote:
> On Friday 27 July 2007, Dave Jiang wrote:
>> +static struct of_device_id mpc85xx_pci_err_of_match[] = {
>> +       {
>> +        .type = "pci",
>> +        .compatible = "fsl,mpc8540-pci",
>> +        },
>> +       {},
>> +};
>> +
>> +static struct of_platform_driver mpc85xx_pci_err_driver = {
>> +       .owner = THIS_MODULE,
>> +       .name = "mpc85xx_pci_err",
>> +       .match_table = mpc85xx_pci_err_of_match,
>> +       .probe = mpc85xx_pci_err_probe,
>> +       .remove = mpc85xx_pci_err_remove,
>> +       .driver = {
>> +                  .name = "mpc85xx_pci_err",
>> +                  .owner = THIS_MODULE,
>> +                  },
>> +};
> 
> This is  a little problematic, if we want to make the PCI bus implementation
> use the PCI code from arch/powerpc/kernel/of_platform.c in the future.
> Right now this is not possible, because that code is still 64-bit only,
> but that may change in the future. Since only one driver can bind
> to the pci host bridge device, the mpc85xx_pci_err driver would conflict
> with the PCI driver itself, which you probably don't intend.
> 
> I'd suggest either to integrate EDAC into the 85xx specific PCI code,
> or to have an extra device in the device tree for this.

How about I create a platform device just for EDAC and leave the PCI of_device
to the 85xx PCI code? That would be a lot less modification than adding a
device for every PCI hose per DTS file.... Just a thought....

>> +       res = of_register_platform_driver(&mpc85xx_mc_err_driver) ? : res;
>> +
>> +       res = of_register_platform_driver(&mpc85xx_l2_err_driver) ? : res;
>> +
>> +#ifdef CONFIG_PCI
>> +       res = of_register_platform_driver(&mpc85xx_pci_err_driver) ? : res;
>> +#endif
>> +
>> +       /*
>> +        * need to clear HID1[RFXE] to disable machine check int
>> +        * so we can catch it
>> +        */
>> +       if (edac_op_state == EDAC_OPSTATE_INT) {
>> +               orig_hid1 = mfspr(SPRN_HID1);
>> +
>> +               mtspr(SPRN_HID1, (orig_hid1 & ~0x20000));
>> +       }
>> +
>> +       return res;
>> +}
> 
> The error handling could use some improvement here. In particular, you should
> unregister the buses in the failure path, maybe you need to clean up other
> parts as well.

I think I want individual "devices" work even some may fail or be missing. For
example, even if L2 fails to register, I still want to be able to get the
memory controller to report errors. So I really don't want to unregister
everything that initialized properly even though some failures exists. Maybe I
need to clean it up a little bit.

-- 

------------------------------------------------------
Dave Jiang
Software Engineer
MontaVista Software, Inc.
http://www.mvista.com
------------------------------------------------------