| Message ID | 20200402121148.1767-8-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multiple fixes in PCIe qcom driver | expand |
Hi Ansuel, Please fix the patch subject for all patches in the series per Bjorn H. request. PCI: qcom: Fix init problem with missing PARF programming Also the patch subject is misleading to me. Actually you change few phy parameters: Tx De-Emphasis, Tx Swing and Rx equalization. On the other side I guess those parameters are board specific and I'm not sure how this change will reflect on apq8064 boards. On 4/2/20 3:11 PM, Ansuel Smith wrote: > PARF programming was missing and this cause initilizzation problem on > some ipq806x based device (Netgear R7800 for example). This cause a > total lock of the system on kernel load. > > Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++++++++----- > 1 file changed, 39 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 211a1aa7d0f1..77b1ab7e23a3 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -46,6 +46,9 @@ > > #define PCIE20_PARF_PHY_CTRL 0x40 > #define PCIE20_PARF_PHY_REFCLK 0x4C > +#define REF_SSP_EN BIT(16) > +#define REF_USE_PAD BIT(12) Could you rename this to: PHY_REFCLK_SSP_EN PHY_REFCLK_USE_PAD > + > #define PCIE20_PARF_DBI_BASE_ADDR 0x168 > #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C > #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL 0x174 > @@ -77,6 +80,18 @@ > #define DBI_RO_WR_EN 1 > > #define PERST_DELAY_US 1000 > +/* PARF registers */ > +#define PCIE20_PARF_PCS_DEEMPH 0x34 > +#define PCS_DEEMPH_TX_DEEMPH_GEN1(x) (x << 16) > +#define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) (x << 8) > +#define PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(x) (x << 0) > + > +#define PCIE20_PARF_PCS_SWING 0x38 > +#define PCS_SWING_TX_SWING_FULL(x) (x << 8) > +#define PCS_SWING_TX_SWING_LOW(x) (x << 0) > + > +#define PCIE20_PARF_CONFIG_BITS 0x50 > +#define PHY_RX0_EQ(x) (x << 24) > > #define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358 > #define SLV_ADDR_SPACE_SZ 0x10000000 > @@ -184,6 +199,16 @@ struct qcom_pcie { > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) > > +static inline void qcom_clear_and_set_dword(void __iomem *addr, drop 'inline' the compiler is smart enough to decide. > + u32 clear_mask, u32 set_mask) > +{ > + u32 val = readl(addr); > + > + val &= ~clear_mask; > + val |= set_mask; > + writel(val, addr); > +} > + If you add such function you should introduce it in a separate patch and use it in the whole driver where it is applicable. After that we can see what is the benefit of it. > static void qcom_ep_reset_assert(struct qcom_pcie *pcie) > { > gpiod_set_value_cansleep(pcie->reset, 1); > @@ -304,7 +329,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie) > struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0; > struct dw_pcie *pci = pcie->pci; > struct device *dev = pci->dev; > - u32 val; > int ret; > > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies); > @@ -355,15 +379,21 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie) > goto err_deassert_ahb; > } > > - /* enable PCIe clocks and resets */ > - val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL); > - val &= ~BIT(0); > - writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL); > + qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0); please keep the comment. > + > + /* PARF programming */ pointless comment, please drop it. > + writel(PCS_DEEMPH_TX_DEEMPH_GEN1(0x18) | > + PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(0x18) | > + PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(0x22), > + pcie->parf + PCIE20_PARF_PCS_DEEMPH); > + writel(PCS_SWING_TX_SWING_FULL(0x78) | > + PCS_SWING_TX_SWING_LOW(0x78), > + pcie->parf + PCIE20_PARF_PCS_SWING); > + writel(PHY_RX0_EQ(0x4), pcie->parf + PCIE20_PARF_CONFIG_BITS); > > - /* enable external reference clock */ > - val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK); > - val |= BIT(16); > - writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK); > + /* enable reference clock */ Why you dropped 'external' ? > + qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_REFCLK, > + REF_USE_PAD, REF_SSP_EN); > > ret = reset_control_deassert(res->phy_reset); > if (ret) { >
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 211a1aa7d0f1..77b1ab7e23a3 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -46,6 +46,9 @@ #define PCIE20_PARF_PHY_CTRL 0x40 #define PCIE20_PARF_PHY_REFCLK 0x4C +#define REF_SSP_EN BIT(16) +#define REF_USE_PAD BIT(12) + #define PCIE20_PARF_DBI_BASE_ADDR 0x168 #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL 0x174 @@ -77,6 +80,18 @@ #define DBI_RO_WR_EN 1 #define PERST_DELAY_US 1000 +/* PARF registers */ +#define PCIE20_PARF_PCS_DEEMPH 0x34 +#define PCS_DEEMPH_TX_DEEMPH_GEN1(x) (x << 16) +#define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) (x << 8) +#define PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(x) (x << 0) + +#define PCIE20_PARF_PCS_SWING 0x38 +#define PCS_SWING_TX_SWING_FULL(x) (x << 8) +#define PCS_SWING_TX_SWING_LOW(x) (x << 0) + +#define PCIE20_PARF_CONFIG_BITS 0x50 +#define PHY_RX0_EQ(x) (x << 24) #define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358 #define SLV_ADDR_SPACE_SZ 0x10000000 @@ -184,6 +199,16 @@ struct qcom_pcie { #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) +static inline void qcom_clear_and_set_dword(void __iomem *addr, + u32 clear_mask, u32 set_mask) +{ + u32 val = readl(addr); + + val &= ~clear_mask; + val |= set_mask; + writel(val, addr); +} + static void qcom_ep_reset_assert(struct qcom_pcie *pcie) { gpiod_set_value_cansleep(pcie->reset, 1); @@ -304,7 +329,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie) struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0; struct dw_pcie *pci = pcie->pci; struct device *dev = pci->dev; - u32 val; int ret; ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies); @@ -355,15 +379,21 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie) goto err_deassert_ahb; } - /* enable PCIe clocks and resets */ - val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL); - val &= ~BIT(0); - writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL); + qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0); + + /* PARF programming */ + writel(PCS_DEEMPH_TX_DEEMPH_GEN1(0x18) | + PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(0x18) | + PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(0x22), + pcie->parf + PCIE20_PARF_PCS_DEEMPH); + writel(PCS_SWING_TX_SWING_FULL(0x78) | + PCS_SWING_TX_SWING_LOW(0x78), + pcie->parf + PCIE20_PARF_PCS_SWING); + writel(PHY_RX0_EQ(0x4), pcie->parf + PCIE20_PARF_CONFIG_BITS); - /* enable external reference clock */ - val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK); - val |= BIT(16); - writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK); + /* enable reference clock */ + qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_REFCLK, + REF_USE_PAD, REF_SSP_EN); ret = reset_control_deassert(res->phy_reset); if (ret) {
PARF programming was missing and this cause initilizzation problem on some ipq806x based device (Netgear R7800 for example). This cause a total lock of the system on kernel load. Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++++++++----- 1 file changed, 39 insertions(+), 9 deletions(-)