| 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: <20240402054538.GJ2933@thinkpad>
Date: Tue, 2 Apr 2024 11:15:38 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Shashank Babu Chinta Venkata <quic_schintav@...cinc.com>
Cc: agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
quic_msarkar@...cinc.com, quic_kraravin@...cinc.com,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
Jingoo Han <jingoohan1@...il.com>,
Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
Serge Semin <fancer.lancer@...il.com>,
Conor Dooley <conor.dooley@...rochip.com>,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 2/3] PCI: qcom: Add equalization settings for gen4
On Wed, Mar 20, 2024 at 12:14:46AM -0700, Shashank Babu Chinta Venkata wrote:
Here, you are referring to 16 GT/s as the Gen4 datarate. So please mention that
explicitly.
> GEN3_RELATED_OFFSET is being used as shadow register for generation4 and
What is 'GEN3_RELATED_OFFSET' register? Where it is defined? More info on this
register would be helpful.
> generation5 data rates based on rate select mask settings on this register.
Please reword this sentence to make it more readable.
> Select relevant mask and equalization settings for generation4 operation.
>
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@...cinc.com>
> ---
> drivers/pci/controller/dwc/pcie-designware.h | 15 ++++++++
> drivers/pci/controller/dwc/pcie-qcom-cmn.c | 36 ++++++++++++++++++++
> drivers/pci/controller/dwc/pcie-qcom-cmn.h | 5 +++
> drivers/pci/controller/dwc/pcie-qcom-ep.c | 3 ++
> drivers/pci/controller/dwc/pcie-qcom.c | 3 ++
> 5 files changed, 62 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 26dae4837462..064744bfe35a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -122,6 +122,21 @@
> #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT 24
> #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK GENMASK(25, 24)
>
> +#define GEN3_EQ_CONTROL_OFF 0x8a8
> +#define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK GENMASK(3, 0)
> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE BIT(4)
> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK GENMASK(23, 8)
> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL BIT(24)
> +
> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF 0x8ac
> +#define GEN3_EQ_FMDC_T_MIN_PHASE23_MASK GENMASK(4, 0)
> +#define GEN3_EQ_FMDC_N_EVALS_MASK GENMASK(9, 5)
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK GENMASK(13, 10)
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK GENMASK(17, 14)
> +#define GEN3_EQ_FMDC_N_EVALS_SHIFT 5
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT 10
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT 14
> +
You are adding the definitions in designware header, but funtion definitions in
Qcom driver. Does this mean, this configuration is specific to Qcom and not
applicable to other DWC drivers?
> #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0
> #define PORT_MLTI_UPCFG_SUPPORT BIT(7)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.c b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> index 64fa412ec293..208a55e8e9a1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> @@ -17,6 +17,42 @@
> #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
>
> +void qcom_pcie_cmn_set_16gt_eq_settings(struct dw_pcie *pci)
> +{
> + u32 reg;
> +
> + /*
> + * GEN3_RELATED_OFF is repurposed to be used with GEN4(16GT/s) rate
> + * as well based on RATE_SHADOW_SEL_MASK settings on this register.
> + */
> + 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 |= (0x1 << GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT);
Please use FIELD_* macros where applicable.
> + 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_MASK;
> + reg &= ~GEN3_EQ_FMDC_N_EVALS_MASK;
> + reg |= (GEN3_EQ_FMDC_N_EVALS_16GT_VAL <<
> + GEN3_EQ_FMDC_N_EVALS_SHIFT);
> + reg &= ~GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK;
> + reg |= (GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_16GT_VAL <<
> + GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT);
> + reg &= ~GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK;
> + reg |= (GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_16GT_VAL <<
> + GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT);
> + 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_MASK;
> + reg &= ~GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE;
> + reg &= ~GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL;
> + reg &= ~GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK;
> + dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_cmn_set_16gt_eq_settings);
> +
> int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> {
> if (IS_ERR(pci))
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.h b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> index 845eda23ae59..97302e8fafa8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -9,6 +9,11 @@
> #include "../../pci.h"
> #include "pcie-designware.h"
>
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_16GT_VAL 0x5
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_16GT_VAL 0x5
> +#define GEN3_EQ_FMDC_N_EVALS_16GT_VAL 0xD
> +
So these settings are Qcom specific? I'd expect this to be documented in commit
message.
> int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem);
> int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
> void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
> +void qcom_pcie_cmn_set_16gt_eq_settings(struct dw_pcie *pci);
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index ce6343426de8..b6bcab21bb9f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -438,6 +438,9 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> goto err_disable_resources;
> }
>
> + if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
> + qcom_pcie_cmn_set_16gt_eq_settings(pci);
So this relies on the optional 'max-link-speed' DT property, but not mentioned
in the commit message :/
- Mani
> +
> /*
> * The physical address of the MMIO region which is exposed as the BAR
> * should be written to MHI BASE registers.
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 57a08294c561..b0a22a000fa3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -263,6 +263,9 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
> {
> struct qcom_pcie *pcie = to_qcom_pcie(pci);
>
> + if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
> + qcom_pcie_cmn_set_16gt_eq_settings(pci);
> +
> /* Enable Link Training state machine */
> if (pcie->cfg->ops->ltssm_enable)
> pcie->cfg->ops->ltssm_enable(pcie);
> --
> 2.43.2
>
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists