diff mbox

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

Message ID BN3PR0301MB126796C0EC18EEBB26702C588CD10@BN3PR0301MB1267.namprd03.prod.outlook.com (mailing list archive)
State Not Applicable
Delegated to: Scott Wood
Headers show

Commit Message

Roy Pledge May 5, 2015, 4:04 p.m. UTC
Sorry for the slow reply.

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  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.) 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.

Roy


-----Original Message-----
From: Linuxppc-dev [mailto:linuxppc-dev-bounces+roy.pledge=freescale.com@lists.ozlabs.org] On Behalf Of Scott Wood

Sent: Friday, April 17, 2015 6:53 PM
To: linuxppc-dev@lists.ozlabs.org
Cc: Wood Scott-B07421; devicetree@vger.kernel.org; Bucur Madalin-Cristian-B32716
Subject: [PATCH 1/2] powerpc/qman: Change fsl,qman-channel-id to cell-index

It turns out that existing U-Boots will dereference NULL pointers if the device tree does not have cell-index in the portal nodes.

No patch has yet been merged adding device tree nodes for this binding (except a dtsi that has not yet been referenced), nor has any driver yet been merged making use of the binding, so it's not too late to change the binding in order to keep compatibility with existing U-Boots.

Signed-off-by: Scott Wood <scottwood@freescale.com>

Cc: Madalin-Cristian Bucur <madalin.bucur@freescale.com>
---
 Documentation/devicetree/bindings/soc/fsl/qman-portals.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
2.1.0

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Comments

Scott Wood May 5, 2015, 8:45 p.m. UTC | #1
On Tue, 2015-05-05 at 11:04 -0500, Pledge Roy-R01356 wrote:
> Sorry for the slow reply.
> 
> 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.

> 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.

> 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.

-Scott
Roy Pledge May 12, 2015, 9:19 p.m. UTC | #2
> >

> > 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, I personal have always been very uncomfortable with the coupling between u-boot and Linux for things like this. 

> 

> > 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 (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.

> 

> > 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.

Roy
Scott Wood May 12, 2015, 10:46 p.m. UTC | #3
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.

> I personal have always been very uncomfortable with the coupling
> between u-boot and Linux for things like this. 

Same here.  I've said for a while that I thought the dtses should live
in the U-Boot tree due to such coupling (the dtb/kernel interface has
well-defined binding documents; the dts/U-Boot interface doesn't), but
nobody else seemed interested.  It would also be good to minimize new
U-Boot fixups in favor of having the kernel do it, even if it results
in unpleasant code duplication.


> > 
> > > 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.

>  (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"?

> > > 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".

-Scott
Roy Pledge Aug. 19, 2015, 8:52 p.m. UTC | #4
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@lists.ozlabs.org; devicetree@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@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
Scott Wood Aug. 19, 2015, 9:30 p.m. UTC | #5
On Wed, Aug 19, 2015 at 03:52:55PM -0500, Pledge Roy-R01356 wrote:
> 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@lists.ozlabs.org; devicetree@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.

Correct.

> The cell-index in this case is referring to the portal index which
> could be calculated from the qman-portal@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 baggage isn't particularly onerous.  It's just using a suboptimal
property name.  The semantics are exactly the same as
fsl,qman-channel-id.

> > > >
> > > > > 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.

The point of putting this in the device tree is to avoid per-SoC code in
*Linux*.  U-Boot does use the device tree for similar purposes on some
platforms, but that's not something we've done yet.  I'm not sure how
it's relevant, though.  How would it be different if we had
fsl,qman-channel-id and no cell-index?

When I mentioned U-Boot needing to be updated for new SoCs, I meant to
run in general, not specifically the QMan code.  So, needing to be
compatible with existing QMan code is only an issue for SoCs where an
older U-Boot actually exists.  We don't need to care what the old code
would have done on newer SoCs.

> > >  (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.

I don't understand why it would cause an issue in any case.  U-Boot would
need to either know the mapping from portal address to channel id, or get
it from the device tree.  There's no need to introduce the concept of
"portal number" except perhaps as some internal implementation detail.

> 
> > 
> > > > > 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.

cell-index has been redefined to not mean that at all.  It now only means
channel ID.  We can do this because the value happens to be the same for
all existing SoCs (and we should be sure to avoid putting things into the
SDK for the aforementioned ARM chips that are contrary to the new
definition).

-Scott
Roy Pledge Aug. 20, 2015, 2:52 p.m. UTC | #6
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, August 19, 2015 5:30 PM
> To: Pledge Roy-R01356
> Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; Bucur
> Madalin-Cristian-B32716; Wang Haiying-R54964
> Subject: Re: [PATCH 1/2] powerpc/qman: Change fsl,qman-channel-id to cell-
> index
> 
> On Wed, Aug 19, 2015 at 03:52:55PM -0500, Pledge Roy-R01356 wrote:
> > 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@lists.ozlabs.org; devicetree@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.
> 
> Correct.
> 
> > The cell-index in this case is referring to the portal index which
> > could be calculated from the qman-portal@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 baggage isn't particularly onerous.  It's just using a suboptimal property
> name.  The semantics are exactly the same as fsl,qman-channel-id.
> 
> > > > >
> > > > > > 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.
> 
> The point of putting this in the device tree is to avoid per-SoC code in
> *Linux*.  U-Boot does use the device tree for similar purposes on some
> platforms, but that's not something we've done yet.  I'm not sure how it's
> relevant, though.  How would it be different if we had fsl,qman-channel-id
> and no cell-index?

I guess my point isn't getting through - channel-id and cell-index are too independent concepts that are coincidentally the same. Cell-index is only used by u-boot and is used to determine the portal number.  It is absolutely possible that we produce and SoC where portal number 0 has channel 0x1000.  We've changed these things in the past as well - this is why we introduced the channel-id property.  This is also consistent with other blocks (FMan, PME, DCE) that have channels associated with them.  

The cell-index is useless - the portal ID can be derived from the portal address.  The fact that you're unwilling to  remove cell-index doesn't mean that channel-id is redundant and should be removed.


> 
> When I mentioned U-Boot needing to be updated for new SoCs, I meant to
> run in general, not specifically the QMan code.  So, needing to be compatible
> with existing QMan code is only an issue for SoCs where an older U-Boot
> actually exists.  We don't need to care what the old code would have done
> on newer SoCs.
> 
> > > >  (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.
> 
> I don't understand why it would cause an issue in any case.  U-Boot would
> need to either know the mapping from portal address to channel id, or get it
> from the device tree.  There's no need to introduce the concept of "portal
> number" except perhaps as some internal implementation detail.
> 
> >
> > >
> > > > > > 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.
> 
> cell-index has been redefined to not mean that at all.  It now only means
> channel ID.  We can do this because the value happens to be the same for all
> existing SoCs (and we should be sure to avoid putting things into the SDK for
> the aforementioned ARM chips that are contrary to the new definition).
> 

U-boot doesn't dictate the HW architecture and shouldn't - you're trying very hard not to admit you changed something you didn't fully understand and actually making the system harder to deal with.  At no point in time have we ever assumed portal ID would equal channel id.  With your logic u-boot is broken because it is using channel ID to index into an array that is per portal not per channel.  We shouldn't have removed channel-id in the first place - trying to redefine that portals = channels is just plain incorrect.  If we can't remove cell-index then we have to live with that - trying to defend removing channel-id because for some cases portal-id = channel-id is wrong.  The fact that someone though cell-index was a good idea in the first place is the error - that data already exists in the device tree in the offset field.  We need channel-id to be consistent with other device tree nodes and because portal-id != cell-index.  I'm not sure how else to get through to you on this, your base assumption is wrong.

> -Scott
Scott Wood Aug. 20, 2015, 4:53 p.m. UTC | #7
On Thu, Aug 20, 2015 at 09:52:46AM -0500, Pledge Roy-R01356 wrote:
> I guess my point isn't getting through - channel-id and cell-index are
> too independent concepts that are coincidentally the same.  Cell-index
> is only used by u-boot and is used to determine the portal number.  It
> is absolutely possible that we produce and SoC where portal number 0
> has channel 0x1000.  We've changed these things in the past as well -
> this is why we introduced the channel-id property.  This is also
> consistent with other blocks (FMan, PME, DCE) that have channels
> associated with them.
> 
> The cell-index is useless - the portal ID can be derived from the
> portal address.  The fact that you're unwilling to remove cell-index
> doesn't mean that channel-id is redundant and should be removed.

I guess my point isn't getting through.  Any concept that cell-index
represented that was not identical to channel id is dead and gone, as is
the concept of "portal number".  Every time you see cell-index just
pretend you are seeing fsl,qman-channel-id.

cell-index is *not* defined as anything to do with the portal address.

> > > The only thing cell-index indicates is the offset of the portal in the
> > > QMan address space.
> > 
> > cell-index has been redefined to not mean that at all.  It now only means
> > channel ID.  We can do this because the value happens to be the same for all
> > existing SoCs (and we should be sure to avoid putting things into the SDK for
> > the aforementioned ARM chips that are contrary to the new definition).
> > 
> 
> U-boot doesn't dictate the HW architecture and shouldn't - you're
> trying very hard not to admit you changed something you didn't fully
> understand and actually making the system harder to deal with.  At no
> point in time have we ever assumed portal ID would equal channel id. 

cell-index does not mean "portal ID".

> With your logic u-boot is broken because it is using channel ID to
> index into an array that is per portal not per channel.  We shouldn't
> have removed channel-id in the first place - trying to redefine that
> portals = channels is just plain incorrect.

That's not what happened.

> I'm not sure how else to get through to you on this, your base
> assumption is wrong.

Likewise.

-Scott
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/soc/fsl/qman-portals.txt b/Documentation/devicetree/bindings/soc/fsl/qman-portals.txt
index 48c4dae..47e46cc 100644
--- a/Documentation/devicetree/bindings/soc/fsl/qman-portals.txt
+++ b/Documentation/devicetree/bindings/soc/fsl/qman-portals.txt
@@ -47,7 +47,7 @@  PROPERTIES
 
 	For additional details about the PAMU/LIODN binding(s) see pamu.txt
 
-- fsl,qman-channel-id
+- cell-index
 	Usage:		Required
 	Value type:	<u32>
 	Definition:	The hardware index of the channel. This can also be
@@ -136,7 +136,7 @@  The example below shows a (P4080) QMan portals container/bus node with two porta
 			reg = <0x4000 0x4000>, <0x101000 0x1000>;
 			interrupts = <106 2 0 0>;
 			fsl,liodn = <3 4>;
-			fsl,qman-channel-id = <1>;
+			cell-index = <1>;
 
 			fman0 {
 				fsl,liodn = <0x22>;