On Tue 22 Aug 03:45 PDT 2017, Ulf Hansson wrote:
> [...]
>
> > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > index 71e01cbc38b6..7b47906ba447 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> > @@ -131,7 +131,7 @@ struct sdhci_msm_host {
> > struct clk *pclk; /* SDHC peripheral bus clock */
> > struct clk *bus_clk; /* SDHC bus voter clock */
> > struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
> > - struct clk_bulk_data bulk_clks[2];
> > + struct clk_bulk_data bulk_clks[4];
> > unsigned long clk_rate;
> > struct mmc_host *mmc;
> > bool use_14lpp_dll_reset;
> > @@ -1125,6 +1125,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> > struct sdhci_pltfm_host *pltfm_host;
> > struct sdhci_msm_host *msm_host;
> > struct resource *core_memres;
> > + struct clk *clk;
> > int ret;
> > u16 host_version, core_minor;
> > u32 core_version, config;
> > @@ -1194,6 +1195,14 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> > msm_host->bulk_clks[0].clk = msm_host->clk;
> > msm_host->bulk_clks[1].clk = msm_host->pclk;
> >
> > + clk = devm_clk_get(&pdev->dev, "cal");
> > + if (!IS_ERR(clk))
> > + msm_host->bulk_clks[2].clk = clk;
> > +
> > + clk = devm_clk_get(&pdev->dev, "sleep");
> > + if (!IS_ERR(clk))
> > + msm_host->bulk_clks[3].clk = clk;
> > +
>
> First, both these clocks needs to be documented in DT doc.
>
Of course, sorry for missing this part.
> Second, I think you should initialize bulk_clks[2|3] to NULL, in case
> the new optional clocks can't be fetched.
>
msm_host does come from a kzalloc() in mmc_alloc_host(), but I can write
this differently to not rely on this "fact".
> > ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
> > msm_host->bulk_clks);
> > if (ret)
> > --
> > 2.12.0
> >
>
> Another observation is the number of clocks for this device. In some
> cases, now six clocks are needed!? Is that really correct? Just wanted
> to point it out as it seems a bit too much. :-)
>
* we need "iface" and "core" for normal operation
* "xo" is the base clock of the entire system and is always present,
question is why its clock rate isn't hard coded in the driver.
* "bus" should probably not be handled directly in the driver, but
rather through the upcoming "interconnect" framework
* And finally these two new clocks are needed on some HS400-enabled
platforms, for calibrating the separate (RCLK) clock delay circuit
So I believe the right answer should have been 2 required and 2 optional
clocks. But we need the interconnect framework and I'll look into why
we need to specify "xo".
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
