在 2016/6/14 8:43, Doug Anderson 写道:
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?).
yes, you could see some others similar capabilities case like
timeoutclkfreq or preset value, etc. SDHCI hope Soc designer to
implement them within the controller but not mandatory.
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?
I agree. But from the code itself, we should still use
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN to see if we could get
it from internal register in case of some platforms don't
provide the clk stuff.. Sounds sane? :)
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.
Ahh, it's trivial, so keep it if you want.
-Doug
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
