Message ID | 20230526062529.46747-3-william.qiu@starfivetech.com |
---|---|
State | New |
Headers | show |
Series | Add initialization of clock for StarFive JH7110 SoC | expand |
On Fri, May 26, 2023 at 02:25:28PM +0800, William Qiu wrote: > if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) { > + qspi_ahb = devm_clk_get(dev, "qspi-ahb"); > + if (IS_ERR(qspi_ahb)) { > + dev_err(dev, "Cannot claim QSPI_AHB clock.\n"); > + ret = PTR_ERR(qspi_ahb); > + return ret; > + } > + > + ret = clk_prepare_enable(qspi_ahb); > + if (ret) { > + dev_err(dev, "Cannot enable QSPI AHB clock.\n"); > + goto probe_clk_failed; > + } Nothing ever disables or unprepares this clock as far as I can tell? Perhaps also consider using the clk_bulk_ APIs.
On 2023/5/26 23:36, Mark Brown wrote: > On Fri, May 26, 2023 at 02:25:28PM +0800, William Qiu wrote: > >> if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) { >> + qspi_ahb = devm_clk_get(dev, "qspi-ahb"); >> + if (IS_ERR(qspi_ahb)) { >> + dev_err(dev, "Cannot claim QSPI_AHB clock.\n"); >> + ret = PTR_ERR(qspi_ahb); >> + return ret; >> + } >> + >> + ret = clk_prepare_enable(qspi_ahb); >> + if (ret) { >> + dev_err(dev, "Cannot enable QSPI AHB clock.\n"); >> + goto probe_clk_failed; >> + } > > Nothing ever disables or unprepares this clock as far as I can tell? > Perhaps also consider using the clk_bulk_ APIs. I will add in next version. Thanks for taking time to review this patch series and give useful suggestions. Best regards, William
On 2023/5/29 14:44, William Qiu wrote: > > > On 2023/5/26 23:36, Mark Brown wrote: >> On Fri, May 26, 2023 at 02:25:28PM +0800, William Qiu wrote: >> >>> if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) { >>> + qspi_ahb = devm_clk_get(dev, "qspi-ahb"); >>> + if (IS_ERR(qspi_ahb)) { >>> + dev_err(dev, "Cannot claim QSPI_AHB clock.\n"); >>> + ret = PTR_ERR(qspi_ahb); >>> + return ret; >>> + } >>> + >>> + ret = clk_prepare_enable(qspi_ahb); >>> + if (ret) { >>> + dev_err(dev, "Cannot enable QSPI AHB clock.\n"); >>> + goto probe_clk_failed; >>> + } >> >> Nothing ever disables or unprepares this clock as far as I can tell? >> Perhaps also consider using the clk_bulk_ APIs. > > I will add in next version. > > Thanks for taking time to review this patch series and give useful > suggestions. > > Best regards, > William Hi Mark, Now I want to replace the original devm_clk_get API in the driver with devm_clk_bulk_get_all API, which can achieve compatibility, but it seems that it is not good for other ip with only one clock, so I want to ask about that can I replace it? Or define that inside jh7110? Best regards, William
On Tue, May 30, 2023 at 10:05:38AM +0800, William Qiu wrote: > On 2023/5/29 14:44, William Qiu wrote: > > On 2023/5/26 23:36, Mark Brown wrote: > >> Nothing ever disables or unprepares this clock as far as I can tell? > >> Perhaps also consider using the clk_bulk_ APIs. > > I will add in next version. > Now I want to replace the original devm_clk_get API in the > driver with devm_clk_bulk_get_all API, which can achieve compatibility, > but it seems that it is not good for other ip with only one clock, so I > want to ask about that can I replace it? Or define that inside jh7110? You could always specify a different array of clocks depending on which compatible the driver sees, just like you'd conditionally request clocks individually.
On 2023/5/30 18:33, Mark Brown wrote: > On Tue, May 30, 2023 at 10:05:38AM +0800, William Qiu wrote: >> On 2023/5/29 14:44, William Qiu wrote: >> > On 2023/5/26 23:36, Mark Brown wrote: > >> >> Nothing ever disables or unprepares this clock as far as I can tell? >> >> Perhaps also consider using the clk_bulk_ APIs. > >> > I will add in next version. > >> Now I want to replace the original devm_clk_get API in the >> driver with devm_clk_bulk_get_all API, which can achieve compatibility, >> but it seems that it is not good for other ip with only one clock, so I >> want to ask about that can I replace it? Or define that inside jh7110? > > You could always specify a different array of clocks depending on which > compatible the driver sees, just like you'd conditionally request clocks > individually. Hi Mark, If specify a different array of clocks depending on which compatible the driver sees, since there will also be clock operations in the suspend and resume interfaces, this can make the code look complicated. My thoughts are as follows: Modify the following code 1658 /* Obtain QSPI clock. */ 1659 cqspi->clk = devm_clk_get(dev, NULL); 1660 if (IS_ERR(cqspi->clk)) { 1661 dev_err(dev, "Cannot claim QSPI clock.\n"); 1662 ret = PTR_ERR(cqspi->clk); 1663 return ret; 1664 } as following: /* Obtain QSPI clock. */ cqspi->num_clks = devm_clk_bulk_get_all(dev, &cqspi->clks); if (cqspi->num_clks < 0) { dev_err(dev, "Cannot claim QSPI clock: %u\n", cqspi->num_clks); return -EINVAL; } This way, the code will look simpler and clearer. How do you think about it. Best Regards, William
On Wed, May 31, 2023 at 02:19:16PM +0800, William Qiu wrote: > On 2023/5/30 18:33, Mark Brown wrote: > > You could always specify a different array of clocks depending on which > > compatible the driver sees, just like you'd conditionally request clocks > > individually. > If specify a different array of clocks depending on which compatible > the driver sees, since there will also be clock operations in the suspend > and resume interfaces, this can make the code look complicated. If you store the clock count and array in the driver data that should be fairly simple I think. > as following: > /* Obtain QSPI clock. */ > cqspi->num_clks = devm_clk_bulk_get_all(dev, &cqspi->clks); > if (cqspi->num_clks < 0) { > dev_err(dev, "Cannot claim QSPI clock: %u\n", cqspi->num_clks); > return -EINVAL; > } > This way, the code will look simpler and clearer. How do you think > about it. I'm not clear how enable and disable would then work?
On 2023/5/31 21:20, Mark Brown wrote: > On Wed, May 31, 2023 at 02:19:16PM +0800, William Qiu wrote: >> On 2023/5/30 18:33, Mark Brown wrote: > >> > You could always specify a different array of clocks depending on which >> > compatible the driver sees, just like you'd conditionally request clocks >> > individually. > >> If specify a different array of clocks depending on which compatible >> the driver sees, since there will also be clock operations in the suspend >> and resume interfaces, this can make the code look complicated. > > If you store the clock count and array in the driver data that should be > fairly simple I think. > >> as following: > >> /* Obtain QSPI clock. */ >> cqspi->num_clks = devm_clk_bulk_get_all(dev, &cqspi->clks); >> if (cqspi->num_clks < 0) { >> dev_err(dev, "Cannot claim QSPI clock: %u\n", cqspi->num_clks); >> return -EINVAL; >> } > >> This way, the code will look simpler and clearer. How do you think >> about it. > > I'm not clear how enable and disable would then work? enable use this API: clk_bulk_prepare_enable(dev->num_clks, dev->clks); then disable: clk_bulk_disable_unprepare(dev->num_clks, dev->clks); But I'll first try specify a different array of clocks depending on which compatible the driver sees first. Best regards, William
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 6ddb2dfc0f00..c6430fb3a0a4 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -1624,6 +1624,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi) static int cqspi_probe(struct platform_device *pdev) { const struct cqspi_driver_platdata *ddata; + struct clk *qspi_ahb, *qspi_apb; struct reset_control *rstc, *rstc_ocp, *rstc_ref; struct device *dev = &pdev->dev; struct spi_master *master; @@ -1715,6 +1716,32 @@ static int cqspi_probe(struct platform_device *pdev) } if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) { + qspi_ahb = devm_clk_get(dev, "qspi-ahb"); + if (IS_ERR(qspi_ahb)) { + dev_err(dev, "Cannot claim QSPI_AHB clock.\n"); + ret = PTR_ERR(qspi_ahb); + return ret; + } + + ret = clk_prepare_enable(qspi_ahb); + if (ret) { + dev_err(dev, "Cannot enable QSPI AHB clock.\n"); + goto probe_clk_failed; + } + + qspi_apb = devm_clk_get(dev, "qspi-apb"); + if (IS_ERR(qspi_apb)) { + dev_err(dev, "Cannot claim QSPI_APB clock.\n"); + ret = PTR_ERR(qspi_apb); + return ret; + } + + ret = clk_prepare_enable(qspi_apb); + if (ret) { + dev_err(dev, "Cannot enable QSPI APB clock.\n"); + goto probe_clk_failed; + } + rstc_ref = devm_reset_control_get_optional_exclusive(dev, "rstc_ref"); if (IS_ERR(rstc_ref)) { ret = PTR_ERR(rstc_ref);