[PATCH 1/2] powerpc/qman: Change fsl,qman-channel-id to cell-index

[Roy Pledge]

Sorry for digging up an old thread here Scott, but we never did close on this discussion.  See my replies inline below....

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, May 12, 2015 6:46 PM
> To: Pledge Roy-R01356
> Cc: linuxppc-dev at lists.ozlabs.org; devicetree at vger.kernel.org; Bucur
> Madalin-Cristian-B32716
> Subject: Re: [PATCH 1/2] powerpc/qman: Change fsl,qman-channel-id to cell-
> index
> 
> On Tue, 2015-05-12 at 16:19 -0500, Pledge Roy-R01356 wrote:
> > > >
> > > > I don't believe this is correct - let me explain the rational why
> > > > we had two
> > > properties in the QMan portal to begin with.
> > > >
> > > > The two properties in question are cell-index and fsl,qman-channel-id.
> > > >
> > > > The cell-index property is used in u-boot as an index for the
> > > > software portal
> > > ID when adding the fsl,liodn from the U-boot table into the device tree.
> > >
> > > The device tree is not supposed to contain arbitrary software identifiers.
> >
> > I agree - this is why the original device tree bindings removed
> > cell-index as it can be calculated.
>  Unfortunately u-boot relied on
> > this value being present so to be backward compatible we don't have a
> > way to remove it.  I'm not sure on what the procedure is to change
> > things u-boot relies on,
> 
> Generally the procedure is that we don't change it.  It wouldn't be so bad if
> using an old U-Boot just meant that datapath doesn't work with upstream
> kernels (since that doesn't work now), but a dts that makes existing U-Boot
> crash is another matter.

If this is true then we can never remove the cell-index property.  The cell-index in this case is referring to the portal index which could be calculated from the qman-portal at XXXX value.  My preference would be to eliminate cell-index and replace it with this calculation but that would mean older u-boot would fail to work with newer kernel.  While the bug that caused older u-boot to crash if this property is annoying this has been addressed in more recent u-boots. I can't comment on a policy where u-boot must always boot newer version of Linux - that means Linux will have to drag along baggage like this property for a long time (forever?). 

> > >
> > > > The  fsl,qman-channel-id property is used in Linux and corresponds
> > > > to a hardware value that indicates which channel is dedicated to
> > > > the software portal.
> > > >
> > > > While I'm not aware of a current SoC where the channel ID for a
> > > > software portal does not match the index (i.e. SWP 0 uses channel
> > > > 0,
> > > > etc.)
> > >
> > > Thus there's no backward compatibility issue with redefining
> > > cell-index to mean the channel ID.
> >
> > Channel IDs do change and are defined when the SoC is created
> 
> But for SoCs that already exist, they won't change, right?  We don't need to
> care about what existing U-Boot does on new SoCs, since U-Boot would
> need to be changed to support the new SoC at all.

This code isn't looking at SoC product numbers - the whole point of putting this in the device tree is to avoid doing just that.  If we had to add code for each SoC to u-boot we may as well get rid of the device tree and hardcode this configuration in the source file that is SoC specific.

> 
> >  (look at checks for QMan versions and adjustments for Pool Channel
> > IDs in the driver).  If the channel ID for portal 0 ever becomes non
> > zero we just end up having to make a mess in the code or reintroduce
> > this field.
> 
> What defines that portal as "portal 0"?

Portal 0 is portal 0 because it is at offset 0 in the QMan portal memory region.  Portal 1 is at 0x4000 etc...  Note that this is not the case for forthcoming ARM devices as portals are distributed at 64K intervals.  However since the device tree parsing code for ARM is separate from the PPC code this will not pose any issue.


> 
> > > > it is possible that future SoCs could stray from this model, there
> > > > is no reason for portal index to equal channel ID at all times.
> > >
> > > How can future SoCs dictate how we assign a software-defined
> identifier?
> > > If software wants it to be the same as the channel id, then it will be.
> > >
> > > If there is some aspect of the hardware itself (not the
> > > documentation) that cell-index currently corresponds to, other than
> > > the channel id, please make that clear.
> >
> > Channel ID is defined in the SoC RTL - it is not controlled by
> > software and it is not a software assigned identifier.  It is not
> > possible for SW to set these values.
> 
> I said "other than the channel id".  In particular, I was asking about the
> concept of "portal index".

The only thing cell-index indicates is the offset of the portal in the QMan address space.  Linux doesn't look at this value.  If we can get past the issue of old u-boots not working I would happily produce patches that remove this from u-boot and the device trees and start using unit-address for determining which portal is being indexed.  Since channel-id is a property of a portal I believe it should be left as is. If not I believe it is best to leave both values in place 

Roy