Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1715351 > unrolled thread

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

Started byBjorn Andersson <bjorn.andersson@linaro.org>
First post2017-08-18 20:00 +0200
Last post2017-08-24 13:00 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Support SDHCI on 8974pro devices Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-08-18 20:00 +0200
    [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-08-18 20:00 +0200
      Re: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks Ulf Hansson <ulf.hansson@linaro.org> - 2017-08-22 12:50 +0200
        Re: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration  clocks Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-08-23 19:30 +0200
          Re: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks Ulf Hansson <ulf.hansson@linaro.org> - 2017-08-24 13:00 +0200

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

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2017-08-18 20:00 +0200
Subject[PATCH 0/2] Support SDHCI on 8974pro devices
Message-ID<ufTIS-6Yr-5@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2017-08-18 20:00 +0200
Subject[PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks
Message-ID<ufTIS-6Yr-17@gated-at.bofh.it>
In reply to#1715351
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

[toc] | [prev] | [next] | [standalone]

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

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-08-22 12:50 +0200
SubjectRe: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks
Message-ID<uheUW-18s-25@gated-at.bofh.it>
In reply to#1715354
[...]

> 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

[toc] | [prev] | [next] | [standalone]

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

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2017-08-23 19:30 +0200
SubjectRe: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks
Message-ID<uhHDA-3qm-35@gated-at.bofh.it>
In reply to#1717308
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

[toc] | [prev] | [next] | [standalone]

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

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-08-24 13:00 +0200
SubjectRe: [PATCH 2/2] mmc: sdhci-msm: Enable delay circuit calibration clocks
Message-ID<uhY1H-5ln-13@gated-at.bofh.it>
In reply to#1718545
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web