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

[Marek Vasut]

On Wednesday, August 12, 2015 at 03:51:07 PM, Simon Glass wrote:
> 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).

OK, but making a .get_mmc_clk() function actually configure something
is a behavior I wouldn't expect from a getter function. It's a bit odd
and illogical in my opinion.