* [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe
@ 2019-03-13 9:15 Richard Zhu
2019-03-13 9:15 ` [RFC 2/2] PCI: imx6: " Richard Zhu
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Richard Zhu @ 2019-03-13 9:15 UTC (permalink / raw)
To: bhelgaas, lorenzo.pieralisi, l.stach, andrew.smirnov
Cc: linux-pci, linux-arm-kernel, linux-kernel, Richard Zhu
Add codes needed to support i.MX8QM/QXP PCIe.
- HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
The PCIe and SATA modules are contained in the HSIO subsystem. There
are two PCIe, one SATA controllers and three mixed lane PHYs on
i.MX8QM. There are three use cases of the HSIO subsystem on i.MX8QM.
1. PCIea 2 lanes and one SATA AHCI port.
2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
3. PCIea 2 lanes, PCIeb 1 lane.
i.MX8QXP only has PCIeb controller and one lane PHY.
Use the hsio-cfg property to specify the different modes.
- The HSIO address map as viewed from system level is as shown below.
address [31:24] Local address Target Address Size
5F 0 HSIO 16MB
60-6F 40-4F HSIO 256MB
70-7F 80-8F HSIO 256MB
The property local-addr is required to specify it.
- Both external OSC and internal PLL can be used as PCIe reference
clock, use the ext_osc property to distinguish them.
- clock request GPIO for controlling the PCI reference clock request
signal. And should be configure OD when L1SS maybe enabled later.
- One more power domain HSIO_GPIO and clock PCIE_PER are required by
i.MX8QM/QXP PCIe.
Add these specific properties to enable i.MX8QM/QXP PCIe.
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
.../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index a7f5f5a..f7586c9 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -10,6 +10,8 @@ Required properties:
- "fsl,imx6qp-pcie"
- "fsl,imx7d-pcie"
- "fsl,imx8mq-pcie"
+ - "fsl,imx8qm-pcie"
+ - "fsl,imx8qxp-pcie"
- reg: base address and length of the PCIe controller
- interrupts: A list of interrupt outputs of the controller. Must contain an
entry for each entry in the interrupt-names property.
@@ -38,6 +40,10 @@ Optional properties:
The regulator will be enabled when initializing the PCIe host and
disabled either as part of the init process or when shutting down the
host.
+- clkreq-gpio: Should specify the GPIO for controlling the PCI reference clock
+ request signal.
+- ext_osc: External OSC is used as PCIe reference clock or not. 0: Internal
+ PLL. 1: External OSC.
Additional required properties for imx6sx-pcie:
- clock names: Must include the following additional entries:
@@ -60,6 +66,21 @@ Additional required properties for imx8mq-pcie:
- clock-names: Must include the following additional entries:
- "pcie_aux"
+Additional required properties for imx8qm/qxp pcie:
+- power-domains: Must be set to a phandle pointing to PCIE, PCIE_PHY power and
+ HSIO_GPIO domains
+- power-domain-names: Must be "pcie", "pcie_phy", "hsio_gpio"
+- clock-names: Must include the following additional entries:
+ - "pcie_per"
+- hsio-cfg: hsio configration mode when the pcie node is supported.
+ 1: pciea 2 lanes and one sata ahci port.
+ 2: pciea 1 lane, pcieb 1 lane and one sata ahci port.
+ 3: pciea 2 lanes, pcieb 1 lane.
+- local-addr: the local address used in hsio module on i.MX8QM/QXP.
+ Example:
+ hsio-cfg = <2>;
+ local-addr = <0x80000000>;
+
Example:
pcie@01000000 {
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
2019-03-13 9:15 [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe Richard Zhu
@ 2019-03-13 9:15 ` Richard Zhu
2019-03-13 20:20 ` Andrey Smirnov
2019-03-14 9:30 ` [RFC 1/2] dt-bindings: imx6q-pcie: " Lucas Stach
2019-03-15 2:26 ` Andrey Smirnov
2 siblings, 1 reply; 10+ messages in thread
From: Richard Zhu @ 2019-03-13 9:15 UTC (permalink / raw)
To: bhelgaas, lorenzo.pieralisi, l.stach, andrew.smirnov
Cc: linux-pci, linux-arm-kernel, linux-kernel, Richard Zhu
Add codes needed to support i.MX8QM/QXP PCIe.
- HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
The PCIe and SATA modules are contained in the HSIO subsystem. There
are two PCIe, one SATA controllers and three mixed lane PHYs on
i.MX8QM. There are three use cases of the HSIO subsystem on i.MX8QM.
1. PCIea 2 lanes and one SATA AHCI port.
2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
3. PCIea 2 lanes, PCIeb 1 lane.
i.MX8QXP only has PCIeb controller and one lane PHY.
- The HSIO address map as viewed from system level is as shown below.
address [31:24] Local address Target Address Size
5F 0 HSIO 16MB
60-6F 40-4F HSIO 256MB
70-7F 80-8F HSIO 256MB
So, the cpu_addr_fixup is required to enable i.MX8QM/QXP PCIe.
- Both external OSC and internal PLL can be used as PCIe reference
clock.
- clock request GPIO for controlling the PCI reference clock request
signal. And should be configure OD when L1SS maybe enabled later.
- One more power domain HSIO_GPIO and clock PCIE_PER are required by
i.MX8QM/QXP PCIe.
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 392 +++++++++++++++++++++++++++++++++-
1 file changed, 387 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index aaa9489..aacefb6 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -39,6 +39,7 @@
#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
#define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
#define IMX8MQ_PCIE2_BASE_ADDR 0x33c00000
+#define IMX8_HSIO_PCIEB_BASE_ADDR 0x5f010000
#define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
@@ -48,10 +49,13 @@ enum imx6_pcie_variants {
IMX6QP,
IMX7D,
IMX8MQ,
+ IMX8QM,
+ IMX8QXP,
};
#define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
#define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
+#define IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP BIT(2)
struct imx6_pcie_drvdata {
enum imx6_pcie_variants variant;
@@ -60,10 +64,12 @@ struct imx6_pcie_drvdata {
struct imx6_pcie {
struct dw_pcie *pci;
+ int clkreq_gpio;
int reset_gpio;
bool gpio_active_high;
struct clk *pcie_bus;
struct clk *pcie_phy;
+ struct clk *pcie_per;
struct clk *pcie_inbound_axi;
struct clk *pcie;
struct clk *pcie_aux;
@@ -77,6 +83,9 @@ struct imx6_pcie {
u32 tx_deemph_gen2_6db;
u32 tx_swing_full;
u32 tx_swing_low;
+ u32 hsio_cfg;
+ u32 ext_osc;
+ u32 local_addr;
int link_gen;
struct regulator *vpcie;
void __iomem *phy_base;
@@ -85,6 +94,8 @@ struct imx6_pcie {
struct device *pd_pcie;
/* power domain for pcie phy */
struct device *pd_pcie_phy;
+ /* power domain for hsio gpio used by pcie */
+ struct device *pd_hsio_gpio;
const struct imx6_pcie_drvdata *drvdata;
};
@@ -92,6 +103,7 @@ struct imx6_pcie {
#define PHY_PLL_LOCK_WAIT_MAX_RETRIES 2000
#define PHY_PLL_LOCK_WAIT_USLEEP_MIN 50
#define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200
+#define L2_ENTRY_WAIT_MAX_RETRIES 10000
/* PCIe Root Complex registers (memory-mapped) */
#define PCIE_RC_IMX6_MSI_CAP 0x50
@@ -157,6 +169,43 @@ struct imx6_pcie {
#define PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5)
#define PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
+/* iMX8 HSIO registers */
+#define IMX8QM_CSR_PHYX2_OFFSET 0x00000
+#define IMX8QM_CSR_PHYX1_OFFSET 0x10000
+#define IMX8QM_CSR_PHYX_STTS0_OFFSET 0x4
+#define IMX8QM_CSR_PCIEA_OFFSET 0x20000
+#define IMX8QM_CSR_PCIEB_OFFSET 0x30000
+#define IMX8QM_CSR_PCIE_CTRL1_OFFSET 0x4
+#define IMX8QM_CSR_PCIE_CTRL2_OFFSET 0x8
+#define IMX8QM_CSR_PCIE_STTS0_OFFSET 0xC
+#define IMX8QM_CSR_MISC_OFFSET 0x50000
+
+#define IMX8QM_CTRL_LTSSM_ENABLE BIT(4)
+#define IMX8QM_CTRL_READY_ENTR_L23 BIT(5)
+#define IMX8QM_CTRL_PM_XMT_TURNOFF BIT(9)
+#define IMX8QM_CTRL_BUTTON_RST_N BIT(21)
+#define IMX8QM_CTRL_PERST_N BIT(22)
+#define IMX8QM_CTRL_POWER_UP_RST_N BIT(23)
+
+#define IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2 BIT(13)
+#define IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST BIT(19)
+#define IMX8QM_STTS0_LANE0_TX_PLL_LOCK BIT(4)
+#define IMX8QM_STTS0_LANE1_TX_PLL_LOCK BIT(12)
+
+#define IMX8QM_PCIE_TYPE_MASK GENMASK(27, 24)
+
+#define IMX8QM_PHYX2_CTRL0_APB_MASK GENMASK(1, 0)
+#define IMX8QM_PHY_APB_RSTN_0 BIT(0)
+#define IMX8QM_PHY_APB_RSTN_1 BIT(1)
+
+#define IMX8QM_MISC_IOB_RXENA BIT(0)
+#define IMX8QM_MISC_IOB_TXENA BIT(1)
+#define IMX8QM_CSR_MISC_IOB_A_0_TXOE BIT(2)
+#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK GENMASK(4, 3)
+#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_2 BIT(4)
+#define IMX8QM_MISC_PHYX1_EPCS_SEL BIT(12)
+#define IMX8QM_MISC_PCIE_AB_SELECT BIT(13)
+
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int exp_val)
{
struct dw_pcie *pci = imx6_pcie->pci;
@@ -373,14 +422,65 @@ static int imx6_pcie_attach_pd(struct device *dev)
return PTR_ERR(link);
}
+ switch (imx6_pcie->drvdata->variant) {
+ case IMX8QM:
+ case IMX8QXP:
+ imx6_pcie->pd_hsio_gpio = dev_pm_domain_attach_by_name(dev,
+ "hsio_gpio");
+ if (IS_ERR(imx6_pcie->pd_hsio_gpio))
+ return PTR_ERR(imx6_pcie->pd_hsio_gpio);
+
+ link = device_link_add(dev, imx6_pcie->pd_hsio_gpio,
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+ if (!link) {
+ dev_err(dev, "Failed to add device_link to gpio pd.\n");
+ return -EINVAL;
+ }
+
+ break;
+ default:
+ break;
+ }
+
return 0;
}
static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
{
+ u32 addr;
+ int i;
struct device *dev = imx6_pcie->pci->dev;
switch (imx6_pcie->drvdata->variant) {
+ case IMX8QXP:
+ addr = IMX8QM_CSR_PCIEB_OFFSET + IMX8QM_CSR_PCIE_CTRL2_OFFSET;
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
+ IMX8QM_CTRL_BUTTON_RST_N,
+ IMX8QM_CTRL_BUTTON_RST_N);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
+ IMX8QM_CTRL_PERST_N,
+ IMX8QM_CTRL_PERST_N);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
+ IMX8QM_CTRL_POWER_UP_RST_N,
+ IMX8QM_CTRL_POWER_UP_RST_N);
+ break;
+ case IMX8QM:
+ for (i = 0; i <= imx6_pcie->controller_id; i++) {
+ addr = IMX8QM_CSR_PCIEA_OFFSET + i * SZ_64K;
+ addr += IMX8QM_CSR_PCIE_CTRL2_OFFSET;
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
+ IMX8QM_CTRL_BUTTON_RST_N,
+ IMX8QM_CTRL_BUTTON_RST_N);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
+ IMX8QM_CTRL_PERST_N,
+ IMX8QM_CTRL_PERST_N);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
+ IMX8QM_CTRL_POWER_UP_RST_N,
+ IMX8QM_CTRL_POWER_UP_RST_N);
+ }
+ break;
case IMX7D:
case IMX8MQ:
reset_control_assert(imx6_pcie->pciephy_reset);
@@ -477,6 +577,21 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
break;
+ case IMX8QXP:
+ case IMX8QM:
+ ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
+ if (ret) {
+ dev_err(dev, "unable to enable pcie_axi clock\n");
+ break;
+ }
+ ret = clk_prepare_enable(imx6_pcie->pcie_per);
+ if (ret) {
+ dev_err(dev, "unable to enable pcie_per clock\n");
+ clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
+ break;
+ }
+
+ break;
}
return ret;
@@ -501,6 +616,63 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
dev_err(dev, "PCIe PLL lock timeout\n");
}
+static int imx8_hsio_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
+{
+ u32 retries, addr, val, lock = 0;
+ int ret;
+ struct dw_pcie *pci = imx6_pcie->pci;
+ struct device *dev = pci->dev;
+
+ addr = IMX8QM_CSR_PCIEA_OFFSET + imx6_pcie->controller_id * SZ_64K;
+ addr += IMX8QM_CSR_PCIE_STTS0_OFFSET;
+ for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES; retries++) {
+ regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
+ if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST) == 0)
+ break;
+ udelay(1);
+ }
+
+ if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST) != 0) {
+ dev_err(dev, "ERROR: PM_REQ_CORE_RST is still set.\n");
+ return -ENODEV;
+ }
+
+ addr = IMX8QM_CSR_PHYX2_OFFSET + imx6_pcie->controller_id * SZ_64K;
+ addr += IMX8QM_CSR_PHYX_STTS0_OFFSET;
+ for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES; retries++) {
+ regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
+ if (imx6_pcie->hsio_cfg == 2) {
+ if (imx6_pcie->controller_id == 0)
+ lock = IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
+ else
+ lock = IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
+ } else if (imx6_pcie->hsio_cfg == 3) {
+ lock = IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
+ if (imx6_pcie->controller_id == 0)
+ lock |= IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
+ } else if (imx6_pcie->hsio_cfg == 1) {
+ lock = IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
+ lock |= IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
+ } else {
+ dev_err(dev, "ERROR: illegal hsio_cfg value.\n");
+ return -EINVAL;
+ }
+ val &= lock;
+ if (val == lock)
+ break;
+ udelay(10);
+ }
+
+ if (retries >= PHY_PLL_LOCK_WAIT_MAX_RETRIES) {
+ dev_info(dev, "pcie phy pll can't be locked.\n");
+ ret = -ENODEV;
+ } else {
+ dev_info(dev, "pcie phy pll is locked.\n");
+ }
+
+ return ret;
+}
+
static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
{
struct dw_pcie *pci = imx6_pcie->pci;
@@ -553,6 +725,11 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
}
switch (imx6_pcie->drvdata->variant) {
+ case IMX8QXP:
+ case IMX8QM:
+ /* wait for phy pll lock firstly. */
+ imx8_hsio_pcie_wait_for_phy_pll_lock(imx6_pcie);
+ break;
case IMX8MQ:
reset_control_deassert(imx6_pcie->pciephy_reset);
break;
@@ -613,25 +790,114 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
{
- unsigned int mask, val;
-
- if (imx6_pcie->drvdata->variant == IMX8MQ &&
+ unsigned int offset, mask, val;
+
+ if (imx6_pcie->drvdata->variant == IMX8QM ||
+ imx6_pcie->drvdata->variant == IMX8QXP) {
+ offset = IMX8QM_CSR_PCIEA_OFFSET +
+ imx6_pcie->controller_id * SZ_64K;
+ mask = IMX8QM_PCIE_TYPE_MASK;
+ val = FIELD_PREP(IMX8QM_PCIE_TYPE_MASK,
+ PCI_EXP_TYPE_ROOT_PORT);
+ } else if (imx6_pcie->drvdata->variant == IMX8MQ &&
imx6_pcie->controller_id == 1) {
+ offset = IOMUXC_GPR12;
mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
PCI_EXP_TYPE_ROOT_PORT);
} else {
+ offset = IOMUXC_GPR12;
mask = IMX6Q_GPR12_DEVICE_TYPE;
val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
PCI_EXP_TYPE_ROOT_PORT);
}
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, offset, mask, val);
}
static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
{
switch (imx6_pcie->drvdata->variant) {
+ case IMX8QXP:
+ case IMX8QM:
+ if (imx6_pcie->hsio_cfg == 1) {
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_PHYX2_OFFSET,
+ IMX8QM_PHYX2_CTRL0_APB_MASK,
+ IMX8QM_PHY_APB_RSTN_0 |
+ IMX8QM_PHY_APB_RSTN_1);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_PHYX1_EPCS_SEL,
+ IMX8QM_MISC_PHYX1_EPCS_SEL);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_PCIE_AB_SELECT,
+ 0);
+ } else if (imx6_pcie->hsio_cfg == 2) {
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_PHYX2_OFFSET,
+ IMX8QM_PHYX2_CTRL0_APB_MASK,
+ IMX8QM_PHY_APB_RSTN_0 |
+ IMX8QM_PHY_APB_RSTN_1);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_PHYX1_EPCS_SEL,
+ IMX8QM_MISC_PHYX1_EPCS_SEL);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_PCIE_AB_SELECT,
+ IMX8QM_MISC_PCIE_AB_SELECT);
+ } else if (imx6_pcie->hsio_cfg == 3) {
+ if (imx6_pcie->controller_id)
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_PHYX1_OFFSET,
+ IMX8QM_PHY_APB_RSTN_0,
+ IMX8QM_PHY_APB_RSTN_0);
+ else
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_PHYX2_OFFSET,
+ IMX8QM_PHYX2_CTRL0_APB_MASK,
+ IMX8QM_PHY_APB_RSTN_0 |
+ IMX8QM_PHY_APB_RSTN_1);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_PHYX1_EPCS_SEL, 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_PCIE_AB_SELECT,
+ IMX8QM_MISC_PCIE_AB_SELECT);
+ }
+
+ if (imx6_pcie->ext_osc) {
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_IOB_RXENA,
+ IMX8QM_MISC_IOB_RXENA);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_IOB_TXENA, 0);
+ } else {
+ /* Try to used the internal pll as ref clk */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_IOB_RXENA, 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_MISC_IOB_TXENA,
+ IMX8QM_MISC_IOB_TXENA);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX8QM_CSR_MISC_OFFSET,
+ IMX8QM_CSR_MISC_IOB_A_0_TXOE |
+ IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK,
+ IMX8QM_CSR_MISC_IOB_A_0_TXOE |
+ IMX8QM_CSR_MISC_IOB_A_0_M1M0_2);
+ }
+
+ break;
case IMX8MQ:
/*
* TODO: Currently this code assumes external
@@ -763,6 +1029,7 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_ltssm_enable(struct device *dev)
{
+ u32 val;
struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
switch (imx6_pcie->drvdata->variant) {
@@ -777,6 +1044,15 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
case IMX8MQ:
reset_control_deassert(imx6_pcie->apps_reset);
break;
+ case IMX8QXP:
+ case IMX8QM:
+ val = IMX8QM_CSR_PCIEA_OFFSET +
+ imx6_pcie->controller_id * SZ_64K;
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ val + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
+ IMX8QM_CTRL_LTSSM_ENABLE,
+ IMX8QM_CTRL_LTSSM_ENABLE);
+ break;
}
}
@@ -908,13 +1184,25 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
return 0;
}
+static u64 imx6_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
+{
+ struct pcie_port *pp = &pcie->pp;
+ struct imx6_pcie *imx6_pcie = to_imx6_pcie(pcie);
+
+ if (imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP)
+ return (cpu_addr + imx6_pcie->local_addr - pp->mem_base);
+ else
+ return cpu_addr;
+}
+
static const struct dw_pcie_ops dw_pcie_ops = {
- /* No special ops needed, but pcie-designware still expects this struct */
+ .cpu_addr_fixup = imx6_pcie_cpu_addr_fixup,
};
#ifdef CONFIG_PM_SLEEP
static void imx6_pcie_ltssm_disable(struct device *dev)
{
+ u32 val;
struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
switch (imx6_pcie->drvdata->variant) {
@@ -926,6 +1214,17 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
case IMX7D:
reset_control_assert(imx6_pcie->apps_reset);
break;
+ case IMX8QXP:
+ case IMX8QM:
+ val = IMX8QM_CSR_PCIEA_OFFSET +
+ imx6_pcie->controller_id * SZ_64K;
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ val + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
+ IMX8QM_CTRL_LTSSM_ENABLE, 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ val + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
+ IMX8QM_CTRL_READY_ENTR_L23, 0);
+ break;
default:
dev_err(dev, "ltssm_disable not supported\n");
}
@@ -933,6 +1232,8 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
{
+ int i;
+ u32 addr, val;
struct device *dev = imx6_pcie->pci->dev;
/* Some variants have a turnoff reset in DT */
@@ -951,6 +1252,34 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0);
break;
+ case IMX8QXP:
+ case IMX8QM:
+ addr = IMX8QM_CSR_PCIEA_OFFSET +
+ imx6_pcie->controller_id * SZ_64K;
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ addr + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
+ IMX8QM_CTRL_PM_XMT_TURNOFF,
+ IMX8QM_CTRL_PM_XMT_TURNOFF);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ addr + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
+ IMX8QM_CTRL_PM_XMT_TURNOFF, 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ addr + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
+ IMX8QM_CTRL_READY_ENTR_L23,
+ IMX8QM_CTRL_READY_ENTR_L23);
+ /* check the L2 is entered or not. */
+ for (i = 0; i < L2_ENTRY_WAIT_MAX_RETRIES; i++) {
+ regmap_read(imx6_pcie->iomuxc_gpr,
+ addr + IMX8QM_CSR_PCIE_STTS0_OFFSET,
+ &val);
+ if (val & IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2)
+ break;
+ udelay(10);
+ }
+ if ((val & IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2) == 0)
+ dev_err(dev, "PCIE%d can't enter into L2.\n",
+ imx6_pcie->controller_id);
+ break;
default:
dev_err(dev, "PME_Turn_Off not implemented\n");
return;
@@ -985,6 +1314,11 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
case IMX8MQ:
clk_disable_unprepare(imx6_pcie->pcie_aux);
break;
+ case IMX8QXP:
+ case IMX8QM:
+ clk_disable_unprepare(imx6_pcie->pcie_per);
+ clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
+ break;
default:
break;
}
@@ -1084,7 +1418,26 @@ static int imx6_pcie_probe(struct platform_device *pdev)
if (IS_ERR(pci->dbi_base))
return PTR_ERR(pci->dbi_base);
+ if (of_property_read_u32(node, "hsio-cfg", &imx6_pcie->hsio_cfg))
+ imx6_pcie->hsio_cfg = 0;
+ if (of_property_read_u32(node, "ext_osc", &imx6_pcie->ext_osc) < 0)
+ imx6_pcie->ext_osc = 0;
+ if (of_property_read_u32(node, "local-addr", &imx6_pcie->local_addr))
+ imx6_pcie->local_addr = 0;
+
/* Fetch GPIOs */
+ imx6_pcie->clkreq_gpio = of_get_named_gpio(node, "clkreq-gpio", 0);
+ if (gpio_is_valid(imx6_pcie->clkreq_gpio)) {
+ ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->clkreq_gpio,
+ GPIOF_OUT_INIT_LOW, "PCIe CLKREQ");
+ if (ret) {
+ dev_err(&pdev->dev, "unable to get clkreq gpio\n");
+ return ret;
+ }
+ } else if (imx6_pcie->clkreq_gpio == -EPROBE_DEFER) {
+ return imx6_pcie->clkreq_gpio;
+ }
+
imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio", 0);
imx6_pcie->gpio_active_high = of_property_read_bool(node,
"reset-gpio-active-high");
@@ -1155,6 +1508,25 @@ static int imx6_pcie_probe(struct platform_device *pdev)
return PTR_ERR(imx6_pcie->pcie_aux);
}
break;
+ case IMX8QM:
+ case IMX8QXP:
+ if (dbi_base->start == IMX8_HSIO_PCIEB_BASE_ADDR)
+ imx6_pcie->controller_id = 1;
+
+ imx6_pcie->pcie_per = devm_clk_get(dev, "pcie_per");
+ if (IS_ERR(imx6_pcie->pcie_per)) {
+ dev_err(dev, "pcie_per clock source missing or invalid\n");
+ return PTR_ERR(imx6_pcie->pcie_per);
+ }
+
+ imx6_pcie->pcie_inbound_axi = devm_clk_get(&pdev->dev,
+ "pcie_inbound_axi");
+ if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
+ dev_err(&pdev->dev,
+ "pcie clock source missing or invalid\n");
+ return PTR_ERR(imx6_pcie->pcie_inbound_axi);
+ }
+ break;
default:
break;
}
@@ -1259,6 +1631,14 @@ static const struct imx6_pcie_drvdata drvdata[] = {
[IMX8MQ] = {
.variant = IMX8MQ,
},
+ [IMX8QM] = {
+ .variant = IMX8QM,
+ .flags = IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP,
+ },
+ [IMX8QXP] = {
+ .variant = IMX8QXP,
+ .flags = IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP,
+ },
};
static const struct of_device_id imx6_pcie_of_match[] = {
@@ -1267,6 +1647,8 @@ static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6qp-pcie", .data = &drvdata[IMX6QP], },
{ .compatible = "fsl,imx7d-pcie", .data = &drvdata[IMX7D], },
{ .compatible = "fsl,imx8mq-pcie", .data = &drvdata[IMX8MQ], } ,
+ { .compatible = "fsl,imx8qm-pcie", .data = &drvdata[IMX8QM], },
+ { .compatible = "fsl,imx8qxp-pcie", .data = &drvdata[IMX8QXP], },
{},
};
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
2019-03-13 9:15 ` [RFC 2/2] PCI: imx6: " Richard Zhu
@ 2019-03-13 20:20 ` Andrey Smirnov
2019-03-14 9:18 ` Richard Zhu
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Smirnov @ 2019-03-13 20:20 UTC (permalink / raw)
To: Richard Zhu
Cc: bhelgaas, lorenzo.pieralisi, l.stach, linux-pci,
linux-arm-kernel, linux-kernel
On Wed, Mar 13, 2019 at 2:15 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> Add codes needed to support i.MX8QM/QXP PCIe.
> - HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
> The PCIe and SATA modules are contained in the HSIO subsystem. There
> are two PCIe, one SATA controllers and three mixed lane PHYs on
> i.MX8QM. There are three use cases of the HSIO subsystem on i.MX8QM.
> 1. PCIea 2 lanes and one SATA AHCI port.
> 2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
> 3. PCIea 2 lanes, PCIeb 1 lane.
> i.MX8QXP only has PCIeb controller and one lane PHY.
> - The HSIO address map as viewed from system level is as shown below.
> address [31:24] Local address Target Address Size
> 5F 0 HSIO 16MB
> 60-6F 40-4F HSIO 256MB
> 70-7F 80-8F HSIO 256MB
> So, the cpu_addr_fixup is required to enable i.MX8QM/QXP PCIe.
> - Both external OSC and internal PLL can be used as PCIe reference
> clock.
> - clock request GPIO for controlling the PCI reference clock request
> signal. And should be configure OD when L1SS maybe enabled later.
> - One more power domain HSIO_GPIO and clock PCIE_PER are required by
> i.MX8QM/QXP PCIe.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 392 +++++++++++++++++++++++++++++++++-
> 1 file changed, 387 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index aaa9489..aacefb6 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -39,6 +39,7 @@
> #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
> #define IMX8MQ_PCIE2_BASE_ADDR 0x33c00000
> +#define IMX8_HSIO_PCIEB_BASE_ADDR 0x5f010000
>
> #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
>
> @@ -48,10 +49,13 @@ enum imx6_pcie_variants {
> IMX6QP,
> IMX7D,
> IMX8MQ,
> + IMX8QM,
> + IMX8QXP,
> };
>
> #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> +#define IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP BIT(2)
This is an IMX8Q* specific flag, so it probably should be called
something like IMX6_PCIE_FLAG_IMX8Qx_CPU_ADD_FIXUP.
>
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> @@ -60,10 +64,12 @@ struct imx6_pcie_drvdata {
>
> struct imx6_pcie {
> struct dw_pcie *pci;
> + int clkreq_gpio;
Is this really necessary? On i.MX8MQ vendor tree for some unknown
reason would reconfigure a dedicated CLKREQ_B signal as a GPIO and
then use it as CLKREQ signal that way instead of controlling it via
dedicated bits in register file, so I am wondering if that is the case
with QM and QXP.
> int reset_gpio;
> bool gpio_active_high;
> struct clk *pcie_bus;
> struct clk *pcie_phy;
> + struct clk *pcie_per;
> struct clk *pcie_inbound_axi;
> struct clk *pcie;
> struct clk *pcie_aux;
> @@ -77,6 +83,9 @@ struct imx6_pcie {
> u32 tx_deemph_gen2_6db;
> u32 tx_swing_full;
> u32 tx_swing_low;
> + u32 hsio_cfg;
> + u32 ext_osc;
> + u32 local_addr;
> int link_gen;
> struct regulator *vpcie;
> void __iomem *phy_base;
> @@ -85,6 +94,8 @@ struct imx6_pcie {
> struct device *pd_pcie;
> /* power domain for pcie phy */
> struct device *pd_pcie_phy;
> + /* power domain for hsio gpio used by pcie */
> + struct device *pd_hsio_gpio;
> const struct imx6_pcie_drvdata *drvdata;
> };
>
> @@ -92,6 +103,7 @@ struct imx6_pcie {
> #define PHY_PLL_LOCK_WAIT_MAX_RETRIES 2000
> #define PHY_PLL_LOCK_WAIT_USLEEP_MIN 50
> #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200
> +#define L2_ENTRY_WAIT_MAX_RETRIES 10000
>
> /* PCIe Root Complex registers (memory-mapped) */
> #define PCIE_RC_IMX6_MSI_CAP 0x50
> @@ -157,6 +169,43 @@ struct imx6_pcie {
> #define PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5)
> #define PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
>
> +/* iMX8 HSIO registers */
> +#define IMX8QM_CSR_PHYX2_OFFSET 0x00000
> +#define IMX8QM_CSR_PHYX1_OFFSET 0x10000
> +#define IMX8QM_CSR_PHYX_STTS0_OFFSET 0x4
> +#define IMX8QM_CSR_PCIEA_OFFSET 0x20000
> +#define IMX8QM_CSR_PCIEB_OFFSET 0x30000
> +#define IMX8QM_CSR_PCIE_CTRL1_OFFSET 0x4
> +#define IMX8QM_CSR_PCIE_CTRL2_OFFSET 0x8
> +#define IMX8QM_CSR_PCIE_STTS0_OFFSET 0xC
> +#define IMX8QM_CSR_MISC_OFFSET 0x50000
> +
> +#define IMX8QM_CTRL_LTSSM_ENABLE BIT(4)
> +#define IMX8QM_CTRL_READY_ENTR_L23 BIT(5)
> +#define IMX8QM_CTRL_PM_XMT_TURNOFF BIT(9)
> +#define IMX8QM_CTRL_BUTTON_RST_N BIT(21)
> +#define IMX8QM_CTRL_PERST_N BIT(22)
> +#define IMX8QM_CTRL_POWER_UP_RST_N BIT(23)
> +
> +#define IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2 BIT(13)
> +#define IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST BIT(19)
> +#define IMX8QM_STTS0_LANE0_TX_PLL_LOCK BIT(4)
> +#define IMX8QM_STTS0_LANE1_TX_PLL_LOCK BIT(12)
> +
> +#define IMX8QM_PCIE_TYPE_MASK GENMASK(27, 24)
> +
> +#define IMX8QM_PHYX2_CTRL0_APB_MASK GENMASK(1, 0)
> +#define IMX8QM_PHY_APB_RSTN_0 BIT(0)
> +#define IMX8QM_PHY_APB_RSTN_1 BIT(1)
> +
> +#define IMX8QM_MISC_IOB_RXENA BIT(0)
> +#define IMX8QM_MISC_IOB_TXENA BIT(1)
> +#define IMX8QM_CSR_MISC_IOB_A_0_TXOE BIT(2)
> +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK GENMASK(4, 3)
> +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_2 BIT(4)
> +#define IMX8QM_MISC_PHYX1_EPCS_SEL BIT(12)
> +#define IMX8QM_MISC_PCIE_AB_SELECT BIT(13)
> +
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int exp_val)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> @@ -373,14 +422,65 @@ static int imx6_pcie_attach_pd(struct device *dev)
> return PTR_ERR(link);
> }
>
> + switch (imx6_pcie->drvdata->variant) {
> + case IMX8QM:
> + case IMX8QXP:
> + imx6_pcie->pd_hsio_gpio = dev_pm_domain_attach_by_name(dev,
> + "hsio_gpio");
> + if (IS_ERR(imx6_pcie->pd_hsio_gpio))
> + return PTR_ERR(imx6_pcie->pd_hsio_gpio);
> +
> + link = device_link_add(dev, imx6_pcie->pd_hsio_gpio,
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!link) {
> + dev_err(dev, "Failed to add device_link to gpio pd.\n");
> + return -EINVAL;
> + }
> +
> + break;
> + default:
> + break;
> + }
> +
> return 0;
> }
>
> static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> {
> + u32 addr;
> + int i;
> struct device *dev = imx6_pcie->pci->dev;
>
> switch (imx6_pcie->drvdata->variant) {
> + case IMX8QXP:
> + addr = IMX8QM_CSR_PCIEB_OFFSET + IMX8QM_CSR_PCIE_CTRL2_OFFSET;
This and similar "IMX8QM_CSR_PCIEA_OFFSET + i * SZ_64K" pattern keeps
popping up quite frequently in the code. I think at the very least it
would be good to calculate this offset in probe and store it as a
member of struct imx6_pcie. However I do wonder if this should
actually be handle by either declaring an additional syscon regmap of
additional reg/reg-name property.
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> + IMX8QM_CTRL_BUTTON_RST_N,
> + IMX8QM_CTRL_BUTTON_RST_N);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> + IMX8QM_CTRL_PERST_N,
> + IMX8QM_CTRL_PERST_N);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> + IMX8QM_CTRL_POWER_UP_RST_N,
> + IMX8QM_CTRL_POWER_UP_RST_N);
> + break;
> + case IMX8QM:
> + for (i = 0; i <= imx6_pcie->controller_id; i++) {
This loop is a bit surprising to me. It's hard to tell why you'd
iterate from 0 to controller_id. I think it'd be good to add a comment
explaining the logic behind this code.
> + addr = IMX8QM_CSR_PCIEA_OFFSET + i * SZ_64K;
> + addr += IMX8QM_CSR_PCIE_CTRL2_OFFSET;
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> + IMX8QM_CTRL_BUTTON_RST_N,
> + IMX8QM_CTRL_BUTTON_RST_N);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> + IMX8QM_CTRL_PERST_N,
> + IMX8QM_CTRL_PERST_N);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> + IMX8QM_CTRL_POWER_UP_RST_N,
> + IMX8QM_CTRL_POWER_UP_RST_N);
> + }
> + break;
> case IMX7D:
> case IMX8MQ:
> reset_control_assert(imx6_pcie->pciephy_reset);
> @@ -477,6 +577,21 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
> break;
> + case IMX8QXP:
> + case IMX8QM:
> + ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_axi clock\n");
> + break;
> + }
> + ret = clk_prepare_enable(imx6_pcie->pcie_per);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_per clock\n");
> + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> + break;
> + }
> +
> + break;
> }
>
> return ret;
> @@ -501,6 +616,63 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> dev_err(dev, "PCIe PLL lock timeout\n");
> }
>
> +static int imx8_hsio_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> +{
> + u32 retries, addr, val, lock = 0;
> + int ret;
> + struct dw_pcie *pci = imx6_pcie->pci;
> + struct device *dev = pci->dev;
> +
> + addr = IMX8QM_CSR_PCIEA_OFFSET + imx6_pcie->controller_id * SZ_64K;
> + addr += IMX8QM_CSR_PCIE_STTS0_OFFSET;
> + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES; retries++) {
> + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> + if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST) == 0)
> + break;
> + udelay(1);
> + }
> +
> + if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST) != 0) {
> + dev_err(dev, "ERROR: PM_REQ_CORE_RST is still set.\n");
> + return -ENODEV;
> + }
> +
You can really cut down on boilerplate here by using regmap_read_poll_timeout()
> + addr = IMX8QM_CSR_PHYX2_OFFSET + imx6_pcie->controller_id * SZ_64K;
> + addr += IMX8QM_CSR_PHYX_STTS0_OFFSET;
> + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES; retries++) {
> + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> + if (imx6_pcie->hsio_cfg == 2) {
> + if (imx6_pcie->controller_id == 0)
> + lock = IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> + else
> + lock = IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> + } else if (imx6_pcie->hsio_cfg == 3) {
> + lock = IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> + if (imx6_pcie->controller_id == 0)
> + lock |= IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> + } else if (imx6_pcie->hsio_cfg == 1) {
> + lock = IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> + lock |= IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> + } else {
> + dev_err(dev, "ERROR: illegal hsio_cfg value.\n");
> + return -EINVAL;
> + }
> + val &= lock;
> + if (val == lock)
> + break;
> + udelay(10);
> + }
> +
> + if (retries >= PHY_PLL_LOCK_WAIT_MAX_RETRIES) {
> + dev_info(dev, "pcie phy pll can't be locked.\n");
> + ret = -ENODEV;
> + } else {
> + dev_info(dev, "pcie phy pll is locked.\n");
> + }
> +
Ditto.
> + return ret;
> +}
> +
> static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> @@ -553,6 +725,11 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> }
>
> switch (imx6_pcie->drvdata->variant) {
> + case IMX8QXP:
> + case IMX8QM:
> + /* wait for phy pll lock firstly. */
> + imx8_hsio_pcie_wait_for_phy_pll_lock(imx6_pcie);
> + break;
> case IMX8MQ:
> reset_control_deassert(imx6_pcie->pciephy_reset);
> break;
> @@ -613,25 +790,114 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>
> static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
> {
> - unsigned int mask, val;
> -
> - if (imx6_pcie->drvdata->variant == IMX8MQ &&
> + unsigned int offset, mask, val;
> +
> + if (imx6_pcie->drvdata->variant == IMX8QM ||
> + imx6_pcie->drvdata->variant == IMX8QXP) {
Since there's now more than two possibilities, it'd probably make
sense to convert this code to use switch statement.
> + offset = IMX8QM_CSR_PCIEA_OFFSET +
> + imx6_pcie->controller_id * SZ_64K;
> + mask = IMX8QM_PCIE_TYPE_MASK;
> + val = FIELD_PREP(IMX8QM_PCIE_TYPE_MASK,
> + PCI_EXP_TYPE_ROOT_PORT);
> + } else if (imx6_pcie->drvdata->variant == IMX8MQ &&
> imx6_pcie->controller_id == 1) {
> + offset = IOMUXC_GPR12;
> mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> PCI_EXP_TYPE_ROOT_PORT);
> } else {
> + offset = IOMUXC_GPR12;
> mask = IMX6Q_GPR12_DEVICE_TYPE;
> val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
> PCI_EXP_TYPE_ROOT_PORT);
> }
>
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, offset, mask, val);
> }
>
> static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> {
> switch (imx6_pcie->drvdata->variant) {
> + case IMX8QXP:
> + case IMX8QM:
> + if (imx6_pcie->hsio_cfg == 1) {
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_PHYX2_OFFSET,
> + IMX8QM_PHYX2_CTRL0_APB_MASK,
> + IMX8QM_PHY_APB_RSTN_0 |
> + IMX8QM_PHY_APB_RSTN_1);
> +
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_PHYX1_EPCS_SEL,
> + IMX8QM_MISC_PHYX1_EPCS_SEL);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_PCIE_AB_SELECT,
> + 0);
> + } else if (imx6_pcie->hsio_cfg == 2) {
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_PHYX2_OFFSET,
> + IMX8QM_PHYX2_CTRL0_APB_MASK,
> + IMX8QM_PHY_APB_RSTN_0 |
> + IMX8QM_PHY_APB_RSTN_1);
> +
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_PHYX1_EPCS_SEL,
> + IMX8QM_MISC_PHYX1_EPCS_SEL);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_PCIE_AB_SELECT,
> + IMX8QM_MISC_PCIE_AB_SELECT);
> + } else if (imx6_pcie->hsio_cfg == 3) {
> + if (imx6_pcie->controller_id)
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_PHYX1_OFFSET,
> + IMX8QM_PHY_APB_RSTN_0,
> + IMX8QM_PHY_APB_RSTN_0);
> + else
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_PHYX2_OFFSET,
> + IMX8QM_PHYX2_CTRL0_APB_MASK,
> + IMX8QM_PHY_APB_RSTN_0 |
> + IMX8QM_PHY_APB_RSTN_1);
> +
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_PHYX1_EPCS_SEL, 0);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_PCIE_AB_SELECT,
> + IMX8QM_MISC_PCIE_AB_SELECT);
> + }
> +
This doesn't look controller independent/local. What would happen if
controller 0 specifies hsio_cfg == 2 and controller 1 specifies
hsio_cfg == 1?
> + if (imx6_pcie->ext_osc) {
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_IOB_RXENA,
> + IMX8QM_MISC_IOB_RXENA);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_IOB_TXENA, 0);
> + } else {
> + /* Try to used the internal pll as ref clk */
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_IOB_RXENA, 0);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_MISC_IOB_TXENA,
> + IMX8QM_MISC_IOB_TXENA);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX8QM_CSR_MISC_OFFSET,
> + IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> + IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK,
> + IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> + IMX8QM_CSR_MISC_IOB_A_0_M1M0_2);
> + }
Same here. It looks like specifying "ext_osc" for one controller and
leaving it out for another would lead to different outcome based on
which controller gets initialized first.
It seems that maybe abstracting all of this away via a generic PHY
subsystem would be a better path. See for example pci-dra7xx.c which
looks like it might be a good example.
> +
> + break;
> case IMX8MQ:
> /*
> * TODO: Currently this code assumes external
> @@ -763,6 +1029,7 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
>
> static void imx6_pcie_ltssm_enable(struct device *dev)
> {
> + u32 val;
> struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
>
> switch (imx6_pcie->drvdata->variant) {
> @@ -777,6 +1044,15 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
> case IMX8MQ:
> reset_control_deassert(imx6_pcie->apps_reset);
> break;
> + case IMX8QXP:
> + case IMX8QM:
> + val = IMX8QM_CSR_PCIEA_OFFSET +
> + imx6_pcie->controller_id * SZ_64K;
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + val + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> + IMX8QM_CTRL_LTSSM_ENABLE,
> + IMX8QM_CTRL_LTSSM_ENABLE);
> + break;
> }
> }
>
> @@ -908,13 +1184,25 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
> return 0;
> }
>
> +static u64 imx6_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> +{
> + struct pcie_port *pp = &pcie->pp;
> + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pcie);
> +
> + if (imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP)
> + return (cpu_addr + imx6_pcie->local_addr - pp->mem_base);
If you do
cpu_addr += mx6_pcie->local_addr - pp->mem_base;
you won't need an else below.
> + else
> + return cpu_addr;
> +}
> +
> static const struct dw_pcie_ops dw_pcie_ops = {
> - /* No special ops needed, but pcie-designware still expects this struct */
> + .cpu_addr_fixup = imx6_pcie_cpu_addr_fixup,
> };
>
> #ifdef CONFIG_PM_SLEEP
> static void imx6_pcie_ltssm_disable(struct device *dev)
> {
> + u32 val;
> struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
>
> switch (imx6_pcie->drvdata->variant) {
> @@ -926,6 +1214,17 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
> case IMX7D:
> reset_control_assert(imx6_pcie->apps_reset);
> break;
> + case IMX8QXP:
> + case IMX8QM:
> + val = IMX8QM_CSR_PCIEA_OFFSET +
> + imx6_pcie->controller_id * SZ_64K;
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + val + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> + IMX8QM_CTRL_LTSSM_ENABLE, 0);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + val + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> + IMX8QM_CTRL_READY_ENTR_L23, 0);
> + break;
> default:
> dev_err(dev, "ltssm_disable not supported\n");
> }
> @@ -933,6 +1232,8 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
>
> static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> {
> + int i;
> + u32 addr, val;
> struct device *dev = imx6_pcie->pci->dev;
>
> /* Some variants have a turnoff reset in DT */
> @@ -951,6 +1252,34 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0);
> break;
> + case IMX8QXP:
> + case IMX8QM:
> + addr = IMX8QM_CSR_PCIEA_OFFSET +
> + imx6_pcie->controller_id * SZ_64K;
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + addr + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> + IMX8QM_CTRL_PM_XMT_TURNOFF,
> + IMX8QM_CTRL_PM_XMT_TURNOFF);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + addr + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> + IMX8QM_CTRL_PM_XMT_TURNOFF, 0);
Is setting IMX8QM_CTRL_PM_XMT_TURNOFF on and then off necessary? I'd
add a comment to highlight that this is intentional.
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + addr + IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> + IMX8QM_CTRL_READY_ENTR_L23,
> + IMX8QM_CTRL_READY_ENTR_L23);
> + /* check the L2 is entered or not. */
> + for (i = 0; i < L2_ENTRY_WAIT_MAX_RETRIES; i++) {
> + regmap_read(imx6_pcie->iomuxc_gpr,
> + addr + IMX8QM_CSR_PCIE_STTS0_OFFSET,
> + &val);
> + if (val & IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2)
> + break;
> + udelay(10);
> + }
> + if ((val & IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2) == 0)
> + dev_err(dev, "PCIE%d can't enter into L2.\n",
> + imx6_pcie->controller_id);
regmap_read_poll_timeout()
> + break;
> default:
> dev_err(dev, "PME_Turn_Off not implemented\n");
> return;
> @@ -985,6 +1314,11 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> case IMX8MQ:
> clk_disable_unprepare(imx6_pcie->pcie_aux);
> break;
> + case IMX8QXP:
> + case IMX8QM:
> + clk_disable_unprepare(imx6_pcie->pcie_per);
> + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
You can probably piggy back on IMX6SX since it has "pcie_inbound_axi" as well.
> + break;
> default:
> break;
> }
> @@ -1084,7 +1418,26 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> if (IS_ERR(pci->dbi_base))
> return PTR_ERR(pci->dbi_base);
>
> + if (of_property_read_u32(node, "hsio-cfg", &imx6_pcie->hsio_cfg))
> + imx6_pcie->hsio_cfg = 0;
> + if (of_property_read_u32(node, "ext_osc", &imx6_pcie->ext_osc) < 0)
> + imx6_pcie->ext_osc = 0;
> + if (of_property_read_u32(node, "local-addr", &imx6_pcie->local_addr))
> + imx6_pcie->local_addr = 0;
All of these properties will be initialized to zero by kzalloc and
of_property_read_u32() won't modify output variable unless it is
successful, so you can probably skip error checking.
> +
> /* Fetch GPIOs */
> + imx6_pcie->clkreq_gpio = of_get_named_gpio(node, "clkreq-gpio", 0);
> + if (gpio_is_valid(imx6_pcie->clkreq_gpio)) {
> + ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->clkreq_gpio,
> + GPIOF_OUT_INIT_LOW, "PCIe CLKREQ");
> + if (ret) {
> + dev_err(&pdev->dev, "unable to get clkreq gpio\n");
> + return ret;
> + }
> + } else if (imx6_pcie->clkreq_gpio == -EPROBE_DEFER) {
> + return imx6_pcie->clkreq_gpio;
> + }
> +
> imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio", 0);
> imx6_pcie->gpio_active_high = of_property_read_bool(node,
> "reset-gpio-active-high");
> @@ -1155,6 +1508,25 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> return PTR_ERR(imx6_pcie->pcie_aux);
> }
> break;
> + case IMX8QM:
> + case IMX8QXP:
> + if (dbi_base->start == IMX8_HSIO_PCIEB_BASE_ADDR)
> + imx6_pcie->controller_id = 1;
> +
> + imx6_pcie->pcie_per = devm_clk_get(dev, "pcie_per");
> + if (IS_ERR(imx6_pcie->pcie_per)) {
> + dev_err(dev, "pcie_per clock source missing or invalid\n");
> + return PTR_ERR(imx6_pcie->pcie_per);
> + }
> +
> + imx6_pcie->pcie_inbound_axi = devm_clk_get(&pdev->dev,
> + "pcie_inbound_axi");
> + if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
> + dev_err(&pdev->dev,
> + "pcie clock source missing or invalid\n");
> + return PTR_ERR(imx6_pcie->pcie_inbound_axi);
> + }
On i.MX8MQ "pcie_bus" clock in vendor tree wasn't actually pointing to
actual PCIE bus clock, so it might be worth checking if that's the
case for i.MX8QM/X and you actually need one more clock.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
2019-03-13 20:20 ` Andrey Smirnov
@ 2019-03-14 9:18 ` Richard Zhu
2019-03-15 2:18 ` Andrey Smirnov
0 siblings, 1 reply; 10+ messages in thread
From: Richard Zhu @ 2019-03-14 9:18 UTC (permalink / raw)
To: Andrey Smirnov
Cc: bhelgaas, lorenzo.pieralisi, l.stach, linux-pci,
linux-arm-kernel, linux-kernel
Hi Andrey:
Thanks a lot for your review comments.
Best Regards
Richard Zhu
Office: 86-21-28937189
Mobile: 86-13386059786
> -----Original Message-----
> From: Andrey Smirnov [mailto:andrew.smirnov@gmail.com]
> Sent: 2019年3月14日 4:20
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com;
> l.stach@pengutronix.de; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
>
> On Wed, Mar 13, 2019 at 2:15 AM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > Add codes needed to support i.MX8QM/QXP PCIe.
> > - HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
> > The PCIe and SATA modules are contained in the HSIO subsystem. There
> > are two PCIe, one SATA controllers and three mixed lane PHYs on
> > i.MX8QM. There are three use cases of the HSIO subsystem on
> i.MX8QM.
> > 1. PCIea 2 lanes and one SATA AHCI port.
> > 2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
> > 3. PCIea 2 lanes, PCIeb 1 lane.
> > i.MX8QXP only has PCIeb controller and one lane PHY.
> > - The HSIO address map as viewed from system level is as shown below.
> > address [31:24] Local address Target Address Size
> > 5F 0 HSIO 16MB
> > 60-6F 40-4F HSIO 256MB
> > 70-7F 80-8F HSIO 256MB
> > So, the cpu_addr_fixup is required to enable i.MX8QM/QXP PCIe.
> > - Both external OSC and internal PLL can be used as PCIe reference
> > clock.
> > - clock request GPIO for controlling the PCI reference clock request
> > signal. And should be configure OD when L1SS maybe enabled later.
> > - One more power domain HSIO_GPIO and clock PCIE_PER are required by
> > i.MX8QM/QXP PCIe.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 392
> > +++++++++++++++++++++++++++++++++-
> > 1 file changed, 387 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index aaa9489..aacefb6 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -39,6 +39,7 @@
> > #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> > #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11,
> 8)
> > #define IMX8MQ_PCIE2_BASE_ADDR 0x33c00000
> > +#define IMX8_HSIO_PCIEB_BASE_ADDR 0x5f010000
> >
> > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
> >
> > @@ -48,10 +49,13 @@ enum imx6_pcie_variants {
> > IMX6QP,
> > IMX7D,
> > IMX8MQ,
> > + IMX8QM,
> > + IMX8QXP,
> > };
> >
> > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> > +#define IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP BIT(2)
>
> This is an IMX8Q* specific flag, so it probably should be called something like
> IMX6_PCIE_FLAG_IMX8Qx_CPU_ADD_FIXUP.
[Richard Zhu] Okay, would change it later.
>
> >
> > struct imx6_pcie_drvdata {
> > enum imx6_pcie_variants variant; @@ -60,10 +64,12 @@ struct
> > imx6_pcie_drvdata {
> >
> > struct imx6_pcie {
> > struct dw_pcie *pci;
> > + int clkreq_gpio;
>
> Is this really necessary? On i.MX8MQ vendor tree for some unknown reason
> would reconfigure a dedicated CLKREQ_B signal as a GPIO and then use it as
> CLKREQ signal that way instead of controlling it via dedicated bits in register
> file, so I am wondering if that is the case with QM and QXP.
[Richard Zhu] There is a same mechanism of the CLKREQ on iMX8QM/QXP/MQ.
Up to now, this pin is configured as GPIO, because that this pin would be pull up when OD is set
and the EP device doesn't support the L1SS at all.
Thus, the external CLK would be turned off in this scenario.
This pin would be used in OD(Open Drain) mode when L1SS is enabled.
The L1SS has been verified on iMX8MQ. But I don't have a dynamic method to
turn the L1SS feature on at RC side yet when the L1SS is supported by EP.
Configure CLK_REQ as GPIO here currently, and hope to figure out one solution in future.
>
> > int reset_gpio;
> > bool gpio_active_high;
> > struct clk *pcie_bus;
> > struct clk *pcie_phy;
> > + struct clk *pcie_per;
> > struct clk *pcie_inbound_axi;
> > struct clk *pcie;
> > struct clk *pcie_aux;
> > @@ -77,6 +83,9 @@ struct imx6_pcie {
> > u32 tx_deemph_gen2_6db;
> > u32 tx_swing_full;
> > u32 tx_swing_low;
> > + u32 hsio_cfg;
> > + u32 ext_osc;
> > + u32 local_addr;
> > int link_gen;
> > struct regulator *vpcie;
> > void __iomem *phy_base;
> > @@ -85,6 +94,8 @@ struct imx6_pcie {
> > struct device *pd_pcie;
> > /* power domain for pcie phy */
> > struct device *pd_pcie_phy;
> > + /* power domain for hsio gpio used by pcie */
> > + struct device *pd_hsio_gpio;
> > const struct imx6_pcie_drvdata *drvdata; };
> >
> > @@ -92,6 +103,7 @@ struct imx6_pcie {
> > #define PHY_PLL_LOCK_WAIT_MAX_RETRIES 2000
> > #define PHY_PLL_LOCK_WAIT_USLEEP_MIN 50
> > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200
> > +#define L2_ENTRY_WAIT_MAX_RETRIES 10000
> >
> > /* PCIe Root Complex registers (memory-mapped) */
> > #define PCIE_RC_IMX6_MSI_CAP 0x50
> > @@ -157,6 +169,43 @@ struct imx6_pcie { #define
> > PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5) #define
> > PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
> >
> > +/* iMX8 HSIO registers */
> > +#define IMX8QM_CSR_PHYX2_OFFSET
> 0x00000
> > +#define IMX8QM_CSR_PHYX1_OFFSET
> 0x10000
> > +#define IMX8QM_CSR_PHYX_STTS0_OFFSET 0x4
> > +#define IMX8QM_CSR_PCIEA_OFFSET
> 0x20000
> > +#define IMX8QM_CSR_PCIEB_OFFSET
> 0x30000
> > +#define IMX8QM_CSR_PCIE_CTRL1_OFFSET 0x4
> > +#define IMX8QM_CSR_PCIE_CTRL2_OFFSET 0x8
> > +#define IMX8QM_CSR_PCIE_STTS0_OFFSET 0xC
> > +#define IMX8QM_CSR_MISC_OFFSET 0x50000
> > +
> > +#define IMX8QM_CTRL_LTSSM_ENABLE BIT(4)
> > +#define IMX8QM_CTRL_READY_ENTR_L23 BIT(5)
> > +#define IMX8QM_CTRL_PM_XMT_TURNOFF BIT(9)
> > +#define IMX8QM_CTRL_BUTTON_RST_N BIT(21)
> > +#define IMX8QM_CTRL_PERST_N BIT(22)
> > +#define IMX8QM_CTRL_POWER_UP_RST_N BIT(23)
> > +
> > +#define IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2 BIT(13)
> > +#define IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST BIT(19)
> > +#define IMX8QM_STTS0_LANE0_TX_PLL_LOCK BIT(4)
> > +#define IMX8QM_STTS0_LANE1_TX_PLL_LOCK BIT(12)
> > +
> > +#define IMX8QM_PCIE_TYPE_MASK GENMASK(27,
> 24)
> > +
> > +#define IMX8QM_PHYX2_CTRL0_APB_MASK GENMASK(1,
> 0)
> > +#define IMX8QM_PHY_APB_RSTN_0 BIT(0)
> > +#define IMX8QM_PHY_APB_RSTN_1 BIT(1)
> > +
> > +#define IMX8QM_MISC_IOB_RXENA BIT(0)
> > +#define IMX8QM_MISC_IOB_TXENA BIT(1)
> > +#define IMX8QM_CSR_MISC_IOB_A_0_TXOE BIT(2)
> > +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK GENMASK(4,
> 3)
> > +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_2 BIT(4)
> > +#define IMX8QM_MISC_PHYX1_EPCS_SEL BIT(12)
> > +#define IMX8QM_MISC_PCIE_AB_SELECT BIT(13)
> > +
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int
> > exp_val) {
> > struct dw_pcie *pci = imx6_pcie->pci; @@ -373,14 +422,65 @@
> > static int imx6_pcie_attach_pd(struct device *dev)
> > return PTR_ERR(link);
> > }
> >
> > + switch (imx6_pcie->drvdata->variant) {
> > + case IMX8QM:
> > + case IMX8QXP:
> > + imx6_pcie->pd_hsio_gpio =
> dev_pm_domain_attach_by_name(dev,
> > + "hsio_gpio");
> > + if (IS_ERR(imx6_pcie->pd_hsio_gpio))
> > + return PTR_ERR(imx6_pcie->pd_hsio_gpio);
> > +
> > + link = device_link_add(dev, imx6_pcie->pd_hsio_gpio,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > + if (!link) {
> > + dev_err(dev, "Failed to add device_link to gpio
> pd.\n");
> > + return -EINVAL;
> > + }
> > +
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > return 0;
> > }
> >
> > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > {
> > + u32 addr;
> > + int i;
> > struct device *dev = imx6_pcie->pci->dev;
> >
> > switch (imx6_pcie->drvdata->variant) {
> > + case IMX8QXP:
> > + addr = IMX8QM_CSR_PCIEB_OFFSET +
> > + IMX8QM_CSR_PCIE_CTRL2_OFFSET;
>
> This and similar "IMX8QM_CSR_PCIEA_OFFSET + i * SZ_64K" pattern keeps
> popping up quite frequently in the code. I think at the very least it would be
> good to calculate this offset in probe and store it as a member of struct
> imx6_pcie. However I do wonder if this should actually be handle by either
> declaring an additional syscon regmap of additional reg/reg-name property.
>
[Richard Zhu] IMHO, I just reduce some more MACRO definitions in the codes.
Actually, the "IMX8QM_CSR_PCIEA_OFFSET + SZ_64K" should be the IMX8QM_CSR_PCIEB_OFFSET.
I can add some more macro-definitions, for example " IMX8QM_CSR_PCIEB_OFFSET " to remove the calculations later.
How do you think about that?
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > + IMX8QM_CTRL_BUTTON_RST_N,
> > + IMX8QM_CTRL_BUTTON_RST_N);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > + IMX8QM_CTRL_PERST_N,
> > + IMX8QM_CTRL_PERST_N);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > +
> IMX8QM_CTRL_POWER_UP_RST_N,
> > +
> IMX8QM_CTRL_POWER_UP_RST_N);
> > + break;
> > + case IMX8QM:
> > + for (i = 0; i <= imx6_pcie->controller_id; i++) {
>
> This loop is a bit surprising to me. It's hard to tell why you'd iterate from 0 to
> controller_id. I think it'd be good to add a comment explaining the logic
> behind this code.
>
[Richard Zhu] Okay, comments would be added here.
/*
* In i.MX8QM, two lanes PHY and one lane PHY share the
* same calibration signal. And one lane PHY would use
* the calibration output from two lanes PHY. So PCIeA
* related resets are configured before configurating PCIeB.
*/
>
> > + addr = IMX8QM_CSR_PCIEA_OFFSET + i *
> SZ_64K;
> > + addr += IMX8QM_CSR_PCIE_CTRL2_OFFSET;
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> addr,
> > +
> IMX8QM_CTRL_BUTTON_RST_N,
> > +
> IMX8QM_CTRL_BUTTON_RST_N);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> addr,
> > +
> IMX8QM_CTRL_PERST_N,
> > +
> IMX8QM_CTRL_PERST_N);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> addr,
> > +
> IMX8QM_CTRL_POWER_UP_RST_N,
> > +
> IMX8QM_CTRL_POWER_UP_RST_N);
> > + }
> > + break;
> > case IMX7D:
> > case IMX8MQ:
> > reset_control_assert(imx6_pcie->pciephy_reset);
> > @@ -477,6 +577,21 @@ static int imx6_pcie_enable_ref_clk(struct
> imx6_pcie *imx6_pcie)
> >
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> >
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
> > break;
> > + case IMX8QXP:
> > + case IMX8QM:
> > + ret =
> clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_axi
> clock\n");
> > + break;
> > + }
> > + ret = clk_prepare_enable(imx6_pcie->pcie_per);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_per
> clock\n");
> > +
> clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > + break;
> > + }
> > +
> > + break;
> > }
> >
> > return ret;
> > @@ -501,6 +616,63 @@ static void
> imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> > dev_err(dev, "PCIe PLL lock timeout\n"); }
> >
> > +static int imx8_hsio_pcie_wait_for_phy_pll_lock(struct imx6_pcie
> > +*imx6_pcie) {
> > + u32 retries, addr, val, lock = 0;
> > + int ret;
> > + struct dw_pcie *pci = imx6_pcie->pci;
> > + struct device *dev = pci->dev;
> > +
> > + addr = IMX8QM_CSR_PCIEA_OFFSET + imx6_pcie->controller_id
> * SZ_64K;
> > + addr += IMX8QM_CSR_PCIE_STTS0_OFFSET;
> > + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES;
> retries++) {
> > + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> > + if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST)
> == 0)
> > + break;
> > + udelay(1);
> > + }
> > +
> > + if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST) != 0) {
> > + dev_err(dev, "ERROR: PM_REQ_CORE_RST is still
> set.\n");
> > + return -ENODEV;
> > + }
> > +
>
> You can really cut down on boilerplate here by using
> regmap_read_poll_timeout()
>
[Richard Zhu] Okay, would change it later.
> > + addr = IMX8QM_CSR_PHYX2_OFFSET + imx6_pcie->controller_id
> * SZ_64K;
> > + addr += IMX8QM_CSR_PHYX_STTS0_OFFSET;
> > + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES;
> retries++) {
> > + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> > + if (imx6_pcie->hsio_cfg == 2) {
> > + if (imx6_pcie->controller_id == 0)
> > + lock =
> IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > + else
> > + lock =
> IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > + } else if (imx6_pcie->hsio_cfg == 3) {
> > + lock =
> IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > + if (imx6_pcie->controller_id == 0)
> > + lock |=
> IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > + } else if (imx6_pcie->hsio_cfg == 1) {
> > + lock =
> IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > + lock |=
> IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > + } else {
> > + dev_err(dev, "ERROR: illegal hsio_cfg
> value.\n");
> > + return -EINVAL;
> > + }
> > + val &= lock;
> > + if (val == lock)
> > + break;
> > + udelay(10);
> > + }
> > +
> > + if (retries >= PHY_PLL_LOCK_WAIT_MAX_RETRIES) {
> > + dev_info(dev, "pcie phy pll can't be locked.\n");
> > + ret = -ENODEV;
> > + } else {
> > + dev_info(dev, "pcie phy pll is locked.\n");
> > + }
> > +
>
> Ditto.
[Richard Zhu] Got that. Thanks.
>
> > + return ret;
> > +}
> > +
> > static void imx6_pcie_deassert_core_reset(struct imx6_pcie
> > *imx6_pcie) {
> > struct dw_pcie *pci = imx6_pcie->pci; @@ -553,6 +725,11 @@
> > static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > }
> >
> > switch (imx6_pcie->drvdata->variant) {
> > + case IMX8QXP:
> > + case IMX8QM:
> > + /* wait for phy pll lock firstly. */
> > + imx8_hsio_pcie_wait_for_phy_pll_lock(imx6_pcie);
> > + break;
> > case IMX8MQ:
> > reset_control_deassert(imx6_pcie->pciephy_reset);
> > break;
> > @@ -613,25 +790,114 @@ static void
> > imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> >
> > static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie) {
> > - unsigned int mask, val;
> > -
> > - if (imx6_pcie->drvdata->variant == IMX8MQ &&
> > + unsigned int offset, mask, val;
> > +
> > + if (imx6_pcie->drvdata->variant == IMX8QM ||
> > + imx6_pcie->drvdata->variant == IMX8QXP) {
>
> Since there's now more than two possibilities, it'd probably make sense to
> convert this code to use switch statement.
[Richard Zhu] Okay, would use switch statement later.
>
> > + offset = IMX8QM_CSR_PCIEA_OFFSET +
> > + imx6_pcie->controller_id * SZ_64K;
> > + mask = IMX8QM_PCIE_TYPE_MASK;
> > + val = FIELD_PREP(IMX8QM_PCIE_TYPE_MASK,
> > + PCI_EXP_TYPE_ROOT_PORT);
> > + } else if (imx6_pcie->drvdata->variant == IMX8MQ &&
> > imx6_pcie->controller_id == 1) {
> > + offset = IOMUXC_GPR12;
> > mask =
> IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> > val =
> FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> > PCI_EXP_TYPE_ROOT_PORT);
> > } else {
> > + offset = IOMUXC_GPR12;
> > mask = IMX6Q_GPR12_DEVICE_TYPE;
> > val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
> > PCI_EXP_TYPE_ROOT_PORT);
> > }
> >
> > - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> mask, val);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, offset, mask, val);
> > }
> >
> > static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie) {
> > switch (imx6_pcie->drvdata->variant) {
> > + case IMX8QXP:
> > + case IMX8QM:
> > + if (imx6_pcie->hsio_cfg == 1) {
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_PHYX2_OFFSET,
> > +
> IMX8QM_PHYX2_CTRL0_APB_MASK,
> > + IMX8QM_PHY_APB_RSTN_0 |
> > + IMX8QM_PHY_APB_RSTN_1);
> > +
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_PHYX1_EPCS_SEL,
> > + IMX8QM_MISC_PHYX1_EPCS_SEL);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_PCIE_AB_SELECT,
> > + 0);
> > + } else if (imx6_pcie->hsio_cfg == 2) {
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_PHYX2_OFFSET,
> > +
> IMX8QM_PHYX2_CTRL0_APB_MASK,
> > + IMX8QM_PHY_APB_RSTN_0 |
> > + IMX8QM_PHY_APB_RSTN_1);
> > +
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_PHYX1_EPCS_SEL,
> > + IMX8QM_MISC_PHYX1_EPCS_SEL);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_PCIE_AB_SELECT,
> > + IMX8QM_MISC_PCIE_AB_SELECT);
> > + } else if (imx6_pcie->hsio_cfg == 3) {
> > + if (imx6_pcie->controller_id)
> > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > +
> IMX8QM_CSR_PHYX1_OFFSET,
> > +
> IMX8QM_PHY_APB_RSTN_0,
> > +
> IMX8QM_PHY_APB_RSTN_0);
> > + else
> > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > +
> IMX8QM_CSR_PHYX2_OFFSET,
> > +
> IMX8QM_PHYX2_CTRL0_APB_MASK,
> > +
> IMX8QM_PHY_APB_RSTN_0 |
> > +
> IMX8QM_PHY_APB_RSTN_1);
> > +
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_PHYX1_EPCS_SEL,
> 0);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_PCIE_AB_SELECT,
> > + IMX8QM_MISC_PCIE_AB_SELECT);
> > + }
> > +
>
> This doesn't look controller independent/local. What would happen if
> controller 0 specifies hsio_cfg == 2 and controller 1 specifies hsio_cfg == 1?
>
[Richard Zhu]Yes, it is. There are usage dependences between PCIeA/PCIeB and SATA in HSIO subsystem.
BTW, It's impossible for controller 1 to specify the hsio_cfg to "1", when controller 0 specifies hsio_cfg==2.
There are three usage cases of the HSIO.
Hsio_cfg pciea pcieb sata
1 2lanes No Enabled
2 1lane 1lane Enabled
3 2lanes 1lane No
So, the possible hsio_cfg values for PCIeB is 2 or 3.
> > + if (imx6_pcie->ext_osc) {
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_IOB_RXENA,
> > + IMX8QM_MISC_IOB_RXENA);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_IOB_TXENA, 0);
> > + } else {
> > + /* Try to used the internal pll as ref clk */
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_IOB_RXENA, 0);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > + IMX8QM_MISC_IOB_TXENA,
> > + IMX8QM_MISC_IOB_TXENA);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + IMX8QM_CSR_MISC_OFFSET,
> > +
> IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> > +
> IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK,
> > +
> IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> > +
> IMX8QM_CSR_MISC_IOB_A_0_M1M0_2);
> > + }
>
> Same here. It looks like specifying "ext_osc" for one controller and leaving it
> out for another would lead to different outcome based on which controller
> gets initialized first.
>
> It seems that maybe abstracting all of this away via a generic PHY subsystem
> would be a better path. See for example pci-dra7xx.c which looks like it might
> be a good example.
>
[Richard Zhu] Okay, would following your suggestions.
Thanks a lot.
> > +
> > + break;
> > case IMX8MQ:
> > /*
> > * TODO: Currently this code assumes external @@
> > -763,6 +1029,7 @@ static int imx6_pcie_wait_for_speed_change(struct
> > imx6_pcie *imx6_pcie)
> >
> > static void imx6_pcie_ltssm_enable(struct device *dev) {
> > + u32 val;
> > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> >
> > switch (imx6_pcie->drvdata->variant) { @@ -777,6 +1044,15
> @@
> > static void imx6_pcie_ltssm_enable(struct device *dev)
> > case IMX8MQ:
> > reset_control_deassert(imx6_pcie->apps_reset);
> > break;
> > + case IMX8QXP:
> > + case IMX8QM:
> > + val = IMX8QM_CSR_PCIEA_OFFSET +
> > + imx6_pcie->controller_id * SZ_64K;
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + val +
> IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > + IMX8QM_CTRL_LTSSM_ENABLE,
> > + IMX8QM_CTRL_LTSSM_ENABLE);
> > + break;
> > }
> > }
> >
> > @@ -908,13 +1184,25 @@ static int imx6_add_pcie_port(struct imx6_pcie
> *imx6_pcie,
> > return 0;
> > }
> >
> > +static u64 imx6_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64
> > +cpu_addr) {
> > + struct pcie_port *pp = &pcie->pp;
> > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pcie);
> > +
> > + if (imx6_pcie->drvdata->flags &
> IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP)
> > + return (cpu_addr + imx6_pcie->local_addr -
> > + pp->mem_base);
>
> If you do
>
> cpu_addr += mx6_pcie->local_addr - pp->mem_base;
>
> you won't need an else below.
>
[Richard Zhu] You're right. Thanks.
> > + else
> > + return cpu_addr;
> > +}
> > +
> > static const struct dw_pcie_ops dw_pcie_ops = {
> > - /* No special ops needed, but pcie-designware still expects this
> struct */
> > + .cpu_addr_fixup = imx6_pcie_cpu_addr_fixup,
> > };
> >
> > #ifdef CONFIG_PM_SLEEP
> > static void imx6_pcie_ltssm_disable(struct device *dev) {
> > + u32 val;
> > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> >
> > switch (imx6_pcie->drvdata->variant) { @@ -926,6 +1214,17
> @@
> > static void imx6_pcie_ltssm_disable(struct device *dev)
> > case IMX7D:
> > reset_control_assert(imx6_pcie->apps_reset);
> > break;
> > + case IMX8QXP:
> > + case IMX8QM:
> > + val = IMX8QM_CSR_PCIEA_OFFSET +
> > + imx6_pcie->controller_id * SZ_64K;
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + val +
> IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > + IMX8QM_CTRL_LTSSM_ENABLE, 0);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + val +
> IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > + IMX8QM_CTRL_READY_ENTR_L23,
> 0);
> > + break;
> > default:
> > dev_err(dev, "ltssm_disable not supported\n");
> > }
> > @@ -933,6 +1232,8 @@ static void imx6_pcie_ltssm_disable(struct device
> > *dev)
> >
> > static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie) {
> > + int i;
> > + u32 addr, val;
> > struct device *dev = imx6_pcie->pci->dev;
> >
> > /* Some variants have a turnoff reset in DT */ @@ -951,6
> > +1252,34 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie
> *imx6_pcie)
> > regmap_update_bits(imx6_pcie->iomuxc_gpr,
> IOMUXC_GPR12,
> >
> IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0);
> > break;
> > + case IMX8QXP:
> > + case IMX8QM:
> > + addr = IMX8QM_CSR_PCIEA_OFFSET +
> > + imx6_pcie->controller_id * SZ_64K;
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + addr +
> IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > +
> IMX8QM_CTRL_PM_XMT_TURNOFF,
> > +
> IMX8QM_CTRL_PM_XMT_TURNOFF);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + addr +
> IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > +
> IMX8QM_CTRL_PM_XMT_TURNOFF, 0);
>
> Is setting IMX8QM_CTRL_PM_XMT_TURNOFF on and then off necessary? I'd
> add a comment to highlight that this is intentional.
>
[Richard Zhu] Designer suggest to do so. One PME message would be kicked off on the link after these turn on/off operations.
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > + addr +
> IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > + IMX8QM_CTRL_READY_ENTR_L23,
> > +
> IMX8QM_CTRL_READY_ENTR_L23);
> > + /* check the L2 is entered or not. */
> > + for (i = 0; i < L2_ENTRY_WAIT_MAX_RETRIES; i++) {
> > + regmap_read(imx6_pcie->iomuxc_gpr,
> > + addr +
> IMX8QM_CSR_PCIE_STTS0_OFFSET,
> > + &val);
> > + if (val &
> IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2)
> > + break;
> > + udelay(10);
> > + }
> > + if ((val & IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2) ==
> 0)
> > + dev_err(dev, "PCIE%d can't enter into L2.\n",
> > +
> imx6_pcie->controller_id);
>
> regmap_read_poll_timeout()
>
[Richard Zhu] Okay, would use regmap_read_poll_timeout() in next version.
> > + break;
> > default:
> > dev_err(dev, "PME_Turn_Off not implemented\n");
> > return;
> > @@ -985,6 +1314,11 @@ static void imx6_pcie_clk_disable(struct
> imx6_pcie *imx6_pcie)
> > case IMX8MQ:
> > clk_disable_unprepare(imx6_pcie->pcie_aux);
> > break;
> > + case IMX8QXP:
> > + case IMX8QM:
> > + clk_disable_unprepare(imx6_pcie->pcie_per);
> > + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
>
> You can probably piggy back on IMX6SX since it has "pcie_inbound_axi" as
> well.
>
[Richard Zhu] Okay, would follow your suggestion. Thanks.
Would change like below.
case IMX8QXP:
case IMX8QM:
clk_disable_unprepare(imx6_pcie->pcie_per);
case IMX6SX:
clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
break;
> > + break;
> > default:
> > break;
> > }
> > @@ -1084,7 +1418,26 @@ static int imx6_pcie_probe(struct
> platform_device *pdev)
> > if (IS_ERR(pci->dbi_base))
> > return PTR_ERR(pci->dbi_base);
> >
> > + if (of_property_read_u32(node, "hsio-cfg",
> &imx6_pcie->hsio_cfg))
> > + imx6_pcie->hsio_cfg = 0;
> > + if (of_property_read_u32(node, "ext_osc", &imx6_pcie->ext_osc)
> < 0)
> > + imx6_pcie->ext_osc = 0;
> > + if (of_property_read_u32(node, "local-addr",
> &imx6_pcie->local_addr))
> > + imx6_pcie->local_addr = 0;
>
> All of these properties will be initialized to zero by kzalloc and
> of_property_read_u32() won't modify output variable unless it is successful,
> so you can probably skip error checking.
[Richard Zhu] Okay, would remove the error checking.
>
> > +
> > /* Fetch GPIOs */
> > + imx6_pcie->clkreq_gpio = of_get_named_gpio(node, "clkreq-gpio",
> 0);
> > + if (gpio_is_valid(imx6_pcie->clkreq_gpio)) {
> > + ret = devm_gpio_request_one(&pdev->dev,
> imx6_pcie->clkreq_gpio,
> > +
> GPIOF_OUT_INIT_LOW, "PCIe CLKREQ");
> > + if (ret) {
> > + dev_err(&pdev->dev, "unable to get clkreq
> gpio\n");
> > + return ret;
> > + }
> > + } else if (imx6_pcie->clkreq_gpio == -EPROBE_DEFER) {
> > + return imx6_pcie->clkreq_gpio;
> > + }
> > +
> > imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio",
> 0);
> > imx6_pcie->gpio_active_high = of_property_read_bool(node,
> >
> > "reset-gpio-active-high"); @@ -1155,6 +1508,25 @@ static int
> imx6_pcie_probe(struct platform_device *pdev)
> > return PTR_ERR(imx6_pcie->pcie_aux);
> > }
> > break;
> > + case IMX8QM:
> > + case IMX8QXP:
> > + if (dbi_base->start == IMX8_HSIO_PCIEB_BASE_ADDR)
> > + imx6_pcie->controller_id = 1;
> > +
> > + imx6_pcie->pcie_per = devm_clk_get(dev, "pcie_per");
> > + if (IS_ERR(imx6_pcie->pcie_per)) {
> > + dev_err(dev, "pcie_per clock source missing
> or invalid\n");
> > + return PTR_ERR(imx6_pcie->pcie_per);
> > + }
> > +
> > + imx6_pcie->pcie_inbound_axi =
> devm_clk_get(&pdev->dev,
> > + "pcie_inbound_axi");
> > + if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
> > + dev_err(&pdev->dev,
> > + "pcie clock source missing or
> invalid\n");
> > + return
> PTR_ERR(imx6_pcie->pcie_inbound_axi);
> > + }
>
> On i.MX8MQ "pcie_bus" clock in vendor tree wasn't actually pointing to
> actual PCIE bus clock, so it might be worth checking if that's the case for
> i.MX8QM/X and you actually need one more clock.
[Richard Zhu] Regarding to my understanding, iMX PCIe module is connected to AXI bus.
Thus, the AXI related clock can be treated as bus clock. Correct me if my understand is wrong.
So, I use the pcie_bus clock for i.MX8QM/QXP PCIe in the dts binding.
Otherwise, I can use another new clock in codes to support i.MX8QM/QXP PCIes.
Thanks a lot for your kindly review.
>
> Thanks,
> Andrey Smirnov
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe
2019-03-13 9:15 [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe Richard Zhu
2019-03-13 9:15 ` [RFC 2/2] PCI: imx6: " Richard Zhu
@ 2019-03-14 9:30 ` Lucas Stach
2019-03-15 1:25 ` Richard Zhu
2019-03-15 2:26 ` Andrey Smirnov
2 siblings, 1 reply; 10+ messages in thread
From: Lucas Stach @ 2019-03-14 9:30 UTC (permalink / raw)
To: Richard Zhu, bhelgaas, lorenzo.pieralisi, andrew.smirnov
Cc: linux-pci, linux-arm-kernel, linux-kernel
Hi Richard,
Am Mittwoch, den 13.03.2019, 09:15 +0000 schrieb Richard Zhu:
> Add codes needed to support i.MX8QM/QXP PCIe.
> - HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
> The PCIe and SATA modules are contained in the HSIO subsystem. There
> are two PCIe, one SATA controllers and three mixed lane PHYs on
> i.MX8QM. There are three use cases of the HSIO subsystem on i.MX8QM.
> 1. PCIea 2 lanes and one SATA AHCI port.
> 2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
> 3. PCIea 2 lanes, PCIeb 1 lane.
> i.MX8QXP only has PCIeb controller and one lane PHY.
> Use the hsio-cfg property to specify the different modes.
> - The HSIO address map as viewed from system level is as shown below.
> address [31:24] Local address Target Address Size
> 5F 0 HSIO 16MB
> 60-6F 40-4F HSIO 256MB
> 70-7F 80-8F HSIO 256MB
> The property local-addr is required to specify it.
> - Both external OSC and internal PLL can be used as PCIe reference
> clock, use the ext_osc property to distinguish them.
> - clock request GPIO for controlling the PCI reference clock request
> signal. And should be configure OD when L1SS maybe enabled later.
> - One more power domain HSIO_GPIO and clock PCIE_PER are required by
> i.MX8QM/QXP PCIe.
> Add these specific properties to enable i.MX8QM/QXP PCIe.
All this HSIO handling should move into a separate PHY driver. This has
nothing to do with the PCIe controller itself. See for example the
Tegra XUSB PHY driver (drivers/phy/tegra/xusb.c), which is a similar
combined PCIE/USB3/SATA PHY driver.
Regards,
Lucas
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index a7f5f5a..f7586c9 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -10,6 +10,8 @@ Required properties:
> > - "fsl,imx6qp-pcie"
> > - "fsl,imx7d-pcie"
> > - "fsl,imx8mq-pcie"
> > + - "fsl,imx8qm-pcie"
> > + - "fsl,imx8qxp-pcie"
> - reg: base address and length of the PCIe controller
> - interrupts: A list of interrupt outputs of the controller. Must contain an
> entry for each entry in the interrupt-names property.
> @@ -38,6 +40,10 @@ Optional properties:
> The regulator will be enabled when initializing the PCIe host and
> disabled either as part of the init process or when shutting down the
> host.
> +- clkreq-gpio: Should specify the GPIO for controlling the PCI reference clock
> + request signal.
> +- ext_osc: External OSC is used as PCIe reference clock or not. 0: Internal
> + PLL. 1: External OSC.
>
> Additional required properties for imx6sx-pcie:
> - clock names: Must include the following additional entries:
> @@ -60,6 +66,21 @@ Additional required properties for imx8mq-pcie:
> - clock-names: Must include the following additional entries:
> > - "pcie_aux"
>
> +Additional required properties for imx8qm/qxp pcie:
> +- power-domains: Must be set to a phandle pointing to PCIE, PCIE_PHY power and
> + HSIO_GPIO domains
> +- power-domain-names: Must be "pcie", "pcie_phy", "hsio_gpio"
> +- clock-names: Must include the following additional entries:
> > + - "pcie_per"
> +- hsio-cfg: hsio configration mode when the pcie node is supported.
> + 1: pciea 2 lanes and one sata ahci port.
> + 2: pciea 1 lane, pcieb 1 lane and one sata ahci port.
> + 3: pciea 2 lanes, pcieb 1 lane.
> +- local-addr: the local address used in hsio module on i.MX8QM/QXP.
> > + Example:
> > + hsio-cfg = <2>;
> > + local-addr = <0x80000000>;
> +
> Example:
>
> > pcie@01000000 {
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe
2019-03-14 9:30 ` [RFC 1/2] dt-bindings: imx6q-pcie: " Lucas Stach
@ 2019-03-15 1:25 ` Richard Zhu
0 siblings, 0 replies; 10+ messages in thread
From: Richard Zhu @ 2019-03-15 1:25 UTC (permalink / raw)
To: Lucas Stach, bhelgaas, lorenzo.pieralisi, andrew.smirnov
Cc: linux-pci, linux-arm-kernel, linux-kernel
Hi Lucas:
Thanks for your reivew.
> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: 2019年3月14日 17:31
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; andrew.smirnov@gmail.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP
> PCIe
>
> Hi Richard,
>
> Am Mittwoch, den 13.03.2019, 09:15 +0000 schrieb Richard Zhu:
> > Add codes needed to support i.MX8QM/QXP PCIe.
> > - HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
> > The PCIe and SATA modules are contained in the HSIO subsystem. There
> > are two PCIe, one SATA controllers and three mixed lane PHYs on
> > i.MX8QM. There are three use cases of the HSIO subsystem on
> i.MX8QM.
> > 1. PCIea 2 lanes and one SATA AHCI port.
> > 2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
> > 3. PCIea 2 lanes, PCIeb 1 lane.
> > i.MX8QXP only has PCIeb controller and one lane PHY.
> > Use the hsio-cfg property to specify the different modes.
> > - The HSIO address map as viewed from system level is as shown below.
> > address [31:24] Local address Target Address Size
> > 5F 0 HSIO 16
> MB
> > 60-6F 40-4F HSIO 256MB
> > 70-7F 80-8F HSIO 256MB
> > The property local-addr is required to specify it.
> > - Both external OSC and internal PLL can be used as PCIe reference
> > clock, use the ext_osc property to distinguish them.
> > - clock request GPIO for controlling the PCI reference clock request
> > signal. And should be configure OD when L1SS maybe enabled later.
> > - One more power domain HSIO_GPIO and clock PCIE_PER are required by
> > i.MX8QM/QXP PCIe.
> > Add these specific properties to enable i.MX8QM/QXP PCIe.
>
> All this HSIO handling should move into a separate PHY driver. This has
> nothing to do with the PCIe controller itself. See for example the Tegra XUSB
> PHY driver (drivers/phy/tegra/xusb.c), which is a similar combined
> PCIE/USB3/SATA PHY driver.
>
[Richard Zhu] Very good suggestion, would make reference to it and re-orange the codes.
Thanks.
> Regards,
> Lucas
>
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 21
> > +++++++++++++++++++++
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > index a7f5f5a..f7586c9 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > @@ -10,6 +10,8 @@ Required properties:
> > > - "fsl,imx6qp-pcie"
> > > - "fsl,imx7d-pcie"
> > > - "fsl,imx8mq-pcie"
> > > + - "fsl,imx8qm-pcie"
> > > + - "fsl,imx8qxp-pcie"
> > - reg: base address and length of the PCIe controller
> > - interrupts: A list of interrupt outputs of the controller. Must
> > contain an
> > entry for each entry in the interrupt-names property.
> > @@ -38,6 +40,10 @@ Optional properties:
> > The regulator will be enabled when initializing the PCIe host and
> > disabled either as part of the init process or when shutting down
> > the
> > host.
> > +- clkreq-gpio: Should specify the GPIO for controlling the PCI
> > +reference clock
> > + request signal.
> > +- ext_osc: External OSC is used as PCIe reference clock or not. 0:
> > +Internal
> > + PLL. 1: External OSC.
> >
> > Additional required properties for imx6sx-pcie:
> > - clock names: Must include the following additional entries:
> > @@ -60,6 +66,21 @@ Additional required properties for imx8mq-pcie:
> > - clock-names: Must include the following additional entries:
> > > - "pcie_aux"
> >
> > +Additional required properties for imx8qm/qxp pcie:
> > +- power-domains: Must be set to a phandle pointing to PCIE, PCIE_PHY
> > +power and
> > + HSIO_GPIO domains
> > +- power-domain-names: Must be "pcie", "pcie_phy", "hsio_gpio"
> > +- clock-names: Must include the following additional entries:
> > > + - "pcie_per"
> > +- hsio-cfg: hsio configration mode when the pcie node is supported.
> > + 1: pciea 2 lanes and one sata ahci port.
> > + 2: pciea 1 lane, pcieb 1 lane and one sata ahci port.
> > + 3: pciea 2 lanes, pcieb 1 lane.
> > +- local-addr: the local address used in hsio module on i.MX8QM/QXP.
> > > + Example:
> > > + hsio-cfg = <2>;
> > > + local-addr = <0x80000000>;
> > +
> > Example:
> >
> > > pcie@01000000 {
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
2019-03-14 9:18 ` Richard Zhu
@ 2019-03-15 2:18 ` Andrey Smirnov
2019-03-15 6:04 ` Richard Zhu
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Smirnov @ 2019-03-15 2:18 UTC (permalink / raw)
To: Richard Zhu
Cc: bhelgaas, lorenzo.pieralisi, l.stach, linux-pci,
linux-arm-kernel, linux-kernel
On Thu, Mar 14, 2019 at 2:18 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> Hi Andrey:
> Thanks a lot for your review comments.
>
> Best Regards
> Richard Zhu
> Office: 86-21-28937189
> Mobile: 86-13386059786
>
>
> > -----Original Message-----
> > From: Andrey Smirnov [mailto:andrew.smirnov@gmail.com]
> > Sent: 2019年3月14日 4:20
> > To: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com;
> > l.stach@pengutronix.de; linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
> >
> > On Wed, Mar 13, 2019 at 2:15 AM Richard Zhu <hongxing.zhu@nxp.com>
> > wrote:
> > >
> > > Add codes needed to support i.MX8QM/QXP PCIe.
> > > - HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
> > > The PCIe and SATA modules are contained in the HSIO subsystem. There
> > > are two PCIe, one SATA controllers and three mixed lane PHYs on
> > > i.MX8QM. There are three use cases of the HSIO subsystem on
> > i.MX8QM.
> > > 1. PCIea 2 lanes and one SATA AHCI port.
> > > 2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
> > > 3. PCIea 2 lanes, PCIeb 1 lane.
> > > i.MX8QXP only has PCIeb controller and one lane PHY.
> > > - The HSIO address map as viewed from system level is as shown below.
> > > address [31:24] Local address Target Address Size
> > > 5F 0 HSIO 16MB
> > > 60-6F 40-4F HSIO 256MB
> > > 70-7F 80-8F HSIO 256MB
> > > So, the cpu_addr_fixup is required to enable i.MX8QM/QXP PCIe.
> > > - Both external OSC and internal PLL can be used as PCIe reference
> > > clock.
> > > - clock request GPIO for controlling the PCI reference clock request
> > > signal. And should be configure OD when L1SS maybe enabled later.
> > > - One more power domain HSIO_GPIO and clock PCIE_PER are required by
> > > i.MX8QM/QXP PCIe.
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > > drivers/pci/controller/dwc/pci-imx6.c | 392
> > > +++++++++++++++++++++++++++++++++-
> > > 1 file changed, 387 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index aaa9489..aacefb6 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -39,6 +39,7 @@
> > > #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> > > #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11,
> > 8)
> > > #define IMX8MQ_PCIE2_BASE_ADDR 0x33c00000
> > > +#define IMX8_HSIO_PCIEB_BASE_ADDR 0x5f010000
> > >
> > > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
> > >
> > > @@ -48,10 +49,13 @@ enum imx6_pcie_variants {
> > > IMX6QP,
> > > IMX7D,
> > > IMX8MQ,
> > > + IMX8QM,
> > > + IMX8QXP,
> > > };
> > >
> > > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> > > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> > > +#define IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP BIT(2)
> >
> > This is an IMX8Q* specific flag, so it probably should be called something like
> > IMX6_PCIE_FLAG_IMX8Qx_CPU_ADD_FIXUP.
> [Richard Zhu] Okay, would change it later.
> >
> > >
> > > struct imx6_pcie_drvdata {
> > > enum imx6_pcie_variants variant; @@ -60,10 +64,12 @@ struct
> > > imx6_pcie_drvdata {
> > >
> > > struct imx6_pcie {
> > > struct dw_pcie *pci;
> > > + int clkreq_gpio;
> >
> > Is this really necessary? On i.MX8MQ vendor tree for some unknown reason
> > would reconfigure a dedicated CLKREQ_B signal as a GPIO and then use it as
> > CLKREQ signal that way instead of controlling it via dedicated bits in register
> > file, so I am wondering if that is the case with QM and QXP.
> [Richard Zhu] There is a same mechanism of the CLKREQ on iMX8QM/QXP/MQ.
> Up to now, this pin is configured as GPIO, because that this pin would be pull up when OD is set
> and the EP device doesn't support the L1SS at all.
> Thus, the external CLK would be turned off in this scenario.
> This pin would be used in OD(Open Drain) mode when L1SS is enabled.
> The L1SS has been verified on iMX8MQ. But I don't have a dynamic method to
> turn the L1SS feature on at RC side yet when the L1SS is supported by EP.
> Configure CLK_REQ as GPIO here currently, and hope to figure out one solution in future.
>
Hmm, I am afraid I still don't understand why that pin has to be
controlled via GPIO subsystem. Here are my assumptions:
1. We can configure, say, PCIE0's CLKREQ as
SC_P_PCIE_CTRL0_CLKREQ_B_HSIO_PCIE0_CLKREQ_B on i.MX8QM, which should
be open drain just by CLKREQ's definition, and we can, if need be,
configure internal pull up in the same pinmux entry
2. It is possible to driver that pin open/closed via some bits in
register file. IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE in case of i.MX8MQ,
maybe something else for other i.MX8 SoC
given those assumptions, why would we need to introduce a new DT
binding to specify a GPIO and then control it via gpio_set_value()?
AFAICT, absence or presence of L1SS support should be irrelevant.
Perhaps some of my assumptions is wrong? Or maybe your use-case does
use a dedicated GPIO pin that can't be configure as CLKREQ_B via
pinmux?
> >
> > > int reset_gpio;
> > > bool gpio_active_high;
> > > struct clk *pcie_bus;
> > > struct clk *pcie_phy;
> > > + struct clk *pcie_per;
> > > struct clk *pcie_inbound_axi;
> > > struct clk *pcie;
> > > struct clk *pcie_aux;
> > > @@ -77,6 +83,9 @@ struct imx6_pcie {
> > > u32 tx_deemph_gen2_6db;
> > > u32 tx_swing_full;
> > > u32 tx_swing_low;
> > > + u32 hsio_cfg;
> > > + u32 ext_osc;
> > > + u32 local_addr;
> > > int link_gen;
> > > struct regulator *vpcie;
> > > void __iomem *phy_base;
> > > @@ -85,6 +94,8 @@ struct imx6_pcie {
> > > struct device *pd_pcie;
> > > /* power domain for pcie phy */
> > > struct device *pd_pcie_phy;
> > > + /* power domain for hsio gpio used by pcie */
> > > + struct device *pd_hsio_gpio;
> > > const struct imx6_pcie_drvdata *drvdata; };
> > >
> > > @@ -92,6 +103,7 @@ struct imx6_pcie {
> > > #define PHY_PLL_LOCK_WAIT_MAX_RETRIES 2000
> > > #define PHY_PLL_LOCK_WAIT_USLEEP_MIN 50
> > > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200
> > > +#define L2_ENTRY_WAIT_MAX_RETRIES 10000
> > >
> > > /* PCIe Root Complex registers (memory-mapped) */
> > > #define PCIE_RC_IMX6_MSI_CAP 0x50
> > > @@ -157,6 +169,43 @@ struct imx6_pcie { #define
> > > PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5) #define
> > > PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
> > >
> > > +/* iMX8 HSIO registers */
> > > +#define IMX8QM_CSR_PHYX2_OFFSET
> > 0x00000
> > > +#define IMX8QM_CSR_PHYX1_OFFSET
> > 0x10000
> > > +#define IMX8QM_CSR_PHYX_STTS0_OFFSET 0x4
> > > +#define IMX8QM_CSR_PCIEA_OFFSET
> > 0x20000
> > > +#define IMX8QM_CSR_PCIEB_OFFSET
> > 0x30000
> > > +#define IMX8QM_CSR_PCIE_CTRL1_OFFSET 0x4
> > > +#define IMX8QM_CSR_PCIE_CTRL2_OFFSET 0x8
> > > +#define IMX8QM_CSR_PCIE_STTS0_OFFSET 0xC
> > > +#define IMX8QM_CSR_MISC_OFFSET 0x50000
> > > +
> > > +#define IMX8QM_CTRL_LTSSM_ENABLE BIT(4)
> > > +#define IMX8QM_CTRL_READY_ENTR_L23 BIT(5)
> > > +#define IMX8QM_CTRL_PM_XMT_TURNOFF BIT(9)
> > > +#define IMX8QM_CTRL_BUTTON_RST_N BIT(21)
> > > +#define IMX8QM_CTRL_PERST_N BIT(22)
> > > +#define IMX8QM_CTRL_POWER_UP_RST_N BIT(23)
> > > +
> > > +#define IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2 BIT(13)
> > > +#define IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST BIT(19)
> > > +#define IMX8QM_STTS0_LANE0_TX_PLL_LOCK BIT(4)
> > > +#define IMX8QM_STTS0_LANE1_TX_PLL_LOCK BIT(12)
> > > +
> > > +#define IMX8QM_PCIE_TYPE_MASK GENMASK(27,
> > 24)
> > > +
> > > +#define IMX8QM_PHYX2_CTRL0_APB_MASK GENMASK(1,
> > 0)
> > > +#define IMX8QM_PHY_APB_RSTN_0 BIT(0)
> > > +#define IMX8QM_PHY_APB_RSTN_1 BIT(1)
> > > +
> > > +#define IMX8QM_MISC_IOB_RXENA BIT(0)
> > > +#define IMX8QM_MISC_IOB_TXENA BIT(1)
> > > +#define IMX8QM_CSR_MISC_IOB_A_0_TXOE BIT(2)
> > > +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK GENMASK(4,
> > 3)
> > > +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_2 BIT(4)
> > > +#define IMX8QM_MISC_PHYX1_EPCS_SEL BIT(12)
> > > +#define IMX8QM_MISC_PCIE_AB_SELECT BIT(13)
> > > +
> > > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int
> > > exp_val) {
> > > struct dw_pcie *pci = imx6_pcie->pci; @@ -373,14 +422,65 @@
> > > static int imx6_pcie_attach_pd(struct device *dev)
> > > return PTR_ERR(link);
> > > }
> > >
> > > + switch (imx6_pcie->drvdata->variant) {
> > > + case IMX8QM:
> > > + case IMX8QXP:
> > > + imx6_pcie->pd_hsio_gpio =
> > dev_pm_domain_attach_by_name(dev,
> > > + "hsio_gpio");
> > > + if (IS_ERR(imx6_pcie->pd_hsio_gpio))
> > > + return PTR_ERR(imx6_pcie->pd_hsio_gpio);
> > > +
> > > + link = device_link_add(dev, imx6_pcie->pd_hsio_gpio,
> > > + DL_FLAG_STATELESS |
> > > + DL_FLAG_PM_RUNTIME |
> > > + DL_FLAG_RPM_ACTIVE);
> > > + if (!link) {
> > > + dev_err(dev, "Failed to add device_link to gpio
> > pd.\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > > {
> > > + u32 addr;
> > > + int i;
> > > struct device *dev = imx6_pcie->pci->dev;
> > >
> > > switch (imx6_pcie->drvdata->variant) {
> > > + case IMX8QXP:
> > > + addr = IMX8QM_CSR_PCIEB_OFFSET +
> > > + IMX8QM_CSR_PCIE_CTRL2_OFFSET;
> >
> > This and similar "IMX8QM_CSR_PCIEA_OFFSET + i * SZ_64K" pattern keeps
> > popping up quite frequently in the code. I think at the very least it would be
> > good to calculate this offset in probe and store it as a member of struct
> > imx6_pcie. However I do wonder if this should actually be handle by either
> > declaring an additional syscon regmap of additional reg/reg-name property.
> >
> [Richard Zhu] IMHO, I just reduce some more MACRO definitions in the codes.
> Actually, the "IMX8QM_CSR_PCIEA_OFFSET + SZ_64K" should be the IMX8QM_CSR_PCIEB_OFFSET.
> I can add some more macro-definitions, for example " IMX8QM_CSR_PCIEB_OFFSET " to remove the calculations later.
> How do you think about that?
>
It's ultimately up to you. I was just pointing out that the code keeps
re-calculating the same thing again and again, so it might have
benefited from caching that value.
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > > + IMX8QM_CTRL_BUTTON_RST_N,
> > > + IMX8QM_CTRL_BUTTON_RST_N);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > > + IMX8QM_CTRL_PERST_N,
> > > + IMX8QM_CTRL_PERST_N);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > > +
> > IMX8QM_CTRL_POWER_UP_RST_N,
> > > +
> > IMX8QM_CTRL_POWER_UP_RST_N);
> > > + break;
> > > + case IMX8QM:
> > > + for (i = 0; i <= imx6_pcie->controller_id; i++) {
> >
> > This loop is a bit surprising to me. It's hard to tell why you'd iterate from 0 to
> > controller_id. I think it'd be good to add a comment explaining the logic
> > behind this code.
> >
> [Richard Zhu] Okay, comments would be added here.
> /*
> * In i.MX8QM, two lanes PHY and one lane PHY share the
> * same calibration signal. And one lane PHY would use
> * the calibration output from two lanes PHY. So PCIeA
> * related resets are configured before configurating PCIeB.
> */
It sounds like this code relies on the fact that PCIeA will be
initialized before PCIeB. I am not sure this can be guaranteed by this
driver, since it supports probe deferral and PCIeA's probing can be
delayed so that PCIeB's would happen first. Am I misinterpreting
this? If not maybe this should be moved out to PHY driver as well?
> >
> > > + addr = IMX8QM_CSR_PCIEA_OFFSET + i *
> > SZ_64K;
> > > + addr += IMX8QM_CSR_PCIE_CTRL2_OFFSET;
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > addr,
> > > +
> > IMX8QM_CTRL_BUTTON_RST_N,
> > > +
> > IMX8QM_CTRL_BUTTON_RST_N);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > addr,
> > > +
> > IMX8QM_CTRL_PERST_N,
> > > +
> > IMX8QM_CTRL_PERST_N);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > addr,
> > > +
> > IMX8QM_CTRL_POWER_UP_RST_N,
> > > +
> > IMX8QM_CTRL_POWER_UP_RST_N);
> > > + }
> > > + break;
> > > case IMX7D:
> > > case IMX8MQ:
> > > reset_control_assert(imx6_pcie->pciephy_reset);
> > > @@ -477,6 +577,21 @@ static int imx6_pcie_enable_ref_clk(struct
> > imx6_pcie *imx6_pcie)
> > >
> > IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> > >
> > IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
> > > break;
> > > + case IMX8QXP:
> > > + case IMX8QM:
> > > + ret =
> > clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> > > + if (ret) {
> > > + dev_err(dev, "unable to enable pcie_axi
> > clock\n");
> > > + break;
> > > + }
> > > + ret = clk_prepare_enable(imx6_pcie->pcie_per);
> > > + if (ret) {
> > > + dev_err(dev, "unable to enable pcie_per
> > clock\n");
> > > +
> > clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > > + break;
> > > + }
> > > +
> > > + break;
> > > }
> > >
> > > return ret;
> > > @@ -501,6 +616,63 @@ static void
> > imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> > > dev_err(dev, "PCIe PLL lock timeout\n"); }
> > >
> > > +static int imx8_hsio_pcie_wait_for_phy_pll_lock(struct imx6_pcie
> > > +*imx6_pcie) {
> > > + u32 retries, addr, val, lock = 0;
> > > + int ret;
> > > + struct dw_pcie *pci = imx6_pcie->pci;
> > > + struct device *dev = pci->dev;
> > > +
> > > + addr = IMX8QM_CSR_PCIEA_OFFSET + imx6_pcie->controller_id
> > * SZ_64K;
> > > + addr += IMX8QM_CSR_PCIE_STTS0_OFFSET;
> > > + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES;
> > retries++) {
> > > + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> > > + if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST)
> > == 0)
> > > + break;
> > > + udelay(1);
> > > + }
> > > +
> > > + if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST) != 0) {
> > > + dev_err(dev, "ERROR: PM_REQ_CORE_RST is still
> > set.\n");
> > > + return -ENODEV;
> > > + }
> > > +
> >
> > You can really cut down on boilerplate here by using
> > regmap_read_poll_timeout()
> >
> [Richard Zhu] Okay, would change it later.
>
> > > + addr = IMX8QM_CSR_PHYX2_OFFSET + imx6_pcie->controller_id
> > * SZ_64K;
> > > + addr += IMX8QM_CSR_PHYX_STTS0_OFFSET;
> > > + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES;
> > retries++) {
> > > + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> > > + if (imx6_pcie->hsio_cfg == 2) {
> > > + if (imx6_pcie->controller_id == 0)
> > > + lock =
> > IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > > + else
> > > + lock =
> > IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > > + } else if (imx6_pcie->hsio_cfg == 3) {
> > > + lock =
> > IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > > + if (imx6_pcie->controller_id == 0)
> > > + lock |=
> > IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > > + } else if (imx6_pcie->hsio_cfg == 1) {
> > > + lock =
> > IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > > + lock |=
> > IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > > + } else {
> > > + dev_err(dev, "ERROR: illegal hsio_cfg
> > value.\n");
> > > + return -EINVAL;
> > > + }
> > > + val &= lock;
> > > + if (val == lock)
> > > + break;
> > > + udelay(10);
> > > + }
> > > +
> > > + if (retries >= PHY_PLL_LOCK_WAIT_MAX_RETRIES) {
> > > + dev_info(dev, "pcie phy pll can't be locked.\n");
> > > + ret = -ENODEV;
> > > + } else {
> > > + dev_info(dev, "pcie phy pll is locked.\n");
> > > + }
> > > +
> >
> > Ditto.
> [Richard Zhu] Got that. Thanks.
> >
> > > + return ret;
> > > +}
> > > +
> > > static void imx6_pcie_deassert_core_reset(struct imx6_pcie
> > > *imx6_pcie) {
> > > struct dw_pcie *pci = imx6_pcie->pci; @@ -553,6 +725,11 @@
> > > static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > > }
> > >
> > > switch (imx6_pcie->drvdata->variant) {
> > > + case IMX8QXP:
> > > + case IMX8QM:
> > > + /* wait for phy pll lock firstly. */
> > > + imx8_hsio_pcie_wait_for_phy_pll_lock(imx6_pcie);
> > > + break;
> > > case IMX8MQ:
> > > reset_control_deassert(imx6_pcie->pciephy_reset);
> > > break;
> > > @@ -613,25 +790,114 @@ static void
> > > imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > >
> > > static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie) {
> > > - unsigned int mask, val;
> > > -
> > > - if (imx6_pcie->drvdata->variant == IMX8MQ &&
> > > + unsigned int offset, mask, val;
> > > +
> > > + if (imx6_pcie->drvdata->variant == IMX8QM ||
> > > + imx6_pcie->drvdata->variant == IMX8QXP) {
> >
> > Since there's now more than two possibilities, it'd probably make sense to
> > convert this code to use switch statement.
> [Richard Zhu] Okay, would use switch statement later.
>
> >
> > > + offset = IMX8QM_CSR_PCIEA_OFFSET +
> > > + imx6_pcie->controller_id * SZ_64K;
> > > + mask = IMX8QM_PCIE_TYPE_MASK;
> > > + val = FIELD_PREP(IMX8QM_PCIE_TYPE_MASK,
> > > + PCI_EXP_TYPE_ROOT_PORT);
> > > + } else if (imx6_pcie->drvdata->variant == IMX8MQ &&
> > > imx6_pcie->controller_id == 1) {
> > > + offset = IOMUXC_GPR12;
> > > mask =
> > IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> > > val =
> > FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> > > PCI_EXP_TYPE_ROOT_PORT);
> > > } else {
> > > + offset = IOMUXC_GPR12;
> > > mask = IMX6Q_GPR12_DEVICE_TYPE;
> > > val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
> > > PCI_EXP_TYPE_ROOT_PORT);
> > > }
> > >
> > > - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > mask, val);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, offset, mask, val);
> > > }
> > >
> > > static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie) {
> > > switch (imx6_pcie->drvdata->variant) {
> > > + case IMX8QXP:
> > > + case IMX8QM:
> > > + if (imx6_pcie->hsio_cfg == 1) {
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_PHYX2_OFFSET,
> > > +
> > IMX8QM_PHYX2_CTRL0_APB_MASK,
> > > + IMX8QM_PHY_APB_RSTN_0 |
> > > + IMX8QM_PHY_APB_RSTN_1);
> > > +
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_PHYX1_EPCS_SEL,
> > > + IMX8QM_MISC_PHYX1_EPCS_SEL);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_PCIE_AB_SELECT,
> > > + 0);
> > > + } else if (imx6_pcie->hsio_cfg == 2) {
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_PHYX2_OFFSET,
> > > +
> > IMX8QM_PHYX2_CTRL0_APB_MASK,
> > > + IMX8QM_PHY_APB_RSTN_0 |
> > > + IMX8QM_PHY_APB_RSTN_1);
> > > +
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_PHYX1_EPCS_SEL,
> > > + IMX8QM_MISC_PHYX1_EPCS_SEL);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_PCIE_AB_SELECT,
> > > + IMX8QM_MISC_PCIE_AB_SELECT);
> > > + } else if (imx6_pcie->hsio_cfg == 3) {
> > > + if (imx6_pcie->controller_id)
> > > +
> > regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > +
> > IMX8QM_CSR_PHYX1_OFFSET,
> > > +
> > IMX8QM_PHY_APB_RSTN_0,
> > > +
> > IMX8QM_PHY_APB_RSTN_0);
> > > + else
> > > +
> > regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > +
> > IMX8QM_CSR_PHYX2_OFFSET,
> > > +
> > IMX8QM_PHYX2_CTRL0_APB_MASK,
> > > +
> > IMX8QM_PHY_APB_RSTN_0 |
> > > +
> > IMX8QM_PHY_APB_RSTN_1);
> > > +
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_PHYX1_EPCS_SEL,
> > 0);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_PCIE_AB_SELECT,
> > > + IMX8QM_MISC_PCIE_AB_SELECT);
> > > + }
> > > +
> >
> > This doesn't look controller independent/local. What would happen if
> > controller 0 specifies hsio_cfg == 2 and controller 1 specifies hsio_cfg == 1?
> >
> [Richard Zhu]Yes, it is. There are usage dependences between PCIeA/PCIeB and SATA in HSIO subsystem.
> BTW, It's impossible for controller 1 to specify the hsio_cfg to "1", when controller 0 specifies hsio_cfg==2.
> There are three usage cases of the HSIO.
> Hsio_cfg pciea pcieb sata
> 1 2lanes No Enabled
> 2 1lane 1lane Enabled
> 3 2lanes 1lane No
> So, the possible hsio_cfg values for PCIeB is 2 or 3.
If I understand you correctly, I think what you mean by "impossible"
is that in order things to work correctly when first controller is
configured as hsio_cfg == 1, second contoller _has_ to be specified
with hsio_cfg != 1. However, what I am trying to point out is that it
is an implicit dependency between the two controllers and AFAICT
there's no enforcement of it preventing me/user from creating a DT
file where hsio_cfg = <1> for both controllers. Moving this
configuration into a separate PHY driver should solve this, however.
>
> > > + if (imx6_pcie->ext_osc) {
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_IOB_RXENA,
> > > + IMX8QM_MISC_IOB_RXENA);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_IOB_TXENA, 0);
> > > + } else {
> > > + /* Try to used the internal pll as ref clk */
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_IOB_RXENA, 0);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > + IMX8QM_MISC_IOB_TXENA,
> > > + IMX8QM_MISC_IOB_TXENA);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IMX8QM_CSR_MISC_OFFSET,
> > > +
> > IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> > > +
> > IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK,
> > > +
> > IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> > > +
> > IMX8QM_CSR_MISC_IOB_A_0_M1M0_2);
> > > + }
> >
> > Same here. It looks like specifying "ext_osc" for one controller and leaving it
> > out for another would lead to different outcome based on which controller
> > gets initialized first.
> >
> > It seems that maybe abstracting all of this away via a generic PHY subsystem
> > would be a better path. See for example pci-dra7xx.c which looks like it might
> > be a good example.
> >
> [Richard Zhu] Okay, would following your suggestions.
> Thanks a lot.
> > > +
> > > + break;
> > > case IMX8MQ:
> > > /*
> > > * TODO: Currently this code assumes external @@
> > > -763,6 +1029,7 @@ static int imx6_pcie_wait_for_speed_change(struct
> > > imx6_pcie *imx6_pcie)
> > >
> > > static void imx6_pcie_ltssm_enable(struct device *dev) {
> > > + u32 val;
> > > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > >
> > > switch (imx6_pcie->drvdata->variant) { @@ -777,6 +1044,15
> > @@
> > > static void imx6_pcie_ltssm_enable(struct device *dev)
> > > case IMX8MQ:
> > > reset_control_deassert(imx6_pcie->apps_reset);
> > > break;
> > > + case IMX8QXP:
> > > + case IMX8QM:
> > > + val = IMX8QM_CSR_PCIEA_OFFSET +
> > > + imx6_pcie->controller_id * SZ_64K;
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + val +
> > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > + IMX8QM_CTRL_LTSSM_ENABLE,
> > > + IMX8QM_CTRL_LTSSM_ENABLE);
> > > + break;
> > > }
> > > }
> > >
> > > @@ -908,13 +1184,25 @@ static int imx6_add_pcie_port(struct imx6_pcie
> > *imx6_pcie,
> > > return 0;
> > > }
> > >
> > > +static u64 imx6_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64
> > > +cpu_addr) {
> > > + struct pcie_port *pp = &pcie->pp;
> > > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pcie);
> > > +
> > > + if (imx6_pcie->drvdata->flags &
> > IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP)
> > > + return (cpu_addr + imx6_pcie->local_addr -
> > > + pp->mem_base);
> >
> > If you do
> >
> > cpu_addr += mx6_pcie->local_addr - pp->mem_base;
> >
> > you won't need an else below.
> >
> [Richard Zhu] You're right. Thanks.
>
> > > + else
> > > + return cpu_addr;
> > > +}
> > > +
> > > static const struct dw_pcie_ops dw_pcie_ops = {
> > > - /* No special ops needed, but pcie-designware still expects this
> > struct */
> > > + .cpu_addr_fixup = imx6_pcie_cpu_addr_fixup,
> > > };
> > >
> > > #ifdef CONFIG_PM_SLEEP
> > > static void imx6_pcie_ltssm_disable(struct device *dev) {
> > > + u32 val;
> > > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > >
> > > switch (imx6_pcie->drvdata->variant) { @@ -926,6 +1214,17
> > @@
> > > static void imx6_pcie_ltssm_disable(struct device *dev)
> > > case IMX7D:
> > > reset_control_assert(imx6_pcie->apps_reset);
> > > break;
> > > + case IMX8QXP:
> > > + case IMX8QM:
> > > + val = IMX8QM_CSR_PCIEA_OFFSET +
> > > + imx6_pcie->controller_id * SZ_64K;
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + val +
> > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > + IMX8QM_CTRL_LTSSM_ENABLE, 0);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + val +
> > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > + IMX8QM_CTRL_READY_ENTR_L23,
> > 0);
> > > + break;
> > > default:
> > > dev_err(dev, "ltssm_disable not supported\n");
> > > }
> > > @@ -933,6 +1232,8 @@ static void imx6_pcie_ltssm_disable(struct device
> > > *dev)
> > >
> > > static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie) {
> > > + int i;
> > > + u32 addr, val;
> > > struct device *dev = imx6_pcie->pci->dev;
> > >
> > > /* Some variants have a turnoff reset in DT */ @@ -951,6
> > > +1252,34 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie
> > *imx6_pcie)
> > > regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > IOMUXC_GPR12,
> > >
> > IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0);
> > > break;
> > > + case IMX8QXP:
> > > + case IMX8QM:
> > > + addr = IMX8QM_CSR_PCIEA_OFFSET +
> > > + imx6_pcie->controller_id * SZ_64K;
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + addr +
> > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > +
> > IMX8QM_CTRL_PM_XMT_TURNOFF,
> > > +
> > IMX8QM_CTRL_PM_XMT_TURNOFF);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + addr +
> > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > +
> > IMX8QM_CTRL_PM_XMT_TURNOFF, 0);
> >
> > Is setting IMX8QM_CTRL_PM_XMT_TURNOFF on and then off necessary? I'd
> > add a comment to highlight that this is intentional.
> >
> [Richard Zhu] Designer suggest to do so. One PME message would be kicked off on the link after these turn on/off operations.
>
Yup, good to hear, that's exactly what I'd put in the comment :-)
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + addr +
> > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > + IMX8QM_CTRL_READY_ENTR_L23,
> > > +
> > IMX8QM_CTRL_READY_ENTR_L23);
> > > + /* check the L2 is entered or not. */
> > > + for (i = 0; i < L2_ENTRY_WAIT_MAX_RETRIES; i++) {
> > > + regmap_read(imx6_pcie->iomuxc_gpr,
> > > + addr +
> > IMX8QM_CSR_PCIE_STTS0_OFFSET,
> > > + &val);
> > > + if (val &
> > IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2)
> > > + break;
> > > + udelay(10);
> > > + }
> > > + if ((val & IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2) ==
> > 0)
> > > + dev_err(dev, "PCIE%d can't enter into L2.\n",
> > > +
> > imx6_pcie->controller_id);
> >
> > regmap_read_poll_timeout()
> >
> [Richard Zhu] Okay, would use regmap_read_poll_timeout() in next version.
>
> > > + break;
> > > default:
> > > dev_err(dev, "PME_Turn_Off not implemented\n");
> > > return;
> > > @@ -985,6 +1314,11 @@ static void imx6_pcie_clk_disable(struct
> > imx6_pcie *imx6_pcie)
> > > case IMX8MQ:
> > > clk_disable_unprepare(imx6_pcie->pcie_aux);
> > > break;
> > > + case IMX8QXP:
> > > + case IMX8QM:
> > > + clk_disable_unprepare(imx6_pcie->pcie_per);
> > > + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> >
> > You can probably piggy back on IMX6SX since it has "pcie_inbound_axi" as
> > well.
> >
> [Richard Zhu] Okay, would follow your suggestion. Thanks.
> Would change like below.
> case IMX8QXP:
> case IMX8QM:
> clk_disable_unprepare(imx6_pcie->pcie_per);
> case IMX6SX:
> clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> break;
>
> > > + break;
> > > default:
> > > break;
> > > }
> > > @@ -1084,7 +1418,26 @@ static int imx6_pcie_probe(struct
> > platform_device *pdev)
> > > if (IS_ERR(pci->dbi_base))
> > > return PTR_ERR(pci->dbi_base);
> > >
> > > + if (of_property_read_u32(node, "hsio-cfg",
> > &imx6_pcie->hsio_cfg))
> > > + imx6_pcie->hsio_cfg = 0;
> > > + if (of_property_read_u32(node, "ext_osc", &imx6_pcie->ext_osc)
> > < 0)
> > > + imx6_pcie->ext_osc = 0;
> > > + if (of_property_read_u32(node, "local-addr",
> > &imx6_pcie->local_addr))
> > > + imx6_pcie->local_addr = 0;
> >
> > All of these properties will be initialized to zero by kzalloc and
> > of_property_read_u32() won't modify output variable unless it is successful,
> > so you can probably skip error checking.
> [Richard Zhu] Okay, would remove the error checking.
>
> >
> > > +
> > > /* Fetch GPIOs */
> > > + imx6_pcie->clkreq_gpio = of_get_named_gpio(node, "clkreq-gpio",
> > 0);
> > > + if (gpio_is_valid(imx6_pcie->clkreq_gpio)) {
> > > + ret = devm_gpio_request_one(&pdev->dev,
> > imx6_pcie->clkreq_gpio,
> > > +
> > GPIOF_OUT_INIT_LOW, "PCIe CLKREQ");
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "unable to get clkreq
> > gpio\n");
> > > + return ret;
> > > + }
> > > + } else if (imx6_pcie->clkreq_gpio == -EPROBE_DEFER) {
> > > + return imx6_pcie->clkreq_gpio;
> > > + }
> > > +
> > > imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio",
> > 0);
> > > imx6_pcie->gpio_active_high = of_property_read_bool(node,
> > >
> > > "reset-gpio-active-high"); @@ -1155,6 +1508,25 @@ static int
> > imx6_pcie_probe(struct platform_device *pdev)
> > > return PTR_ERR(imx6_pcie->pcie_aux);
> > > }
> > > break;
> > > + case IMX8QM:
> > > + case IMX8QXP:
> > > + if (dbi_base->start == IMX8_HSIO_PCIEB_BASE_ADDR)
> > > + imx6_pcie->controller_id = 1;
> > > +
> > > + imx6_pcie->pcie_per = devm_clk_get(dev, "pcie_per");
> > > + if (IS_ERR(imx6_pcie->pcie_per)) {
> > > + dev_err(dev, "pcie_per clock source missing
> > or invalid\n");
> > > + return PTR_ERR(imx6_pcie->pcie_per);
> > > + }
> > > +
> > > + imx6_pcie->pcie_inbound_axi =
> > devm_clk_get(&pdev->dev,
> > > + "pcie_inbound_axi");
> > > + if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
> > > + dev_err(&pdev->dev,
> > > + "pcie clock source missing or
> > invalid\n");
> > > + return
> > PTR_ERR(imx6_pcie->pcie_inbound_axi);
> > > + }
> >
> > On i.MX8MQ "pcie_bus" clock in vendor tree wasn't actually pointing to
> > actual PCIE bus clock, so it might be worth checking if that's the case for
> > i.MX8QM/X and you actually need one more clock.
> [Richard Zhu] Regarding to my understanding, iMX PCIe module is connected to AXI bus.
> Thus, the AXI related clock can be treated as bus clock. Correct me if my understand is wrong.
> So, I use the pcie_bus clock for i.MX8QM/QXP PCIe in the dts binding.
> Otherwise, I can use another new clock in codes to support i.MX8QM/QXP PCIes.
>
So, "pcie_bus" is supposed to be the clock driving PCIE bus itself. In
this case the clock that is controlled by CLKREQ_B. On i.MX8MQ EVK
that was an external 100 Mhz oscillator, so the final patch has
"pcie_bus" pointing to a dedicated "fixed-clock":
https://lore.kernel.org/lkml/20190220015857.7136-6-andrew.smirnov@gmail.com/T/#u
Originally vendor tree was using "pcie_bus" to point at
IMX8MQ_CLK_PCIE1_AUX. If the situation on i.MX8QM/QXP is similar,
then, yeah, I think it should be moved out into a separate clock.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe
2019-03-13 9:15 [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe Richard Zhu
2019-03-13 9:15 ` [RFC 2/2] PCI: imx6: " Richard Zhu
2019-03-14 9:30 ` [RFC 1/2] dt-bindings: imx6q-pcie: " Lucas Stach
@ 2019-03-15 2:26 ` Andrey Smirnov
2 siblings, 0 replies; 10+ messages in thread
From: Andrey Smirnov @ 2019-03-15 2:26 UTC (permalink / raw)
To: Richard Zhu
Cc: bhelgaas, lorenzo.pieralisi, l.stach, linux-pci,
linux-arm-kernel, linux-kernel
On Wed, Mar 13, 2019 at 2:15 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> Add codes needed to support i.MX8QM/QXP PCIe.
> - HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
> The PCIe and SATA modules are contained in the HSIO subsystem. There
> are two PCIe, one SATA controllers and three mixed lane PHYs on
> i.MX8QM. There are three use cases of the HSIO subsystem on i.MX8QM.
> 1. PCIea 2 lanes and one SATA AHCI port.
> 2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
> 3. PCIea 2 lanes, PCIeb 1 lane.
> i.MX8QXP only has PCIeb controller and one lane PHY.
> Use the hsio-cfg property to specify the different modes.
> - The HSIO address map as viewed from system level is as shown below.
> address [31:24] Local address Target Address Size
> 5F 0 HSIO 16MB
> 60-6F 40-4F HSIO 256MB
> 70-7F 80-8F HSIO 256MB
> The property local-addr is required to specify it.
> - Both external OSC and internal PLL can be used as PCIe reference
> clock, use the ext_osc property to distinguish them.
> - clock request GPIO for controlling the PCI reference clock request
> signal. And should be configure OD when L1SS maybe enabled later.
> - One more power domain HSIO_GPIO and clock PCIE_PER are required by
> i.MX8QM/QXP PCIe.
> Add these specific properties to enable i.MX8QM/QXP PCIe.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index a7f5f5a..f7586c9 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -10,6 +10,8 @@ Required properties:
> - "fsl,imx6qp-pcie"
> - "fsl,imx7d-pcie"
> - "fsl,imx8mq-pcie"
> + - "fsl,imx8qm-pcie"
> + - "fsl,imx8qxp-pcie"
> - reg: base address and length of the PCIe controller
> - interrupts: A list of interrupt outputs of the controller. Must contain an
> entry for each entry in the interrupt-names property.
> @@ -38,6 +40,10 @@ Optional properties:
> The regulator will be enabled when initializing the PCIe host and
> disabled either as part of the init process or when shutting down the
> host.
> +- clkreq-gpio: Should specify the GPIO for controlling the PCI reference clock
> + request signal.
> +- ext_osc: External OSC is used as PCIe reference clock or not. 0: Internal
> + PLL. 1: External OSC.
Forgot to mention one thing in my very first reply, so I'll mention it
here. I think figuring out the way to add support for external vs
internal reference bus clock (that "ext_osc" binding above) is going
to be a whole other discussion. It might be easier/more expedient to
focus on just one particular use-case first (say external oscillator
only), get it all done and accepted and then return to this particular
feature in a separate series/discussion. However, that's just how I'd
approach this, so, please don't feel compelled to do it that way just
because I suggested it.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
2019-03-15 2:18 ` Andrey Smirnov
@ 2019-03-15 6:04 ` Richard Zhu
2019-03-19 3:35 ` Andrey Smirnov
0 siblings, 1 reply; 10+ messages in thread
From: Richard Zhu @ 2019-03-15 6:04 UTC (permalink / raw)
To: Andrey Smirnov
Cc: bhelgaas, lorenzo.pieralisi, l.stach, linux-pci,
linux-arm-kernel, linux-kernel
Hi Andrey:
> -----Original Message-----
> From: Andrey Smirnov [mailto:andrew.smirnov@gmail.com]
> Sent: 2019年3月15日 10:18
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com;
> l.stach@pengutronix.de; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
>
> On Thu, Mar 14, 2019 at 2:18 AM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > Hi Andrey:
> > Thanks a lot for your review comments.
> >
> > Best Regards
> > Richard Zhu
> > Office: 86-21-28937189
> > Mobile: 86-13386059786
> >
> >
> > > -----Original Message-----
> > > From: Andrey Smirnov [mailto:andrew.smirnov@gmail.com]
> > > Sent: 2019年3月14日 4:20
> > > To: Richard Zhu <hongxing.zhu@nxp.com>
> > > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com;
> > > l.stach@pengutronix.de; linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
> > >
> > > On Wed, Mar 13, 2019 at 2:15 AM Richard Zhu <hongxing.zhu@nxp.com>
> > > wrote:
> > > >
> > > > Add codes needed to support i.MX8QM/QXP PCIe.
> > > > - HSIO(High Speed IO) subsystem is new defined on i.MX8QM/QXP.
> > > > The PCIe and SATA modules are contained in the HSIO subsystem.
> There
> > > > are two PCIe, one SATA controllers and three mixed lane PHYs on
> > > > i.MX8QM. There are three use cases of the HSIO subsystem on
> > > i.MX8QM.
> > > > 1. PCIea 2 lanes and one SATA AHCI port.
> > > > 2. PCIea 1 lane, PCIeb 1 lane and one SATA AHCI port.
> > > > 3. PCIea 2 lanes, PCIeb 1 lane.
> > > > i.MX8QXP only has PCIeb controller and one lane PHY.
> > > > - The HSIO address map as viewed from system level is as shown below.
> > > > address [31:24] Local address Target Address Size
> > > > 5F 0 HSIO 16MB
> > > > 60-6F 40-4F HSIO 256MB
> > > > 70-7F 80-8F HSIO 256MB
> > > > So, the cpu_addr_fixup is required to enable i.MX8QM/QXP PCIe.
> > > > - Both external OSC and internal PLL can be used as PCIe reference
> > > > clock.
> > > > - clock request GPIO for controlling the PCI reference clock request
> > > > signal. And should be configure OD when L1SS maybe enabled later.
> > > > - One more power domain HSIO_GPIO and clock PCIE_PER are required
> by
> > > > i.MX8QM/QXP PCIe.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > ---
> > > > drivers/pci/controller/dwc/pci-imx6.c | 392
> > > > +++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 387 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index aaa9489..aacefb6 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -39,6 +39,7 @@
> > > > #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> > > > #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE
> GENMASK(11,
> > > 8)
> > > > #define IMX8MQ_PCIE2_BASE_ADDR
> 0x33c00000
> > > > +#define IMX8_HSIO_PCIEB_BASE_ADDR 0x5f010000
> > > >
> > > > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
> > > >
> > > > @@ -48,10 +49,13 @@ enum imx6_pcie_variants {
> > > > IMX6QP,
> > > > IMX7D,
> > > > IMX8MQ,
> > > > + IMX8QM,
> > > > + IMX8QXP,
> > > > };
> > > >
> > > > #define IMX6_PCIE_FLAG_IMX6_PHY
> BIT(0)
> > > > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> > > > +#define IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP BIT(2)
> > >
> > > This is an IMX8Q* specific flag, so it probably should be called
> > > something like IMX6_PCIE_FLAG_IMX8Qx_CPU_ADD_FIXUP.
> > [Richard Zhu] Okay, would change it later.
> > >
> > > >
> > > > struct imx6_pcie_drvdata {
> > > > enum imx6_pcie_variants variant; @@ -60,10 +64,12 @@
> > > > struct imx6_pcie_drvdata {
> > > >
> > > > struct imx6_pcie {
> > > > struct dw_pcie *pci;
> > > > + int clkreq_gpio;
> > >
> > > Is this really necessary? On i.MX8MQ vendor tree for some unknown
> > > reason would reconfigure a dedicated CLKREQ_B signal as a GPIO and
> > > then use it as CLKREQ signal that way instead of controlling it via
> > > dedicated bits in register file, so I am wondering if that is the case with QM
> and QXP.
> > [Richard Zhu] There is a same mechanism of the CLKREQ on
> iMX8QM/QXP/MQ.
> > Up to now, this pin is configured as GPIO, because that this pin would
> > be pull up when OD is set and the EP device doesn't support the L1SS at all.
> > Thus, the external CLK would be turned off in this scenario.
> > This pin would be used in OD(Open Drain) mode when L1SS is enabled.
> > The L1SS has been verified on iMX8MQ. But I don't have a dynamic
> > method to turn the L1SS feature on at RC side yet when the L1SS is
> supported by EP.
> > Configure CLK_REQ as GPIO here currently, and hope to figure out one
> solution in future.
> >
>
> Hmm, I am afraid I still don't understand why that pin has to be controlled via
> GPIO subsystem. Here are my assumptions:
>
> 1. We can configure, say, PCIE0's CLKREQ as
> SC_P_PCIE_CTRL0_CLKREQ_B_HSIO_PCIE0_CLKREQ_B on i.MX8QM, which
> should be open drain just by CLKREQ's definition, and we can, if need be,
> configure internal pull up in the same pinmux entry
>
> 2. It is possible to driver that pin open/closed via some bits in register file.
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE in case of i.MX8MQ, maybe
> something else for other i.MX8 SoC
>
> given those assumptions, why would we need to introduce a new DT binding
> to specify a GPIO and then control it via gpio_set_value()?
> AFAICT, absence or presence of L1SS support should be irrelevant.
> Perhaps some of my assumptions is wrong? Or maybe your use-case does use
> a dedicated GPIO pin that can't be configure as CLKREQ_B via pinmux?
[Richard Zhu] You're right. The 8MQ's similar override mechanism to force the pin to low or high by register bit operations.
Otherwise used this pin as GPIO. Thus, these codes can be remove. Thanks for your comments here. 😊.
>
> > >
> > > > int reset_gpio;
> > > > bool gpio_active_high;
> > > > struct clk *pcie_bus;
> > > > struct clk *pcie_phy;
> > > > + struct clk *pcie_per;
> > > > struct clk *pcie_inbound_axi;
> > > > struct clk *pcie;
> > > > struct clk *pcie_aux;
> > > > @@ -77,6 +83,9 @@ struct imx6_pcie {
> > > > u32 tx_deemph_gen2_6db;
> > > > u32 tx_swing_full;
> > > > u32 tx_swing_low;
> > > > + u32 hsio_cfg;
> > > > + u32 ext_osc;
> > > > + u32 local_addr;
> > > > int link_gen;
> > > > struct regulator *vpcie;
> > > > void __iomem *phy_base;
> > > > @@ -85,6 +94,8 @@ struct imx6_pcie {
> > > > struct device *pd_pcie;
> > > > /* power domain for pcie phy */
> > > > struct device *pd_pcie_phy;
> > > > + /* power domain for hsio gpio used by pcie */
> > > > + struct device *pd_hsio_gpio;
> > > > const struct imx6_pcie_drvdata *drvdata; };
> > > >
> > > > @@ -92,6 +103,7 @@ struct imx6_pcie { #define
> > > > PHY_PLL_LOCK_WAIT_MAX_RETRIES 2000
> > > > #define PHY_PLL_LOCK_WAIT_USLEEP_MIN 50
> > > > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200
> > > > +#define L2_ENTRY_WAIT_MAX_RETRIES 10000
> > > >
> > > > /* PCIe Root Complex registers (memory-mapped) */
> > > > #define PCIE_RC_IMX6_MSI_CAP 0x50
> > > > @@ -157,6 +169,43 @@ struct imx6_pcie { #define
> > > > PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5) #define
> > > > PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
> > > >
> > > > +/* iMX8 HSIO registers */
> > > > +#define IMX8QM_CSR_PHYX2_OFFSET
> > > 0x00000
> > > > +#define IMX8QM_CSR_PHYX1_OFFSET
> > > 0x10000
> > > > +#define IMX8QM_CSR_PHYX_STTS0_OFFSET 0x4
> > > > +#define IMX8QM_CSR_PCIEA_OFFSET
> > > 0x20000
> > > > +#define IMX8QM_CSR_PCIEB_OFFSET
> > > 0x30000
> > > > +#define IMX8QM_CSR_PCIE_CTRL1_OFFSET 0x4
> > > > +#define IMX8QM_CSR_PCIE_CTRL2_OFFSET 0x8
> > > > +#define IMX8QM_CSR_PCIE_STTS0_OFFSET 0xC
> > > > +#define IMX8QM_CSR_MISC_OFFSET 0x50000
> > > > +
> > > > +#define IMX8QM_CTRL_LTSSM_ENABLE BIT(4)
> > > > +#define IMX8QM_CTRL_READY_ENTR_L23 BIT(5)
> > > > +#define IMX8QM_CTRL_PM_XMT_TURNOFF BIT(9)
> > > > +#define IMX8QM_CTRL_BUTTON_RST_N BIT(21)
> > > > +#define IMX8QM_CTRL_PERST_N BIT(22)
> > > > +#define IMX8QM_CTRL_POWER_UP_RST_N BIT(23)
> > > > +
> > > > +#define IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2 BIT(13)
> > > > +#define IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST BIT(19)
> > > > +#define IMX8QM_STTS0_LANE0_TX_PLL_LOCK BIT(4)
> > > > +#define IMX8QM_STTS0_LANE1_TX_PLL_LOCK BIT(12)
> > > > +
> > > > +#define IMX8QM_PCIE_TYPE_MASK
> GENMASK(27,
> > > 24)
> > > > +
> > > > +#define IMX8QM_PHYX2_CTRL0_APB_MASK
> GENMASK(1,
> > > 0)
> > > > +#define IMX8QM_PHY_APB_RSTN_0 BIT(0)
> > > > +#define IMX8QM_PHY_APB_RSTN_1 BIT(1)
> > > > +
> > > > +#define IMX8QM_MISC_IOB_RXENA BIT(0)
> > > > +#define IMX8QM_MISC_IOB_TXENA BIT(1)
> > > > +#define IMX8QM_CSR_MISC_IOB_A_0_TXOE BIT(2)
> > > > +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK
> GENMASK(4,
> > > 3)
> > > > +#define IMX8QM_CSR_MISC_IOB_A_0_M1M0_2 BIT(4)
> > > > +#define IMX8QM_MISC_PHYX1_EPCS_SEL BIT(12)
> > > > +#define IMX8QM_MISC_PCIE_AB_SELECT BIT(13)
> > > > +
> > > > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int
> > > > exp_val) {
> > > > struct dw_pcie *pci = imx6_pcie->pci; @@ -373,14 +422,65
> > > > @@ static int imx6_pcie_attach_pd(struct device *dev)
> > > > return PTR_ERR(link);
> > > > }
> > > >
> > > > + switch (imx6_pcie->drvdata->variant) {
> > > > + case IMX8QM:
> > > > + case IMX8QXP:
> > > > + imx6_pcie->pd_hsio_gpio =
> > > dev_pm_domain_attach_by_name(dev,
> > > > + "hsio_gpio");
> > > > + if (IS_ERR(imx6_pcie->pd_hsio_gpio))
> > > > + return
> PTR_ERR(imx6_pcie->pd_hsio_gpio);
> > > > +
> > > > + link = device_link_add(dev,
> imx6_pcie->pd_hsio_gpio,
> > > > + DL_FLAG_STATELESS |
> > > > + DL_FLAG_PM_RUNTIME |
> > > > + DL_FLAG_RPM_ACTIVE);
> > > > + if (!link) {
> > > > + dev_err(dev, "Failed to add device_link to
> > > > + gpio
> > > pd.\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > static void imx6_pcie_assert_core_reset(struct imx6_pcie
> > > > *imx6_pcie) {
> > > > + u32 addr;
> > > > + int i;
> > > > struct device *dev = imx6_pcie->pci->dev;
> > > >
> > > > switch (imx6_pcie->drvdata->variant) {
> > > > + case IMX8QXP:
> > > > + addr = IMX8QM_CSR_PCIEB_OFFSET +
> > > > + IMX8QM_CSR_PCIE_CTRL2_OFFSET;
> > >
> > > This and similar "IMX8QM_CSR_PCIEA_OFFSET + i * SZ_64K" pattern
> > > keeps popping up quite frequently in the code. I think at the very
> > > least it would be good to calculate this offset in probe and store
> > > it as a member of struct imx6_pcie. However I do wonder if this
> > > should actually be handle by either declaring an additional syscon regmap
> of additional reg/reg-name property.
> > >
> > [Richard Zhu] IMHO, I just reduce some more MACRO definitions in the
> codes.
> > Actually, the "IMX8QM_CSR_PCIEA_OFFSET + SZ_64K" should be the
> IMX8QM_CSR_PCIEB_OFFSET.
> > I can add some more macro-definitions, for example "
> IMX8QM_CSR_PCIEB_OFFSET " to remove the calculations later.
> > How do you think about that?
> >
>
> It's ultimately up to you. I was just pointing out that the code keeps
> re-calculating the same thing again and again, so it might have benefited from
> caching that value.
[Richard Zhu] Thanks, some extra MACRO-definitions would be added later.
>
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > > > +
> IMX8QM_CTRL_BUTTON_RST_N,
> > > > +
> IMX8QM_CTRL_BUTTON_RST_N);
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > > > + IMX8QM_CTRL_PERST_N,
> > > > + IMX8QM_CTRL_PERST_N);
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, addr,
> > > > +
> > > IMX8QM_CTRL_POWER_UP_RST_N,
> > > > +
> > > IMX8QM_CTRL_POWER_UP_RST_N);
> > > > + break;
> > > > + case IMX8QM:
> > > > + for (i = 0; i <= imx6_pcie->controller_id; i++) {
> > >
> > > This loop is a bit surprising to me. It's hard to tell why you'd
> > > iterate from 0 to controller_id. I think it'd be good to add a
> > > comment explaining the logic behind this code.
> > >
> > [Richard Zhu] Okay, comments would be added here.
> > /*
> > * In i.MX8QM, two lanes PHY and one lane PHY share the
> > * same calibration signal. And one lane PHY would use
> > * the calibration output from two lanes PHY. So PCIeA
> > * related resets are configured before configurating PCIeB.
> > */
>
> It sounds like this code relies on the fact that PCIeA will be initialized before
> PCIeB. I am not sure this can be guaranteed by this driver, since it supports
> probe deferral and PCIeA's probing can be delayed so that PCIeB's would
> happen first. Am I misinterpreting this? If not maybe this should be moved out
> to PHY driver as well?
>
[Richard Zhu] Yes, it is. These HSIO configuration codes should be moved to standalone PHY driver pointed by Lucas in another email.
> > >
> > > > + addr = IMX8QM_CSR_PCIEA_OFFSET + i *
> > > SZ_64K;
> > > > + addr +=
> IMX8QM_CSR_PCIE_CTRL2_OFFSET;
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > addr,
> > > > +
> > > IMX8QM_CTRL_BUTTON_RST_N,
> > > > +
> > > IMX8QM_CTRL_BUTTON_RST_N);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > addr,
> > > > +
> > > IMX8QM_CTRL_PERST_N,
> > > > +
> > > IMX8QM_CTRL_PERST_N);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > addr,
> > > > +
> > > IMX8QM_CTRL_POWER_UP_RST_N,
> > > > +
> > > IMX8QM_CTRL_POWER_UP_RST_N);
> > > > + }
> > > > + break;
> > > > case IMX7D:
> > > > case IMX8MQ:
> > > > reset_control_assert(imx6_pcie->pciephy_reset);
> > > > @@ -477,6 +577,21 @@ static int imx6_pcie_enable_ref_clk(struct
> > > imx6_pcie *imx6_pcie)
> > > >
> > > IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> > > >
> > > IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
> > > > break;
> > > > + case IMX8QXP:
> > > > + case IMX8QM:
> > > > + ret =
> > > clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> > > > + if (ret) {
> > > > + dev_err(dev, "unable to enable pcie_axi
> > > clock\n");
> > > > + break;
> > > > + }
> > > > + ret = clk_prepare_enable(imx6_pcie->pcie_per);
> > > > + if (ret) {
> > > > + dev_err(dev, "unable to enable pcie_per
> > > clock\n");
> > > > +
> > > clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > > > + break;
> > > > + }
> > > > +
> > > > + break;
> > > > }
> > > >
> > > > return ret;
> > > > @@ -501,6 +616,63 @@ static void
> > > imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> > > > dev_err(dev, "PCIe PLL lock timeout\n"); }
> > > >
> > > > +static int imx8_hsio_pcie_wait_for_phy_pll_lock(struct imx6_pcie
> > > > +*imx6_pcie) {
> > > > + u32 retries, addr, val, lock = 0;
> > > > + int ret;
> > > > + struct dw_pcie *pci = imx6_pcie->pci;
> > > > + struct device *dev = pci->dev;
> > > > +
> > > > + addr = IMX8QM_CSR_PCIEA_OFFSET +
> imx6_pcie->controller_id
> > > * SZ_64K;
> > > > + addr += IMX8QM_CSR_PCIE_STTS0_OFFSET;
> > > > + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES;
> > > retries++) {
> > > > + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> > > > + if ((val &
> IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST)
> > > == 0)
> > > > + break;
> > > > + udelay(1);
> > > > + }
> > > > +
> > > > + if ((val & IMX8QM_CTRL_STTS0_PM_REQ_CORE_RST) != 0) {
> > > > + dev_err(dev, "ERROR: PM_REQ_CORE_RST is still
> > > set.\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > >
> > > You can really cut down on boilerplate here by using
> > > regmap_read_poll_timeout()
> > >
> > [Richard Zhu] Okay, would change it later.
> >
> > > > + addr = IMX8QM_CSR_PHYX2_OFFSET +
> imx6_pcie->controller_id
> > > * SZ_64K;
> > > > + addr += IMX8QM_CSR_PHYX_STTS0_OFFSET;
> > > > + for (retries = 0; retries < PHY_PLL_LOCK_WAIT_MAX_RETRIES;
> > > retries++) {
> > > > + regmap_read(imx6_pcie->iomuxc_gpr, addr, &val);
> > > > + if (imx6_pcie->hsio_cfg == 2) {
> > > > + if (imx6_pcie->controller_id == 0)
> > > > + lock =
> > > IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > > > + else
> > > > + lock =
> > > IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > > > + } else if (imx6_pcie->hsio_cfg == 3) {
> > > > + lock =
> > > IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > > > + if (imx6_pcie->controller_id == 0)
> > > > + lock |=
> > > IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > > > + } else if (imx6_pcie->hsio_cfg == 1) {
> > > > + lock =
> > > IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> > > > + lock |=
> > > IMX8QM_STTS0_LANE1_TX_PLL_LOCK;
> > > > + } else {
> > > > + dev_err(dev, "ERROR: illegal hsio_cfg
> > > value.\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + val &= lock;
> > > > + if (val == lock)
> > > > + break;
> > > > + udelay(10);
> > > > + }
> > > > +
> > > > + if (retries >= PHY_PLL_LOCK_WAIT_MAX_RETRIES) {
> > > > + dev_info(dev, "pcie phy pll can't be locked.\n");
> > > > + ret = -ENODEV;
> > > > + } else {
> > > > + dev_info(dev, "pcie phy pll is locked.\n");
> > > > + }
> > > > +
> > >
> > > Ditto.
> > [Richard Zhu] Got that. Thanks.
> > >
> > > > + return ret;
> > > > +}
> > > > +
> > > > static void imx6_pcie_deassert_core_reset(struct imx6_pcie
> > > > *imx6_pcie) {
> > > > struct dw_pcie *pci = imx6_pcie->pci; @@ -553,6 +725,11
> @@
> > > > static void imx6_pcie_deassert_core_reset(struct imx6_pcie
> *imx6_pcie)
> > > > }
> > > >
> > > > switch (imx6_pcie->drvdata->variant) {
> > > > + case IMX8QXP:
> > > > + case IMX8QM:
> > > > + /* wait for phy pll lock firstly. */
> > > > + imx8_hsio_pcie_wait_for_phy_pll_lock(imx6_pcie);
> > > > + break;
> > > > case IMX8MQ:
> > > > reset_control_deassert(imx6_pcie->pciephy_reset);
> > > > break;
> > > > @@ -613,25 +790,114 @@ static void
> > > > imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > > >
> > > > static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie) {
> > > > - unsigned int mask, val;
> > > > -
> > > > - if (imx6_pcie->drvdata->variant == IMX8MQ &&
> > > > + unsigned int offset, mask, val;
> > > > +
> > > > + if (imx6_pcie->drvdata->variant == IMX8QM ||
> > > > + imx6_pcie->drvdata->variant == IMX8QXP) {
> > >
> > > Since there's now more than two possibilities, it'd probably make
> > > sense to convert this code to use switch statement.
> > [Richard Zhu] Okay, would use switch statement later.
> >
> > >
> > > > + offset = IMX8QM_CSR_PCIEA_OFFSET +
> > > > + imx6_pcie->controller_id * SZ_64K;
> > > > + mask = IMX8QM_PCIE_TYPE_MASK;
> > > > + val = FIELD_PREP(IMX8QM_PCIE_TYPE_MASK,
> > > > +
> PCI_EXP_TYPE_ROOT_PORT);
> > > > + } else if (imx6_pcie->drvdata->variant == IMX8MQ &&
> > > > imx6_pcie->controller_id == 1) {
> > > > + offset = IOMUXC_GPR12;
> > > > mask =
> > > IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> > > > val =
> > > FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> > > >
> PCI_EXP_TYPE_ROOT_PORT);
> > > > } else {
> > > > + offset = IOMUXC_GPR12;
> > > > mask = IMX6Q_GPR12_DEVICE_TYPE;
> > > > val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
> > > >
> PCI_EXP_TYPE_ROOT_PORT);
> > > > }
> > > >
> > > > - regmap_update_bits(imx6_pcie->iomuxc_gpr,
> IOMUXC_GPR12,
> > > mask, val);
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, offset, mask,
> > > > + val);
> > > > }
> > > >
> > > > static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie) {
> > > > switch (imx6_pcie->drvdata->variant) {
> > > > + case IMX8QXP:
> > > > + case IMX8QM:
> > > > + if (imx6_pcie->hsio_cfg == 1) {
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_PHYX2_OFFSET,
> > > > +
> > > IMX8QM_PHYX2_CTRL0_APB_MASK,
> > > > + IMX8QM_PHY_APB_RSTN_0 |
> > > > + IMX8QM_PHY_APB_RSTN_1);
> > > > +
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > +
> IMX8QM_MISC_PHYX1_EPCS_SEL,
> > > > +
> IMX8QM_MISC_PHYX1_EPCS_SEL);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > +
> IMX8QM_MISC_PCIE_AB_SELECT,
> > > > + 0);
> > > > + } else if (imx6_pcie->hsio_cfg == 2) {
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_PHYX2_OFFSET,
> > > > +
> > > IMX8QM_PHYX2_CTRL0_APB_MASK,
> > > > + IMX8QM_PHY_APB_RSTN_0 |
> > > > + IMX8QM_PHY_APB_RSTN_1);
> > > > +
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > +
> IMX8QM_MISC_PHYX1_EPCS_SEL,
> > > > +
> IMX8QM_MISC_PHYX1_EPCS_SEL);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > +
> IMX8QM_MISC_PCIE_AB_SELECT,
> > > > +
> IMX8QM_MISC_PCIE_AB_SELECT);
> > > > + } else if (imx6_pcie->hsio_cfg == 3) {
> > > > + if (imx6_pcie->controller_id)
> > > > +
> > > regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > +
> > > IMX8QM_CSR_PHYX1_OFFSET,
> > > > +
> > > IMX8QM_PHY_APB_RSTN_0,
> > > > +
> > > IMX8QM_PHY_APB_RSTN_0);
> > > > + else
> > > > +
> > > regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > +
> > > IMX8QM_CSR_PHYX2_OFFSET,
> > > > +
> > > IMX8QM_PHYX2_CTRL0_APB_MASK,
> > > > +
> > > IMX8QM_PHY_APB_RSTN_0 |
> > > > +
> > > IMX8QM_PHY_APB_RSTN_1);
> > > > +
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > +
> IMX8QM_MISC_PHYX1_EPCS_SEL,
> > > 0);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > +
> IMX8QM_MISC_PCIE_AB_SELECT,
> > > > +
> IMX8QM_MISC_PCIE_AB_SELECT);
> > > > + }
> > > > +
> > >
> > > This doesn't look controller independent/local. What would happen if
> > > controller 0 specifies hsio_cfg == 2 and controller 1 specifies hsio_cfg == 1?
> > >
> > [Richard Zhu]Yes, it is. There are usage dependences between PCIeA/PCIeB
> and SATA in HSIO subsystem.
> > BTW, It's impossible for controller 1 to specify the hsio_cfg to "1", when
> controller 0 specifies hsio_cfg==2.
> > There are three usage cases of the HSIO.
> > Hsio_cfg pciea pcieb sata
> > 1 2lanes No Enabled
> > 2 1lane 1lane Enabled
> > 3 2lanes 1lane No
> > So, the possible hsio_cfg values for PCIeB is 2 or 3.
>
> If I understand you correctly, I think what you mean by "impossible"
> is that in order things to work correctly when first controller is configured as
> hsio_cfg == 1, second contoller _has_ to be specified with hsio_cfg != 1.
> However, what I am trying to point out is that it is an implicit dependency
> between the two controllers and AFAICT there's no enforcement of it
> preventing me/user from creating a DT file where hsio_cfg = <1> for both
> controllers. Moving this configuration into a separate PHY driver should solve
> this, however.
>
[Richard Zhu] Yes it is. You're correct. There is an implicit dependency between the usages of the PCIeA/B and SATA.
Regarding to Lucas' comments, all the HSIO related codes should be moved to standalone PHY driver.
> >
> > > > + if (imx6_pcie->ext_osc) {
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > + IMX8QM_MISC_IOB_RXENA,
> > > > + IMX8QM_MISC_IOB_RXENA);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > + IMX8QM_MISC_IOB_TXENA, 0);
> > > > + } else {
> > > > + /* Try to used the internal pll as ref clk */
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > + IMX8QM_MISC_IOB_RXENA,
> 0);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > + IMX8QM_MISC_IOB_TXENA,
> > > > + IMX8QM_MISC_IOB_TXENA);
> > > > +
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + IMX8QM_CSR_MISC_OFFSET,
> > > > +
> > > IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> > > > +
> > > IMX8QM_CSR_MISC_IOB_A_0_M1M0_MASK,
> > > > +
> > > IMX8QM_CSR_MISC_IOB_A_0_TXOE |
> > > > +
> > > IMX8QM_CSR_MISC_IOB_A_0_M1M0_2);
> > > > + }
> > >
> > > Same here. It looks like specifying "ext_osc" for one controller and
> > > leaving it out for another would lead to different outcome based on
> > > which controller gets initialized first.
> > >
> > > It seems that maybe abstracting all of this away via a generic PHY
> > > subsystem would be a better path. See for example pci-dra7xx.c which
> > > looks like it might be a good example.
> > >
> > [Richard Zhu] Okay, would following your suggestions.
> > Thanks a lot.
> > > > +
> > > > + break;
> > > > case IMX8MQ:
> > > > /*
> > > > * TODO: Currently this code assumes external
> @@
> > > > -763,6 +1029,7 @@ static int
> > > > imx6_pcie_wait_for_speed_change(struct
> > > > imx6_pcie *imx6_pcie)
> > > >
> > > > static void imx6_pcie_ltssm_enable(struct device *dev) {
> > > > + u32 val;
> > > > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > >
> > > > switch (imx6_pcie->drvdata->variant) { @@ -777,6 +1044,15
> > > @@
> > > > static void imx6_pcie_ltssm_enable(struct device *dev)
> > > > case IMX8MQ:
> > > > reset_control_deassert(imx6_pcie->apps_reset);
> > > > break;
> > > > + case IMX8QXP:
> > > > + case IMX8QM:
> > > > + val = IMX8QM_CSR_PCIEA_OFFSET +
> > > > + imx6_pcie->controller_id * SZ_64K;
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + val +
> > > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > > +
> IMX8QM_CTRL_LTSSM_ENABLE,
> > > > +
> IMX8QM_CTRL_LTSSM_ENABLE);
> > > > + break;
> > > > }
> > > > }
> > > >
> > > > @@ -908,13 +1184,25 @@ static int imx6_add_pcie_port(struct
> > > > imx6_pcie
> > > *imx6_pcie,
> > > > return 0;
> > > > }
> > > >
> > > > +static u64 imx6_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64
> > > > +cpu_addr) {
> > > > + struct pcie_port *pp = &pcie->pp;
> > > > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pcie);
> > > > +
> > > > + if (imx6_pcie->drvdata->flags &
> > > IMX6_PCIE_FLAG_IMX6_CPU_ADDR_FIXUP)
> > > > + return (cpu_addr + imx6_pcie->local_addr -
> > > > + pp->mem_base);
> > >
> > > If you do
> > >
> > > cpu_addr += mx6_pcie->local_addr - pp->mem_base;
> > >
> > > you won't need an else below.
> > >
> > [Richard Zhu] You're right. Thanks.
> >
> > > > + else
> > > > + return cpu_addr;
> > > > +}
> > > > +
> > > > static const struct dw_pcie_ops dw_pcie_ops = {
> > > > - /* No special ops needed, but pcie-designware still expects
> this
> > > struct */
> > > > + .cpu_addr_fixup = imx6_pcie_cpu_addr_fixup,
> > > > };
> > > >
> > > > #ifdef CONFIG_PM_SLEEP
> > > > static void imx6_pcie_ltssm_disable(struct device *dev) {
> > > > + u32 val;
> > > > struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > >
> > > > switch (imx6_pcie->drvdata->variant) { @@ -926,6 +1214,17
> > > @@
> > > > static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > case IMX7D:
> > > > reset_control_assert(imx6_pcie->apps_reset);
> > > > break;
> > > > + case IMX8QXP:
> > > > + case IMX8QM:
> > > > + val = IMX8QM_CSR_PCIEA_OFFSET +
> > > > + imx6_pcie->controller_id * SZ_64K;
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + val +
> > > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > > + IMX8QM_CTRL_LTSSM_ENABLE,
> 0);
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + val +
> > > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > > +
> IMX8QM_CTRL_READY_ENTR_L23,
> > > 0);
> > > > + break;
> > > > default:
> > > > dev_err(dev, "ltssm_disable not supported\n");
> > > > }
> > > > @@ -933,6 +1232,8 @@ static void imx6_pcie_ltssm_disable(struct
> > > > device
> > > > *dev)
> > > >
> > > > static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie) {
> > > > + int i;
> > > > + u32 addr, val;
> > > > struct device *dev = imx6_pcie->pci->dev;
> > > >
> > > > /* Some variants have a turnoff reset in DT */ @@ -951,6
> > > > +1252,34 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie
> > > *imx6_pcie)
> > > > regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > IOMUXC_GPR12,
> > > >
> > > IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0);
> > > > break;
> > > > + case IMX8QXP:
> > > > + case IMX8QM:
> > > > + addr = IMX8QM_CSR_PCIEA_OFFSET +
> > > > + imx6_pcie->controller_id * SZ_64K;
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + addr +
> > > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > > +
> > > IMX8QM_CTRL_PM_XMT_TURNOFF,
> > > > +
> > > IMX8QM_CTRL_PM_XMT_TURNOFF);
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + addr +
> > > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > > +
> > > IMX8QM_CTRL_PM_XMT_TURNOFF, 0);
> > >
> > > Is setting IMX8QM_CTRL_PM_XMT_TURNOFF on and then off necessary?
> I'd
> > > add a comment to highlight that this is intentional.
> > >
> > [Richard Zhu] Designer suggest to do so. One PME message would be kicked
> off on the link after these turn on/off operations.
> >
>
> Yup, good to hear, that's exactly what I'd put in the comment :-)
>
> > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > > + addr +
> > > IMX8QM_CSR_PCIE_CTRL2_OFFSET,
> > > > +
> IMX8QM_CTRL_READY_ENTR_L23,
> > > > +
> > > IMX8QM_CTRL_READY_ENTR_L23);
> > > > + /* check the L2 is entered or not. */
> > > > + for (i = 0; i < L2_ENTRY_WAIT_MAX_RETRIES; i++) {
> > > > + regmap_read(imx6_pcie->iomuxc_gpr,
> > > > + addr +
> > > IMX8QM_CSR_PCIE_STTS0_OFFSET,
> > > > + &val);
> > > > + if (val &
> > > IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2)
> > > > + break;
> > > > + udelay(10);
> > > > + }
> > > > + if ((val & IMX8QM_CTRL_STTS0_PM_LINKST_IN_L2)
> ==
> > > 0)
> > > > + dev_err(dev, "PCIE%d can't enter into
> > > > + L2.\n",
> > > > +
> > > imx6_pcie->controller_id);
> > >
> > > regmap_read_poll_timeout()
> > >
> > [Richard Zhu] Okay, would use regmap_read_poll_timeout() in next version.
> >
> > > > + break;
> > > > default:
> > > > dev_err(dev, "PME_Turn_Off not implemented\n");
> > > > return;
> > > > @@ -985,6 +1314,11 @@ static void imx6_pcie_clk_disable(struct
> > > imx6_pcie *imx6_pcie)
> > > > case IMX8MQ:
> > > > clk_disable_unprepare(imx6_pcie->pcie_aux);
> > > > break;
> > > > + case IMX8QXP:
> > > > + case IMX8QM:
> > > > + clk_disable_unprepare(imx6_pcie->pcie_per);
> > > > +
> > > > + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > >
> > > You can probably piggy back on IMX6SX since it has
> > > "pcie_inbound_axi" as well.
> > >
> > [Richard Zhu] Okay, would follow your suggestion. Thanks.
> > Would change like below.
> > case IMX8QXP:
> > case IMX8QM:
> > clk_disable_unprepare(imx6_pcie->pcie_per);
> > case IMX6SX:
> > clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > break;
> >
> > > > + break;
> > > > default:
> > > > break;
> > > > }
> > > > @@ -1084,7 +1418,26 @@ static int imx6_pcie_probe(struct
> > > platform_device *pdev)
> > > > if (IS_ERR(pci->dbi_base))
> > > > return PTR_ERR(pci->dbi_base);
> > > >
> > > > + if (of_property_read_u32(node, "hsio-cfg",
> > > &imx6_pcie->hsio_cfg))
> > > > + imx6_pcie->hsio_cfg = 0;
> > > > + if (of_property_read_u32(node, "ext_osc",
> > > > + &imx6_pcie->ext_osc)
> > > < 0)
> > > > + imx6_pcie->ext_osc = 0;
> > > > + if (of_property_read_u32(node, "local-addr",
> > > &imx6_pcie->local_addr))
> > > > + imx6_pcie->local_addr = 0;
> > >
> > > All of these properties will be initialized to zero by kzalloc and
> > > of_property_read_u32() won't modify output variable unless it is
> > > successful, so you can probably skip error checking.
> > [Richard Zhu] Okay, would remove the error checking.
> >
> > >
> > > > +
> > > > /* Fetch GPIOs */
> > > > + imx6_pcie->clkreq_gpio = of_get_named_gpio(node,
> > > > + "clkreq-gpio",
> > > 0);
> > > > + if (gpio_is_valid(imx6_pcie->clkreq_gpio)) {
> > > > + ret = devm_gpio_request_one(&pdev->dev,
> > > imx6_pcie->clkreq_gpio,
> > > > +
> > > GPIOF_OUT_INIT_LOW, "PCIe CLKREQ");
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "unable to get clkreq
> > > gpio\n");
> > > > + return ret;
> > > > + }
> > > > + } else if (imx6_pcie->clkreq_gpio == -EPROBE_DEFER) {
> > > > + return imx6_pcie->clkreq_gpio;
> > > > + }
> > > > +
> > > > imx6_pcie->reset_gpio = of_get_named_gpio(node,
> > > > "reset-gpio",
> > > 0);
> > > > imx6_pcie->gpio_active_high = of_property_read_bool(node,
> > > >
> > > > "reset-gpio-active-high"); @@ -1155,6 +1508,25 @@ static int
> > > imx6_pcie_probe(struct platform_device *pdev)
> > > > return PTR_ERR(imx6_pcie->pcie_aux);
> > > > }
> > > > break;
> > > > + case IMX8QM:
> > > > + case IMX8QXP:
> > > > + if (dbi_base->start ==
> IMX8_HSIO_PCIEB_BASE_ADDR)
> > > > + imx6_pcie->controller_id = 1;
> > > > +
> > > > + imx6_pcie->pcie_per = devm_clk_get(dev,
> "pcie_per");
> > > > + if (IS_ERR(imx6_pcie->pcie_per)) {
> > > > + dev_err(dev, "pcie_per clock source
> > > > + missing
> > > or invalid\n");
> > > > + return PTR_ERR(imx6_pcie->pcie_per);
> > > > + }
> > > > +
> > > > + imx6_pcie->pcie_inbound_axi =
> > > devm_clk_get(&pdev->dev,
> > > > + "pcie_inbound_axi");
> > > > + if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
> > > > + dev_err(&pdev->dev,
> > > > + "pcie clock source missing or
> > > invalid\n");
> > > > + return
> > > PTR_ERR(imx6_pcie->pcie_inbound_axi);
> > > > + }
> > >
> > > On i.MX8MQ "pcie_bus" clock in vendor tree wasn't actually pointing
> > > to actual PCIE bus clock, so it might be worth checking if that's
> > > the case for i.MX8QM/X and you actually need one more clock.
> > [Richard Zhu] Regarding to my understanding, iMX PCIe module is
> connected to AXI bus.
> > Thus, the AXI related clock can be treated as bus clock. Correct me if my
> understand is wrong.
> > So, I use the pcie_bus clock for i.MX8QM/QXP PCIe in the dts binding.
> > Otherwise, I can use another new clock in codes to support i.MX8QM/QXP
> PCIes.
> >
>
> So, "pcie_bus" is supposed to be the clock driving PCIE bus itself. In this case
> the clock that is controlled by CLKREQ_B. On i.MX8MQ EVK that was an
> external 100 Mhz oscillator, so the final patch has "pcie_bus" pointing to a
> dedicated "fixed-clock":
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke
> rnel.org%2Flkml%2F20190220015857.7136-6-andrew.smirnov%40gmail.com
> %2FT%2F%23u&data=02%7C01%7Chongxing.zhu%40nxp.com%7Cb745
> 5fe59e384723f44208d6a8ec835a%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636882131105162138&sdata=s7438xBSNxnWwTMxZXkj
> LhgdPiS6puCRdrgr9suZpPQ%3D&reserved=0
>
> Originally vendor tree was using "pcie_bus" to point at
> IMX8MQ_CLK_PCIE1_AUX. If the situation on i.MX8QM/QXP is similar, then,
> yeah, I think it should be moved out into a separate clock.
>
[Richard Zhu] The clocks of the i.MX8QM/QXP PCIe are different to the iMX8MQ PCIe's.
Five clocks MASTER_AXI, SLAVE_AXI, DBI_AXI, PIPE_CLK and PER_CLK are mandatory required.
Currently, They are named "pcie", "pcie_bus", "pcie_inbound_axi", "pcie_phy", "pcie_per" in the vendor tree.
PIPE_CLK is output to PHY, so "pcie_phy" clock name is used by it.
I'm not sure that the names of the xxx_AXI clocks are proper or not.
What're your suggests about the names of xxx_AXI clocks?
Did new clock names for all or part of these three xxx_AXI clocks shall be added in to the codes?
Thanks in advanced.
> Thanks,
> Andrey Smirnov
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 2/2] PCI: imx6: Add support for i.MX8QM/QXP PCIe
2019-03-15 6:04 ` Richard Zhu
@ 2019-03-19 3:35 ` Andrey Smirnov
0 siblings, 0 replies; 10+ messages in thread
From: Andrey Smirnov @ 2019-03-19 3:35 UTC (permalink / raw)
To: Richard Zhu, l.stach
Cc: bhelgaas, lorenzo.pieralisi, linux-pci, linux-arm-kernel, linux-kernel
On Thu, Mar 14, 2019 at 11:05 PM Richard Zhu <hongxing.zhu@nxp.com> wrote:
> > > > > + imx6_pcie->pcie_inbound_axi =
> > > > devm_clk_get(&pdev->dev,
> > > > > + "pcie_inbound_axi");
> > > > > + if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
> > > > > + dev_err(&pdev->dev,
> > > > > + "pcie clock source missing or
> > > > invalid\n");
> > > > > + return
> > > > PTR_ERR(imx6_pcie->pcie_inbound_axi);
> > > > > + }
> > > >
> > > > On i.MX8MQ "pcie_bus" clock in vendor tree wasn't actually pointing
> > > > to actual PCIE bus clock, so it might be worth checking if that's
> > > > the case for i.MX8QM/X and you actually need one more clock.
> > > [Richard Zhu] Regarding to my understanding, iMX PCIe module is
> > connected to AXI bus.
> > > Thus, the AXI related clock can be treated as bus clock. Correct me if my
> > understand is wrong.
> > > So, I use the pcie_bus clock for i.MX8QM/QXP PCIe in the dts binding.
> > > Otherwise, I can use another new clock in codes to support i.MX8QM/QXP
> > PCIes.
> > >
> >
> > So, "pcie_bus" is supposed to be the clock driving PCIE bus itself. In this case
> > the clock that is controlled by CLKREQ_B. On i.MX8MQ EVK that was an
> > external 100 Mhz oscillator, so the final patch has "pcie_bus" pointing to a
> > dedicated "fixed-clock":
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke
> > rnel.org%2Flkml%2F20190220015857.7136-6-andrew.smirnov%40gmail.com
> > %2FT%2F%23u&data=02%7C01%7Chongxing.zhu%40nxp.com%7Cb745
> > 5fe59e384723f44208d6a8ec835a%7C686ea1d3bc2b4c6fa92cd99c5c301635
> > %7C0%7C0%7C636882131105162138&sdata=s7438xBSNxnWwTMxZXkj
> > LhgdPiS6puCRdrgr9suZpPQ%3D&reserved=0
> >
> > Originally vendor tree was using "pcie_bus" to point at
> > IMX8MQ_CLK_PCIE1_AUX. If the situation on i.MX8QM/QXP is similar, then,
> > yeah, I think it should be moved out into a separate clock.
> >
> [Richard Zhu] The clocks of the i.MX8QM/QXP PCIe are different to the iMX8MQ PCIe's.
> Five clocks MASTER_AXI, SLAVE_AXI, DBI_AXI, PIPE_CLK and PER_CLK are mandatory required.
> Currently, They are named "pcie", "pcie_bus", "pcie_inbound_axi", "pcie_phy", "pcie_per" in the vendor tree.
> PIPE_CLK is output to PHY, so "pcie_phy" clock name is used by it.
> I'm not sure that the names of the xxx_AXI clocks are proper or not.
> What're your suggests about the names of xxx_AXI clocks?
> Did new clock names for all or part of these three xxx_AXI clocks shall be added in to the codes?
> Thanks in advanced.
>
I am hardly an authority on how those clocks should be named, so don't
put too much value in my suggestion. However if I had to do, I'd
probably use "pcie" for MASTER_AXI and add "pcie_slave" or maybe
"pcie2" to control SLAVE_AXI.
Lucas, if you don't mind, could you please comment on clock naming
situation here?
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-03-19 3:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-13 9:15 [RFC 1/2] dt-bindings: imx6q-pcie: Add support for i.MX8QM/QXP PCIe Richard Zhu
2019-03-13 9:15 ` [RFC 2/2] PCI: imx6: " Richard Zhu
2019-03-13 20:20 ` Andrey Smirnov
2019-03-14 9:18 ` Richard Zhu
2019-03-15 2:18 ` Andrey Smirnov
2019-03-15 6:04 ` Richard Zhu
2019-03-19 3:35 ` Andrey Smirnov
2019-03-14 9:30 ` [RFC 1/2] dt-bindings: imx6q-pcie: " Lucas Stach
2019-03-15 1:25 ` Richard Zhu
2019-03-15 2:26 ` Andrey Smirnov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).