| Message ID | 20231227182727.1747435-2-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: imx6: Clean up and add imx95 pci support | expand |
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 <Frank.Li@nxp.com> > --- > > 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 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 <Frank.Li@nxp.com> > > --- > > > > 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 <Frank.Li@nxp.com> > --- > > 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 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 <Frank.Li@nxp.com> > > --- > > > > 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 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 <Frank.Li@nxp.com> > > > --- > > > > > > 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 > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
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"}, }, };
Refactors the clock handling logic. Adds clk_names[] define in drvdata. Using clk_bulk*() api simplifies the code. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- 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(-)