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