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@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@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.
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 :)