| Message ID | 20170818175506.5035-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | Support SDHCI on 8974pro devices | expand |
On 23 August 2017 at 19:28, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > 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". No, it's fine as is, we often relies on that fact. Sorry about the noise. > >> > 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". Thanks for the explanation so far. Looking forward to further clarifications. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html