| Message ID | 1637314953-4215-2-git-send-email-quic_c_skakit@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Qualcomm Technologies, Inc. PM8008 regulator driver | expand |
On Fri, Nov 19, 2021 at 03:12:28PM +0530, Satya Priya wrote: > + regulator-min-dropout-voltage-microvolt: > + description: Specifies the minimum voltage in microvolts that the > + parent supply regulator must output, above the output of this regulator. Usually this is a fixed property of the regulator rather than something that varies per board - why have a DT property? Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
On 11/25/2021 8:47 PM, Mark Brown wrote: > On Fri, Nov 19, 2021 at 03:12:28PM +0530, Satya Priya wrote: > >> + regulator-min-dropout-voltage-microvolt: >> + description: Specifies the minimum voltage in microvolts that the >> + parent supply regulator must output, above the output of this regulator. > Usually this is a fixed property of the regulator rather than something > that varies per board - why have a DT property? The min-dropout value (headroom) varies with boards, that's why we have a DT property for it. We overwrite the default value in driver with actual value read from DT
On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote: > On 11/25/2021 8:47 PM, Mark Brown wrote: > > Usually this is a fixed property of the regulator rather than something > > that varies per board - why have a DT property? > The min-dropout value (headroom) varies with boards, that's why we have a DT > property for it. We overwrite the default value in driver with actual value > read from DT Interesting. How exactly does that end up happening - presumably other systems are going to run into it? If you do have board designs which somehow managed to introduce additional dropouts (seems pretty concerning TBH) then I think the best way to handle that is to add a generic property for it and have that either added on to or override the requirements of the regulator itself which should continue to be defined in the driver. That way only boards with issues need to do anything which will avoid bugs with the property being omitted in what should be the common case.
On 12/6/2021 11:55 PM, Mark Brown wrote: > On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote: >> On 11/25/2021 8:47 PM, Mark Brown wrote: >>> Usually this is a fixed property of the regulator rather than something >>> that varies per board - why have a DT property? >> The min-dropout value (headroom) varies with boards, that's why we have a DT >> property for it. We overwrite the default value in driver with actual value >> read from DT > Interesting. How exactly does that end up happening - presumably other > systems are going to run into it? The parent supplies such as "vdd-l1-l2" are coming from other pmic regulators, which are shared supplies with other subsystems like BT, Display etc, they vary between boards as per requirements, so we cannot expect these to be fixed and so are the headroom values. We get the headroom values from PMIC systems team for every target. > If you do have board designs which somehow managed to introduce > additional dropouts (seems pretty concerning TBH) then I think the best > way to handle that is to add a generic property for it and have that > either added on to or override the requirements of the regulator itself > which should continue to be defined in the driver. That way only boards > with issues need to do anything which will avoid bugs with the property > being omitted in what should be the common case.
On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote: > On 12/6/2021 11:55 PM, Mark Brown wrote: > > On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote: > > > The min-dropout value (headroom) varies with boards, that's why we have a DT > > > property for it. We overwrite the default value in driver with actual value > > > read from DT > > Interesting. How exactly does that end up happening - presumably other > > systems are going to run into it? > The parent supplies such as "vdd-l1-l2" are coming from other pmic > regulators, which are shared supplies with other subsystems like BT, Display > etc, they vary between boards as per requirements, so we cannot expect these > to be fixed and so are the headroom values. We get the headroom values from > PMIC systems team for every target. I don't think you're talking about the thing the code is saying it's describing here. The regulator API is referring to the minimum droput voltage that individual regulators require, that is how much higher the input to a single regulator must be than the voltage being output by that regulator. We absolutely can and do expect this to be board independent, it's a function of the design of the regulator. Sharing the input supply has no impact on this, the input voltage that the regulator needs just get fed into the requiremnts on the supply voltage. If there is a board specific constraint on the minimum voltage that a given supply can have then that should be expressed using the normal constraint mechanism, that's nothing to do with the headroom that the regulators require to operate though.
On 12/7/21 7:19 AM, Mark Brown wrote: > On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote: >> On 12/6/2021 11:55 PM, Mark Brown wrote: >>> On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote: > >>>> The min-dropout value (headroom) varies with boards, that's why we have a DT >>>> property for it. We overwrite the default value in driver with actual value >>>> read from DT > >>> Interesting. How exactly does that end up happening - presumably other >>> systems are going to run into it? > >> The parent supplies such as "vdd-l1-l2" are coming from other pmic >> regulators, which are shared supplies with other subsystems like BT, Display >> etc, they vary between boards as per requirements, so we cannot expect these >> to be fixed and so are the headroom values. We get the headroom values from >> PMIC systems team for every target. > > I don't think you're talking about the thing the code is saying it's > describing here. The regulator API is referring to the minimum droput > voltage that individual regulators require, that is how much higher the > input to a single regulator must be than the voltage being output by > that regulator. We absolutely can and do expect this to be board > independent, it's a function of the design of the regulator. Sharing > the input supply has no impact on this, the input voltage that the > regulator needs just get fed into the requiremnts on the supply voltage. > > If there is a board specific constraint on the minimum voltage that a > given supply can have then that should be expressed using the normal > constraint mechanism, that's nothing to do with the headroom that the > regulators require to operate though. The PM8008 LDOs are low noise LDOs intended to supply noise sensitive camera sensor hardware. They can maintain output regulation with a fixed headroom voltage. However, in order to guarantee high PSRR, the headroom voltage must be scaled according to the peak load expected from the each LDO on a given board. Thus, we included support for a DT property to specify the headroom per LDO to meet noise requirements across boards. As a minor note the PM8008 chip package has a single pin to supply LDOs 1 and 2 along with a single pin for LDOs 3 and 4. That is why vdd_l1_l2-supply is specified instead of vdd_l1-supply and vdd_l2-supply. Take care, David
On Wed, Dec 08, 2021 at 04:56:48PM -0800, David Collins wrote: > On 12/7/21 7:19 AM, Mark Brown wrote: > > On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote: > > that regulator. We absolutely can and do expect this to be board > > independent, it's a function of the design of the regulator. Sharing > > the input supply has no impact on this, the input voltage that the > > regulator needs just get fed into the requiremnts on the supply voltage. > The PM8008 LDOs are low noise LDOs intended to supply noise sensitive > camera sensor hardware. They can maintain output regulation with a > fixed headroom voltage. However, in order to guarantee high PSRR, the > headroom voltage must be scaled according to the peak load expected from > the each LDO on a given board. Thus, we included support for a DT > property to specify the headroom per LDO to meet noise requirements > across boards. Interesting... how much extra headroom are we talking about here? I'd be unsurprised to see this usually just quoted as part of the standard headroom requirement and this smells like the sort of thing that's going to be frequently misused. If the gains are something worth writing home about I'd think we should consider if it's better to support this dynamically at runtime based on load information and provide options for configuring the peak load information through DT instead for static configurations. That would fit in with the stuff we have for managing modes on DCDCs (which isn't really deployed but is there) and the API we have for allowing client drivers to indicate their load requirements at runtime that fits in with that. That'd allow us to only boost the headroom when it's really needed.
On 12/11/2021 2:41 AM, Mark Brown wrote: > On Wed, Dec 08, 2021 at 04:56:48PM -0800, David Collins wrote: >> On 12/7/21 7:19 AM, Mark Brown wrote: >>> On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote: >>> that regulator. We absolutely can and do expect this to be board >>> independent, it's a function of the design of the regulator. Sharing >>> the input supply has no impact on this, the input voltage that the >>> regulator needs just get fed into the requiremnts on the supply voltage. >> The PM8008 LDOs are low noise LDOs intended to supply noise sensitive >> camera sensor hardware. They can maintain output regulation with a >> fixed headroom voltage. However, in order to guarantee high PSRR, the >> headroom voltage must be scaled according to the peak load expected from >> the each LDO on a given board. Thus, we included support for a DT >> property to specify the headroom per LDO to meet noise requirements >> across boards. > Interesting... how much extra headroom are we talking about here? I'd > be unsurprised to see this usually just quoted as part of the standard > headroom requirement and this smells like the sort of thing that's going > to be frequently misused. If the gains are something worth writing home > about > I'd think we should consider if it's better to support this > dynamically at runtime based on load information and provide options for > configuring the peak load information through DT instead for static > configurations. That would fit in with the stuff we have for managing > modes on DCDCs (which isn't really deployed but is there) and the API we > have for allowing client drivers to indicate their load requirements at > runtime that fits in with that. That'd allow us to only boost the > headroom when it's really needed. This means Dynamic headroom control feature needs to be implemented. I need to explore more on this and gather info from team, Could we merge the present driver with "static headroom" for now? I'll do a follow up series to implement this feature.
On Mon, Jan 03, 2022 at 08:05:40PM +0530, Satya Priya Kakitapalli (Temp) wrote: > On 12/11/2021 2:41 AM, Mark Brown wrote: > > I'd think we should consider if it's better to support this > > dynamically at runtime based on load information and provide options for > > configuring the peak load information through DT instead for static > > configurations. That would fit in with the stuff we have for managing > > modes on DCDCs (which isn't really deployed but is there) and the API we > > have for allowing client drivers to indicate their load requirements at > > runtime that fits in with that. That'd allow us to only boost the > > headroom when it's really needed. > This means Dynamic headroom control feature needs to be implemented. I need > to explore more on this and gather info from team, Could we merge the > present driver with "static headroom" for now? I'll do a follow up series to > implement this feature. I'd be happy to merge something with the headroom configured statically in the driver like we do for other devices - I guess if you set the highest headroom that should cover it.
diff --git a/Documentation/devicetree/bindings/regulator/regulator.yaml b/Documentation/devicetree/bindings/regulator/regulator.yaml index a6ae9ec..fdb37d3 100644 --- a/Documentation/devicetree/bindings/regulator/regulator.yaml +++ b/Documentation/devicetree/bindings/regulator/regulator.yaml @@ -224,6 +224,10 @@ properties: description: Maximum difference between current and target voltages that can be changed safely in a single step. + regulator-min-dropout-voltage-microvolt: + description: Specifies the minimum voltage in microvolts that the + parent supply regulator must output, above the output of this regulator. + patternProperties: ".*-supply$": description: Input supply phandle(s) for this node
Add "regulator-min-dropout-voltage-microvolt" property. Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> --- Changes in V4: - This is newly added in V4. Documentation/devicetree/bindings/regulator/regulator.yaml | 4 ++++ 1 file changed, 4 insertions(+)