| Message ID | 20240708121018.246476-3-ciprianmarian.costea@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | address S32G2/S32G3 SoC based boards particularities | expand |
On 8/07/24 15:10, Ciprian Costea wrote: > The I.MX SDHCI driver assumes that the frequency of the 'per' clock > can be obtained even on disabled clocks, which is not always the case. > > According to 'clk_get_rate' documentation, it is only valid > once the clock source has been enabled. The kerneldoc comment for clk_get_rate() says Can be called regardless of the clock enabledness which sounds like the opposite. Please clarify. > > Signed-off-by: Ciprian Costea <ciprianmarian.costea@oss.nxp.com> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 21d984a77be8..8f0bc6dca2b0 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -1709,7 +1709,6 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > } > > pltfm_host->clk = imx_data->clk_per; > - pltfm_host->clock = clk_get_rate(pltfm_host->clk); > err = clk_prepare_enable(imx_data->clk_per); > if (err) > goto free_sdhci; > @@ -1720,6 +1719,13 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > if (err) > goto disable_ipg_clk; > > + pltfm_host->clock = clk_get_rate(pltfm_host->clk); > + if (!pltfm_host->clock) { > + dev_err(mmc_dev(host->mmc), "could not get clk rate\n"); > + err = -EINVAL; > + goto disable_ahb_clk; > + } > + > imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); > if (IS_ERR(imx_data->pinctrl)) > dev_warn(mmc_dev(host->mmc), "could not get pinctrl\n");
On 7/10/2024 3:33 PM, Adrian Hunter wrote: > On 8/07/24 15:10, Ciprian Costea wrote: >> The I.MX SDHCI driver assumes that the frequency of the 'per' clock >> can be obtained even on disabled clocks, which is not always the case. >> >> According to 'clk_get_rate' documentation, it is only valid >> once the clock source has been enabled. > > The kerneldoc comment for clk_get_rate() says > > Can be called regardless of the clock enabledness > > which sounds like the opposite. Please clarify. > Hello Adrian, My observation was based on the following [1] 'clk_get_rate()' documentation. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/clk.h#n720 Best Regards, Ciprian >> >> Signed-off-by: Ciprian Costea <ciprianmarian.costea@oss.nxp.com> >> --- >> drivers/mmc/host/sdhci-esdhc-imx.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c >> index 21d984a77be8..8f0bc6dca2b0 100644 >> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >> @@ -1709,7 +1709,6 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >> } >> >> pltfm_host->clk = imx_data->clk_per; >> - pltfm_host->clock = clk_get_rate(pltfm_host->clk); >> err = clk_prepare_enable(imx_data->clk_per); >> if (err) >> goto free_sdhci; >> @@ -1720,6 +1719,13 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >> if (err) >> goto disable_ipg_clk; >> >> + pltfm_host->clock = clk_get_rate(pltfm_host->clk); >> + if (!pltfm_host->clock) { >> + dev_err(mmc_dev(host->mmc), "could not get clk rate\n"); >> + err = -EINVAL; >> + goto disable_ahb_clk; >> + } >> + >> imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); >> if (IS_ERR(imx_data->pinctrl)) >> dev_warn(mmc_dev(host->mmc), "could not get pinctrl\n"); >
On 10/07/24 15:50, Ciprian Marian Costea wrote: > On 7/10/2024 3:33 PM, Adrian Hunter wrote: >> On 8/07/24 15:10, Ciprian Costea wrote: >>> The I.MX SDHCI driver assumes that the frequency of the 'per' clock >>> can be obtained even on disabled clocks, which is not always the case. >>> >>> According to 'clk_get_rate' documentation, it is only valid >>> once the clock source has been enabled. >> >> The kerneldoc comment for clk_get_rate() says >> >> Can be called regardless of the clock enabledness >> >> which sounds like the opposite. Please clarify. >> > > Hello Adrian, > > My observation was based on the following [1] 'clk_get_rate()' documentation. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/clk.h#n720 > > Best Regards, > Ciprian Yes, that is clear. Acked-by: Adrian Hunter <adrian.hunter@intel.com> > >>> >>> Signed-off-by: Ciprian Costea <ciprianmarian.costea@oss.nxp.com> >>> --- >>> drivers/mmc/host/sdhci-esdhc-imx.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c >>> index 21d984a77be8..8f0bc6dca2b0 100644 >>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c >>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >>> @@ -1709,7 +1709,6 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >>> } >>> pltfm_host->clk = imx_data->clk_per; >>> - pltfm_host->clock = clk_get_rate(pltfm_host->clk); >>> err = clk_prepare_enable(imx_data->clk_per); >>> if (err) >>> goto free_sdhci; >>> @@ -1720,6 +1719,13 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) >>> if (err) >>> goto disable_ipg_clk; >>> + pltfm_host->clock = clk_get_rate(pltfm_host->clk); >>> + if (!pltfm_host->clock) { >>> + dev_err(mmc_dev(host->mmc), "could not get clk rate\n"); >>> + err = -EINVAL; >>> + goto disable_ahb_clk; >>> + } >>> + >>> imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); >>> if (IS_ERR(imx_data->pinctrl)) >>> dev_warn(mmc_dev(host->mmc), "could not get pinctrl\n"); >> >
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 21d984a77be8..8f0bc6dca2b0 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -1709,7 +1709,6 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) } pltfm_host->clk = imx_data->clk_per; - pltfm_host->clock = clk_get_rate(pltfm_host->clk); err = clk_prepare_enable(imx_data->clk_per); if (err) goto free_sdhci; @@ -1720,6 +1719,13 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) if (err) goto disable_ipg_clk; + pltfm_host->clock = clk_get_rate(pltfm_host->clk); + if (!pltfm_host->clock) { + dev_err(mmc_dev(host->mmc), "could not get clk rate\n"); + err = -EINVAL; + goto disable_ahb_clk; + } + imx_data->pinctrl = devm_pinctrl_get(&pdev->dev); if (IS_ERR(imx_data->pinctrl)) dev_warn(mmc_dev(host->mmc), "could not get pinctrl\n");
The I.MX SDHCI driver assumes that the frequency of the 'per' clock can be obtained even on disabled clocks, which is not always the case. According to 'clk_get_rate' documentation, it is only valid once the clock source has been enabled. Signed-off-by: Ciprian Costea <ciprianmarian.costea@oss.nxp.com> --- drivers/mmc/host/sdhci-esdhc-imx.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)