On Thu, Feb 29, 2024 at 09:11:35PM -0800, Shashank Babu Chinta Venkata wrote:
> GEN3_RELATED_OFFSET is being used as shadow register for generation4 and
> generation5 data rates based on rate select mask settings on this register.
> Select relevant mask and equalization settings for generation4 operation.
Please capitalize subject lines to match history ("PCI: qcom: Add ...")
s/GEN3_RELATED_OFFSET/GEN3_RELATED_OFF/ (I think?)
I wish these "GEN3_RELATED" things were named with the data rate
instead of "GEN3". The PCIe spec defines these things based on the
data rate (8GT/s, 16GT/s, etc), not the revision of the spec they
appeared in (gen3/gen4/etc).
Using "GEN3" means we have to first look up the "gen -> rate" mapping
before finding the relevant spec info.
Applies to the subject line, commit log, #defines, function names,
etc.
> +void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
> +{
> + u32 reg;
> +
> + reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
Warrants a one-line comment about using "GEN3_..." in a function named
"..._gen4_..." (But ideally both would be renamed based on the data
rate instead.)
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -9,10 +9,29 @@
> #include "../../pci.h"
> #include "pcie-designware.h"
>
> +#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)
Are these qcom-specific registers, or should they be added alongside
GEN3_RELATED_OFF in pcie-designware.h?
> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF 0x8ac
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL 0x5
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL 0x5
> +#define GEN3_EQ_FMDC_N_EVALS_VAL 0xD
> +#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
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> goto err_disable_resources;
> }
>
> + /* set Gen4 equalization settings */
Pointless comment.
> + if (pci->link_gen == 4)
> + qcom_pcie_cmn_set_gen4_eq_settings(pci);
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
> {
> struct qcom_pcie *pcie = to_qcom_pcie(pci);
>
> + /* set Gen4 equalization settings */
Pointless comment.
> + if (pci->link_gen == 4)
> + qcom_pcie_cmn_set_gen4_eq_settings(pci);
