diff mbox

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

Message ID 20170818175506.5035-3-bjorn.andersson@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Bjorn Andersson Aug. 18, 2017, 5:55 p.m. UTC
The delay circuit used to support HS400 is calibrated based on two
additional clocks. When these clocks are not available and
FF_CLK_SW_RST_DIS is not set in CORE_HC_MODE, reset might fail. But on
some platforms this doesn't work properly and below dump can be seen in
the kernel log.

  mmc0: Reset 0x1 never completed.
  mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
  mmc0: sdhci: Sys addr:  0x00000000 | Version:  0x00001102
  mmc0: sdhci: Blk size:  0x00004000 | Blk cnt:  0x00000000
  mmc0: sdhci: Argument:  0x00000000 | Trn mode: 0x00000000
  mmc0: sdhci: Present:   0x01f80000 | Host ctl: 0x00000000
  mmc0: sdhci: Power:     0x00000000 | Blk gap:  0x00000000
  mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00000002
  mmc0: sdhci: Timeout:   0x00000000 | Int stat: 0x00000000
  mmc0: sdhci: Int enab:  0x00000000 | Sig enab: 0x00000000
  mmc0: sdhci: AC12 err:  0x00000000 | Slot int: 0x00000000
  mmc0: sdhci: Caps:      0x742dc8b2 | Caps_1:   0x00008007
  mmc0: sdhci: Cmd:       0x00000000 | Max curr: 0x00000000
  mmc0: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x00000000
  mmc0: sdhci: Resp[2]:   0x00000000 | Resp[3]:  0x00000000
  mmc0: sdhci: Host ctl2: 0x00000000
  mmc0: sdhci: ============================================

Add support for the additional calibration clocks to allow these
platforms to be configured appropriately.

Cc: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Cc: Ritesh Harjani <riteshh@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/mmc/host/sdhci-msm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Ulf Hansson Aug. 22, 2017, 10:45 a.m. UTC | #1
[...]

> 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.

Second, I think you should initialize bulk_clks[2|3] to NULL, in case
the new optional clocks can't be fetched.

>         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. :-)

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Aug. 23, 2017, 5:28 p.m. UTC | #2
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Aug. 24, 2017, 10:51 a.m. UTC | #3
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-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
+
 	ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
 				      msm_host->bulk_clks);
 	if (ret)