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

[Simon Glass]

Hi Marek,

On 12 August 2015 at 07:48, Marek Vasut <marex at denx.de> wrote:
> On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote:
>> Hi Marek,
>
> Hi!
>
> [...]
>
>> >> >> > 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.
>
> I see now, this wasn't really clear from the patch description. Shouldn't
> you introduce another callback for this purpose then, like .set_mmc_clk()
> instead ?

We could do, but I don't like introducing another interface for one
client. Also I think the right solution is to move it to use the
generic clock infrastructure, when it exists (well we have it, but
nothing uses it yet).

>
>> 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.
>
> I see, thanks for clearing this up!

Regards,
Simon