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
------------------------------------------------------
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
