Received: by 2002:ac0:a594:0:0:0:0:0 with SMTP id m20-v6csp259952imm; Tue, 22 May 2018 18:20:05 -0700 (PDT) X-Google-Smtp-Source: AB8JxZqFrnX2qlgArViZ5OBgHSvyJWqrfSOKfcXq8VCIoQRk8fscvRaUbTn4PIKYTggT/YbzujQ7 X-Received: by 2002:a65:6085:: with SMTP id t5-v6mr639838pgu.362.1527038404943; Tue, 22 May 2018 18:20:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1527038404; cv=none; d=google.com; s=arc-20160816; b=BLhuQlKOR3Cf3jzF4Y9Z6RqXko3Fya6JXAD7ec0CxXFUUZEn20JqQMUiEq9dv7fA6I PUWE6FWNWweEyezebFcLqOT1TTpZ8wgw3tjjCqddKVrtWysDV5qgt+2+fK44aOw2TWxp WY45rGjHGHGPE3C2XmKwL0SPa/nxYMtHOD1tDFtkUrD/DriYTcqVs3sUGjAMNEiXcm5G s3HOWultBVrLJqVWFElEWM4BxMZ5xR12g2vsK/rJ3tmJ7lIlTUowclXAfvV0zbiDEI1/ /+pX77VJMdhJBEi5qbl1XiZiHLcn+9AVYpHkz2134WhQ60KG8A68yTnVw3o9CnCvFdTi 4klA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dmarc-filter :dkim-signature:dkim-signature:arc-authentication-results; bh=d/K9sRKAOHph5HXzwMMrkjuHyH6dY5h2nBYSuWmIZxY=; b=KV24TOMKJV4+EMcZIEmewWXQWsUTpE+Fc/aGsfCc0S55xnuerRks05Dd0FM183vS1D CW26iMrmc2jtTZ51QNshVkBs5nCp+W7qZCQ75tRWTvwKwGMhDQRorKlz5FZP9oMlPq5J qBBdsEOle9Y6bu92wzP6k+n83cUsMHeuY2sqOi5/mPHLnq05YUf7ofgwstn+zxB+X35k 4qipIPqsbGRZ1kIvOfA+x30gxLBZ4qEANuS1edz5kCuX5x0BuUa/Mc5FKFXhMncVs4nH aKGkWZ7Dy4B013krnZhLTR9xSn6mrVkcaEGZGSEX6XbWuf+ieFVyqkmX2oZQzKEGcYNf VZmw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=WGU0R7bp; dkim=pass header.i=@codeaurora.org header.s=default header.b=HDXVY6zB; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n59-v6si17423592plb.198.2018.05.22.18.19.50; Tue, 22 May 2018 18:20:04 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=WGU0R7bp; dkim=pass header.i=@codeaurora.org header.s=default header.b=HDXVY6zB; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753789AbeEWBTZ (ORCPT + 99 others); Tue, 22 May 2018 21:19:25 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:40170 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753769AbeEWBTX (ORCPT ); Tue, 22 May 2018 21:19:23 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id CB627601E8; Wed, 23 May 2018 01:19:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1527038362; bh=Joa09oZT6mgzo5a2xzvoxqYTQV3lkynwNpVNkN+KbPo=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=WGU0R7bpcTFsZUv8bErPfCXV+cbXusgqVxDwG1IXPqan62doYs977nYLpGYMfxnau 4SmywOmqkMH1jOumrSG8oB5n94SQ3HLZiNPHSjD8rZ38nR/M+muXohR0lkQX3UUe50 FWApHcDkvuu/AR0+7cIEkzZXisLe0TnfgMJVx318= X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on pdx-caf-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=2.0 tests=ALL_TRUSTED,BAYES_00, DKIM_SIGNED,T_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.0 Received: from [10.46.160.165] (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: collinsd@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 01DB5604D4; Wed, 23 May 2018 01:19:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1527038361; bh=Joa09oZT6mgzo5a2xzvoxqYTQV3lkynwNpVNkN+KbPo=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=HDXVY6zBcXI2br0kLuZ9jhNIbdmesEF4OB92TmSdXkomH63VU/1I12q6wp0V3fLOl gdCgJM3t+YPmPyNfCqnn2vxvL1KweQAcynkkKrSJ2JjyuhBVKzI298uAT+dl3d4xHN 1NM/1gVJIDmXnb8h+2zw7h6BABAtmvDCdaSNjuQk= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 01DB5604D4 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=collinsd@codeaurora.org Subject: Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings To: Doug Anderson Cc: Mark Brown , Liam Girdwood , Rob Herring , Mark Rutland , linux-arm-msm@vger.kernel.org, Linux ARM , devicetree@vger.kernel.org, LKML , Rajendra Nayak , Stephen Boyd References: <41d5df73ddac772551d2966e0752bb0c357b1ded.1526088081.git.collinsd@codeaurora.org> <869aad59-1cc5-28ef-1fb5-4ef846696c40@codeaurora.org> From: David Collins Message-ID: Date: Tue, 22 May 2018 18:19:20 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/22/2018 05:08 PM, Doug Anderson wrote: > On Tue, May 22, 2018 at 3:46 PM, David Collins wrote: >> On 05/22/2018 09:43 AM, Doug Anderson wrote: >>> On Mon, May 21, 2018 at 5:00 PM, David Collins wrote: ... >> However, if the voltage caching feature is acceptable for upstream usage, >> then I could add it. With that in place, I see less of a need for the >> qcom,regulator-initial-microvolt property and would be ok with removing it >> for now. > > I think it's the right thing to do an Mark didn't seem to yell, so I'd > say go for it. I'll remove qcom,regulator-initial-microvolt support and add voltage caching. >>> OK, so how's this for a proposal then: >>> >>> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't >>> specified that the regulator is enabled then we don't send the >>> voltage, we just cache it. >>> >>> 2. When we see the first enable then we first send the cached voltage >>> and then do the enable. >>> >>> 3. We don't need an "initial voltage" because any rails that are >>> expected to be variable voltage the client should be choosing a >>> voltage. >>> >>> >>> ...taking the SD card case as an example: if the regulator framework >>> says "set to the minimum: 1.8V" we'll cache this but not apply it yet >>> because the rail is off as far as Linux is concerned. Then when the >>> SD card framework starts up it will set the voltage to 3.3V which will >>> overwrite the cache. Then the SD card framework will enable the >>> regulator and RPMh will set the voltage to 3.3V and enable. >> >> I am ok with implementing this feature. >> >> However, should the voltage be cached instead of sent immediately any time >> that Linux has not explicitly requested the regulator to be enabled, or >> only before the first time that an enable request is sent? >> >> 1. regulator_set_voltage(reg, 2960000, 2960000) >> --> cache voltage=2960 mV >> 2. regulator_enable(reg) >> --> Send voltage=2960 then enable=1 >> 3. regulator_disable(reg) >> --> Send enable=0 >> 4. regulator_set_voltage(reg, 1800000, 2960000) >> --> A. Send voltage=1800 OR B. cache voltage=1800? >> >> Option A is used on the downstream rpmh-regulator driver in order to avoid >> cases where AP votes to disable a regulator that is kept enabled by >> another subsystem but then does not lower the voltage requested thanks to >> regulator_set_voltage() being called after regulator_disable(). I plan to >> go with option A for qcom-rpmh-regulator unless there are objections. > > So one client's vote for a voltage continues to be in effect even if > that client votes to have the regulator disabled? That seems > fundamentally broken in RPMh. I guess my take would be to work around > this RPMh misfeature by saying that whenever Linux votes to disable a > regulator it also votes for a voltage of 0. Then another client of > RPMh won't be affected. > > That seems like it would be beneficial in any case. If the AP always > asks for 1.3 V and the modem always asks for 1.1 V for the same rail > then the rail should go down to 1.1 V when the AP says to disable the > rail. The VRM in RPMh hardware aggregates enable state, output voltage, mode, and headroom voltage requests independently for each regulator. This allows for maximum request flexibility and makes no assumptions about consumer use cases. There is nothing inherently wrong about this approach. I'm concerned about a blanket policy of setting voltage=0 when issuing disable requests. That seems to violate the semantics of the regulator_set_voltage() API which enforces the requested voltage range until a new range is specified. There may be workarounds that require a regulator to operate at a specific voltage even when no Linux consumers explicitly need the regulator enabled. Given that not masking the voltage on disable is guaranteed to be safe and does not silently break potential use cases, I plan to stick with this approach. Therefore, the question about option A or B for voltage caching is still relevant. I think that option A is safer/more API conforming so I plan to implement that. >>> This whole thing makes me worry about another problem, though. You >>> say that the bootloader left the SD card rail on, right? ...but as >>> far as Linux is concerned the SD card rail is off (argh, it can't read >>> the state that the bootloader left the rail in!) >>> >>> ...now imagine any of the following: >>> >>> * The user boots up a kernel where the SD card driver is disabled. >>> >>> * The user ejects the SD card right after the bootloader used it but >>> before the kernel driver started. >>> >>> When the kernel comes up it will believe that the SD card rail is >>> disabled so it won't try to disable it. So the voltage will be left >>> on. >> >> We have not encountered issues with regulators getting left on >> indefinitely because Linux devices failed to take control of them during >> boot. I don't think that this hypothetical issue needs to be addressed in >> the first qcom-rpmh-regulator driver patch if at all. > > I don't think it hypothetical at all. Linux takes control of a > regulator and then at bootup it disables all regulators that aren't > currently used/enabled. In your case you will report that regulators > are already disabled and thus you'll neuter the kernel's attempt to > disable regulators that nobody is using but that might have been left > on by the bootloader. It seemed like Mark agreed here. I did not consider regulator_late_cleanup(). I'll initialize the enabled value in qcom-rpmh-regulator to -EINVAL to handle this case and also so that _regulator_enable() succeeds without further core modification. >>> You can even come up with a less contrived use case here. One could >>> argue that the SD card framework really ought to be ensuring VMMC and >>> VQMMC are off before it starts tweaking with them just in case the >>> bootloader left them on. Thus, it should do: >>> >>> A) Turn off VMMC and VQMMC >>> B) Program VMMC and VQMMC to defaults >>> C) Turn on VMMC and VQMMC >>> >>> ...right now we bootup and pretend to Linux that VMMC and VQMMC start >>> off, so step A) will be no-op. Sigh. >> >> Step A) would not work because the regulator's use_count would be 0 and >> regulator_disable() can only be called successfully if use_count > 0. The >> call would have no impact and it would return an error. > > Are you sure regulator_force_disable() won't do the trick on most > boards (which will report the regulator being enabled at bootup)? I > haven't tried it, but it seems like it might. regulator_force_disable() would indeed call the disable() callback. However, this function is designed for emergency uses only and will cause the use_count to become out of sync with the requested regulator state. I don't think that we want to suggest to consumers that they call regulator_force_disable(). It would completely break any shared regulator uses cases between multiple Linux consumers. Take care, David -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project