diff mbox series

[2/9] arm64: dts: qcom: ipq8074: fix Gen3 PCIe QMP PHY

Message ID 20221116214841.1116735-2-robimarko@gmail.com
State New
Headers
Series [1/9] arm64: dts: qcom: ipq8074: fix Gen2 PCIe QMP PHY |

Commit Message

Robert Marko Nov. 16, 2022, 9:48 p.m. UTC 
  IPQ8074 comes in 2 silicon versions:
* v1 with 2x Gen2 PCIe ports and QMP PHY-s
* v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s

v2 is the final and production version that is actually supported by the
kernel, however it looks like PCIe related nodes were added for the v1 SoC.

Now that we have Gen3 QMP PHY support, we can start fixing the PCIe support
by fixing the Gen3 QMP PHY node first.

Change the compatible to the Gen3 QMP PHY, correct the register space start
and size, add the missing misc PCS register space.

Fixes: 33057e1672fe ("ARM: dts: ipq8074: Add pcie nodes")
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 arch/arm64/boot/dts/qcom/ipq8074.dtsi | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Bjorn Andersson Dec. 5, 2022, 9:52 p.m. UTC | #1 
On Wed, Nov 16, 2022 at 10:48:34PM +0100, Robert Marko wrote:
> IPQ8074 comes in 2 silicon versions:
> * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
> 
> v2 is the final and production version that is actually supported by the
> kernel, however it looks like PCIe related nodes were added for the v1 SoC.
> 
> Now that we have Gen3 QMP PHY support, we can start fixing the PCIe support
> by fixing the Gen3 QMP PHY node first.
> 
> Change the compatible to the Gen3 QMP PHY, correct the register space start
> and size, add the missing misc PCS register space.
> 

Does this imply that the current node doesn't actually work?

If that's the case, could we perhaps adopt Johan Hovolds' new binding
and drop the subnode in favor of just a flat reg covering the whole
QMP region?

Regards,
Bjorn

> Fixes: 33057e1672fe ("ARM: dts: ipq8074: Add pcie nodes")
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq8074.dtsi | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
> index 6649a758d8df..9503dfb25d50 100644
> --- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
> @@ -232,9 +232,9 @@ qusb_phy_0: phy@79000 {
>  			status = "disabled";
>  		};
>  
> -		pcie_qmp0: phy@86000 {
> -			compatible = "qcom,ipq8074-qmp-pcie-phy";
> -			reg = <0x00086000 0x1c4>;
> +		pcie_qmp0: phy@84000 {
> +			compatible = "qcom,ipq8074-qmp-gen3-pcie-phy";
> +			reg = <0x00084000 0x1bc>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			ranges;
> @@ -248,10 +248,11 @@ pcie_qmp0: phy@86000 {
>  				      "common";
>  			status = "disabled";
>  
> -			pcie_phy0: phy@86200 {
> -				reg = <0x86200 0x16c>,
> -				      <0x86400 0x200>,
> -				      <0x86800 0x4f4>;
> +			pcie_phy0: phy@84200 {
> +				reg = <0x84200 0x16c>,
> +				      <0x84400 0x200>,
> +				      <0x84800 0x1f0>,
> +				      <0x84c00 0xf4>;
>  				#phy-cells = <0>;
>  				#clock-cells = <0>;
>  				clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
> -- 
> 2.38.1
>
Robert Marko Dec. 6, 2022, 9:51 a.m. UTC | #2 
On Mon, 5 Dec 2022 at 22:52, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Wed, Nov 16, 2022 at 10:48:34PM +0100, Robert Marko wrote:
> > IPQ8074 comes in 2 silicon versions:
> > * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> > * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
> >
> > v2 is the final and production version that is actually supported by the
> > kernel, however it looks like PCIe related nodes were added for the v1 SoC.
> >
> > Now that we have Gen3 QMP PHY support, we can start fixing the PCIe support
> > by fixing the Gen3 QMP PHY node first.
> >
> > Change the compatible to the Gen3 QMP PHY, correct the register space start
> > and size, add the missing misc PCS register space.
> >
>
> Does this imply that the current node doesn't actually work?

Hi Bjorn,
Yes, the node is for a completely different PHY generation, basically
PCIe on IPQ8074
is completely broken, hence this patch series.

>
> If that's the case, could we perhaps adopt Johan Hovolds' new binding
> and drop the subnode in favor of just a flat reg covering the whole
> QMP region?

I have not seen that so far, any examples?

Regards,
Robert
>
> Regards,
> Bjorn
>
> > Fixes: 33057e1672fe ("ARM: dts: ipq8074: Add pcie nodes")
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> >  arch/arm64/boot/dts/qcom/ipq8074.dtsi | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
> > index 6649a758d8df..9503dfb25d50 100644
> > --- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
> > @@ -232,9 +232,9 @@ qusb_phy_0: phy@79000 {
> >                       status = "disabled";
> >               };
> >
> > -             pcie_qmp0: phy@86000 {
> > -                     compatible = "qcom,ipq8074-qmp-pcie-phy";
> > -                     reg = <0x00086000 0x1c4>;
> > +             pcie_qmp0: phy@84000 {
> > +                     compatible = "qcom,ipq8074-qmp-gen3-pcie-phy";
> > +                     reg = <0x00084000 0x1bc>;
> >                       #address-cells = <1>;
> >                       #size-cells = <1>;
> >                       ranges;
> > @@ -248,10 +248,11 @@ pcie_qmp0: phy@86000 {
> >                                     "common";
> >                       status = "disabled";
> >
> > -                     pcie_phy0: phy@86200 {
> > -                             reg = <0x86200 0x16c>,
> > -                                   <0x86400 0x200>,
> > -                                   <0x86800 0x4f4>;
> > +                     pcie_phy0: phy@84200 {
> > +                             reg = <0x84200 0x16c>,
> > +                                   <0x84400 0x200>,
> > +                                   <0x84800 0x1f0>,
> > +                                   <0x84c00 0xf4>;
> >                               #phy-cells = <0>;
> >                               #clock-cells = <0>;
> >                               clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
> > --
> > 2.38.1
> >
Bjorn Andersson Dec. 27, 2022, 7:20 p.m. UTC | #3 
On Tue, Dec 06, 2022 at 10:51:40AM +0100, Robert Marko wrote:
> On Mon, 5 Dec 2022 at 22:52, Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Wed, Nov 16, 2022 at 10:48:34PM +0100, Robert Marko wrote:
> > > IPQ8074 comes in 2 silicon versions:
> > > * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> > > * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
> > >
> > > v2 is the final and production version that is actually supported by the
> > > kernel, however it looks like PCIe related nodes were added for the v1 SoC.
> > >
> > > Now that we have Gen3 QMP PHY support, we can start fixing the PCIe support
> > > by fixing the Gen3 QMP PHY node first.
> > >
> > > Change the compatible to the Gen3 QMP PHY, correct the register space start
> > > and size, add the missing misc PCS register space.
> > >
> >
> > Does this imply that the current node doesn't actually work?
> 
> Hi Bjorn,
> Yes, the node is for a completely different PHY generation, basically
> PCIe on IPQ8074
> is completely broken, hence this patch series.
> 
> >
> > If that's the case, could we perhaps adopt Johan Hovolds' new binding
> > and drop the subnode in favor of just a flat reg covering the whole
> > QMP region?
> 
> I have not seen that so far, any examples?
> 

See
Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml in
v6.2-rc1.

The idea is to, at least, use this for all new platforms introduced.

And if the current definition doesn't actually work I suggest that we
replace it with the new one.

Regards,
Bjorn
Robert Marko Dec. 28, 2022, 11:10 a.m. UTC | #4 
On Tue, 27 Dec 2022 at 20:20, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Tue, Dec 06, 2022 at 10:51:40AM +0100, Robert Marko wrote:
> > On Mon, 5 Dec 2022 at 22:52, Bjorn Andersson <andersson@kernel.org> wrote:
> > >
> > > On Wed, Nov 16, 2022 at 10:48:34PM +0100, Robert Marko wrote:
> > > > IPQ8074 comes in 2 silicon versions:
> > > > * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> > > > * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
> > > >
> > > > v2 is the final and production version that is actually supported by the
> > > > kernel, however it looks like PCIe related nodes were added for the v1 SoC.
> > > >
> > > > Now that we have Gen3 QMP PHY support, we can start fixing the PCIe support
> > > > by fixing the Gen3 QMP PHY node first.
> > > >
> > > > Change the compatible to the Gen3 QMP PHY, correct the register space start
> > > > and size, add the missing misc PCS register space.
> > > >
> > >
> > > Does this imply that the current node doesn't actually work?
> >
> > Hi Bjorn,
> > Yes, the node is for a completely different PHY generation, basically
> > PCIe on IPQ8074
> > is completely broken, hence this patch series.
> >
> > >
> > > If that's the case, could we perhaps adopt Johan Hovolds' new binding
> > > and drop the subnode in favor of just a flat reg covering the whole
> > > QMP region?
> >
> > I have not seen that so far, any examples?
> >
>
> See
> Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml in
> v6.2-rc1.
>
> The idea is to, at least, use this for all new platforms introduced.
>
> And if the current definition doesn't actually work I suggest that we
> replace it with the new one.

I understand the intention, but these bindings dont match the QMP generation
found in IPQ8074 at all, and Gen3 has already been documented in bindings.

This would require updating the driver to carry the offsets and rework
of bindings to
not require power domains, etc for IPQ8074 as I have not found any
code downstream
to indicate it has GSDC-s for PCIe though I dont have any docs at all
for the SoC.

Regards,
Robert
>
> Regards,
> Bjorn
Bjorn Andersson Dec. 29, 2022, 5:29 p.m. UTC | #5 
On Wed, Dec 28, 2022 at 12:10:17PM +0100, Robert Marko wrote:
> On Tue, 27 Dec 2022 at 20:20, Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Tue, Dec 06, 2022 at 10:51:40AM +0100, Robert Marko wrote:
> > > On Mon, 5 Dec 2022 at 22:52, Bjorn Andersson <andersson@kernel.org> wrote:
> > > >
> > > > On Wed, Nov 16, 2022 at 10:48:34PM +0100, Robert Marko wrote:
> > > > > IPQ8074 comes in 2 silicon versions:
> > > > > * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> > > > > * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
> > > > >
> > > > > v2 is the final and production version that is actually supported by the
> > > > > kernel, however it looks like PCIe related nodes were added for the v1 SoC.
> > > > >
> > > > > Now that we have Gen3 QMP PHY support, we can start fixing the PCIe support
> > > > > by fixing the Gen3 QMP PHY node first.
> > > > >
> > > > > Change the compatible to the Gen3 QMP PHY, correct the register space start
> > > > > and size, add the missing misc PCS register space.
> > > > >
> > > >
> > > > Does this imply that the current node doesn't actually work?
> > >
> > > Hi Bjorn,
> > > Yes, the node is for a completely different PHY generation, basically
> > > PCIe on IPQ8074
> > > is completely broken, hence this patch series.
> > >
> > > >
> > > > If that's the case, could we perhaps adopt Johan Hovolds' new binding
> > > > and drop the subnode in favor of just a flat reg covering the whole
> > > > QMP region?
> > >
> > > I have not seen that so far, any examples?
> > >
> >
> > See
> > Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml in
> > v6.2-rc1.
> >
> > The idea is to, at least, use this for all new platforms introduced.
> >
> > And if the current definition doesn't actually work I suggest that we
> > replace it with the new one.
> 
> I understand the intention, but these bindings dont match the QMP generation
> found in IPQ8074 at all, and Gen3 has already been documented in bindings.
> 
> This would require updating the driver to carry the offsets and rework
> of bindings to
> not require power domains, etc for IPQ8074 as I have not found any
> code downstream
> to indicate it has GSDC-s for PCIe though I dont have any docs at all
> for the SoC.
> 

I was only thinking of the structural difference, not the power-domains
etc. But yes you're right that it means updating the driver and the
binding.

The end result would be much nicer though...

Regards,
Bjorn
Robert Marko Jan. 3, 2023, 8:31 p.m. UTC | #6 
On Thu, 29 Dec 2022 at 18:29, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Wed, Dec 28, 2022 at 12:10:17PM +0100, Robert Marko wrote:
> > On Tue, 27 Dec 2022 at 20:20, Bjorn Andersson <andersson@kernel.org> wrote:
> > >
> > > On Tue, Dec 06, 2022 at 10:51:40AM +0100, Robert Marko wrote:
> > > > On Mon, 5 Dec 2022 at 22:52, Bjorn Andersson <andersson@kernel.org> wrote:
> > > > >
> > > > > On Wed, Nov 16, 2022 at 10:48:34PM +0100, Robert Marko wrote:
> > > > > > IPQ8074 comes in 2 silicon versions:
> > > > > > * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> > > > > > * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
> > > > > >
> > > > > > v2 is the final and production version that is actually supported by the
> > > > > > kernel, however it looks like PCIe related nodes were added for the v1 SoC.
> > > > > >
> > > > > > Now that we have Gen3 QMP PHY support, we can start fixing the PCIe support
> > > > > > by fixing the Gen3 QMP PHY node first.
> > > > > >
> > > > > > Change the compatible to the Gen3 QMP PHY, correct the register space start
> > > > > > and size, add the missing misc PCS register space.
> > > > > >
> > > > >
> > > > > Does this imply that the current node doesn't actually work?
> > > >
> > > > Hi Bjorn,
> > > > Yes, the node is for a completely different PHY generation, basically
> > > > PCIe on IPQ8074
> > > > is completely broken, hence this patch series.
> > > >
> > > > >
> > > > > If that's the case, could we perhaps adopt Johan Hovolds' new binding
> > > > > and drop the subnode in favor of just a flat reg covering the whole
> > > > > QMP region?
> > > >
> > > > I have not seen that so far, any examples?
> > > >
> > >
> > > See
> > > Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml in
> > > v6.2-rc1.
> > >
> > > The idea is to, at least, use this for all new platforms introduced.
> > >
> > > And if the current definition doesn't actually work I suggest that we
> > > replace it with the new one.
> >
> > I understand the intention, but these bindings dont match the QMP generation
> > found in IPQ8074 at all, and Gen3 has already been documented in bindings.
> >
> > This would require updating the driver to carry the offsets and rework
> > of bindings to
> > not require power domains, etc for IPQ8074 as I have not found any
> > code downstream
> > to indicate it has GSDC-s for PCIe though I dont have any docs at all
> > for the SoC.
> >
>
> I was only thinking of the structural difference, not the power-domains
> etc. But yes you're right that it means updating the driver and the
> binding.
>
> The end result would be much nicer though...

I agree that it would be nicer, though it's asking for many changes, especially
bindings since it's already described there.

The current patchset has been thoroughly tested for months at this point.

How would you like to proceed here, as I would really like to get PCI working
upstream?

Regards,
Robert
>
> Regards,
> Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
index 6649a758d8df..9503dfb25d50 100644
--- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
@@ -232,9 +232,9 @@  qusb_phy_0: phy@79000 {
 			status = "disabled";
 		};
 
-		pcie_qmp0: phy@86000 {
-			compatible = "qcom,ipq8074-qmp-pcie-phy";
-			reg = <0x00086000 0x1c4>;
+		pcie_qmp0: phy@84000 {
+			compatible = "qcom,ipq8074-qmp-gen3-pcie-phy";
+			reg = <0x00084000 0x1bc>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -248,10 +248,11 @@  pcie_qmp0: phy@86000 {
 				      "common";
 			status = "disabled";
 
-			pcie_phy0: phy@86200 {
-				reg = <0x86200 0x16c>,
-				      <0x86400 0x200>,
-				      <0x86800 0x4f4>;
+			pcie_phy0: phy@84200 {
+				reg = <0x84200 0x16c>,
+				      <0x84400 0x200>,
+				      <0x84800 0x1f0>,
+				      <0x84c00 0xf4>;
 				#phy-cells = <0>;
 				#clock-cells = <0>;
 				clocks = <&gcc GCC_PCIE0_PIPE_CLK>;