[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 11:09:15 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 12 August 2015 at 08:40, Marek Vasut <marex at denx.de> wrote:
> > On Wednesday, August 12, 2015 at 03:55:59 PM, Simon Glass wrote:
> >> Hi Marek,
> >> 
> >> On 12 August 2015 at 07:53, Marek Vasut <marex at denx.de> wrote:
> >> > 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.
> >> 
> >> Yes fair enough, it is odd. I did start an MMC uclass so perhaps that
> >> will lead to a better solution. It's unfortunately that dw_mmc need
> >> its own callback infrastructure.
> > 
> > I hope we can iron that out shortly. The good thing is that you now have
> > a board with the DWMMC and SoCFPGA also has one, so we have at least two
> > pairs of eyes on it.
> > 
> > Also, what do you prefer to do about this patch ? Shall we go with the
> > .set_mmc_clock() callback and be done with it or do you want to stick
> > with the current approach ? I'm inclined to the former as it's less
> > confusing in my opinion.
> 
> Let's revisit it when I get back to the rockchip series.

I just checked the socfpga and I see I can use this functionality too :)