[PATCH 0/2] Support SDHCI on 8974pro devices

[PATCH 0/2] Support SDHCI on 8974pro devices

[Bjorn Andersson]

The calibration clocks for the delay circut should be enabled, as done in the
downstream kernel, in order for reset of the SDHCI not to fail on some Qualcomm
platforms (e.g. 8974pro). These patches makes it possible to reference these
clocks.

Bjorn Andersson (2):
  mmc: sdhci-msm: Utilize bulk clock API
  mmc: sdhci-msm: Enable delay circuit calibration clocks

 drivers/mmc/host/sdhci-msm.c | 53 +++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

-- 
2.12.0

[PATCH 1/2] mmc: sdhci-msm: Utilize bulk clock API

[Bjorn Andersson]

By stuffing the runtime controlled clocks into a clk_bulk_data array we
can utilize the newly introduced bulk clock operations and clean up the
error paths. This allow us to handle additional clocks in subsequent
patch, without the added complexity.

Cc: Ritesh Harjani <riteshh@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/mmc/host/sdhci-msm.c | 44 ++++++++++++++++----------------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index fc73e56eb1e2..71e01cbc38b6 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -131,6 +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];
 	unsigned long clk_rate;
 	struct mmc_host *mmc;
 	bool use_14lpp_dll_reset;
@@ -1167,16 +1168,12 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto bus_clk_disable;
 	}
 
-	ret = clk_prepare_enable(msm_host->pclk);
-	if (ret)
-		goto bus_clk_disable;
-
 	/* Setup SDC MMC clock */
 	msm_host->clk = devm_clk_get(&pdev->dev, "core");
 	if (IS_ERR(msm_host->clk)) {
 		ret = PTR_ERR(msm_host->clk);
 		dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n", ret);
-		goto pclk_disable;
+		goto bus_clk_disable;
 	}
 
 	/*
@@ -1194,9 +1191,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	if (ret)
 		dev_warn(&pdev->dev, "core clock boost failed\n");
 
-	ret = clk_prepare_enable(msm_host->clk);
+	msm_host->bulk_clks[0].clk = msm_host->clk;
+	msm_host->bulk_clks[1].clk = msm_host->pclk;
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
+				      msm_host->bulk_clks);
 	if (ret)
-		goto pclk_disable;
+		goto bus_clk_disable;
 
 	core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
@@ -1290,9 +1291,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
 clk_disable:
-	clk_disable_unprepare(msm_host->clk);
-pclk_disable:
-	clk_disable_unprepare(msm_host->pclk);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
+				   msm_host->bulk_clks);
 bus_clk_disable:
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
@@ -1315,8 +1315,8 @@ static int sdhci_msm_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
 
-	clk_disable_unprepare(msm_host->clk);
-	clk_disable_unprepare(msm_host->pclk);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
+				   msm_host->bulk_clks);
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
 	sdhci_pltfm_free(pdev);
@@ -1330,8 +1330,8 @@ static int sdhci_msm_runtime_suspend(struct device *dev)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 
-	clk_disable_unprepare(msm_host->clk);
-	clk_disable_unprepare(msm_host->pclk);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
+				   msm_host->bulk_clks);
 
 	return 0;
 }
@@ -1341,21 +1341,9 @@ static int sdhci_msm_runtime_resume(struct device *dev)
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
-	int ret;
 
-	ret = clk_prepare_enable(msm_host->clk);
-	if (ret) {
-		dev_err(dev, "clk_enable failed for core_clk: %d\n", ret);
-		return ret;
-	}
-	ret = clk_prepare_enable(msm_host->pclk);
-	if (ret) {
-		dev_err(dev, "clk_enable failed for iface_clk: %d\n", ret);
-		clk_disable_unprepare(msm_host->clk);
-		return ret;
-	}
-
-	return 0;
+	return clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
+				       msm_host->bulk_clks);
 }
 #endif
 
-- 
2.12.0

Re: [PATCH 1/2] mmc: sdhci-msm: Utilize bulk clock API

[Ulf Hansson]

On 18 August 2017 at 19:55, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> By stuffing the runtime controlled clocks into a clk_bulk_data array we
> can utilize the newly introduced bulk clock operations and clean up the
> error paths. This allow us to handle additional clocks in subsequent
> patch, without the added complexity.
>
> Cc: Ritesh Harjani <riteshh@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 44 ++++++++++++++++----------------------------
>  1 file changed, 16 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index fc73e56eb1e2..71e01cbc38b6 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -131,6 +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];

Adding bulk clocks, should also enable you to remove the core/pclk
from the struct sdhci_mcm_host, no?

[...]

Kind regards
Uffe

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

[Bjorn Andersson]

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(-)

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)
-- 
2.12.0

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

[Ulf Hansson]

[...]

> 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

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

[Bjorn Andersson]

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

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