Messages in this thread | | | Date | Wed, 15 Jul 2009 12:52:49 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 2/4] edac: mpc85xx add mpc83xx support |
| |
On Wed, 15 Jul 2009 11:38:49 -0600 dougthompson@xmission.com wrote:
> > Add support for the Freescale MPC83xx memory controller to the existing > driver for the Freescale MPC85xx memory controller. The only difference > between the two processors are in the CS_BNDS register parsing code, which > has been changed so it will work on both processors. > > The L2 cache controller does not exist on the MPC83xx, but the OF subsystem > will not use the driver if the device is not present in the OF device tree. > > > Kumar, I had to change the nr_pages calculation to make the math work > out. I checked it on my board and did the math by hand for a 64GB 85xx > using 64K pages. In both cases, nr_pages * PAGE_SIZE comes out to the > correct value. Thanks for the help. > > v1 -> v2: > * Use PAGE_SHIFT to parse cs_bnds regardless of board type > * Remove special-casing for the 83xx processor > > ... > > @@ -789,19 +791,20 @@ static void __devinit mpc85xx_init_csrow > csrow = &mci->csrows[index]; > cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 + > (index * MPC85XX_MC_CS_BNDS_OFS)); > - start = (cs_bnds & 0xfff0000) << 4; > - end = ((cs_bnds & 0xfff) << 20); > - if (start) > - start |= 0xfffff; > - if (end) > - end |= 0xfffff; > + > + start = (cs_bnds & 0xffff0000) >> 16; > + end = (cs_bnds & 0x0000ffff); > > if (start == end) > continue; /* not populated */ > > + start <<= (24 - PAGE_SHIFT); > + end <<= (24 - PAGE_SHIFT); > + end |= (1 << (24 - PAGE_SHIFT)) - 1;
<stares for a while>
That looks like the original code was really really wrong.
The setting of all the lower bits in `end' is funny-looking. What's happening here? Should it be commented?
> csrow->first_page = start >> PAGE_SHIFT; > csrow->last_page = end >> PAGE_SHIFT; > - csrow->nr_pages = csrow->last_page + 1 - csrow->first_page; > + csrow->nr_pages = end + 1 - start; > csrow->grain = 8; > csrow->mtype = mtype; > csrow->dtype = DEV_UNKNOWN; > @@ -985,6 +988,7 @@ static struct of_device_id mpc85xx_mc_er > { .compatible = "fsl,mpc8560-memory-controller", }, > { .compatible = "fsl,mpc8568-memory-controller", }, > { .compatible = "fsl,mpc8572-memory-controller", }, > + { .compatible = "fsl,mpc8349-memory-controller", }, > { .compatible = "fsl,p2020-memory-controller", }, > {}, > }; > @@ -1001,13 +1005,13 @@ static struct of_platform_driver mpc85xx > }, > }; > > - > +#ifdef CONFIG_MPC85xx > static void __init mpc85xx_mc_clear_rfxe(void *data) > { > orig_hid1[smp_processor_id()] = mfspr(SPRN_HID1); > mtspr(SPRN_HID1, (orig_hid1[smp_processor_id()] & ~0x20000)); > } > - > +#endif > > static int __init mpc85xx_mc_init(void) > { > @@ -1040,26 +1044,32 @@ static int __init mpc85xx_mc_init(void) > printk(KERN_WARNING EDAC_MOD_STR "PCI fails to register\n"); > #endif > > +#ifdef CONFIG_MPC85xx > /* > * need to clear HID1[RFXE] to disable machine check int > * so we can catch it > */ > if (edac_op_state == EDAC_OPSTATE_INT) > on_each_cpu(mpc85xx_mc_clear_rfxe, NULL, 0); > +#endif > > return 0; > }
The patch adds lots of ifdefs :(
> module_init(mpc85xx_mc_init); > > +#ifdef CONFIG_MPC85xx > static void __exit mpc85xx_mc_restore_hid1(void *data) > { > mtspr(SPRN_HID1, orig_hid1[smp_processor_id()]); > } > +#endif
afacit this will run smp_processor_id() from within preemptible code, which is often buggy on preemptible kernels and will cause runtime warnings on at least some architectures.
> static void __exit mpc85xx_mc_exit(void) > { > +#ifdef CONFIG_MPC85xx > on_each_cpu(mpc85xx_mc_restore_hid1, NULL, 0); > +#endif > #ifdef CONFIG_PCI > of_unregister_platform_driver(&mpc85xx_pci_err_driver); > #endif
| |