Received: by 2002:ac0:a594:0:0:0:0:0 with SMTP id m20-v6csp410109imm; Tue, 22 May 2018 22:11:19 -0700 (PDT) X-Google-Smtp-Source: AB8JxZoSHA6TlYCsOGKPAdUJc91f4b2ZVt5BgYlgJLwvdYQleFiVB4yAtJggaqxM9Nbrj4AyhHUt X-Received: by 2002:a65:4b4d:: with SMTP id k13-v6mr1117615pgt.198.1527052279780; Tue, 22 May 2018 22:11:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1527052279; cv=none; d=google.com; s=arc-20160816; b=FB4pgd65/FLsUXgAsmd62784Q7wgYaZ1EuYfs3y2S2agbETMkxtx7Ywh9Hi7tpQje4 72Iq6fVPaxeyW7VXnr9tizkmRMJdgnAkvadlGhNXQUNt6laKFIMY4xLApsW6a/Mv7xgf 2mPohdeiPh5yM2VydxB2Y/2SbW3TJTfxGv19OUZ8zm2d0yvwc9pB1oOd1MI0BeSD373i n5Vk/1tUAjLEU3ECZEEbWg7+QvBZqoXjcIKkNi5q5/aqNUO1qKv1LXza3o2S0jIm8F4W rTLOKBNwIF6yAL1GkrBKIZ8GcVSbUrfMckqfXgwUDC3L6kVwgi0C9tisWsxvZgq0y9p4 1j9w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature:dkim-signature :arc-authentication-results; bh=Uswc/8rP7z0XNTzd65Hr7d53Rm9VLMPWhB0NPE4QpoE=; b=eFq9Jv/06NCOhl13CkUX/253/KBjVofkGaDNeSP0FPUzhbx2nfWxfIBCGST2Ja/pgj 90rzyEB4CoEX8Jipvo34XUf/MUG5c0H0olC0hz3zH+TS890+k/W9lXbBDnVXV6jZRNLu cprfoFtfKAf1F2OiHZTok0Bbk73UcwX8JHUWkaZXF1UEwkFsRB1p5h6Y7QShEWhCKO5v i/rwhY5F2cjWqMG8hi2C0U/RPoxeGX7Bu6rONkPjoSMpk1rwmpf3shgIyPe6XEZC5KTQ wPzMkmN937jAq9MsNcDeNVs9PbYsFqZ6zxfZ2M3EkE0sqbL8r9Nc3DgoXRWhFpwk7mwk dMPQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@google.com header.s=20161025 header.b=NrURQxHn; dkim=fail header.i=@chromium.org header.s=google header.b=b/b2FJxQ; 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; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 77-v6si17661136pfz.334.2018.05.22.22.11.04; Tue, 22 May 2018 22:11:19 -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=fail header.i=@google.com header.s=20161025 header.b=NrURQxHn; dkim=fail header.i=@chromium.org header.s=google header.b=b/b2FJxQ; 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; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753949AbeEWFKx (ORCPT + 99 others); Wed, 23 May 2018 01:10:53 -0400 Received: from mail-vk0-f66.google.com ([209.85.213.66]:34752 "EHLO mail-vk0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750744AbeEWFKu (ORCPT ); Wed, 23 May 2018 01:10:50 -0400 Received: by mail-vk0-f66.google.com with SMTP id t63-v6so12361714vkb.1 for ; Tue, 22 May 2018 22:10:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=Uswc/8rP7z0XNTzd65Hr7d53Rm9VLMPWhB0NPE4QpoE=; b=NrURQxHnLu6vZ2JlWwa82p6fKPr8XXjGFpxhFY2liqrdm4wMKizEoI//n8r7ScMTYM x77qPnXiFKYoYYK2UmyWbH0uTsRuZ/mKpZAGY3wvvnnEMHPGOt24Ugl+ZcJFaTioW15y 1Hr2LJr/XIJukrWWl8MvEBQyJCDDfVPXTe+gK+CLL89Ky/aQjXQWdqizVtUpolCau7N7 c4w83RWcmnLbspVvn2Ld1qHwZrf6WLw03Hl3irASkVdh+t1MfgrHJgvivDynqp0wwt7i 8Sm8haXNgMSKPkNjM3Mp6lCICPB+4V3t2DBEfYJLBDfNqf9ACO4WHDdIMzAtKCq/FFFn bVAw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=Uswc/8rP7z0XNTzd65Hr7d53Rm9VLMPWhB0NPE4QpoE=; b=b/b2FJxQh3SR9t8adfAkKwhZ6FtLzEOjEEr83dct3E75QZ6o+QZIFobowl6mkvvwwe 5Z7ZMwCBAEPm3jJDdjoOta8U091kYpD6gy42nJrkFP+7lmGYOS2n0x8FAyZRMUxplg5c QAzSzSLjqnufmXj/pMAeYXMrq/9hpJBXLGvRE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=Uswc/8rP7z0XNTzd65Hr7d53Rm9VLMPWhB0NPE4QpoE=; b=ZohQ7fWVepmIr2/J8GE2/aWninsTHQqrOs2PkZyWLGhEmy5isnpTSNZGb9pb6E/oTC qp+IRilXVq4Jou2A21nPBFcgqi5SjO+01Holo76lPKxWH0TTCpUd/v8CKnPeZxHCvmt6 1u7Of9GBjj0xkFONzTEMKUws0P6nAZDh4lwLZ8999DrbYclKWppipjnZW66b6bzMM3DE BnmE5GOaGUvOVOSGhFWOR4H4SWNXe756eQuqy7w88ptPU0pX5ne+EsbbB8t/o44nKzfm FlmVDm3b9AGS4mwnrAjB6aKI9xaqIy4JPNNhvl5fOpYt4YKDJo1opi+9FwLDx+NgyJ27 mY2g== X-Gm-Message-State: ALKqPwd8YIR6s0STJ4S9vJv+IAqUxG4836AB2jXn+rt3X6AltFJvR8FL IbUjHd5YDfl4U3mfSIK9WzFJRqNBpOr7eEHRcqmsDQ== X-Received: by 2002:a1f:fc4a:: with SMTP id a71-v6mr778497vki.141.1527052249140; Tue, 22 May 2018 22:10:49 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a1f:3052:0:0:0:0:0 with HTTP; Tue, 22 May 2018 22:10:47 -0700 (PDT) In-Reply-To: References: <41d5df73ddac772551d2966e0752bb0c357b1ded.1526088081.git.collinsd@codeaurora.org> <869aad59-1cc5-28ef-1fb5-4ef846696c40@codeaurora.org> From: Doug Anderson Date: Tue, 22 May 2018 22:10:47 -0700 X-Google-Sender-Auth: BCBYiAq0zSaBssAwBf2X02Jbo4o Message-ID: Subject: Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings To: David Collins 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 Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Tue, May 22, 2018 at 6:19 PM, David Collins wrote: >>>> 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. Just to confirm I'm understanding correctly: 1. AP: request that regulator A be at 1.3 V and enabled ==> actual output: regulator A is 1.3 V and enabled 2. modem: requests that regulator A be at 1.1 V and enabled ==> actual output: regulator A is 1.3 V and enabled 3. AP: request that regulator A be disabled You're saying that here the output of regulator A will be "1.3 V" and "enabled". I just can't see how that can be correct behavior. A given client's vote for the voltage should really only make sense if that client wants the regulator enabled. In the case above the kernel would have no idea that someone else might have the regulator enabled. Why would it care if that other user gets it at 1.3 V or at 1.1 V? You have some use case in mind where the kernel would need to have control over the voltage of a regulator that it thinks is disabled? Now obviously if the kernel re-enables the regulator then its old voltage vote should be re-instated right away, but until then its vote about the voltage shouldn't count. If that means that the kernel has to "vote" for 0V then I guess that's the way the API works. > 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. I still can't understand how it ever makes sense to vote for a voltage for a regulator that you think is disabled. ...but if there's some reason it does then I guess I'm OK with A. >>>> 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. That's weird that the regulator code has a special case for -EINVAL in _regulator_enable(). Given how most of the code in the regulator/core.c doesn't seem to check for error codes I wonder if anyone is using that... I guess I'd leave it to Mark whether he's happy with -EINVAL for this case even though it's asymmetric to using -ENOTRECOVERABLE for getting the voltage. Are we really sure there aren't any places that the regulator code needs to handle an error from _regulator_enable()? An an example, in regulator_resolve_supply() are we sure we should be passing a regulator_enable() on to our parent supply even if _regulator_is_enabled() returns an error? I haven't looked in depth of all use cases, though... I see you posted a new version. Thanks! I'll try to find some time soon to review it, but I'll be a bit busy over the next few days. -Doug