From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423503AbcFNA70 (ORCPT ); Mon, 13 Jun 2016 20:59:26 -0400 Received: from lucky1.263xmail.com ([211.157.147.133]:47049 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1422642AbcFNA7Y (ORCPT ); Mon, 13 Jun 2016 20:59:24 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: shawn.lin@rock-chips.com X-FST-TO: linux-kernel@vger.kernel.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: shawn.lin@rock-chips.com X-UNIQUE-TAG: <5e0a1d6e0fc1f9cf2d46cbdd379b6e8f> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399 To: Doug Anderson References: <1465339484-969-1-git-send-email-dianders@chromium.org> <1465339484-969-5-git-send-email-dianders@chromium.org> <47a2dcd9-9c3c-8fde-2be0-40e305c25e8d@rock-chips.com> Cc: shawn.lin@rock-chips.com, Ulf Hansson , Kishon Vijay Abraham I , Heiko Stuebner , Rob Herring , Ziyuan Xu , Brian Norris , Adrian Hunter , "open list:ARM/Rockchip SoC..." , "linux-mmc@vger.kernel.org" , "devicetree@vger.kernel.org" , Michal Simek , soren.brinkmann@xilinx.com, "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" From: Shawn Lin Message-ID: Date: Tue, 14 Jun 2016 08:59:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2016/6/14 8:43, Doug Anderson 写道: > Hi, > > On Mon, Jun 13, 2016 at 5:14 PM, Shawn Lin 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