* [PATCH v2 0/2] Add support to handle ZRX-DC Compliant PHYs [not found] <CGME20191028121704epcas5p483bf05ccb4cd25b1757cd5645e819d12@epcas5p4.samsung.com> @ 2019-10-28 12:16 ` Anvesh Salveru [not found] ` <CGME20191028121748epcas5p3054c9583c14a2edde9f725d005895a04@epcas5p3.samsung.com> [not found] ` <CGME20191028121758epcas5p2dda6d0842be32bcab2e6025fac1f3e78@epcas5p2.samsung.com> 0 siblings, 2 replies; 8+ messages in thread From: Anvesh Salveru @ 2019-10-28 12:16 UTC (permalink / raw) To: linux-pci, devicetree, linux-kernel Cc: bhelgaas, gustavo.pimentel, jingoohan1, pankaj.dubey, Anvesh Salveru According the PCI Express base specification when PHY does not meet ZRX-DC specification, after every 100ms timeout the link should transition to recovery state when the link is in low power states. Ports that meet the ZRX-DC specification for 2.5 GT/s while in the L1.Idle state and are therefore not required to implement the 100 ms timeout and transition to Recovery should avoid implementing it, since it will reduce the power savings expected from the L1 state. DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY. Anvesh Salveru (2): dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property PCI: dwc: Add support to handle ZRX-DC Compliant PHYs Documentation/devicetree/bindings/pci/designware-pcie.txt | 2 ++ drivers/pci/controller/dwc/pcie-designware.c | 7 +++++++ drivers/pci/controller/dwc/pcie-designware.h | 3 +++ 3 files changed, 12 insertions(+) -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CGME20191028121748epcas5p3054c9583c14a2edde9f725d005895a04@epcas5p3.samsung.com>]
* [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property [not found] ` <CGME20191028121748epcas5p3054c9583c14a2edde9f725d005895a04@epcas5p3.samsung.com> @ 2019-10-28 12:16 ` Anvesh Salveru 2019-11-05 21:53 ` Rob Herring 0 siblings, 1 reply; 8+ messages in thread From: Anvesh Salveru @ 2019-10-28 12:16 UTC (permalink / raw) To: linux-pci, devicetree, linux-kernel Cc: bhelgaas, gustavo.pimentel, jingoohan1, pankaj.dubey, Anvesh Salveru, Rob Herring, Mark Rutland Add support for ZRX-DC compliant PHYs. If PHY is not compliant to ZRX-DC specification, then after every 100ms link should transition to recovery state during the low power states which increases power consumption. Platforms with ZRX-DC compliant PHY can use "snps,phy-zrxdc-compliant" property in DesignWare controller DT node. CC: Rob Herring <robh+dt@kernel.org> CC: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> --- Change in v2: None Documentation/devicetree/bindings/pci/designware-pcie.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt index 78494c4050f7..9507ac38ac89 100644 --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt @@ -38,6 +38,8 @@ Optional properties: for data corruption. CDM registers include standard PCIe configuration space registers, Port Logic registers, DMA and iATU (internal Address Translation Unit) registers. +- snps,phy-zrxdc-compliant: This property is needed if phy complies with the + ZRX-DC specification. RC mode: - num-viewport: number of view ports configured in hardware. If a platform does not specify it, the driver assumes 2. -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property 2019-10-28 12:16 ` [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property Anvesh Salveru @ 2019-11-05 21:53 ` Rob Herring 2019-11-06 9:53 ` Andrew Murray 0 siblings, 1 reply; 8+ messages in thread From: Rob Herring @ 2019-11-05 21:53 UTC (permalink / raw) To: Anvesh Salveru Cc: linux-pci, devicetree, linux-kernel, bhelgaas, gustavo.pimentel, jingoohan1, pankaj.dubey, Mark Rutland On Mon, Oct 28, 2019 at 05:46:27PM +0530, Anvesh Salveru wrote: > Add support for ZRX-DC compliant PHYs. If PHY is not compliant to ZRX-DC > specification, then after every 100ms link should transition to recovery > state during the low power states which increases power consumption. > > Platforms with ZRX-DC compliant PHY can use "snps,phy-zrxdc-compliant" > property in DesignWare controller DT node. > > CC: Rob Herring <robh+dt@kernel.org> > CC: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > --- > Change in v2: None > > Documentation/devicetree/bindings/pci/designware-pcie.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > index 78494c4050f7..9507ac38ac89 100644 > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > @@ -38,6 +38,8 @@ Optional properties: > for data corruption. CDM registers include standard PCIe configuration > space registers, Port Logic registers, DMA and iATU (internal Address > Translation Unit) registers. > +- snps,phy-zrxdc-compliant: This property is needed if phy complies with the > + ZRX-DC specification. If this is a property of the phy, then it belongs in the phy node or should just be implied by the phy's compatible. IOW, you should be able to support this or not without changing DTs. Is this spec Synopys specific? (About the only thing Google turns up are your patches.) If not, then probably shouldn't have a 'snps' prefix. > RC mode: > - num-viewport: number of view ports configured in hardware. If a platform > does not specify it, the driver assumes 2. > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property 2019-11-05 21:53 ` Rob Herring @ 2019-11-06 9:53 ` Andrew Murray 2019-11-08 3:24 ` Pankaj Dubey 0 siblings, 1 reply; 8+ messages in thread From: Andrew Murray @ 2019-11-06 9:53 UTC (permalink / raw) To: Rob Herring Cc: Anvesh Salveru, linux-pci, devicetree, linux-kernel, bhelgaas, gustavo.pimentel, jingoohan1, pankaj.dubey, Mark Rutland On Tue, Nov 05, 2019 at 03:53:32PM -0600, Rob Herring wrote: > On Mon, Oct 28, 2019 at 05:46:27PM +0530, Anvesh Salveru wrote: > > Add support for ZRX-DC compliant PHYs. If PHY is not compliant to ZRX-DC > > specification, then after every 100ms link should transition to recovery > > state during the low power states which increases power consumption. > > > > Platforms with ZRX-DC compliant PHY can use "snps,phy-zrxdc-compliant" > > property in DesignWare controller DT node. > > > > CC: Rob Herring <robh+dt@kernel.org> > > CC: Mark Rutland <mark.rutland@arm.com> > > Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com> > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > --- > > Change in v2: None > > > > Documentation/devicetree/bindings/pci/designware-pcie.txt | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > index 78494c4050f7..9507ac38ac89 100644 > > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > @@ -38,6 +38,8 @@ Optional properties: > > for data corruption. CDM registers include standard PCIe configuration > > space registers, Port Logic registers, DMA and iATU (internal Address > > Translation Unit) registers. > > +- snps,phy-zrxdc-compliant: This property is needed if phy complies with the > > + ZRX-DC specification. > > If this is a property of the phy, then it belongs in the phy node or > should just be implied by the phy's compatible. As suggested in the previous revision of this series [1], this is absolutely a property of the phy. > IOW, you should be able > to support this or not without changing DTs. > > Is this spec Synopys specific? (About the only thing Google turns up are > your patches.) If not, then probably shouldn't have a 'snps' prefix. This was also unfamiliar to me, however my current understanding is that Zrx-dc describes the 'Receiver DC single ended impedance' limits, this is specified in the PCI specification (table 'Common Receiver Parameters'), with a different limit for each speed. I believe the purpose of this series is to to satisfy the following implementation note in the spec "Ports that meet the Zrx-dc specification for 2.5 GT/s while in the L1.Idle state are therefore not required to implement the 100 ms timeout and transition to Recovery should avoid implementing it, since it will reduce the power savings expected from the L1 state". In other words, if it is known that the phy is compliant then an unnecessary transition to a higher energy state can be avoided. Though it's the PCI controller (in this case) that must action this and must find out about the phy it is connected to. So in my view 'phy-zrxdc-compliant' should be a property of a phy (without snps prefix), and if a controller wants to determine if it is compliant then there must be a phandle to the phy so the controller can find out. [1] https://patchwork.kernel.org/patch/11202121/ Thanks, Andrew Murray > > > RC mode: > > - num-viewport: number of view ports configured in hardware. If a platform > > does not specify it, the driver assumes 2. > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property 2019-11-06 9:53 ` Andrew Murray @ 2019-11-08 3:24 ` Pankaj Dubey 2019-11-08 9:55 ` Andrew Murray 2019-11-11 17:52 ` Rob Herring 0 siblings, 2 replies; 8+ messages in thread From: Pankaj Dubey @ 2019-11-08 3:24 UTC (permalink / raw) To: 'Andrew Murray', 'Rob Herring', kishon Cc: 'Anvesh Salveru', linux-pci, devicetree, linux-kernel, bhelgaas, gustavo.pimentel, jingoohan1, 'Mark Rutland' > -----Original Message----- > From: Andrew Murray <andrew.murray@arm.com> > Sent: Wednesday, November 6, 2019 3:24 PM > To: Rob Herring <robh@kernel.org> > Cc: Anvesh Salveru <anvesh.s@samsung.com>; linux-pci@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > bhelgaas@google.com; gustavo.pimentel@synopsys.com; > jingoohan1@gmail.com; pankaj.dubey@samsung.com; Mark Rutland > <mark.rutland@arm.com> > Subject: Re: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX- > DC PHY property > > On Tue, Nov 05, 2019 at 03:53:32PM -0600, Rob Herring wrote: > > On Mon, Oct 28, 2019 at 05:46:27PM +0530, Anvesh Salveru wrote: > > > Add support for ZRX-DC compliant PHYs. If PHY is not compliant to > > > ZRX-DC specification, then after every 100ms link should transition > > > to recovery state during the low power states which increases power > consumption. > > > > > > Platforms with ZRX-DC compliant PHY can use "snps,phy-zrxdc-compliant" > > > property in DesignWare controller DT node. > > > > > > CC: Rob Herring <robh+dt@kernel.org> > > > CC: Mark Rutland <mark.rutland@arm.com> > > > Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com> > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > > --- > > > Change in v2: None > > > > > > Documentation/devicetree/bindings/pci/designware-pcie.txt | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > index 78494c4050f7..9507ac38ac89 100644 > > > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > @@ -38,6 +38,8 @@ Optional properties: > > > for data corruption. CDM registers include standard PCIe configuration > > > space registers, Port Logic registers, DMA and iATU (internal Address > > > Translation Unit) registers. > > > +- snps,phy-zrxdc-compliant: This property is needed if phy complies > > > +with the > > > + ZRX-DC specification. > > > > If this is a property of the phy, then it belongs in the phy node or > > should just be implied by the phy's compatible. > Yes, from HW point of view this is a property of the PHY. As PHY is the one which is ZRXDC compliant or non-compliant. But as the DW controller programming needs to be altered for handling such phys, so we added it as a DT binding of DW controller driver. We can't use PHY's compatible in case we want this to be in common code (pcie-designware.c), as PHY compatible for each of the platform would be different and PCIe DWC core file is not aware of this compatible. Of-course this can be handled in platform specific driver, but then in that case each of these drivers will add similar code to handle this. > As suggested in the previous revision of this series [1], this is absolutely a > property of the phy. Agreed, but as I tried to explain in series [1], this we are adding in controller driver, as controller driver needs to program it's register based on PHY HW property. > > > IOW, you should be able > > to support this or not without changing DTs. > > > > Is this spec Synopys specific? (About the only thing Google turns up > > are your patches.) If not, then probably shouldn't have a 'snps' prefix. > > This was also unfamiliar to me, however my current understanding is that Zrx-dc > describes the 'Receiver DC single ended impedance' limits, this is specified in the > PCI specification (table 'Common Receiver Parameters'), with a different limit > for each speed. > > I believe the purpose of this series is to to satisfy the following implementation > note in the spec "Ports that meet the Zrx-dc specification for 2.5 GT/s while in > the L1.Idle state are therefore not required to implement the 100 ms timeout > and transition to Recovery should avoid implementing it, since it will reduce the > power savings expected from the > L1 state". > > In other words, if it is known that the phy is compliant then an unnecessary > transition to a higher energy state can be avoided. Though it's the PCI controller > (in this case) that must action this and must find out about the phy it is > connected to. > Thanks, this is exactly the purpose of the patch. Currently this is being handled in respective platform's driver, this patch intends to move this in common code, where any platform driver using DesignWare controller can use it without repeating same piece of code. > So in my view 'phy-zrxdc-compliant' should be a property of a phy (without snps > prefix), and if a controller wants to determine if it is compliant then there must > be a phandle to the phy so the controller can find out. Removing snps prefix, I am OK with it. But for moving this property to PHY node, we need to find solution, how PCIe controller driver will access this property of PHY device. Platform driver which are using DesignWare controller driver has a phandle to PHY, but question is here how does DesignWare controller driver will infer this information from PHY driver. Some approaches which I can think of are: 1) If PHY framework has some generic API to check if a particular property exists in PHY node then it will be useful in such cases. Currently I don't see any such API exists. Though I am not sure if it is OK to add such API in PHY framework for such cases? Adding Kishon to comment on this. 2) Currently phandle to PHY is being stored as part of private data structure of platform specific controller driver, and DesignWare core driver can't access the phandle. So we need to move or keep copy of phandle pointer to DesignWare core driver structure instead of keeping it in platform specific private structure. I am open to any suggestion which can help us in keeping common piece of code in DesignWare controller driver than repeating same in each and every platform driver using DWC controller. Thanks, Pankaj Dubey > > [1] https://patchwork.kernel.org/patch/11202121/ > > Thanks, > > Andrew Murray > > > > > > RC mode: > > > - num-viewport: number of view ports configured in hardware. If a platform > > > does not specify it, the driver assumes 2. > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property 2019-11-08 3:24 ` Pankaj Dubey @ 2019-11-08 9:55 ` Andrew Murray 2019-11-11 17:52 ` Rob Herring 1 sibling, 0 replies; 8+ messages in thread From: Andrew Murray @ 2019-11-08 9:55 UTC (permalink / raw) To: Pankaj Dubey Cc: 'Rob Herring', kishon, 'Anvesh Salveru', linux-pci, devicetree, linux-kernel, bhelgaas, gustavo.pimentel, jingoohan1, 'Mark Rutland' On Fri, Nov 08, 2019 at 08:54:53AM +0530, Pankaj Dubey wrote: > > > > -----Original Message----- > > From: Andrew Murray <andrew.murray@arm.com> > > Sent: Wednesday, November 6, 2019 3:24 PM > > To: Rob Herring <robh@kernel.org> > > Cc: Anvesh Salveru <anvesh.s@samsung.com>; linux-pci@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > bhelgaas@google.com; gustavo.pimentel@synopsys.com; > > jingoohan1@gmail.com; pankaj.dubey@samsung.com; Mark Rutland > > <mark.rutland@arm.com> > > Subject: Re: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for > ZRX- > > DC PHY property > > > > On Tue, Nov 05, 2019 at 03:53:32PM -0600, Rob Herring wrote: > > > On Mon, Oct 28, 2019 at 05:46:27PM +0530, Anvesh Salveru wrote: > > > > Add support for ZRX-DC compliant PHYs. If PHY is not compliant to > > > > ZRX-DC specification, then after every 100ms link should transition > > > > to recovery state during the low power states which increases power > > consumption. > > > > > > > > Platforms with ZRX-DC compliant PHY can use "snps,phy-zrxdc-compliant" > > > > property in DesignWare controller DT node. > > > > > > > > CC: Rob Herring <robh+dt@kernel.org> > > > > CC: Mark Rutland <mark.rutland@arm.com> > > > > Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com> > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > > > --- > > > > Change in v2: None > > > > > > > > Documentation/devicetree/bindings/pci/designware-pcie.txt | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > index 78494c4050f7..9507ac38ac89 100644 > > > > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > @@ -38,6 +38,8 @@ Optional properties: > > > > for data corruption. CDM registers include standard PCIe > configuration > > > > space registers, Port Logic registers, DMA and iATU (internal > Address > > > > Translation Unit) registers. > > > > +- snps,phy-zrxdc-compliant: This property is needed if phy complies > > > > +with the > > > > + ZRX-DC specification. > > > > > > If this is a property of the phy, then it belongs in the phy node or > > > should just be implied by the phy's compatible. > > > Yes, from HW point of view this is a property of the PHY. As PHY is the one > which is ZRXDC compliant or non-compliant. But as the DW controller > programming needs to be altered for handling such phys, so we added it as a > DT binding of DW controller driver. I understand how you arrived at the current approach. > > We can't use PHY's compatible in case we want this to be in common code > (pcie-designware.c), as PHY compatible for each of the platform would be > different and PCIe DWC core file is not aware of this compatible. Of-course > this can be handled in platform specific driver, but then in that case each > of these drivers will add similar code to handle this. > > > As suggested in the previous revision of this series [1], this is > absolutely a > > property of the phy. > > Agreed, but as I tried to explain in series [1], this we are adding in > controller driver, as controller driver needs to program it's register based > on PHY HW property. It creates a challenge, but it isn't a good justification for putting the property in the wrong place in the DT. The DT should describe the hardware without much consideration for ease of programming. > > > > > > IOW, you should be able > > > to support this or not without changing DTs. > > > > > > Is this spec Synopys specific? (About the only thing Google turns up > > > are your patches.) If not, then probably shouldn't have a 'snps' prefix. > > > > This was also unfamiliar to me, however my current understanding is that > Zrx-dc > > describes the 'Receiver DC single ended impedance' limits, this is > specified in the > > PCI specification (table 'Common Receiver Parameters'), with a different > limit > > for each speed. > > > > I believe the purpose of this series is to to satisfy the following > implementation > > note in the spec "Ports that meet the Zrx-dc specification for 2.5 GT/s > while in > > the L1.Idle state are therefore not required to implement the 100 ms > timeout > > and transition to Recovery should avoid implementing it, since it will > reduce the > > power savings expected from the > > L1 state". > > > > > In other words, if it is known that the phy is compliant then an > unnecessary > > transition to a higher energy state can be avoided. Though it's the PCI > controller > > (in this case) that must action this and must find out about the phy it is > > connected to. > > > > Thanks, this is exactly the purpose of the patch. Currently this is being > handled in respective platform's driver, this patch intends to move this in > common code, where any platform driver using DesignWare controller can use > it without repeating same piece of code. > > > So in my view 'phy-zrxdc-compliant' should be a property of a phy (without > snps > > prefix), and if a controller wants to determine if it is compliant then > there must > > be a phandle to the phy so the controller can find out. > > > Removing snps prefix, I am OK with it. But for moving this property to PHY > node, we need to find solution, how PCIe controller driver will access this > property of PHY device. > > Platform driver which are using DesignWare controller driver has a phandle > to PHY, but question is here how does DesignWare controller driver will > infer this information from PHY driver. Some approaches which I can think of > are: > 1) If PHY framework has some generic API to check if a particular property > exists in PHY node then it will be useful in such cases. Currently I don't > see any such API exists. Though I am not sure if it is OK to add such API in > PHY framework for such cases? Adding Kishon to comment on this. > 2) Currently phandle to PHY is being stored as part of private data > structure of platform specific controller driver, and DesignWare core driver > can't access the phandle. So we need to move or keep copy of phandle pointer > to DesignWare core driver structure instead of keeping it in platform > specific private structure. Indeed, these are both valid solutions. In other words put the property in the phy, and figure out how you can access that from the controller. There must be many examples in the kernel of drivers needing to understand properties of hardware components they are connected to. Though I haven't explored in any detail how to solve this particular challenge. > > I am open to any suggestion which can help us in keeping common piece of > code in DesignWare controller driver than repeating same in each and every > platform driver using DWC controller. Agreed, I'd expect to see a solution that doesn't involve duplicating code in the host bridge drivers. Thanks, Andrew Murray > > > Thanks, > Pankaj Dubey > > > > > [1] https://patchwork.kernel.org/patch/11202121/ > > > > Thanks, > > > > Andrew Murray > > > > > > > > > RC mode: > > > > - num-viewport: number of view ports configured in hardware. If a > platform > > > > does not specify it, the driver assumes 2. > > > > -- > > > > 2.17.1 > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property 2019-11-08 3:24 ` Pankaj Dubey 2019-11-08 9:55 ` Andrew Murray @ 2019-11-11 17:52 ` Rob Herring 1 sibling, 0 replies; 8+ messages in thread From: Rob Herring @ 2019-11-11 17:52 UTC (permalink / raw) To: Pankaj Dubey Cc: Andrew Murray, Kishon Vijay Abraham I, Anvesh Salveru, PCI, devicetree, linux-kernel, Bjorn Helgaas, Gustavo Pimentel, Jingoo Han, Mark Rutland On Thu, Nov 7, 2019 at 9:25 PM Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > > > > > -----Original Message----- > > From: Andrew Murray <andrew.murray@arm.com> > > Sent: Wednesday, November 6, 2019 3:24 PM > > To: Rob Herring <robh@kernel.org> > > Cc: Anvesh Salveru <anvesh.s@samsung.com>; linux-pci@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > bhelgaas@google.com; gustavo.pimentel@synopsys.com; > > jingoohan1@gmail.com; pankaj.dubey@samsung.com; Mark Rutland > > <mark.rutland@arm.com> > > Subject: Re: [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for > ZRX- > > DC PHY property > > > > On Tue, Nov 05, 2019 at 03:53:32PM -0600, Rob Herring wrote: > > > On Mon, Oct 28, 2019 at 05:46:27PM +0530, Anvesh Salveru wrote: > > > > Add support for ZRX-DC compliant PHYs. If PHY is not compliant to > > > > ZRX-DC specification, then after every 100ms link should transition > > > > to recovery state during the low power states which increases power > > consumption. > > > > > > > > Platforms with ZRX-DC compliant PHY can use "snps,phy-zrxdc-compliant" > > > > property in DesignWare controller DT node. > > > > > > > > CC: Rob Herring <robh+dt@kernel.org> > > > > CC: Mark Rutland <mark.rutland@arm.com> > > > > Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com> > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > > > --- > > > > Change in v2: None > > > > > > > > Documentation/devicetree/bindings/pci/designware-pcie.txt | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > index 78494c4050f7..9507ac38ac89 100644 > > > > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > > > > @@ -38,6 +38,8 @@ Optional properties: > > > > for data corruption. CDM registers include standard PCIe > configuration > > > > space registers, Port Logic registers, DMA and iATU (internal > Address > > > > Translation Unit) registers. > > > > +- snps,phy-zrxdc-compliant: This property is needed if phy complies > > > > +with the > > > > + ZRX-DC specification. > > > > > > If this is a property of the phy, then it belongs in the phy node or > > > should just be implied by the phy's compatible. > > > Yes, from HW point of view this is a property of the PHY. As PHY is the one > which is ZRXDC compliant or non-compliant. But as the DW controller > programming needs to be altered for handling such phys, so we added it as a > DT binding of DW controller driver. Bindings are for h/w blocks not drivers. > We can't use PHY's compatible in case we want this to be in common code > (pcie-designware.c), as PHY compatible for each of the platform would be > different and PCIe DWC core file is not aware of this compatible. Of-course > this can be handled in platform specific driver, but then in that case each > of these drivers will add similar code to handle this. > > > As suggested in the previous revision of this series [1], this is > absolutely a > > property of the phy. > > Agreed, but as I tried to explain in series [1], this we are adding in > controller driver, as controller driver needs to program it's register based > on PHY HW property. > > > > > > IOW, you should be able > > > to support this or not without changing DTs. > > > > > > Is this spec Synopys specific? (About the only thing Google turns up > > > are your patches.) If not, then probably shouldn't have a 'snps' prefix. > > > > This was also unfamiliar to me, however my current understanding is that > Zrx-dc > > describes the 'Receiver DC single ended impedance' limits, this is > specified in the > > PCI specification (table 'Common Receiver Parameters'), with a different > limit > > for each speed. > > > > I believe the purpose of this series is to to satisfy the following > implementation > > note in the spec "Ports that meet the Zrx-dc specification for 2.5 GT/s > while in > > the L1.Idle state are therefore not required to implement the 100 ms > timeout > > and transition to Recovery should avoid implementing it, since it will > reduce the > > power savings expected from the > > L1 state". > > > > > In other words, if it is known that the phy is compliant then an > unnecessary > > transition to a higher energy state can be avoided. Though it's the PCI > controller > > (in this case) that must action this and must find out about the phy it is > > connected to. > > > > Thanks, this is exactly the purpose of the patch. Currently this is being > handled in respective platform's driver, this patch intends to move this in > common code, where any platform driver using DesignWare controller can use > it without repeating same piece of code. Driver structure is not an argument for DT binding design. > > So in my view 'phy-zrxdc-compliant' should be a property of a phy (without > snps > > prefix), and if a controller wants to determine if it is compliant then > there must > > be a phandle to the phy so the controller can find out. > > > Removing snps prefix, I am OK with it. But for moving this property to PHY > node, we need to find solution, how PCIe controller driver will access this > property of PHY device. > > Platform driver which are using DesignWare controller driver has a phandle > to PHY, but question is here how does DesignWare controller driver will > infer this information from PHY driver. Some approaches which I can think of > are: > 1) If PHY framework has some generic API to check if a particular property > exists in PHY node then it will be useful in such cases. Currently I don't > see any such API exists. Though I am not sure if it is OK to add such API in > PHY framework for such cases? Adding Kishon to comment on this. It's always okay to change/extend kernel APIs. We don't work-around kernel APIs in drivers nor in DT. (Maybe not agreement in how though) IMO, this is what should happen. This solution makes enabling this feature or not detached from DT. It could be a DT property, determined by compatible of the phy, or just always enabled for a phy driver. This is not the first time this issue has come up with the phy api needing something like this (USB PHYs in particular). > 2) Currently phandle to PHY is being stored as part of private data > structure of platform specific controller driver, and DesignWare core driver > can't access the phandle. So we need to move or keep copy of phandle pointer > to DesignWare core driver structure instead of keeping it in platform > specific private structure. Or just parse it again... Rob ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CGME20191028121758epcas5p2dda6d0842be32bcab2e6025fac1f3e78@epcas5p2.samsung.com>]
* [PATCH v2 2/2] PCI: dwc: Add support to handle ZRX-DC Compliant PHYs [not found] ` <CGME20191028121758epcas5p2dda6d0842be32bcab2e6025fac1f3e78@epcas5p2.samsung.com> @ 2019-10-28 12:16 ` Anvesh Salveru 0 siblings, 0 replies; 8+ messages in thread From: Anvesh Salveru @ 2019-10-28 12:16 UTC (permalink / raw) To: linux-pci, devicetree, linux-kernel Cc: bhelgaas, gustavo.pimentel, jingoohan1, pankaj.dubey, Anvesh Salveru, Lorenzo Pieralisi, Andrew Murray Many platforms use DesignWare controller but the PHY can be different in different platforms. If the PHY is compliant to the ZRX-DC specification it helps lower power consumption during power states. If current data rate is 8.0 GT/s or higher and PHY is not compliant to ZRX-DC specification, then after every 100ms link should transition to recovery state during the low power states. DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY. Platforms with ZRX-DC compliant PHY can set "snps,phy-zrxdc-compliant" property in controller DT node to specify this property to the controller. CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> CC: Andrew Murray <andrew.murray@arm.com> Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> --- Change in v2: - trivial change in patch description drivers/pci/controller/dwc/pcie-designware.c | 7 +++++++ drivers/pci/controller/dwc/pcie-designware.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 820488dfeaed..6560d9f765d7 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -556,4 +556,11 @@ void dw_pcie_setup(struct dw_pcie *pci) PCIE_PL_CHK_REG_CHK_REG_START; dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val); } + + if (of_property_read_bool(np, "snps,phy-zrxdc-compliant")) { + val = dw_pcie_readl_dbi(pci, PCIE_PORT_GEN3_RELATED); + val &= ~PORT_LOGIC_GEN3_ZRXDC_NONCOMPL; + dw_pcie_writel_dbi(pci, PCIE_PORT_GEN3_RELATED, val); + } + } diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 5a18e94e52c8..427a55ec43c6 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -60,6 +60,9 @@ #define PCIE_MSI_INTR0_MASK 0x82C #define PCIE_MSI_INTR0_STATUS 0x830 +#define PCIE_PORT_GEN3_RELATED 0x890 +#define PORT_LOGIC_GEN3_ZRXDC_NONCOMPL BIT(0) + #define PCIE_ATU_VIEWPORT 0x900 #define PCIE_ATU_REGION_INBOUND BIT(31) #define PCIE_ATU_REGION_OUTBOUND 0 -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-11-11 17:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20191028121704epcas5p483bf05ccb4cd25b1757cd5645e819d12@epcas5p4.samsung.com> 2019-10-28 12:16 ` [PATCH v2 0/2] Add support to handle ZRX-DC Compliant PHYs Anvesh Salveru [not found] ` <CGME20191028121748epcas5p3054c9583c14a2edde9f725d005895a04@epcas5p3.samsung.com> 2019-10-28 12:16 ` [PATCH v2 1/2] dt-bindings: PCI: designware: Add binding for ZRX-DC PHY property Anvesh Salveru 2019-11-05 21:53 ` Rob Herring 2019-11-06 9:53 ` Andrew Murray 2019-11-08 3:24 ` Pankaj Dubey 2019-11-08 9:55 ` Andrew Murray 2019-11-11 17:52 ` Rob Herring [not found] ` <CGME20191028121758epcas5p2dda6d0842be32bcab2e6025fac1f3e78@epcas5p2.samsung.com> 2019-10-28 12:16 ` [PATCH v2 2/2] PCI: dwc: Add support to handle ZRX-DC Compliant PHYs Anvesh Salveru
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).