| Message ID | cover.1529712888.git.collinsd@codeaurora.org |
|---|---|
| Headers | show |
| Series | regulator: add QCOM RPMh regulator driver | expand |
Hi, On Fri, Jun 22, 2018 at 5:46 PM, David Collins <collinsd@codeaurora.org> wrote: > Add the QCOM RPMh regulator driver to manage PMIC regulators > which are controlled via RPMh on some Qualcomm Technologies, Inc. > SoCs. RPMh is a hardware block which contains several > accelerators which are used to manage various hardware resources > that are shared between the processors of the SoC. The final > hardware state of a regulator is determined within RPMh by > performing max aggregation of the requests made by all of the > processors. > > Add support for PMIC regulator control via the voltage regulator > manager (VRM) and oscillator buffer (XOB) RPMh accelerators. > VRM supports manipulation of enable state, voltage, and mode. > XOB supports manipulation of enable state. > > Signed-off-by: David Collins <collinsd@codeaurora.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/regulator/Kconfig | 9 + > drivers/regulator/Makefile | 1 + > drivers/regulator/qcom-rpmh-regulator.c | 746 ++++++++++++++++++++++++++++++++ > 3 files changed, 756 insertions(+) I've been working with David's v7 patch series for a bit of time now and things have been working well. v8 looks good. Feel free to add: Tested-by: Douglas Anderson <dianders@chromium.org> ...to this patch. Speaking of which it might be getting very close to time for this series to land along with David's other series, AKA: * [1/2] regulator: of: add property for allowed modes specification https://patchwork.kernel.org/patch/10395731/ * [2/2] regulator: of: add support for allowed modes configuration https://patchwork.kernel.org/patch/10395725/ Specifically I think we are mostly waiting for RPMh to land. This is on Andy's plate right now but I think it's happy and just waiting for the actual push from Andy. NOTE: I don't know if there anything special we need to do for landing this and RPMh across two git repositories. Offline Stephen Boyd mentioned to me that maybe it was fine to land this in Mark's tree without the RPMh code since it depends on "QCOM_RPMH", but the "|| COMPILE_TEST" worries me and makes me think auto-builders will start yelling. Andy and Mark: what do you think? If it's helpful to have a link to the latest RPMh, here are patchwork IDs on linux-arm-msm: 10477501 [v13,01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs 10477559 [v13,02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs 10477545 [v13,03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE 10477543 [v13,04/10] drivers: qcom: rpmh: add RPMH helper functions 10477537 [v13,05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS 10477539 [v13,06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS 10477533 [v13,07/10] drivers: qcom: rpmh: cache sleep/wake state requests 10477529 [v13,08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously 10477525 [v13,09/10] drivers: qcom: rpmh: add support for batch RPMH request 10477519 [v13,10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 25, 2018 at 11:50:52AM -0700, Doug Anderson wrote: > ...to this patch. Speaking of which it might be getting very close to > time for this series to land along with David's other series, AKA: > * [1/2] regulator: of: add property for allowed modes specification > https://patchwork.kernel.org/patch/10395731/ > * [2/2] regulator: of: add support for allowed modes configuration > https://patchwork.kernel.org/patch/10395725/ Please do not submit new versions of already applied patches, please submit incremental updates to the existing code. Modifying existing commits creates problems for other users building on top of those commits so it's best practice to only change pubished git commits if absolutely essential.
Hi, On Tue, Jun 26, 2018 at 5:07 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jun 25, 2018 at 11:50:52AM -0700, Doug Anderson wrote: > >> ...to this patch. Speaking of which it might be getting very close to >> time for this series to land along with David's other series, AKA: > >> * [1/2] regulator: of: add property for allowed modes specification >> https://patchwork.kernel.org/patch/10395731/ > >> * [2/2] regulator: of: add support for allowed modes configuration >> https://patchwork.kernel.org/patch/10395725/ > > Please do not submit new versions of already applied patches, please > submit incremental updates to the existing code. Modifying existing > commits creates problems for other users building on top of those > commits so it's best practice to only change pubished git commits if > absolutely essential. Sorry, wasn't suggesting making any changes to those two patches, just was noting the dependency. ...but, as you said, the two dependent patches have already landed and I just didn't notice. :( Sorry for the noise. -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 08:00:29AM -0700, Doug Anderson wrote: > On Tue, Jun 26, 2018 at 5:07 AM, Mark Brown <broonie@kernel.org> wrote: > > Please do not submit new versions of already applied patches, please > > submit incremental updates to the existing code. Modifying existing > > commits creates problems for other users building on top of those > > commits so it's best practice to only change pubished git commits if > > absolutely essential. > Sorry, wasn't suggesting making any changes to those two patches, just > was noting the dependency. ...but, as you said, the two dependent > patches have already landed and I just didn't notice. :( Sorry for > the noise. Ah, so there's no revisions that need merging? That's great.
Hi, On Tue, Jun 26, 2018 at 8:02 AM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Jun 26, 2018 at 08:00:29AM -0700, Doug Anderson wrote: >> On Tue, Jun 26, 2018 at 5:07 AM, Mark Brown <broonie@kernel.org> wrote: > >> > Please do not submit new versions of already applied patches, please >> > submit incremental updates to the existing code. Modifying existing >> > commits creates problems for other users building on top of those >> > commits so it's best practice to only change pubished git commits if >> > absolutely essential. > >> Sorry, wasn't suggesting making any changes to those two patches, just >> was noting the dependency. ...but, as you said, the two dependent >> patches have already landed and I just didn't notice. :( Sorry for >> the noise. > > Ah, so there's no revisions that need merging? That's great. Right. So all that's left to do is decide if ${SUBJECT} patch is ready to land and how to land it. Andy has landed RPMh into his for-next tree. You can see it at <https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git/log/?h=for-next>. I think he's still planning on re-shuffling his tree a bit. When he does this, do you need him to put the RPMh patches somewhere you can merge into your tree? Thanks! -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 11:15:14AM -0700, Doug Anderson wrote: > I think he's still planning on re-shuffling his tree a bit. When he > does this, do you need him to put the RPMh patches somewhere you can > merge into your tree? Well, I *think* there's no actual dependency here since it's a new driver with a Kconfig dependency. It really just needs me to get round to trawling through what's a fairly large patch with a troubled history now you've reviewed it.
Hi, On Wed, Jun 27, 2018 at 8:01 AM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Jun 26, 2018 at 11:15:14AM -0700, Doug Anderson wrote: > >> I think he's still planning on re-shuffling his tree a bit. When he >> does this, do you need him to put the RPMh patches somewhere you can >> merge into your tree? > > Well, I *think* there's no actual dependency here since it's a new > driver with a Kconfig dependency. It really just needs me to get round > to trawling through what's a fairly large patch with a troubled history > now you've reviewed it. OK, great. I guess I'm confused about the "|| COMPILE_TEST" causing problems then? I was worried that anyone trying to do "COMPILE_TEST" on your tree (or linuxnext if RPMh isn't there) would get failures due to the lack of header files. I guess if it's a problem you could just gut the "|| COMPILE_TEST" and it could be added back in later? Hoping my reviews saved you time overall. -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 09:28:03AM -0700, Doug Anderson wrote: > OK, great. I guess I'm confused about the "|| COMPILE_TEST" causing > problems then? I was worried that anyone trying to do "COMPILE_TEST" > on your tree (or linuxnext if RPMh isn't there) would get failures due > to the lack of header files. I guess if it's a problem you could just > gut the "|| COMPILE_TEST" and it could be added back in later? Ugh, yes - that'll break things. In that case I can't apply this without a signed tag from Andy's tree with the dependency stuff in.
Hello Mark, On 06/28/2018 03:18 AM, Mark Brown wrote: > On Wed, Jun 27, 2018 at 09:28:03AM -0700, Doug Anderson wrote: > >> OK, great. I guess I'm confused about the "|| COMPILE_TEST" causing >> problems then? I was worried that anyone trying to do "COMPILE_TEST" >> on your tree (or linuxnext if RPMh isn't there) would get failures due >> to the lack of header files. I guess if it's a problem you could just >> gut the "|| COMPILE_TEST" and it could be added back in later? > > Ugh, yes - that'll break things. In that case I can't apply this > without a signed tag from Andy's tree with the dependency stuff in. > Do you have any remaining concerns with the qcom-rpmh-regulator binding and driver patches that would keep you from applying them (other than the dependency patches being applied first)? Thanks, David
On Thu, Jun 28, 2018 at 11:04:17AM -0700, David Collins wrote: > Do you have any remaining concerns with the qcom-rpmh-regulator binding > and driver patches that would keep you from applying them (other than the > dependency patches being applied first)? To repeat I need to find the time to sit down and go through them properly, the problems this driver has had mean I feel I need to be extra careful there.
On Fri, Jun 22, 2018 at 05:46:14PM -0700, David Collins wrote: > --- /dev/null > +++ b/drivers/regulator/qcom-rpmh-regulator.c > @@ -0,0 +1,746 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ > + Please make the entire header block C++ so it looks intentional. > + cmd.data = bypassed ? PMIC4_BOB_MODE_PASS : pmic_mode; Please just write normal if statements, the ternery operator isn't really helping legibility. > +static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = { > + [REGULATOR_MODE_INVALID] = -EINVAL, > + [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION, > + [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM, > + [REGULATOR_MODE_NORMAL] = -EINVAL, > + [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM, > +}; This mapping is really weird, I'd expect one of the modes to correspond to the normal operating mode of the regulator. > +static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode) > +{ > + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = { > + [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, > + [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, > + [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_INVALID, > + [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST, > + }; Same here, based on that it looks like auto mode is a good map for normal. > + if (mode >= RPMH_REGULATOR_MODE_COUNT) > + return -EINVAL; Why not use ARRAY_SIZE?
Hello Mark, On 07/02/2018 03:28 AM, Mark Brown wrote: > On Fri, Jun 22, 2018 at 05:46:14PM -0700, David Collins wrote: > >> --- /dev/null >> +++ b/drivers/regulator/qcom-rpmh-regulator.c >> @@ -0,0 +1,746 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */ >> + > > Please make the entire header block C++ so it looks intentional. Sure, I'll change this. I was trying to follow the guideline that kernel C source files should use C style comments while at the same time following the SPDX guideline that C++ style comments are needed for the SPDX line in C source files [1]. >> + cmd.data = bypassed ? PMIC4_BOB_MODE_PASS : pmic_mode; > > Please just write normal if statements, the ternery operator isn't > really helping legibility. I'll change this. >> +static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = { >> + [REGULATOR_MODE_INVALID] = -EINVAL, >> + [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION, >> + [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM, >> + [REGULATOR_MODE_NORMAL] = -EINVAL, >> + [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM, >> +}; > > This mapping is really weird, I'd expect one of the modes to correspond > to the normal operating mode of the regulator. My thinking here was to have a consistent mapping for consumers to use between REGULATOR_MODE_* and physical regulator modes for both LDO and SMPS type regulators: REGULATOR_MODE_STANDBY --> Retention (if supported) REGULATOR_MODE_IDLE --> Low power mode (if supported) LPM for LDO and PFM for SMPS REGULATOR_MODE_NORMAL --> Auto HW switching between low and high power mode (if supported) REGULATOR_MODE_FAST --> High power mode HPM for LDO and PWM for SMPS This allows a consumer to request NORMAL in typical use cases and FAST in use cases that require low voltage ripple. If NORMAL is not supported, then it automatically gets upgraded to FAST by the regulator framework. I could change it so that REGULATOR_MODE_NORMAL maps to LDO HPM mode. However, doing so would make it so that REGULATOR_MODE_FAST requests would fail for LDOs. Thus, consumers would need to know if their supply is an LDO or an SMPS (which seems undesirable). Would it be acceptable to have both NORMAL and FAST map to LDO HPM? >> +static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode) >> +{ >> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = { >> + [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, >> + [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, >> + [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_INVALID, >> + [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST, >> + }; > > Same here, based on that it looks like auto mode is a good map for > normal. LDO type regulators physically do not support AUTO mode. That is why I specified REGULATOR_MODE_INVALID in the mapping. >> + if (mode >= RPMH_REGULATOR_MODE_COUNT) >> + return -EINVAL; > > Why not use ARRAY_SIZE? I'll change this. Thanks, David [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst?h=v4.18-rc4#n74
On Mon, Jul 09, 2018 at 04:44:14PM -0700, David Collins wrote: > On 07/02/2018 03:28 AM, Mark Brown wrote: > > On Fri, Jun 22, 2018 at 05:46:14PM -0700, David Collins wrote: > >> +static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = { > >> + [REGULATOR_MODE_INVALID] = -EINVAL, > >> + [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION, > >> + [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM, > >> + [REGULATOR_MODE_NORMAL] = -EINVAL, > >> + [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM, > >> +}; > > This mapping is really weird, I'd expect one of the modes to correspond > > to the normal operating mode of the regulator. > My thinking here was to have a consistent mapping for consumers to use > between REGULATOR_MODE_* and physical regulator modes for both LDO and > SMPS type regulators: No, that's not useful or helpful - if there's any modes I'd *really* expect to see one of them be _NORMAL. > This allows a consumer to request NORMAL in typical use cases and FAST in > use cases that require low voltage ripple. If NORMAL is not supported, > then it automatically gets upgraded to FAST by the regulator framework. > I could change it so that REGULATOR_MODE_NORMAL maps to LDO HPM mode. > However, doing so would make it so that REGULATOR_MODE_FAST requests would > fail for LDOs. Thus, consumers would need to know if their supply is an > LDO or an SMPS (which seems undesirable). You've just discovered why it's a bad idea for consumers to do anything with modes directly! The mappings are just never going to be consistent given the massive variation in what regulators can support, the retention mode of one regulator might be able to deliver more power than the normal mode of another. > Would it be acceptable to have both NORMAL and FAST map to LDO HPM? Having something other than a 1:1 mapping is going to lead to pain at some point. > >> +static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode) > >> +{ > >> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = { > >> + [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, > >> + [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, > >> + [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_INVALID, > >> + [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST, > >> + }; > > Same here, based on that it looks like auto mode is a good map for > > normal. > LDO type regulators physically do not support AUTO mode. That is why I > specified REGULATOR_MODE_INVALID in the mapping. The other question here is why this is even in the table if it's not valid (I'm not seeing a need for the MODE_COUNT define)?
On 07/12/2018 09:54 AM, Mark Brown wrote: > On Mon, Jul 09, 2018 at 04:44:14PM -0700, David Collins wrote: >> On 07/02/2018 03:28 AM, Mark Brown wrote: >>> On Fri, Jun 22, 2018 at 05:46:14PM -0700, David Collins wrote: >>>> +static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode) >>>> +{ >>>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = { >>>> + [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY, >>>> + [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, >>>> + [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_INVALID, >>>> + [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST, >>>> + }; > >>> Same here, based on that it looks like auto mode is a good map for >>> normal. > >> LDO type regulators physically do not support AUTO mode. That is why I >> specified REGULATOR_MODE_INVALID in the mapping. > > The other question here is why this is even in the table if it's not > valid (I'm not seeing a need for the MODE_COUNT define)? I thought that having a table would be more concise and easier to follow. I can change this to a switch case statement. Take care, David