first 6 patches use drvdata: flags to simplify some switch-case code.
Improve maintaince and easy to read code.
Then add imx95 basic pci host function.
follow two patch do endpoint code clean up.
Then add imx95 basic endpont function.
Compared with v2, added EP function support and some fixes, please change
notes at each patches.
dt-binding pass pcie node:
pcie0: pcie@4c300000 {
compatible = "fsl,imx95-pcie";
reg = <0 0x4c300000 0 0x40000>,
<0 0x4c360000 0 0x10000>,
<0 0x4c340000 0 0x20000>,
<0 0x60100000 0 0xfe00000>;
reg-names = "dbi", "atu", "app", "config";
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
linux,pci-domain = <0>;
bus-range = <0x00 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x0 0x6ff00000 0 0x00100000>,
<0x82000000 0x0 0x10000000 0x9 0x10000000 0 0x10000000>;
num-lanes = <1>;
num-viewport = <8>;
interrupts = <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "msi";
#interrupt-cells = <1>;
interrupt-map-mask = <0 0 0 0x7>;
interrupt-map = <0 0 0 1 &gic 0 0 GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
<0 0 0 2 &gic 0 0 GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>,
<0 0 0 3 &gic 0 0 GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
<0 0 0 4 &gic 0 0 GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>;
fsl,max-link-speed = <3>;
clocks = <&scmi_clk IMX95_CLK_HSIO>,
<&scmi_clk IMX95_CLK_HSIOPLL>,
<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
<&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux";
assigned-clocks =<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
<&scmi_clk IMX95_CLK_HSIOPLL>,
<&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
assigned-clock-rates = <3600000000>, <100000000>, <10000000>;
assigned-clock-parents = <0>, <0>,
<&scmi_clk IMX95_CLK_SYSPLL1_PFD1_DIV2>;
power-domains = <&scmi_devpd IMX95_PD_HSIO_TOP>;
/* 0x30~0x37 stream id for pci0 */
/*
* iommu-map = <0x000 &apps_smmu 0x30 0x1>,
* <0x100 &apps_smmu 0x31 0x1>;
*/
status = "disabled";
};
pcie1: pcie-ep@4c380000 {
compatible = "fsl,imx95-pcie-ep";
reg = <0 0x4c380000 0 0x20000>,
<0 0x4c3e0000 0 0x1000>,
<0 0x4c3a0000 0 0x1000>,
<0 0x4c3c0000 0 0x10000>,
<0 0x4c3f0000 0 0x10000>,
<0xa 0 1 0>;
reg-names = "dbi", "atu", "dbi2", "app", "dma", "addr_space";
interrupts = <GIC_SPI 317 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "dma";
fsl,max-link-speed = <3>;
clocks = <&scmi_clk IMX95_CLK_HSIO>,
<&scmi_clk IMX95_CLK_HSIOPLL>,
<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
<&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux";
assigned-clocks =<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
<&scmi_clk IMX95_CLK_HSIOPLL>,
<&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
assigned-clock-rates = <3600000000>, <100000000>, <10000000>;
assigned-clock-parents = <0>, <0>,
<&scmi_clk IMX95_CLK_SYSPLL1_PFD1_DIV2>;
power-domains = <&scmi_devpd IMX95_PD_HSIO_TOP>;
status = "disabled";
};
Frank Li (15):
PCI: imx6: Simplify clock handling by using bulk_clk_*() function
PCI: imx6: Simplify phy handling by using by using
IMX6_PCIE_FLAG_HAS_PHY
PCI: imx6: Simplify reset handling by using by using
*_FLAG_HAS_*_RESET
dt-bindings: imx6q-pcie: Add linux,pci-domain as required for iMX8MQ
PCI: imx6: Using "linux,pci-domain" as slot ID
PCI: imx6: Simplify ltssm_enable() by using ltssm_off and ltssm_mask
PCI: imx6: Simplify configure_type() by using mode_off and mode_mask
PCI: imx6: Simplify switch-case logic by involve init_phy callback
dt-bindings: imx6q-pcie: Clean up irrationality clocks check
dt-bindings: imx6q-pcie: restruct reg and reg-name
PCI: imx6: Add iMX95 PCIe support
PCI: imx6: Clean up get addr_space code
PCI: imx6: Add epc_features in imx6_pcie_drvdata
dt-bindings: imx6q-pcie: Add iMX95 pcie endpoint compatible string
PCI: imx6: Add iMX95 Endpoint (EP) function support
Richard Zhu (1):
dt-bindings: imx6q-pcie: Add imx95 pcie compatible string
.../bindings/pci/fsl,imx6q-pcie-common.yaml | 28 +-
.../bindings/pci/fsl,imx6q-pcie-ep.yaml | 57 +-
.../bindings/pci/fsl,imx6q-pcie.yaml | 49 +-
drivers/pci/controller/dwc/pci-imx6.c | 628 ++++++++++--------
4 files changed, 452 insertions(+), 310 deletions(-)
--
2.34.1
Refactors the clock handling logic. Adds clk_names[] define in drvdata.
Using clk_bulk*() api simplifies the code.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v4 to v5
- update commit message
- direct using clk name list, instead of macro
- still keep caculate clk list count because sizeof return pre allocated
array size.
Change from v3 to v4
- using clk_bulk_*() API
Change from v1 to v3
- none
drivers/pci/controller/dwc/pci-imx6.c | 125 ++++++++------------------
1 file changed, 35 insertions(+), 90 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 74703362aeec7..50d9faaa17f71 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -61,12 +61,15 @@ enum imx6_pcie_variants {
#define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
#define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
+#define IMX6_PCIE_MAX_CLKS 6
+
struct imx6_pcie_drvdata {
enum imx6_pcie_variants variant;
enum dw_pcie_device_mode mode;
u32 flags;
int dbi_length;
const char *gpr;
+ const char *clk_names[IMX6_PCIE_MAX_CLKS];
};
struct imx6_pcie {
@@ -74,11 +77,8 @@ struct imx6_pcie {
int reset_gpio;
bool gpio_active_high;
bool link_is_up;
- struct clk *pcie_bus;
- struct clk *pcie_phy;
- struct clk *pcie_inbound_axi;
- struct clk *pcie;
- struct clk *pcie_aux;
+ struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
+ u32 clks_cnt;
struct regmap *iomuxc_gpr;
u16 msi_ctrl;
u32 controller_id;
@@ -407,13 +407,18 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
{
- unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
+ unsigned long phy_rate = 0;
int mult, div;
u16 val;
+ int i;
if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
return 0;
+ for (i = 0; i < imx6_pcie->clks_cnt; i++)
+ if (strncmp(imx6_pcie->clks[i].id, "pcie_phy", 8) == 0)
+ phy_rate = clk_get_rate(imx6_pcie->clks[i].clk);
+
switch (phy_rate) {
case 125000000:
/*
@@ -550,19 +555,11 @@ static int imx6_pcie_attach_pd(struct device *dev)
static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
{
- struct dw_pcie *pci = imx6_pcie->pci;
- struct device *dev = pci->dev;
unsigned int offset;
int ret = 0;
switch (imx6_pcie->drvdata->variant) {
case IMX6SX:
- ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
- if (ret) {
- dev_err(dev, "unable to enable pcie_axi clock\n");
- break;
- }
-
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0);
break;
@@ -589,12 +586,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
case IMX8MQ_EP:
case IMX8MP:
case IMX8MP_EP:
- ret = clk_prepare_enable(imx6_pcie->pcie_aux);
- if (ret) {
- dev_err(dev, "unable to enable pcie_aux clock\n");
- break;
- }
-
offset = imx6_pcie_grp_offset(imx6_pcie);
/*
* Set the over ride low and enabled
@@ -615,9 +606,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
{
switch (imx6_pcie->drvdata->variant) {
- case IMX6SX:
- clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
- break;
case IMX6QP:
case IMX6Q:
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
@@ -631,14 +619,6 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
break;
- case IMX8MM:
- case IMX8MM_EP:
- case IMX8MQ:
- case IMX8MQ_EP:
- case IMX8MP:
- case IMX8MP_EP:
- clk_disable_unprepare(imx6_pcie->pcie_aux);
- break;
default:
break;
}
@@ -650,23 +630,9 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
struct device *dev = pci->dev;
int ret;
- ret = clk_prepare_enable(imx6_pcie->pcie_phy);
- if (ret) {
- dev_err(dev, "unable to enable pcie_phy clock\n");
+ ret = clk_bulk_prepare_enable(imx6_pcie->clks_cnt, imx6_pcie->clks);
+ if (ret)
return ret;
- }
-
- ret = clk_prepare_enable(imx6_pcie->pcie_bus);
- if (ret) {
- dev_err(dev, "unable to enable pcie_bus clock\n");
- goto err_pcie_bus;
- }
-
- ret = clk_prepare_enable(imx6_pcie->pcie);
- if (ret) {
- dev_err(dev, "unable to enable pcie clock\n");
- goto err_pcie;
- }
ret = imx6_pcie_enable_ref_clk(imx6_pcie);
if (ret) {
@@ -679,11 +645,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
return 0;
err_ref_clk:
- clk_disable_unprepare(imx6_pcie->pcie);
-err_pcie:
- clk_disable_unprepare(imx6_pcie->pcie_bus);
-err_pcie_bus:
- clk_disable_unprepare(imx6_pcie->pcie_phy);
+ clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
return ret;
}
@@ -691,9 +653,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
{
imx6_pcie_disable_ref_clk(imx6_pcie);
- clk_disable_unprepare(imx6_pcie->pcie);
- clk_disable_unprepare(imx6_pcie->pcie_bus);
- clk_disable_unprepare(imx6_pcie->pcie_phy);
+ clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
}
static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
@@ -1305,32 +1265,19 @@ static int imx6_pcie_probe(struct platform_device *pdev)
return imx6_pcie->reset_gpio;
}
- /* Fetch clocks */
- imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
- if (IS_ERR(imx6_pcie->pcie_bus))
- return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
- "pcie_bus clock source missing or invalid\n");
+ while (imx6_pcie->drvdata->clk_names[imx6_pcie->clks_cnt]) {
+ int i = imx6_pcie->clks_cnt;
+
+ imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
+ imx6_pcie->clks_cnt++;
+ }
- imx6_pcie->pcie = devm_clk_get(dev, "pcie");
- if (IS_ERR(imx6_pcie->pcie))
- return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
- "pcie clock source missing or invalid\n");
+ /* Fetch clocks */
+ ret = devm_clk_bulk_get(dev, imx6_pcie->clks_cnt, imx6_pcie->clks);
+ if (ret)
+ return ret;
switch (imx6_pcie->drvdata->variant) {
- case IMX6SX:
- imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
- "pcie_inbound_axi");
- if (IS_ERR(imx6_pcie->pcie_inbound_axi))
- return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
- "pcie_inbound_axi clock missing or invalid\n");
- break;
- case IMX8MQ:
- case IMX8MQ_EP:
- imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
- if (IS_ERR(imx6_pcie->pcie_aux))
- return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
- "pcie_aux clock source missing or invalid\n");
- fallthrough;
case IMX7D:
if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
imx6_pcie->controller_id = 1;
@@ -1353,10 +1300,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
case IMX8MM_EP:
case IMX8MP:
case IMX8MP_EP:
- imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
- if (IS_ERR(imx6_pcie->pcie_aux))
- return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
- "pcie_aux clock source missing or invalid\n");
imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
"apps");
if (IS_ERR(imx6_pcie->apps_reset))
@@ -1372,14 +1315,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
default:
break;
}
- /* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
- if (imx6_pcie->phy == NULL) {
- imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
- if (IS_ERR(imx6_pcie->pcie_phy))
- return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
- "pcie_phy clock source missing or invalid\n");
- }
-
/* Grab turnoff reset */
imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
@@ -1477,6 +1412,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
.dbi_length = 0x200,
.gpr = "fsl,imx6q-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
},
[IMX6SX] = {
.variant = IMX6SX,
@@ -1484,6 +1420,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
.gpr = "fsl,imx6q-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
},
[IMX6QP] = {
.variant = IMX6QP,
@@ -1492,40 +1429,48 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
.dbi_length = 0x200,
.gpr = "fsl,imx6q-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
},
[IMX7D] = {
.variant = IMX7D,
.flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
.gpr = "fsl,imx7d-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
},
[IMX8MQ] = {
.variant = IMX8MQ,
.gpr = "fsl,imx8mq-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
},
[IMX8MM] = {
.variant = IMX8MM,
.flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
.gpr = "fsl,imx8mm-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
[IMX8MP] = {
.variant = IMX8MP,
.flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
.gpr = "fsl,imx8mp-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
[IMX8MQ_EP] = {
.variant = IMX8MQ_EP,
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mq-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
},
[IMX8MM_EP] = {
.variant = IMX8MM_EP,
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mm-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
[IMX8MP_EP] = {
.variant = IMX8MP_EP,
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mp-iomuxc-gpr",
+ .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
};
--
2.34.1
Refactors the phy handling logic in the imx6 PCI driver by adding
IMX6_PCIE_FLAG_HAS_PHY bitmask define for drvdata::flags.
The drvdata::flags and a bitmask ensures a cleaner and more scalable
switch-case structure for handling phy.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v4 to v5:
- none, Keep IMX6_PCIE_FLAG_HAS_PHY to indicate dts mismatch when platform
require phy suppport.
Change from v1 to v3:
- none
drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 50d9faaa17f71..4d620249f3d52 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -60,6 +60,9 @@ enum imx6_pcie_variants {
#define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
#define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
#define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
+#define IMX6_PCIE_FLAG_HAS_PHY BIT(3)
+
+#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
#define IMX6_PCIE_MAX_CLKS 6
@@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev)
if (ret)
return ret;
+ if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY)) {
+ imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
+ if (IS_ERR(imx6_pcie->phy))
+ return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
+ "failed to get pcie phy\n");
+ }
+
switch (imx6_pcie->drvdata->variant) {
case IMX7D:
if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
@@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset),
"failed to get pcie apps reset control\n");
- imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
- if (IS_ERR(imx6_pcie->phy))
- return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
- "failed to get pcie phy\n");
-
break;
default:
break;
@@ -1444,13 +1449,15 @@ static const struct imx6_pcie_drvdata drvdata[] = {
},
[IMX8MM] = {
.variant = IMX8MM,
- .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
+ .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
+ IMX6_PCIE_FLAG_HAS_PHY,
.gpr = "fsl,imx8mm-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
[IMX8MP] = {
.variant = IMX8MP,
- .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
+ .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
+ IMX6_PCIE_FLAG_HAS_PHY,
.gpr = "fsl,imx8mp-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
@@ -1462,12 +1469,14 @@ static const struct imx6_pcie_drvdata drvdata[] = {
},
[IMX8MM_EP] = {
.variant = IMX8MM_EP,
+ .flags = IMX6_PCIE_FLAG_HAS_PHY,
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mm-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
[IMX8MP_EP] = {
.variant = IMX8MP_EP,
+ .flags = IMX6_PCIE_FLAG_HAS_PHY,
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mp-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
--
2.34.1
Refactors the reset handling logic in the imx6 PCI driver by adding
IMX6_PCIE_FLAG_HAS_*_RESET bitmask define for drvdata::flags.
The drvdata::flags and a bitmask ensures a cleaner and more scalable
switch-case structure for handling reset.
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Philipp Zabel <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v4 to v5:
- Add Mani's Reviewed-by tag
- Fixed MQ_EP's flags
Chagne from v3 to v4:
- none
Change from v2 to v3:
- add Philipp's Reviewed-by tag
Change from v1 to v2:
- remove condition check before reset_control_(de)assert() because it is
none ops if a NULL pointer pass down.
- still keep condition check at probe to help identify dts file mismatch
problem.
Change from v1 to v2:
- remove condition check before reset_control_(de)assert() because it is
none ops if a NULL pointer pass down.
- still keep condition check at probe to help identify dts file mismatch
problem.
drivers/pci/controller/dwc/pci-imx6.c | 108 ++++++++++----------------
1 file changed, 41 insertions(+), 67 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 4d620249f3d52..294f61a9c6fd9 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -61,6 +61,8 @@ enum imx6_pcie_variants {
#define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
#define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
#define IMX6_PCIE_FLAG_HAS_PHY BIT(3)
+#define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4)
+#define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5)
#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
@@ -661,18 +663,10 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
{
+ reset_control_assert(imx6_pcie->pciephy_reset);
+ reset_control_assert(imx6_pcie->apps_reset);
+
switch (imx6_pcie->drvdata->variant) {
- case IMX7D:
- case IMX8MQ:
- case IMX8MQ_EP:
- reset_control_assert(imx6_pcie->pciephy_reset);
- fallthrough;
- case IMX8MM:
- case IMX8MM_EP:
- case IMX8MP:
- case IMX8MP_EP:
- reset_control_assert(imx6_pcie->apps_reset);
- break;
case IMX6SX:
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
@@ -693,6 +687,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
break;
+ default:
+ break;
}
/* Some boards don't have PCIe reset GPIO. */
@@ -706,14 +702,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
struct dw_pcie *pci = imx6_pcie->pci;
struct device *dev = pci->dev;
+ reset_control_deassert(imx6_pcie->pciephy_reset);
+
switch (imx6_pcie->drvdata->variant) {
- case IMX8MQ:
- case IMX8MQ_EP:
- reset_control_deassert(imx6_pcie->pciephy_reset);
- break;
case IMX7D:
- reset_control_deassert(imx6_pcie->pciephy_reset);
-
/* Workaround for ERR010728, failure of PCI-e PLL VCO to
* oscillate, especially when cold. This turns off "Duty-cycle
* Corrector" and other mysterious undocumented things.
@@ -745,11 +737,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
usleep_range(200, 500);
break;
- case IMX6Q: /* Nothing to do */
- case IMX8MM:
- case IMX8MM_EP:
- case IMX8MP:
- case IMX8MP_EP:
+ default:
break;
}
@@ -796,16 +784,11 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
IMX6Q_GPR12_PCIE_CTL_2,
IMX6Q_GPR12_PCIE_CTL_2);
break;
- case IMX7D:
- case IMX8MQ:
- case IMX8MQ_EP:
- case IMX8MM:
- case IMX8MM_EP:
- case IMX8MP:
- case IMX8MP_EP:
- reset_control_deassert(imx6_pcie->apps_reset);
+ default:
break;
}
+
+ reset_control_deassert(imx6_pcie->apps_reset);
}
static void imx6_pcie_ltssm_disable(struct device *dev)
@@ -819,16 +802,11 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6Q_GPR12_PCIE_CTL_2, 0);
break;
- case IMX7D:
- case IMX8MQ:
- case IMX8MQ_EP:
- case IMX8MM:
- case IMX8MM_EP:
- case IMX8MP:
- case IMX8MP_EP:
- reset_control_assert(imx6_pcie->apps_reset);
+ default:
break;
}
+
+ reset_control_assert(imx6_pcie->apps_reset);
}
static int imx6_pcie_start_link(struct dw_pcie *pci)
@@ -1287,36 +1265,24 @@ static int imx6_pcie_probe(struct platform_device *pdev)
"failed to get pcie phy\n");
}
- switch (imx6_pcie->drvdata->variant) {
- case IMX7D:
- if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
- imx6_pcie->controller_id = 1;
-
- imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
- "pciephy");
- if (IS_ERR(imx6_pcie->pciephy_reset)) {
- dev_err(dev, "Failed to get PCIEPHY reset control\n");
- return PTR_ERR(imx6_pcie->pciephy_reset);
- }
-
- imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
- "apps");
- if (IS_ERR(imx6_pcie->apps_reset)) {
- dev_err(dev, "Failed to get PCIE APPS reset control\n");
- return PTR_ERR(imx6_pcie->apps_reset);
- }
- break;
- case IMX8MM:
- case IMX8MM_EP:
- case IMX8MP:
- case IMX8MP_EP:
- imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
- "apps");
+ if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_APP_RESET)) {
+ imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev, "apps");
if (IS_ERR(imx6_pcie->apps_reset))
return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset),
"failed to get pcie apps reset control\n");
+ }
- break;
+ if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY_RESET)) {
+ imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev, "pciephy");
+ if (IS_ERR(imx6_pcie->pciephy_reset))
+ return dev_err_probe(dev, PTR_ERR(imx6_pcie->pciephy_reset),
+ "Failed to get PCIEPHY reset control\n");
+ }
+
+ switch (imx6_pcie->drvdata->variant) {
+ case IMX7D:
+ if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
+ imx6_pcie->controller_id = 1;
default:
break;
}
@@ -1438,31 +1404,39 @@ static const struct imx6_pcie_drvdata drvdata[] = {
},
[IMX7D] = {
.variant = IMX7D,
- .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
+ .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
+ IMX6_PCIE_FLAG_HAS_APP_RESET |
+ IMX6_PCIE_FLAG_HAS_PHY_RESET,
.gpr = "fsl,imx7d-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy"},
},
[IMX8MQ] = {
.variant = IMX8MQ,
+ .flags = IMX6_PCIE_FLAG_HAS_APP_RESET |
+ IMX6_PCIE_FLAG_HAS_PHY_RESET,
.gpr = "fsl,imx8mq-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
},
[IMX8MM] = {
.variant = IMX8MM,
.flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
- IMX6_PCIE_FLAG_HAS_PHY,
+ IMX6_PCIE_FLAG_HAS_PHY |
+ IMX6_PCIE_FLAG_HAS_APP_RESET,
.gpr = "fsl,imx8mm-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
[IMX8MP] = {
.variant = IMX8MP,
.flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
- IMX6_PCIE_FLAG_HAS_PHY,
+ IMX6_PCIE_FLAG_HAS_PHY |
+ IMX6_PCIE_FLAG_HAS_APP_RESET,
.gpr = "fsl,imx8mp-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
},
[IMX8MQ_EP] = {
.variant = IMX8MQ_EP,
+ .flags = IMX6_PCIE_FLAG_HAS_APP_RESET |
+ IMX6_PCIE_FLAG_HAS_PHY_RESET,
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mq-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
--
2.34.1
iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
proptery for iMX8MQ to indicate pci controller index.
This adjustment paves the way for eliminating the hardcoded check on the
base register for acquiring the controller_id.
...
if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
imx6_pcie->controller_id = 1;
...
The controller_id is crucial and utilized for certain register bit
positions. It must align precisely with the controller index in the SoC.
An auto-incremented ID don't fit this case. The DTS or fuse configurations
may deactivate specific PCI controllers.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v5 to v6
- rework commit message to explain why need required and why auto increase
id not work
Change from v4 to v5
- new patch at v5
.../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
index d91b639ae7ae7..8f39b4e6e8491 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
@@ -265,6 +265,17 @@ allOf:
- const: apps
- const: turnoff
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - fsl,imx8mq-pcie
+ - fsl,imx8mq-pcie-ep
+ then:
+ required:
+ - linux,pci-domain
+
additionalProperties: true
...
--
2.34.1
Avoid use get slot id by compared with register physical address. If there
are more than 2 slots, compared logic will become complex.
"linux,pci-domain" already exist at dts since commit:
commit (c0b70f05c87f3b arm64: dts: imx8mq: use_dt_domains for pci node).
So it is safe to remove compare basic address code:
...
if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
imx6_pcie->controller_id = 1;
...
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v3 to v4
- remove compare basic address logic
Change from v2 to v3
- none
Change from v1 to v2
- fix of_get_pci_domain_nr return value check logic
drivers/pci/controller/dwc/pci-imx6.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 294f61a9c6fd9..332c392f8e5bc 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -33,6 +33,7 @@
#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
+#include "../../pci.h"
#include "pcie-designware.h"
#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
@@ -40,7 +41,6 @@
#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
#define IMX8MQ_GPR_PCIE_VREG_BYPASS BIT(12)
#define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
-#define IMX8MQ_PCIE2_BASE_ADDR 0x33c00000
#define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
@@ -1279,13 +1279,14 @@ static int imx6_pcie_probe(struct platform_device *pdev)
"Failed to get PCIEPHY reset control\n");
}
- switch (imx6_pcie->drvdata->variant) {
- case IMX7D:
- if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
- imx6_pcie->controller_id = 1;
- default:
- break;
- }
+ /* Using linux,pci-domain as PCI slot id */
+ imx6_pcie->controller_id = of_get_pci_domain_nr(node);
+ /* If there are not "linux,pci-domain" in dts file, means only 1 controller */
+ if (imx6_pcie->controller_id == -EINVAL)
+ imx6_pcie->controller_id = 0;
+ else if (imx6_pcie->controller_id < 0)
+ return dev_err_probe(dev, imx6_pcie->controller_id,
+ "linux,pci-domain have wrong value\n");
/* Grab turnoff reset */
imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
--
2.34.1
Add drvdata::ltssm_off and drvdata::ltssm_mask to simple
imx6_pcie_ltssm_enable(disable)() logic.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v1 to v3
- none
drivers/pci/controller/dwc/pci-imx6.c | 37 ++++++++++++---------------
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 332c392f8e5bc..588bfb616260e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -75,6 +75,8 @@ struct imx6_pcie_drvdata {
int dbi_length;
const char *gpr;
const char *clk_names[IMX6_PCIE_MAX_CLKS];
+ const u32 ltssm_off;
+ const u32 ltssm_mask;
};
struct imx6_pcie {
@@ -775,18 +777,11 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_ltssm_enable(struct device *dev)
{
struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+ const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
- switch (imx6_pcie->drvdata->variant) {
- case IMX6Q:
- case IMX6SX:
- case IMX6QP:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2,
- IMX6Q_GPR12_PCIE_CTL_2);
- break;
- default:
- break;
- }
+ if (drvdata->ltssm_mask)
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->ltssm_off, drvdata->ltssm_mask,
+ drvdata->ltssm_mask);
reset_control_deassert(imx6_pcie->apps_reset);
}
@@ -794,17 +789,11 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
static void imx6_pcie_ltssm_disable(struct device *dev)
{
struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+ const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
- switch (imx6_pcie->drvdata->variant) {
- case IMX6Q:
- case IMX6SX:
- case IMX6QP:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 0);
- break;
- default:
- break;
- }
+ if (drvdata->ltssm_mask)
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->ltssm_off,
+ drvdata->ltssm_mask, 0);
reset_control_assert(imx6_pcie->apps_reset);
}
@@ -1385,6 +1374,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.dbi_length = 0x200,
.gpr = "fsl,imx6q-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy"},
+ .ltssm_off = IOMUXC_GPR12,
+ .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
},
[IMX6SX] = {
.variant = IMX6SX,
@@ -1393,6 +1384,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
.gpr = "fsl,imx6q-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
+ .ltssm_off = IOMUXC_GPR12,
+ .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
},
[IMX6QP] = {
.variant = IMX6QP,
@@ -1402,6 +1395,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.dbi_length = 0x200,
.gpr = "fsl,imx6q-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy"},
+ .ltssm_off = IOMUXC_GPR12,
+ .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
},
[IMX7D] = {
.variant = IMX7D,
--
2.34.1
Add drvdata::init_phy() callback function, so difference SOC choose
difference callback function to simple switch-case logic.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
change from v1 to v4:
- none
drivers/pci/controller/dwc/pci-imx6.c | 135 ++++++++++++++------------
1 file changed, 71 insertions(+), 64 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 717e8fa030deb..d66a2db53bdb7 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -69,6 +69,9 @@ enum imx6_pcie_variants {
#define IMX6_PCIE_MAX_CLKS 6
#define IMX6_PCIE_MAX_INSTANCES 2
+
+struct imx6_pcie;
+
struct imx6_pcie_drvdata {
enum imx6_pcie_variants variant;
enum dw_pcie_device_mode mode;
@@ -80,6 +83,7 @@ struct imx6_pcie_drvdata {
const u32 ltssm_mask;
const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
+ int (*init_phy)(struct imx6_pcie *pcie);
};
struct imx6_pcie {
@@ -323,76 +327,69 @@ static int pcie_phy_write(struct imx6_pcie *imx6_pcie, int addr, u16 data)
return 0;
}
-static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
+static int imx8mq_pcie_init_phy(struct imx6_pcie *imx6_pcie)
{
- switch (imx6_pcie->drvdata->variant) {
- case IMX8MM:
- case IMX8MM_EP:
- case IMX8MP:
- case IMX8MP_EP:
- /*
- * The PHY initialization had been done in the PHY
- * driver, break here directly.
- */
- break;
- case IMX8MQ:
- case IMX8MQ_EP:
- /*
- * TODO: Currently this code assumes external
- * oscillator is being used
- */
+ /*
+ * TODO: Currently this code assumes external
+ * oscillator is being used
+ */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ imx6_pcie_grp_offset(imx6_pcie),
+ IMX8MQ_GPR_PCIE_REF_USE_PAD,
+ IMX8MQ_GPR_PCIE_REF_USE_PAD);
+ /*
+ * Regarding the datasheet, the PCIE_VPH is suggested
+ * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
+ * VREG_BYPASS should be cleared to zero.
+ */
+ if (imx6_pcie->vph && regulator_get_voltage(imx6_pcie->vph) > 3000000)
regmap_update_bits(imx6_pcie->iomuxc_gpr,
imx6_pcie_grp_offset(imx6_pcie),
- IMX8MQ_GPR_PCIE_REF_USE_PAD,
- IMX8MQ_GPR_PCIE_REF_USE_PAD);
- /*
- * Regarding the datasheet, the PCIE_VPH is suggested
- * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
- * VREG_BYPASS should be cleared to zero.
- */
- if (imx6_pcie->vph &&
- regulator_get_voltage(imx6_pcie->vph) > 3000000)
- regmap_update_bits(imx6_pcie->iomuxc_gpr,
- imx6_pcie_grp_offset(imx6_pcie),
- IMX8MQ_GPR_PCIE_VREG_BYPASS,
- 0);
- break;
- case IMX7D:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX8MQ_GPR_PCIE_VREG_BYPASS,
+ 0);
+
+ return 0;
+}
+
+static int imx7d_pcie_init_phy(struct imx6_pcie *imx6_pcie)
+{
+ return regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
- break;
- case IMX6SX:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6SX_GPR12_PCIE_RX_EQ_MASK,
- IMX6SX_GPR12_PCIE_RX_EQ_2);
- fallthrough;
- default:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+}
+
+static int imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
+{
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
- /* configure constant input signal to the pcie ctrl and phy */
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
-
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_DEEMPH_GEN1,
- imx6_pcie->tx_deemph_gen1 << 0);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
- imx6_pcie->tx_deemph_gen2_3p5db << 6);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
- imx6_pcie->tx_deemph_gen2_6db << 12);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_SWING_FULL,
- imx6_pcie->tx_swing_full << 18);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_SWING_LOW,
- imx6_pcie->tx_swing_low << 25);
- break;
- }
+ /* configure constant input signal to the pcie ctrl and phy */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_DEEMPH_GEN1,
+ imx6_pcie->tx_deemph_gen1 << 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
+ imx6_pcie->tx_deemph_gen2_3p5db << 6);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
+ imx6_pcie->tx_deemph_gen2_6db << 12);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_SWING_FULL,
+ imx6_pcie->tx_swing_full << 18);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_SWING_LOW,
+ imx6_pcie->tx_swing_low << 25);
+ return 0;
+}
- imx6_pcie_configure_type(imx6_pcie);
+static int imx6sx_pcie_init_phy(struct imx6_pcie *imx6_pcie)
+{
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_PCIE_RX_EQ_MASK, IMX6SX_GPR12_PCIE_RX_EQ_2);
+
+ return imx6_pcie_init_phy(imx6_pcie);
}
static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
@@ -903,7 +900,11 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp)
}
imx6_pcie_assert_core_reset(imx6_pcie);
- imx6_pcie_init_phy(imx6_pcie);
+
+ if (imx6_pcie->drvdata->init_phy)
+ imx6_pcie->drvdata->init_phy(imx6_pcie);
+
+ imx6_pcie_configure_type(imx6_pcie);
ret = imx6_pcie_clk_enable(imx6_pcie);
if (ret) {
@@ -1374,6 +1375,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
.mode_off[0] = IOMUXC_GPR12,
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .init_phy = imx6_pcie_init_phy,
},
[IMX6SX] = {
.variant = IMX6SX,
@@ -1386,6 +1388,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
.mode_off[0] = IOMUXC_GPR12,
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .init_phy = imx6sx_pcie_init_phy,
},
[IMX6QP] = {
.variant = IMX6QP,
@@ -1399,6 +1402,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
.mode_off[0] = IOMUXC_GPR12,
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .init_phy = imx6_pcie_init_phy,
},
[IMX7D] = {
.variant = IMX7D,
@@ -1409,6 +1413,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.clk_names = {"pcie_bus", "pcie", "pcie_phy"},
.mode_off[0] = IOMUXC_GPR12,
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .init_phy = imx7d_pcie_init_phy,
},
[IMX8MQ] = {
.variant = IMX8MQ,
@@ -1420,6 +1425,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
.mode_off[1] = IOMUXC_GPR12,
.mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
+ .init_phy = imx8mq_pcie_init_phy,
},
[IMX8MM] = {
.variant = IMX8MM,
@@ -1452,6 +1458,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
.mode_off[1] = IOMUXC_GPR12,
.mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
+ .init_phy = imx8mq_pcie_init_phy,
},
[IMX8MM_EP] = {
.variant = IMX8MM_EP,
--
2.34.1
From: Richard Zhu <[email protected]>
Add i.MX95 PCIe "fsl,imx95-pcie" compatible string.
Add "atu" and "app" to reg-names.
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v6 to v7
- Added my sign off
Change from v5 to v6
- move atu and app after config
Change from v2 to v3
- Remove krzy's ACK tag
- Add condition check for imx95, which required more reg-names then old
platform, so need Krzy review again,
Change from v1 to v2
- add Krzy's ACK tag
.../bindings/pci/fsl,imx6q-pcie-common.yaml | 1 +
.../bindings/pci/fsl,imx6q-pcie.yaml | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
index a284a27c5e873..1b63089ff0aee 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
@@ -207,6 +207,7 @@ allOf:
- fsl,imx6sx-pcie
- fsl,imx6q-pcie
- fsl,imx6qp-pcie
+ - fsl,imx95-pcie
- fsl,imx6sx-pcie-ep
- fsl,imx6q-pcie-ep
- fsl,imx6qp-pcie-ep
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
index eeca6b7b540f9..8b8d77b1154b5 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -29,6 +29,7 @@ properties:
- fsl,imx8mq-pcie
- fsl,imx8mm-pcie
- fsl,imx8mp-pcie
+ - fsl,imx95-pcie
clocks:
minItems: 3
@@ -100,6 +101,23 @@ allOf:
- const: dbi
- const: config
+ - if:
+ properties:
+ compatible:
+ enum:
+ - fsl,imx95-pcie
+ then:
+ properties:
+ reg:
+ minItems: 4
+ maxItems: 4
+ reg-names:
+ items:
+ - const: dbi
+ - const: config
+ - const: atu
+ - const: app
+
- if:
properties:
compatible:
@@ -121,6 +139,7 @@ allOf:
compatible:
enum:
- fsl,imx8mq-pcie
+ - fsl,imx95-pcie
then:
properties:
clocks:
--
2.34.1
The common dw_pcie_ep_init() already do the same thing. Needn't platform
driver do it again.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v1 to v3
- new patches
drivers/pci/controller/dwc/pci-imx6.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 9e60ab6f1885a..4b2b9aafad1b4 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1080,7 +1080,6 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
int ret;
unsigned int pcie_dbi2_offset;
struct dw_pcie_ep *ep;
- struct resource *res;
struct dw_pcie *pci = imx6_pcie->pci;
struct dw_pcie_rp *pp = &pci->pp;
struct device *dev = pci->dev;
@@ -1099,14 +1098,8 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
pcie_dbi2_offset = SZ_4K;
break;
}
- pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
- if (!res)
- return -EINVAL;
- ep->phys_base = res->start;
- ep->addr_size = resource_size(res);
- ep->page_size = SZ_64K;
+ pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
ret = dw_pcie_ep_init(ep);
if (ret) {
--
2.34.1
Add i.MX95 PCIe "fsl,imx95-pcie-ep" compatible string.
Add reg-name: "atu", "dbi2", "dma" and "app".
Reuse PCI linux,pci-domain as controller id at endpoint.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v1 to v3
- new patches at v3
.../bindings/pci/fsl,imx6q-pcie-ep.yaml | 57 ++++++++++++++++---
1 file changed, 49 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
index ee155ed5f1811..f4d6ae5dab785 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
@@ -22,14 +22,7 @@ properties:
- fsl,imx8mm-pcie-ep
- fsl,imx8mq-pcie-ep
- fsl,imx8mp-pcie-ep
-
- reg:
- minItems: 2
-
- reg-names:
- items:
- - const: dbi
- - const: addr_space
+ - fsl,imx95-pcie-ep
clocks:
minItems: 3
@@ -62,11 +55,48 @@ required:
allOf:
- $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
- $ref: /schemas/pci/fsl,imx6q-pcie-common.yaml#
+ - if:
+ properties:
+ compatible:
+ enum:
+ - fsl,imx8mm-pcie-ep
+ - fsl,imx8mq-pcie-ep
+ - fsl,imx8mp-pcie-ep
+ then:
+ properties:
+ reg:
+ minItems: 2
+ maxItems: 2
+ reg-names:
+ items:
+ - const: dbi
+ - const: addr_space
+
+ - if:
+ properties:
+ compatible:
+ enum:
+ - fsl,imx95-pcie-ep
+ then:
+ properties:
+ reg:
+ minItems: 6
+ maxItems: 6
+ reg-names:
+ items:
+ - const: dbi
+ - const: atu
+ - const: dbi2
+ - const: app
+ - const: dma
+ - const: addr_space
+
- if:
properties:
compatible:
enum:
- fsl,imx8mq-pcie-ep
+ - fsl,imx95-pcie-ep
then:
properties:
clocks:
@@ -87,6 +117,17 @@ allOf:
- const: pcie_bus
- const: pcie_aux
+# reuse PCI linux,pci-domain as controller id at Endpoint
+ - if:
+ properties:
+ compatible:
+ enum:
+ - fsl,imx95-pcie-ep
+ then:
+ properties:
+ linux,pci-domain: true
+ required:
+ - linux,pci-domain
unevaluatedProperties: false
--
2.34.1
The i.MX EP exhibits variations in epc_features among different EP
configurations. This introduces the addition of epc_features in
imx6_pcie_drvdata to accommodate these differences. It's important to note
that there are no functional changes in this commit; instead, it lays the
groundwork for supporting i.MX95 EP functions.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v5 to v6
- add missed maxitems.
- add comments about reuse linux,pci-domain as controller id.
linux,pci-domain have not defined at PCI endpoint side.
Change from v1 to v3
- new patch at v3
drivers/pci/controller/dwc/pci-imx6.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 4b2b9aafad1b4..6a58fd63a9dd2 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -104,6 +104,7 @@ struct imx6_pcie_drvdata {
const u32 ltssm_mask;
const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
+ const struct pci_epc_features *epc_features;
int (*init_phy)(struct imx6_pcie *pcie);
};
@@ -1065,7 +1066,10 @@ static const struct pci_epc_features imx8m_pcie_epc_features = {
static const struct pci_epc_features*
imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
{
- return &imx8m_pcie_epc_features;
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+ return imx6_pcie->drvdata->epc_features;
}
static const struct dw_pcie_ep_ops pcie_ep_ops = {
@@ -1530,6 +1534,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
.mode_off[1] = IOMUXC_GPR12,
.mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
+ .epc_features = &imx8m_pcie_epc_features,
.init_phy = imx8mq_pcie_init_phy,
},
[IMX8MM_EP] = {
@@ -1540,6 +1545,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
.mode_off[0] = IOMUXC_GPR12,
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .epc_features = &imx8m_pcie_epc_features,
},
[IMX8MP_EP] = {
.variant = IMX8MP_EP,
@@ -1549,6 +1555,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
.mode_off[0] = IOMUXC_GPR12,
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .epc_features = &imx8m_pcie_epc_features,
},
};
--
2.34.1
Add iMX95 EP function support and add 64bit address support. Internal bus
bridge for PCI support 64bit dma address in iMX95. So set call
dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)).
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v3 to v4
- change align to 4k for imx95
Change from v1 to v3
- new patches at v3
drivers/pci/controller/dwc/pci-imx6.c | 45 +++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6a58fd63a9dd2..00ec59867c17b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -75,6 +75,7 @@ enum imx6_pcie_variants {
IMX8MQ_EP,
IMX8MM_EP,
IMX8MP_EP,
+ IMX95_EP,
};
#define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
@@ -84,6 +85,7 @@ enum imx6_pcie_variants {
#define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4)
#define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5)
#define IMX6_PCIE_FLAG_HAS_SERDES BIT(6)
+#define IMX6_PCIE_FLAG_SUPPORT_64BIT BIT(7)
#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
@@ -620,6 +622,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
break;
case IMX7D:
case IMX95:
+ case IMX95_EP:
break;
case IMX8MM:
case IMX8MM_EP:
@@ -1063,6 +1066,23 @@ static const struct pci_epc_features imx8m_pcie_epc_features = {
.align = SZ_64K,
};
+/*
+ * BAR# | Default BAR enable | Default BAR Type | Default BAR Size | BAR Sizing Scheme
+ * ================================================================================================
+ * BAR0 | Enable | 64-bit | 1 MB | Programmable Size
+ * BAR1 | Disable | 32-bit | 64 KB | Fixed Size
+ * | (BAR0 is 64-bit) | if BAR0 is 32-bit | | As Bar0 is 64bit
+ * BAR2 | Enable | 32-bit | 1 MB | Programmable Size
+ * BAR3 | Enable | 32-bit | 64 KB | Programmable Size
+ * BAR4 | Enable | 32-bit | 1M | Programmable Size
+ * BAR5 | Enable | 32-bit | 64 KB | Programmable Size
+ */
+static const struct pci_epc_features imx95_pcie_epc_features = {
+ .msi_capable = true,
+ .bar_fixed_size[1] = SZ_64K,
+ .align = SZ_4K,
+};
+
static const struct pci_epc_features*
imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
{
@@ -1105,6 +1125,14 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
+ /*
+ * db2 information should fetch from dtb file. dw_pcie_ep_init() can get dbi_base2 from
+ * "dbi2" if pci->dbi_base2 is NULL. All code related pcie_dbi2_offset should be removed
+ * after all dts added "dbi2" reg.
+ */
+ if (imx6_pcie->drvdata->variant == IMX95_EP)
+ pci->dbi_base2 = NULL;
+
ret = dw_pcie_ep_init(ep);
if (ret) {
dev_err(dev, "failed to initialize endpoint\n");
@@ -1355,6 +1383,9 @@ static int imx6_pcie_probe(struct platform_device *pdev)
"unable to find iomuxc registers\n");
}
+ if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_SUPPORT_64BIT))
+ dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+
/* Grab PCIe PHY Tx Settings */
if (of_property_read_u32(node, "fsl,tx-deemph-gen1",
&imx6_pcie->tx_deemph_gen1))
@@ -1557,6 +1588,19 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
.epc_features = &imx8m_pcie_epc_features,
},
+ [IMX95_EP] = {
+ .variant = IMX95_EP,
+ .flags = IMX6_PCIE_FLAG_HAS_SERDES |
+ IMX6_PCIE_FLAG_SUPPORT_64BIT,
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
+ .ltssm_off = IMX95_PE0_GEN_CTRL_3,
+ .ltssm_mask = IMX95_PCIE_LTSSM_EN,
+ .mode_off[0] = IMX95_PE0_GEN_CTRL_1,
+ .mode_mask[0] = IMX95_PCIE_DEVICE_TYPE,
+ .init_phy = imx95_pcie_init_phy,
+ .epc_features = &imx95_pcie_epc_features,
+ .mode = DW_PCIE_EP_TYPE,
+ },
};
static const struct of_device_id imx6_pcie_of_match[] = {
@@ -1571,6 +1615,7 @@ static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx8mq-pcie-ep", .data = &drvdata[IMX8MQ_EP], },
{ .compatible = "fsl,imx8mm-pcie-ep", .data = &drvdata[IMX8MM_EP], },
{ .compatible = "fsl,imx8mp-pcie-ep", .data = &drvdata[IMX8MP_EP], },
+ { .compatible = "fsl,imx95-pcie-ep", .data = &drvdata[IMX95_EP], },
{},
};
--
2.34.1
snps,dw-pcie.yaml already have reg and reg-name information. Needn't
duplciate here.
Add 'if' check for existed compatible string to restrict reg and reg-names.
This prepare to add new compatible string with difference reg-names sets.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v6 to v7
- add Krzysztof's review tag
Change from v5 to v6
- Add if check for existed compatible string
Change from v4 to v5
- add Rob's Acked
Change from v1 to v4:
- new patch at v4
.../bindings/pci/fsl,imx6q-pcie.yaml | 30 ++++++++++++-------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
index 81bbb8728f0f9..eeca6b7b540f9 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -30,16 +30,6 @@ properties:
- fsl,imx8mm-pcie
- fsl,imx8mp-pcie
- reg:
- items:
- - description: Data Bus Interface (DBI) registers.
- - description: PCIe configuration space region.
-
- reg-names:
- items:
- - const: dbi
- - const: config
-
clocks:
minItems: 3
items:
@@ -90,6 +80,26 @@ required:
allOf:
- $ref: /schemas/pci/snps,dw-pcie.yaml#
- $ref: /schemas/pci/fsl,imx6q-pcie-common.yaml#
+ - if:
+ properties:
+ compatible:
+ enum:
+ - fsl,imx6q-pcie
+ - fsl,imx6sx-pcie
+ - fsl,imx6qp-pcie
+ - fsl,imx7d-pcie
+ - fsl,imx8mq-pcie
+ - fsl,imx8mm-pcie
+ - fsl,imx8mp-pcie
+ then:
+ properties:
+ reg:
+ maxItems: 2
+ reg-names:
+ items:
+ - const: dbi
+ - const: config
+
- if:
properties:
compatible:
--
2.34.1
The bindings referencing this file already define these constraints for
each of the variants, so the if not: then: is redundant.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v6 to v7
- rewrite git commit message by using simple words
Change from v5 to v6
- rewrite git commit message and explain why remove it safely.
- Add Rob's Ack
Change from v1 to v4
- new patch at v4
.../bindings/pci/fsl,imx6q-pcie-common.yaml | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
index 8f39b4e6e8491..a284a27c5e873 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
@@ -150,22 +150,6 @@ allOf:
- {}
- const: pcie_phy
- const: pcie_aux
- - if:
- properties:
- compatible:
- not:
- contains:
- enum:
- - fsl,imx6sx-pcie
- - fsl,imx8mq-pcie
- - fsl,imx6sx-pcie-ep
- - fsl,imx8mq-pcie-ep
- then:
- properties:
- clocks:
- maxItems: 3
- clock-names:
- maxItems: 3
- if:
properties:
--
2.34.1
Add drvdata::mode_off and drvdata::mode_mask to simple
imx6_pcie_configure_type() logic.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v2 to v3
- none
Change from v1 to v2
- use ffs() to fixe build error.
drivers/pci/controller/dwc/pci-imx6.c | 60 ++++++++++++++++++---------
1 file changed, 40 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 588bfb616260e..717e8fa030deb 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -68,6 +68,7 @@ enum imx6_pcie_variants {
#define IMX6_PCIE_MAX_CLKS 6
+#define IMX6_PCIE_MAX_INSTANCES 2
struct imx6_pcie_drvdata {
enum imx6_pcie_variants variant;
enum dw_pcie_device_mode mode;
@@ -77,6 +78,8 @@ struct imx6_pcie_drvdata {
const char *clk_names[IMX6_PCIE_MAX_CLKS];
const u32 ltssm_off;
const u32 ltssm_mask;
+ const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
+ const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
};
struct imx6_pcie {
@@ -174,32 +177,25 @@ static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
{
- unsigned int mask, val, mode;
+ const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
+ unsigned int mask, val, mode, id;
- if (imx6_pcie->drvdata->mode == DW_PCIE_EP_TYPE)
+ if (drvdata->mode == DW_PCIE_EP_TYPE)
mode = PCI_EXP_TYPE_ENDPOINT;
else
mode = PCI_EXP_TYPE_ROOT_PORT;
- switch (imx6_pcie->drvdata->variant) {
- case IMX8MQ:
- case IMX8MQ_EP:
- if (imx6_pcie->controller_id == 1) {
- mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
- val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
- mode);
- } else {
- mask = IMX6Q_GPR12_DEVICE_TYPE;
- val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
- }
- break;
- default:
- mask = IMX6Q_GPR12_DEVICE_TYPE;
- val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
- break;
- }
+ id = imx6_pcie->controller_id;
+
+ /* If mode_mask[id] is zero, means each controller have its individual gpr */
+ if (!drvdata->mode_mask[id])
+ id = 0;
+
+ mask = drvdata->mode_mask[id];
+ /* FIELD_PREP mask have been constant */
+ val = mode << (ffs(mask) - 1);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->mode_off[id], mask, val);
}
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
@@ -1376,6 +1372,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.clk_names = {"pcie_bus", "pcie", "pcie_phy"},
.ltssm_off = IOMUXC_GPR12,
.ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
[IMX6SX] = {
.variant = IMX6SX,
@@ -1386,6 +1384,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
.ltssm_off = IOMUXC_GPR12,
.ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
[IMX6QP] = {
.variant = IMX6QP,
@@ -1397,6 +1397,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.clk_names = {"pcie_bus", "pcie", "pcie_phy"},
.ltssm_off = IOMUXC_GPR12,
.ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
[IMX7D] = {
.variant = IMX7D,
@@ -1405,6 +1407,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_HAS_PHY_RESET,
.gpr = "fsl,imx7d-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy"},
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
[IMX8MQ] = {
.variant = IMX8MQ,
@@ -1412,6 +1416,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_HAS_PHY_RESET,
.gpr = "fsl,imx8mq-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .mode_off[1] = IOMUXC_GPR12,
+ .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
},
[IMX8MM] = {
.variant = IMX8MM,
@@ -1420,6 +1428,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_HAS_APP_RESET,
.gpr = "fsl,imx8mm-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
[IMX8MP] = {
.variant = IMX8MP,
@@ -1428,6 +1438,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
IMX6_PCIE_FLAG_HAS_APP_RESET,
.gpr = "fsl,imx8mp-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
[IMX8MQ_EP] = {
.variant = IMX8MQ_EP,
@@ -1436,6 +1448,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mq-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
+ .mode_off[1] = IOMUXC_GPR12,
+ .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
},
[IMX8MM_EP] = {
.variant = IMX8MM_EP,
@@ -1443,6 +1459,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mm-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
[IMX8MP_EP] = {
.variant = IMX8MP_EP,
@@ -1450,6 +1468,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode = DW_PCIE_EP_TYPE,
.gpr = "fsl,imx8mp-iomuxc-gpr",
.clk_names = {"pcie_bus", "pcie", "pcie_aux"},
+ .mode_off[0] = IOMUXC_GPR12,
+ .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
};
--
2.34.1
Add iMX95 PCIe basic root complex function support.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
Change from v1 to v3
- none
drivers/pci/controller/dwc/pci-imx6.c | 90 +++++++++++++++++++++++++--
1 file changed, 85 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index d66a2db53bdb7..9e60ab6f1885a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -42,6 +42,25 @@
#define IMX8MQ_GPR_PCIE_VREG_BYPASS BIT(12)
#define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
+#define IMX95_PCIE_PHY_GEN_CTRL 0x0
+#define IMX95_PCIE_REF_USE_PAD BIT(17)
+
+#define IMX95_PCIE_PHY_MPLLA_CTRL 0x10
+#define IMX95_PCIE_PHY_MPLL_STATE BIT(30)
+
+#define IMX95_PCIE_SS_RW_REG_0 0xf0
+#define IMX95_PCIE_REF_CLKEN BIT(23)
+#define IMX95_PCIE_PHY_CR_PARA_SEL BIT(9)
+
+#define IMX95_PE0_GEN_CTRL_1 0x1050
+#define IMX95_PCIE_DEVICE_TYPE GENMASK(3, 0)
+
+#define IMX95_PE0_GEN_CTRL_3 0x1058
+#define IMX95_PCIE_LTSSM_EN BIT(0)
+
+#define IMX95_PE0_PM_STS 0x1064
+#define IMX95_PCIE_PM_LINKST_IN_L2 BIT(14)
+
#define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
enum imx6_pcie_variants {
@@ -52,6 +71,7 @@ enum imx6_pcie_variants {
IMX8MQ,
IMX8MM,
IMX8MP,
+ IMX95,
IMX8MQ_EP,
IMX8MM_EP,
IMX8MP_EP,
@@ -63,6 +83,7 @@ enum imx6_pcie_variants {
#define IMX6_PCIE_FLAG_HAS_PHY BIT(3)
#define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4)
#define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5)
+#define IMX6_PCIE_FLAG_HAS_SERDES BIT(6)
#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
@@ -179,6 +200,24 @@ static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
}
+static int imx95_pcie_init_phy(struct imx6_pcie *imx6_pcie)
+{
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX95_PCIE_SS_RW_REG_0,
+ IMX95_PCIE_PHY_CR_PARA_SEL,
+ IMX95_PCIE_PHY_CR_PARA_SEL);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX95_PCIE_PHY_GEN_CTRL,
+ IMX95_PCIE_REF_USE_PAD, 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IMX95_PCIE_SS_RW_REG_0,
+ IMX95_PCIE_REF_CLKEN,
+ IMX95_PCIE_REF_CLKEN);
+
+ return 0;
+}
+
static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
{
const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
@@ -579,6 +618,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
break;
case IMX7D:
+ case IMX95:
break;
case IMX8MM:
case IMX8MM_EP:
@@ -696,10 +736,19 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
{
struct dw_pcie *pci = imx6_pcie->pci;
struct device *dev = pci->dev;
+ u32 val;
reset_control_deassert(imx6_pcie->pciephy_reset);
switch (imx6_pcie->drvdata->variant) {
+ case IMX95:
+ /* Polling the MPLL_STATE */
+ if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
+ IMX95_PCIE_PHY_MPLLA_CTRL, val,
+ val & IMX95_PCIE_PHY_MPLL_STATE,
+ 10, 10000))
+ dev_err(dev, "PCIe PLL lock timeout\n");
+ break;
case IMX7D:
/* Workaround for ERR010728, failure of PCI-e PLL VCO to
* oscillate, especially when cold. This turns off "Duty-cycle
@@ -1281,12 +1330,32 @@ static int imx6_pcie_probe(struct platform_device *pdev)
return PTR_ERR(imx6_pcie->turnoff_reset);
}
+ if (imx6_pcie->drvdata->gpr) {
/* Grab GPR config register range */
- imx6_pcie->iomuxc_gpr =
- syscon_regmap_lookup_by_compatible(imx6_pcie->drvdata->gpr);
- if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
- dev_err(dev, "unable to find iomuxc registers\n");
- return PTR_ERR(imx6_pcie->iomuxc_gpr);
+ imx6_pcie->iomuxc_gpr =
+ syscon_regmap_lookup_by_compatible(imx6_pcie->drvdata->gpr);
+ if (IS_ERR(imx6_pcie->iomuxc_gpr))
+ return dev_err_probe(dev, PTR_ERR(imx6_pcie->iomuxc_gpr),
+ "unable to find iomuxc registers\n");
+ }
+
+ if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_SERDES)) {
+ void __iomem *off = devm_platform_ioremap_resource_byname(pdev, "app");
+
+ if (IS_ERR(off))
+ return dev_err_probe(dev, PTR_ERR(off),
+ "unable to find serdes registers\n");
+
+ static struct regmap_config regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ };
+
+ imx6_pcie->iomuxc_gpr = devm_regmap_init_mmio(dev, off, ®map_config);
+ if (IS_ERR(imx6_pcie->iomuxc_gpr))
+ return dev_err_probe(dev, PTR_ERR(imx6_pcie->iomuxc_gpr),
+ "unable to find iomuxc registers\n");
}
/* Grab PCIe PHY Tx Settings */
@@ -1447,6 +1516,16 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.mode_off[0] = IOMUXC_GPR12,
.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
},
+ [IMX95] = {
+ .variant = IMX95,
+ .flags = IMX6_PCIE_FLAG_HAS_SERDES,
+ .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
+ .ltssm_off = IMX95_PE0_GEN_CTRL_3,
+ .ltssm_mask = IMX95_PCIE_LTSSM_EN,
+ .mode_off[0] = IMX95_PE0_GEN_CTRL_1,
+ .mode_mask[0] = IMX95_PCIE_DEVICE_TYPE,
+ .init_phy = imx95_pcie_init_phy,
+ },
[IMX8MQ_EP] = {
.variant = IMX8MQ_EP,
.flags = IMX6_PCIE_FLAG_HAS_APP_RESET |
@@ -1488,6 +1567,7 @@ static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx8mq-pcie", .data = &drvdata[IMX8MQ], },
{ .compatible = "fsl,imx8mm-pcie", .data = &drvdata[IMX8MM], },
{ .compatible = "fsl,imx8mp-pcie", .data = &drvdata[IMX8MP], },
+ { .compatible = "fsl,imx95-pcie", .data = &drvdata[IMX95], },
{ .compatible = "fsl,imx8mq-pcie-ep", .data = &drvdata[IMX8MQ_EP], },
{ .compatible = "fsl,imx8mm-pcie-ep", .data = &drvdata[IMX8MM_EP], },
{ .compatible = "fsl,imx8mp-pcie-ep", .data = &drvdata[IMX8MP_EP], },
--
2.34.1
Hi Frank,
On 23-12-27, Frank Li wrote:
> Refactors the clock handling logic. Adds clk_names[] define in drvdata.
> Using clk_bulk*() api simplifies the code.
does this influence the clock enable/disable sequence ordering? Just
asking to avoid regressions on older platforms which may require some
sort of order (e.g. require clock-a before clock-b).
Regards,
Marco
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v4 to v5
> - update commit message
> - direct using clk name list, instead of macro
> - still keep caculate clk list count because sizeof return pre allocated
> array size.
>
> Change from v3 to v4
> - using clk_bulk_*() API
> Change from v1 to v3
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 125 ++++++++------------------
> 1 file changed, 35 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 74703362aeec7..50d9faaa17f71 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -61,12 +61,15 @@ enum imx6_pcie_variants {
> #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
>
> +#define IMX6_PCIE_MAX_CLKS 6
> +
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> enum dw_pcie_device_mode mode;
> u32 flags;
> int dbi_length;
> const char *gpr;
> + const char *clk_names[IMX6_PCIE_MAX_CLKS];
> };
>
> struct imx6_pcie {
> @@ -74,11 +77,8 @@ struct imx6_pcie {
> int reset_gpio;
> bool gpio_active_high;
> bool link_is_up;
> - struct clk *pcie_bus;
> - struct clk *pcie_phy;
> - struct clk *pcie_inbound_axi;
> - struct clk *pcie;
> - struct clk *pcie_aux;
> + struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
> + u32 clks_cnt;
> struct regmap *iomuxc_gpr;
> u16 msi_ctrl;
> u32 controller_id;
> @@ -407,13 +407,18 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
>
> static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
> {
> - unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
> + unsigned long phy_rate = 0;
> int mult, div;
> u16 val;
> + int i;
>
> if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
> return 0;
>
> + for (i = 0; i < imx6_pcie->clks_cnt; i++)
> + if (strncmp(imx6_pcie->clks[i].id, "pcie_phy", 8) == 0)
> + phy_rate = clk_get_rate(imx6_pcie->clks[i].clk);
> +
> switch (phy_rate) {
> case 125000000:
> /*
> @@ -550,19 +555,11 @@ static int imx6_pcie_attach_pd(struct device *dev)
>
> static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> {
> - struct dw_pcie *pci = imx6_pcie->pci;
> - struct device *dev = pci->dev;
> unsigned int offset;
> int ret = 0;
>
> switch (imx6_pcie->drvdata->variant) {
> case IMX6SX:
> - ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_axi clock\n");
> - break;
> - }
> -
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0);
> break;
> @@ -589,12 +586,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> case IMX8MQ_EP:
> case IMX8MP:
> case IMX8MP_EP:
> - ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_aux clock\n");
> - break;
> - }
> -
> offset = imx6_pcie_grp_offset(imx6_pcie);
> /*
> * Set the over ride low and enabled
> @@ -615,9 +606,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> {
> switch (imx6_pcie->drvdata->variant) {
> - case IMX6SX:
> - clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> - break;
> case IMX6QP:
> case IMX6Q:
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> @@ -631,14 +619,6 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> break;
> - case IMX8MM:
> - case IMX8MM_EP:
> - case IMX8MQ:
> - case IMX8MQ_EP:
> - case IMX8MP:
> - case IMX8MP_EP:
> - clk_disable_unprepare(imx6_pcie->pcie_aux);
> - break;
> default:
> break;
> }
> @@ -650,23 +630,9 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> struct device *dev = pci->dev;
> int ret;
>
> - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_phy clock\n");
> + ret = clk_bulk_prepare_enable(imx6_pcie->clks_cnt, imx6_pcie->clks);
> + if (ret)
> return ret;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_bus clock\n");
> - goto err_pcie_bus;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie clock\n");
> - goto err_pcie;
> - }
>
> ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> if (ret) {
> @@ -679,11 +645,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> return 0;
>
> err_ref_clk:
> - clk_disable_unprepare(imx6_pcie->pcie);
> -err_pcie:
> - clk_disable_unprepare(imx6_pcie->pcie_bus);
> -err_pcie_bus:
> - clk_disable_unprepare(imx6_pcie->pcie_phy);
> + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
>
> return ret;
> }
> @@ -691,9 +653,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> {
> imx6_pcie_disable_ref_clk(imx6_pcie);
> - clk_disable_unprepare(imx6_pcie->pcie);
> - clk_disable_unprepare(imx6_pcie->pcie_bus);
> - clk_disable_unprepare(imx6_pcie->pcie_phy);
> + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
> }
>
> static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> @@ -1305,32 +1265,19 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> return imx6_pcie->reset_gpio;
> }
>
> - /* Fetch clocks */
> - imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> - if (IS_ERR(imx6_pcie->pcie_bus))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
> - "pcie_bus clock source missing or invalid\n");
> + while (imx6_pcie->drvdata->clk_names[imx6_pcie->clks_cnt]) {
> + int i = imx6_pcie->clks_cnt;
> +
> + imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
> + imx6_pcie->clks_cnt++;
> + }
>
> - imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> - if (IS_ERR(imx6_pcie->pcie))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> - "pcie clock source missing or invalid\n");
> + /* Fetch clocks */
> + ret = devm_clk_bulk_get(dev, imx6_pcie->clks_cnt, imx6_pcie->clks);
> + if (ret)
> + return ret;
>
> switch (imx6_pcie->drvdata->variant) {
> - case IMX6SX:
> - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> - "pcie_inbound_axi");
> - if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> - "pcie_inbound_axi clock missing or invalid\n");
> - break;
> - case IMX8MQ:
> - case IMX8MQ_EP:
> - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> - if (IS_ERR(imx6_pcie->pcie_aux))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> - "pcie_aux clock source missing or invalid\n");
> - fallthrough;
> case IMX7D:
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> imx6_pcie->controller_id = 1;
> @@ -1353,10 +1300,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> case IMX8MM_EP:
> case IMX8MP:
> case IMX8MP_EP:
> - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> - if (IS_ERR(imx6_pcie->pcie_aux))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> - "pcie_aux clock source missing or invalid\n");
> imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> "apps");
> if (IS_ERR(imx6_pcie->apps_reset))
> @@ -1372,14 +1315,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> default:
> break;
> }
> - /* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> - if (imx6_pcie->phy == NULL) {
> - imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> - if (IS_ERR(imx6_pcie->pcie_phy))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> - "pcie_phy clock source missing or invalid\n");
> - }
> -
>
> /* Grab turnoff reset */
> imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> @@ -1477,6 +1412,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> .dbi_length = 0x200,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> },
> [IMX6SX] = {
> .variant = IMX6SX,
> @@ -1484,6 +1420,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
> IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> },
> [IMX6QP] = {
> .variant = IMX6QP,
> @@ -1492,40 +1429,48 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .dbi_length = 0x200,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> },
> [IMX7D] = {
> .variant = IMX7D,
> .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx7d-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> },
> [IMX8MQ] = {
> .variant = IMX8MQ,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> },
> [IMX8MM] = {
> .variant = IMX8MM,
> .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MP] = {
> .variant = IMX8MP,
> .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MQ_EP] = {
> .variant = IMX8MQ_EP,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> },
> [IMX8MM_EP] = {
> .variant = IMX8MM_EP,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MP_EP] = {
> .variant = IMX8MP_EP,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> };
>
> --
> 2.34.1
>
>
>
On Wed, 27 Dec 2023 13:27:22 -0500, Frank Li wrote:
> From: Richard Zhu <[email protected]>
>
> Add i.MX95 PCIe "fsl,imx95-pcie" compatible string.
> Add "atu" and "app" to reg-names.
>
> Signed-off-by: Richard Zhu <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v6 to v7
> - Added my sign off
>
> Change from v5 to v6
> - move atu and app after config
>
> Change from v2 to v3
> - Remove krzy's ACK tag
> - Add condition check for imx95, which required more reg-names then old
> platform, so need Krzy review again,
>
> Change from v1 to v2
> - add Krzy's ACK tag
>
> .../bindings/pci/fsl,imx6q-pcie-common.yaml | 1 +
> .../bindings/pci/fsl,imx6q-pcie.yaml | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Tue, Jan 02, 2024 at 09:47:44AM +0100, Marco Felsch wrote:
> Hi Frank,
>
> On 23-12-27, Frank Li wrote:
> > Refactors the clock handling logic. Adds clk_names[] define in drvdata.
> > Using clk_bulk*() api simplifies the code.
>
> does this influence the clock enable/disable sequence ordering? Just
> asking to avoid regressions on older platforms which may require some
> sort of order (e.g. require clock-a before clock-b).
drvdata::clk_names is order of enble sequence. So far we have not found
the problem.
Frank Li
>
> Regards,
> Marco
>
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> >
> > Notes:
> > Change from v4 to v5
> > - update commit message
> > - direct using clk name list, instead of macro
> > - still keep caculate clk list count because sizeof return pre allocated
> > array size.
> >
> > Change from v3 to v4
> > - using clk_bulk_*() API
> > Change from v1 to v3
> > - none
> >
> > drivers/pci/controller/dwc/pci-imx6.c | 125 ++++++++------------------
> > 1 file changed, 35 insertions(+), 90 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 74703362aeec7..50d9faaa17f71 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -61,12 +61,15 @@ enum imx6_pcie_variants {
> > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> > #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
> >
> > +#define IMX6_PCIE_MAX_CLKS 6
> > +
> > struct imx6_pcie_drvdata {
> > enum imx6_pcie_variants variant;
> > enum dw_pcie_device_mode mode;
> > u32 flags;
> > int dbi_length;
> > const char *gpr;
> > + const char *clk_names[IMX6_PCIE_MAX_CLKS];
> > };
> >
> > struct imx6_pcie {
> > @@ -74,11 +77,8 @@ struct imx6_pcie {
> > int reset_gpio;
> > bool gpio_active_high;
> > bool link_is_up;
> > - struct clk *pcie_bus;
> > - struct clk *pcie_phy;
> > - struct clk *pcie_inbound_axi;
> > - struct clk *pcie;
> > - struct clk *pcie_aux;
> > + struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
> > + u32 clks_cnt;
> > struct regmap *iomuxc_gpr;
> > u16 msi_ctrl;
> > u32 controller_id;
> > @@ -407,13 +407,18 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> >
> > static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
> > {
> > - unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
> > + unsigned long phy_rate = 0;
> > int mult, div;
> > u16 val;
> > + int i;
> >
> > if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
> > return 0;
> >
> > + for (i = 0; i < imx6_pcie->clks_cnt; i++)
> > + if (strncmp(imx6_pcie->clks[i].id, "pcie_phy", 8) == 0)
> > + phy_rate = clk_get_rate(imx6_pcie->clks[i].clk);
> > +
> > switch (phy_rate) {
> > case 125000000:
> > /*
> > @@ -550,19 +555,11 @@ static int imx6_pcie_attach_pd(struct device *dev)
> >
> > static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > {
> > - struct dw_pcie *pci = imx6_pcie->pci;
> > - struct device *dev = pci->dev;
> > unsigned int offset;
> > int ret = 0;
> >
> > switch (imx6_pcie->drvdata->variant) {
> > case IMX6SX:
> > - ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_axi clock\n");
> > - break;
> > - }
> > -
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0);
> > break;
> > @@ -589,12 +586,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > case IMX8MQ_EP:
> > case IMX8MP:
> > case IMX8MP_EP:
> > - ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_aux clock\n");
> > - break;
> > - }
> > -
> > offset = imx6_pcie_grp_offset(imx6_pcie);
> > /*
> > * Set the over ride low and enabled
> > @@ -615,9 +606,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> > {
> > switch (imx6_pcie->drvdata->variant) {
> > - case IMX6SX:
> > - clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > - break;
> > case IMX6QP:
> > case IMX6Q:
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > @@ -631,14 +619,6 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > break;
> > - case IMX8MM:
> > - case IMX8MM_EP:
> > - case IMX8MQ:
> > - case IMX8MQ_EP:
> > - case IMX8MP:
> > - case IMX8MP_EP:
> > - clk_disable_unprepare(imx6_pcie->pcie_aux);
> > - break;
> > default:
> > break;
> > }
> > @@ -650,23 +630,9 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> > struct device *dev = pci->dev;
> > int ret;
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_phy clock\n");
> > + ret = clk_bulk_prepare_enable(imx6_pcie->clks_cnt, imx6_pcie->clks);
> > + if (ret)
> > return ret;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_bus clock\n");
> > - goto err_pcie_bus;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie clock\n");
> > - goto err_pcie;
> > - }
> >
> > ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > if (ret) {
> > @@ -679,11 +645,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> > return 0;
> >
> > err_ref_clk:
> > - clk_disable_unprepare(imx6_pcie->pcie);
> > -err_pcie:
> > - clk_disable_unprepare(imx6_pcie->pcie_bus);
> > -err_pcie_bus:
> > - clk_disable_unprepare(imx6_pcie->pcie_phy);
> > + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
> >
> > return ret;
> > }
> > @@ -691,9 +653,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> > static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> > {
> > imx6_pcie_disable_ref_clk(imx6_pcie);
> > - clk_disable_unprepare(imx6_pcie->pcie);
> > - clk_disable_unprepare(imx6_pcie->pcie_bus);
> > - clk_disable_unprepare(imx6_pcie->pcie_phy);
> > + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
> > }
> >
> > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > @@ -1305,32 +1265,19 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > return imx6_pcie->reset_gpio;
> > }
> >
> > - /* Fetch clocks */
> > - imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> > - if (IS_ERR(imx6_pcie->pcie_bus))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
> > - "pcie_bus clock source missing or invalid\n");
> > + while (imx6_pcie->drvdata->clk_names[imx6_pcie->clks_cnt]) {
> > + int i = imx6_pcie->clks_cnt;
> > +
> > + imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
> > + imx6_pcie->clks_cnt++;
> > + }
> >
> > - imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> > - if (IS_ERR(imx6_pcie->pcie))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> > - "pcie clock source missing or invalid\n");
> > + /* Fetch clocks */
> > + ret = devm_clk_bulk_get(dev, imx6_pcie->clks_cnt, imx6_pcie->clks);
> > + if (ret)
> > + return ret;
> >
> > switch (imx6_pcie->drvdata->variant) {
> > - case IMX6SX:
> > - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> > - "pcie_inbound_axi");
> > - if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> > - "pcie_inbound_axi clock missing or invalid\n");
> > - break;
> > - case IMX8MQ:
> > - case IMX8MQ_EP:
> > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > - if (IS_ERR(imx6_pcie->pcie_aux))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > - "pcie_aux clock source missing or invalid\n");
> > - fallthrough;
> > case IMX7D:
> > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > imx6_pcie->controller_id = 1;
> > @@ -1353,10 +1300,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > case IMX8MM_EP:
> > case IMX8MP:
> > case IMX8MP_EP:
> > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > - if (IS_ERR(imx6_pcie->pcie_aux))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > - "pcie_aux clock source missing or invalid\n");
> > imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > "apps");
> > if (IS_ERR(imx6_pcie->apps_reset))
> > @@ -1372,14 +1315,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > default:
> > break;
> > }
> > - /* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> > - if (imx6_pcie->phy == NULL) {
> > - imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> > - if (IS_ERR(imx6_pcie->pcie_phy))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> > - "pcie_phy clock source missing or invalid\n");
> > - }
> > -
> >
> > /* Grab turnoff reset */
> > imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> > @@ -1477,6 +1412,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> > .dbi_length = 0x200,
> > .gpr = "fsl,imx6q-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > },
> > [IMX6SX] = {
> > .variant = IMX6SX,
> > @@ -1484,6 +1420,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
> > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx6q-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> > },
> > [IMX6QP] = {
> > .variant = IMX6QP,
> > @@ -1492,40 +1429,48 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .dbi_length = 0x200,
> > .gpr = "fsl,imx6q-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > },
> > [IMX7D] = {
> > .variant = IMX7D,
> > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx7d-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > },
> > [IMX8MQ] = {
> > .variant = IMX8MQ,
> > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > },
> > [IMX8MM] = {
> > .variant = IMX8MM,
> > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MP] = {
> > .variant = IMX8MP,
> > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MQ_EP] = {
> > .variant = IMX8MQ_EP,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > },
> > [IMX8MM_EP] = {
> > .variant = IMX8MM_EP,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MP_EP] = {
> > .variant = IMX8MP_EP,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > };
> >
> > --
> > 2.34.1
> >
> >
> >
On 24-01-03, Frank Li wrote:
> On Tue, Jan 02, 2024 at 09:47:44AM +0100, Marco Felsch wrote:
> > Hi Frank,
> >
> > On 23-12-27, Frank Li wrote:
> > > Refactors the clock handling logic. Adds clk_names[] define in drvdata.
> > > Using clk_bulk*() api simplifies the code.
> >
> > does this influence the clock enable/disable sequence ordering? Just
> > asking to avoid regressions on older platforms which may require some
> > sort of order (e.g. require clock-a before clock-b).
>
> drvdata::clk_names is order of enble sequence. So far we have not found
> the problem.
Okay, thanks.
Regards,
Marco
On Wed, Dec 27, 2023 at 01:27:12PM -0500, Frank Li wrote:
Subject mentions, 'bulk_clk' APIs but there is no such thing. It should be
'clk_bulk()' APIs.
> Refactors the clock handling logic. Adds clk_names[] define in drvdata.
> Using clk_bulk*() api simplifies the code.
>
I've mentioned this many times in the past. But let me reiterate here again:
Commit message should be in imperative mood as per Linux Kernel rules for
submitting patches. Please see here:
Documentation/process/submitting-patches.rst
The relevant part is:
"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."
Please use this same format for rest of the patches as well for future ones.
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v4 to v5
> - update commit message
> - direct using clk name list, instead of macro
> - still keep caculate clk list count because sizeof return pre allocated
> array size.
>
> Change from v3 to v4
> - using clk_bulk_*() API
> Change from v1 to v3
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 125 ++++++++------------------
> 1 file changed, 35 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 74703362aeec7..50d9faaa17f71 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -61,12 +61,15 @@ enum imx6_pcie_variants {
> #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
>
> +#define IMX6_PCIE_MAX_CLKS 6
> +
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> enum dw_pcie_device_mode mode;
> u32 flags;
> int dbi_length;
> const char *gpr;
> + const char *clk_names[IMX6_PCIE_MAX_CLKS];
> };
>
> struct imx6_pcie {
> @@ -74,11 +77,8 @@ struct imx6_pcie {
> int reset_gpio;
> bool gpio_active_high;
> bool link_is_up;
> - struct clk *pcie_bus;
> - struct clk *pcie_phy;
> - struct clk *pcie_inbound_axi;
> - struct clk *pcie;
> - struct clk *pcie_aux;
> + struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
> + u32 clks_cnt;
> struct regmap *iomuxc_gpr;
> u16 msi_ctrl;
> u32 controller_id;
> @@ -407,13 +407,18 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
>
> static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
> {
> - unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
> + unsigned long phy_rate = 0;
> int mult, div;
> u16 val;
> + int i;
>
> if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
> return 0;
>
> + for (i = 0; i < imx6_pcie->clks_cnt; i++)
> + if (strncmp(imx6_pcie->clks[i].id, "pcie_phy", 8) == 0)
> + phy_rate = clk_get_rate(imx6_pcie->clks[i].clk);
> +
> switch (phy_rate) {
> case 125000000:
> /*
> @@ -550,19 +555,11 @@ static int imx6_pcie_attach_pd(struct device *dev)
>
> static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> {
> - struct dw_pcie *pci = imx6_pcie->pci;
> - struct device *dev = pci->dev;
> unsigned int offset;
> int ret = 0;
>
> switch (imx6_pcie->drvdata->variant) {
> case IMX6SX:
> - ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_axi clock\n");
> - break;
> - }
> -
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0);
> break;
> @@ -589,12 +586,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> case IMX8MQ_EP:
> case IMX8MP:
> case IMX8MP_EP:
> - ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_aux clock\n");
> - break;
> - }
> -
> offset = imx6_pcie_grp_offset(imx6_pcie);
> /*
> * Set the over ride low and enabled
> @@ -615,9 +606,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> {
> switch (imx6_pcie->drvdata->variant) {
> - case IMX6SX:
> - clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> - break;
> case IMX6QP:
> case IMX6Q:
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> @@ -631,14 +619,6 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> break;
> - case IMX8MM:
> - case IMX8MM_EP:
> - case IMX8MQ:
> - case IMX8MQ_EP:
> - case IMX8MP:
> - case IMX8MP_EP:
> - clk_disable_unprepare(imx6_pcie->pcie_aux);
> - break;
> default:
> break;
> }
> @@ -650,23 +630,9 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> struct device *dev = pci->dev;
> int ret;
>
> - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_phy clock\n");
> + ret = clk_bulk_prepare_enable(imx6_pcie->clks_cnt, imx6_pcie->clks);
Extra space after =
> + if (ret)
> return ret;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_bus clock\n");
> - goto err_pcie_bus;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie clock\n");
> - goto err_pcie;
> - }
>
> ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> if (ret) {
> @@ -679,11 +645,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> return 0;
>
> err_ref_clk:
> - clk_disable_unprepare(imx6_pcie->pcie);
> -err_pcie:
> - clk_disable_unprepare(imx6_pcie->pcie_bus);
> -err_pcie_bus:
> - clk_disable_unprepare(imx6_pcie->pcie_phy);
> + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
>
> return ret;
> }
> @@ -691,9 +653,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> {
> imx6_pcie_disable_ref_clk(imx6_pcie);
> - clk_disable_unprepare(imx6_pcie->pcie);
> - clk_disable_unprepare(imx6_pcie->pcie_bus);
> - clk_disable_unprepare(imx6_pcie->pcie_phy);
> + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
> }
>
> static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> @@ -1305,32 +1265,19 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> return imx6_pcie->reset_gpio;
> }
>
> - /* Fetch clocks */
> - imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> - if (IS_ERR(imx6_pcie->pcie_bus))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
> - "pcie_bus clock source missing or invalid\n");
> + while (imx6_pcie->drvdata->clk_names[imx6_pcie->clks_cnt]) {
> + int i = imx6_pcie->clks_cnt;
Why can't you initialize i to 0 directly?
Rest looks good to me.
- Mani
> +
> + imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
> + imx6_pcie->clks_cnt++;
> + }
>
> - imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> - if (IS_ERR(imx6_pcie->pcie))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> - "pcie clock source missing or invalid\n");
> + /* Fetch clocks */
> + ret = devm_clk_bulk_get(dev, imx6_pcie->clks_cnt, imx6_pcie->clks);
> + if (ret)
> + return ret;
>
> switch (imx6_pcie->drvdata->variant) {
> - case IMX6SX:
> - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> - "pcie_inbound_axi");
> - if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> - "pcie_inbound_axi clock missing or invalid\n");
> - break;
> - case IMX8MQ:
> - case IMX8MQ_EP:
> - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> - if (IS_ERR(imx6_pcie->pcie_aux))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> - "pcie_aux clock source missing or invalid\n");
> - fallthrough;
> case IMX7D:
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> imx6_pcie->controller_id = 1;
> @@ -1353,10 +1300,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> case IMX8MM_EP:
> case IMX8MP:
> case IMX8MP_EP:
> - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> - if (IS_ERR(imx6_pcie->pcie_aux))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> - "pcie_aux clock source missing or invalid\n");
> imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> "apps");
> if (IS_ERR(imx6_pcie->apps_reset))
> @@ -1372,14 +1315,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> default:
> break;
> }
> - /* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> - if (imx6_pcie->phy == NULL) {
> - imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> - if (IS_ERR(imx6_pcie->pcie_phy))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> - "pcie_phy clock source missing or invalid\n");
> - }
> -
>
> /* Grab turnoff reset */
> imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> @@ -1477,6 +1412,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> .dbi_length = 0x200,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> },
> [IMX6SX] = {
> .variant = IMX6SX,
> @@ -1484,6 +1420,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
> IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> },
> [IMX6QP] = {
> .variant = IMX6QP,
> @@ -1492,40 +1429,48 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .dbi_length = 0x200,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> },
> [IMX7D] = {
> .variant = IMX7D,
> .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx7d-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> },
> [IMX8MQ] = {
> .variant = IMX8MQ,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> },
> [IMX8MM] = {
> .variant = IMX8MM,
> .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MP] = {
> .variant = IMX8MP,
> .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MQ_EP] = {
> .variant = IMX8MQ_EP,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> },
> [IMX8MM_EP] = {
> .variant = IMX8MM_EP,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MP_EP] = {
> .variant = IMX8MP_EP,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> };
>
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:13PM -0500, Frank Li wrote:
> Refactors the phy handling logic in the imx6 PCI driver by adding
> IMX6_PCIE_FLAG_HAS_PHY bitmask define for drvdata::flags.
>
> The drvdata::flags and a bitmask ensures a cleaner and more scalable
> switch-case structure for handling phy.
>
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v4 to v5:
> - none, Keep IMX6_PCIE_FLAG_HAS_PHY to indicate dts mismatch when platform
> require phy suppport.
>
> Change from v1 to v3:
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 50d9faaa17f71..4d620249f3d52 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -60,6 +60,9 @@ enum imx6_pcie_variants {
> #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
> +#define IMX6_PCIE_FLAG_HAS_PHY BIT(3)
Every PCIe setup requires PHY for its operation. Perhaps you are referring to
external PHY? If so, please rename this to IMX6_PCIE_FLAG_HAS_EXT_PHY.
> +
> +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
>
> #define IMX6_PCIE_MAX_CLKS 6
>
> @@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY)) {
IMO, we would not need these kind of checks in the driver if the DT binding is
properly validated using schema. But folks always want to validate "broken DT"
in the drivers :(
But I'm fine with this check for now since not everyone agree with above.
- Mani
> + imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> + if (IS_ERR(imx6_pcie->phy))
> + return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> + "failed to get pcie phy\n");
> + }
> +
> switch (imx6_pcie->drvdata->variant) {
> case IMX7D:
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> @@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset),
> "failed to get pcie apps reset control\n");
>
> - imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> - if (IS_ERR(imx6_pcie->phy))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> - "failed to get pcie phy\n");
> -
> break;
> default:
> break;
> @@ -1444,13 +1449,15 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> },
> [IMX8MM] = {
> .variant = IMX8MM,
> - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> + IMX6_PCIE_FLAG_HAS_PHY,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MP] = {
> .variant = IMX8MP,
> - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> + IMX6_PCIE_FLAG_HAS_PHY,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> @@ -1462,12 +1469,14 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> },
> [IMX8MM_EP] = {
> .variant = IMX8MM_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_PHY,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> },
> [IMX8MP_EP] = {
> .variant = IMX8MP_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_PHY,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Sat, Jan 06, 2024 at 08:57:08PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 27, 2023 at 01:27:12PM -0500, Frank Li wrote:
>
> Subject mentions, 'bulk_clk' APIs but there is no such thing. It should be
> 'clk_bulk()' APIs.
>
> > Refactors the clock handling logic. Adds clk_names[] define in drvdata.
> > Using clk_bulk*() api simplifies the code.
> >
>
> I've mentioned this many times in the past. But let me reiterate here again:
>
> Commit message should be in imperative mood as per Linux Kernel rules for
> submitting patches. Please see here:
> Documentation/process/submitting-patches.rst
>
> The relevant part is:
>
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."
>
> Please use this same format for rest of the patches as well for future ones.
I may have not understand *imperative mode*. Asked an English native
speaker. Do you menas
*Refector* the clock handling logic. *Add* clk_names[] define in drvdata.
*Use* clk_bulk*() api *simplify* the code.
>
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> >
> > Notes:
> > Change from v4 to v5
> > - update commit message
> > - direct using clk name list, instead of macro
> > - still keep caculate clk list count because sizeof return pre allocated
> > array size.
> >
> > Change from v3 to v4
> > - using clk_bulk_*() API
> > Change from v1 to v3
> > - none
> >
> > drivers/pci/controller/dwc/pci-imx6.c | 125 ++++++++------------------
> > 1 file changed, 35 insertions(+), 90 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 74703362aeec7..50d9faaa17f71 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -61,12 +61,15 @@ enum imx6_pcie_variants {
> > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> > #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
> >
> > +#define IMX6_PCIE_MAX_CLKS 6
> > +
> > struct imx6_pcie_drvdata {
> > enum imx6_pcie_variants variant;
> > enum dw_pcie_device_mode mode;
> > u32 flags;
> > int dbi_length;
> > const char *gpr;
> > + const char *clk_names[IMX6_PCIE_MAX_CLKS];
> > };
> >
> > struct imx6_pcie {
> > @@ -74,11 +77,8 @@ struct imx6_pcie {
> > int reset_gpio;
> > bool gpio_active_high;
> > bool link_is_up;
> > - struct clk *pcie_bus;
> > - struct clk *pcie_phy;
> > - struct clk *pcie_inbound_axi;
> > - struct clk *pcie;
> > - struct clk *pcie_aux;
> > + struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
> > + u32 clks_cnt;
> > struct regmap *iomuxc_gpr;
> > u16 msi_ctrl;
> > u32 controller_id;
> > @@ -407,13 +407,18 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> >
> > static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
> > {
> > - unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
> > + unsigned long phy_rate = 0;
> > int mult, div;
> > u16 val;
> > + int i;
> >
> > if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
> > return 0;
> >
> > + for (i = 0; i < imx6_pcie->clks_cnt; i++)
> > + if (strncmp(imx6_pcie->clks[i].id, "pcie_phy", 8) == 0)
> > + phy_rate = clk_get_rate(imx6_pcie->clks[i].clk);
> > +
> > switch (phy_rate) {
> > case 125000000:
> > /*
> > @@ -550,19 +555,11 @@ static int imx6_pcie_attach_pd(struct device *dev)
> >
> > static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > {
> > - struct dw_pcie *pci = imx6_pcie->pci;
> > - struct device *dev = pci->dev;
> > unsigned int offset;
> > int ret = 0;
> >
> > switch (imx6_pcie->drvdata->variant) {
> > case IMX6SX:
> > - ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_axi clock\n");
> > - break;
> > - }
> > -
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0);
> > break;
> > @@ -589,12 +586,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > case IMX8MQ_EP:
> > case IMX8MP:
> > case IMX8MP_EP:
> > - ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_aux clock\n");
> > - break;
> > - }
> > -
> > offset = imx6_pcie_grp_offset(imx6_pcie);
> > /*
> > * Set the over ride low and enabled
> > @@ -615,9 +606,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> > {
> > switch (imx6_pcie->drvdata->variant) {
> > - case IMX6SX:
> > - clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > - break;
> > case IMX6QP:
> > case IMX6Q:
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > @@ -631,14 +619,6 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > break;
> > - case IMX8MM:
> > - case IMX8MM_EP:
> > - case IMX8MQ:
> > - case IMX8MQ_EP:
> > - case IMX8MP:
> > - case IMX8MP_EP:
> > - clk_disable_unprepare(imx6_pcie->pcie_aux);
> > - break;
> > default:
> > break;
> > }
> > @@ -650,23 +630,9 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> > struct device *dev = pci->dev;
> > int ret;
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_phy clock\n");
> > + ret = clk_bulk_prepare_enable(imx6_pcie->clks_cnt, imx6_pcie->clks);
>
> Extra space after =
>
> > + if (ret)
> > return ret;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_bus clock\n");
> > - goto err_pcie_bus;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie clock\n");
> > - goto err_pcie;
> > - }
> >
> > ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > if (ret) {
> > @@ -679,11 +645,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> > return 0;
> >
> > err_ref_clk:
> > - clk_disable_unprepare(imx6_pcie->pcie);
> > -err_pcie:
> > - clk_disable_unprepare(imx6_pcie->pcie_bus);
> > -err_pcie_bus:
> > - clk_disable_unprepare(imx6_pcie->pcie_phy);
> > + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
> >
> > return ret;
> > }
> > @@ -691,9 +653,7 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
> > static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> > {
> > imx6_pcie_disable_ref_clk(imx6_pcie);
> > - clk_disable_unprepare(imx6_pcie->pcie);
> > - clk_disable_unprepare(imx6_pcie->pcie_bus);
> > - clk_disable_unprepare(imx6_pcie->pcie_phy);
> > + clk_bulk_disable_unprepare(imx6_pcie->clks_cnt, imx6_pcie->clks);
> > }
> >
> > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > @@ -1305,32 +1265,19 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > return imx6_pcie->reset_gpio;
> > }
> >
> > - /* Fetch clocks */
> > - imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> > - if (IS_ERR(imx6_pcie->pcie_bus))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
> > - "pcie_bus clock source missing or invalid\n");
> > + while (imx6_pcie->drvdata->clk_names[imx6_pcie->clks_cnt]) {
> > + int i = imx6_pcie->clks_cnt;
>
> Why can't you initialize i to 0 directly?
can't init i to 0 directly here, otherwise next loop i will not increase.
i just reduce refer imx6_pcie->clks_cnt in
imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
Frank
>
> Rest looks good to me.
>
> - Mani
>
> > +
> > + imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
> > + imx6_pcie->clks_cnt++;
> > + }
> >
> > - imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> > - if (IS_ERR(imx6_pcie->pcie))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> > - "pcie clock source missing or invalid\n");
> > + /* Fetch clocks */
> > + ret = devm_clk_bulk_get(dev, imx6_pcie->clks_cnt, imx6_pcie->clks);
> > + if (ret)
> > + return ret;
> >
> > switch (imx6_pcie->drvdata->variant) {
> > - case IMX6SX:
> > - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> > - "pcie_inbound_axi");
> > - if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> > - "pcie_inbound_axi clock missing or invalid\n");
> > - break;
> > - case IMX8MQ:
> > - case IMX8MQ_EP:
> > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > - if (IS_ERR(imx6_pcie->pcie_aux))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > - "pcie_aux clock source missing or invalid\n");
> > - fallthrough;
> > case IMX7D:
> > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > imx6_pcie->controller_id = 1;
> > @@ -1353,10 +1300,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > case IMX8MM_EP:
> > case IMX8MP:
> > case IMX8MP_EP:
> > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > - if (IS_ERR(imx6_pcie->pcie_aux))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > - "pcie_aux clock source missing or invalid\n");
> > imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > "apps");
> > if (IS_ERR(imx6_pcie->apps_reset))
> > @@ -1372,14 +1315,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > default:
> > break;
> > }
> > - /* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> > - if (imx6_pcie->phy == NULL) {
> > - imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> > - if (IS_ERR(imx6_pcie->pcie_phy))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> > - "pcie_phy clock source missing or invalid\n");
> > - }
> > -
> >
> > /* Grab turnoff reset */
> > imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> > @@ -1477,6 +1412,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> > .dbi_length = 0x200,
> > .gpr = "fsl,imx6q-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > },
> > [IMX6SX] = {
> > .variant = IMX6SX,
> > @@ -1484,6 +1420,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
> > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx6q-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> > },
> > [IMX6QP] = {
> > .variant = IMX6QP,
> > @@ -1492,40 +1429,48 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .dbi_length = 0x200,
> > .gpr = "fsl,imx6q-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > },
> > [IMX7D] = {
> > .variant = IMX7D,
> > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx7d-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > },
> > [IMX8MQ] = {
> > .variant = IMX8MQ,
> > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > },
> > [IMX8MM] = {
> > .variant = IMX8MM,
> > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MP] = {
> > .variant = IMX8MP,
> > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MQ_EP] = {
> > .variant = IMX8MQ_EP,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > },
> > [IMX8MM_EP] = {
> > .variant = IMX8MM_EP,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MP_EP] = {
> > .variant = IMX8MP_EP,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > };
> >
> > --
> > 2.34.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On Sat, Jan 06, 2024 at 09:03:23PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 27, 2023 at 01:27:13PM -0500, Frank Li wrote:
> > Refactors the phy handling logic in the imx6 PCI driver by adding
> > IMX6_PCIE_FLAG_HAS_PHY bitmask define for drvdata::flags.
> >
> > The drvdata::flags and a bitmask ensures a cleaner and more scalable
> > switch-case structure for handling phy.
> >
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> >
> > Notes:
> > Change from v4 to v5:
> > - none, Keep IMX6_PCIE_FLAG_HAS_PHY to indicate dts mismatch when platform
> > require phy suppport.
> >
> > Change from v1 to v3:
> > - none
> >
> > drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++++-------
> > 1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 50d9faaa17f71..4d620249f3d52 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -60,6 +60,9 @@ enum imx6_pcie_variants {
> > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> > #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
> > +#define IMX6_PCIE_FLAG_HAS_PHY BIT(3)
>
> Every PCIe setup requires PHY for its operation. Perhaps you are referring to
> external PHY? If so, please rename this to IMX6_PCIE_FLAG_HAS_EXT_PHY.
Actually, it means use phy driver. How about using IMX6_PCIE_HAS_PHYDRV?
>
> > +
> > +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
> >
> > #define IMX6_PCIE_MAX_CLKS 6
> >
> > @@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY)) {
>
> IMO, we would not need these kind of checks in the driver if the DT binding is
> properly validated using schema. But folks always want to validate "broken DT"
> in the drivers :(
>
> But I'm fine with this check for now since not everyone agree with above.
>
> - Mani
>
> > + imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > + if (IS_ERR(imx6_pcie->phy))
> > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> > + "failed to get pcie phy\n");
> > + }
> > +
> > switch (imx6_pcie->drvdata->variant) {
> > case IMX7D:
> > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > @@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset),
> > "failed to get pcie apps reset control\n");
> >
> > - imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > - if (IS_ERR(imx6_pcie->phy))
> > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> > - "failed to get pcie phy\n");
> > -
> > break;
> > default:
> > break;
> > @@ -1444,13 +1449,15 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > },
> > [IMX8MM] = {
> > .variant = IMX8MM,
> > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> > + IMX6_PCIE_FLAG_HAS_PHY,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MP] = {
> > .variant = IMX8MP,
> > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> > + IMX6_PCIE_FLAG_HAS_PHY,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > @@ -1462,12 +1469,14 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > },
> > [IMX8MM_EP] = {
> > .variant = IMX8MM_EP,
> > + .flags = IMX6_PCIE_FLAG_HAS_PHY,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > },
> > [IMX8MP_EP] = {
> > .variant = IMX8MP_EP,
> > + .flags = IMX6_PCIE_FLAG_HAS_PHY,
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > --
> > 2.34.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On Sat, Jan 06, 2024 at 11:48:35AM -0500, Frank Li wrote:
> On Sat, Jan 06, 2024 at 08:57:08PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Dec 27, 2023 at 01:27:12PM -0500, Frank Li wrote:
> >
> > Subject mentions, 'bulk_clk' APIs but there is no such thing. It should be
> > 'clk_bulk()' APIs.
> >
> > > Refactors the clock handling logic. Adds clk_names[] define in drvdata.
> > > Using clk_bulk*() api simplifies the code.
> > >
> >
> > I've mentioned this many times in the past. But let me reiterate here again:
> >
> > Commit message should be in imperative mood as per Linux Kernel rules for
> > submitting patches. Please see here:
> > Documentation/process/submitting-patches.rst
> >
> > The relevant part is:
> >
> > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> > to do frotz", as if you are giving orders to the codebase to change
> > its behaviour."
> >
> > Please use this same format for rest of the patches as well for future ones.
>
> I may have not understand *imperative mode*. Asked an English native
> speaker. Do you menas
>
> *Refector* the clock handling logic. *Add* clk_names[] define in drvdata.
> *Use* clk_bulk*() api *simplify* the code.
Yes!
>
> >
> > > Signed-off-by: Frank Li <[email protected]>
> > > ---
> > >
> > > Notes:
> > > Change from v4 to v5
> > > - update commit message
> > > - direct using clk name list, instead of macro
> > > - still keep caculate clk list count because sizeof return pre allocated
> > > array size.
> > >
> > > Change from v3 to v4
> > > - using clk_bulk_*() API
> > > Change from v1 to v3
> > > - none
> > >
> > > drivers/pci/controller/dwc/pci-imx6.c | 125 ++++++++------------------
> > > 1 file changed, 35 insertions(+), 90 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 74703362aeec7..50d9faaa17f71 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
[...]
> > >
> > > - /* Fetch clocks */
> > > - imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> > > - if (IS_ERR(imx6_pcie->pcie_bus))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
> > > - "pcie_bus clock source missing or invalid\n");
> > > + while (imx6_pcie->drvdata->clk_names[imx6_pcie->clks_cnt]) {
> > > + int i = imx6_pcie->clks_cnt;
> >
> > Why can't you initialize i to 0 directly?
>
> can't init i to 0 directly here, otherwise next loop i will not increase.
> i just reduce refer imx6_pcie->clks_cnt in
>
> imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
>
Wait... Can't you just use ARRAY_SIZE() to calculate the clks_cnt statically?
Like,
static const char * const imx8_clk_names[] = {
"pcie_bus", "pcie", "pcie_aux",
};
[...]
.clk_names = imx8_clk_names,
.clks_cnt = ARRAY_SIZE(imx8_clk_names),
You can use the same clk_names array for multiple SoCs if the clocks are same.
I should've mentioned this in last review itself. Sorry about that.
- Mani
> Frank
>
> >
> > Rest looks good to me.
> >
> > - Mani
> >
> > > +
> > > + imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
> > > + imx6_pcie->clks_cnt++;
> > > + }
> > >
> > > - imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> > > - if (IS_ERR(imx6_pcie->pcie))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> > > - "pcie clock source missing or invalid\n");
> > > + /* Fetch clocks */
> > > + ret = devm_clk_bulk_get(dev, imx6_pcie->clks_cnt, imx6_pcie->clks);
> > > + if (ret)
> > > + return ret;
> > >
> > > switch (imx6_pcie->drvdata->variant) {
> > > - case IMX6SX:
> > > - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> > > - "pcie_inbound_axi");
> > > - if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> > > - "pcie_inbound_axi clock missing or invalid\n");
> > > - break;
> > > - case IMX8MQ:
> > > - case IMX8MQ_EP:
> > > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > > - if (IS_ERR(imx6_pcie->pcie_aux))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > > - "pcie_aux clock source missing or invalid\n");
> > > - fallthrough;
> > > case IMX7D:
> > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > imx6_pcie->controller_id = 1;
> > > @@ -1353,10 +1300,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > case IMX8MM_EP:
> > > case IMX8MP:
> > > case IMX8MP_EP:
> > > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > > - if (IS_ERR(imx6_pcie->pcie_aux))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > > - "pcie_aux clock source missing or invalid\n");
> > > imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > > "apps");
> > > if (IS_ERR(imx6_pcie->apps_reset))
> > > @@ -1372,14 +1315,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > default:
> > > break;
> > > }
> > > - /* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> > > - if (imx6_pcie->phy == NULL) {
> > > - imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> > > - if (IS_ERR(imx6_pcie->pcie_phy))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> > > - "pcie_phy clock source missing or invalid\n");
> > > - }
> > > -
> > >
> > > /* Grab turnoff reset */
> > > imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> > > @@ -1477,6 +1412,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> > > .dbi_length = 0x200,
> > > .gpr = "fsl,imx6q-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > },
> > > [IMX6SX] = {
> > > .variant = IMX6SX,
> > > @@ -1484,6 +1420,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
> > > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx6q-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> > > },
> > > [IMX6QP] = {
> > > .variant = IMX6QP,
> > > @@ -1492,40 +1429,48 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .dbi_length = 0x200,
> > > .gpr = "fsl,imx6q-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > },
> > > [IMX7D] = {
> > > .variant = IMX7D,
> > > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx7d-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > },
> > > [IMX8MQ] = {
> > > .variant = IMX8MQ,
> > > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > > },
> > > [IMX8MM] = {
> > > .variant = IMX8MM,
> > > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MP] = {
> > > .variant = IMX8MP,
> > > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MQ_EP] = {
> > > .variant = IMX8MQ_EP,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > > },
> > > [IMX8MM_EP] = {
> > > .variant = IMX8MM_EP,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MP_EP] = {
> > > .variant = IMX8MP_EP,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > };
> > >
> > > --
> > > 2.34.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
On Sat, Jan 06, 2024 at 11:50:28AM -0500, Frank Li wrote:
> On Sat, Jan 06, 2024 at 09:03:23PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Dec 27, 2023 at 01:27:13PM -0500, Frank Li wrote:
> > > Refactors the phy handling logic in the imx6 PCI driver by adding
> > > IMX6_PCIE_FLAG_HAS_PHY bitmask define for drvdata::flags.
> > >
> > > The drvdata::flags and a bitmask ensures a cleaner and more scalable
> > > switch-case structure for handling phy.
> > >
> > > Signed-off-by: Frank Li <[email protected]>
> > > ---
> > >
> > > Notes:
> > > Change from v4 to v5:
> > > - none, Keep IMX6_PCIE_FLAG_HAS_PHY to indicate dts mismatch when platform
> > > require phy suppport.
> > >
> > > Change from v1 to v3:
> > > - none
> > >
> > > drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++++-------
> > > 1 file changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 50d9faaa17f71..4d620249f3d52 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -60,6 +60,9 @@ enum imx6_pcie_variants {
> > > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> > > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> > > #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
> > > +#define IMX6_PCIE_FLAG_HAS_PHY BIT(3)
> >
> > Every PCIe setup requires PHY for its operation. Perhaps you are referring to
> > external PHY? If so, please rename this to IMX6_PCIE_FLAG_HAS_EXT_PHY.
>
> Actually, it means use phy driver. How about using IMX6_PCIE_HAS_PHYDRV?
>
Ah, ok. Yes, this makes sense.
- Mani
> >
> > > +
> > > +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
> > >
> > > #define IMX6_PCIE_MAX_CLKS 6
> > >
> > > @@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > if (ret)
> > > return ret;
> > >
> > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY)) {
> >
> > IMO, we would not need these kind of checks in the driver if the DT binding is
> > properly validated using schema. But folks always want to validate "broken DT"
> > in the drivers :(
> >
> > But I'm fine with this check for now since not everyone agree with above.
> >
> > - Mani
> >
> > > + imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > > + if (IS_ERR(imx6_pcie->phy))
> > > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> > > + "failed to get pcie phy\n");
> > > + }
> > > +
> > > switch (imx6_pcie->drvdata->variant) {
> > > case IMX7D:
> > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > @@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset),
> > > "failed to get pcie apps reset control\n");
> > >
> > > - imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > > - if (IS_ERR(imx6_pcie->phy))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> > > - "failed to get pcie phy\n");
> > > -
> > > break;
> > > default:
> > > break;
> > > @@ -1444,13 +1449,15 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > },
> > > [IMX8MM] = {
> > > .variant = IMX8MM,
> > > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> > > + IMX6_PCIE_FLAG_HAS_PHY,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MP] = {
> > > .variant = IMX8MP,
> > > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> > > + IMX6_PCIE_FLAG_HAS_PHY,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > @@ -1462,12 +1469,14 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > },
> > > [IMX8MM_EP] = {
> > > .variant = IMX8MM_EP,
> > > + .flags = IMX6_PCIE_FLAG_HAS_PHY,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MP_EP] = {
> > > .variant = IMX8MP_EP,
> > > + .flags = IMX6_PCIE_FLAG_HAS_PHY,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > --
> > > 2.34.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:15PM -0500, Frank Li wrote:
> iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
> proptery for iMX8MQ to indicate pci controller index.
>
property
> This adjustment paves the way for eliminating the hardcoded check on the
> base register for acquiring the controller_id.
>
> ...
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> imx6_pcie->controller_id = 1;
> ...
>
> The controller_id is crucial and utilized for certain register bit
> positions. It must align precisely with the controller index in the SoC.
> An auto-incremented ID don't fit this case. The DTS or fuse configurations
> may deactivate specific PCI controllers.
>
You cannot change the binding for the sake of driver. But you can make this
change in other way. See below...
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v5 to v6
> - rework commit message to explain why need required and why auto increase
> id not work
>
> Change from v4 to v5
> - new patch at v5
>
> .../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> index d91b639ae7ae7..8f39b4e6e8491 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> @@ -265,6 +265,17 @@ allOf:
> - const: apps
> - const: turnoff
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - fsl,imx8mq-pcie
> + - fsl,imx8mq-pcie-ep
"linux,pci-domain" is a generic property. So you cannot make it required only
for certain SoCs. But you can make it so for all SoCs. This way, the drivers
can also rely on it.
Now, you should get rid of the commit message about driver internals:
> This adjustment paves the way for eliminating the hardcoded check on the
> base register for acquiring the controller_id.
>
> ...
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> imx6_pcie->controller_id = 1;
> ...
>
> The controller_id is crucial and utilized for certain register bit
> positions. It must align precisely with the controller index in the SoC.
> An auto-incremented ID don't fit this case. The DTS or fuse configurations
> may deactivate specific PCI controllers.
>
- Mani
> + then:
> + required:
> + - linux,pci-domain
> +
> additionalProperties: true
>
> ...
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:16PM -0500, Frank Li wrote:
> Avoid use get slot id by compared with register physical address. If there
> are more than 2 slots, compared logic will become complex.
>
> "linux,pci-domain" already exist at dts since commit:
> commit (c0b70f05c87f3b arm64: dts: imx8mq: use_dt_domains for pci node).
>
> So it is safe to remove compare basic address code:
> ...
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> imx6_pcie->controller_id = 1;
> ...
>
> Signed-off-by: Frank Li <[email protected]>
One comment below. With that fixed,
Reviewed-by: Manivannan Sadhasivam <[email protected]>
> ---
>
> Notes:
> Change from v3 to v4
> - remove compare basic address logic
> Change from v2 to v3
> - none
> Change from v1 to v2
> - fix of_get_pci_domain_nr return value check logic
>
> drivers/pci/controller/dwc/pci-imx6.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 294f61a9c6fd9..332c392f8e5bc 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -33,6 +33,7 @@
> #include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
>
> +#include "../../pci.h"
> #include "pcie-designware.h"
>
> #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> @@ -40,7 +41,6 @@
> #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> #define IMX8MQ_GPR_PCIE_VREG_BYPASS BIT(12)
> #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
> -#define IMX8MQ_PCIE2_BASE_ADDR 0x33c00000
>
> #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
>
> @@ -1279,13 +1279,14 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> "Failed to get PCIEPHY reset control\n");
> }
>
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX7D:
> - if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> - imx6_pcie->controller_id = 1;
> - default:
> - break;
> - }
> + /* Using linux,pci-domain as PCI slot id */
> + imx6_pcie->controller_id = of_get_pci_domain_nr(node);
> + /* If there are not "linux,pci-domain" in dts file, means only 1 controller */
"If there are no "linux,pci-domain" property specified in DT, then assume only
one controller is available."
- Mani
> + if (imx6_pcie->controller_id == -EINVAL)
> + imx6_pcie->controller_id = 0;
> + else if (imx6_pcie->controller_id < 0)
> + return dev_err_probe(dev, imx6_pcie->controller_id,
> + "linux,pci-domain have wrong value\n");
>
> /* Grab turnoff reset */
> imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:17PM -0500, Frank Li wrote:
> Add drvdata::ltssm_off and drvdata::ltssm_mask to simple
> imx6_pcie_ltssm_enable(disable)() logic.
>
> Signed-off-by: Frank Li <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
>
> Notes:
> Change from v1 to v3
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 37 ++++++++++++---------------
> 1 file changed, 16 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 332c392f8e5bc..588bfb616260e 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -75,6 +75,8 @@ struct imx6_pcie_drvdata {
> int dbi_length;
> const char *gpr;
> const char *clk_names[IMX6_PCIE_MAX_CLKS];
> + const u32 ltssm_off;
> + const u32 ltssm_mask;
> };
>
> struct imx6_pcie {
> @@ -775,18 +777,11 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
> static void imx6_pcie_ltssm_enable(struct device *dev)
> {
> struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> + const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
>
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX6Q:
> - case IMX6SX:
> - case IMX6QP:
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> - IMX6Q_GPR12_PCIE_CTL_2,
> - IMX6Q_GPR12_PCIE_CTL_2);
> - break;
> - default:
> - break;
> - }
> + if (drvdata->ltssm_mask)
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->ltssm_off, drvdata->ltssm_mask,
> + drvdata->ltssm_mask);
>
> reset_control_deassert(imx6_pcie->apps_reset);
> }
> @@ -794,17 +789,11 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
> static void imx6_pcie_ltssm_disable(struct device *dev)
> {
> struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> + const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
>
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX6Q:
> - case IMX6SX:
> - case IMX6QP:
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> - IMX6Q_GPR12_PCIE_CTL_2, 0);
> - break;
> - default:
> - break;
> - }
> + if (drvdata->ltssm_mask)
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->ltssm_off,
> + drvdata->ltssm_mask, 0);
>
> reset_control_assert(imx6_pcie->apps_reset);
> }
> @@ -1385,6 +1374,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .dbi_length = 0x200,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> + .ltssm_off = IOMUXC_GPR12,
> + .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> },
> [IMX6SX] = {
> .variant = IMX6SX,
> @@ -1393,6 +1384,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> + .ltssm_off = IOMUXC_GPR12,
> + .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> },
> [IMX6QP] = {
> .variant = IMX6QP,
> @@ -1402,6 +1395,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .dbi_length = 0x200,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> + .ltssm_off = IOMUXC_GPR12,
> + .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> },
> [IMX7D] = {
> .variant = IMX7D,
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Sun, Jan 07, 2024 at 08:45:06AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 27, 2023 at 01:27:15PM -0500, Frank Li wrote:
> > iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
> > proptery for iMX8MQ to indicate pci controller index.
> >
>
> property
>
> > This adjustment paves the way for eliminating the hardcoded check on the
> > base register for acquiring the controller_id.
> >
> > ...
> > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > imx6_pcie->controller_id = 1;
> > ...
> >
> > The controller_id is crucial and utilized for certain register bit
> > positions. It must align precisely with the controller index in the SoC.
> > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > may deactivate specific PCI controllers.
> >
>
> You cannot change the binding for the sake of driver. But you can make this
> change in other way. See below...
>
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> >
> > Notes:
> > Change from v5 to v6
> > - rework commit message to explain why need required and why auto increase
> > id not work
> >
> > Change from v4 to v5
> > - new patch at v5
> >
> > .../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > index d91b639ae7ae7..8f39b4e6e8491 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > @@ -265,6 +265,17 @@ allOf:
> > - const: apps
> > - const: turnoff
> >
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - fsl,imx8mq-pcie
> > + - fsl,imx8mq-pcie-ep
>
> "linux,pci-domain" is a generic property. So you cannot make it required only
> for certain SoCs.
Sorry, why not? there are many generic property.
> But you can make it so for all SoCs. This way, the drivers
> can also rely on it.
>
> Now, you should get rid of the commit message about driver internals:
Not all dts already added "linux,pci-domain" yet. If required for all SOCs,
it will cause dtb check warnings.
Frank
>
> > This adjustment paves the way for eliminating the hardcoded check on the
> > base register for acquiring the controller_id.
> >
> > ...
> > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > imx6_pcie->controller_id = 1;
> > ...
> >
> > The controller_id is crucial and utilized for certain register bit
> > positions. It must align precisely with the controller index in the SoC.
> > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > may deactivate specific PCI controllers.
> >
>
> - Mani
>
> > + then:
> > + required:
> > + - linux,pci-domain
> > +
> > additionalProperties: true
> >
> > ...
> > --
> > 2.34.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:18PM -0500, Frank Li wrote:
> Add drvdata::mode_off and drvdata::mode_mask to simple
simplify
> imx6_pcie_configure_type() logic.
>
> Signed-off-by: Frank Li <[email protected]>
Couple of comments below.
> ---
>
> Notes:
> Change from v2 to v3
> - none
> Change from v1 to v2
> - use ffs() to fixe build error.
>
> drivers/pci/controller/dwc/pci-imx6.c | 60 ++++++++++++++++++---------
> 1 file changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 588bfb616260e..717e8fa030deb 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -68,6 +68,7 @@ enum imx6_pcie_variants {
>
> #define IMX6_PCIE_MAX_CLKS 6
>
> +#define IMX6_PCIE_MAX_INSTANCES 2
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> enum dw_pcie_device_mode mode;
> @@ -77,6 +78,8 @@ struct imx6_pcie_drvdata {
> const char *clk_names[IMX6_PCIE_MAX_CLKS];
> const u32 ltssm_off;
> const u32 ltssm_mask;
> + const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
> + const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
> };
>
> struct imx6_pcie {
> @@ -174,32 +177,25 @@ static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
>
> static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
> {
> - unsigned int mask, val, mode;
> + const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
> + unsigned int mask, val, mode, id;
>
> - if (imx6_pcie->drvdata->mode == DW_PCIE_EP_TYPE)
> + if (drvdata->mode == DW_PCIE_EP_TYPE)
> mode = PCI_EXP_TYPE_ENDPOINT;
> else
> mode = PCI_EXP_TYPE_ROOT_PORT;
>
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX8MQ:
> - case IMX8MQ_EP:
> - if (imx6_pcie->controller_id == 1) {
> - mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> - val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> - mode);
> - } else {
> - mask = IMX6Q_GPR12_DEVICE_TYPE;
> - val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
> - }
> - break;
> - default:
> - mask = IMX6Q_GPR12_DEVICE_TYPE;
> - val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
> - break;
> - }
> + id = imx6_pcie->controller_id;
> +
> + /* If mode_mask[id] is zero, means each controller have its individual gpr */
> + if (!drvdata->mode_mask[id])
> + id = 0;
> +
> + mask = drvdata->mode_mask[id];
> + /* FIELD_PREP mask have been constant */
No need of this comment.
> + val = mode << (ffs(mask) - 1);
>
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->mode_off[id], mask, val);
> }
>
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> @@ -1376,6 +1372,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> .ltssm_off = IOMUXC_GPR12,
> .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> [IMX6SX] = {
> .variant = IMX6SX,
> @@ -1386,6 +1384,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> .ltssm_off = IOMUXC_GPR12,
> .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> [IMX6QP] = {
> .variant = IMX6QP,
> @@ -1397,6 +1397,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> .ltssm_off = IOMUXC_GPR12,
> .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> [IMX7D] = {
> .variant = IMX7D,
> @@ -1405,6 +1407,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_HAS_PHY_RESET,
> .gpr = "fsl,imx7d-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> [IMX8MQ] = {
> .variant = IMX8MQ,
> @@ -1412,6 +1416,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_HAS_PHY_RESET,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> + .mode_off[1] = IOMUXC_GPR12,
> + .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
Is the mode_mask differ between SoCs or fixed based on instances? I mean, if
there is a guarantee that it is going to be IMX6Q_GPR12_DEVICE_TYPE for instance
1 and IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE for instance 2 etc...
Then we can avoid these SoC specific config and simplify the code further.
- Mani
> },
> [IMX8MM] = {
> .variant = IMX8MM,
> @@ -1420,6 +1428,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_HAS_APP_RESET,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> [IMX8MP] = {
> .variant = IMX8MP,
> @@ -1428,6 +1438,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> IMX6_PCIE_FLAG_HAS_APP_RESET,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> [IMX8MQ_EP] = {
> .variant = IMX8MQ_EP,
> @@ -1436,6 +1448,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> + .mode_off[1] = IOMUXC_GPR12,
> + .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> },
> [IMX8MM_EP] = {
> .variant = IMX8MM_EP,
> @@ -1443,6 +1459,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> [IMX8MP_EP] = {
> .variant = IMX8MP_EP,
> @@ -1450,6 +1468,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> + .mode_off[0] = IOMUXC_GPR12,
> + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> },
> };
>
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Sat, Jan 06, 2024 at 11:47:36PM -0500, Frank Li wrote:
> On Sun, Jan 07, 2024 at 08:45:06AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Dec 27, 2023 at 01:27:15PM -0500, Frank Li wrote:
> > > iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
> > > proptery for iMX8MQ to indicate pci controller index.
> > >
> >
> > property
> >
> > > This adjustment paves the way for eliminating the hardcoded check on the
> > > base register for acquiring the controller_id.
> > >
> > > ...
> > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > imx6_pcie->controller_id = 1;
> > > ...
> > >
> > > The controller_id is crucial and utilized for certain register bit
> > > positions. It must align precisely with the controller index in the SoC.
> > > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > > may deactivate specific PCI controllers.
> > >
> >
> > You cannot change the binding for the sake of driver. But you can make this
> > change in other way. See below...
> >
> > > Signed-off-by: Frank Li <[email protected]>
> > > ---
> > >
> > > Notes:
> > > Change from v5 to v6
> > > - rework commit message to explain why need required and why auto increase
> > > id not work
> > >
> > > Change from v4 to v5
> > > - new patch at v5
> > >
> > > .../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > index d91b639ae7ae7..8f39b4e6e8491 100644
> > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > @@ -265,6 +265,17 @@ allOf:
> > > - const: apps
> > > - const: turnoff
> > >
> > > + - if:
> > > + properties:
> > > + compatible:
> > > + contains:
> > > + enum:
> > > + - fsl,imx8mq-pcie
> > > + - fsl,imx8mq-pcie-ep
> >
> > "linux,pci-domain" is a generic property. So you cannot make it required only
> > for certain SoCs.
>
> Sorry, why not? there are many generic property.
>
It doesn't make sense to make it required only for specific SoCs since it is not
specific to any SoC. You can make it required for all.
> > But you can make it so for all SoCs. This way, the drivers
> > can also rely on it.
> >
> > Now, you should get rid of the commit message about driver internals:
>
> Not all dts already added "linux,pci-domain" yet. If required for all SOCs,
> it will cause dtb check warnings.
>
You can safely add this property to all DTS. Nothing will break.
- Mani
> Frank
> >
> > > This adjustment paves the way for eliminating the hardcoded check on the
> > > base register for acquiring the controller_id.
> > >
> > > ...
> > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > imx6_pcie->controller_id = 1;
> > > ...
> > >
> > > The controller_id is crucial and utilized for certain register bit
> > > positions. It must align precisely with the controller index in the SoC.
> > > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > > may deactivate specific PCI controllers.
> > >
> >
> > - Mani
> >
> > > + then:
> > > + required:
> > > + - linux,pci-domain
> > > +
> > > additionalProperties: true
> > >
> > > ...
> > > --
> > > 2.34.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
On Sun, Jan 07, 2024 at 10:46:55AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 27, 2023 at 01:27:18PM -0500, Frank Li wrote:
> > Add drvdata::mode_off and drvdata::mode_mask to simple
>
> simplify
>
> > imx6_pcie_configure_type() logic.
> >
> > Signed-off-by: Frank Li <[email protected]>
>
> Couple of comments below.
>
> > ---
> >
> > Notes:
> > Change from v2 to v3
> > - none
> > Change from v1 to v2
> > - use ffs() to fixe build error.
> >
> > drivers/pci/controller/dwc/pci-imx6.c | 60 ++++++++++++++++++---------
> > 1 file changed, 40 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 588bfb616260e..717e8fa030deb 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -68,6 +68,7 @@ enum imx6_pcie_variants {
> >
> > #define IMX6_PCIE_MAX_CLKS 6
> >
> > +#define IMX6_PCIE_MAX_INSTANCES 2
> > struct imx6_pcie_drvdata {
> > enum imx6_pcie_variants variant;
> > enum dw_pcie_device_mode mode;
> > @@ -77,6 +78,8 @@ struct imx6_pcie_drvdata {
> > const char *clk_names[IMX6_PCIE_MAX_CLKS];
> > const u32 ltssm_off;
> > const u32 ltssm_mask;
> > + const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
> > + const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
> > };
> >
> > struct imx6_pcie {
> > @@ -174,32 +177,25 @@ static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> >
> > static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
> > {
> > - unsigned int mask, val, mode;
> > + const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
> > + unsigned int mask, val, mode, id;
> >
> > - if (imx6_pcie->drvdata->mode == DW_PCIE_EP_TYPE)
> > + if (drvdata->mode == DW_PCIE_EP_TYPE)
> > mode = PCI_EXP_TYPE_ENDPOINT;
> > else
> > mode = PCI_EXP_TYPE_ROOT_PORT;
> >
> > - switch (imx6_pcie->drvdata->variant) {
> > - case IMX8MQ:
> > - case IMX8MQ_EP:
> > - if (imx6_pcie->controller_id == 1) {
> > - mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> > - val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> > - mode);
> > - } else {
> > - mask = IMX6Q_GPR12_DEVICE_TYPE;
> > - val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
> > - }
> > - break;
> > - default:
> > - mask = IMX6Q_GPR12_DEVICE_TYPE;
> > - val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
> > - break;
> > - }
> > + id = imx6_pcie->controller_id;
> > +
> > + /* If mode_mask[id] is zero, means each controller have its individual gpr */
> > + if (!drvdata->mode_mask[id])
> > + id = 0;
> > +
> > + mask = drvdata->mode_mask[id];
> > + /* FIELD_PREP mask have been constant */
>
> No need of this comment.
>
> > + val = mode << (ffs(mask) - 1);
> >
> > - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
> > + regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->mode_off[id], mask, val);
> > }
> >
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> > @@ -1376,6 +1372,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > .ltssm_off = IOMUXC_GPR12,
> > .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > [IMX6SX] = {
> > .variant = IMX6SX,
> > @@ -1386,6 +1384,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> > .ltssm_off = IOMUXC_GPR12,
> > .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > [IMX6QP] = {
> > .variant = IMX6QP,
> > @@ -1397,6 +1397,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > .ltssm_off = IOMUXC_GPR12,
> > .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > [IMX7D] = {
> > .variant = IMX7D,
> > @@ -1405,6 +1407,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_HAS_PHY_RESET,
> > .gpr = "fsl,imx7d-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > [IMX8MQ] = {
> > .variant = IMX8MQ,
> > @@ -1412,6 +1416,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_HAS_PHY_RESET,
> > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > + .mode_off[1] = IOMUXC_GPR12,
> > + .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
>
> Is the mode_mask differ between SoCs or fixed based on instances? I mean, if
> there is a guarantee that it is going to be IMX6Q_GPR12_DEVICE_TYPE for instance
> 1 and IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE for instance 2 etc...
>
> Then we can avoid these SoC specific config and simplify the code further.
No, iMX95 will change it.
Frank
>
> - Mani
>
> > },
> > [IMX8MM] = {
> > .variant = IMX8MM,
> > @@ -1420,6 +1428,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_HAS_APP_RESET,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > [IMX8MP] = {
> > .variant = IMX8MP,
> > @@ -1428,6 +1438,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > IMX6_PCIE_FLAG_HAS_APP_RESET,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > [IMX8MQ_EP] = {
> > .variant = IMX8MQ_EP,
> > @@ -1436,6 +1448,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > + .mode_off[1] = IOMUXC_GPR12,
> > + .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> > },
> > [IMX8MM_EP] = {
> > .variant = IMX8MM_EP,
> > @@ -1443,6 +1459,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > [IMX8MP_EP] = {
> > .variant = IMX8MP_EP,
> > @@ -1450,6 +1468,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > .mode = DW_PCIE_EP_TYPE,
> > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > + .mode_off[0] = IOMUXC_GPR12,
> > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > },
> > };
> >
> > --
> > 2.34.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:19PM -0500, Frank Li wrote:
Subject: PCI: imx6: Introduce init_phy() callback to simplify PHY initialization
> Add drvdata::init_phy() callback function, so difference SOC choose
> difference callback function to simple switch-case logic.
>
Same subject can be used in commit message.
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> change from v1 to v4:
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 135 ++++++++++++++------------
> 1 file changed, 71 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 717e8fa030deb..d66a2db53bdb7 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -69,6 +69,9 @@ enum imx6_pcie_variants {
> #define IMX6_PCIE_MAX_CLKS 6
>
> #define IMX6_PCIE_MAX_INSTANCES 2
> +
> +struct imx6_pcie;
> +
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> enum dw_pcie_device_mode mode;
> @@ -80,6 +83,7 @@ struct imx6_pcie_drvdata {
> const u32 ltssm_mask;
> const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
> const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
> + int (*init_phy)(struct imx6_pcie *pcie);
> };
>
> struct imx6_pcie {
> @@ -323,76 +327,69 @@ static int pcie_phy_write(struct imx6_pcie *imx6_pcie, int addr, u16 data)
> return 0;
> }
>
> -static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> +static int imx8mq_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> {
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX8MM:
> - case IMX8MM_EP:
> - case IMX8MP:
> - case IMX8MP_EP:
> - /*
> - * The PHY initialization had been done in the PHY
> - * driver, break here directly.
> - */
> - break;
> - case IMX8MQ:
> - case IMX8MQ_EP:
> - /*
> - * TODO: Currently this code assumes external
> - * oscillator is being used
> - */
> + /*
> + * TODO: Currently this code assumes external
> + * oscillator is being used
Wrap the comments to 80 column.
> + */
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + imx6_pcie_grp_offset(imx6_pcie),
> + IMX8MQ_GPR_PCIE_REF_USE_PAD,
> + IMX8MQ_GPR_PCIE_REF_USE_PAD);
> + /*
> + * Regarding the datasheet, the PCIE_VPH is suggested
> + * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
> + * VREG_BYPASS should be cleared to zero.
> + */
Same here.
> + if (imx6_pcie->vph && regulator_get_voltage(imx6_pcie->vph) > 3000000)
> regmap_update_bits(imx6_pcie->iomuxc_gpr,
> imx6_pcie_grp_offset(imx6_pcie),
> - IMX8MQ_GPR_PCIE_REF_USE_PAD,
> - IMX8MQ_GPR_PCIE_REF_USE_PAD);
> - /*
> - * Regarding the datasheet, the PCIE_VPH is suggested
> - * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
> - * VREG_BYPASS should be cleared to zero.
> - */
> - if (imx6_pcie->vph &&
> - regulator_get_voltage(imx6_pcie->vph) > 3000000)
> - regmap_update_bits(imx6_pcie->iomuxc_gpr,
> - imx6_pcie_grp_offset(imx6_pcie),
> - IMX8MQ_GPR_PCIE_VREG_BYPASS,
> - 0);
> - break;
> - case IMX7D:
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> + IMX8MQ_GPR_PCIE_VREG_BYPASS,
> + 0);
> +
> + return 0;
> +}
> +
> +static int imx7d_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> +{
> + return regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
For consistency, return 0.
- Mani
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:20PM -0500, Frank Li wrote:
> The bindings referencing this file already define these constraints for
> each of the variants, so the if not: then: is redundant.
>
> Acked-by: Rob Herring <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
Acked-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
>
> Notes:
> Change from v6 to v7
> - rewrite git commit message by using simple words
> Change from v5 to v6
> - rewrite git commit message and explain why remove it safely.
> - Add Rob's Ack
> Change from v1 to v4
> - new patch at v4
>
> .../bindings/pci/fsl,imx6q-pcie-common.yaml | 16 ----------------
> 1 file changed, 16 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> index 8f39b4e6e8491..a284a27c5e873 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> @@ -150,22 +150,6 @@ allOf:
> - {}
> - const: pcie_phy
> - const: pcie_aux
> - - if:
> - properties:
> - compatible:
> - not:
> - contains:
> - enum:
> - - fsl,imx6sx-pcie
> - - fsl,imx8mq-pcie
> - - fsl,imx6sx-pcie-ep
> - - fsl,imx8mq-pcie-ep
> - then:
> - properties:
> - clocks:
> - maxItems: 3
> - clock-names:
> - maxItems: 3
>
> - if:
> properties:
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:21PM -0500, Frank Li wrote:
> snps,dw-pcie.yaml already have reg and reg-name information. Needn't
> duplciate here.
>
> Add 'if' check for existed compatible string to restrict reg and reg-names.
> This prepare to add new compatible string with difference reg-names sets.
>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
Acked-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
>
> Notes:
> Change from v6 to v7
> - add Krzysztof's review tag
> Change from v5 to v6
> - Add if check for existed compatible string
> Change from v4 to v5
> - add Rob's Acked
> Change from v1 to v4:
> - new patch at v4
>
> .../bindings/pci/fsl,imx6q-pcie.yaml | 30 ++++++++++++-------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> index 81bbb8728f0f9..eeca6b7b540f9 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> @@ -30,16 +30,6 @@ properties:
> - fsl,imx8mm-pcie
> - fsl,imx8mp-pcie
>
> - reg:
> - items:
> - - description: Data Bus Interface (DBI) registers.
> - - description: PCIe configuration space region.
> -
> - reg-names:
> - items:
> - - const: dbi
> - - const: config
> -
> clocks:
> minItems: 3
> items:
> @@ -90,6 +80,26 @@ required:
> allOf:
> - $ref: /schemas/pci/snps,dw-pcie.yaml#
> - $ref: /schemas/pci/fsl,imx6q-pcie-common.yaml#
> + - if:
> + properties:
> + compatible:
> + enum:
> + - fsl,imx6q-pcie
> + - fsl,imx6sx-pcie
> + - fsl,imx6qp-pcie
> + - fsl,imx7d-pcie
> + - fsl,imx8mq-pcie
> + - fsl,imx8mm-pcie
> + - fsl,imx8mp-pcie
> + then:
> + properties:
> + reg:
> + maxItems: 2
> + reg-names:
> + items:
> + - const: dbi
> + - const: config
> +
> - if:
> properties:
> compatible:
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Sun, Jan 07, 2024 at 12:32:14AM -0500, Frank Li wrote:
> On Sun, Jan 07, 2024 at 10:46:55AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Dec 27, 2023 at 01:27:18PM -0500, Frank Li wrote:
> > > Add drvdata::mode_off and drvdata::mode_mask to simple
> >
> > simplify
> >
> > > imx6_pcie_configure_type() logic.
> > >
> > > Signed-off-by: Frank Li <[email protected]>
> >
> > Couple of comments below.
> >
> > > ---
> > >
> > > Notes:
> > > Change from v2 to v3
> > > - none
> > > Change from v1 to v2
> > > - use ffs() to fixe build error.
> > >
> > > drivers/pci/controller/dwc/pci-imx6.c | 60 ++++++++++++++++++---------
> > > 1 file changed, 40 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 588bfb616260e..717e8fa030deb 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -68,6 +68,7 @@ enum imx6_pcie_variants {
> > >
> > > #define IMX6_PCIE_MAX_CLKS 6
> > >
> > > +#define IMX6_PCIE_MAX_INSTANCES 2
> > > struct imx6_pcie_drvdata {
> > > enum imx6_pcie_variants variant;
> > > enum dw_pcie_device_mode mode;
> > > @@ -77,6 +78,8 @@ struct imx6_pcie_drvdata {
> > > const char *clk_names[IMX6_PCIE_MAX_CLKS];
> > > const u32 ltssm_off;
> > > const u32 ltssm_mask;
> > > + const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
> > > + const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
> > > };
> > >
> > > struct imx6_pcie {
> > > @@ -174,32 +177,25 @@ static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> > >
> > > static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
> > > {
> > > - unsigned int mask, val, mode;
> > > + const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
> > > + unsigned int mask, val, mode, id;
> > >
> > > - if (imx6_pcie->drvdata->mode == DW_PCIE_EP_TYPE)
> > > + if (drvdata->mode == DW_PCIE_EP_TYPE)
> > > mode = PCI_EXP_TYPE_ENDPOINT;
> > > else
> > > mode = PCI_EXP_TYPE_ROOT_PORT;
> > >
> > > - switch (imx6_pcie->drvdata->variant) {
> > > - case IMX8MQ:
> > > - case IMX8MQ_EP:
> > > - if (imx6_pcie->controller_id == 1) {
> > > - mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> > > - val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> > > - mode);
> > > - } else {
> > > - mask = IMX6Q_GPR12_DEVICE_TYPE;
> > > - val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
> > > - }
> > > - break;
> > > - default:
> > > - mask = IMX6Q_GPR12_DEVICE_TYPE;
> > > - val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, mode);
> > > - break;
> > > - }
> > > + id = imx6_pcie->controller_id;
> > > +
> > > + /* If mode_mask[id] is zero, means each controller have its individual gpr */
> > > + if (!drvdata->mode_mask[id])
> > > + id = 0;
> > > +
> > > + mask = drvdata->mode_mask[id];
> > > + /* FIELD_PREP mask have been constant */
> >
> > No need of this comment.
> >
> > > + val = mode << (ffs(mask) - 1);
> > >
> > > - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, drvdata->mode_off[id], mask, val);
> > > }
> > >
> > > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> > > @@ -1376,6 +1372,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > .ltssm_off = IOMUXC_GPR12,
> > > .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > [IMX6SX] = {
> > > .variant = IMX6SX,
> > > @@ -1386,6 +1384,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> > > .ltssm_off = IOMUXC_GPR12,
> > > .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > [IMX6QP] = {
> > > .variant = IMX6QP,
> > > @@ -1397,6 +1397,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > .ltssm_off = IOMUXC_GPR12,
> > > .ltssm_mask = IMX6Q_GPR12_PCIE_CTL_2,
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > [IMX7D] = {
> > > .variant = IMX7D,
> > > @@ -1405,6 +1407,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_HAS_PHY_RESET,
> > > .gpr = "fsl,imx7d-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > [IMX8MQ] = {
> > > .variant = IMX8MQ,
> > > @@ -1412,6 +1416,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_HAS_PHY_RESET,
> > > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > + .mode_off[1] = IOMUXC_GPR12,
> > > + .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> >
> > Is the mode_mask differ between SoCs or fixed based on instances? I mean, if
> > there is a guarantee that it is going to be IMX6Q_GPR12_DEVICE_TYPE for instance
> > 1 and IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE for instance 2 etc...
> >
> > Then we can avoid these SoC specific config and simplify the code further.
>
> No, iMX95 will change it.
>
Ok, fine then.
- Mani
> Frank
>
> >
> > - Mani
> >
> > > },
> > > [IMX8MM] = {
> > > .variant = IMX8MM,
> > > @@ -1420,6 +1428,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_HAS_APP_RESET,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > [IMX8MP] = {
> > > .variant = IMX8MP,
> > > @@ -1428,6 +1438,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_HAS_APP_RESET,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > [IMX8MQ_EP] = {
> > > .variant = IMX8MQ_EP,
> > > @@ -1436,6 +1448,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > + .mode_off[1] = IOMUXC_GPR12,
> > > + .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> > > },
> > > [IMX8MM_EP] = {
> > > .variant = IMX8MM_EP,
> > > @@ -1443,6 +1459,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > [IMX8MP_EP] = {
> > > .variant = IMX8MP_EP,
> > > @@ -1450,6 +1468,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > + .mode_off[0] = IOMUXC_GPR12,
> > > + .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > },
> > > };
> > >
> > > --
> > > 2.34.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
On Sun, Jan 07, 2024 at 10:49:17AM +0530, Manivannan Sadhasivam wrote:
> On Sat, Jan 06, 2024 at 11:47:36PM -0500, Frank Li wrote:
> > On Sun, Jan 07, 2024 at 08:45:06AM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Dec 27, 2023 at 01:27:15PM -0500, Frank Li wrote:
> > > > iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
> > > > proptery for iMX8MQ to indicate pci controller index.
> > > >
> > >
> > > property
> > >
> > > > This adjustment paves the way for eliminating the hardcoded check on the
> > > > base register for acquiring the controller_id.
> > > >
> > > > ...
> > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > > imx6_pcie->controller_id = 1;
> > > > ...
> > > >
> > > > The controller_id is crucial and utilized for certain register bit
> > > > positions. It must align precisely with the controller index in the SoC.
> > > > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > > > may deactivate specific PCI controllers.
> > > >
> > >
> > > You cannot change the binding for the sake of driver. But you can make this
> > > change in other way. See below...
> > >
> > > > Signed-off-by: Frank Li <[email protected]>
> > > > ---
> > > >
> > > > Notes:
> > > > Change from v5 to v6
> > > > - rework commit message to explain why need required and why auto increase
> > > > id not work
> > > >
> > > > Change from v4 to v5
> > > > - new patch at v5
> > > >
> > > > .../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > index d91b639ae7ae7..8f39b4e6e8491 100644
> > > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > @@ -265,6 +265,17 @@ allOf:
> > > > - const: apps
> > > > - const: turnoff
> > > >
> > > > + - if:
> > > > + properties:
> > > > + compatible:
> > > > + contains:
> > > > + enum:
> > > > + - fsl,imx8mq-pcie
> > > > + - fsl,imx8mq-pcie-ep
> > >
> > > "linux,pci-domain" is a generic property. So you cannot make it required only
> > > for certain SoCs.
> >
> > Sorry, why not? there are many generic property.
> >
>
> It doesn't make sense to make it required only for specific SoCs since it is not
> specific to any SoC. You can make it required for all.
More than 2 controller need require "linux,pci-domain".
>
> > > But you can make it so for all SoCs. This way, the drivers
> > > can also rely on it.
> > >
> > > Now, you should get rid of the commit message about driver internals:
> >
> > Not all dts already added "linux,pci-domain" yet. If required for all SOCs,
> > it will cause dtb check warnings.
> >
>
> You can safely add this property to all DTS. Nothing will break.
Yes, but it will be off topic of this patch serial.
I can submit new patches for this later. After all dts changed, then I
remove this conditional check.
This patch serial is already quite big, (17 patches).
And I don't want to involve new DTB check warning.
Frank
>
> - Mani
>
> > Frank
> > >
> > > > This adjustment paves the way for eliminating the hardcoded check on the
> > > > base register for acquiring the controller_id.
> > > >
> > > > ...
> > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > > imx6_pcie->controller_id = 1;
> > > > ...
> > > >
> > > > The controller_id is crucial and utilized for certain register bit
> > > > positions. It must align precisely with the controller index in the SoC.
> > > > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > > > may deactivate specific PCI controllers.
> > > >
> > >
> > > - Mani
> > >
> > > > + then:
> > > > + required:
> > > > + - linux,pci-domain
> > > > +
> > > > additionalProperties: true
> > > >
> > > > ...
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்
>
> --
> மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:23PM -0500, Frank Li wrote:
Mention 'RC' in subject.
> Add iMX95 PCIe basic root complex function support.
>
Add iMX95 PCIe Root Complex support.
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v1 to v3
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 90 +++++++++++++++++++++++++--
> 1 file changed, 85 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index d66a2db53bdb7..9e60ab6f1885a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -42,6 +42,25 @@
> #define IMX8MQ_GPR_PCIE_VREG_BYPASS BIT(12)
> #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
>
> +#define IMX95_PCIE_PHY_GEN_CTRL 0x0
> +#define IMX95_PCIE_REF_USE_PAD BIT(17)
> +
> +#define IMX95_PCIE_PHY_MPLLA_CTRL 0x10
> +#define IMX95_PCIE_PHY_MPLL_STATE BIT(30)
> +
> +#define IMX95_PCIE_SS_RW_REG_0 0xf0
> +#define IMX95_PCIE_REF_CLKEN BIT(23)
> +#define IMX95_PCIE_PHY_CR_PARA_SEL BIT(9)
> +
> +#define IMX95_PE0_GEN_CTRL_1 0x1050
> +#define IMX95_PCIE_DEVICE_TYPE GENMASK(3, 0)
> +
> +#define IMX95_PE0_GEN_CTRL_3 0x1058
> +#define IMX95_PCIE_LTSSM_EN BIT(0)
> +
> +#define IMX95_PE0_PM_STS 0x1064
> +#define IMX95_PCIE_PM_LINKST_IN_L2 BIT(14)
> +
> #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
>
> enum imx6_pcie_variants {
> @@ -52,6 +71,7 @@ enum imx6_pcie_variants {
> IMX8MQ,
> IMX8MM,
> IMX8MP,
> + IMX95,
> IMX8MQ_EP,
> IMX8MM_EP,
> IMX8MP_EP,
> @@ -63,6 +83,7 @@ enum imx6_pcie_variants {
> #define IMX6_PCIE_FLAG_HAS_PHY BIT(3)
> #define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4)
> #define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5)
> +#define IMX6_PCIE_FLAG_HAS_SERDES BIT(6)
>
> #define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
>
> @@ -179,6 +200,24 @@ static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
> }
>
> +static int imx95_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> +{
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX95_PCIE_SS_RW_REG_0,
> + IMX95_PCIE_PHY_CR_PARA_SEL,
> + IMX95_PCIE_PHY_CR_PARA_SEL);
> +
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX95_PCIE_PHY_GEN_CTRL,
> + IMX95_PCIE_REF_USE_PAD, 0);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + IMX95_PCIE_SS_RW_REG_0,
> + IMX95_PCIE_REF_CLKEN,
> + IMX95_PCIE_REF_CLKEN);
> +
> + return 0;
> +}
> +
> static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
> {
> const struct imx6_pcie_drvdata *drvdata = imx6_pcie->drvdata;
> @@ -579,6 +618,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
> break;
> case IMX7D:
> + case IMX95:
> break;
> case IMX8MM:
> case IMX8MM_EP:
> @@ -696,10 +736,19 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> struct device *dev = pci->dev;
> + u32 val;
>
> reset_control_deassert(imx6_pcie->pciephy_reset);
>
> switch (imx6_pcie->drvdata->variant) {
> + case IMX95:
> + /* Polling the MPLL_STATE */
> + if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
> + IMX95_PCIE_PHY_MPLLA_CTRL, val,
> + val & IMX95_PCIE_PHY_MPLL_STATE,
> + 10, 10000))
> + dev_err(dev, "PCIe PLL lock timeout\n");
You should return err here because, if core deassert is not performed then the
core itself cannot be used.
> + break;
> case IMX7D:
> /* Workaround for ERR010728, failure of PCI-e PLL VCO to
> * oscillate, especially when cold. This turns off "Duty-cycle
> @@ -1281,12 +1330,32 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> return PTR_ERR(imx6_pcie->turnoff_reset);
> }
>
> + if (imx6_pcie->drvdata->gpr) {
> /* Grab GPR config register range */
> - imx6_pcie->iomuxc_gpr =
> - syscon_regmap_lookup_by_compatible(imx6_pcie->drvdata->gpr);
> - if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
> - dev_err(dev, "unable to find iomuxc registers\n");
> - return PTR_ERR(imx6_pcie->iomuxc_gpr);
> + imx6_pcie->iomuxc_gpr =
> + syscon_regmap_lookup_by_compatible(imx6_pcie->drvdata->gpr);
> + if (IS_ERR(imx6_pcie->iomuxc_gpr))
> + return dev_err_probe(dev, PTR_ERR(imx6_pcie->iomuxc_gpr),
> + "unable to find iomuxc registers\n");
> + }
> +
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_SERDES)) {
> + void __iomem *off = devm_platform_ioremap_resource_byname(pdev, "app");
> +
> + if (IS_ERR(off))
> + return dev_err_probe(dev, PTR_ERR(off),
> + "unable to find serdes registers\n");
> +
> + static struct regmap_config regmap_config = {
const
- Mani
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:24PM -0500, Frank Li wrote:
Subject: PCI: imx6: Rely on DWC core to fetch 'addr_space' region
> The common dw_pcie_ep_init() already do the same thing. Needn't platform
> driver do it again.
>
'Since the dw_pcie_ep_init() function is already fetching the 'addr_space'
region, no need to do the same in this driver.'
> Signed-off-by: Frank Li <[email protected]>
With above changes,
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
>
> Notes:
> Change from v1 to v3
> - new patches
>
> drivers/pci/controller/dwc/pci-imx6.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 9e60ab6f1885a..4b2b9aafad1b4 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1080,7 +1080,6 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
> int ret;
> unsigned int pcie_dbi2_offset;
> struct dw_pcie_ep *ep;
> - struct resource *res;
> struct dw_pcie *pci = imx6_pcie->pci;
> struct dw_pcie_rp *pp = &pci->pp;
> struct device *dev = pci->dev;
> @@ -1099,14 +1098,8 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
> pcie_dbi2_offset = SZ_4K;
> break;
> }
> - pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> - if (!res)
> - return -EINVAL;
>
> - ep->phys_base = res->start;
> - ep->addr_size = resource_size(res);
> - ep->page_size = SZ_64K;
> + pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
>
> ret = dw_pcie_ep_init(ep);
> if (ret) {
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:25PM -0500, Frank Li wrote:
> The i.MX EP exhibits variations in epc_features among different EP
> configurations. This introduces the addition of epc_features in
> imx6_pcie_drvdata to accommodate these differences. It's important to note
> that there are no functional changes in this commit; instead, it lays the
> groundwork for supporting i.MX95 EP functions.
>
> Signed-off-by: Frank Li <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
>
> Notes:
> Change from v5 to v6
> - add missed maxitems.
> - add comments about reuse linux,pci-domain as controller id.
> linux,pci-domain have not defined at PCI endpoint side.
>
> Change from v1 to v3
> - new patch at v3
>
> drivers/pci/controller/dwc/pci-imx6.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 4b2b9aafad1b4..6a58fd63a9dd2 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -104,6 +104,7 @@ struct imx6_pcie_drvdata {
> const u32 ltssm_mask;
> const u32 mode_off[IMX6_PCIE_MAX_INSTANCES];
> const u32 mode_mask[IMX6_PCIE_MAX_INSTANCES];
> + const struct pci_epc_features *epc_features;
> int (*init_phy)(struct imx6_pcie *pcie);
> };
>
> @@ -1065,7 +1066,10 @@ static const struct pci_epc_features imx8m_pcie_epc_features = {
> static const struct pci_epc_features*
> imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
> {
> - return &imx8m_pcie_epc_features;
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> + return imx6_pcie->drvdata->epc_features;
> }
>
> static const struct dw_pcie_ep_ops pcie_ep_ops = {
> @@ -1530,6 +1534,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> .mode_off[1] = IOMUXC_GPR12,
> .mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> + .epc_features = &imx8m_pcie_epc_features,
> .init_phy = imx8mq_pcie_init_phy,
> },
> [IMX8MM_EP] = {
> @@ -1540,6 +1545,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> .mode_off[0] = IOMUXC_GPR12,
> .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> + .epc_features = &imx8m_pcie_epc_features,
> },
> [IMX8MP_EP] = {
> .variant = IMX8MP_EP,
> @@ -1549,6 +1555,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> .mode_off[0] = IOMUXC_GPR12,
> .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> + .epc_features = &imx8m_pcie_epc_features,
> },
> };
>
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Wed, Dec 27, 2023 at 01:27:27PM -0500, Frank Li wrote:
Subject: PCI: imx6: Add iMX95 Endpoint (EP) support
> Add iMX95 EP function support and add 64bit address support. Internal bus
Remove 'function' as that refers to endpoint function.
> bridge for PCI support 64bit dma address in iMX95. So set call
> dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)).
>
'Hence, call dma_set_mask_and_coherent() to set 64 bit DMA mask.'
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v3 to v4
> - change align to 4k for imx95
> Change from v1 to v3
> - new patches at v3
>
> drivers/pci/controller/dwc/pci-imx6.c | 45 +++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6a58fd63a9dd2..00ec59867c17b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -75,6 +75,7 @@ enum imx6_pcie_variants {
> IMX8MQ_EP,
> IMX8MM_EP,
> IMX8MP_EP,
> + IMX95_EP,
> };
>
> #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> @@ -84,6 +85,7 @@ enum imx6_pcie_variants {
> #define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4)
> #define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5)
> #define IMX6_PCIE_FLAG_HAS_SERDES BIT(6)
> +#define IMX6_PCIE_FLAG_SUPPORT_64BIT BIT(7)
>
> #define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
>
> @@ -620,6 +622,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> break;
> case IMX7D:
> case IMX95:
> + case IMX95_EP:
> break;
> case IMX8MM:
> case IMX8MM_EP:
> @@ -1063,6 +1066,23 @@ static const struct pci_epc_features imx8m_pcie_epc_features = {
> .align = SZ_64K,
> };
>
> +/*
> + * BAR# | Default BAR enable | Default BAR Type | Default BAR Size | BAR Sizing Scheme
> + * ================================================================================================
> + * BAR0 | Enable | 64-bit | 1 MB | Programmable Size
> + * BAR1 | Disable | 32-bit | 64 KB | Fixed Size
> + * | (BAR0 is 64-bit) | if BAR0 is 32-bit | | As Bar0 is 64bit
I couldn't understand above. And not aligned properly.
> + * BAR2 | Enable | 32-bit | 1 MB | Programmable Size
> + * BAR3 | Enable | 32-bit | 64 KB | Programmable Size
> + * BAR4 | Enable | 32-bit | 1M | Programmable Size
> + * BAR5 | Enable | 32-bit | 64 KB | Programmable Size
> + */
> +static const struct pci_epc_features imx95_pcie_epc_features = {
> + .msi_capable = true,
> + .bar_fixed_size[1] = SZ_64K,
> + .align = SZ_4K,
> +};
> +
> static const struct pci_epc_features*
> imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
> {
> @@ -1105,6 +1125,14 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
>
> pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
>
> + /*
Use FIXME here.
- Mani
> + * db2 information should fetch from dtb file. dw_pcie_ep_init() can get dbi_base2 from
> + * "dbi2" if pci->dbi_base2 is NULL. All code related pcie_dbi2_offset should be removed
> + * after all dts added "dbi2" reg.
> + */
> + if (imx6_pcie->drvdata->variant == IMX95_EP)
> + pci->dbi_base2 = NULL;
> +
> ret = dw_pcie_ep_init(ep);
> if (ret) {
> dev_err(dev, "failed to initialize endpoint\n");
> @@ -1355,6 +1383,9 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> "unable to find iomuxc registers\n");
> }
>
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_SUPPORT_64BIT))
> + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +
> /* Grab PCIe PHY Tx Settings */
> if (of_property_read_u32(node, "fsl,tx-deemph-gen1",
> &imx6_pcie->tx_deemph_gen1))
> @@ -1557,6 +1588,19 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> .epc_features = &imx8m_pcie_epc_features,
> },
> + [IMX95_EP] = {
> + .variant = IMX95_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_SERDES |
> + IMX6_PCIE_FLAG_SUPPORT_64BIT,
> + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> + .ltssm_off = IMX95_PE0_GEN_CTRL_3,
> + .ltssm_mask = IMX95_PCIE_LTSSM_EN,
> + .mode_off[0] = IMX95_PE0_GEN_CTRL_1,
> + .mode_mask[0] = IMX95_PCIE_DEVICE_TYPE,
> + .init_phy = imx95_pcie_init_phy,
> + .epc_features = &imx95_pcie_epc_features,
> + .mode = DW_PCIE_EP_TYPE,
> + },
> };
>
> static const struct of_device_id imx6_pcie_of_match[] = {
> @@ -1571,6 +1615,7 @@ static const struct of_device_id imx6_pcie_of_match[] = {
> { .compatible = "fsl,imx8mq-pcie-ep", .data = &drvdata[IMX8MQ_EP], },
> { .compatible = "fsl,imx8mm-pcie-ep", .data = &drvdata[IMX8MM_EP], },
> { .compatible = "fsl,imx8mp-pcie-ep", .data = &drvdata[IMX8MP_EP], },
> + { .compatible = "fsl,imx95-pcie-ep", .data = &drvdata[IMX95_EP], },
> {},
> };
>
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Sun, Jan 07, 2024 at 12:38:10AM -0500, Frank Li wrote:
> On Sun, Jan 07, 2024 at 10:49:17AM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Jan 06, 2024 at 11:47:36PM -0500, Frank Li wrote:
> > > On Sun, Jan 07, 2024 at 08:45:06AM +0530, Manivannan Sadhasivam wrote:
> > > > On Wed, Dec 27, 2023 at 01:27:15PM -0500, Frank Li wrote:
> > > > > iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
> > > > > proptery for iMX8MQ to indicate pci controller index.
> > > > >
> > > >
> > > > property
> > > >
> > > > > This adjustment paves the way for eliminating the hardcoded check on the
> > > > > base register for acquiring the controller_id.
> > > > >
> > > > > ...
> > > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > > > imx6_pcie->controller_id = 1;
> > > > > ...
> > > > >
> > > > > The controller_id is crucial and utilized for certain register bit
> > > > > positions. It must align precisely with the controller index in the SoC.
> > > > > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > > > > may deactivate specific PCI controllers.
> > > > >
> > > >
> > > > You cannot change the binding for the sake of driver. But you can make this
> > > > change in other way. See below...
> > > >
> > > > > Signed-off-by: Frank Li <[email protected]>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > Change from v5 to v6
> > > > > - rework commit message to explain why need required and why auto increase
> > > > > id not work
> > > > >
> > > > > Change from v4 to v5
> > > > > - new patch at v5
> > > > >
> > > > > .../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > > index d91b639ae7ae7..8f39b4e6e8491 100644
> > > > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > > @@ -265,6 +265,17 @@ allOf:
> > > > > - const: apps
> > > > > - const: turnoff
> > > > >
> > > > > + - if:
> > > > > + properties:
> > > > > + compatible:
> > > > > + contains:
> > > > > + enum:
> > > > > + - fsl,imx8mq-pcie
> > > > > + - fsl,imx8mq-pcie-ep
> > > >
> > > > "linux,pci-domain" is a generic property. So you cannot make it required only
> > > > for certain SoCs.
> > >
> > > Sorry, why not? there are many generic property.
> > >
> >
> > It doesn't make sense to make it required only for specific SoCs since it is not
> > specific to any SoC. You can make it required for all.
>
> More than 2 controller need require "linux,pci-domain".
>
But this property is applicable to single controller also.
> >
> > > > But you can make it so for all SoCs. This way, the drivers
> > > > can also rely on it.
> > > >
> > > > Now, you should get rid of the commit message about driver internals:
> > >
> > > Not all dts already added "linux,pci-domain" yet. If required for all SOCs,
> > > it will cause dtb check warnings.
> > >
> >
> > You can safely add this property to all DTS. Nothing will break.
>
> Yes, but it will be off topic of this patch serial.
>
> I can submit new patches for this later. After all dts changed, then I
> remove this conditional check.
>
> This patch serial is already quite big, (17 patches).
>
> And I don't want to involve new DTB check warning.
>
Okay. But please follow up on this once this series gets merged.
- Mani
> Frank
>
> >
> > - Mani
> >
> > > Frank
> > > >
> > > > > This adjustment paves the way for eliminating the hardcoded check on the
> > > > > base register for acquiring the controller_id.
> > > > >
> > > > > ...
> > > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > > > imx6_pcie->controller_id = 1;
> > > > > ...
> > > > >
> > > > > The controller_id is crucial and utilized for certain register bit
> > > > > positions. It must align precisely with the controller index in the SoC.
> > > > > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > > > > may deactivate specific PCI controllers.
> > > > >
> > > >
> > > > - Mani
> > > >
> > > > > + then:
> > > > > + required:
> > > > > + - linux,pci-domain
> > > > > +
> > > > > additionalProperties: true
> > > > >
> > > > > ...
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
On Sun, Jan 07, 2024 at 11:56:54AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 27, 2023 at 01:27:27PM -0500, Frank Li wrote:
>
> Subject: PCI: imx6: Add iMX95 Endpoint (EP) support
>
> > Add iMX95 EP function support and add 64bit address support. Internal bus
>
> Remove 'function' as that refers to endpoint function.
>
> > bridge for PCI support 64bit dma address in iMX95. So set call
> > dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)).
> >
>
> 'Hence, call dma_set_mask_and_coherent() to set 64 bit DMA mask.'
>
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> >
> > Notes:
> > Change from v3 to v4
> > - change align to 4k for imx95
> > Change from v1 to v3
> > - new patches at v3
> >
> > drivers/pci/controller/dwc/pci-imx6.c | 45 +++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 6a58fd63a9dd2..00ec59867c17b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -75,6 +75,7 @@ enum imx6_pcie_variants {
> > IMX8MQ_EP,
> > IMX8MM_EP,
> > IMX8MP_EP,
> > + IMX95_EP,
> > };
> >
> > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> > @@ -84,6 +85,7 @@ enum imx6_pcie_variants {
> > #define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4)
> > #define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5)
> > #define IMX6_PCIE_FLAG_HAS_SERDES BIT(6)
> > +#define IMX6_PCIE_FLAG_SUPPORT_64BIT BIT(7)
> >
> > #define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
> >
> > @@ -620,6 +622,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > break;
> > case IMX7D:
> > case IMX95:
> > + case IMX95_EP:
> > break;
> > case IMX8MM:
> > case IMX8MM_EP:
> > @@ -1063,6 +1066,23 @@ static const struct pci_epc_features imx8m_pcie_epc_features = {
> > .align = SZ_64K,
> > };
> >
> > +/*
> > + * BAR# | Default BAR enable | Default BAR Type | Default BAR Size | BAR Sizing Scheme
> > + * ================================================================================================
> > + * BAR0 | Enable | 64-bit | 1 MB | Programmable Size
> > + * BAR1 | Disable | 32-bit | 64 KB | Fixed Size
> > + * | (BAR0 is 64-bit) | if BAR0 is 32-bit | | As Bar0 is 64bit
>
> I couldn't understand above. And not aligned properly.
If BAR0 is 64bit, BAR1 will be disable. (Supposed it is default).
If BAR0 is 32bit, BAR1 will 64KB fixed sized.
Aligned because 'tab' show problem in patch. It show algin at '|" at code.
Frank
>
> > + * BAR2 | Enable | 32-bit | 1 MB | Programmable Size
> > + * BAR3 | Enable | 32-bit | 64 KB | Programmable Size
> > + * BAR4 | Enable | 32-bit | 1M | Programmable Size
> > + * BAR5 | Enable | 32-bit | 64 KB | Programmable Size
> > + */
> > +static const struct pci_epc_features imx95_pcie_epc_features = {
> > + .msi_capable = true,
> > + .bar_fixed_size[1] = SZ_64K,
> > + .align = SZ_4K,
> > +};
> > +
> > static const struct pci_epc_features*
> > imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
> > {
> > @@ -1105,6 +1125,14 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
> >
> > pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
> >
> > + /*
>
> Use FIXME here.
>
> - Mani
>
> > + * db2 information should fetch from dtb file. dw_pcie_ep_init() can get dbi_base2 from
> > + * "dbi2" if pci->dbi_base2 is NULL. All code related pcie_dbi2_offset should be removed
> > + * after all dts added "dbi2" reg.
> > + */
> > + if (imx6_pcie->drvdata->variant == IMX95_EP)
> > + pci->dbi_base2 = NULL;
> > +
> > ret = dw_pcie_ep_init(ep);
> > if (ret) {
> > dev_err(dev, "failed to initialize endpoint\n");
> > @@ -1355,6 +1383,9 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > "unable to find iomuxc registers\n");
> > }
> >
> > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_SUPPORT_64BIT))
> > + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > +
> > /* Grab PCIe PHY Tx Settings */
> > if (of_property_read_u32(node, "fsl,tx-deemph-gen1",
> > &imx6_pcie->tx_deemph_gen1))
> > @@ -1557,6 +1588,19 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > .epc_features = &imx8m_pcie_epc_features,
> > },
> > + [IMX95_EP] = {
> > + .variant = IMX95_EP,
> > + .flags = IMX6_PCIE_FLAG_HAS_SERDES |
> > + IMX6_PCIE_FLAG_SUPPORT_64BIT,
> > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > + .ltssm_off = IMX95_PE0_GEN_CTRL_3,
> > + .ltssm_mask = IMX95_PCIE_LTSSM_EN,
> > + .mode_off[0] = IMX95_PE0_GEN_CTRL_1,
> > + .mode_mask[0] = IMX95_PCIE_DEVICE_TYPE,
> > + .init_phy = imx95_pcie_init_phy,
> > + .epc_features = &imx95_pcie_epc_features,
> > + .mode = DW_PCIE_EP_TYPE,
> > + },
> > };
> >
> > static const struct of_device_id imx6_pcie_of_match[] = {
> > @@ -1571,6 +1615,7 @@ static const struct of_device_id imx6_pcie_of_match[] = {
> > { .compatible = "fsl,imx8mq-pcie-ep", .data = &drvdata[IMX8MQ_EP], },
> > { .compatible = "fsl,imx8mm-pcie-ep", .data = &drvdata[IMX8MM_EP], },
> > { .compatible = "fsl,imx8mp-pcie-ep", .data = &drvdata[IMX8MP_EP], },
> > + { .compatible = "fsl,imx95-pcie-ep", .data = &drvdata[IMX95_EP], },
> > {},
> > };
> >
> > --
> > 2.34.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On Sun, Jan 07, 2024 at 11:59:11AM +0530, Manivannan Sadhasivam wrote:
> On Sun, Jan 07, 2024 at 12:38:10AM -0500, Frank Li wrote:
> > On Sun, Jan 07, 2024 at 10:49:17AM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Jan 06, 2024 at 11:47:36PM -0500, Frank Li wrote:
> > > > On Sun, Jan 07, 2024 at 08:45:06AM +0530, Manivannan Sadhasivam wrote:
> > > > > On Wed, Dec 27, 2023 at 01:27:15PM -0500, Frank Li wrote:
> > > > > > iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
> > > > > > proptery for iMX8MQ to indicate pci controller index.
> > > > > >
> > > > >
> > > > > property
> > > > >
> > > > > > This adjustment paves the way for eliminating the hardcoded check on the
> > > > > > base register for acquiring the controller_id.
> > > > > >
> > > > > > ...
> > > > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > > > > imx6_pcie->controller_id = 1;
> > > > > > ...
> > > > > >
> > > > > > The controller_id is crucial and utilized for certain register bit
> > > > > > positions. It must align precisely with the controller index in the SoC.
> > > > > > An auto-incremented ID don't fit this case. The DTS or fuse configurations
> > > > > > may deactivate specific PCI controllers.
> > > > > >
> > > > >
> > > > > You cannot change the binding for the sake of driver. But you can make this
> > > > > change in other way. See below...
> > > > >
> > > > > > Signed-off-by: Frank Li <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > Notes:
> > > > > > Change from v5 to v6
> > > > > > - rework commit message to explain why need required and why auto increase
> > > > > > id not work
> > > > > >
> > > > > > Change from v4 to v5
> > > > > > - new patch at v5
> > > > > >
> > > > > > .../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
> > > > > > 1 file changed, 11 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > > > index d91b639ae7ae7..8f39b4e6e8491 100644
> > > > > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> > > > > > @@ -265,6 +265,17 @@ allOf:
> > > > > > - const: apps
> > > > > > - const: turnoff
> > > > > >
> > > > > > + - if:
> > > > > > + properties:
> > > > > > + compatible:
> > > > > > + contains:
> > > > > > + enum:
> > > > > > + - fsl,imx8mq-pcie
> > > > > > + - fsl,imx8mq-pcie-ep
> > > > >
> > > > > "linux,pci-domain" is a generic property. So you cannot make it required only
> > > > > for certain SoCs.
> > > >
> > > > Sorry, why not? there are many generic property.
> > > >
> > >
> > > It doesn't make sense to make it required only for specific SoCs since it is not
> > > specific to any SoC. You can make it required for all.
> >
> > More than 2 controller need require "linux,pci-domain".
> >
>
> But this property is applicable to single controller also.
Not really.
I don't understand the issue. Some SoCs have a dependency on the
numbering and need the property. Others don't. They just want
(but don't need) consistent numbering.
Rob
On Wed, 27 Dec 2023 13:27:15 -0500, Frank Li wrote:
> iMX8MQ have two pci controllers. Adds "linux,pci-domain" as required
> proptery for iMX8MQ to indicate pci controller index.
>
> This adjustment paves the way for eliminating the hardcoded check on the
> base register for acquiring the controller_id.
>
> ...
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> imx6_pcie->controller_id = 1;
> ...
>
> The controller_id is crucial and utilized for certain register bit
> positions. It must align precisely with the controller index in the SoC.
> An auto-incremented ID don't fit this case. The DTS or fuse configurations
> may deactivate specific PCI controllers.
>
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v5 to v6
> - rework commit message to explain why need required and why auto increase
> id not work
>
> Change from v4 to v5
> - new patch at v5
>
> .../bindings/pci/fsl,imx6q-pcie-common.yaml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Wed, 27 Dec 2023 13:27:26 -0500, Frank Li wrote:
> Add i.MX95 PCIe "fsl,imx95-pcie-ep" compatible string.
> Add reg-name: "atu", "dbi2", "dma" and "app".
> Reuse PCI linux,pci-domain as controller id at endpoint.
>
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v1 to v3
> - new patches at v3
>
> .../bindings/pci/fsl,imx6q-pcie-ep.yaml | 57 ++++++++++++++++---
> 1 file changed, 49 insertions(+), 8 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>