| Message ID | 20230818153446.1076027-1-shenwei.wang@nxp.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [1/2] dt-bindings: power: Add regulator-pd yaml file | expand |
| Context | Check | Description |
|---|---|---|
| robh/dtbs-check | warning | build log |
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | success |
On Fri, Aug 18, 2023 at 10:35 AM Shenwei Wang <shenwei.wang@nxp.com> wrote: > > Documenting the regulator power domain properties and usage examples. This needs to answer why we need this. It looks like just an abstraction layer to make regulators look like a power domain. Rob
> -----Original Message----- > From: Rob Herring <robh+dt@kernel.org> > Sent: Friday, August 18, 2023 3:52 PM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > > > Documenting the regulator power domain properties and usage examples. > > This needs to answer why we need this. > > It looks like just an abstraction layer to make regulators look like a power > domain. > Yes, it is a wrapper that allows using regulators as a power domain. This removes the need to add regulator operating code in each consumer device driver. As a power domain, the regulator will be managed automatically by the device driver framework and PM subsystem. This is very useful when a device's power is controlled by a GPIO pin, which currently requires using the fixed-regulator to achieve the same purpose. However, the fixed-regulator approach may have to add code in the driver in order to use it. Thanks, Shenwei > Rob
On 18/08/2023 23:06, Shenwei Wang wrote: > > >> -----Original Message----- >> From: Rob Herring <robh+dt@kernel.org> >> Sent: Friday, August 18, 2023 3:52 PM >> To: Shenwei Wang <shenwei.wang@nxp.com> >> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley >> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood >> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; >> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> dl-linux-imx <linux-imx@nxp.com> >> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file >>> >>> Documenting the regulator power domain properties and usage examples. >> >> This needs to answer why we need this. >> >> It looks like just an abstraction layer to make regulators look like a power >> domain. >> > > Yes, it is a wrapper that allows using regulators as a power domain. This removes > the need to add regulator operating code in each consumer device driver. As a power > domain, the regulator will be managed automatically by the device driver framework > and PM subsystem. > > This is very useful when a device's power is controlled by a GPIO pin, which currently > requires using the fixed-regulator to achieve the same purpose. However, the > fixed-regulator approach may have to add code in the driver in order to use it. Why do you start discussion from zero ignoring all previous history of this patchset? https://lore.kernel.org/all/20220609150851.23084-1-max.oss.09@gmail.com/ Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Saturday, August 19, 2023 3:04 AM > To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring > <robh+dt@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > >> > >> This needs to answer why we need this. > >> > >> It looks like just an abstraction layer to make regulators look like > >> a power domain. > >> > > > > Yes, it is a wrapper that allows using regulators as a power domain. > > This removes the need to add regulator operating code in each consumer > > device driver. As a power domain, the regulator will be managed > > automatically by the device driver framework and PM subsystem. > > > > This is very useful when a device's power is controlled by a GPIO pin, > > which currently requires using the fixed-regulator to achieve the same > > purpose. However, the fixed-regulator approach may have to add code in the > driver in order to use it. > > Why do you start discussion from zero ignoring all previous history of this > patchset? > Thank you for providing the link. After reviewing the entire thread, I still don't understand how to proceed. What is the conclusion regarding this commonly used use case but overlooked feature in the upstream kernel? Thanks, Shenwei > https://lore.kern/ > el.org%2Fall%2F20220609150851.23084-1- > max.oss.09%40gmail.com%2F&data=05%7C01%7Cshenwei.wang%40nxp.com% > 7C2cf40d23202c430302c908dba08add2e%7C686ea1d3bc2b4c6fa92cd99c5c301 > 635%7C0%7C0%7C638280290493921016%7CUnknown%7CTWFpbGZsb3d8eyJ > WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C > 3000%7C%7C%7C&sdata=0O%2FIytE6YbJL26je7hoxEu0ayZYs07PBjfZkBDVaC1M > %3D&reserved=0 > > Best regards, > Krzysztof
On Mon, Aug 21, 2023 at 8:22 AM Shenwei Wang <shenwei.wang@nxp.com> wrote: > > > > > -----Original Message----- > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Sent: Saturday, August 19, 2023 3:04 AM > > To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring > > <robh+dt@kernel.org> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > > <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood > > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > dl-linux-imx <linux-imx@nxp.com> > > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > > > Caution: This is an external email. Please take care when clicking links or > > opening attachments. When in doubt, report the message using the 'Report this > > email' button > > > > > > >> > > >> This needs to answer why we need this. > > >> > > >> It looks like just an abstraction layer to make regulators look like > > >> a power domain. > > >> > > > > > > Yes, it is a wrapper that allows using regulators as a power domain. > > > This removes the need to add regulator operating code in each consumer > > > device driver. As a power domain, the regulator will be managed > > > automatically by the device driver framework and PM subsystem. > > > > > > This is very useful when a device's power is controlled by a GPIO pin, > > > which currently requires using the fixed-regulator to achieve the same > > > purpose. However, the fixed-regulator approach may have to add code in the > > driver in order to use it. > > > > Why do you start discussion from zero ignoring all previous history of this > > patchset? > > > > Thank you for providing the link. After reviewing the entire thread, I still don't understand how > to proceed. What is the conclusion regarding this commonly used use case but overlooked feature > in the upstream kernel? Overlooked implies we missed and ignored it, but the same concept has been submitted twice and rejected twice. What use case cannot be supported? The detail that power-domains get handled automatically is an implementation detail in the kernel (currently). That could easily change and you'd be in the same position as with regulator supplies. We could just as easily decide to make the driver core turn on all supplies in a node. That would give you the same "feature". Why would you design your DT around implementation decisions of the OS? Rob
> -----Original Message----- > From: Rob Herring <robh+dt@kernel.org> > Sent: Monday, August 21, 2023 1:50 PM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > > Thank you for providing the link. After reviewing the entire thread, I > > still don't understand how to proceed. What is the conclusion > > regarding this commonly used use case but overlooked feature in the > upstream kernel? > > Overlooked implies we missed and ignored it, but the same concept has been > submitted twice and rejected twice. What use case cannot be supported? > No offend. :) Sorry for my poor word. To provide more context, a common use case example is using a GPIO pin as a power switch. The current implementation operates as a fixed regulator, which makes it difficult to control the on/off timing without modifying its driver. It also lacks power management support. > The detail that power-domains get handled automatically is an implementation > detail in the kernel (currently). That could easily change and you'd be in the same > position as with regulator supplies. The proposed regulator-pd driver follows the standard PD driver framework, so it for sure relies on certain kernel implementation details. If those underlying implementation details change in the future, this driver as well as other PD drivers built on the same framework would need to be updated accordingly. > We could just as easily decide to make the driver core turn on all supplies in a > node. That would give you the same "feature". Why would you design your DT > around implementation decisions of the OS? > This DT properties are proposed solely for this specific driver, not to hack the OS. This is no different than other PD drivers like gpc/scu-pd/imx93-pd. Thanks, Shenwei > Rob
On 22/08/2023 17:18, Shenwei Wang wrote: > > >> -----Original Message----- >> From: Rob Herring <robh+dt@kernel.org> >> Sent: Monday, August 21, 2023 1:50 PM >> To: Shenwei Wang <shenwei.wang@nxp.com> >> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; >> Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood >> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; >>> Thank you for providing the link. After reviewing the entire thread, I >>> still don't understand how to proceed. What is the conclusion >>> regarding this commonly used use case but overlooked feature in the >> upstream kernel? >> >> Overlooked implies we missed and ignored it, but the same concept has been >> submitted twice and rejected twice. What use case cannot be supported? >> > > No offend. :) Sorry for my poor word. To provide more context, a common use case > example is using a GPIO pin as a power switch. The current implementation operates > as a fixed regulator, which makes it difficult to control the on/off timing without modifying > its driver. So it is a problem of a driver? > It also lacks power management support. Which is not related to bindings but implementation in given driver. > >> The detail that power-domains get handled automatically is an implementation >> detail in the kernel (currently). That could easily change and you'd be in the same >> position as with regulator supplies. > > The proposed regulator-pd driver follows the standard PD driver framework, so it for sure > relies on certain kernel implementation details. If those underlying implementation details > change in the future, this driver as well as other PD drivers built on the same framework > would need to be updated accordingly. We talk about bindings which you would not be allowed to change. Thus your case would stop working... > >> We could just as easily decide to make the driver core turn on all supplies in a >> node. That would give you the same "feature". Why would you design your DT >> around implementation decisions of the OS? >> > > This DT properties are proposed solely for this specific driver, not to hack the OS. This > is no different than other PD drivers like gpc/scu-pd/imx93-pd. I am not sure if you got Rob's point, I have feelings that not. Argument that some OS implements something some way, is not an argument for a new binding, barely hardware related. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, August 22, 2023 10:26 AM > To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring > <robh+dt@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > > > No offend. :) Sorry for my poor word. To provide more context, a > > common use case example is using a GPIO pin as a power switch. The > > current implementation operates as a fixed regulator, which makes it > > difficult to control the on/off timing without modifying its driver. > > So it is a problem of a driver? > That is arguable too. The driver may assume its power is on when probed, which aligns with how the PD behaves. > > It also lacks power management support. > > Which is not related to bindings but implementation in given driver. > For those simple drivers, the default power management logic can handle power correctly without requiring any specialized implementation in the driver code. > > > >> The detail that power-domains get handled automatically is an > >> implementation detail in the kernel (currently). That could easily > >> change and you'd be in the same position as with regulator supplies. > > > > The proposed regulator-pd driver follows the standard PD driver > > framework, so it for sure relies on certain kernel implementation > > details. If those underlying implementation details change in the > > future, this driver as well as other PD drivers built on the same framework > would need to be updated accordingly. > > We talk about bindings which you would not be allowed to change. Thus your > case would stop working... > As a new driver, it has to involve some new bindings especially the compatible string. > > > >> We could just as easily decide to make the driver core turn on all > >> supplies in a node. That would give you the same "feature". Why would > >> you design your DT around implementation decisions of the OS? > >> > > > > This DT properties are proposed solely for this specific driver, not > > to hack the OS. This is no different than other PD drivers like gpc/scu- > pd/imx93-pd. > > I am not sure if you got Rob's point, I have feelings that not. Argument that > some OS implements something some way, is not an argument for a new > binding, barely hardware related. > Thank you for the clarification. The issue is that this driver is purely a software layer that wraps the regulators as a power domain. The bindings make the implementation clean and easy to understand. I don't think we should add extra complex logic inside the driver solely to avoid introducing new bindings. Thanks, Shenwei > Best regards, > Krzysztof
On 22/08/2023 17:50, Shenwei Wang wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Tuesday, August 22, 2023 10:26 AM >> To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring >> <robh+dt@kernel.org> >> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley >> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood >> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; >> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> dl-linux-imx <linux-imx@nxp.com> >> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file >>> >>> No offend. :) Sorry for my poor word. To provide more context, a >>> common use case example is using a GPIO pin as a power switch. The >>> current implementation operates as a fixed regulator, which makes it >>> difficult to control the on/off timing without modifying its driver. >> >> So it is a problem of a driver? >> > > That is arguable too. The driver may assume its power is on when probed, which > aligns with how the PD behaves. So everything in driver... no discussion about bindings. > >>> It also lacks power management support. >> >> Which is not related to bindings but implementation in given driver. >> > > For those simple drivers, the default power management logic can handle > power correctly without requiring any specialized implementation in the > driver code. You can create any drivers you wish or change existing ones. I don't see a problem here. > >>> >>>> The detail that power-domains get handled automatically is an >>>> implementation detail in the kernel (currently). That could easily >>>> change and you'd be in the same position as with regulator supplies. >>> >>> The proposed regulator-pd driver follows the standard PD driver >>> framework, so it for sure relies on certain kernel implementation >>> details. If those underlying implementation details change in the >>> future, this driver as well as other PD drivers built on the same framework >> would need to be updated accordingly. >> >> We talk about bindings which you would not be allowed to change. Thus your >> case would stop working... >> > > As a new driver, it has to involve some new bindings especially the compatible > string. I am not talking about this. I do not speak about creating new bindings, but changing them. > >>> >>>> We could just as easily decide to make the driver core turn on all >>>> supplies in a node. That would give you the same "feature". Why would >>>> you design your DT around implementation decisions of the OS? >>>> >>> >>> This DT properties are proposed solely for this specific driver, not >>> to hack the OS. This is no different than other PD drivers like gpc/scu- >> pd/imx93-pd. >> >> I am not sure if you got Rob's point, I have feelings that not. Argument that >> some OS implements something some way, is not an argument for a new >> binding, barely hardware related. >> > > Thank you for the clarification. The issue is that this driver is purely a software layer > that wraps the regulators as a power domain. The bindings make the implementation > clean and easy to understand. I don't think we should add extra complex logic inside > the driver solely to avoid introducing new bindings. Since bindings are not for software layers, I don't think we should be talking about them just to avoid introducing driver changes. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, August 22, 2023 10:58 AM > To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring > <robh+dt@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml > file > > > > > That is arguable too. The driver may assume its power is on when > > probed, which aligns with how the PD behaves. > > So everything in driver... no discussion about bindings. > That's true only when you have a PD driver for it. > > > >>> It also lacks power management support. > >> > >> Which is not related to bindings but implementation in given driver. > >> > > > > For those simple drivers, the default power management logic can > > handle power correctly without requiring any specialized > > implementation in the driver code. > > You can create any drivers you wish or change existing ones. I don't see a > problem here. > That's what this patch set does. > > > >>> > > > > As a new driver, it has to involve some new bindings especially the > > compatible string. > > I am not talking about this. I do not speak about creating new bindings, but > changing them. I'm a bit confused by your statement. The patch only creates some necessary new bindings for the new driver. It doesn't change any existing bindings. > > > >>> > > Thank you for the clarification. The issue is that this driver is > > purely a software layer that wraps the regulators as a power domain. > > The bindings make the implementation clean and easy to understand. I > > don't think we should add extra complex logic inside the driver solely to avoid > introducing new bindings. > > Since bindings are not for software layers, I don't think we should be talking > about them just to avoid introducing driver changes. > While it is a software layer, it is a driver that requires bindings. I don't think you would recommend hardcoding those properties inside the driver itself. Thanks, Shenwei > Best regards, > Krzysztof
On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com> wrote: > > Documenting the regulator power domain properties and usage examples. As Rob and Krzysztof already pointed out, I agree that this binding looks a bit questionable. Rather than adding a new DT binding, why can't we just use the existing way of describing a platform specific power-domain provider? This still looks platform specific to me. Kind regards Uffe > > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> > --- > .../bindings/power/regulator-pd.yaml | 71 +++++++++++++++++++ > 1 file changed, 71 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/regulator-pd.yaml > > diff --git a/Documentation/devicetree/bindings/power/regulator-pd.yaml b/Documentation/devicetree/bindings/power/regulator-pd.yaml > new file mode 100644 > index 000000000000..181d2fa83f8a > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/regulator-pd.yaml > @@ -0,0 +1,71 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/regulator-pd.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Regulator Power Domain > + > +maintainers: > + - Shenwei Wang <shenwei.wang@nxp.com> > + > +description: | > + This describes a power domain which manages a group of regulators. > + > +allOf: > + - $ref: power-domain.yaml# > + > +properties: > + compatible: > + items: > + - const: regulator-power-domain > + > + '#power-domain-cells': > + const: 1 > + > + regulator-number: > + minimum: 1 > + maximum: 100 > + description: The count of regulator to be managed by this power domain > + > +patternProperties: > + "regulator-[0-99]-supply$": > + description: The regulator supply phandle to be managed by this power domain > + > +required: > + - compatible > + - '#power-domain-cells' > + - regulator-number > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + reg1: regulator-1 { > + compatible = "regulator-fixed"; > + regulator-name = "REG1"; > + regulator-min-microvolt = <3000000>; > + regulator-max-microvolt = <3000000>; > + gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + }; > + > + reg2: regulator-2 { > + compatible = "regulator-fixed"; > + regulator-name = "REG2"; > + regulator-min-microvolt = <3000000>; > + regulator-max-microvolt = <3000000>; > + gpio = <&lsio_gpio4 20 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + }; > + > + power-controller { > + compatible = "regulator-power-domain"; > + #power-domain-cells = <1>; > + > + regulator-number = <2>; > + regulator-0-supply = <®1>; > + regulator-1-supply = <®2>; > + }; > -- > 2.34.1 >
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Thursday, August 24, 2023 4:27 AM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com> wrote: > > > > Documenting the regulator power domain properties and usage examples. > > As Rob and Krzysztof already pointed out, I agree that this binding looks a bit > questionable. > > Rather than adding a new DT binding, why can't we just use the existing way of > describing a platform specific power-domain provider? Can you please provide more details on how you thought we should implement this feature using the existing way? Very appreciate if you could provide a simple example. > This still looks platform specific to me. What does platform specific exactly mean here? I want to make sure I understand what you were referring to. Thanks, Shenwei > > Kind regards > Uffe > > > > > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> > > --- > > .../bindings/power/regulator-pd.yaml | 71 +++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/power/regulator-pd.yaml > > > > diff --git a/Documentation/devicetree/bindings/power/regulator-pd.yaml > > b/Documentation/devicetree/bindings/power/regulator-pd.yaml > > new file mode 100644 > > index 000000000000..181d2fa83f8a
On Thu, 24 Aug 2023 at 18:35, Shenwei Wang <shenwei.wang@nxp.com> wrote: > > > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: Thursday, August 24, 2023 4:27 AM > > To: Shenwei Wang <shenwei.wang@nxp.com> > > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > dl-linux-imx <linux-imx@nxp.com> > > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > > > Caution: This is an external email. Please take care when clicking links or > > opening attachments. When in doubt, report the message using the 'Report this > > email' button > > > > > > On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com> wrote: > > > > > > Documenting the regulator power domain properties and usage examples. > > > > As Rob and Krzysztof already pointed out, I agree that this binding looks a bit > > questionable. > > > > Rather than adding a new DT binding, why can't we just use the existing way of > > describing a platform specific power-domain provider? > > Can you please provide more details on how you thought we should implement this > feature using the existing way? Very appreciate if you could provide a simple example. > > > This still looks platform specific to me. > > What does platform specific exactly mean here? I want to make sure I understand > what you were referring to. There are plenty of examples of how a platform specific genpd provider looks in DT. You may have a look a imx platforms for example. git grep "#power-domain-cells" arch/arm/boot/dts/nxp/imx The genpd provider then needs to be a consumer of the resources it needs. In this case a couple of regulators it seems like. [...] Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Friday, August 25, 2023 7:25 AM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > On Thu, 24 Aug 2023 at 18:35, Shenwei Wang <shenwei.wang@nxp.com> wrote: > > > > > > > > > -----Original Message----- > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > Sent: Thursday, August 24, 2023 4:27 AM > > > To: Shenwei Wang <shenwei.wang@nxp.com> > > > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > > > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark > > > Brown <broonie@kernel.org>; imx@lists.linux.dev; > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > > dl-linux-imx <linux-imx@nxp.com> > > > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd > > > yaml file > > > > > > Caution: This is an external email. Please take care when clicking > > > links or opening attachments. When in doubt, report the message > > > using the 'Report this email' button > > > > > > > > > On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com> > wrote: > > > > > > > > Documenting the regulator power domain properties and usage examples. > > > > > > As Rob and Krzysztof already pointed out, I agree that this binding > > > looks a bit questionable. > > > > > > Rather than adding a new DT binding, why can't we just use the > > > existing way of describing a platform specific power-domain provider? > > > > Can you please provide more details on how you thought we should > > implement this feature using the existing way? Very appreciate if you could > provide a simple example. > > > > > This still looks platform specific to me. > > > > What does platform specific exactly mean here? I want to make sure I > > understand what you were referring to. > > There are plenty of examples of how a platform specific genpd provider looks in > DT. You may have a look a imx platforms for example. > > git grep "#power-domain-cells" arch/arm/boot/dts/nxp/imx > > The genpd provider then needs to be a consumer of the resources it needs. In > this case a couple of regulators it seems like. > If I understood your reply correctly, it seems that the current implementation of regulator-pd is what you have described. Please correct me if I'm mistaken. The following are the diff of scu-pd and this regulator-pd. power-controller { power-controller { compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd"; | compatible = "regulator-power-domain"; #power-domain-cells = <1>; #power-domain-cells = <1>; > > regulator-number = <2>; > regulator-0-supply = <®1>; > regulator-1-supply = <®2>; }; }; Are you suggesting to move the regulator-pd to the imx directory and add a company prefix to the compatible string? Thanks, Shenwei > [...] > > Kind regards > Uffe
On 25/08/2023 17:44, Shenwei Wang wrote: >> >> The genpd provider then needs to be a consumer of the resources it needs. In >> this case a couple of regulators it seems like. >> > > If I understood your reply correctly, it seems that the current implementation of > regulator-pd is what you have described. Please correct me if I'm mistaken. > > The following are the diff of scu-pd and this regulator-pd. > > power-controller { power-controller { > compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd"; | compatible = "regulator-power-domain"; > #power-domain-cells = <1>; #power-domain-cells = <1>; > > > > regulator-number = <2>; > > regulator-0-supply = <®1>; > > regulator-1-supply = <®2>; > }; }; > > Are you suggesting to move the regulator-pd to the imx directory and add a company prefix > to the compatible string? There is no such part of iMX processor as such regulator-power-domain, so I don't recommend that approach. DTS nodes represent hardware, not your SW layers. Best regards, Krzysztof
On Sat, 26 Aug 2023 at 19:31, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 25/08/2023 17:44, Shenwei Wang wrote: > >> > >> The genpd provider then needs to be a consumer of the resources it needs. In > >> this case a couple of regulators it seems like. > >> > > > > If I understood your reply correctly, it seems that the current implementation of > > regulator-pd is what you have described. Please correct me if I'm mistaken. > > > > The following are the diff of scu-pd and this regulator-pd. > > > > power-controller { power-controller { > > compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd"; | compatible = "regulator-power-domain"; > > #power-domain-cells = <1>; #power-domain-cells = <1>; > > > > > > regulator-number = <2>; > > > regulator-0-supply = <®1>; > > > regulator-1-supply = <®2>; > > }; }; > > > > Are you suggesting to move the regulator-pd to the imx directory and add a company prefix > > to the compatible string? > > There is no such part of iMX processor as such regulator-power-domain, > so I don't recommend that approach. DTS nodes represent hardware, not > your SW layers. I would agree if this was pure SW layers, but I don't think it is. At least, if I have understood the earlier discussions correctly [1], there are certainly one or more power-domains here. The power-domains just happen to be powered through something that can be modelled as a regular regulator(s). No? Note that, we already have other power-domains that are consumers of regulators too. Kind regards Uffe [1] https://lore.kernel.org/all/20220609150851.23084-1-max.oss.09@gmail.com/
On 28/08/2023 11:59, Ulf Hansson wrote: > On Sat, 26 Aug 2023 at 19:31, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 25/08/2023 17:44, Shenwei Wang wrote: >>>> >>>> The genpd provider then needs to be a consumer of the resources it needs. In >>>> this case a couple of regulators it seems like. >>>> >>> >>> If I understood your reply correctly, it seems that the current implementation of >>> regulator-pd is what you have described. Please correct me if I'm mistaken. >>> >>> The following are the diff of scu-pd and this regulator-pd. >>> >>> power-controller { power-controller { >>> compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd"; | compatible = "regulator-power-domain"; >>> #power-domain-cells = <1>; #power-domain-cells = <1>; >>> > >>> > regulator-number = <2>; >>> > regulator-0-supply = <®1>; >>> > regulator-1-supply = <®2>; >>> }; }; >>> >>> Are you suggesting to move the regulator-pd to the imx directory and add a company prefix >>> to the compatible string? >> >> There is no such part of iMX processor as such regulator-power-domain, >> so I don't recommend that approach. DTS nodes represent hardware, not >> your SW layers. > > I would agree if this was pure SW layers, but I don't think it is. At > least, if I have understood the earlier discussions correctly [1], > there are certainly one or more power-domains here. The power-domains > just happen to be powered through something that can be modelled as a > regular regulator(s). No? No. It was for controlling power of multiple devices, supplied by multiple different or similar regulators, where Linux drivers for these devices (so not even all drivers...) do not have regulator control. The bindings for these devices allow power-domains, but not regulator. There are no multiple power domains in the problem. Even term "power domain" is questionable here, because we tend to look power domain as part of SoC. Here it is some selected part of the circuitry, like few totally independent devices which share purpose and power rails. But more important is my first paragraph - this is purely to avoid adding regulators to these devices. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Saturday, August 26, 2023 12:32 PM > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson > <ulf.hansson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > > > > > regulator-number = <2>; > > > regulator-0-supply = <®1>; > > > regulator-1-supply = <®2>; > > }; }; > > > > Are you suggesting to move the regulator-pd to the imx directory and > > add a company prefix to the compatible string? > > There is no such part of iMX processor as such regulator-power-domain, so I > don't recommend that approach. DTS nodes represent hardware, not your SW > layers. > That's not always the case, as we do sometimes need a virtual device. As an example, the "regulator-fixed" acts as a software abstraction layer to create virtual regulator devices by interfacing with the underlying GPIO drivers. Similarly, "regulator-pd" provides a software abstraction layer for virtual PD devices built on top of existing regulator drivers. When looking at the conceptual purpose, regulator-fixed and regulator-pd are comparable in that they both offer software abstraction layers for virtual devices." Thanks, Shenwei > Best regards, > Krzysztof
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Monday, August 28, 2023 4:59 AM > To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Cc: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > > If I understood your reply correctly, it seems that the current > > > implementation of regulator-pd is what you have described. Please correct > me if I'm mistaken. > > > > > > The following are the diff of scu-pd and this regulator-pd. > > > > > > power-controller { power-controller { > > > compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd"; | compatible > = "regulator-power-domain"; > > > #power-domain-cells = <1>; #power-domain-cells = > <1>; > > > > > > > > regulator-number = <2>; > > > > regulator-0-supply = <®1>; > > > > regulator-1-supply = <®2>; > > > }; }; > > > > > > Are you suggesting to move the regulator-pd to the imx directory and > > > add a company prefix to the compatible string? > > > > There is no such part of iMX processor as such regulator-power-domain, > > so I don't recommend that approach. DTS nodes represent hardware, not > > your SW layers. > > I would agree if this was pure SW layers, but I don't think it is. At least, if I have > understood the earlier discussions correctly [1], there are certainly one or more > power-domains here. The power-domains just happen to be powered through > something that can be modelled as a regular regulator(s). No? > > Note that, we already have other power-domains that are consumers of > regulators too. Can you please point me an example? I would be happy to use the existing implementation if it fits this use case. Thanks, Shenwei > > Kind regards > Uffe > > [1] > https://lore.kern/ > el.org%2Fall%2F20220609150851.23084-1- > max.oss.09%40gmail.com%2F&data=05%7C01%7Cshenwei.wang%40nxp.com% > 7Cebeaa5d30be14033509f08dba7ad7dbd%7C686ea1d3bc2b4c6fa92cd99c5c30 > 1635%7C0%7C0%7C638288135797275723%7CUnknown%7CTWFpbGZsb3d8eyJ > WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C > 3000%7C%7C%7C&sdata=re2A5wbng9haHoMKTbPICh%2Fh9VJVsG66H9jybd2fN > k8%3D&reserved=0
On 28/08/2023 16:04, Shenwei Wang wrote: >>> Are you suggesting to move the regulator-pd to the imx directory and >>> add a company prefix to the compatible string? >> >> There is no such part of iMX processor as such regulator-power-domain, so I >> don't recommend that approach. DTS nodes represent hardware, not your SW >> layers. >> > > That's not always the case, as we do sometimes need a virtual device. > As an example, the "regulator-fixed" acts as a software abstraction layer to create virtual regulator > devices by interfacing with the underlying GPIO drivers. Not true. This is a real regulator device. Real hardware on the board. You can even see and touch it. > Similarly, "regulator-pd" provides a software abstraction layer for virtual PD devices built on > top of existing regulator drivers. This is not related to regulator-fixed at all. > When looking at the conceptual purpose, regulator-fixed and regulator-pd are comparable in > that they both offer software abstraction layers for virtual devices." No. regulator-fixed is a real device. Yours is not. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, August 28, 2023 12:11 PM > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson > <ulf.hansson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > On 28/08/2023 16:04, Shenwei Wang wrote: > > >>> Are you suggesting to move the regulator-pd to the imx directory and > >>> add a company prefix to the compatible string? > >> > >> There is no such part of iMX processor as such > >> regulator-power-domain, so I don't recommend that approach. DTS nodes > >> represent hardware, not your SW layers. > >> > > > > That's not always the case, as we do sometimes need a virtual device. > > As an example, the "regulator-fixed" acts as a software abstraction > > layer to create virtual regulator devices by interfacing with the underlying > GPIO drivers. > > Not true. This is a real regulator device. Real hardware on the board. > You can even see and touch it. > The physical hardware component is the GPIO pin, which is what you can only touch. The regulator functions virtually through software layer above of the GPIO driver. While we may call it a "regulator" or whatever else, this cannot obscure the fact that the underlying hardware is just a GPIO pin being used in a specialized way. Thanks, Shenwei > > Similarly, "regulator-pd" provides a software abstraction layer for > > virtual PD devices built on top of existing regulator drivers. > > This is not related to regulator-fixed at all. > > > When looking at the conceptual purpose, regulator-fixed and > > regulator-pd are comparable in that they both offer software abstraction > layers for virtual devices." > > No. regulator-fixed is a real device. Yours is not. > > > Best regards, > Krzysztof
On 28/08/2023 20:39, Shenwei Wang wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Monday, August 28, 2023 12:11 PM >> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson >> <ulf.hansson@linaro.org> >> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; >> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; >> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> dl-linux-imx <linux-imx@nxp.com> >> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file >> On 28/08/2023 16:04, Shenwei Wang wrote: >> >>>>> Are you suggesting to move the regulator-pd to the imx directory and >>>>> add a company prefix to the compatible string? >>>> >>>> There is no such part of iMX processor as such >>>> regulator-power-domain, so I don't recommend that approach. DTS nodes >>>> represent hardware, not your SW layers. >>>> >>> >>> That's not always the case, as we do sometimes need a virtual device. >>> As an example, the "regulator-fixed" acts as a software abstraction >>> layer to create virtual regulator devices by interfacing with the underlying >> GPIO drivers. >> >> Not true. This is a real regulator device. Real hardware on the board. >> You can even see and touch it. >> > > The physical hardware component is the GPIO pin, which is what you can only touch. No. The regulator is the chip. > The regulator functions virtually through software layer above of the GPIO driver. While > we may call it a "regulator" or whatever else, this cannot obscure the fact that the underlying > hardware is just a GPIO pin being used in a specialized way. The regulator is some tiny little box, you can touch and called ti,tps51632 or similar. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, August 28, 2023 1:42 PM > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson > <ulf.hansson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > >>>>> Are you suggesting to move the regulator-pd to the imx directory > >>>>> and add a company prefix to the compatible string? > >>>> > >>>> There is no such part of iMX processor as such > >>>> regulator-power-domain, so I don't recommend that approach. DTS > >>>> nodes represent hardware, not your SW layers. > >>>> > >>> > >>> That's not always the case, as we do sometimes need a virtual device. > >>> As an example, the "regulator-fixed" acts as a software abstraction > >>> layer to create virtual regulator devices by interfacing with the > >>> underlying > >> GPIO drivers. > >> > >> Not true. This is a real regulator device. Real hardware on the board. > >> You can even see and touch it. > >> > > > > The physical hardware component is the GPIO pin, which is what you can only > touch. > > No. The regulator is the chip. > In the definition of dts node below, where is the chip? The real hardware is just a GPIO Pin. reg1: regulator-1 { compatible = "regulator-fixed"; regulator-name = "REG1"; regulator-min-microvolt = <3000000>; regulator-max-microvolt = <3000000>; gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; enable-active-high; }; > > The regulator functions virtually through software layer above of the > > GPIO driver. While we may call it a "regulator" or whatever else, this > > cannot obscure the fact that the underlying hardware is just a GPIO pin being > used in a specialized way. > > The regulator is some tiny little box, you can touch and called > ti,tps51632 or similar. > We are talking about the specific "regulator-fixed" driver, why did you bring up "ti,tps51632" here? Thanks, Shenwei > > > Best regards, > Krzysztof
On 28/08/2023 20:50, Shenwei Wang wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Monday, August 28, 2023 1:42 PM >> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson >> <ulf.hansson@linaro.org> >> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; >> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; >> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> dl-linux-imx <linux-imx@nxp.com> >> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file >>>>>>> Are you suggesting to move the regulator-pd to the imx directory >>>>>>> and add a company prefix to the compatible string? >>>>>> >>>>>> There is no such part of iMX processor as such >>>>>> regulator-power-domain, so I don't recommend that approach. DTS >>>>>> nodes represent hardware, not your SW layers. >>>>>> >>>>> >>>>> That's not always the case, as we do sometimes need a virtual device. >>>>> As an example, the "regulator-fixed" acts as a software abstraction >>>>> layer to create virtual regulator devices by interfacing with the >>>>> underlying >>>> GPIO drivers. >>>> >>>> Not true. This is a real regulator device. Real hardware on the board. >>>> You can even see and touch it. >>>> >>> >>> The physical hardware component is the GPIO pin, which is what you can only >> touch. >> >> No. The regulator is the chip. >> > > In the definition of dts node below, where is the chip? The real hardware is just a GPIO Pin. > reg1: regulator-1 { > compatible = "regulator-fixed"; > regulator-name = "REG1"; > regulator-min-microvolt = <3000000>; > regulator-max-microvolt = <3000000>; > gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; > enable-active-high; > }; There is a chip. This is the chip. If you have there only GPIO pin, then your DTS is just wrong. Drop it. If you learn from wrong DTS, then sure, power-domain-regulator seems like great idea... > >>> The regulator functions virtually through software layer above of the >>> GPIO driver. While we may call it a "regulator" or whatever else, this >>> cannot obscure the fact that the underlying hardware is just a GPIO pin being >> used in a specialized way. >> >> The regulator is some tiny little box, you can touch and called >> ti,tps51632 or similar. >> > > We are talking about the specific "regulator-fixed" driver, why did you bring up "ti,tps51632" here? Just an example. Can be TPS123098. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, August 28, 2023 1:53 PM > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson > <ulf.hansson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > >>>>>>> Are you suggesting to move the regulator-pd to the imx directory > >>>>>>> and add a company prefix to the compatible string? > >>>>>> > >>>>>> There is no such part of iMX processor as such > >>>>>> regulator-power-domain, so I don't recommend that approach. DTS > >>>>>> nodes represent hardware, not your SW layers. > >>>>>> > >>>>> > >>>>> That's not always the case, as we do sometimes need a virtual device. > >>>>> As an example, the "regulator-fixed" acts as a software > >>>>> abstraction layer to create virtual regulator devices by > >>>>> interfacing with the underlying > >>>> GPIO drivers. > >>>> > >>>> Not true. This is a real regulator device. Real hardware on the board. > >>>> You can even see and touch it. > >>>> > >>> > >>> The physical hardware component is the GPIO pin, which is what you > >>> can only > >> touch. > >> > >> No. The regulator is the chip. > >> > > > > In the definition of dts node below, where is the chip? The real hardware is just > a GPIO Pin. > > reg1: regulator-1 { > > compatible = "regulator-fixed"; > > regulator-name = "REG1"; > > regulator-min-microvolt = <3000000>; > > regulator-max-microvolt = <3000000>; > > gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; > > enable-active-high; > > }; > > There is a chip. This is the chip. If you have there only GPIO pin, then your DTS is > just wrong. Drop it. If you learn from wrong DTS, then sure, power-domain- > regulator seems like great idea... > When you talk about the chip, can you please be more specific? Regarding the dts node, how about the example in the fixed-regulator.yaml under the bindings directory. reg_1v8: regulator-1v8 { compatible = "regulator-fixed"; regulator-name = "1v8"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; gpio = <&gpio1 16 0>; startup-delay-us = <70000>; enable-active-high; regulator-boot-on; gpio-open-drain; vin-supply = <&parent_reg>; }; If you take a look at the fixed regulator driver (fixed.c), I don't think you'll find anything related to a hardware component (chip) other than the GPIO Pin. Thanks, Shenwei > > > >>> The regulator functions virtually through software layer above of > >>> the GPIO driver. While we may call it a "regulator" or whatever > >>> else, this cannot obscure the fact that the underlying hardware is > >>> just a GPIO pin being > >> used in a specialized way. > >> > >> The regulator is some tiny little box, you can touch and called > >> ti,tps51632 or similar. > >> > > > > We are talking about the specific "regulator-fixed" driver, why did you bring up > "ti,tps51632" here? > > Just an example. Can be TPS123098. > > Best regards, > Krzysztof
On 28/08/2023 21:09, Shenwei Wang wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Monday, August 28, 2023 1:53 PM >> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson >> <ulf.hansson@linaro.org> >> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; >> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; >> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> dl-linux-imx <linux-imx@nxp.com> >> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file >>>>>>>>> Are you suggesting to move the regulator-pd to the imx directory >>>>>>>>> and add a company prefix to the compatible string? >>>>>>>> >>>>>>>> There is no such part of iMX processor as such >>>>>>>> regulator-power-domain, so I don't recommend that approach. DTS >>>>>>>> nodes represent hardware, not your SW layers. >>>>>>>> >>>>>>> >>>>>>> That's not always the case, as we do sometimes need a virtual device. >>>>>>> As an example, the "regulator-fixed" acts as a software >>>>>>> abstraction layer to create virtual regulator devices by >>>>>>> interfacing with the underlying >>>>>> GPIO drivers. >>>>>> >>>>>> Not true. This is a real regulator device. Real hardware on the board. >>>>>> You can even see and touch it. >>>>>> >>>>> >>>>> The physical hardware component is the GPIO pin, which is what you >>>>> can only >>>> touch. >>>> >>>> No. The regulator is the chip. >>>> >>> >>> In the definition of dts node below, where is the chip? The real hardware is just >> a GPIO Pin. >>> reg1: regulator-1 { >>> compatible = "regulator-fixed"; >>> regulator-name = "REG1"; >>> regulator-min-microvolt = <3000000>; >>> regulator-max-microvolt = <3000000>; >>> gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; >>> enable-active-high; >>> }; >> >> There is a chip. This is the chip. If you have there only GPIO pin, then your DTS is >> just wrong. Drop it. If you learn from wrong DTS, then sure, power-domain- >> regulator seems like great idea... >> > > When you talk about the chip, can you please be more specific? What to say more? The device node you quoted above is the regulator. You brought specific example and now claim this is not a regulator, but just GPIO. Please fix your DTS. > > Regarding the dts node, how about the example in the fixed-regulator.yaml under the bindings directory. That's an example, how is it related to anything? > > reg_1v8: regulator-1v8 { > compatible = "regulator-fixed"; > regulator-name = "1v8"; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > gpio = <&gpio1 16 0>; > startup-delay-us = <70000>; > enable-active-high; > regulator-boot-on; > gpio-open-drain; > vin-supply = <&parent_reg>; > }; > > If you take a look at the fixed regulator driver (fixed.c), I don't think you'll find anything related to a hardware > component (chip) other than the GPIO Pin. That's a driver. How is it related to this discussion? Bindings are not about drivers. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, August 28, 2023 2:12 PM > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson > <ulf.hansson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > >>>>>>>>> Are you suggesting to move the regulator-pd to the imx > >>>>>>>>> directory and add a company prefix to the compatible string? > >>>>>>>> > >>>>>>>> There is no such part of iMX processor as such > >>>>>>>> regulator-power-domain, so I don't recommend that approach. DTS > >>>>>>>> nodes represent hardware, not your SW layers. > >>>>>>>> > >>>>>>> > >>>>>>> That's not always the case, as we do sometimes need a virtual device. > >>>>>>> As an example, the "regulator-fixed" acts as a software > >>>>>>> abstraction layer to create virtual regulator devices by > >>>>>>> interfacing with the underlying > >>>>>> GPIO drivers. > >>>>>> > >>>>>> Not true. This is a real regulator device. Real hardware on the board. > >>>>>> You can even see and touch it. > >>>>>> > >>>>> > >>>>> The physical hardware component is the GPIO pin, which is what you > >>>>> can only > >>>> touch. > >>>> > >>>> No. The regulator is the chip. > >>>> > >>> > >>> In the definition of dts node below, where is the chip? The real > >>> hardware is just > >> a GPIO Pin. > >>> reg1: regulator-1 { > >>> compatible = "regulator-fixed"; > >>> regulator-name = "REG1"; > >>> regulator-min-microvolt = <3000000>; > >>> regulator-max-microvolt = <3000000>; > >>> gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; > >>> enable-active-high; > >>> }; > >> > >> There is a chip. This is the chip. If you have there only GPIO pin, > >> then your DTS is just wrong. Drop it. If you learn from wrong DTS, > >> then sure, power-domain- regulator seems like great idea... > >> > > > > When you talk about the chip, can you please be more specific? > > What to say more? The device node you quoted above is the regulator. You > brought specific example and now claim this is not a regulator, but just GPIO. > Please fix your DTS. > The fixed-regulator is a virtual regulator driver that uses the GPIO pin. You claimed this as a hardware chip. The regulator-pd driver also uses the same GPIO pin. You now claimed this as a software layer. What's your standard? Thanks, Shenwei > > > > Regarding the dts node, how about the example in the fixed-regulator.yaml > under the bindings directory. > > That's an example, how is it related to anything? > > > > > reg_1v8: regulator-1v8 { > > compatible = "regulator-fixed"; > > regulator-name = "1v8"; > > regulator-min-microvolt = <1800000>; > > regulator-max-microvolt = <1800000>; > > gpio = <&gpio1 16 0>; > > startup-delay-us = <70000>; > > enable-active-high; > > regulator-boot-on; > > gpio-open-drain; > > vin-supply = <&parent_reg>; > > }; > > > > If you take a look at the fixed regulator driver (fixed.c), I don't > > think you'll find anything related to a hardware component (chip) other than > the GPIO Pin. > > That's a driver. How is it related to this discussion? Bindings are not about > drivers. > > > Best regards, > Krzysztof
On 28/08/2023 21:23, Shenwei Wang wrote: >>>>> reg1: regulator-1 { >>>>> compatible = "regulator-fixed"; >>>>> regulator-name = "REG1"; >>>>> regulator-min-microvolt = <3000000>; >>>>> regulator-max-microvolt = <3000000>; >>>>> gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; >>>>> enable-active-high; >>>>> }; >>>> >>>> There is a chip. This is the chip. If you have there only GPIO pin, >>>> then your DTS is just wrong. Drop it. If you learn from wrong DTS, >>>> then sure, power-domain- regulator seems like great idea... >>>> >>> >>> When you talk about the chip, can you please be more specific? >> >> What to say more? The device node you quoted above is the regulator. You >> brought specific example and now claim this is not a regulator, but just GPIO. >> Please fix your DTS. >> > > The fixed-regulator is a virtual regulator driver that uses the GPIO pin. We do not talk about drivers but bindings and DTS. Why do you bring again drivers, all the time? > You claimed this > as a hardware chip. ??? Sorry, this is getting boring. The DTS-snippet is a hardware chip. If it is not, then drop it from your DTS. I insist. Srsly, third time I insist. > > The regulator-pd driver also uses the same GPIO pin. Again, what is with the drivers? Can you stop bringing it to the discussion? > You now claimed this as a software layer. ??? > > What's your standard? I don't think there is anything more to say. You clearly do not understand what is DTS, schematics and how the actual hardware looks like. I am not going to respond more to this patchset (which is a clear NAK just in case). Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Monday, August 28, 2023 2:31 PM > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson > <ulf.hansson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml > > The fixed-regulator is a virtual regulator driver that uses the GPIO pin. > > We do not talk about drivers but bindings and DTS. Why do you bring again > drivers, all the time? > > > You claimed this > > as a hardware chip. > > ??? Sorry, this is getting boring. The DTS-snippet is a hardware chip. > If it is not, then drop it from your DTS. I insist. Srsly, third time I insist. > > > > > > The regulator-pd driver also uses the same GPIO pin. > > Again, what is with the drivers? Can you stop bringing it to the discussion? > I have to admit you have a real talent for debate. Thanks, Shenwei > > You now claimed this as a software layer. > > ??? > > > > > What's your standard? > > I don't think there is anything more to say. You clearly do not understand what is > DTS, schematics and how the actual hardware looks like. > > I am not going to respond more to this patchset (which is a clear NAK just in > case). > > Best regards, > Krzysztof
On Mon, Aug 28, 2023 at 2:49 PM Shenwei Wang <shenwei.wang@nxp.com> wrote: > > > > > -----Original Message----- > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Sent: Monday, August 28, 2023 2:31 PM > > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson > > <ulf.hansson@linaro.org> > > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > dl-linux-imx <linux-imx@nxp.com> > > Subject: Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml > > > The fixed-regulator is a virtual regulator driver that uses the GPIO pin. > > > > We do not talk about drivers but bindings and DTS. Why do you bring again > > drivers, all the time? > > > > > You claimed this > > > as a hardware chip. > > > > ??? Sorry, this is getting boring. The DTS-snippet is a hardware chip. > > If it is not, then drop it from your DTS. I insist. Srsly, third time I insist. > > > > > > > > > > The regulator-pd driver also uses the same GPIO pin. > > > > Again, what is with the drivers? Can you stop bringing it to the discussion? > > > > I have to admit you have a real talent for debate. It takes 2... You've gotten feedback from multiple people that your proposal is not going to be accepted. The prior attempt of the same thing had similar feedback from even more people. Please go re-read the responses until you understand. For fixed-regulator, I can tell you very easily what the h/w looks like: Vfix---|gate|---Vfix-gated | GPIO--------| 'gate' here may be a chip or discrete transistor. That's a very common board level component. If you want to discuss this any further, describe the h/w in terms of simplified schematics. Otherwise, there is nothing more to discuss. Rob
> -----Original Message----- > From: Rob Herring <robh+dt@kernel.org> > Sent: Monday, August 28, 2023 4:14 PM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Ulf Hansson > <ulf.hansson@linaro.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file > > > > The fixed-regulator is a virtual regulator driver that uses the GPIO pin. > > > > > > We do not talk about drivers but bindings and DTS. Why do you bring > > > again drivers, all the time? > > > > > > > You claimed this > > > > as a hardware chip. > > > > > > ??? Sorry, this is getting boring. The DTS-snippet is a hardware chip. > > > If it is not, then drop it from your DTS. I insist. Srsly, third time I insist. > > > > > > > > > > > > > > The regulator-pd driver also uses the same GPIO pin. > > > > > > Again, what is with the drivers? Can you stop bringing it to the discussion? > > > > > > > I have to admit you have a real talent for debate. > > It takes 2... > > You've gotten feedback from multiple people that your proposal is not going to > be accepted. The prior attempt of the same thing had similar feedback from > even more people. Please go re-read the responses until you understand. > > For fixed-regulator, I can tell you very easily what the h/w looks like: > > Vfix---|gate|---Vfix-gated > | > GPIO--------| > > 'gate' here may be a chip or discrete transistor. That's a very common board > level component. > The difference is in how we model the hardware. In your example, you model the GPIO as a simple switch to fit the fixed regulator use case. However, we could also model the same GPIO as a power domain if we consider the device connected to it. This allows for more nuanced hardware modeling based on the context and components involved. Regulator-1 -+-> [Device A] This give you one regulator(via GPIO Pin) and one power domain (Device A). The following are the example diagram given by the power domain overview doc: Regulator-1 -+-> Regulator-2 -+-> [Consumer A] | +-> [Consumer B] This gives us two regulators and two power domains: - Domain 1: Regulator-2, Consumer B. - Domain 2: Consumer A. Thanks, Shenwei > If you want to discuss this any further, describe the h/w in terms of simplified > schematics. Otherwise, there is nothing more to discuss. > > Rob
diff --git a/Documentation/devicetree/bindings/power/regulator-pd.yaml b/Documentation/devicetree/bindings/power/regulator-pd.yaml new file mode 100644 index 000000000000..181d2fa83f8a --- /dev/null +++ b/Documentation/devicetree/bindings/power/regulator-pd.yaml @@ -0,0 +1,71 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/regulator-pd.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Regulator Power Domain + +maintainers: + - Shenwei Wang <shenwei.wang@nxp.com> + +description: | + This describes a power domain which manages a group of regulators. + +allOf: + - $ref: power-domain.yaml# + +properties: + compatible: + items: + - const: regulator-power-domain + + '#power-domain-cells': + const: 1 + + regulator-number: + minimum: 1 + maximum: 100 + description: The count of regulator to be managed by this power domain + +patternProperties: + "regulator-[0-99]-supply$": + description: The regulator supply phandle to be managed by this power domain + +required: + - compatible + - '#power-domain-cells' + - regulator-number + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + reg1: regulator-1 { + compatible = "regulator-fixed"; + regulator-name = "REG1"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3000000>; + gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>; + enable-active-high; + }; + + reg2: regulator-2 { + compatible = "regulator-fixed"; + regulator-name = "REG2"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3000000>; + gpio = <&lsio_gpio4 20 GPIO_ACTIVE_HIGH>; + enable-active-high; + }; + + power-controller { + compatible = "regulator-power-domain"; + #power-domain-cells = <1>; + + regulator-number = <2>; + regulator-0-supply = <®1>; + regulator-1-supply = <®2>; + };
Documenting the regulator power domain properties and usage examples. Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> --- .../bindings/power/regulator-pd.yaml | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/regulator-pd.yaml