From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Wed, 12 Aug 2015 15:53:28 +0200 Subject: [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method In-Reply-To: References: <1438913789-22308-1-git-send-email-sjg@chromium.org> <201508121548.57831.marex@denx.de> Message-ID: <201508121553.29104.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wednesday, August 12, 2015 at 03:51:07 PM, Simon Glass wrote: > Hi Marek, > > On 12 August 2015 at 07:48, Marek Vasut 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.