From mboxrd@z Thu Jan 1 00:00:00 1970 From: Srinivas Kandagatla Subject: Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe Date: Tue, 10 Jun 2014 18:47:29 +0100 Message-ID: <539744B1.9090503@linaro.org> References: <1402410678-12931-1-git-send-email-srinivas.kandagatla@linaro.org> <1402410717-12977-1-git-send-email-srinivas.kandagatla@linaro.org> <6D685866-4AE5-458B-BB8C-EFEBAE221A9E@codeaurora.org> <539742B6.7030303@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-wi0-f172.google.com ([209.85.212.172]:52361 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752715AbaFJRrd (ORCPT ); Tue, 10 Jun 2014 13:47:33 -0400 Received: by mail-wi0-f172.google.com with SMTP id hi2so4368729wib.5 for ; Tue, 10 Jun 2014 10:47:32 -0700 (PDT) In-Reply-To: <539742B6.7030303@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Stephen Boyd , Kumar Gala Cc: linux-arm-msm , Andy Gross , Mike Turquette On 10/06/14 18:39, Stephen Boyd wrote: > On 06/10/14 08:20, Kumar Gala wrote: >> On Jun 10, 2014, at 9:31 AM, Srinivas Kandagatla wrote: >> >>> The use case here is when we have a bootconsole which is printing t= he >>> characters on serial console and gsbi driver comes up after some ti= me. >>> As gsbi driver disables the clock in probe the bootconsole locks up= =2E >>> >>> This patch fixes the problem by disabling the clock in platform rem= ove >>> rather than in probe. >>> >>> Signed-off-by: Srinivas Kandagatla >>> --- >>> drivers/soc/qcom/qcom_gsbi.c | 46 +++++++++++++++++++++++++++++++--= ----------- >>> 1 file changed, 33 insertions(+), 13 deletions(-) >> It seems like we shouldn=92t need this change. Adding Stephen to se= e if there is a reason we don=92t have the clk=92s enable_count adjuste= d for how the bootloader setup clks. > > This is a long standing problem with the clock framework. In our vend= or > tree we've added something called "handoff" which basically detects t= he > state of all clocks upon registration and keeps clocks enabled until > late_init() if the clocks were enabled at the time of registration. > > For this case though "handoff" doesn't seem necessary. It's easier to > just disable the clock when the driver is removed. With finer grained > power management this driver can participate in runtime_pm and disabl= e > the ahb clock when the device is runtime suspended; which would only > happen when the child devices (uart/spi/i2c) are also runtime suspend= ed. I had same thought about gsbi participating in PM. > >> - k >> >>> diff --git a/drivers/soc/qcom/qcom_gsbi.c b/drivers/soc/qcom/qcom_g= sbi.c >>> index ab7b441..64fb298 100644 >>> --- a/drivers/soc/qcom/qcom_gsbi.c >>> +++ b/drivers/soc/qcom/qcom_gsbi.c >>> @@ -22,44 +22,63 @@ >>> #define GSBI_CTRL_REG 0x0000 >>> #define GSBI_PROTOCOL_SHIFT 4 >>> >>> +struct gsbi_info { >>> + struct clk *hclk; >>> + u32 mode; >>> + u32 crci; >>> +}; > > What does mode and crci have to do with this patch? Can't we just put > the clock into the platform data? It has nothing to do with this but, for completeness and we might need=20 this if we are doing PM in future. for example pm resume might want to=20 reconfigure the gsbi. Am Ok with either approaches. --srini >