[U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method

[Simon Glass]

Hi Marek,

On 6 August 2015 at 21:07, Marek Vasut <marex at denx.de> wrote:
> On Friday, August 07, 2015 at 05:00:24 AM, Simon Glass wrote:
>> Hi Marek,
>
> Hi Simon,
>
>> On 6 August 2015 at 20:58, Marek Vasut <marex at denx.de> wrote:
>> > On Friday, August 07, 2015 at 04:54:42 AM, Simon Glass wrote:
>> >> Hi Marek,
>> >
>> > Hi Simon,
>> >
>> >> On 6 August 2015 at 20:51, Marek Vasut <marex at denx.de> wrote:
>> >> > On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
>> >> >> Some SoCs want to adjust the input clock to the DWMMC block as a way
>> >> >> of controlling the MMC bus clock. Update the get_mmc_clk() method to
>> >> >> support this.
>> >> >>
>> >> >> Signed-off-by: Simon Glass <sjg at chromium.org>
>> >> >
>> >> > Hi Simon,
>> >> >
>> >> >> ---
>> >> >>
>> >> >> Changes in v4:
>> >> >> - Update commit message to indicate this patch is for the dw_mmc
>> >> >> driver
>> >> >>
>> >> >>  drivers/mmc/dw_mmc.c        |  2 +-
>> >> >>  drivers/mmc/exynos_dw_mmc.c |  2 +-
>> >> >>  include/dwmmc.h             | 16 +++++++++++++++-
>> >> >>  3 files changed, 17 insertions(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>> >> >> index 8f28d7e..a034c3f 100644
>> >> >> --- a/drivers/mmc/dw_mmc.c
>> >> >> +++ b/drivers/mmc/dw_mmc.c
>> >> >> @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host
>> >> >> *host, u32 freq) * host->bus_hz should be set by user.
>> >> >>
>> >> >>        */
>> >> >>
>> >> >>       if (host->get_mmc_clk)
>> >> >>
>> >> >> -             sclk = host->get_mmc_clk(host);
>> >> >> +             sclk = host->get_mmc_clk(host, freq);
>> >> >
>> >> > Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call
>> >> > some clock framework function to determine the input frequency of the
>> >> > DWMMC block from within the get_mmc_clk() implementation instead ?
>> >> > What do you think please ?
>> >>
>> >> Well, yes. If such a clock frame work existed I would call it :-) We
>> >> do have a uclass now so we are getting there.
>> >
>> > Excellent, so do you really need this kind of patch ? :) Why don't you
>> > make just some kind of function -- get_dwmmc_clock() -- and call it
>> > instead ?
>>
>> This is sort-of what is happening. It is calling a function in the
>> host controller - i.e. the SoC's MMC controller. It is one step closer
>> to knowing the input clock to the dwmmc input clock. Note that it is
>> not the clock of the MMC bus itself, but the input clock to the dwmmc
>> logic block.
>
> I don't think I quite understand what you mean here. We're talking about
> obtaining the frequency of the clock which go into the DWMMC IP block,
> right ?
>
> So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and
> call it from within the implementation of the .get_mmc_clk(), which is
> specific for that particular chip of yours*, you don't need this patch.
> Or am I really missing something fundamental ?
>
> *the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c

The purpose of the existing code (before my change) is to find out the
input frequency of the IP block. By knowing this, the dw_mmc driver
can work out what divisor it needs to achieve a particular MMC bus
clock.

The implementation of get_mmc_clk() (which will be in the SoC-specific
MMC driver) is indeed the place where the clock is figured out. My
only change is to add a parameter which is the desired bus clock. This
parameter can be ignored, but for implementations which can select the
source clock such that it matches this bus clock, then they can do
this and dw_mmc can just use bypass mode.

Unless we pass the bus frequency to get_mmc_clk() it has no way of
knowing what bus clock is required and thus cannot implement this
feature. The feature implementation is entirely within the
implementation of get_mmc_clk() - it just needs one more piece of
information to do its job.

Regards,
Simon