| Message ID | 20231211215842.134823-2-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: imx6: Clean up and add imx95 pci support | expand |
On Mon, Dec 11, 2023 at 04:58:30PM -0500, Frank Li wrote: > Refactors the clock handling logic in the imx6 PCI driver by adding > HAS_CLK_* bitmask define for drvdata::flags . Simplifies the code and makes > it more maintainable, as future additions of SOC support will only require > straightforward changes. The drvdata::flags and a bitmask ensures a cleaner > and more scalable switch-case structure for handling clocks. > Is there any necessity to validate each clock in the driver? I mean, can't you just rely on devicetree to provide enough clocks for the functioning of the PCIe controller? If you can rely on devicetree (everyone should in an ideal world), then you can just use devm_clk_bulk_get_all() to get all available clocks for the SoC and just enable/disable whatever is available. This will greatly simplify the code. Only downside of this approach is, if the devicetree is not supplying enough clocks, then it will be difficult to find why PCIe is not working. But this also means that the devicetree is screwed. - Mani > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > > Notes: > Change from v1 to v3 > - none > > drivers/pci/controller/dwc/pci-imx6.c | 84 +++++++++++++-------------- > 1 file changed, 42 insertions(+), 42 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 74703362aeec7..8a9b527934f80 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -60,6 +60,10 @@ 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_CLK_INBOUND_AXI BIT(3) > +#define IMX6_PCIE_FLAG_HAS_CLK_AUX BIT(4) > + > +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val) > > struct imx6_pcie_drvdata { > enum imx6_pcie_variants variant; > @@ -550,19 +554,23 @@ 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: > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) { > ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi); > - if (ret) { > - dev_err(dev, "unable to enable pcie_axi clock\n"); > - break; > - } > + if (ret) > + return ret; > + } > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) { > + ret = clk_prepare_enable(imx6_pcie->pcie_aux); > + if (ret) > + return ret; > + } > + > + switch (imx6_pcie->drvdata->variant) { > + case IMX6SX: > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0); > break; > @@ -589,12 +597,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 > @@ -614,10 +616,13 @@ 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: > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) > clk_disable_unprepare(imx6_pcie->pcie_inbound_axi); > - break; > + > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) > + clk_disable_unprepare(imx6_pcie->pcie_aux); > + > + switch (imx6_pcie->drvdata->variant) { > case IMX6QP: > case IMX6Q: > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > @@ -631,14 +636,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; > } > @@ -1316,21 +1313,21 @@ static int imx6_pcie_probe(struct platform_device *pdev) > return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie), > "pcie clock source missing or invalid\n"); > > - switch (imx6_pcie->drvdata->variant) { > - case IMX6SX: > - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev, > - "pcie_inbound_axi"); > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) { > + 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: > + dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi), > + "pcie_inbound_axi clock missing or invalid\n"); > + } > + > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) { > 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; > + } > + > + switch (imx6_pcie->drvdata->variant) { > case IMX7D: > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > imx6_pcie->controller_id = 1; > @@ -1353,10 +1350,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)) > @@ -1482,7 +1475,8 @@ static const struct imx6_pcie_drvdata drvdata[] = { > .variant = IMX6SX, > .flags = IMX6_PCIE_FLAG_IMX6_PHY | > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE | > - IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > + IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > + IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI, > .gpr = "fsl,imx6q-iomuxc-gpr", > }, > [IMX6QP] = { > @@ -1500,30 +1494,36 @@ static const struct imx6_pcie_drvdata drvdata[] = { > }, > [IMX8MQ] = { > .variant = IMX8MQ, > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > .gpr = "fsl,imx8mq-iomuxc-gpr", > }, > [IMX8MM] = { > .variant = IMX8MM, > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > + IMX6_PCIE_FLAG_HAS_CLK_AUX, > .gpr = "fsl,imx8mm-iomuxc-gpr", > }, > [IMX8MP] = { > .variant = IMX8MP, > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > + IMX6_PCIE_FLAG_HAS_CLK_AUX, > .gpr = "fsl,imx8mp-iomuxc-gpr", > }, > [IMX8MQ_EP] = { > .variant = IMX8MQ_EP, > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > .mode = DW_PCIE_EP_TYPE, > .gpr = "fsl,imx8mq-iomuxc-gpr", > }, > [IMX8MM_EP] = { > .variant = IMX8MM_EP, > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > .mode = DW_PCIE_EP_TYPE, > .gpr = "fsl,imx8mm-iomuxc-gpr", > }, > [IMX8MP_EP] = { > .variant = IMX8MP_EP, > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > .mode = DW_PCIE_EP_TYPE, > .gpr = "fsl,imx8mp-iomuxc-gpr", > }, > -- > 2.34.1 >
On Tue, Dec 12, 2023 at 10:19:13PM +0530, Manivannan Sadhasivam wrote: > On Mon, Dec 11, 2023 at 04:58:30PM -0500, Frank Li wrote: > > Refactors the clock handling logic in the imx6 PCI driver by adding > > HAS_CLK_* bitmask define for drvdata::flags . Simplifies the code and makes > > it more maintainable, as future additions of SOC support will only require > > straightforward changes. The drvdata::flags and a bitmask ensures a cleaner > > and more scalable switch-case structure for handling clocks. > > > > Is there any necessity to validate each clock in the driver? I mean, can't you > just rely on devicetree to provide enough clocks for the functioning of the PCIe > controller? > > If you can rely on devicetree (everyone should in an ideal world), then you can > just use devm_clk_bulk_get_all() to get all available clocks for the SoC and > just enable/disable whatever is available. > > This will greatly simplify the code. > > Only downside of this approach is, if the devicetree is not supplying enough > clocks, then it will be difficult to find why PCIe is not working. But this also > means that the devicetree is screwed. I have idea, how about add clk name array in drvdata. such as clk_names[] = {"bus", "aux", "axi", ...} clk_cnt = 4. Then use devm_clk_blk_get_all(). Code will be simplied and not too much depend on devicetree's validate. Frank > > - Mani > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > > > Notes: > > Change from v1 to v3 > > - none > > > > drivers/pci/controller/dwc/pci-imx6.c | 84 +++++++++++++-------------- > > 1 file changed, 42 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index 74703362aeec7..8a9b527934f80 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -60,6 +60,10 @@ 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_CLK_INBOUND_AXI BIT(3) > > +#define IMX6_PCIE_FLAG_HAS_CLK_AUX BIT(4) > > + > > +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val) > > > > struct imx6_pcie_drvdata { > > enum imx6_pcie_variants variant; > > @@ -550,19 +554,23 @@ 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: > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) { > > ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi); > > - if (ret) { > > - dev_err(dev, "unable to enable pcie_axi clock\n"); > > - break; > > - } > > + if (ret) > > + return ret; > > + } > > > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) { > > + ret = clk_prepare_enable(imx6_pcie->pcie_aux); > > + if (ret) > > + return ret; > > + } > > + > > + switch (imx6_pcie->drvdata->variant) { > > + case IMX6SX: > > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > > IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0); > > break; > > @@ -589,12 +597,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 > > @@ -614,10 +616,13 @@ 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: > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) > > clk_disable_unprepare(imx6_pcie->pcie_inbound_axi); > > - break; > > + > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) > > + clk_disable_unprepare(imx6_pcie->pcie_aux); > > + > > + switch (imx6_pcie->drvdata->variant) { > > case IMX6QP: > > case IMX6Q: > > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > > @@ -631,14 +636,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; > > } > > @@ -1316,21 +1313,21 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie), > > "pcie clock source missing or invalid\n"); > > > > - switch (imx6_pcie->drvdata->variant) { > > - case IMX6SX: > > - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev, > > - "pcie_inbound_axi"); > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) { > > + 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: > > + dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi), > > + "pcie_inbound_axi clock missing or invalid\n"); > > + } > > + > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) { > > 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; > > + } > > + > > + switch (imx6_pcie->drvdata->variant) { > > case IMX7D: > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > > imx6_pcie->controller_id = 1; > > @@ -1353,10 +1350,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)) > > @@ -1482,7 +1475,8 @@ static const struct imx6_pcie_drvdata drvdata[] = { > > .variant = IMX6SX, > > .flags = IMX6_PCIE_FLAG_IMX6_PHY | > > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE | > > - IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > > + IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > > + IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI, > > .gpr = "fsl,imx6q-iomuxc-gpr", > > }, > > [IMX6QP] = { > > @@ -1500,30 +1494,36 @@ static const struct imx6_pcie_drvdata drvdata[] = { > > }, > > [IMX8MQ] = { > > .variant = IMX8MQ, > > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > > .gpr = "fsl,imx8mq-iomuxc-gpr", > > }, > > [IMX8MM] = { > > .variant = IMX8MM, > > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > > + IMX6_PCIE_FLAG_HAS_CLK_AUX, > > .gpr = "fsl,imx8mm-iomuxc-gpr", > > }, > > [IMX8MP] = { > > .variant = IMX8MP, > > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > > + IMX6_PCIE_FLAG_HAS_CLK_AUX, > > .gpr = "fsl,imx8mp-iomuxc-gpr", > > }, > > [IMX8MQ_EP] = { > > .variant = IMX8MQ_EP, > > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > > .mode = DW_PCIE_EP_TYPE, > > .gpr = "fsl,imx8mq-iomuxc-gpr", > > }, > > [IMX8MM_EP] = { > > .variant = IMX8MM_EP, > > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > > .mode = DW_PCIE_EP_TYPE, > > .gpr = "fsl,imx8mm-iomuxc-gpr", > > }, > > [IMX8MP_EP] = { > > .variant = IMX8MP_EP, > > + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, > > .mode = DW_PCIE_EP_TYPE, > > .gpr = "fsl,imx8mp-iomuxc-gpr", > > }, > > -- > > 2.34.1 > > > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Dec 12, 2023 at 10:19:13PM +0530, Manivannan Sadhasivam wrote: > On Mon, Dec 11, 2023 at 04:58:30PM -0500, Frank Li wrote: > > Refactors the clock handling logic in the imx6 PCI driver by adding > > HAS_CLK_* bitmask define for drvdata::flags . Simplifies the code and makes > > it more maintainable, as future additions of SOC support will only require > > straightforward changes. The drvdata::flags and a bitmask ensures a cleaner > > and more scalable switch-case structure for handling clocks. > > > > Is there any necessity to validate each clock in the driver? I mean, can't you > just rely on devicetree to provide enough clocks for the functioning of the PCIe > controller? > > If you can rely on devicetree (everyone should in an ideal world), then you can > just use devm_clk_bulk_get_all() to get all available clocks for the SoC and > just enable/disable whatever is available. Or just use the *_get_optional() variants of functions. They return NULL such that subsequent calls are just NOPs if the resource is not present. Of course, they aren't really optional on any given platform in this case, but does that really matter. There isn't an optional variant for phys, but it can be added. > > This will greatly simplify the code. > > Only downside of this approach is, if the devicetree is not supplying enough > clocks, then it will be difficult to find why PCIe is not working. But this also > means that the devicetree is screwed. A sufficient schema should prevent that... That's what they're for, not just torturing people to learn json-schema. :) Rob
On Tue, Dec 12, 2023 at 04:54:26PM -0600, Rob Herring wrote: > On Tue, Dec 12, 2023 at 10:19:13PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Dec 11, 2023 at 04:58:30PM -0500, Frank Li wrote: > > > Refactors the clock handling logic in the imx6 PCI driver by adding > > > HAS_CLK_* bitmask define for drvdata::flags . Simplifies the code and makes > > > it more maintainable, as future additions of SOC support will only require > > > straightforward changes. The drvdata::flags and a bitmask ensures a cleaner > > > and more scalable switch-case structure for handling clocks. > > > > > > > Is there any necessity to validate each clock in the driver? I mean, can't you > > just rely on devicetree to provide enough clocks for the functioning of the PCIe > > controller? > > > > If you can rely on devicetree (everyone should in an ideal world), then you can > > just use devm_clk_bulk_get_all() to get all available clocks for the SoC and > > just enable/disable whatever is available. > > Or just use the *_get_optional() variants of functions. They return NULL > such that subsequent calls are just NOPs if the resource is not present. > Of course, they aren't really optional on any given platform in this > case, but does that really matter. I think it'd better use devm_clk_bulk_get() by passed down a clk name list .clk_names = {"pcie_aux", ...}, So it will not silient when dts are not matched requirement. It is not complex because only check once at probe. Frank > > There isn't an optional variant for phys, but it can be added. > > > > > This will greatly simplify the code. > > > > Only downside of this approach is, if the devicetree is not supplying enough > > clocks, then it will be difficult to find why PCIe is not working. But this also > > means that the devicetree is screwed. > > A sufficient schema should prevent that... That's what they're for, not > just torturing people to learn json-schema. :) > > Rob
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 74703362aeec7..8a9b527934f80 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -60,6 +60,10 @@ 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_CLK_INBOUND_AXI BIT(3) +#define IMX6_PCIE_FLAG_HAS_CLK_AUX BIT(4) + +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val) struct imx6_pcie_drvdata { enum imx6_pcie_variants variant; @@ -550,19 +554,23 @@ 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: + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) { ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi); - if (ret) { - dev_err(dev, "unable to enable pcie_axi clock\n"); - break; - } + if (ret) + return ret; + } + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) { + ret = clk_prepare_enable(imx6_pcie->pcie_aux); + if (ret) + return ret; + } + + switch (imx6_pcie->drvdata->variant) { + case IMX6SX: regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0); break; @@ -589,12 +597,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 @@ -614,10 +616,13 @@ 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: + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) clk_disable_unprepare(imx6_pcie->pcie_inbound_axi); - break; + + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) + clk_disable_unprepare(imx6_pcie->pcie_aux); + + switch (imx6_pcie->drvdata->variant) { case IMX6QP: case IMX6Q: regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, @@ -631,14 +636,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; } @@ -1316,21 +1313,21 @@ static int imx6_pcie_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie), "pcie clock source missing or invalid\n"); - switch (imx6_pcie->drvdata->variant) { - case IMX6SX: - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev, - "pcie_inbound_axi"); + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) { + 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: + dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi), + "pcie_inbound_axi clock missing or invalid\n"); + } + + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) { 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; + } + + switch (imx6_pcie->drvdata->variant) { case IMX7D: if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) imx6_pcie->controller_id = 1; @@ -1353,10 +1350,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)) @@ -1482,7 +1475,8 @@ static const struct imx6_pcie_drvdata drvdata[] = { .variant = IMX6SX, .flags = IMX6_PCIE_FLAG_IMX6_PHY | IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE | - IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, + IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | + IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI, .gpr = "fsl,imx6q-iomuxc-gpr", }, [IMX6QP] = { @@ -1500,30 +1494,36 @@ static const struct imx6_pcie_drvdata drvdata[] = { }, [IMX8MQ] = { .variant = IMX8MQ, + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, .gpr = "fsl,imx8mq-iomuxc-gpr", }, [IMX8MM] = { .variant = IMX8MM, - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | + IMX6_PCIE_FLAG_HAS_CLK_AUX, .gpr = "fsl,imx8mm-iomuxc-gpr", }, [IMX8MP] = { .variant = IMX8MP, - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | + IMX6_PCIE_FLAG_HAS_CLK_AUX, .gpr = "fsl,imx8mp-iomuxc-gpr", }, [IMX8MQ_EP] = { .variant = IMX8MQ_EP, + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, .mode = DW_PCIE_EP_TYPE, .gpr = "fsl,imx8mq-iomuxc-gpr", }, [IMX8MM_EP] = { .variant = IMX8MM_EP, + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, .mode = DW_PCIE_EP_TYPE, .gpr = "fsl,imx8mm-iomuxc-gpr", }, [IMX8MP_EP] = { .variant = IMX8MP_EP, + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX, .mode = DW_PCIE_EP_TYPE, .gpr = "fsl,imx8mp-iomuxc-gpr", },
Refactors the clock handling logic in the imx6 PCI driver by adding HAS_CLK_* bitmask define for drvdata::flags . Simplifies the code and makes it more maintainable, as future additions of SOC support will only require straightforward changes. The drvdata::flags and a bitmask ensures a cleaner and more scalable switch-case structure for handling clocks. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Notes: Change from v1 to v3 - none drivers/pci/controller/dwc/pci-imx6.c | 84 +++++++++++++-------------- 1 file changed, 42 insertions(+), 42 deletions(-)