Regards,
Igal Liberman
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, October 28, 2015 11:31 PM
> To: Liberman Igal-B31950 <igal.liber...@freescale.com>
> Cc: netdev@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> ker...@vger.kernel.org; Bucur Madalin-Cristian-B32716
> <madalin.bu...@freescale.com>
> Subject: Re: [V5, 2/6] fsl/fman: Add FMan support
>
> On Tue, 2015-10-27 at 11:32 -0500, Liberman Igal-B31950 wrote:
>
> > > > +
> > > > +struct device *fman_get_device(struct fman *fman) { return
> > > > +fman->dev; }
> > >
> > > Is this really necessary?
> > >
> >
> > Fman port needs fman->dev, fman structure is opaque, so yes, it's needed.
>
> Why is opacity being maintained from one part of the fman driver to
> another?
> Isn't this the sort of excessive layering that was complained about?
>
>
It's not really layering.
Fman Port uses Fman resources, it's not completely standalone.
> > > > + /* In B4 rev 2.0 (and above) the MURAM size is 512KB.
> > > > + * Check the SVR and update MURAM size if required.
> > > > + */
> > > > + u32 svr;
> > > > +
> > > > + svr = mfspr(SPRN_SVR);
> > > > +
> > > > + if ((SVR_SOC_VER(svr) == SVR_B4860) && (SVR_MAJ(svr) >=
> > > 2))
> > > > + fman->dts_params.muram_size = 0x80000; }
> > >
> > > Why wasn't the MURAM size described in the device tree, as it was
> > > with CPM/QE?
> > >
> >
> > MURAM size described by the device-tree.
> > In B4860 rev 2.0 (and above) MURAM size is bigger.
> > This is workaround, in order to have the same device tree for all
> > B4860 revisions.
>
> We don't support b4860 prior to rev 2.0 (due to e6500 core errata) so this is
> irrelevant. Fix the device tree.
>
OK, thanks.
I'll submit a new device tree patch (on top of the existing patches, which are
awaiting upstream) and remove this code from Fman.
> > > > +
> > > > + of_node_put(muram_node);
> > > > + of_node_put(fm_node);
> > > > +
> > > > + err = devm_request_irq(&of_dev->dev, irq, fman_irq,
> > > > + IRQF_NO_SUSPEND, "fman", fman); if (err <
> > > > + 0) {
> > > > + pr_err("Error: allocating irq %d (error = %d)\n", irq, err);
> > > > + goto fman_free;
> > > > + }
> > >
> > > Why IRQF_NO_SUSPEND?
> > >
> >
> > It shouldn't be IRQF_NO_SUSPEND for now, removed.
>
> Why just "for now"?
>
Unsuccessful wording, sorry.
> -Scott
