From mboxrd@z Thu Jan 1 00:00:00 1970 From: Saravana Kannan Subject: Re: [PATCH] drivers: mmc: msm: remove clock disable in probe Date: Mon, 24 Jan 2011 11:40:20 -0800 Message-ID: <4D3DD5A4.6020607@codeaurora.org> References: <1295374473-27872-1-git-send-email-dwalker@codeaurora.org> <4D38FC55.2030601@codeaurora.org> <1295629086.19880.17.camel@m0nster> <4D39DBA4.2040107@codeaurora.org> <1295873046.32231.54.camel@stummala-linux.in.qualcomm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from wolverine02.qualcomm.com ([199.106.114.251]:50823 "EHLO wolverine02.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752438Ab1AXTkU (ORCPT ); Mon, 24 Jan 2011 14:40:20 -0500 In-Reply-To: <1295873046.32231.54.camel@stummala-linux.in.qualcomm.com> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Sahitya Tummala Cc: Daniel Walker , Vitaly Wool , davidb@quicinc.com, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org On 01/24/2011 04:44 AM, Sahitya Tummala wrote: > On Fri, 2011-01-21 at 11:16 -0800, Saravana Kannan wrote: > >> I will look further into this if I have time, but at this point I'm >> still skeptical whether this is the right fix. The reason I went fro= m >> "sure" to "skeptical" is due to the deferred clock delay code in the= driver. > > Saravana, > > Let me try to make things clear regarding clocks management in this > driver - > > 1. Before sending any MMC command msmsdcc_enable_clocks() will be cal= led > (check the function msmsdcc_request()) and at the end of that command > disabling of clocks will be deferred by calling msmsdcc_disable_clock= s() > (check the function msmsdcc_request_end()). > > 2. The driver ensures that clocks are enabled/disabled only once for = any > number of calls to msmsdcc_enable_clocks/msmsdcc_disable_clocks. Thus= , > there is no harm in calling msmsdcc_disable_clocks/msmsdcc_enable_clo= cks > twice. > > Now coming to the actual problem - > > During msmsdcc_probe(), clocks are enabled and at the end of probe > clocks are disabled in the current code. But just before disabling > clocks, there is a call to mmc_add_host(). This mmc_add_host() will > schedule the detect work to run in another thread context. The detect > work will send MMC commands for card detection. Thus, if clocks are > disabled immediately after calling mmc_add_host(), then we might end = up > turning off clocks while the detect work commands are in progress. > > Fix to this problem =E2=80=93 > > 1. The clocks will anyways be turned off at the end of MMC commands t= hat > are sent by detect work. Thus, there is no need to disable clocks a= t > the end of probe. This is exactly the fix that Daniel submitted. > > 2. Otherwise, we can also move disabling of clocks just before > mmc_add_host() so that clocks are enabled and disabled in probe itsel= f. >> From there on, clocks will be enabled for any MMC command and disabl= ed > at the end of that request. > > I would prefer Daniel's fix because the clocks are anyway going to be > turned on soon in mmc_add_host() and thus there will be no real > advantage of disabling before add_host and enabling immediately for > detect work MMC commands. Please let us know your comments. Sahitya, Thanks a lot for the detailed explanation. It certainly makes it easier= =20 to understand what's going on. Going to the fix, the 2nd way would work without unnecessarily turning=20 on/off clocks if you didn't ensure that clk_enable/clk_disable is not=20 called more than once. They already maintain ref count, so there is no=20 need for doing it again in your driver. Also, having the driver do something like "if I already disabled the=20 clock, then ignore this new disable request I got", _could_ mask a lot=20 of incorrect behavior. Having said that, the way the msm sdcc driver is designed today, I'm=20 willing to consider Daniel's patch as acceptable first step fix. You=20 might still want to look into seeing if this could be modified to take=20 advantage of the ref counting done in the clock code. Thanks, Saravana --=20 Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora For= um. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html