[PATCH v2] mmc: sdhci-msm: Boost controller core clock

[PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ivan T. Ivanov]

Ensure SDCC is working with maximum clock otherwise card
detection could be extremely slow, up to 7 seconds.

Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
Acked-by: Stephen Boyd <sboyd@codeaurora.org>
---

Changes since v0:
- s/falied/failed in warning message.

 drivers/mmc/host/sdhci-msm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 4a09f76..4bcee03 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto pclk_disable;
 	}

+	/* Vote for maximum clock rate for maximum performance */
+	ret = clk_set_rate(msm_host->clk, INT_MAX);
+	if (ret)
+		dev_warn(&pdev->dev, "core clock boost failed\n");
+
 	ret = clk_prepare_enable(msm_host->clk);
 	if (ret)
 		goto pclk_disable;
--
1.9.1

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ulf Hansson]

On 6 July 2015 at 13:53, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
> Ensure SDCC is working with maximum clock otherwise card
> detection could be extremely slow, up to 7 seconds.
>
> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
> Acked-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks, applied!

Kind regards
Uffe

> ---
>
> Changes since v0:
> - s/falied/failed in warning message.
>
>  drivers/mmc/host/sdhci-msm.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 4a09f76..4bcee03 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>                 goto pclk_disable;
>         }
>
> +       /* Vote for maximum clock rate for maximum performance */
> +       ret = clk_set_rate(msm_host->clk, INT_MAX);
> +       if (ret)
> +               dev_warn(&pdev->dev, "core clock boost failed\n");
> +
>         ret = clk_prepare_enable(msm_host->clk);
>         if (ret)
>                 goto pclk_disable;
> --
> 1.9.1
>

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Bjorn Andersson]

On Mon, Jul 6, 2015 at 4:53 AM, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
> Ensure SDCC is working with maximum clock otherwise card
> detection could be extremely slow, up to 7 seconds.
>
> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
> Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>
> Changes since v0:
> - s/falied/failed in warning message.
>
>  drivers/mmc/host/sdhci-msm.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 4a09f76..4bcee03 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>                 goto pclk_disable;
>         }
>
> +       /* Vote for maximum clock rate for maximum performance */
> +       ret = clk_set_rate(msm_host->clk, INT_MAX);
> +       if (ret)
> +               dev_warn(&pdev->dev, "core clock boost failed\n");
> +

On my 8974AC devices this results in GCC_SDCC1_APPS_CLK changing from
100MHz to 200MHz for my eMMC. Unfortunately this results in the
following error:

[    5.103241] mmcblk0: retrying because a re-tune was needed
[    5.109270] mmcblk0: error -84 transferring data, sector 5816322,
nr 2, cmd response 0x900, card status 0xc00

Looking at the board specification it's stated that these card should
run in DDR50, so I've tried specifying "max-frequency" in the dt. I
verified in sdhci_set_clock() that we get a divisor of 4, but the
result is a repetition of:

[    1.702312] mmc1: Switching to 3.3V signalling voltage failed
[    1.837987] mmc1: power class selection to bus width 8 ddr 0 failed
[    1.846227] mmc1: error -110 whilst initialising MMC card
[    1.946303] mmc1: Reset 0x1 never completed.

I tried to disable HS200 by specifying SDHCI_QUIRK2_BROKEN_HS200. This
makes the card come up nicely again.


Is there any other way to force the link to DDR50? Is it acceptable to
expose the broken-hs200 quirk in dt so I can use that for now?

(The downstream fix used for this was apparently to just remove all
other caps...)

Regards,
Bjorn

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ulf Hansson]

On 6 November 2015 at 02:42, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Mon, Jul 6, 2015 at 4:53 AM, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>> Ensure SDCC is working with maximum clock otherwise card
>> detection could be extremely slow, up to 7 seconds.
>>
>> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
>> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
>> Acked-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>>
>> Changes since v0:
>> - s/falied/failed in warning message.
>>
>>  drivers/mmc/host/sdhci-msm.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4a09f76..4bcee03 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>                 goto pclk_disable;
>>         }
>>
>> +       /* Vote for maximum clock rate for maximum performance */
>> +       ret = clk_set_rate(msm_host->clk, INT_MAX);
>> +       if (ret)
>> +               dev_warn(&pdev->dev, "core clock boost failed\n");
>> +
>
> On my 8974AC devices this results in GCC_SDCC1_APPS_CLK changing from
> 100MHz to 200MHz for my eMMC. Unfortunately this results in the
> following error:
>
> [    5.103241] mmcblk0: retrying because a re-tune was needed
> [    5.109270] mmcblk0: error -84 transferring data, sector 5816322,
> nr 2, cmd response 0x900, card status 0xc00
>
> Looking at the board specification it's stated that these card should
> run in DDR50, so I've tried specifying "max-frequency" in the dt. I
> verified in sdhci_set_clock() that we get a divisor of 4, but the
> result is a repetition of:

I don't follow. Are you saying that changing the clock frequency to
200MHz caused the card to be initialized in HS200 mode instead of
DDR50?

>
> [    1.702312] mmc1: Switching to 3.3V signalling voltage failed
> [    1.837987] mmc1: power class selection to bus width 8 ddr 0 failed
> [    1.846227] mmc1: error -110 whilst initialising MMC card
> [    1.946303] mmc1: Reset 0x1 never completed.
>
> I tried to disable HS200 by specifying SDHCI_QUIRK2_BROKEN_HS200. This
> makes the card come up nicely again.
>
>
> Is there any other way to force the link to DDR50? Is it acceptable to
> expose the broken-hs200 quirk in dt so I can use that for now?

We already have these DT bindings.
- mmc-ddr-1_8v: eMMC high-speed DDR mode(1.8V I/O) is supported
- mmc-ddr-1_2v: eMMC high-speed DDR mode(1.2V I/O) is supported
- mmc-hs200-1_8v: eMMC HS200 mode(1.8V I/O) is supported
- mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported

Can't we use these instead?

>
> (The downstream fix used for this was apparently to just remove all
> other caps...)
>
> Regards,
> Bjorn

Thanks for reporting the regression!

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Bjorn Andersson]

On Fri 06 Nov 00:10 PST 2015, Ulf Hansson wrote:

> On 6 November 2015 at 02:42, Bjorn Andersson <bjorn@kryo.se> wrote:
> > On Mon, Jul 6, 2015 at 4:53 AM, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
> >> Ensure SDCC is working with maximum clock otherwise card
> >> detection could be extremely slow, up to 7 seconds.
> >>
> >> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
> >> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
> >> Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> >> ---
> >>
> >> Changes since v0:
> >> - s/falied/failed in warning message.
> >>
> >>  drivers/mmc/host/sdhci-msm.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> >> index 4a09f76..4bcee03 100644
> >> --- a/drivers/mmc/host/sdhci-msm.c
> >> +++ b/drivers/mmc/host/sdhci-msm.c
> >> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> >>                 goto pclk_disable;
> >>         }
> >>
> >> +       /* Vote for maximum clock rate for maximum performance */
> >> +       ret = clk_set_rate(msm_host->clk, INT_MAX);
> >> +       if (ret)
> >> +               dev_warn(&pdev->dev, "core clock boost failed\n");
> >> +
> >
> > On my 8974AC devices this results in GCC_SDCC1_APPS_CLK changing from
> > 100MHz to 200MHz for my eMMC. Unfortunately this results in the
> > following error:
> >
> > [    5.103241] mmcblk0: retrying because a re-tune was needed
> > [    5.109270] mmcblk0: error -84 transferring data, sector 5816322,
> > nr 2, cmd response 0x900, card status 0xc00
> >
> > Looking at the board specification it's stated that these card should
> > run in DDR50, so I've tried specifying "max-frequency" in the dt. I
> > verified in sdhci_set_clock() that we get a divisor of 4, but the
> > result is a repetition of:
> 
> I don't follow. Are you saying that changing the clock frequency to
> 200MHz caused the card to be initialized in HS200 mode instead of
> DDR50?
> 

No, we clock the sdhci block at 100MHz, the host->max_clk is 200MHz and
the divisor in sdhci_set_clock() becomes 1. So if I read this correctly
we're running HS200 at 100MHz.

Bumping the clock rate to 200MHz at the block doesn't affect the max_clk
and hence we're trying to run the bus at 200MHz.

I therefor tried to just set "max-frequency" to 50MHz, getting the
divider to be 4 and the below error.

So I assume it just happened to work at 100MHz, but 200MHz is way off
from the 50MHz the board is designed and tested for.


Unfortunately I don't have the equipment to measure these assumptions :/

> >
> > [    1.702312] mmc1: Switching to 3.3V signalling voltage failed
> > [    1.837987] mmc1: power class selection to bus width 8 ddr 0 failed
> > [    1.846227] mmc1: error -110 whilst initialising MMC card
> > [    1.946303] mmc1: Reset 0x1 never completed.
> >
> > I tried to disable HS200 by specifying SDHCI_QUIRK2_BROKEN_HS200. This
> > makes the card come up nicely again.
> >
> >
> > Is there any other way to force the link to DDR50? Is it acceptable to
> > expose the broken-hs200 quirk in dt so I can use that for now?
> 
> We already have these DT bindings.
> - mmc-ddr-1_8v: eMMC high-speed DDR mode(1.8V I/O) is supported
> - mmc-ddr-1_2v: eMMC high-speed DDR mode(1.2V I/O) is supported
> - mmc-hs200-1_8v: eMMC HS200 mode(1.8V I/O) is supported
> - mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported
> 
> Can't we use these instead?
> 

These are all additive and the hardware is correctly advertising that
it's capable of supporting SDR104, which implies HS200. Dropping this
assumption (or unsetting these bits) drops us down to the high-speed
rates and things work again.

Regards,
Bjorn

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ulf Hansson]

On 7 November 2015 at 00:39, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Fri 06 Nov 00:10 PST 2015, Ulf Hansson wrote:
>
>> On 6 November 2015 at 02:42, Bjorn Andersson <bjorn@kryo.se> wrote:
>> > On Mon, Jul 6, 2015 at 4:53 AM, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>> >> Ensure SDCC is working with maximum clock otherwise card
>> >> detection could be extremely slow, up to 7 seconds.
>> >>
>> >> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
>> >> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
>> >> Acked-by: Stephen Boyd <sboyd@codeaurora.org>
>> >> ---
>> >>
>> >> Changes since v0:
>> >> - s/falied/failed in warning message.
>> >>
>> >>  drivers/mmc/host/sdhci-msm.c | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> >> index 4a09f76..4bcee03 100644
>> >> --- a/drivers/mmc/host/sdhci-msm.c
>> >> +++ b/drivers/mmc/host/sdhci-msm.c
>> >> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>> >>                 goto pclk_disable;
>> >>         }
>> >>
>> >> +       /* Vote for maximum clock rate for maximum performance */
>> >> +       ret = clk_set_rate(msm_host->clk, INT_MAX);
>> >> +       if (ret)
>> >> +               dev_warn(&pdev->dev, "core clock boost failed\n");
>> >> +
>> >
>> > On my 8974AC devices this results in GCC_SDCC1_APPS_CLK changing from
>> > 100MHz to 200MHz for my eMMC. Unfortunately this results in the
>> > following error:
>> >
>> > [    5.103241] mmcblk0: retrying because a re-tune was needed
>> > [    5.109270] mmcblk0: error -84 transferring data, sector 5816322,
>> > nr 2, cmd response 0x900, card status 0xc00
>> >
>> > Looking at the board specification it's stated that these card should
>> > run in DDR50, so I've tried specifying "max-frequency" in the dt. I
>> > verified in sdhci_set_clock() that we get a divisor of 4, but the
>> > result is a repetition of:
>>
>> I don't follow. Are you saying that changing the clock frequency to
>> 200MHz caused the card to be initialized in HS200 mode instead of
>> DDR50?
>>
>
> No, we clock the sdhci block at 100MHz, the host->max_clk is 200MHz and
> the divisor in sdhci_set_clock() becomes 1. So if I read this correctly
> we're running HS200 at 100MHz.
>
> Bumping the clock rate to 200MHz at the block doesn't affect the max_clk
> and hence we're trying to run the bus at 200MHz.
>
> I therefor tried to just set "max-frequency" to 50MHz, getting the
> divider to be 4 and the below error.
>
> So I assume it just happened to work at 100MHz, but 200MHz is way off
> from the 50MHz the board is designed and tested for.
>
>
> Unfortunately I don't have the equipment to measure these assumptions :/

Ahh, I see.

It seems like a reasonable assumption that the controller can't cope
with a higher clock rate than 100 MHz as "input" clock. That would
then mean that there are different versions of the controller, as it
seems like for some version it's fine with 200MHz and for some 100MHz.

According to the DT compatible strings, *one* version is currently
supported, "qcom,sdhci-msm-v4"...

I see two viable solutions. One would be to limit the clock rate
depending on the version of the controller (new compatible strings
needs to be added). Another one would be to limit the clock rate by
using the existing DT binding for max-frequency, and thus do a
clk_set_rate(mmc->f_max) during probe.

Both examples are being used in other host drivers. The mmci host
driver does it the first way and the sdhci_bcm_kona does it in the
other way.

What do you think?

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Stephen Boyd]

On 11/09, Ulf Hansson wrote:
> On 7 November 2015 at 00:39, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > On Fri 06 Nov 00:10 PST 2015, Ulf Hansson wrote:
> >
> >> On 6 November 2015 at 02:42, Bjorn Andersson <bjorn@kryo.se> wrote:
> >> > On Mon, Jul 6, 2015 at 4:53 AM, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
> >> >> Ensure SDCC is working with maximum clock otherwise card
> >> >> detection could be extremely slow, up to 7 seconds.
> >> >>
> >> >> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
> >> >> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
> >> >> Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> >> >> ---
> >> >>
> >> >> Changes since v0:
> >> >> - s/falied/failed in warning message.
> >> >>
> >> >>  drivers/mmc/host/sdhci-msm.c | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >>
> >> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> >> >> index 4a09f76..4bcee03 100644
> >> >> --- a/drivers/mmc/host/sdhci-msm.c
> >> >> +++ b/drivers/mmc/host/sdhci-msm.c
> >> >> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> >> >>                 goto pclk_disable;
> >> >>         }
> >> >>
> >> >> +       /* Vote for maximum clock rate for maximum performance */
> >> >> +       ret = clk_set_rate(msm_host->clk, INT_MAX);
> >> >> +       if (ret)
> >> >> +               dev_warn(&pdev->dev, "core clock boost failed\n");
> >> >> +
> >> >
> >> > On my 8974AC devices this results in GCC_SDCC1_APPS_CLK changing from
> >> > 100MHz to 200MHz for my eMMC. Unfortunately this results in the
> >> > following error:
> >> >
> >> > [    5.103241] mmcblk0: retrying because a re-tune was needed
> >> > [    5.109270] mmcblk0: error -84 transferring data, sector 5816322,
> >> > nr 2, cmd response 0x900, card status 0xc00
> >> >
> >> > Looking at the board specification it's stated that these card should
> >> > run in DDR50, so I've tried specifying "max-frequency" in the dt. I
> >> > verified in sdhci_set_clock() that we get a divisor of 4, but the
> >> > result is a repetition of:
> >>
> >> I don't follow. Are you saying that changing the clock frequency to
> >> 200MHz caused the card to be initialized in HS200 mode instead of
> >> DDR50?
> >>
> >
> > No, we clock the sdhci block at 100MHz, the host->max_clk is 200MHz and
> > the divisor in sdhci_set_clock() becomes 1. So if I read this correctly
> > we're running HS200 at 100MHz.
> >
> > Bumping the clock rate to 200MHz at the block doesn't affect the max_clk
> > and hence we're trying to run the bus at 200MHz.
> >
> > I therefor tried to just set "max-frequency" to 50MHz, getting the
> > divider to be 4 and the below error.
> >
> > So I assume it just happened to work at 100MHz, but 200MHz is way off
> > from the 50MHz the board is designed and tested for.
> >
> >
> > Unfortunately I don't have the equipment to measure these assumptions :/
> 
> Ahh, I see.
> 
> It seems like a reasonable assumption that the controller can't cope
> with a higher clock rate than 100 MHz as "input" clock. That would
> then mean that there are different versions of the controller, as it
> seems like for some version it's fine with 200MHz and for some 100MHz.
> 
> According to the DT compatible strings, *one* version is currently
> supported, "qcom,sdhci-msm-v4"...

The same version of hardware is there 4 times. The difference is
the maximum clock frequency supported by them is different. In
downstream kernels we've handled this by trimming the frequency
tables for the different controllers in the clock driver.
Setting the clock to INT_MAX will make it run at 400MHz, which
doesn't look to be supported by anything besides sdc1 on 8974ac.

> 
> I see two viable solutions. One would be to limit the clock rate
> depending on the version of the controller (new compatible strings
> needs to be added). Another one would be to limit the clock rate by
> using the existing DT binding for max-frequency, and thus do a
> clk_set_rate(mmc->f_max) during probe.
> 

I'd rather see that done via OPP tables in DT, but I suppose
max-frequency is fine too. We'll need to use OPPs soon enough
because there's a voltage associated with that frequency.

In case you're wondering, the max frequency for sdc1 on 8974ac is
400MHz. If it's just a plain 8974pro then the max frequency is
200MHz. Otherwise, sdc2 maxes out at 200Mhz and sdc3 and sdc4 max
out at 100MHz.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ulf Hansson]

[...]

>> Ahh, I see.
>>
>> It seems like a reasonable assumption that the controller can't cope
>> with a higher clock rate than 100 MHz as "input" clock. That would
>> then mean that there are different versions of the controller, as it
>> seems like for some version it's fine with 200MHz and for some 100MHz.
>>
>> According to the DT compatible strings, *one* version is currently
>> supported, "qcom,sdhci-msm-v4"...
>
> The same version of hardware is there 4 times. The difference is
> the maximum clock frequency supported by them is different. In
> downstream kernels we've handled this by trimming the frequency
> tables for the different controllers in the clock driver.
> Setting the clock to INT_MAX will make it run at 400MHz, which
> doesn't look to be supported by anything besides sdc1 on 8974ac.
>
>>
>> I see two viable solutions. One would be to limit the clock rate
>> depending on the version of the controller (new compatible strings
>> needs to be added). Another one would be to limit the clock rate by
>> using the existing DT binding for max-frequency, and thus do a
>> clk_set_rate(mmc->f_max) during probe.
>>
>
> I'd rather see that done via OPP tables in DT, but I suppose
> max-frequency is fine too. We'll need to use OPPs soon enough
> because there's a voltage associated with that frequency.

Okay, thanks for sharing the details.

>
> In case you're wondering, the max frequency for sdc1 on 8974ac is
> 400MHz. If it's just a plain 8974pro then the max frequency is
> 200MHz. Otherwise, sdc2 maxes out at 200Mhz and sdc3 and sdc4 max
> out at 100MHz.

When you say that sdc1 supports 400MHz, what does that mean? That it
actually can cope with that clock rate when communicating with the MMC
card?

This makes me wonder how you deal with power management (DVFS).

For example when you have the possibility to gate this clock (at
request inactivity) when the rate is set to 400 MHz and OPP is
increased, how will then that clock gating affect the OPP?

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Stephen Boyd]

On 11/16, Ulf Hansson wrote:
> [...]
> 
> 
> >
> > In case you're wondering, the max frequency for sdc1 on 8974ac is
> > 400MHz. If it's just a plain 8974pro then the max frequency is
> > 200MHz. Otherwise, sdc2 maxes out at 200Mhz and sdc3 and sdc4 max
> > out at 100MHz.
> 
> When you say that sdc1 supports 400MHz, what does that mean? That it
> actually can cope with that clock rate when communicating with the MMC
> card?

I suspect there must be some internal divider in the sdc IP
itself so that it doesn't put out 400MHz on the bus, but I really
don't know. What I mean is that the clock going into the IP from
the clock controller is running at 400MHz, after it goes into the
IP it could be divided, etc. before exiting the SoC on some pin.

> 
> This makes me wonder how you deal with power management (DVFS).
> 
> For example when you have the possibility to gate this clock (at
> request inactivity) when the rate is set to 400 MHz and OPP is
> increased, how will then that clock gating affect the OPP?

Sorry I'm not really following the question here. The gate will
disable the clock in the clock controller, cutting the signal off
upstream of the sdc IP. When we do DVFS we'll stop considering
this clock as part of the overall power level for the voltage
associated with the frequency. When all other clocks that are
using the same voltage and are on and running at frequencies that
don't need that high of voltage we can reduce the voltage and
drop down to something lower.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ulf Hansson]

On 18 November 2015 at 02:38, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/16, Ulf Hansson wrote:
>> [...]
>>
>>
>> >
>> > In case you're wondering, the max frequency for sdc1 on 8974ac is
>> > 400MHz. If it's just a plain 8974pro then the max frequency is
>> > 200MHz. Otherwise, sdc2 maxes out at 200Mhz and sdc3 and sdc4 max
>> > out at 100MHz.
>>
>> When you say that sdc1 supports 400MHz, what does that mean? That it
>> actually can cope with that clock rate when communicating with the MMC
>> card?
>
> I suspect there must be some internal divider in the sdc IP
> itself so that it doesn't put out 400MHz on the bus, but I really
> don't know. What I mean is that the clock going into the IP from
> the clock controller is running at 400MHz, after it goes into the
> IP it could be divided, etc. before exiting the SoC on some pin.

Okay, got it!

>
>>
>> This makes me wonder how you deal with power management (DVFS).
>>
>> For example when you have the possibility to gate this clock (at
>> request inactivity) when the rate is set to 400 MHz and OPP is
>> increased, how will then that clock gating affect the OPP?
>
> Sorry I'm not really following the question here. The gate will
> disable the clock in the clock controller, cutting the signal off
> upstream of the sdc IP. When we do DVFS we'll stop considering
> this clock as part of the overall power level for the voltage
> associated with the frequency. When all other clocks that are
> using the same voltage and are on and running at frequencies that
> don't need that high of voltage we can reduce the voltage and
> drop down to something lower.

Sorry for being a bit vague, but you kind of answered my question, thanks!

I haven't been following DVFS evolution lately, but one of my earlier
concern was the how to use clk rate change notifiers to deal with OPP
transitions.

As in this case the clock rate would be fixed, and thus we can't use
the clk rate change notifiers to trigger an OPP change. But I guess
there is a solution to that then?

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ulf Hansson]

[...]

>> It seems like a reasonable assumption that the controller can't cope
>> with a higher clock rate than 100 MHz as "input" clock. That would
>> then mean that there are different versions of the controller, as it
>> seems like for some version it's fine with 200MHz and for some 100MHz.
>>
>> According to the DT compatible strings, *one* version is currently
>> supported, "qcom,sdhci-msm-v4"...
>
> The same version of hardware is there 4 times. The difference is
> the maximum clock frequency supported by them is different. In
> downstream kernels we've handled this by trimming the frequency
> tables for the different controllers in the clock driver.
> Setting the clock to INT_MAX will make it run at 400MHz, which
> doesn't look to be supported by anything besides sdc1 on 8974ac.
>
>>
>> I see two viable solutions. One would be to limit the clock rate
>> depending on the version of the controller (new compatible strings
>> needs to be added). Another one would be to limit the clock rate by
>> using the existing DT binding for max-frequency, and thus do a
>> clk_set_rate(mmc->f_max) during probe.
>>
>
> I'd rather see that done via OPP tables in DT, but I suppose
> max-frequency is fine too. We'll need to use OPPs soon enough
> because there's a voltage associated with that frequency.
>
> In case you're wondering, the max frequency for sdc1 on 8974ac is
> 400MHz. If it's just a plain 8974pro then the max frequency is
> 200MHz. Otherwise, sdc2 maxes out at 200Mhz and sdc3 and sdc4 max
> out at 100MHz.
>

I think we have reached a consensus on the viable options.

As we haven't heard from Ivan, do someone want to send a patch for
this or shall we just revert $subject patch (which I can deal with)?

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ivan T. Ivanov]

> On Dec 16, 2015, at 12:18 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> [...]
> 
>>> It seems like a reasonable assumption that the controller can't cope
>>> with a higher clock rate than 100 MHz as "input" clock. That would
>>> then mean that there are different versions of the controller, as it
>>> seems like for some version it's fine with 200MHz and for some 100MHz.
>>> 
>>> According to the DT compatible strings, *one* version is currently
>>> supported, "qcom,sdhci-msm-v4"...
>> 
>> The same version of hardware is there 4 times. The difference is
>> the maximum clock frequency supported by them is different. In
>> downstream kernels we've handled this by trimming the frequency
>> tables for the different controllers in the clock driver.
>> Setting the clock to INT_MAX will make it run at 400MHz, which
>> doesn't look to be supported by anything besides sdc1 on 8974ac.
>> 
>>> 
>>> I see two viable solutions. One would be to limit the clock rate
>>> depending on the version of the controller (new compatible strings
>>> needs to be added). Another one would be to limit the clock rate by
>>> using the existing DT binding for max-frequency, and thus do a
>>> clk_set_rate(mmc->f_max) during probe.
>>> 
>> 
>> I'd rather see that done via OPP tables in DT, but I suppose
>> max-frequency is fine too. We'll need to use OPPs soon enough
>> because there's a voltage associated with that frequency.
>> 
>> In case you're wondering, the max frequency for sdc1 on 8974ac is
>> 400MHz. If it's just a plain 8974pro then the max frequency is
>> 200MHz. Otherwise, sdc2 maxes out at 200Mhz and sdc3 and sdc4 max
>> out at 100MHz.
>> 
> 
> I think we have reached a consensus on the viable options.
> 
> As we haven't heard from Ivan, do someone want to send a patch for
> this or shall we just revert $subject patch (which I can deal with)?

I am following this discussion, but I am not sure what I could do. 

If my understanding is correct, even if controllers report same version,
they don’t support 400MHz core clock. 

Initial patch fixes real issue. I am voting for “max-frequency” DT property.

I don’t have 8974ac, so I can not test the change.

Regards,
Ivan

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Ulf Hansson]

On 16 December 2015 at 12:44, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>
>> On Dec 16, 2015, at 12:18 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> [...]
>>
>>>> It seems like a reasonable assumption that the controller can't cope
>>>> with a higher clock rate than 100 MHz as "input" clock. That would
>>>> then mean that there are different versions of the controller, as it
>>>> seems like for some version it's fine with 200MHz and for some 100MHz.
>>>>
>>>> According to the DT compatible strings, *one* version is currently
>>>> supported, "qcom,sdhci-msm-v4"...
>>>
>>> The same version of hardware is there 4 times. The difference is
>>> the maximum clock frequency supported by them is different. In
>>> downstream kernels we've handled this by trimming the frequency
>>> tables for the different controllers in the clock driver.
>>> Setting the clock to INT_MAX will make it run at 400MHz, which
>>> doesn't look to be supported by anything besides sdc1 on 8974ac.
>>>
>>>>
>>>> I see two viable solutions. One would be to limit the clock rate
>>>> depending on the version of the controller (new compatible strings
>>>> needs to be added). Another one would be to limit the clock rate by
>>>> using the existing DT binding for max-frequency, and thus do a
>>>> clk_set_rate(mmc->f_max) during probe.
>>>>
>>>
>>> I'd rather see that done via OPP tables in DT, but I suppose
>>> max-frequency is fine too. We'll need to use OPPs soon enough
>>> because there's a voltage associated with that frequency.
>>>
>>> In case you're wondering, the max frequency for sdc1 on 8974ac is
>>> 400MHz. If it's just a plain 8974pro then the max frequency is
>>> 200MHz. Otherwise, sdc2 maxes out at 200Mhz and sdc3 and sdc4 max
>>> out at 100MHz.
>>>
>>
>> I think we have reached a consensus on the viable options.
>>
>> As we haven't heard from Ivan, do someone want to send a patch for
>> this or shall we just revert $subject patch (which I can deal with)?
>
> I am following this discussion, but I am not sure what I could do.
>
> If my understanding is correct, even if controllers report same version,
> they don’t support 400MHz core clock.

Correct.

>
> Initial patch fixes real issue. I am voting for “max-frequency” DT property.

I don't have any strong opinion, so I am fine with that approach.

That means at ->probe() we need to consider whether "host->f_max" has
been obtained via DT (mmc_of_parse() parses the "max-frequency" DT
property).
If that's the case, we can choose between two solutions.

1) Consider f_max as the correct value to set via clk_set_rate() and
if f_max isn't available, fall back to a lower "default" frequency to
make sure we are safe.
2) Consider f_max as an upper limit. That means if f_max isn't
available we use INT_MAX instead.

Both alternatives requires DTB to be updated, but option 1) may be
safer as it should allow all cards to be functional, although running
in a lower frequency potentially affecting throughput.

But again, I don't care much, alternative 2 is okay by me as well.

>
> I don’t have 8974ac, so I can not test the change.
>

Let's hope for some help here then!

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

[Georgi Djakov]

On 11/06/2015 03:42 AM, Bjorn Andersson wrote:
> On Mon, Jul 6, 2015 at 4:53 AM, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>> Ensure SDCC is working with maximum clock otherwise card
>> detection could be extremely slow, up to 7 seconds.
>>
>> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
>> Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
>> Acked-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>>
>> Changes since v0:
>> - s/falied/failed in warning message.
>>
>>  drivers/mmc/host/sdhci-msm.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4a09f76..4bcee03 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>                 goto pclk_disable;
>>         }
>>
>> +       /* Vote for maximum clock rate for maximum performance */
>> +       ret = clk_set_rate(msm_host->clk, INT_MAX);
>> +       if (ret)
>> +               dev_warn(&pdev->dev, "core clock boost failed\n");
>> +
> 
> On my 8974AC devices this results in GCC_SDCC1_APPS_CLK changing from
> 100MHz to 200MHz for my eMMC. Unfortunately this results in the
> following error:
> 
> [    5.103241] mmcblk0: retrying because a re-tune was needed
> [    5.109270] mmcblk0: error -84 transferring data, sector 5816322,
> nr 2, cmd response 0x900, card status 0xc00
> 
> Looking at the board specification it's stated that these card should
> run in DDR50, so I've tried specifying "max-frequency" in the dt. I
> verified in sdhci_set_clock() that we get a divisor of 4, but the
> result is a repetition of:
> 
> [    1.702312] mmc1: Switching to 3.3V signalling voltage failed
> [    1.837987] mmc1: power class selection to bus width 8 ddr 0 failed
> [    1.846227] mmc1: error -110 whilst initialising MMC card
> [    1.946303] mmc1: Reset 0x1 never completed.
> 
> I tried to disable HS200 by specifying SDHCI_QUIRK2_BROKEN_HS200. This
> makes the card come up nicely again.
> 
> 
> Is there any other way to force the link to DDR50? Is it acceptable to
> expose the broken-hs200 quirk in dt so I can use that for now?
> 
> (The downstream fix used for this was apparently to just remove all
> other caps...)
> 

Hi Bjorn,
There are two dev_dbg lines in the driver. What do they print for
major and minor versions?

Thanks,
Georgi