Hi, On Mon, Jun 13, 2016 at 5:14 PM, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote: >>> It's broken when reading capabilities reg on RK3399 platform >>> which means you should get it via clk framework. But you should consider >>> the non-broken case. >> >> >> I'm afraid I don't understand. Can you elaborate? Are you saying if >> things weren't broken then we wouldn't have to ask the common clock >> framework for this? Where would we get this value? > > > > I mean bascially we should get baseclk from capabilities register[15:8] > (offset 0x40@sdhci), namely EMMCCORE_CAP on TRM. Only when you get 0x0 > from there, can you consider to get it from clock framework. Ah, got it! I guess I would be super surprised if an SoC implemented register[15:8] but still then required you to manually copy that value to corecfg_baseclkfreq. Presumably nobody would be crazy enough to try to measure the clock rate in the SDHCI driver, so this would probably only be non-zero where the SDHCI clock is totally fixed. ...in that case probably the SoC designer would also put a default value in corecfg_baseclkfreq that matched (and maybe even make corecfg_baseclkfreq read-only?). Even in the case that an SoC designer didn't put a value into corecfg_baseclkfreq that matched register[15:8], it seems very likely that the rate returned from the clk_get_rate() would match. I guess what I'm saying is that, to me, it seems like my patch isn't broken in any real systems. If we ever find a system that needs this behavior in the future, we can add it. Until then, it seems like my patch would be fine. Do you agree? Note: right now we only provide a register map for rk3399, so certainly we can't be breaking any other SoCs with our current method. > I don't see a reason to check the return code here. Specifically: >> >> * If this is a SoC where you don't need to write corecfg_baseclkfreq >> then we need do nothing about this error. >> >> * If the regmap write failed (which would be terribly unexpected for a >> memory mapped register) then we've already printed an error (in >> sdhci_arasan_syscon_write). Best course of action seems to keep going >> and try anyway. >> >> >> I don't think a retry is likely to help anything. > > > Well, I saw you add a return value for sdhci_arasan_syscon_write, so > should we remove it? It was presuming that there might be future callers who might want to write other corecfg registers and might need to know whether the write worked or not. Since having the return value doesn't hurt anything I'd rather leave it in. If you really want me to remove it, though, I will. Just let me know. -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
