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 ?
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!