Re: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks

[Ulf Hansson]

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