[PATCH v2 1/9] mmc: meson-gx: fix setting f_min

[PATCH v2 1/9] mmc: meson-gx: fix setting f_min

[Heiner Kallweit]

Currently f_min is set to 4 MHz whilst the comment states 400 MHz.
I think the itention is to set f_min to 400 kHz.
Change value and comment accordingly.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Kevin Hilman <khilman@baylibre.com>
---
v2:
- added acked-by
---
 drivers/mmc/host/meson-gx-mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 5eca88bc..da3cce31 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -268,7 +268,7 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	if (f_min != UINT_MAX)
 		f_min = DIV_ROUND_UP(CLK_SRC_XTAL_RATE, CLK_DIV_MAX);
 	else
-		f_min = 4000000;  /* default min: 400 MHz */
+		f_min = 400000;  /* default min: 400 kHz */
 	host->mmc->f_min = f_min;
 
 	/* create the mux */
-- 
2.11.0

Re: [PATCH v2 1/9] mmc: meson-gx: fix setting f_min

[Ulf Hansson]

On 1 February 2017 at 07:48, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Currently f_min is set to 4 MHz whilst the comment states 400 MHz.
> I think the itention is to set f_min to 400 kHz.
> Change value and comment accordingly.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Acked-by: Kevin Hilman <khilman@baylibre.com>
> ---
> v2:
> - added acked-by
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index 5eca88bc..da3cce31 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -268,7 +268,7 @@ static int meson_mmc_clk_init(struct meson_host *host)
>         if (f_min != UINT_MAX)
>                 f_min = DIV_ROUND_UP(CLK_SRC_XTAL_RATE, CLK_DIV_MAX);
>         else
> -               f_min = 4000000;  /* default min: 400 MHz */
> +               f_min = 400000;  /* default min: 400 kHz */
>         host->mmc->f_min = f_min;

This hole thing looks really weird to me. Although instead of
discussing it here, I decided to post a patch to show how I think this
should be done. Could you please have look!?
https://patchwork.kernel.org/patch/9553599/

Kind regards
Uffe

Re: [PATCH v2 1/9] mmc: meson-gx: fix setting f_min

[Heiner Kallweit]

Am 03.02.2017 um 09:29 schrieb Ulf Hansson:
> On 1 February 2017 at 07:48, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Currently f_min is set to 4 MHz whilst the comment states 400 MHz.
>> I think the itention is to set f_min to 400 kHz.
>> Change value and comment accordingly.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Acked-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>> v2:
>> - added acked-by
>> ---
>>  drivers/mmc/host/meson-gx-mmc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
>> index 5eca88bc..da3cce31 100644
>> --- a/drivers/mmc/host/meson-gx-mmc.c
>> +++ b/drivers/mmc/host/meson-gx-mmc.c
>> @@ -268,7 +268,7 @@ static int meson_mmc_clk_init(struct meson_host *host)
>>         if (f_min != UINT_MAX)
>>                 f_min = DIV_ROUND_UP(CLK_SRC_XTAL_RATE, CLK_DIV_MAX);
>>         else
>> -               f_min = 4000000;  /* default min: 400 MHz */
>> +               f_min = 400000;  /* default min: 400 kHz */
>>         host->mmc->f_min = f_min;
> 
> This hole thing looks really weird to me. Although instead of
> discussing it here, I decided to post a patch to show how I think this
> should be done. Could you please have look!?
> https://patchwork.kernel.org/patch/9553599/
> 
Thanks for the patch. Two remarks:

Member mux_parent_rate of struct meson_host is unused after this patch.
So it should be removed as part of the patch.

Requesting 100kHz resulted in f_min = 0 in my tests. Same for 200kHz
and 300 kHz. 400kHz results in f_min = 400kHz with actual rate = 380kHz.
380kHz = 24MHz / 63 is the lowest possible frequency on Meson anyway.
All requested frequencies below this value seem to be rounded down to 0.
Therefore I'd suggest to set the requested rate to 400kHz.

Rgds, Heiner

> Kind regards
> Uffe
> 

Re: [PATCH v2 1/9] mmc: meson-gx: fix setting f_min

[Martin Blumenstingl]

On Sat, Feb 4, 2017 at 11:12 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
[snip]
> Requesting 100kHz resulted in f_min = 0 in my tests. Same for 200kHz
> and 300 kHz. 400kHz results in f_min = 400kHz with actual rate = 380kHz.
> 380kHz = 24MHz / 63 is the lowest possible frequency on Meson anyway.
> All requested frequencies below this value seem to be rounded down to 0.
doesn't that sound more like an issue with the clock configuration
which should be investigated? the divider for example has
CLK_DIVIDER_ROUND_CLOSEST set, removing that will probably make it
round up (but I'm not sure if that would break other things).


Martin

Re: [PATCH v2 1/9] mmc: meson-gx: fix setting f_min

[Heiner Kallweit]

Am 05.02.2017 um 02:28 schrieb Martin Blumenstingl:
> On Sat, Feb 4, 2017 at 11:12 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> [snip]
>> Requesting 100kHz resulted in f_min = 0 in my tests. Same for 200kHz
>> and 300 kHz. 400kHz results in f_min = 400kHz with actual rate = 380kHz.
>> 380kHz = 24MHz / 63 is the lowest possible frequency on Meson anyway.
>> All requested frequencies below this value seem to be rounded down to 0.
> doesn't that sound more like an issue with the clock configuration
> which should be investigated? the divider for example has
> CLK_DIVIDER_ROUND_CLOSEST set, removing that will probably make it
> round up (but I'm not sure if that would break other things).
> 
IMHO rounding down makes sense as it prevents the system from silently
setting a frequency higher than requested.

In case we would prefer rounding to closest frequency, most likely
we would have to replace the default ops in init.ops = &clk_mux_ops;
with ops using __clk_mux_determine_rate_closest.

Heiner
> 
> Martin
>