| Message ID | 1500293043-1887-8-git-send-email-varada@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon 17 Jul 05:04 PDT 2017, Varadarajan Narayanan wrote: > Add support for the IPQ8074 PCIe controller. IPQ8074 supports > Gen 1/2, one lane, two PCIe root complex with support for MSI and > legacy interrupts, and it conforms to PCI Express Base 2.1 > specification. > > The core init is the similar to the existing SoC, however the > clocks and reset lines differ. > > Signed-off-by: smuthayy <smuthayy@codeaurora.org> > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> > --- > drivers/pci/dwc/pcie-qcom.c | 259 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 259 insertions(+) > > diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c > index d15657d..c1fa356 100644 > --- a/drivers/pci/dwc/pcie-qcom.c > +++ b/drivers/pci/dwc/pcie-qcom.c > @@ -37,6 +37,20 @@ > #include "pcie-designware.h" > > #define PCIE20_PARF_SYS_CTRL 0x00 > +#define MST_WAKEUP_EN BIT(13) > +#define SLV_WAKEUP_EN BIT(12) > +#define MSTR_ACLK_CGC_DIS BIT(10) > +#define SLV_ACLK_CGC_DIS BIT(9) > +#define CORE_CLK_CGC_DIS BIT(6) > +#define AUX_PWR_DET BIT(4) > +#define L23_CLK_RMV_DIS BIT(2) > +#define L1_CLK_RMV_DIS BIT(1) > + > +#define PCIE20_COMMAND_STATUS 0x04 > +#define CMD_BME_VAL 0x4 > +#define PCIE20_DEVICE_CONTROL2_STATUS2 0x98 > +#define PCIE_CAP_CPL_TIMEOUT_DISABLE 0x10 > + > #define PCIE20_PARF_PHY_CTRL 0x40 > #define PCIE20_PARF_PHY_REFCLK 0x4C > #define PCIE20_PARF_DBI_BASE_ADDR 0x168 > @@ -58,9 +72,22 @@ > #define CFG_BRIDGE_SB_INIT BIT(0) > > #define PCIE20_CAP 0x70 > +#define PCIE20_CAP_LINK_CAPABILITIES (PCIE20_CAP + 0xC) > +#define PCIE20_CAP_LINK_1 (PCIE20_CAP + 0x14) > +#define PCIE_CAP_LINK1_VAL 0x2fd7f > + > +#define PCIE20_PARF_Q2A_FLUSH 0x1AC > + > +#define PCIE20_MISC_CONTROL_1_REG 0x8BC > +#define DBI_RO_WR_EN 1 > > #define PERST_DELAY_US 1000 > > +#define AXI_CLK_RATE 200000000 > + > +#define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358 > +#define SLV_ADDR_SPACE_SZ 0x10000000 > + > struct qcom_pcie_resources_v0 { > struct clk *iface_clk; > struct clk *core_clk; > @@ -110,11 +137,26 @@ struct qcom_pcie_resources_v3 { > struct reset_control *phy_ahb_reset; > }; > > +struct qphy_reset { > + struct reset_control *rst; > + char *name; > +}; > + > +struct qcom_pcie_resources_v4 { > + struct clk *sys_noc_clk; > + struct clk *axi_m_clk; > + struct clk *axi_s_clk; > + struct clk *ahb_clk; > + struct clk *aux_clk; > + struct qphy_reset rst[7]; Just store the struct reset_control here directly, carrying the name doesn't serve much of a purpose - and it clutters the code. > +}; Can you confirm that this is actually version 4 of this block? Or are we just incrementing an arbitrary number here? > + > union qcom_pcie_resources { > struct qcom_pcie_resources_v0 v0; > struct qcom_pcie_resources_v1 v1; > struct qcom_pcie_resources_v2 v2; > struct qcom_pcie_resources_v3 v3; > + struct qcom_pcie_resources_v4 v4; > }; > > struct qcom_pcie; > @@ -139,6 +181,16 @@ struct qcom_pcie { > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) > > +static inline void > +writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask) This function name is very generic and in the two places you call it set_mask is 0. So please just inline this. > +{ > + 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(pcie->reset, 1); > @@ -884,6 +936,205 @@ static int qcom_pcie_init_v3(struct qcom_pcie *pcie) > return ret; > } > > +static int qcom_pcie_get_resources_v4(struct qcom_pcie *pcie) > +{ > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + int i; > + > + res->sys_noc_clk = devm_clk_get(dev, "sys_noc"); > + if (IS_ERR(res->sys_noc_clk)) > + return PTR_ERR(res->sys_noc_clk); > + > + res->axi_m_clk = devm_clk_get(dev, "axi_m"); > + if (IS_ERR(res->axi_m_clk)) > + return PTR_ERR(res->axi_m_clk); > + > + res->axi_s_clk = devm_clk_get(dev, "axi_s"); > + if (IS_ERR(res->axi_s_clk)) > + return PTR_ERR(res->axi_s_clk); > + > + res->ahb_clk = devm_clk_get(dev, "ahb"); > + if (IS_ERR(res->ahb_clk)) > + return PTR_ERR(res->ahb_clk); > + > + res->aux_clk = devm_clk_get(dev, "aux"); > + if (IS_ERR(res->aux_clk)) > + return PTR_ERR(res->aux_clk); > + > + res->rst[0].name = "axi_m"; > + res->rst[1].name = "axi_s"; > + res->rst[2].name = "pipe"; > + res->rst[3].name = "axi_m_sticky"; > + res->rst[4].name = "sticky"; > + res->rst[5].name = "ahb"; > + res->rst[6].name = "sleep"; > + > + for (i = 0; i < ARRAY_SIZE(res->rst); i++) { > + res->rst[i].rst = devm_reset_control_get(dev, res->rst[i].name); > + if (IS_ERR(res->rst[i].rst)) > + return PTR_ERR(res->rst[i].rst); > + } > + > + return 0; > +} > + > +static void qcom_pcie_deinit_v4(struct qcom_pcie *pcie) > +{ > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > + > + clk_disable_unprepare(res->sys_noc_clk); > + clk_disable_unprepare(res->axi_m_clk); > + clk_disable_unprepare(res->axi_s_clk); > + clk_disable_unprepare(res->ahb_clk); > + clk_disable_unprepare(res->aux_clk); > +} > + > +static int qcom_pcie_enable_resources_v4(struct qcom_pcie *pcie) > +{ > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + int ret; > + > + ret = clk_prepare_enable(res->sys_noc_clk); > + if (ret) { > + dev_err(dev, "cannot prepare/enable core clock\n"); > + return ret; > + } Should these clocks really be handled explicitly in the driver? Are these not the bus clocks, to be handled by "msm_bus"? > + > + ret = clk_prepare_enable(res->axi_m_clk); > + if (ret) { > + dev_err(dev, "cannot prepare/enable core clock\n"); > + goto err_clk_axi_m; > + } > + > + ret = clk_set_rate(res->axi_m_clk, AXI_CLK_RATE); > + if (ret) { > + dev_err(dev, "MClk rate set failed (%d)\n", ret); > + goto err_clk_axi_m; > + } > + > + ret = clk_prepare_enable(res->axi_s_clk); > + if (ret) { > + dev_err(dev, "cannot prepare/enable axi slave clock\n"); > + goto err_clk_axi_s; > + } > + > + ret = clk_set_rate(res->axi_s_clk, AXI_CLK_RATE); > + if (ret) { > + dev_err(dev, "MClk rate set failed (%d)\n", ret); > + goto err_clk_axi_s; > + } > + > + ret = clk_prepare_enable(res->ahb_clk); > + if (ret) { > + dev_err(dev, "cannot prepare/enable ahb clock\n"); > + goto err_clk_ahb; > + } > + > + ret = clk_prepare_enable(res->aux_clk); > + if (ret) { > + dev_err(dev, "cannot prepare/enable aux clock\n"); > + goto err_clk_aux; > + } > + > + udelay(1); > + > + return 0; > + > +err_clk_aux: > + clk_disable_unprepare(res->ahb_clk); > +err_clk_ahb: > + clk_disable_unprepare(res->axi_s_clk); > +err_clk_axi_s: > + clk_disable_unprepare(res->axi_m_clk); > +err_clk_axi_m: > + clk_disable_unprepare(res->sys_noc_clk); > + > + return ret; > +} > + > +static inline int qphy_reset_control(struct qcom_pcie *pcie, > + struct qphy_reset *r, > + bool assert) > +{ > + int ret; > + > + if (assert) > + ret = reset_control_assert(r->rst); > + else > + ret = reset_control_deassert(r->rst); > + > + if (ret) > + dev_err(pcie->pci->dev, "%s: reset %s failed for %s\n", > + __func__, assert ? "assert" : "deassert", r->name); > + > + return ret; > +} > + > +static void qcom_pcie_v4_reset(struct qcom_pcie *pcie) > +{ > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > + struct qphy_reset *qphy_rst = &res->rst[0]; &res->rst[0] is supposed to be written as res->rst, but that's exactly the same number of characters to type as your local variable. So just skip the variable. > + int i; > + > + for (i = 0; i < ARRAY_SIZE(res->rst); i++) > + if (qphy_reset_control(pcie, &qphy_rst[i], true)) This is a complicated way of saying: if (reset_control_assert(qphy_rst[i].rst)) > + return; You should most definitely propagate this error. > + > + usleep_range(10000, 12000); /* wait 12ms */ This is not 12ms, this is 10-12ms. This is a _long_ time to usleep, how about just msleep(20) instead? > + > + for (i = 0; i < ARRAY_SIZE(res->rst); i++) > + if (qphy_reset_control(pcie, &qphy_rst[i], false)) Same as above, this just write: if (reset_control_deassert(qphy_rst[i].rst)) > + return; > + > + usleep_range(10000, 12000); /* wait 12ms */ > + wmb(); /* ensure data is written to hw register */ wmb() ensures ordering of writes, it does not wait for data to reach the hardware registers. > +} > + > +static int qcom_pcie_init_v4(struct qcom_pcie *pcie) > +{ > + struct dw_pcie *pci = pcie->pci; > + int ret; > + > + qcom_pcie_v4_reset(pcie); > + qcom_ep_reset_assert(pcie); > + > + ret = qcom_pcie_enable_resources_v4(pcie); > + if (ret) > + return ret; > + > + writel(SLV_ADDR_SPACE_SZ, pcie->parf + > + PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE); > + > + ret = phy_power_on(pcie->phy); > + if (ret) This will leave all the resources enabled, you should issue a deinit here.. > + return ret; > + > + writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0); > + Regards, Bjorn
On Mon, Jul 17, 2017 at 03:07:18PM -0700, Bjorn Andersson wrote: > On Mon 17 Jul 05:04 PDT 2017, Varadarajan Narayanan wrote: > > > Add support for the IPQ8074 PCIe controller. IPQ8074 supports > > Gen 1/2, one lane, two PCIe root complex with support for MSI and > > legacy interrupts, and it conforms to PCI Express Base 2.1 > > specification. > > > > The core init is the similar to the existing SoC, however the > > clocks and reset lines differ. > > > > Signed-off-by: smuthayy <smuthayy@codeaurora.org> > > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> > > --- > > drivers/pci/dwc/pcie-qcom.c | 259 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 259 insertions(+) > > > > diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c > > index d15657d..c1fa356 100644 > > --- a/drivers/pci/dwc/pcie-qcom.c > > +++ b/drivers/pci/dwc/pcie-qcom.c > > @@ -37,6 +37,20 @@ > > #include "pcie-designware.h" > > > > #define PCIE20_PARF_SYS_CTRL 0x00 > > +#define MST_WAKEUP_EN BIT(13) > > +#define SLV_WAKEUP_EN BIT(12) > > +#define MSTR_ACLK_CGC_DIS BIT(10) > > +#define SLV_ACLK_CGC_DIS BIT(9) > > +#define CORE_CLK_CGC_DIS BIT(6) > > +#define AUX_PWR_DET BIT(4) > > +#define L23_CLK_RMV_DIS BIT(2) > > +#define L1_CLK_RMV_DIS BIT(1) > > + > > +#define PCIE20_COMMAND_STATUS 0x04 > > +#define CMD_BME_VAL 0x4 > > +#define PCIE20_DEVICE_CONTROL2_STATUS2 0x98 > > +#define PCIE_CAP_CPL_TIMEOUT_DISABLE 0x10 > > + > > #define PCIE20_PARF_PHY_CTRL 0x40 > > #define PCIE20_PARF_PHY_REFCLK 0x4C > > #define PCIE20_PARF_DBI_BASE_ADDR 0x168 > > @@ -58,9 +72,22 @@ > > #define CFG_BRIDGE_SB_INIT BIT(0) > > > > #define PCIE20_CAP 0x70 > > +#define PCIE20_CAP_LINK_CAPABILITIES (PCIE20_CAP + 0xC) > > +#define PCIE20_CAP_LINK_1 (PCIE20_CAP + 0x14) > > +#define PCIE_CAP_LINK1_VAL 0x2fd7f > > + > > +#define PCIE20_PARF_Q2A_FLUSH 0x1AC > > + > > +#define PCIE20_MISC_CONTROL_1_REG 0x8BC > > +#define DBI_RO_WR_EN 1 > > > > #define PERST_DELAY_US 1000 > > > > +#define AXI_CLK_RATE 200000000 > > + > > +#define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358 > > +#define SLV_ADDR_SPACE_SZ 0x10000000 > > + > > struct qcom_pcie_resources_v0 { > > struct clk *iface_clk; > > struct clk *core_clk; > > @@ -110,11 +137,26 @@ struct qcom_pcie_resources_v3 { > > struct reset_control *phy_ahb_reset; > > }; > > > > +struct qphy_reset { > > + struct reset_control *rst; > > + char *name; > > +}; > > + > > +struct qcom_pcie_resources_v4 { > > + struct clk *sys_noc_clk; > > + struct clk *axi_m_clk; > > + struct clk *axi_s_clk; > > + struct clk *ahb_clk; > > + struct clk *aux_clk; > > + struct qphy_reset rst[7]; > > Just store the struct reset_control here directly, carrying the name > doesn't serve much of a purpose - and it clutters the code. Ok. > > +}; > > Can you confirm that this is actually version 4 of this block? Or are we > just incrementing an arbitrary number here? This is not exactly the 4th version of the block. However, it is a different version than the ones that are already supported in this driver. Since the existing driver didn't exactly tie it with the block IP version, I too followed the same versioning convention. > > + > > union qcom_pcie_resources { > > struct qcom_pcie_resources_v0 v0; > > struct qcom_pcie_resources_v1 v1; > > struct qcom_pcie_resources_v2 v2; > > struct qcom_pcie_resources_v3 v3; > > + struct qcom_pcie_resources_v4 v4; > > }; > > > > struct qcom_pcie; > > @@ -139,6 +181,16 @@ struct qcom_pcie { > > > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) > > > > +static inline void > > +writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask) > > This function name is very generic and in the two places you call it > set_mask is 0. So please just inline this. Ok. > > +{ > > + 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(pcie->reset, 1); > > @@ -884,6 +936,205 @@ static int qcom_pcie_init_v3(struct qcom_pcie *pcie) > > return ret; > > } > > > > +static int qcom_pcie_get_resources_v4(struct qcom_pcie *pcie) > > +{ > > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > > + struct dw_pcie *pci = pcie->pci; > > + struct device *dev = pci->dev; > > + int i; > > + > > + res->sys_noc_clk = devm_clk_get(dev, "sys_noc"); > > + if (IS_ERR(res->sys_noc_clk)) > > + return PTR_ERR(res->sys_noc_clk); > > + > > + res->axi_m_clk = devm_clk_get(dev, "axi_m"); > > + if (IS_ERR(res->axi_m_clk)) > > + return PTR_ERR(res->axi_m_clk); > > + > > + res->axi_s_clk = devm_clk_get(dev, "axi_s"); > > + if (IS_ERR(res->axi_s_clk)) > > + return PTR_ERR(res->axi_s_clk); > > + > > + res->ahb_clk = devm_clk_get(dev, "ahb"); > > + if (IS_ERR(res->ahb_clk)) > > + return PTR_ERR(res->ahb_clk); > > + > > + res->aux_clk = devm_clk_get(dev, "aux"); > > + if (IS_ERR(res->aux_clk)) > > + return PTR_ERR(res->aux_clk); > > + > > + res->rst[0].name = "axi_m"; > > + res->rst[1].name = "axi_s"; > > + res->rst[2].name = "pipe"; > > + res->rst[3].name = "axi_m_sticky"; > > + res->rst[4].name = "sticky"; > > + res->rst[5].name = "ahb"; > > + res->rst[6].name = "sleep"; > > + > > + for (i = 0; i < ARRAY_SIZE(res->rst); i++) { > > + res->rst[i].rst = devm_reset_control_get(dev, res->rst[i].name); > > + if (IS_ERR(res->rst[i].rst)) > > + return PTR_ERR(res->rst[i].rst); > > + } > > + > > + return 0; > > +} > > + > > +static void qcom_pcie_deinit_v4(struct qcom_pcie *pcie) > > +{ > > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > > + > > + clk_disable_unprepare(res->sys_noc_clk); > > + clk_disable_unprepare(res->axi_m_clk); > > + clk_disable_unprepare(res->axi_s_clk); > > + clk_disable_unprepare(res->ahb_clk); > > + clk_disable_unprepare(res->aux_clk); > > +} > > + > > +static int qcom_pcie_enable_resources_v4(struct qcom_pcie *pcie) > > +{ > > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > > + struct dw_pcie *pci = pcie->pci; > > + struct device *dev = pci->dev; > > + int ret; > > + > > + ret = clk_prepare_enable(res->sys_noc_clk); > > + if (ret) { > > + dev_err(dev, "cannot prepare/enable core clock\n"); > > + return ret; > > + } > > Should these clocks really be handled explicitly in the driver? Are > these not the bus clocks, to be handled by "msm_bus"? This not the bus clock. This clock is for the Bus Interface Unit between the PCIe module and the System NOC. > > + > > + ret = clk_prepare_enable(res->axi_m_clk); > > + if (ret) { > > + dev_err(dev, "cannot prepare/enable core clock\n"); > > + goto err_clk_axi_m; > > + } > > + > > + ret = clk_set_rate(res->axi_m_clk, AXI_CLK_RATE); > > + if (ret) { > > + dev_err(dev, "MClk rate set failed (%d)\n", ret); > > + goto err_clk_axi_m; > > + } > > + > > + ret = clk_prepare_enable(res->axi_s_clk); > > + if (ret) { > > + dev_err(dev, "cannot prepare/enable axi slave clock\n"); > > + goto err_clk_axi_s; > > + } > > + > > + ret = clk_set_rate(res->axi_s_clk, AXI_CLK_RATE); > > + if (ret) { > > + dev_err(dev, "MClk rate set failed (%d)\n", ret); > > + goto err_clk_axi_s; > > + } > > + > > + ret = clk_prepare_enable(res->ahb_clk); > > + if (ret) { > > + dev_err(dev, "cannot prepare/enable ahb clock\n"); > > + goto err_clk_ahb; > > + } > > + > > + ret = clk_prepare_enable(res->aux_clk); > > + if (ret) { > > + dev_err(dev, "cannot prepare/enable aux clock\n"); > > + goto err_clk_aux; > > + } > > + > > + udelay(1); > > + > > + return 0; > > + > > +err_clk_aux: > > + clk_disable_unprepare(res->ahb_clk); > > +err_clk_ahb: > > + clk_disable_unprepare(res->axi_s_clk); > > +err_clk_axi_s: > > + clk_disable_unprepare(res->axi_m_clk); > > +err_clk_axi_m: > > + clk_disable_unprepare(res->sys_noc_clk); > > + > > + return ret; > > +} > > + > > +static inline int qphy_reset_control(struct qcom_pcie *pcie, > > + struct qphy_reset *r, > > + bool assert) > > +{ > > + int ret; > > + > > + if (assert) > > + ret = reset_control_assert(r->rst); > > + else > > + ret = reset_control_deassert(r->rst); > > + > > + if (ret) > > + dev_err(pcie->pci->dev, "%s: reset %s failed for %s\n", > > + __func__, assert ? "assert" : "deassert", r->name); > > + > > + return ret; > > +} > > + > > +static void qcom_pcie_v4_reset(struct qcom_pcie *pcie) > > +{ > > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > > + struct qphy_reset *qphy_rst = &res->rst[0]; > > &res->rst[0] is supposed to be written as res->rst, but that's exactly > the same number of characters to type as your local variable. So just > skip the variable. Ok. > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(res->rst); i++) > > + if (qphy_reset_control(pcie, &qphy_rst[i], true)) > > This is a complicated way of saying: > if (reset_control_assert(qphy_rst[i].rst)) Ok, will remove the helper function. > > + return; > > You should most definitely propagate this error. Ok. > > + > > + usleep_range(10000, 12000); /* wait 12ms */ > > This is not 12ms, this is 10-12ms. This is a _long_ time to usleep, how > about just msleep(20) instead? Ok. > > + > > + for (i = 0; i < ARRAY_SIZE(res->rst); i++) > > + if (qphy_reset_control(pcie, &qphy_rst[i], false)) > > Same as above, this just write: > if (reset_control_deassert(qphy_rst[i].rst)) Ok. > > + return; > > + > > + usleep_range(10000, 12000); /* wait 12ms */ > > + wmb(); /* ensure data is written to hw register */ > > wmb() ensures ordering of writes, it does not wait for data to reach the > hardware registers. Ok. Will remove it. > > +} > > + > > +static int qcom_pcie_init_v4(struct qcom_pcie *pcie) > > +{ > > + struct dw_pcie *pci = pcie->pci; > > + int ret; > > + > > + qcom_pcie_v4_reset(pcie); > > + qcom_ep_reset_assert(pcie); > > + > > + ret = qcom_pcie_enable_resources_v4(pcie); > > + if (ret) > > + return ret; > > + > > + writel(SLV_ADDR_SPACE_SZ, pcie->parf + > > + PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE); > > + > > + ret = phy_power_on(pcie->phy); > > + if (ret) > > This will leave all the resources enabled, you should issue a deinit > here.. Ok. Thanks Varada > > + return ret; > > + > > + writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0); > > + > > Regards, > Bjorn -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Tue 18 Jul 02:58 PDT 2017, Varadarajan Narayanan wrote: > On Mon, Jul 17, 2017 at 03:07:18PM -0700, Bjorn Andersson wrote: > > On Mon 17 Jul 05:04 PDT 2017, Varadarajan Narayanan wrote: [..] > > > > Can you confirm that this is actually version 4 of this block? Or are we > > just incrementing an arbitrary number here? > > This is not exactly the 4th version of the block. However, it is > a different version than the ones that are already supported in > this driver. Since the existing driver didn't exactly tie it with > the block IP version, I too followed the same versioning > convention. > Do you have a block IP version that you could base your numbering on, to break the trend? (We should go back and fix up the others as well) [..] > > > +static int qcom_pcie_enable_resources_v4(struct qcom_pcie *pcie) > > > +{ > > > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > > > + struct dw_pcie *pci = pcie->pci; > > > + struct device *dev = pci->dev; > > > + int ret; > > > + > > > + ret = clk_prepare_enable(res->sys_noc_clk); > > > + if (ret) { > > > + dev_err(dev, "cannot prepare/enable core clock\n"); > > > + return ret; > > > + } > > > > Should these clocks really be handled explicitly in the driver? Are > > these not the bus clocks, to be handled by "msm_bus"? > > This not the bus clock. This clock is for the Bus Interface Unit > between the PCIe module and the System NOC. > Right, that was the piece I meant. Sorry for not using the right nomenclature. So then it would be handled by the msm_bus in the downstream kernel? Perhaps we can merge it like this and once we have the interconnect framework setup we can make this the fallback method. Thanks, Bjorn
On Tue, Jul 18, 2017 at 09:44:38AM -0700, Bjorn Andersson wrote: > On Tue 18 Jul 02:58 PDT 2017, Varadarajan Narayanan wrote: > > > On Mon, Jul 17, 2017 at 03:07:18PM -0700, Bjorn Andersson wrote: > > > On Mon 17 Jul 05:04 PDT 2017, Varadarajan Narayanan wrote: > [..] > > > > > > Can you confirm that this is actually version 4 of this block? Or are we > > > just incrementing an arbitrary number here? > > > > This is not exactly the 4th version of the block. However, it is > > a different version than the ones that are already supported in > > this driver. Since the existing driver didn't exactly tie it with > > the block IP version, I too followed the same versioning > > convention. > > > > Do you have a block IP version that you could base your numbering on, to > break the trend? (We should go back and fix up the others as well) Presently, the driver supports the ipq8064, apq8064, apq8084, msm8996, ipq4019 and ipq8074. The SoCs, qcom_pcie_ops version and the block IP versions are as follows. ipq8064 - v0 - 2.1.0 apq8064 - v0 - 2.1.0 apq8084 - v1 - 1.0.0 msm8996 - v2 - 2.3.2 ipq4019 - v3 - 2.4.0 ipq8074 - v4 - 2.3.3 I will rename the qcom_pcie_ops structure and related functions with the block IP version instead of vX numbering and post the patch. > [..] > > > > +static int qcom_pcie_enable_resources_v4(struct qcom_pcie *pcie) > > > > +{ > > > > + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; > > > > + struct dw_pcie *pci = pcie->pci; > > > > + struct device *dev = pci->dev; > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(res->sys_noc_clk); > > > > + if (ret) { > > > > + dev_err(dev, "cannot prepare/enable core clock\n"); > > > > + return ret; > > > > + } > > > > > > Should these clocks really be handled explicitly in the driver? Are > > > these not the bus clocks, to be handled by "msm_bus"? > > > > This not the bus clock. This clock is for the Bus Interface Unit > > between the PCIe module and the System NOC. > > > > Right, that was the piece I meant. Sorry for not using the right > nomenclature. > > So then it would be handled by the msm_bus in the downstream kernel? > > > Perhaps we can merge it like this and once we have the interconnect > framework setup we can make this the fallback method. Agree that this has to be handled by the interconnect framework. For now will rename this as "iface" similar to v0 and v1. Thanks Varada > Thanks, > Bjorn -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi, On 07/19/2017 09:49 AM, Varadarajan Narayanan wrote: > On Tue, Jul 18, 2017 at 09:44:38AM -0700, Bjorn Andersson wrote: >> On Tue 18 Jul 02:58 PDT 2017, Varadarajan Narayanan wrote: >> >>> On Mon, Jul 17, 2017 at 03:07:18PM -0700, Bjorn Andersson wrote: >>>> On Mon 17 Jul 05:04 PDT 2017, Varadarajan Narayanan wrote: >> [..] >>>> >>>> Can you confirm that this is actually version 4 of this block? Or are we >>>> just incrementing an arbitrary number here? >>> >>> This is not exactly the 4th version of the block. However, it is >>> a different version than the ones that are already supported in >>> this driver. Since the existing driver didn't exactly tie it with >>> the block IP version, I too followed the same versioning >>> convention. >>> >> >> Do you have a block IP version that you could base your numbering on, to >> break the trend? (We should go back and fix up the others as well) > > Presently, the driver supports the ipq8064, apq8064, apq8084, > msm8996, ipq4019 and ipq8074. The SoCs, qcom_pcie_ops version and > the block IP versions are as follows. > > ipq8064 - v0 - 2.1.0 > apq8064 - v0 - 2.1.0 > apq8084 - v1 - 1.0.0 > msm8996 - v2 - 2.3.2 > ipq4019 - v3 - 2.4.0 > ipq8074 - v4 - 2.3.3 That's nice, but I think we need the Synopsys IP versions too, can you provide such an information and after that we can decide how the names should look like. > > I will rename the qcom_pcie_ops structure and related functions > with the block IP version instead of vX numbering and post the > patch. regards, Stan
Stan, On Wed, Jul 19, 2017 at 10:12:45AM +0300, Stanimir Varbanov wrote: > Hi, > > On 07/19/2017 09:49 AM, Varadarajan Narayanan wrote: > > On Tue, Jul 18, 2017 at 09:44:38AM -0700, Bjorn Andersson wrote: > >> On Tue 18 Jul 02:58 PDT 2017, Varadarajan Narayanan wrote: > >> > >>> On Mon, Jul 17, 2017 at 03:07:18PM -0700, Bjorn Andersson wrote: > >>>> On Mon 17 Jul 05:04 PDT 2017, Varadarajan Narayanan wrote: > >> [..] > >>>> > >>>> Can you confirm that this is actually version 4 of this block? Or are we > >>>> just incrementing an arbitrary number here? > >>> > >>> This is not exactly the 4th version of the block. However, it is > >>> a different version than the ones that are already supported in > >>> this driver. Since the existing driver didn't exactly tie it with > >>> the block IP version, I too followed the same versioning > >>> convention. > >>> > >> > >> Do you have a block IP version that you could base your numbering on, to > >> break the trend? (We should go back and fix up the others as well) > > > > Presently, the driver supports the ipq8064, apq8064, apq8084, > > msm8996, ipq4019 and ipq8074. The SoCs, qcom_pcie_ops version and > > the block IP versions are as follows. > > > > ipq8064 - v0 - 2.1.0 > > apq8064 - v0 - 2.1.0 > > apq8084 - v1 - 1.0.0 > > msm8996 - v2 - 2.3.2 > > ipq4019 - v3 - 2.4.0 > > ipq8074 - v4 - 2.3.3 > > That's nice, but I think we need the Synopsys IP versions too, can you > provide such an information and after that we can decide how the names > should look like. Sorry, I posted v2 before I saw this e-mail. Will post v3 based on what naming style is decided. The SoCs, qcom_pcie_ops version, the block IP version and Synopsys IP versions are as follows. ipq8064 - v0 - 2.1.0 - 4.01a apq8064 - v0 - 2.1.0 - 4.01a apq8084 - v1 - 1.0.0 - 4.11a msm8996 - v2 - 2.3.2 - 4.21a ipq4019 - v3 - 2.4.0 - 4.20a ipq8074 - v4 - 2.3.3 - 4.30a Thanks Varada > > I will rename the qcom_pcie_ops structure and related functions > > with the block IP version instead of vX numbering and post the > > patch. > > > > regards, > Stan -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi, On 07/19/2017 02:59 PM, Varadarajan Narayanan wrote: > Stan, > > On Wed, Jul 19, 2017 at 10:12:45AM +0300, Stanimir Varbanov wrote: >> Hi, >> >> On 07/19/2017 09:49 AM, Varadarajan Narayanan wrote: >>> On Tue, Jul 18, 2017 at 09:44:38AM -0700, Bjorn Andersson wrote: >>>> On Tue 18 Jul 02:58 PDT 2017, Varadarajan Narayanan wrote: >>>> >>>>> On Mon, Jul 17, 2017 at 03:07:18PM -0700, Bjorn Andersson wrote: >>>>>> On Mon 17 Jul 05:04 PDT 2017, Varadarajan Narayanan wrote: >>>> [..] >>>>>> Can you confirm that this is actually version 4 of this block? Or are we >>>>>> just incrementing an arbitrary number here? >>>>> This is not exactly the 4th version of the block. However, it is >>>>> a different version than the ones that are already supported in >>>>> this driver. Since the existing driver didn't exactly tie it with >>>>> the block IP version, I too followed the same versioning >>>>> convention. >>>>> >>>> Do you have a block IP version that you could base your numbering on, to >>>> break the trend? (We should go back and fix up the others as well) >>> Presently, the driver supports the ipq8064, apq8064, apq8084, >>> msm8996, ipq4019 and ipq8074. The SoCs, qcom_pcie_ops version and >>> the block IP versions are as follows. >>> >>> ipq8064 - v0 - 2.1.0 >>> apq8064 - v0 - 2.1.0 >>> apq8084 - v1 - 1.0.0 >>> msm8996 - v2 - 2.3.2 >>> ipq4019 - v3 - 2.4.0 >>> ipq8074 - v4 - 2.3.3 >> That's nice, but I think we need the Synopsys IP versions too, can you >> provide such an information and after that we can decide how the names >> should look like. > Sorry, I posted v2 before I saw this e-mail. Will post v3 based > on what naming style is decided. > > The SoCs, qcom_pcie_ops version, the block IP version and > Synopsys IP versions are as follows. > > ipq8064 - v0 - 2.1.0 - 4.01a > apq8064 - v0 - 2.1.0 - 4.01a > apq8084 - v1 - 1.0.0 - 4.11a > msm8996 - v2 - 2.3.2 - 4.21a > ipq4019 - v3 - 2.4.0 - 4.20a > ipq8074 - v4 - 2.3.3 - 4.30a I would have loved to say the dwc IP versions sounds better, but this is the qcom wrapper over dwc. So, for me it makes more sense to have qcom specific IP version names. Can we have couple or more versions of qcom pcie IPs based on same dwc IP version? I have not looked closely, but in that case too using qcom nomenclature would again make more sense. Thanks Vivek > > Thanks > Varada > >>> I will rename the qcom_pcie_ops structure and related functions >>> with the block IP version instead of vX numbering and post the >>> patch. >> >> >> regards, >> Stan > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 07/19/2017 12:29 PM, Varadarajan Narayanan wrote: > Stan, > <cut> >> That's nice, but I think we need the Synopsys IP versions too, can you >> provide such an information and after that we can decide how the names >> should look like. > > Sorry, I posted v2 before I saw this e-mail. Will post v3 based > on what naming style is decided. > > The SoCs, qcom_pcie_ops version, the block IP version and > Synopsys IP versions are as follows. > > ipq8064 - v0 - 2.1.0 - 4.01a > apq8064 - v0 - 2.1.0 - 4.01a > apq8084 - v1 - 1.0.0 - 4.11a > msm8996 - v2 - 2.3.2 - 4.21a > ipq4019 - v3 - 2.4.0 - 4.20a > ipq8074 - v4 - 2.3.3 - 4.30a I'm not sure, with Synopsys versions the mess becomes bigger. So I tend to agree with qcom versions (because this driver taking care of qcom wrappers over Synopsys IP) plus comments about Synopsys version used for this particular SoC, just for completeness. regards, Stan
diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c index d15657d..c1fa356 100644 --- a/drivers/pci/dwc/pcie-qcom.c +++ b/drivers/pci/dwc/pcie-qcom.c @@ -37,6 +37,20 @@ #include "pcie-designware.h" #define PCIE20_PARF_SYS_CTRL 0x00 +#define MST_WAKEUP_EN BIT(13) +#define SLV_WAKEUP_EN BIT(12) +#define MSTR_ACLK_CGC_DIS BIT(10) +#define SLV_ACLK_CGC_DIS BIT(9) +#define CORE_CLK_CGC_DIS BIT(6) +#define AUX_PWR_DET BIT(4) +#define L23_CLK_RMV_DIS BIT(2) +#define L1_CLK_RMV_DIS BIT(1) + +#define PCIE20_COMMAND_STATUS 0x04 +#define CMD_BME_VAL 0x4 +#define PCIE20_DEVICE_CONTROL2_STATUS2 0x98 +#define PCIE_CAP_CPL_TIMEOUT_DISABLE 0x10 + #define PCIE20_PARF_PHY_CTRL 0x40 #define PCIE20_PARF_PHY_REFCLK 0x4C #define PCIE20_PARF_DBI_BASE_ADDR 0x168 @@ -58,9 +72,22 @@ #define CFG_BRIDGE_SB_INIT BIT(0) #define PCIE20_CAP 0x70 +#define PCIE20_CAP_LINK_CAPABILITIES (PCIE20_CAP + 0xC) +#define PCIE20_CAP_LINK_1 (PCIE20_CAP + 0x14) +#define PCIE_CAP_LINK1_VAL 0x2fd7f + +#define PCIE20_PARF_Q2A_FLUSH 0x1AC + +#define PCIE20_MISC_CONTROL_1_REG 0x8BC +#define DBI_RO_WR_EN 1 #define PERST_DELAY_US 1000 +#define AXI_CLK_RATE 200000000 + +#define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358 +#define SLV_ADDR_SPACE_SZ 0x10000000 + struct qcom_pcie_resources_v0 { struct clk *iface_clk; struct clk *core_clk; @@ -110,11 +137,26 @@ struct qcom_pcie_resources_v3 { struct reset_control *phy_ahb_reset; }; +struct qphy_reset { + struct reset_control *rst; + char *name; +}; + +struct qcom_pcie_resources_v4 { + struct clk *sys_noc_clk; + struct clk *axi_m_clk; + struct clk *axi_s_clk; + struct clk *ahb_clk; + struct clk *aux_clk; + struct qphy_reset rst[7]; +}; + union qcom_pcie_resources { struct qcom_pcie_resources_v0 v0; struct qcom_pcie_resources_v1 v1; struct qcom_pcie_resources_v2 v2; struct qcom_pcie_resources_v3 v3; + struct qcom_pcie_resources_v4 v4; }; struct qcom_pcie; @@ -139,6 +181,16 @@ struct qcom_pcie { #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) +static inline void +writel_masked(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(pcie->reset, 1); @@ -884,6 +936,205 @@ static int qcom_pcie_init_v3(struct qcom_pcie *pcie) return ret; } +static int qcom_pcie_get_resources_v4(struct qcom_pcie *pcie) +{ + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; + struct dw_pcie *pci = pcie->pci; + struct device *dev = pci->dev; + int i; + + res->sys_noc_clk = devm_clk_get(dev, "sys_noc"); + if (IS_ERR(res->sys_noc_clk)) + return PTR_ERR(res->sys_noc_clk); + + res->axi_m_clk = devm_clk_get(dev, "axi_m"); + if (IS_ERR(res->axi_m_clk)) + return PTR_ERR(res->axi_m_clk); + + res->axi_s_clk = devm_clk_get(dev, "axi_s"); + if (IS_ERR(res->axi_s_clk)) + return PTR_ERR(res->axi_s_clk); + + res->ahb_clk = devm_clk_get(dev, "ahb"); + if (IS_ERR(res->ahb_clk)) + return PTR_ERR(res->ahb_clk); + + res->aux_clk = devm_clk_get(dev, "aux"); + if (IS_ERR(res->aux_clk)) + return PTR_ERR(res->aux_clk); + + res->rst[0].name = "axi_m"; + res->rst[1].name = "axi_s"; + res->rst[2].name = "pipe"; + res->rst[3].name = "axi_m_sticky"; + res->rst[4].name = "sticky"; + res->rst[5].name = "ahb"; + res->rst[6].name = "sleep"; + + for (i = 0; i < ARRAY_SIZE(res->rst); i++) { + res->rst[i].rst = devm_reset_control_get(dev, res->rst[i].name); + if (IS_ERR(res->rst[i].rst)) + return PTR_ERR(res->rst[i].rst); + } + + return 0; +} + +static void qcom_pcie_deinit_v4(struct qcom_pcie *pcie) +{ + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; + + clk_disable_unprepare(res->sys_noc_clk); + clk_disable_unprepare(res->axi_m_clk); + clk_disable_unprepare(res->axi_s_clk); + clk_disable_unprepare(res->ahb_clk); + clk_disable_unprepare(res->aux_clk); +} + +static int qcom_pcie_enable_resources_v4(struct qcom_pcie *pcie) +{ + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; + struct dw_pcie *pci = pcie->pci; + struct device *dev = pci->dev; + int ret; + + ret = clk_prepare_enable(res->sys_noc_clk); + if (ret) { + dev_err(dev, "cannot prepare/enable core clock\n"); + return ret; + } + + ret = clk_prepare_enable(res->axi_m_clk); + if (ret) { + dev_err(dev, "cannot prepare/enable core clock\n"); + goto err_clk_axi_m; + } + + ret = clk_set_rate(res->axi_m_clk, AXI_CLK_RATE); + if (ret) { + dev_err(dev, "MClk rate set failed (%d)\n", ret); + goto err_clk_axi_m; + } + + ret = clk_prepare_enable(res->axi_s_clk); + if (ret) { + dev_err(dev, "cannot prepare/enable axi slave clock\n"); + goto err_clk_axi_s; + } + + ret = clk_set_rate(res->axi_s_clk, AXI_CLK_RATE); + if (ret) { + dev_err(dev, "MClk rate set failed (%d)\n", ret); + goto err_clk_axi_s; + } + + ret = clk_prepare_enable(res->ahb_clk); + if (ret) { + dev_err(dev, "cannot prepare/enable ahb clock\n"); + goto err_clk_ahb; + } + + ret = clk_prepare_enable(res->aux_clk); + if (ret) { + dev_err(dev, "cannot prepare/enable aux clock\n"); + goto err_clk_aux; + } + + udelay(1); + + return 0; + +err_clk_aux: + clk_disable_unprepare(res->ahb_clk); +err_clk_ahb: + clk_disable_unprepare(res->axi_s_clk); +err_clk_axi_s: + clk_disable_unprepare(res->axi_m_clk); +err_clk_axi_m: + clk_disable_unprepare(res->sys_noc_clk); + + return ret; +} + +static inline int qphy_reset_control(struct qcom_pcie *pcie, + struct qphy_reset *r, + bool assert) +{ + int ret; + + if (assert) + ret = reset_control_assert(r->rst); + else + ret = reset_control_deassert(r->rst); + + if (ret) + dev_err(pcie->pci->dev, "%s: reset %s failed for %s\n", + __func__, assert ? "assert" : "deassert", r->name); + + return ret; +} + +static void qcom_pcie_v4_reset(struct qcom_pcie *pcie) +{ + struct qcom_pcie_resources_v4 *res = &pcie->res.v4; + struct qphy_reset *qphy_rst = &res->rst[0]; + int i; + + for (i = 0; i < ARRAY_SIZE(res->rst); i++) + if (qphy_reset_control(pcie, &qphy_rst[i], true)) + return; + + usleep_range(10000, 12000); /* wait 12ms */ + + for (i = 0; i < ARRAY_SIZE(res->rst); i++) + if (qphy_reset_control(pcie, &qphy_rst[i], false)) + return; + + usleep_range(10000, 12000); /* wait 12ms */ + wmb(); /* ensure data is written to hw register */ +} + +static int qcom_pcie_init_v4(struct qcom_pcie *pcie) +{ + struct dw_pcie *pci = pcie->pci; + int ret; + + qcom_pcie_v4_reset(pcie); + qcom_ep_reset_assert(pcie); + + ret = qcom_pcie_enable_resources_v4(pcie); + if (ret) + return ret; + + writel(SLV_ADDR_SPACE_SZ, pcie->parf + + PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE); + + ret = phy_power_on(pcie->phy); + if (ret) + return ret; + + writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0); + + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR); + + writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS + | SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS | + AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS, + pcie->parf + PCIE20_PARF_SYS_CTRL); + writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH); + + writel(CMD_BME_VAL, pci->dbi_base + PCIE20_COMMAND_STATUS); + writel(DBI_RO_WR_EN, pci->dbi_base + PCIE20_MISC_CONTROL_1_REG); + writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + PCIE20_CAP_LINK_1); + + writel_masked(pci->dbi_base + PCIE20_CAP_LINK_CAPABILITIES, + BIT(10) | BIT(11), 0); + writel(PCIE_CAP_CPL_TIMEOUT_DISABLE, pci->dbi_base + + PCIE20_DEVICE_CONTROL2_STATUS2); + + return 0; +} + static int qcom_pcie_link_up(struct dw_pcie *pci) { u16 val = readw(pci->dbi_base + PCIE20_CAP + PCI_EXP_LNKSTA); @@ -985,6 +1236,13 @@ static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, .ltssm_enable = qcom_pcie_v2_ltssm_enable, }; +static const struct qcom_pcie_ops ops_v4 = { + .get_resources = qcom_pcie_get_resources_v4, + .init = qcom_pcie_init_v4, + .deinit = qcom_pcie_deinit_v4, + .ltssm_enable = qcom_pcie_v2_ltssm_enable, +}; + static int qcom_pcie_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -1076,6 +1334,7 @@ static int qcom_pcie_probe(struct platform_device *pdev) { .compatible = "qcom,pcie-apq8084", .data = &ops_v1 }, { .compatible = "qcom,pcie-msm8996", .data = &ops_v2 }, { .compatible = "qcom,pcie-ipq4019", .data = &ops_v3 }, + { .compatible = "qcom,pcie-ipq8074", .data = &ops_v4 }, { } };