| Message ID | 20240904-pci-qcom-gen4-stability-v6-3-ec39f7ae3f62@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | PCI: qcom: Add 16.0 GT/s equalization and margining settings | expand |
On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > > During high data transmission rates such as 16.0 GT/s, there is an > increased risk of signal loss due to poor channel quality and interference. > This can impact receiver's ability to capture signals accurately. Hence, > signal compensation is achieved through appropriate lane equalization > settings at both transmitter and receiver. This will result in increased > PCIe signal strength. > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > [mani: dropped the code refactoring and minor changes] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > +#define GEN3_EQ_CONTROL_OFF 0x8a8 Nit: uppercase hex since that's what is used for the other offsets > +#define GEN3_EQ_CONTROL_OFF_FB_MODE GENMASK(3, 0) > +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE BIT(4) > +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC GENMASK(23, 8) > +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL BIT(24) > + > +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF 0x8ac Nit: odd indentation uses spaces, uppercase > +#define GEN3_EQ_FMDC_T_MIN_PHASE23 GENMASK(4, 0) > +#define GEN3_EQ_FMDC_N_EVALS GENMASK(9, 5) > +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA GENMASK(13, 10) > +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA GENMASK(17, 14) > + > #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 > #define PORT_MLTI_UPCFG_SUPPORT BIT(7) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c > new file mode 100644 > index 000000000000..dc7d93db9dc5 > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c > @@ -0,0 +1,45 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/pci.h> > + > +#include "pcie-designware.h" > +#include "pcie-qcom-common.h" > + > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) > +{ > + u32 reg; > + > + /* > + * GEN3_RELATED_OFF register is repurposed to apply equalization > + * settings at various data transmission rates through registers namely > + * GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF determines > + * data rate for which this equalization settings are applied. *The* RATE_SHADOW_SEL bit field *the* data rate s/this/these/ > + */ > + 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, 0x1); How does 0x1 map to gen4/16 GT? > + 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_eq_settings); > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h > new file mode 100644 > index 000000000000..259e04b7bdf9 > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h > @@ -0,0 +1,8 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include "pcie-designware.h" You only need a forward declaration: struct dw_pcie; > + > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci); Compile guard still missing. Johan
On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote: > On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > > > > During high data transmission rates such as 16.0 GT/s, there is an > > increased risk of signal loss due to poor channel quality and interference. > > This can impact receiver's ability to capture signals accurately. Hence, > > signal compensation is achieved through appropriate lane equalization > > settings at both transmitter and receiver. This will result in increased > > PCIe signal strength. > > > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > [mani: dropped the code refactoring and minor changes] > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > +#define GEN3_EQ_CONTROL_OFF 0x8a8 > > Nit: uppercase hex since that's what is used for the other offsets > > > +#define GEN3_EQ_CONTROL_OFF_FB_MODE GENMASK(3, 0) > > +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE BIT(4) > > +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC GENMASK(23, 8) > > +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL BIT(24) > > + > > +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF 0x8ac > > Nit: odd indentation uses spaces, uppercase > > > +#define GEN3_EQ_FMDC_T_MIN_PHASE23 GENMASK(4, 0) > > +#define GEN3_EQ_FMDC_N_EVALS GENMASK(9, 5) > > +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA GENMASK(13, 10) > > +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA GENMASK(17, 14) > > + > > #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 > > #define PORT_MLTI_UPCFG_SUPPORT BIT(7) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c > > new file mode 100644 > > index 000000000000..dc7d93db9dc5 > > --- /dev/null > > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c > > @@ -0,0 +1,45 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > > + */ > > + > > +#include <linux/pci.h> > > + > > +#include "pcie-designware.h" > > +#include "pcie-qcom-common.h" > > + > > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) > > +{ > > + u32 reg; > > + > > + /* > > + * GEN3_RELATED_OFF register is repurposed to apply equalization > > + * settings at various data transmission rates through registers namely > > + * GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF determines > > + * data rate for which this equalization settings are applied. > > *The* RATE_SHADOW_SEL bit field > > *the* data rate > > s/this/these/ > > > + */ > > + 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, 0x1); > > How does 0x1 map to gen4/16 GT? > I need inputs from Shashank here as I don't know the answer. - Mani > > + 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_eq_settings); > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h > > new file mode 100644 > > index 000000000000..259e04b7bdf9 > > --- /dev/null > > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h > > @@ -0,0 +1,8 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > > + */ > > + > > +#include "pcie-designware.h" > > You only need a forward declaration: > > struct dw_pcie; > > > + > > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci); > > Compile guard still missing. > > Johan
On 9/4/24 08:52, Manivannan Sadhasivam wrote: > On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote: >> On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote: >>> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> >>> >>> During high data transmission rates such as 16.0 GT/s, there is an >>> increased risk of signal loss due to poor channel quality and interference. >>> This can impact receiver's ability to capture signals accurately. Hence, >>> signal compensation is achieved through appropriate lane equalization >>> settings at both transmitter and receiver. This will result in increased >>> PCIe signal strength. >>> >>> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> >>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >>> [mani: dropped the code refactoring and minor changes] >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> >>> +#define GEN3_EQ_CONTROL_OFF 0x8a8 >> >> Nit: uppercase hex since that's what is used for the other offsets >> >>> +#define GEN3_EQ_CONTROL_OFF_FB_MODE GENMASK(3, 0) >>> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE BIT(4) >>> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC GENMASK(23, 8) >>> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL BIT(24) >>> + >>> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF 0x8ac >> >> Nit: odd indentation uses spaces, uppercase >> >>> +#define GEN3_EQ_FMDC_T_MIN_PHASE23 GENMASK(4, 0) >>> +#define GEN3_EQ_FMDC_N_EVALS GENMASK(9, 5) >>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA GENMASK(13, 10) >>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA GENMASK(17, 14) >>> + >>> #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 >>> #define PORT_MLTI_UPCFG_SUPPORT BIT(7) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c >>> new file mode 100644 >>> index 000000000000..dc7d93db9dc5 >>> --- /dev/null >>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c >>> @@ -0,0 +1,45 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. >>> + */ >>> + >>> +#include <linux/pci.h> >>> + >>> +#include "pcie-designware.h" >>> +#include "pcie-qcom-common.h" >>> + >>> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) >>> +{ >>> + u32 reg; >>> + >>> + /* >>> + * GEN3_RELATED_OFF register is repurposed to apply equalization >>> + * settings at various data transmission rates through registers namely >>> + * GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF determines >>> + * data rate for which this equalization settings are applied. >> >> *The* RATE_SHADOW_SEL bit field >> >> *the* data rate >> >> s/this/these/ >> >>> + */ >>> + 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, 0x1); >> >> How does 0x1 map to gen4/16 GT? >> > > I need inputs from Shashank here as I don't know the answer. > > - Mani GEN3_RELATED_OFF has been repurposed to use with multiple data rates. RATE_SHADOW_SEL_MASK on GEN3_RELATED_OFF value decides the data rate of shadow registers namely GEN3_EQ_* registers. Per documentation 0x0 maps to 8 GT/s, 0x1 maps to 16 GT/s and 0x2 maps to 32 GT/s. > >>> + 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_eq_settings); >>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h >>> new file mode 100644 >>> index 000000000000..259e04b7bdf9 >>> --- /dev/null >>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h >>> @@ -0,0 +1,8 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. >>> + */ >>> + >>> +#include "pcie-designware.h" >> >> You only need a forward declaration: >> >> struct dw_pcie; >> >>> + >>> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci); >> >> Compile guard still missing. >> >> Johan >
On Wed, Sep 04, 2024 at 01:46:09PM -0700, Shashank Babu Chinta Venkata wrote: > On 9/4/24 08:52, Manivannan Sadhasivam wrote: > > On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote: > >> On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > >>> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > >>> + /* > >>> + * GEN3_RELATED_OFF register is repurposed to apply equalization > >>> + * settings at various data transmission rates through registers namely > >>> + * GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF determines > >>> + * data rate for which this equalization settings are applied. > >>> + reg |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK, 0x1); > >> > >> How does 0x1 map to gen4/16 GT? > GEN3_RELATED_OFF has been repurposed to use with multiple data rates. > RATE_SHADOW_SEL_MASK on GEN3_RELATED_OFF value decides the data rate > of shadow registers namely GEN3_EQ_* registers. Per documentation 0x0 > maps to 8 GT/s, 0x1 maps to 16 GT/s and 0x2 maps to 32 GT/s. Thanks for clarifying. Perhaps these should become defines eventually (or the comment could be extended). There are a lot of "magic" constants in here. Johan
On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote: > On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > > > > During high data transmission rates such as 16.0 GT/s, there is an > > increased risk of signal loss due to poor channel quality and interference. > > This can impact receiver's ability to capture signals accurately. Hence, > > signal compensation is achieved through appropriate lane equalization > > settings at both transmitter and receiver. This will result in increased > > PCIe signal strength. > > > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > [mani: dropped the code refactoring and minor changes] > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> [...] > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h > > new file mode 100644 > > index 000000000000..259e04b7bdf9 > > --- /dev/null > > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h > > @@ -0,0 +1,8 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > > + */ > > + > > +#include "pcie-designware.h" > > You only need a forward declaration: > > struct dw_pcie; > > > + > > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci); > > Compile guard still missing. > Perhaps we can just get rid of the Kconfig entry and build it by default for both RC and EP drivers? I don't see a value in building it as a separate module. And we may also move more common code in the future. - Mani
On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote: > On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote: > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h > > > new file mode 100644 > > > index 000000000000..259e04b7bdf9 > > > --- /dev/null > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h > > > @@ -0,0 +1,8 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* > > > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > > > + */ > > > + > > > +#include "pcie-designware.h" > > > > You only need a forward declaration: > > > > struct dw_pcie; > > > > > + > > > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci); > > > > Compile guard still missing. Sorry, I meant to say *include* guard here. > Perhaps we can just get rid of the Kconfig entry and build it by default for > both RC and EP drivers? I don't see a value in building it as a separate module. > And we may also move more common code in the future. It is already built by default for both drivers. I'm not sure what you're suggesting here. Johan
On Thu, Sep 05, 2024 at 06:27:36PM +0200, Johan Hovold wrote: > On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote: > > On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote: > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h > > > > new file mode 100644 > > > > index 000000000000..259e04b7bdf9 > > > > --- /dev/null > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h > > > > @@ -0,0 +1,8 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +/* > > > > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > > > > + */ > > > > + > > > > +#include "pcie-designware.h" > > > > > > You only need a forward declaration: > > > > > > struct dw_pcie; > > > > > > > + > > > > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci); > > > > > > Compile guard still missing. > > Sorry, I meant to say *include* guard here. > Okay. I got confused initially. > > Perhaps we can just get rid of the Kconfig entry and build it by default for > > both RC and EP drivers? I don't see a value in building it as a separate module. > > And we may also move more common code in the future. > > It is already built by default for both drivers. I'm not sure what > you're suggesting here. > Right now it is selected by both drivers using a Kconfig symbol. But I'm thinking of building it by default as below: -obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o -obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o +obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o pcie-qcom-common.o +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o pcie-qcom-common.o A separate Kconfig symbol is not really needed here as this file contains common code required by both the drivers. - Mani
On Thu, Sep 05, 2024 at 11:04:37PM +0530, Manivannan Sadhasivam wrote: > On Thu, Sep 05, 2024 at 06:27:36PM +0200, Johan Hovold wrote: > > On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote: > > > Perhaps we can just get rid of the Kconfig entry and build it by default for > > > both RC and EP drivers? I don't see a value in building it as a separate module. > > > And we may also move more common code in the future. > > > > It is already built by default for both drivers. I'm not sure what > > you're suggesting here. > > Right now it is selected by both drivers using a Kconfig symbol. But I'm > thinking of building it by default as below: > > -obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o > -obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o > +obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o pcie-qcom-common.o > +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o pcie-qcom-common.o > > A separate Kconfig symbol is not really needed here as this file contains common > code required by both the drivers. But the separate Kconfig symbol will only be enabled via either PCI driver's option (e.g. can't be enabled on its own). I'm also not sure if the above works if you build one driver as a module and the other into the kernel (yes, I still intend to resubmit my patch for making the rc driver modular). Johan
On Fri, Sep 06, 2024 at 08:49:03AM +0200, Johan Hovold wrote: > On Thu, Sep 05, 2024 at 11:04:37PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Sep 05, 2024 at 06:27:36PM +0200, Johan Hovold wrote: > > > On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote: > > > > > Perhaps we can just get rid of the Kconfig entry and build it by default for > > > > both RC and EP drivers? I don't see a value in building it as a separate module. > > > > And we may also move more common code in the future. > > > > > > It is already built by default for both drivers. I'm not sure what > > > you're suggesting here. > > > > Right now it is selected by both drivers using a Kconfig symbol. But I'm > > thinking of building it by default as below: > > > > -obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o > > -obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o > > +obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o pcie-qcom-common.o > > +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o pcie-qcom-common.o > > > > A separate Kconfig symbol is not really needed here as this file contains common > > code required by both the drivers. > > But the separate Kconfig symbol will only be enabled via either PCI > driver's option (e.g. can't be enabled on its own). > True. But since the common file is required by both drivers, I thought of just building it by default. But looking at your below reply, it won't be possible. > I'm also not sure if the above works if you build one driver as a module > and the other into the kernel (yes, I still intend to resubmit my patch > for making the rc driver modular). > Hmm, I thought you dropped that patch ;) Anyway, if that happens, it will be a problem. I'll keep it as it is. - Mani
diff --git a/MAINTAINERS b/MAINTAINERS index f328373463b0..3cfb6068b9f0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2728,7 +2728,7 @@ F: drivers/iommu/msm* F: drivers/mfd/ssbi.c F: drivers/mmc/host/mmci_qcom* F: drivers/mmc/host/sdhci-msm.c -F: drivers/pci/controller/dwc/pcie-qcom.c +F: drivers/pci/controller/dwc/pcie-qcom* F: drivers/phy/qualcomm/ F: drivers/power/*/msm* F: drivers/reset/reset-qcom-* @@ -17757,6 +17757,7 @@ M: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> L: linux-pci@vger.kernel.org L: linux-arm-msm@vger.kernel.org S: Maintained +F: drivers/pci/controller/dwc/pcie-qcom-common.c F: drivers/pci/controller/dwc/pcie-qcom.c PCIE DRIVER FOR ROCKCHIP @@ -17793,6 +17794,7 @@ L: linux-pci@vger.kernel.org L: linux-arm-msm@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml +F: drivers/pci/controller/dwc/pcie-qcom-common.c F: drivers/pci/controller/dwc/pcie-qcom-ep.c PCMCIA SUBSYSTEM diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 4c38181acffa..b6d6778b0698 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -265,12 +265,16 @@ config PCIE_DW_PLAT_EP order to enable device-specific features PCI_DW_PLAT_EP must be selected. +config PCIE_QCOM_COMMON + bool + config PCIE_QCOM bool "Qualcomm PCIe controller (host mode)" depends on OF && (ARCH_QCOM || COMPILE_TEST) depends on PCI_MSI select PCIE_DW_HOST select CRC8 + select PCIE_QCOM_COMMON help Say Y here to enable PCIe controller support on Qualcomm SoCs. The PCIe controller uses the DesignWare core plus Qualcomm-specific @@ -281,6 +285,7 @@ config PCIE_QCOM_EP depends on OF && (ARCH_QCOM || COMPILE_TEST) depends on PCI_ENDPOINT select PCIE_DW_EP + select PCIE_QCOM_COMMON help Say Y here to enable support for the PCIe controllers on Qualcomm SoCs to work in endpoint mode. The PCIe controller uses the DesignWare core diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index ec215b3d6191..a8308d9ea986 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o +obj-$(CONFIG_PCIE_QCOM_COMMON) += pcie-qcom-common.o obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 22765564f301..51744ad25575 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -126,6 +126,18 @@ #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 GENMASK(3, 0) +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE BIT(4) +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC 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 GENMASK(4, 0) +#define GEN3_EQ_FMDC_N_EVALS GENMASK(9, 5) +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA GENMASK(13, 10) +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA GENMASK(17, 14) + #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 #define PORT_MLTI_UPCFG_SUPPORT BIT(7) diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c new file mode 100644 index 000000000000..dc7d93db9dc5 --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include <linux/pci.h> + +#include "pcie-designware.h" +#include "pcie-qcom-common.h" + +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) +{ + u32 reg; + + /* + * GEN3_RELATED_OFF register is repurposed to apply equalization + * settings at various data transmission rates through registers namely + * GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF determines + * data rate for which this 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, 0x1); + 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_eq_settings); diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h new file mode 100644 index 000000000000..259e04b7bdf9 --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include "pcie-designware.h" + +void qcom_pcie_common_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 236229f66c80..af83470216e8 100644 --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c @@ -25,6 +25,7 @@ #include "../../pci.h" #include "pcie-designware.h" +#include "pcie-qcom-common.h" /* PARF registers */ #define PARF_SYS_CTRL 0x00 @@ -486,6 +487,9 @@ 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_eq_settings(pci); + /* * 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 0180edf3310e..2742e82fdcb3 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -35,6 +35,7 @@ #include "../../pci.h" #include "pcie-designware.h" +#include "pcie-qcom-common.h" /* PARF registers */ #define PARF_SYS_CTRL 0x00 @@ -283,6 +284,9 @@ 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_eq_settings(pci); + /* Enable Link Training state machine */ if (pcie->cfg->ops->ltssm_enable) pcie->cfg->ops->ltssm_enable(pcie);