[PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Qiang Yu]

Currently, the cfg_1_9_0 which is being used for X1E80100 has config_sid
callback in its ops and doesn't disable ASPM L0s. However, as same as
SC8280X, PCIe controllers on X1E80100 are connected to SMMUv3, hence don't
need config_sid() callback and hardware team has recommended to disable
L0s as it is broken in the controller. Hence reuse cfg_sc8280xp for
X1E80100.

Fixes: 6d0c39324c5f ("PCI: qcom: Add X1E80100 PCIe support")
Cc: stable@vger.kernel.org
Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 468bd4242e61..c533e6024ba2 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1847,7 +1847,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-sm8450-pcie0", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sm8550", .data = &cfg_1_9_0 },
-	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_1_9_0 },
+	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_sc8280xp },
 	{ }
 };
 
-- 
2.34.1

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Johan Hovold]

Please use a more concise subject (e.g. try to stay within 72 chars)
than:

	PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

Here you could drop "SoC", maybe "ASPM" and "config" for example.

On Wed, Oct 16, 2024 at 08:04:11PM -0700, Qiang Yu wrote:
> Currently, the cfg_1_9_0 which is being used for X1E80100 has config_sid
> callback in its ops and doesn't disable ASPM L0s. However, as same as
> SC8280X, PCIe controllers on X1E80100 are connected to SMMUv3, hence don't
> need config_sid() callback and hardware team has recommended to disable
> L0s as it is broken in the controller. Hence reuse cfg_sc8280xp for
> X1E80100.

Since the x1e80100 dtsi, like sc8280xp, do not specify an iommu-map,
that bit is effectively just a cleanup and all this patch does is to
disable L0s.

Please rephrase to make this clear. This will also allow you to make the
Subject even shorter (no need to mention the SID bit in Subject).

Also say something about how L0s is broken so that it is more clear what
the effect of this patch is. On sc8280xp enabling L0s lead to
correctable errors for example.

> Fixes: 6d0c39324c5f ("PCI: qcom: Add X1E80100 PCIe support")
> Cc: stable@vger.kernel.org

Johan

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Qiang Yu]

On 10/18/2024 10:06 PM, Johan Hovold wrote:
> Please use a more concise subject (e.g. try to stay within 72 chars)
> than:
>
> 	PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC
>
> Here you could drop "SoC", maybe "ASPM" and "config" for example.
>
> On Wed, Oct 16, 2024 at 08:04:11PM -0700, Qiang Yu wrote:
>> Currently, the cfg_1_9_0 which is being used for X1E80100 has config_sid
>> callback in its ops and doesn't disable ASPM L0s. However, as same as
>> SC8280X, PCIe controllers on X1E80100 are connected to SMMUv3, hence don't
>> need config_sid() callback and hardware team has recommended to disable
>> L0s as it is broken in the controller. Hence reuse cfg_sc8280xp for
>> X1E80100.
> Since the x1e80100 dtsi, like sc8280xp, do not specify an iommu-map,
> that bit is effectively just a cleanup and all this patch does is to
> disable L0s.
>
> Please rephrase to make this clear. This will also allow you to make the
> Subject even shorter (no need to mention the SID bit in Subject).
>
> Also say something about how L0s is broken so that it is more clear what
> the effect of this patch is. On sc8280xp enabling L0s lead to
> correctable errors for example.
Need more time to confirm the exact reason about disabling L0s.
Will update if get any progress

Thanks,
Qiang
>
>> Fixes: 6d0c39324c5f ("PCI: qcom: Add X1E80100 PCIe support")
>> Cc: stable@vger.kernel.org
> Johan

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Qiang Yu]

On 10/24/2024 2:42 PM, Qiang Yu wrote:
>
> On 10/18/2024 10:06 PM, Johan Hovold wrote:
>> Please use a more concise subject (e.g. try to stay within 72 chars)
>> than:
>>
>>     PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for 
>> X1E80100 SoC
>>
>> Here you could drop "SoC", maybe "ASPM" and "config" for example.
>>
>> On Wed, Oct 16, 2024 at 08:04:11PM -0700, Qiang Yu wrote:
>>> Currently, the cfg_1_9_0 which is being used for X1E80100 has 
>>> config_sid
>>> callback in its ops and doesn't disable ASPM L0s. However, as same as
>>> SC8280X, PCIe controllers on X1E80100 are connected to SMMUv3, hence 
>>> don't
>>> need config_sid() callback and hardware team has recommended to disable
>>> L0s as it is broken in the controller. Hence reuse cfg_sc8280xp for
>>> X1E80100.
>> Since the x1e80100 dtsi, like sc8280xp, do not specify an iommu-map,
>> that bit is effectively just a cleanup and all this patch does is to
>> disable L0s.
>>
>> Please rephrase to make this clear. This will also allow you to make the
>> Subject even shorter (no need to mention the SID bit in Subject).
>>
>> Also say something about how L0s is broken so that it is more clear what
>> the effect of this patch is. On sc8280xp enabling L0s lead to
>> correctable errors for example.
> Need more time to confirm the exact reason about disabling L0s.
> Will update if get any progress
Hi Johan Hovold

I confirmed with HW team and SW team. L0s is not supported on X1E80100, 
it is not fully verified. So we don't want to enable it.

Thanks,
Qiang Yu
>
> Thanks,
> Qiang
>>
>>> Fixes: 6d0c39324c5f ("PCI: qcom: Add X1E80100 PCIe support")
>>> Cc: stable@vger.kernel.org
>> Johan

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Johan Hovold]

On Wed, Oct 30, 2024 at 01:54:59PM +0800, Qiang Yu wrote:
> On 10/24/2024 2:42 PM, Qiang Yu wrote:
> > On 10/18/2024 10:06 PM, Johan Hovold wrote:

> >> Also say something about how L0s is broken so that it is more clear what
> >> the effect of this patch is. On sc8280xp enabling L0s lead to
> >> correctable errors for example.

> > Need more time to confirm the exact reason about disabling L0s.
> > Will update if get any progress

> I confirmed with HW team and SW team. L0s is not supported on X1E80100, 
> it is not fully verified. So we don't want to enable it.

Thanks for checking. A word about what can happen if not disabling it
may still be in place (e.g. the link state transition stats in debugfs
on x1e80100 looked pretty erratic with L0s enabled IIRC).

Also, are there any Qualcomm platforms that actually support L0s?
Perhaps we should just disable it everywhere?

Johan

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Manivannan Sadhasivam]

On Wed, Oct 30, 2024 at 08:15:05AM +0100, Johan Hovold wrote:
> On Wed, Oct 30, 2024 at 01:54:59PM +0800, Qiang Yu wrote:
> > On 10/24/2024 2:42 PM, Qiang Yu wrote:
> > > On 10/18/2024 10:06 PM, Johan Hovold wrote:
> 
> > >> Also say something about how L0s is broken so that it is more clear what
> > >> the effect of this patch is. On sc8280xp enabling L0s lead to
> > >> correctable errors for example.
> 
> > > Need more time to confirm the exact reason about disabling L0s.
> > > Will update if get any progress
> 
> > I confirmed with HW team and SW team. L0s is not supported on X1E80100, 
> > it is not fully verified. So we don't want to enable it.
> 
> Thanks for checking. A word about what can happen if not disabling it
> may still be in place (e.g. the link state transition stats in debugfs
> on x1e80100 looked pretty erratic with L0s enabled IIRC).
> 
> Also, are there any Qualcomm platforms that actually support L0s?
> Perhaps we should just disable it everywhere?
> 

Most of the mobile chipsets from Qcom support L0s. It is not supported only on
the compute ones. So we cannot disable it everywhere.

Again, it is not the hw issue but the PHY init sequence not tuned support L0s.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Johan Hovold]

On Wed, Oct 30, 2024 at 12:48:51PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Oct 30, 2024 at 08:15:05AM +0100, Johan Hovold wrote:

> > Also, are there any Qualcomm platforms that actually support L0s?
> > Perhaps we should just disable it everywhere?
> 
> Most of the mobile chipsets from Qcom support L0s. It is not supported only on
> the compute ones. So we cannot disable it everywhere.
> 
> Again, it is not the hw issue but the PHY init sequence not tuned support L0s.

Right, this should be mentioned in the commit message.

Johan

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Qiang Yu]

On 10/30/2024 3:42 PM, Johan Hovold wrote:
> On Wed, Oct 30, 2024 at 12:48:51PM +0530, Manivannan Sadhasivam wrote:
>> On Wed, Oct 30, 2024 at 08:15:05AM +0100, Johan Hovold wrote:
>>> Also, are there any Qualcomm platforms that actually support L0s?
>>> Perhaps we should just disable it everywhere?
>> Most of the mobile chipsets from Qcom support L0s. It is not supported only on
>> the compute ones. So we cannot disable it everywhere.
>>
>> Again, it is not the hw issue but the PHY init sequence not tuned support L0s.
> Right, this should be mentioned in the commit message.
OK, I got it. Will write this into commit message.

Thanks,
Qiang Yu
>
> Johan

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Bjorn Andersson]

On Wed, Oct 16, 2024 at 08:04:11PM -0700, Qiang Yu wrote:
> Currently, the cfg_1_9_0 which is being used for X1E80100 has config_sid
> callback in its ops and doesn't disable ASPM L0s. However, as same as
> SC8280X, PCIe controllers on X1E80100 are connected to SMMUv3, hence don't

Would be nice to document the connection between SMMUv3 and "don't need
config_sid()" is because we don't have support for the SMMUv3.

> need config_sid() callback and hardware team has recommended to disable
> L0s as it is broken in the controller. Hence reuse cfg_sc8280xp for

I expect that config_sid() and "disable L0s" are two separate issues.
I'm fine with you solving both in a single commit, but I'd prefer the
two subjects to be covered in at least two separate sentences.

Regards,
Bjorn

> X1E80100.
> 
> Fixes: 6d0c39324c5f ("PCI: qcom: Add X1E80100 PCIe support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 468bd4242e61..c533e6024ba2 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1847,7 +1847,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sm8450-pcie0", .data = &cfg_1_9_0 },
>  	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 },
>  	{ .compatible = "qcom,pcie-sm8550", .data = &cfg_1_9_0 },
> -	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_1_9_0 },
> +	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_sc8280xp },
>  	{ }
>  };
>  
> -- 
> 2.34.1
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v7 6/7] PCI: qcom: Disable ASPM L0s and remove BDF2SID mapping config for X1E80100 SoC

[Qiang Yu]

On 10/18/2024 10:25 PM, Bjorn Andersson wrote:
> On Wed, Oct 16, 2024 at 08:04:11PM -0700, Qiang Yu wrote:
>> Currently, the cfg_1_9_0 which is being used for X1E80100 has config_sid
>> callback in its ops and doesn't disable ASPM L0s. However, as same as
>> SC8280X, PCIe controllers on X1E80100 are connected to SMMUv3, hence don't
> Would be nice to document the connection between SMMUv3 and "don't need
> config_sid()" is because we don't have support for the SMMUv3.
We don't need config_sid because we have support for SMMUv3 on HW.
SMMUv3 is able to use BDF as SID, so BDF2SID mapping is not required
and removed on HW.

Thanks,
Qiang
>> need config_sid() callback and hardware team has recommended to disable
>> L0s as it is broken in the controller. Hence reuse cfg_sc8280xp for
> I expect that config_sid() and "disable L0s" are two separate issues.
> I'm fine with you solving both in a single commit, but I'd prefer the
> two subjects to be covered in at least two separate sentences.
>
> Regards,
> Bjorn
>
>> X1E80100.
>>
>> Fixes: 6d0c39324c5f ("PCI: qcom: Add X1E80100 PCIe support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 468bd4242e61..c533e6024ba2 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1847,7 +1847,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>>   	{ .compatible = "qcom,pcie-sm8450-pcie0", .data = &cfg_1_9_0 },
>>   	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 },
>>   	{ .compatible = "qcom,pcie-sm8550", .data = &cfg_1_9_0 },
>> -	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_1_9_0 },
>> +	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_sc8280xp },
>>   	{ }
>>   };
>>   
>> -- 
>> 2.34.1
>>
>>
>> -- 
>> linux-phy mailing list
>> linux-phy@lists.infradead.org
>> https://lists.infradead.org/mailman/listinfo/linux-phy