Hi Marek,
On 12 August 2015 at 07:53, Marek Vasut marex@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@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.
Regards, Simon