On 23 August 2017 at 19:28, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> 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-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
