| Message ID | 20200402121148.1767-8-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Lorenzo Pieralisi |
| 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) { >
> PARF programming > > 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. > I also think that this would brake apq8064, on ipq8064 this is needed or the system doesn't boot. Should I move this to the dts and set this params only if they are present in dts or also here check for compatible and set accordingly? > 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) { > > > > -- > regards, > Stan
Hi Ansuel, On 4/8/20 3:38 PM, ansuelsmth@gmail.com wrote: >> PARF programming >> >> 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. >> > > I also think that this would brake apq8064, on ipq8064 this is needed or > the system doesn't boot. > Should I move this to the dts and set this params only if they are present > in dts or also here check for compatible and set accordingly? > I also think that these phy params should be per board (and they are tunable). Maybe you can propose those as generic phy params in pci.txt binding document and then we can ask DT maintainers for opinion. If they refuse such generic bindings, we could switch to custom qcom,phy properties.
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(-)