| Message ID | e8f1b59a-927b-b50e-4137-0b4b9e3cbc6e@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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 >
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
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 >
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 */