| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b43382fc-a2c3-4c6b-b462-0cabf7a2103d@oss.qualcomm.com>
Date: Fri, 22 Aug 2025 18:05:04 +0800
From: Qiang Yu <qiang.yu@....qualcomm.com>
To: Manivannan Sadhasivam <mani@...nel.org>,
Ziyue Zhang <ziyue.zhang@....qualcomm.com>
Cc: andersson@...nel.org, konradybcio@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, jingoohan1@...il.com,
lpieralisi@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
johan+linaro@...nel.org, vkoul@...nel.org, kishon@...nel.org,
neil.armstrong@...aro.org, abel.vesa@...aro.org, kw@...ux.com,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-phy@...ts.infradead.org, quic_krichai@...cinc.com,
quic_vbadigan@...cinc.com
Subject: Re: [PATCH v5 1/3] PCI: qcom: Add equalization settings for 8.0 GT/s
and 32.0 GT/s
On 8/22/2025 4:06 PM, Manivannan Sadhasivam wrote:
> On Tue, Aug 19, 2025 at 03:16:46PM GMT, Ziyue Zhang wrote:
>> Add lane equalization setting for 8.0 GT/s and 32.0 GT/s to enhance link
>> stability and avoid AER Correctable Errors reported on some platforms
>> (eg. SA8775P).
>>
>
> So this is fixing an issue, right? Then you should add relevant Fixes tag. I
> guess the tag here would be the commit that added SA8775p.
>
>> 8.0 GT/s, 16.0 GT/s and 32.0 GT/s require the same equalization setting.
>> This setting is programmed into a group of shadow registers, which can be
>> switched to configure equalization for different speeds by writing 00b,
>> 01b and 10b to `RATE_SHADOW_SEL`.
>>
>> Hence program equalization registers in a loop using link speed as index,
>> so that equalization setting can be programmed for 8.0 GT/s, 16.0 GT/s
>> and 32.0 GT/s.
>>
>> Co-developed-by: Qiang Yu <qiang.yu@....qualcomm.com>
>> Signed-off-by: Qiang Yu <qiang.yu@....qualcomm.com>
>> Signed-off-by: Ziyue Zhang <ziyue.zhang@....qualcomm.com>
>> ---
>> drivers/pci/controller/dwc/pcie-designware.h | 1 -
>> drivers/pci/controller/dwc/pcie-qcom-common.c | 58 +++++++++++--------
>> drivers/pci/controller/dwc/pcie-qcom-common.h | 2 +-
>> drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 +-
>> drivers/pci/controller/dwc/pcie-qcom.c | 6 +-
>> 5 files changed, 41 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index b5e7e18138a6..11de844428e5 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -123,7 +123,6 @@
>> #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE BIT(16)
>> #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT 24
>> #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK GENMASK(25, 24)
>> -#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_16_0GT 0x1
>>
>> #define GEN3_EQ_CONTROL_OFF 0x8A8
>> #define GEN3_EQ_CONTROL_OFF_FB_MODE GENMASK(3, 0)
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
>> index 3aad19b56da8..cb98e66d81d9 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
>> @@ -8,9 +8,11 @@
>> #include "pcie-designware.h"
>> #include "pcie-qcom-common.h"
>>
>> -void qcom_pcie_common_set_16gt_equalization(struct dw_pcie *pci)
>> +void qcom_pcie_common_set_equalization(struct dw_pcie *pci)
>> {
>> u32 reg;
>> + u16 speed;
>> + struct device *dev = pci->dev;
>
> Reverse Xmas order please.
>
>>
>> /*
>> * GEN3_RELATED_OFF register is repurposed to apply equalization
>> @@ -19,32 +21,40 @@ void qcom_pcie_common_set_16gt_equalization(struct dw_pcie *pci)
>> * determines the data rate for which these equalization settings are
>> * applied.
>> */
>> - reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
>> - reg &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
>> - reg &= ~GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK;
>> - reg |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK,
>> - GEN3_RELATED_OFF_RATE_SHADOW_SEL_16_0GT);
>> - dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, reg);
>>
>> - reg = dw_pcie_readl_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF);
>> - reg &= ~(GEN3_EQ_FMDC_T_MIN_PHASE23 |
>> - GEN3_EQ_FMDC_N_EVALS |
>> - GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA |
>> - GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA);
>> - reg |= FIELD_PREP(GEN3_EQ_FMDC_T_MIN_PHASE23, 0x1) |
>> - FIELD_PREP(GEN3_EQ_FMDC_N_EVALS, 0xd) |
>> - FIELD_PREP(GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA, 0x5) |
>> - FIELD_PREP(GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA, 0x5);
>> - dw_pcie_writel_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF, reg);
>> + for (speed = PCIE_SPEED_8_0GT; speed <= pcie_link_speed[pci->max_link_speed]; ++speed) {
>> + if (speed > PCIE_SPEED_32_0GT) {
>> + dev_warn(dev, "Skipped equalization settings for speeds higher than 32.0 GT/s\n");
>> + break;
>> + }
>>
>> - reg = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
>> - reg &= ~(GEN3_EQ_CONTROL_OFF_FB_MODE |
>> - GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE |
>> - GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL |
>> - GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
>> - dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
>> + reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
>> + reg &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
>> + reg &= ~GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK;
>> + reg |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK,
>> + speed - PCIE_SPEED_8_0GT);
>> + dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, reg);
>> +
>> + reg = dw_pcie_readl_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF);
>> + reg &= ~(GEN3_EQ_FMDC_T_MIN_PHASE23 |
>> + GEN3_EQ_FMDC_N_EVALS |
>> + GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA |
>> + GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA);
>> + reg |= FIELD_PREP(GEN3_EQ_FMDC_T_MIN_PHASE23, 0x1) |
>> + FIELD_PREP(GEN3_EQ_FMDC_N_EVALS, 0xd) |
>> + FIELD_PREP(GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA, 0x5) |
>> + FIELD_PREP(GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA, 0x5);
>> + dw_pcie_writel_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF, reg);
>> +
>> + reg = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
>> + reg &= ~(GEN3_EQ_CONTROL_OFF_FB_MODE |
>> + GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE |
>> + GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL |
>> + GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
>> + dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
>> + }
>> }
>> -EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_equalization);
>> +EXPORT_SYMBOL_GPL(qcom_pcie_common_set_equalization);
>>
>> void qcom_pcie_common_set_16gt_lane_margining(struct dw_pcie *pci)
>> {
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
>> index 7d88d29e4766..7f5ca2fd9a72 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-common.h
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
>> @@ -8,7 +8,7 @@
>>
>> struct dw_pcie;
>>
>> -void qcom_pcie_common_set_16gt_equalization(struct dw_pcie *pci);
>> +void qcom_pcie_common_set_equalization(struct dw_pcie *pci);
>> void qcom_pcie_common_set_16gt_lane_margining(struct dw_pcie *pci);
>>
>> #endif
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> index 60afb4d0134c..aeb166f68d55 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -511,10 +511,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>> goto err_disable_resources;
>> }
>>
>> - if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT) {
>> - qcom_pcie_common_set_16gt_equalization(pci);
>> + qcom_pcie_common_set_equalization(pci);
>> +
>> + if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT)
>
> This condition has existed even before this patch, but just noticing this
> possible issue. So if 'max_link_speed' is > 16 GT/s, we do not need to set lane
> margining? We used the same logic to set equalization setting earlier also.
Lane margining is supported for 16 GT/s and 32 GT/s. The settings are
dependent on phy design. For a specific phy, they have same settings
for 16 GT/s and 32 GT/s. Perhaps we can get the settings from devicetree
and program them in a loop.
But I'm not sure why we need to program it. It will no affect singal
quality and only required when user wants to collect margining info.
- Qiang Yu
>
> - Mani
>
Powered by blists - more mailing lists