RE: [V5, 2/6] fsl/fman: Add FMan support

[Liberman Igal]

Regards,
Igal Liberman

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, October 28, 2015 11:31 PM
> To: Liberman Igal-B31950 <Igal.Liberman@xxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Bucur Madalin-Cristian-B32716
> <madalin.bucur@xxxxxxxxxxxxx>
> 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