diff mbox series

[v2] arm64: dts: qcom: x1e80100: Switch PCIe 6a to 4 lanes mode

Message ID 20241004-x1e80100-dts-fixes-pcie6a-v2-1-3af9ff7a5a71@linaro.org (mailing list archive)
State Changes Requested
Headers show
Series [v2] arm64: dts: qcom: x1e80100: Switch PCIe 6a to 4 lanes mode | expand

Commit Message

Abel Vesa Oct. 4, 2024, 9:06 a.m. UTC 
The PCIe 6a controller and PHY can be configured in 4-lanes mode or
2-lanes mode. For 4-lanes mode, it fetches the lanes provided by PCIe 6b.
For 2-lanes mode, PCIe 6a uses 2 lanes and then PCIe 6b uses the other 2
lanes. Configure it in 4-lane mode and then each board can configure it
depending on the design. Both the QCP and CRD boards, currently upstream,
use PCIe 6a for NVMe in 4-lane mode. Mark the controller as 4-lane as
well. This is the last change needed in order to support NVMe with Gen4
4-lanes on all existing X Elite boards.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v2:
- Re-worded the commit message according to Johan's suggestions
- Dropped the clocks changes.
- Dropped the fixes tag as this relies on the Gen4 4-lanes stability
  patchset which has been only merged in 6.12, so backporting this patch
  would break NVMe support for all platforms.
- Link to v1: https://lore.kernel.org/r/20240531-x1e80100-dts-fixes-pcie6a-v1-2-1573ebcae1e8@linaro.org
---
 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


---
base-commit: c02d24a5af66a9806922391493205a344749f2c4
change-id: 20241003-x1e80100-dts-fixes-pcie6a-b9f1171e8d5b

Best regards,

Comments

Konrad Dybcio Oct. 4, 2024, 9:28 a.m. UTC | #1 
On 4.10.2024 11:06 AM, Abel Vesa wrote:
> The PCIe 6a controller and PHY can be configured in 4-lanes mode or
> 2-lanes mode. For 4-lanes mode, it fetches the lanes provided by PCIe 6b.
> For 2-lanes mode, PCIe 6a uses 2 lanes and then PCIe 6b uses the other 2
> lanes. Configure it in 4-lane mode and then each board can configure it
> depending on the design. Both the QCP and CRD boards, currently upstream,
> use PCIe 6a for NVMe in 4-lane mode. Mark the controller as 4-lane as
> well. This is the last change needed in order to support NVMe with Gen4
> 4-lanes on all existing X Elite boards.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Re-worded the commit message according to Johan's suggestions
> - Dropped the clocks changes.
> - Dropped the fixes tag as this relies on the Gen4 4-lanes stability
>   patchset which has been only merged in 6.12, so backporting this patch
>   would break NVMe support for all platforms.
> - Link to v1: https://lore.kernel.org/r/20240531-x1e80100-dts-fixes-pcie6a-v1-2-1573ebcae1e8@linaro.org
> ---
Depends on

https://lore.kernel.org/linux-arm-msm/20240916082307.29393-3-johan+linaro@kernel.org/

Otherwise

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad
Dmitry Baryshkov Oct. 6, 2024, 7:57 p.m. UTC | #2 
On Fri, Oct 04, 2024 at 12:06:33PM GMT, Abel Vesa wrote:
> The PCIe 6a controller and PHY can be configured in 4-lanes mode or
> 2-lanes mode. For 4-lanes mode, it fetches the lanes provided by PCIe 6b.
> For 2-lanes mode, PCIe 6a uses 2 lanes and then PCIe 6b uses the other 2
> lanes. Configure it in 4-lane mode and then each board can configure it
> depending on the design. Both the QCP and CRD boards, currently upstream,
> use PCIe 6a for NVMe in 4-lane mode. Mark the controller as 4-lane as
> well. This is the last change needed in order to support NVMe with Gen4
> 4-lanes on all existing X Elite boards.

What about other X1E80100 devices supported upstream? Do they also use
this controller in 4 lane mode?

> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Re-worded the commit message according to Johan's suggestions
> - Dropped the clocks changes.
> - Dropped the fixes tag as this relies on the Gen4 4-lanes stability
>   patchset which has been only merged in 6.12, so backporting this patch
>   would break NVMe support for all platforms.
> - Link to v1: https://lore.kernel.org/r/20240531-x1e80100-dts-fixes-pcie6a-v1-2-1573ebcae1e8@linaro.org
> ---
>  arch/arm64/boot/dts/qcom/x1e80100.dtsi | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index a36076e3c56b5b8815eb41ec55e2e1e5bd878201..4ec712cb7a26d8fe434631cf15949524fd22c7d9 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -2931,7 +2931,7 @@ pcie6a: pci@1bf8000 {
>  			dma-coherent;
>  
>  			linux,pci-domain = <6>;
> -			num-lanes = <2>;
> +			num-lanes = <4>;
>  
>  			interrupts = <GIC_SPI 773 IRQ_TYPE_LEVEL_HIGH>,
>  				     <GIC_SPI 774 IRQ_TYPE_LEVEL_HIGH>,
> @@ -2997,8 +2997,9 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  		};
>  
>  		pcie6a_phy: phy@1bfc000 {
> -			compatible = "qcom,x1e80100-qmp-gen4x2-pcie-phy";
> -			reg = <0 0x01bfc000 0 0x2000>;
> +			compatible = "qcom,x1e80100-qmp-gen4x4-pcie-phy";

Oh...

> +			reg = <0 0x01bfc000 0 0x2000>,
> +			      <0 0x01bfe000 0 0x2000>;
>  
>  			clocks = <&gcc GCC_PCIE_6A_PHY_AUX_CLK>,
>  				 <&gcc GCC_PCIE_6A_CFG_AHB_CLK>,
> @@ -3021,6 +3022,8 @@ pcie6a_phy: phy@1bfc000 {
>  
>  			power-domains = <&gcc GCC_PCIE_6_PHY_GDSC>;
>  
> +			qcom,4ln-config-sel = <&tcsr 0x1a000 0>;
> +
>  			#clock-cells = <0>;
>  			clock-output-names = "pcie6a_pipe_clk";
>  
> 
> ---
> base-commit: c02d24a5af66a9806922391493205a344749f2c4
> change-id: 20241003-x1e80100-dts-fixes-pcie6a-b9f1171e8d5b
> 
> Best regards,
> -- 
> Abel Vesa <abel.vesa@linaro.org>
>
Abel Vesa Oct. 7, 2024, 10:26 a.m. UTC | #3 
On 24-10-06 22:57:05, Dmitry Baryshkov wrote:
> On Fri, Oct 04, 2024 at 12:06:33PM GMT, Abel Vesa wrote:
> > The PCIe 6a controller and PHY can be configured in 4-lanes mode or
> > 2-lanes mode. For 4-lanes mode, it fetches the lanes provided by PCIe 6b.
> > For 2-lanes mode, PCIe 6a uses 2 lanes and then PCIe 6b uses the other 2
> > lanes. Configure it in 4-lane mode and then each board can configure it
> > depending on the design. Both the QCP and CRD boards, currently upstream,
> > use PCIe 6a for NVMe in 4-lane mode. Mark the controller as 4-lane as
> > well. This is the last change needed in order to support NVMe with Gen4
> > 4-lanes on all existing X Elite boards.
> 
> What about other X1E80100 devices supported upstream? Do they also use
> this controller in 4 lane mode?

Yes, by my knowledge, all upstream boards with X1E80100 use this
controller for NVMe in 4 lanes mode.

There is a question about the Galaxy Book4 Edge, as I think that uses
UFS instead, and my guess is it doesn't use the PCIe 6a for anything.
But that is not yet merged.

> 
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> > Changes in v2:
> > - Re-worded the commit message according to Johan's suggestions
> > - Dropped the clocks changes.
> > - Dropped the fixes tag as this relies on the Gen4 4-lanes stability
> >   patchset which has been only merged in 6.12, so backporting this patch
> >   would break NVMe support for all platforms.
> > - Link to v1: https://lore.kernel.org/r/20240531-x1e80100-dts-fixes-pcie6a-v1-2-1573ebcae1e8@linaro.org
> > ---
> >  arch/arm64/boot/dts/qcom/x1e80100.dtsi | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > index a36076e3c56b5b8815eb41ec55e2e1e5bd878201..4ec712cb7a26d8fe434631cf15949524fd22c7d9 100644
> > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > @@ -2931,7 +2931,7 @@ pcie6a: pci@1bf8000 {
> >  			dma-coherent;
> >  
> >  			linux,pci-domain = <6>;
> > -			num-lanes = <2>;
> > +			num-lanes = <4>;
> >  
> >  			interrupts = <GIC_SPI 773 IRQ_TYPE_LEVEL_HIGH>,
> >  				     <GIC_SPI 774 IRQ_TYPE_LEVEL_HIGH>,
> > @@ -2997,8 +2997,9 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> >  		};
> >  
> >  		pcie6a_phy: phy@1bfc000 {
> > -			compatible = "qcom,x1e80100-qmp-gen4x2-pcie-phy";
> > -			reg = <0 0x01bfc000 0 0x2000>;
> > +			compatible = "qcom,x1e80100-qmp-gen4x4-pcie-phy";
> 
> Oh...

Yes, we default to 4 lanes here.

> 
> > +			reg = <0 0x01bfc000 0 0x2000>,
> > +			      <0 0x01bfe000 0 0x2000>;
> >  
> >  			clocks = <&gcc GCC_PCIE_6A_PHY_AUX_CLK>,
> >  				 <&gcc GCC_PCIE_6A_CFG_AHB_CLK>,
> > @@ -3021,6 +3022,8 @@ pcie6a_phy: phy@1bfc000 {
> >  
> >  			power-domains = <&gcc GCC_PCIE_6_PHY_GDSC>;
> >  
> > +			qcom,4ln-config-sel = <&tcsr 0x1a000 0>;
> > +
> >  			#clock-cells = <0>;
> >  			clock-output-names = "pcie6a_pipe_clk";
> >  
> > 
> > ---
> > base-commit: c02d24a5af66a9806922391493205a344749f2c4
> > change-id: 20241003-x1e80100-dts-fixes-pcie6a-b9f1171e8d5b
> > 
> > Best regards,
> > -- 
> > Abel Vesa <abel.vesa@linaro.org>
> > 
> 
> -- 
> With best wishes
> Dmitry
Johan Hovold Oct. 7, 2024, 11:19 a.m. UTC | #4 
On Fri, Oct 04, 2024 at 12:06:33PM +0300, Abel Vesa wrote:
> The PCIe 6a controller and PHY can be configured in 4-lanes mode or
> 2-lanes mode. For 4-lanes mode, it fetches the lanes provided by PCIe 6b.

nit: I still think you should use "uses" over "fetches" here.

> For 2-lanes mode, PCIe 6a uses 2 lanes and then PCIe 6b uses the other 2
> lanes. Configure it in 4-lane mode and then each board can configure it
> depending on the design. 

To avoid confusion you could avoid the word "configure" here. pcie6a is
a four-lane controller (and PHY) that can also be used in used in
two-lane mode, depending on how the system is configured and this can be
read out through a TCSR register (e.g. the PHY driver adapts
accordingly).

So you should perhaps rather say that you are fixing the description and
compatible of pcie6a, which *is* a 4-lane controller, that can also be
used in 2-lane mode. Or similar.

We also discussed this here:

	https://lore.kernel.org/lkml/ZtG2dUVkdwBpBbix@hovoldconsulting.com/

> Both the QCP and CRD boards, currently upstream,
> use PCIe 6a for NVMe in 4-lane mode. Mark the controller as 4-lane as
> well.

> This is the last change needed in order to support NVMe with Gen4
> 4-lanes on all existing X Elite boards.

I'd skip comments like this, or put them in the cover letter, or just
rephrase as "This will enable 4-lane NVMe on...".

> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Re-worded the commit message according to Johan's suggestions
> - Dropped the clocks changes.
> - Dropped the fixes tag as this relies on the Gen4 4-lanes stability
>   patchset which has been only merged in 6.12, so backporting this patch
>   would break NVMe support for all platforms.
> - Link to v1: https://lore.kernel.org/r/20240531-x1e80100-dts-fixes-pcie6a-v1-2-1573ebcae1e8@linaro.org

Patch itself looks good.

Johan
Abel Vesa Oct. 7, 2024, 12:01 p.m. UTC | #5 
On 24-10-07 13:19:33, Johan Hovold wrote:
> On Fri, Oct 04, 2024 at 12:06:33PM +0300, Abel Vesa wrote:
> > The PCIe 6a controller and PHY can be configured in 4-lanes mode or
> > 2-lanes mode. For 4-lanes mode, it fetches the lanes provided by PCIe 6b.
> 
> nit: I still think you should use "uses" over "fetches" here.

Urgh, I missed that one. Will fix.

> 
> > For 2-lanes mode, PCIe 6a uses 2 lanes and then PCIe 6b uses the other 2
> > lanes. Configure it in 4-lane mode and then each board can configure it
> > depending on the design. 
> 
> To avoid confusion you could avoid the word "configure" here. pcie6a is
> a four-lane controller (and PHY) that can also be used in used in
> two-lane mode, depending on how the system is configured and this can be
> read out through a TCSR register (e.g. the PHY driver adapts
> accordingly).

OK, will that.

> 
> So you should perhaps rather say that you are fixing the description and
> compatible of pcie6a, which *is* a 4-lane controller, that can also be
> used in 2-lane mode. Or similar.

Agreed. Will reword to say fixing the description as suggested.

Just to be sure. We still don't want this backported (even with such
rewording), so no fixes tag, right?

> 
> We also discussed this here:
> 
> 	https://lore.kernel.org/lkml/ZtG2dUVkdwBpBbix@hovoldconsulting.com/
> 
> > Both the QCP and CRD boards, currently upstream,
> > use PCIe 6a for NVMe in 4-lane mode. Mark the controller as 4-lane as
> > well.
> 
> > This is the last change needed in order to support NVMe with Gen4
> > 4-lanes on all existing X Elite boards.
> 
> I'd skip comments like this, or put them in the cover letter, or just
> rephrase as "This will enable 4-lane NVMe on...".

Will rephrase as suggested.

> 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> > Changes in v2:
> > - Re-worded the commit message according to Johan's suggestions
> > - Dropped the clocks changes.
> > - Dropped the fixes tag as this relies on the Gen4 4-lanes stability
> >   patchset which has been only merged in 6.12, so backporting this patch
> >   would break NVMe support for all platforms.
> > - Link to v1: https://lore.kernel.org/r/20240531-x1e80100-dts-fixes-pcie6a-v1-2-1573ebcae1e8@linaro.org
> 
> Patch itself looks good.
> 
> Johan

Thanks for reviewing.

Abel
Johan Hovold Oct. 7, 2024, 12:24 p.m. UTC | #6 
On Mon, Oct 07, 2024 at 03:01:53PM +0300, Abel Vesa wrote:
> On 24-10-07 13:19:33, Johan Hovold wrote:

> > So you should perhaps rather say that you are fixing the description and
> > compatible of pcie6a, which *is* a 4-lane controller, that can also be
> > used in 2-lane mode. Or similar.
> 
> Agreed. Will reword to say fixing the description as suggested.
> 
> Just to be sure. We still don't want this backported (even with such
> rewording), so no fixes tag, right?

We don't want this one backported (because of the missing deps) but you
can still add a Fixes tag. Just tell Sasha to drop the patch if autosel
picks it up anyway or use the new do-not-backport stable tag to achieve
the same:

	Cc: <stable+noautosel@kernel.org> # reason goes here, and must be present

See Documentation/process/stable-kernel-rules.rst.

Johan
Abel Vesa Oct. 7, 2024, 1:41 p.m. UTC | #7 
On 24-10-07 14:24:20, Johan Hovold wrote:
> On Mon, Oct 07, 2024 at 03:01:53PM +0300, Abel Vesa wrote:
> > On 24-10-07 13:19:33, Johan Hovold wrote:
> 
> > > So you should perhaps rather say that you are fixing the description and
> > > compatible of pcie6a, which *is* a 4-lane controller, that can also be
> > > used in 2-lane mode. Or similar.
> > 
> > Agreed. Will reword to say fixing the description as suggested.
> > 
> > Just to be sure. We still don't want this backported (even with such
> > rewording), so no fixes tag, right?
> 
> We don't want this one backported (because of the missing deps) but you
> can still add a Fixes tag. Just tell Sasha to drop the patch if autosel
> picks it up anyway or use the new do-not-backport stable tag to achieve
> the same:

Makes sense. Will do that.

> 
> 	Cc: <stable+noautosel@kernel.org> # reason goes here, and must be present
> 
> See Documentation/process/stable-kernel-rules.rst.
> 
> Johan

Thanks.

Abel
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index a36076e3c56b5b8815eb41ec55e2e1e5bd878201..4ec712cb7a26d8fe434631cf15949524fd22c7d9 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -2931,7 +2931,7 @@  pcie6a: pci@1bf8000 {
 			dma-coherent;
 
 			linux,pci-domain = <6>;
-			num-lanes = <2>;
+			num-lanes = <4>;
 
 			interrupts = <GIC_SPI 773 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 774 IRQ_TYPE_LEVEL_HIGH>,
@@ -2997,8 +2997,9 @@  &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 		};
 
 		pcie6a_phy: phy@1bfc000 {
-			compatible = "qcom,x1e80100-qmp-gen4x2-pcie-phy";
-			reg = <0 0x01bfc000 0 0x2000>;
+			compatible = "qcom,x1e80100-qmp-gen4x4-pcie-phy";
+			reg = <0 0x01bfc000 0 0x2000>,
+			      <0 0x01bfe000 0 0x2000>;
 
 			clocks = <&gcc GCC_PCIE_6A_PHY_AUX_CLK>,
 				 <&gcc GCC_PCIE_6A_CFG_AHB_CLK>,
@@ -3021,6 +3022,8 @@  pcie6a_phy: phy@1bfc000 {
 
 			power-domains = <&gcc GCC_PCIE_6_PHY_GDSC>;
 
+			qcom,4ln-config-sel = <&tcsr 0x1a000 0>;
+
 			#clock-cells = <0>;
 			clock-output-names = "pcie6a_pipe_clk";