Ira or Kumar,
can you address Andrew's concerns below and what was posted in prior posts on
this?
thanks
doug t
--- On Wed, 7/15/09, Andrew Morton <a...@linux-foundation.org> wrote:
> From: Andrew Morton <a...@linux-foundation.org>
> Subject: Re: [PATCH 2/4] edac: mpc85xx add mpc83xx support
> To: dougthomp...@xmission.com
> Cc: bluesmoke-de...@lists.sourceforge.net, linux-ker...@vger.kernel.org
> Date: Wednesday, July 15, 2009, 1:52 PM
> On Wed, 15 Jul 2009 11:38:49 -0600
> dougthomp...@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
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
