Received: by 10.213.65.68 with SMTP id h4csp67187imn; Tue, 27 Mar 2018 16:40:06 -0700 (PDT) X-Google-Smtp-Source: AIpwx4+rapf6JtjzJG0Xwg3ml7F4S96p4obyozyGkKA2eFwKH4zxLFk4ClaYBwmaOzKSG3H+cF/r X-Received: by 2002:a17:902:b485:: with SMTP id y5-v6mr1274624plr.91.1522194006609; Tue, 27 Mar 2018 16:40:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1522194006; cv=none; d=google.com; s=arc-20160816; b=INuCO+p46ZYkNM77zHKUt6F1IutddYIaO0v4dNgD2y5IMl5M5dvkl3YbsnWoagXHhJ PGjk9Py7fXUDB+i7c6EX/HacKEINRRky3y5clxvONolWKWyh2ah0ESUEswtUqZtbNz78 0oJJc3IoK4Ar+f2KemnmZdeX+BRDUA3Xo9ij8cvDwscDDDRkAkuBSbwGpOygsWuAI5ZA XfFt1B7ePgY+ZrqOBYVEX3oFlrPDxQB3aPv6k4ftohfvTss7fRroKCg1pNQFSW1dn/EX yn7hBDanUICe0T38czOZCRt0Tdon/f5TLEvxAAeV1KP0YVYrRx398p1hRkf+YCKQwQhB UJJg== 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=8nOQxBzLcgJOJpz2p1wWe+x9SnKt+0FpK75ag3lpy9E=; b=0TUGW+s81pNQz+J3bTXLrR6dSLn5v2LSjFjLC681KipUBHY2exq+4gxzqdgGLhxnQn 5Uslt1FQL0e20S9opG5MRnI/GpXzPUujvLm5mYvBZFFHfCDvAI1F2B81tWYDWLHnBXFQ cYPmaRyROr5XpSNN0yVlNDZIDOKDgsgDbS4s4+fXZDdsfbIoby5BfVbW1jXaP3HQqZP4 EXC8xhE3G04j2Ha3HfOsf5zCP6cAixi0I07U8C8QS2efdaOUuOV8XCeftxASRCl8QLCp sR2KEuw2wbWTRFAqUhvoJ6PpZxmBVeFS+YiBOYyfJ9Q8YmtNTBiV6NrhHTUdVXHlGxn3 zmfQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=L+B3+IhG; dkim=pass header.i=@codeaurora.org header.s=default header.b=Fg7xhsyP; 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 m8si1551907pgp.369.2018.03.27.16.39.41; Tue, 27 Mar 2018 16:40:06 -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=L+B3+IhG; dkim=pass header.i=@codeaurora.org header.s=default header.b=Fg7xhsyP; 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 S1752338AbeC0XiM (ORCPT + 99 others); Tue, 27 Mar 2018 19:38:12 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:35274 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752129AbeC0XiL (ORCPT ); Tue, 27 Mar 2018 19:38:11 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 6560F606DB; Tue, 27 Mar 2018 23:38:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1522193890; bh=skysTQF9fKbxGbzoFnMRIeE9FyihE8gurl7Zg40fwmE=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=L+B3+IhGWsAXF35OvAAnD1As7IByXqbhS8A/3tz7ZcUmDCDOCqpby5oA87jLqgDBH 1hEyBXAbVRdyRef0LnB04qXLXVMmyOdNA9cMZN9KNY4k6neLktEDx2OqnG4Xa36zv7 MdgsHQqlie0JO4YEduQPmEwvxYeqepDxUXryVvGc= 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.164.143] (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 4B5C260588; Tue, 27 Mar 2018 23:38:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1522193888; bh=skysTQF9fKbxGbzoFnMRIeE9FyihE8gurl7Zg40fwmE=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=Fg7xhsyPDjkI1+SyOhUvNyTU9DUOA9VSWRcggGER5Mv+PmURrlzwCO9C8ZXoKvR7h OC26nEEM2IhjR3AFbGZL5ncOK+lrm6YDeJUIs9DrEq20F92W8Ste68WxOt2ARpr8Sk xITNIyNkVz18ihO8Jwa5Jtcmk1VC1QIb7cXPkno0= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 4B5C260588 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 1/2] regulator: add QCOM RPMh regulator driver To: Mark Brown , Doug Anderson Cc: Liam Girdwood , Rob Herring , Mark Rutland , linux-arm-msm@vger.kernel.org, Linux ARM , devicetree@vger.kernel.org, LKML , Rajendra Nayak , sboyd@kernel.org, ilina@codeaurora.org References: <71fab82672524b95632cdb588c16edfc9711866a.1521246069.git.collinsd@codeaurora.org> <184378e4-caf8-6ce3-e089-3690588fcb28@codeaurora.org> <20180327115606.GC29239@sirena.org.uk> From: David Collins Message-ID: <172d1da4-bde0-1f0c-b907-5582c31c8156@codeaurora.org> Date: Tue, 27 Mar 2018 16:38:07 -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: <20180327115606.GC29239@sirena.org.uk> Content-Type: text/plain; charset=windows-1252 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 03/27/2018 04:56 AM, Mark Brown wrote: > On Fri, Mar 23, 2018 at 01:00:47PM -0700, Doug Anderson wrote: >> On Thu, Mar 22, 2018 at 3:31 PM, David Collins wrote: > >>> There are two cases that I can think of: 1. Having a set_voltage() >>> callback allows for delaying for an RPMh request ACK during certain >>> voltage set point decreasing scenarios (to be elaborated below). > >> Can't you still have a delay in set_voltage_sel()? > > We have specific support for adding ramp delays, no need to open code it > in operations. I'll switch to using set_voltage_sel(). No ramp delay is need in Linux. RPMh hardware sends an ACK back to Linux after the voltage reaches the desired set point (i.e. is greater than or equal to the set point). The qcom_rpmh-regulator driver just needs to decide if it is going to wait for the ACK from RPMh or not. >>> 2. >>> Having a get_voltage() as opposed to get_voltage_sel() callback allows an >>> uninitialized voltage of 0 to be returned in the case that no initial >>> voltage is specified in device tree. This eliminates the possibility of >>> the regulator framework short-circuiting a regulator_set_voltage() call >>> that happens to match the voltage corresponding to selector 0. > >> Interesting. I suppose you could mix them (have set_voltage_sel() and >> get_voltage()) as long as you documented why you were doing it. Then >> we'd have to see if Mark was happy with that... > > This is a *terrible* idea which will almost certainly break. If the > driver can't read values it should return an appropriate error code. I'll switch to using get_voltage_sel(). I'll test out returning an error value in the case of an uninitialized voltage. Hopefully the regulator framework won't halt regulator registration because of it. >>> I'm aware of one important instance in which decreasing voltage needs a >>> delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case >>> that I had in mind with the 'max_uv < vreg->voltage' check. However, you >>> are correct that hardware will report completion of the voltage slewing >>> early in this case since the comparator is checking for output >= set >>> point. I can remove this special case check. > > You can't usefully wait for voltages to fall, you can never guarantee > what the loading on the device is. It's something the user has to > manage if they care. I agree. This case can't be handled in the regulator driver. >>> Here is an explanation for why the "device tree mode" abstraction is >>> present in the first place. Between different Qualcomm Technologies, Inc. >>> PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, >>> LPM/PFM, Retention, and pass-through). However, the register values for >>> the same modes vary between different regulator types and different PMIC >>> families. This patch is adding support for several PMIC4 family PMICs. >>> The values needed for to-be-released PMIC5 PMIC regulators are different. >>> As an example, here are the different values for LPM/PFM across PMIC >>> families and regulator types: PMIC4 LDO/SMPS = 5, PMIC4 BOB = 1, PMIC5 >>> LDO/HFSMPS/BOB = 4, PMIC5 FTSMPS = N/A. Having the "device tree mode" >>> ensures that it is not possible to inadvertently specify a PMIC specific >>> mode in device tree which corresponds to the wrong type or family but >>> which aliases a value that would be accepted as correct. > >> I'm OK with having the "device tree mode" abstraction, and in fact the >> current regulator framework seems to want you to have this anyway. If >> I read the code correctly, you're required to have the conversion >> function and there's no default. > > I didn't spot this in the code but something called "device tree mode" > sounds like it's going to be awfully confusing... As I explained in the earlier email, it makes the device tree configurations much simpler and less confusing/error prone. I'd like to keep this concept around unless their are strong objections. >>> Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm >>> Technologies, Inc. buck-or-boost regulators (BOB). When PASS is selected, >>> the BOB output is directly connected to the BOB input (typically Vbat). > > ... > >>> qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode >>> could be handled via get_bypass() and set_bypass() regulator ops. Doing >>> this would require more complicated ops selections in the driver since it > > This is exactly the functionality supported by the bypass operations. > Any complexity due to the hardware design is unfortunate but honestly > the way the QC regulator stuff is designed they seem like a bit of a > lost cause on that front - they look very different to any other > hardware we've seen. > >> I've never poked at the get_bypass() / set_bypass(), but it sounds >> better to me to use them. I'm not a fan of the current hack. Even >> aside from the bit of hackiness, I'm slightly concerned that some of >> your logic that generally assumes lower integers = lower power modes >> would break. > > Yes, abusing the framework is just going to make things even worse. I'll implement get_bypass() and set_bypass() callbacks for BOB. >>>>> +arch_initcall(rpmh_regulator_init); > >>>> I always get yelled at when I try to use arch_initcall() for stuff >>>> like this. You should do what everyone else does and use > >>> I agree that consumers should handle probe deferral. Unfortunately, >>> reality isn't always so nice. Also, probe deferrals increase boot-up time >>> which is particularly problematic in the mobile space. > >> Sigh, yeah. I'm not a fan either. If you can convince Mark that you >> should use arch_initcall() or subsys_initcall() I won't yell. ...but >> in the past I've seen others get yelled at. > > Do you have concrete consumers that have a good reason for doing this? I don't have any examples off the top of my head. >> Note: in actuality it doesn't always increase boot time a whole lot. > > Note also that we now have the device dependency mechanism that Raphael > implemented with the explicit idea that that it'd be used to avoid > unneeded deferrals. > >> ...but deferrals _do_ for sure increase the time for certain >> peripherals to come up, and if those peripherals are things like the >> LCD displays then it sucks. > > There's been some discussion of allowing the user to specific certain > devices to target as priorities for probing which should deal with that. I'll look into these features. Take care, David -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project