| Message ID | 20230818153446.1076027-1-shenwei.wang@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: power: Add regulator-pd yaml file | expand |
> -----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
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
> -----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
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: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
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