Hi Marek, On 12 August 2015 at 07:48, Marek Vasut <ma...@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). > >> 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! Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
