From: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
To: Manivannan Sadhasivam <mani@kernel.org>
Cc: <andersson@kernel.org>, <konradybcio@kernel.org>,
<robh@kernel.org>, <krzk+dt@kernel.org>, <conor+dt@kernel.org>,
<jingoohan1@gmail.com>, <lpieralisi@kernel.org>,
<kwilczynski@kernel.org>, <bhelgaas@google.com>,
<johan+linaro@kernel.org>, <vkoul@kernel.org>,
<kishon@kernel.org>, <neil.armstrong@linaro.org>,
<abel.vesa@linaro.org>, <kw@linux.com>,
<linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
<linux-phy@lists.infradead.org>, <qiang.yu@oss.qualcomm.com>,
<quic_krichai@quicinc.com>, <quic_vbadigan@quicinc.com>
Subject: Re: [PATCH v3 1/3] PCI: qcom: Add equalization settings for 8.0 GT/s
Date: Mon, 30 Jun 2025 15:19:12 +0800 [thread overview]
Message-ID: <8ccd3731-8dbc-4972-a79a-ba78e90ec4a8@quicinc.com> (raw)
In-Reply-To: <uakd5br4e5l24xmb6rxqs2drlt3fcmemfjilxo7ozph6vysjzs@ag3wjtic3qfm>
On 6/25/2025 11:58 PM, Manivannan Sadhasivam wrote:
> On Wed, Jun 25, 2025 at 04:57:59PM +0800, Ziyue Zhang wrote:
>> Add lane equalization setting for 8.0 GT/s to enhance link stability and
>> aviod AER Correctable Errors reported on some platforms (eg. SA8775P).
>>
>> 8.0 GT/s and 16.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 to `RATE_SHADOW_SEL`.
>>
>> Hence program equalization registers in a loop using link speed as index,
>> so that equalization setting can be programmed for both 8.0 GT/s and
>> 16.0 GT/s.
>>
>> Co-developed-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
>> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
>> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
>> ---
>> drivers/pci/controller/dwc/pcie-designware.h | 1 -
>> drivers/pci/controller/dwc/pcie-qcom-common.c | 55 +++++++++++--------
>> 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, 38 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index ce9e18554e42..388306991467 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -127,7 +127,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..ed466496f077 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, max_speed = PCIE_SPEED_16_0GT;
>> + struct device *dev = pci->dev;
>>
>> /*
>> * GEN3_RELATED_OFF register is repurposed to apply equalization
>> @@ -19,32 +21,37 @@ 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);
>> + if (pcie_link_speed[pci->max_link_speed] < PCIE_SPEED_32_0GT)
>> + max_speed = pcie_link_speed[pci->max_link_speed];
> So the logic here is that you want to limit the max_speed to < 32 GT/s because
> you are not sure if 32 GT/s or more would require the same settings?
>
> If so, why can't you just simply bail out early if the link speed > 16 GT/s and
> just use pci->max_link_speed directly? Right now, 32 GT/s or more would be
> skipped implicitly because you have initialized max_speed to PCIE_SPEED_16_0GT.
>
> - Mani
Hi Mani
I'll update the code according to your feedback in my next submission.
BRs
Ziyue
>>
>> - 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 <= max_speed; ++speed) {
>> + 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_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_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 bf7c6ac0f3e3..aaf060bf39d4 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)
>> qcom_pcie_common_set_16gt_lane_margining(pci);
>> - }
>>
>> /*
>> * The physical address of the MMIO region which is exposed as the BAR
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index c789e3f85655..0fcb17ffd2e9 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -298,10 +298,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>> {
>> struct qcom_pcie *pcie = to_qcom_pcie(pci);
>>
>> - 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)
>> qcom_pcie_common_set_16gt_lane_margining(pci);
>> - }
>>
>> /* Enable Link Training state machine */
>> if (pcie->cfg->ops->ltssm_enable)
>> --
>> 2.34.1
>>
next prev parent reply other threads:[~2025-06-30 7:19 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-25 8:57 [PATCH v3 0/3] Add Equalization Settings for 8.0 GT/s and Add PCIe Lane Equalization Preset Properties for 8.0 GT/s and 16.0 GT/s Ziyue Zhang
2025-06-25 8:57 ` [PATCH v3 1/3] PCI: qcom: Add equalization settings for 8.0 GT/s Ziyue Zhang
2025-06-25 15:58 ` Manivannan Sadhasivam
2025-06-30 7:19 ` Ziyue Zhang [this message]
2025-06-25 19:15 ` kernel test robot
2025-06-25 8:58 ` [PATCH v3 2/3] PCI: qcom: fix macro typo for CURSOR Ziyue Zhang
2025-06-25 8:58 ` [PATCH v3 3/3] arm64: dts: qcom: sa8775p: Add PCIe lane equalization preset properties Ziyue Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8ccd3731-8dbc-4972-a79a-ba78e90ec4a8@quicinc.com \
--to=quic_ziyuzhan@quicinc.com \
--cc=abel.vesa@linaro.org \
--cc=andersson@kernel.org \
--cc=bhelgaas@google.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jingoohan1@gmail.com \
--cc=johan+linaro@kernel.org \
--cc=kishon@kernel.org \
--cc=konradybcio@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=kw@linux.com \
--cc=kwilczynski@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=lpieralisi@kernel.org \
--cc=mani@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=qiang.yu@oss.qualcomm.com \
--cc=quic_krichai@quicinc.com \
--cc=quic_vbadigan@quicinc.com \
--cc=robh@kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®