| Message ID | 20240320071527.13443-1-quic_schintav@quicinc.com |
|---|---|
| Headers | show |
| Series | Add Gen4 equalization and margining settings | expand |
On 20.03.2024 08:14, Shashank Babu Chinta Venkata wrote: > Add rx margining settings for gen4 operation. Why are these necessary? What do they change? > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-designware.h | 23 +++++++++++++ > drivers/pci/controller/dwc/pcie-qcom-cmn.c | 35 ++++++++++++++++++++ > drivers/pci/controller/dwc/pcie-qcom-cmn.h | 11 +++++- > drivers/pci/controller/dwc/pcie-qcom-ep.c | 4 ++- > drivers/pci/controller/dwc/pcie-qcom.c | 4 ++- > 5 files changed, 74 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 064744bfe35a..ce1c5f9c406a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -206,6 +206,29 @@ > > #define PCIE_PL_CHK_REG_ERR_ADDR 0xB28 > > +/* > + * GEN4 lane margining register definitions > + */ > +#define GEN4_LANE_MARGINING_1_OFF 0xb80 > +#define MARGINING_MAX_VOLTAGE_OFFSET_MASK GENMASK(29, 24) > +#define MARGINING_NUM_VOLTAGE_STEPS_MASK GENMASK(22, 16) > +#define MARGINING_MAX_TIMING_OFFSET_MASK GENMASK(13, 8) > +#define MARGINING_NUM_TIMING_STEPS_MASK GENMASK(5, 0) > +#define MARGINING_MAX_VOLTAGE_OFFSET_SHIFT 24 > +#define MARGINING_NUM_VOLTAGE_STEPS_SHIFT 16 > +#define MARGINING_MAX_TIMING_OFFSET_SHIFT 8 > + > +#define GEN4_LANE_MARGINING_2_OFF 0xb84 The file drivers/accel/habanalabs/include/gaudi2/asic_reg/pcie_dbi_regs.h defines registers with exactly the same names at exacly the same offsets. If this is a DWC-common thing, it should go to DWC-common code. Konrad
On Wed, Mar 20, 2024 at 12:14:45AM -0700, Shashank Babu Chinta Venkata wrote: > Refactor common code from RC(Root Complex) and EP(End Point) Please add a space before the open parentheses above, these are not function calls. > drivers and move them to a common repository. This acts as placeholder > for common source code for both drivers avoiding duplication. > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c > @@ -0,0 +1,81 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved. > + * Copyright 2015, 2021 Linaro Limited. > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + * > + */ > + > +#include <linux/debugfs.h> Not needed. > +#include <linux/pci.h> > +#include <linux/interconnect.h> > + > +#include "../../pci.h" > +#include "pcie-designware.h" > +#include "pcie-qcom-cmn.h" > + > +#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \ > + Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed])) > + > +int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem) > +{ > + if (IS_ERR(pci)) > + return PTR_ERR(pci); Not needed. > + > + icc_mem = devm_of_icc_get(pci->dev, "pcie-mem"); > + if (IS_ERR(icc_mem)) > + return PTR_ERR(icc_mem); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(qcom_pcie_cmn_icc_get_resource); So this series was clearly never tested properly as the above function will leave the driver's icc_mem path uninitialised. You're passing in a NULL pointer by value and then update your local variable, which obviously has no effect for the caller. This means that all later icc operation become no-ops, which crashes machine like the Lenovo ThinkPad X13s and the x1e80100 CRD that depends on having a non-zero vote before enabling clocks at probe. How did this go unnoticed? I can only assume you did not test this series (in isolation) before posting? > +int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem) > +{ > + int ret; > + > + if (IS_ERR(pci)) > + return PTR_ERR(pci); > + > + if (IS_ERR(icc_mem)) > + return PTR_ERR(icc_mem); Neither is needed. > + > + /* > + * Some Qualcomm platforms require interconnect bandwidth constraints > + * to be set before enabling interconnect clocks. > + * > + * Set an initial peak bandwidth corresponding to single-lane Gen 1 > + * for the pcie-mem path. > + */ I'm not sure about hiding this away in a separate compilation unit. The above comments makes sense in the driver, where it's easy to see that the icc is initialised and the vote added before enabling clocks. Also these helpers are so small it may not even be worth trying to refactor them (all). > + ret = icc_set_bw(icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); > + if (ret) { > + dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n", > + ret); > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(qcom_pcie_cmn_icc_init); > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved. > + * Copyright 2015, 2021 Linaro Limited. > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/pci.h> > +#include "../../pci.h" > +#include "pcie-designware.h" > + Compile guards missing. > +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); Johan