devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] ARM: st: add new compatible for PWR regulators on STM32MP13
@ 2024-05-13  9:56 Patrick Delaunay
  2024-05-13  9:56 ` [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13 Patrick Delaunay
  0 siblings, 1 reply; 11+ messages in thread
From: Patrick Delaunay @ 2024-05-13  9:56 UTC (permalink / raw)
  To: Alexandre TORGUE, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet
  Cc: Marek Vasut, Patrick Delaunay, devicetree, linux-arm-kernel,
	linux-kernel, linux-stm32


This patchset adds the new PWR regulators compatible for STM32MP13:
"st,stm32mp13-pwr-reg".

As this node is just introduced by [1] and it is is not used by any
board in Linux,  it is the good time to introduced this compatible
and update the STM32MP13 SoC dtsi without ABI break.

A new compatible is needed as the content of the PWR_CR3 register,
used by this driver change with new bits on STM32MP13 for SD IO domain:
- bit 23: VDDSD2VALID
- bit 22: VDDSD1VALID
- bit 16: VDDSD2RDY
- bit 15: VDDSD2EN
- bit 14: VDDSD1RDY
- bit 13: VDDSD1EN

I will push a update on STM32MP13 SoC dtsi if this new compatible
is accepted to preserve the bisectability.

[1] commit f798f7079233 ("ARM: dts: stm32: add PWR regulators support on stm32mp131")
    https://lore.kernel.org/linux-arm-kernel/b89d0531-067f-4356-91b0-ed7434cee3d7@foss.st.com/


Changes in v3:
- Replace oneOf/const by enum; solve the V2 issues for dt_binding_check

Changes in v2:
- Add new compatible for STM32MP13 and change title after Rob remarks
  V1: "ARM: st: use a correct pwr compatible for stm32mp15"

Patrick Delaunay (2):
  dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for
    STM32MP13
  regulator: stm32-pwr: add support of STM32MP13

 .../devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml    | 4 +++-
 drivers/regulator/stm32-pwr.c                                 | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-13  9:56 [PATCH v3 0/2] ARM: st: add new compatible for PWR regulators on STM32MP13 Patrick Delaunay
@ 2024-05-13  9:56 ` Patrick Delaunay
  2024-05-13 14:34   ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Patrick Delaunay @ 2024-05-13  9:56 UTC (permalink / raw)
  To: Alexandre TORGUE, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet
  Cc: Marek Vasut, Patrick Delaunay, devicetree, linux-arm-kernel,
	linux-kernel, linux-stm32

Add new compatible "st,stm32mp13-pwr-reg" for STM32MP13 SoC family.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

Changes in v3:
- Replace oneOf/const by enum; solve the V2 issues for dt_binding_check

Changes in v2:
- update for Rob review, only add compatible for STM32MP13 family

 .../devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml    | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
index c9586d277f41..c766f0a15a31 100644
--- a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
+++ b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
@@ -11,7 +11,9 @@ maintainers:
 
 properties:
   compatible:
-    const: st,stm32mp1,pwr-reg
+    enum:
+      - st,stm32mp1,pwr-reg
+      - st,stm32mp13-pwr-reg
 
   reg:
     maxItems: 1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-13  9:56 ` [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13 Patrick Delaunay
@ 2024-05-13 14:34   ` Marek Vasut
  2024-05-13 15:16     ` Conor Dooley
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2024-05-13 14:34 UTC (permalink / raw)
  To: Patrick Delaunay, Alexandre TORGUE, Liam Girdwood, Mark Brown,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet
  Cc: devicetree, linux-arm-kernel, linux-kernel, linux-stm32

On 5/13/24 11:56 AM, Patrick Delaunay wrote:
> Add new compatible "st,stm32mp13-pwr-reg" for STM32MP13 SoC family.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
> Changes in v3:
> - Replace oneOf/const by enum; solve the V2 issues for dt_binding_check
> 
> Changes in v2:
> - update for Rob review, only add compatible for STM32MP13 family
> 
>   .../devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml    | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> index c9586d277f41..c766f0a15a31 100644
> --- a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> +++ b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> @@ -11,7 +11,9 @@ maintainers:
>   
>   properties:
>     compatible:
> -    const: st,stm32mp1,pwr-reg
> +    enum:
> +      - st,stm32mp1,pwr-reg
> +      - st,stm32mp13-pwr-reg

Should the st,stm32mp1,pwr-reg be treated as fallback compatible for 
st,stm32mp13-pwr-reg or not ?

In other words, should the DT contain:
compatible = "st,stm32mp13-pwr-reg", "st,stm32mp1,pwr-reg";
or
compatible = "st,stm32mp13-pwr-reg";
? Which one is preferable ?

I think the former one, since the MP13 PWR block could also be operated 
by older MP1(5) PWR block driver(s) without any adverse effects, except 
the SD IO domain configuration won't be available, right ?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-13 14:34   ` Marek Vasut
@ 2024-05-13 15:16     ` Conor Dooley
  2024-05-13 18:02       ` Patrick DELAUNAY
  0 siblings, 1 reply; 11+ messages in thread
From: Conor Dooley @ 2024-05-13 15:16 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Patrick Delaunay, Alexandre TORGUE, Liam Girdwood, Mark Brown,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

On Mon, May 13, 2024 at 04:34:20PM +0200, Marek Vasut wrote:
> On 5/13/24 11:56 AM, Patrick Delaunay wrote:
> > Add new compatible "st,stm32mp13-pwr-reg" for STM32MP13 SoC family.
> > 
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > ---
> > 
> > Changes in v3:
> > - Replace oneOf/const by enum; solve the V2 issues for dt_binding_check
> > 
> > Changes in v2:
> > - update for Rob review, only add compatible for STM32MP13 family
> > 
> >   .../devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml    | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> > index c9586d277f41..c766f0a15a31 100644
> > --- a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> > +++ b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> > @@ -11,7 +11,9 @@ maintainers:
> >   properties:
> >     compatible:
> > -    const: st,stm32mp1,pwr-reg
> > +    enum:
> > +      - st,stm32mp1,pwr-reg
> > +      - st,stm32mp13-pwr-reg
> 
> Should the st,stm32mp1,pwr-reg be treated as fallback compatible for
> st,stm32mp13-pwr-reg or not ?
> 
> In other words, should the DT contain:
> compatible = "st,stm32mp13-pwr-reg", "st,stm32mp1,pwr-reg";
> or
> compatible = "st,stm32mp13-pwr-reg";
> ? Which one is preferable ?
> 
> I think the former one, since the MP13 PWR block could also be operated by
> older MP1(5) PWR block driver(s) without any adverse effects, except the SD
> IO domain configuration won't be available, right ?

Aye, the fallback sounds like what should be being used here, especially
if another user of the DT might not need to implement the extra domain.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-13 15:16     ` Conor Dooley
@ 2024-05-13 18:02       ` Patrick DELAUNAY
  2024-05-14 20:08         ` Conor Dooley
  0 siblings, 1 reply; 11+ messages in thread
From: Patrick DELAUNAY @ 2024-05-13 18:02 UTC (permalink / raw)
  To: Conor Dooley, Marek Vasut
  Cc: Alexandre TORGUE, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

Hi,

On 5/13/24 17:16, Conor Dooley wrote:
> On Mon, May 13, 2024 at 04:34:20PM +0200, Marek Vasut wrote:
>> On 5/13/24 11:56 AM, Patrick Delaunay wrote:
>>> Add new compatible "st,stm32mp13-pwr-reg" for STM32MP13 SoC family.
>>>
>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>> ---
>>>
>>> Changes in v3:
>>> - Replace oneOf/const by enum; solve the V2 issues for dt_binding_check
>>>
>>> Changes in v2:
>>> - update for Rob review, only add compatible for STM32MP13 family
>>>
>>>    .../devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml    | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
>>> index c9586d277f41..c766f0a15a31 100644
>>> --- a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
>>> +++ b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
>>> @@ -11,7 +11,9 @@ maintainers:
>>>    properties:
>>>      compatible:
>>> -    const: st,stm32mp1,pwr-reg
>>> +    enum:
>>> +      - st,stm32mp1,pwr-reg
>>> +      - st,stm32mp13-pwr-reg
>> Should the st,stm32mp1,pwr-reg be treated as fallback compatible for
>> st,stm32mp13-pwr-reg or not ?
>>
>> In other words, should the DT contain:
>> compatible = "st,stm32mp13-pwr-reg", "st,stm32mp1,pwr-reg";
>> or
>> compatible = "st,stm32mp13-pwr-reg";
>> ? Which one is preferable ?
>>
>> I think the former one, since the MP13 PWR block could also be operated by
>> older MP1(5) PWR block driver(s) without any adverse effects, except the SD
>> IO domain configuration won't be available, right ?
> Aye, the fallback sounds like what should be being used here, especially
> if another user of the DT might not need to implement the extra domain.


Yes it is the the only difference but I think that type of fallback is 
no more recommended for different device and

the PWR device on STM32MP13 and on STM32MP15 are different.


The other user of the non-secure device tree don't use the yet the PWR 
driver for STM32MP13,

so for me the fallback is not needed for non secure world (Linux/U-Boot).


So I prefer to introduce a new compatible in Linux kernel before the 
STM32MP13 PWR node is really used  to avoid ABI break in futur.

PS: I will update the U-Boot PWR driver to avoid issue for boards 
managing PWR in non-secure world (alignment with Linux device tree).

For information: on the STMicroelectronics STM32MP13 reference designs the PWR IP is only managed in secure world by OP-TEE
and the support of SD IO domain on PWR STM32MP13 is mandatory for ultra High Speed support on SD/eMMC devices.


The node is introduced in SoC device tree by [1], copied from first up 
streamed OP-TEE device tree,

[1] commit f798f7079233 ("ARM: dts: stm32: add PWR regulators support on stm32mp131")
     https://lore.kernel.org/linux-arm-kernel/b89d0531-067f-4356-91b0-ed7434cee3d7@foss.st.com/


but unfortunately the OP-TEE binding not acceptable.


For OP-TEE this new feature of PWR is managed with new nodes with compatible "st,stm32mp13-iod"
and a separate driver core/drivers/regulator/stm32mp1_regulator_iod.c

But it is NOT acceptable for Linux binding / driver because the register PWR_CR3 = 0x5000100C
are used in this driver and also in PWR regulator driver core/arch/arm/plat-stm32mp1/drivers/stm32mp1_pwr.c

It is not acceptable because offset 0xC of the register range of pwr_regulators: pwr@50001000
(with reg = <0x50001000 0x10>) so the SD IO domain must be defined in the same node.


For example, when the PWR is managed in secure world, the SCMI regulator are it is used with:

&sdmmc1 {
	pinctrl-names = "default", "opendrain", "sleep";
	pinctrl-0 = <&sdmmc1_b4_pins_a &sdmmc1_clk_pins_a>;
	pinctrl-1 = <&sdmmc1_b4_od_pins_a &sdmmc1_clk_pins_a>;
	pinctrl-2 = <&sdmmc1_b4_sleep_pins_a>;
	cd-gpios = <&gpioh 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
	disable-wp;
	st,neg-edge;
	bus-width = <4>;
	vmmc-supply = <&scmi_vdd_sd>;
	vqmmc-supply = <&scmi_sdmmc1_io>;
	sd-uhs-sdr12;
	sd-uhs-sdr25;
	sd-uhs-sdr50;
	sd-uhs-ddr50;
	sd-uhs-sdr104;
	status = "okay";
};


For me the IOD must be export as a regulator of PWR node.

because for hardware point of view sdmmc1_io/sdmmc2_io are at the same level that othe STM32MP13 regulator reg11/reg18/usb33,


So I will align the OP-TEE device tree/ driver if the Linux binding is accepted.


Something like:


		pwr_regulators: pwr@50001000 {
			compatible = "st,stm32mp13,pwr-reg";
			reg = <0x50001000 0x10>;

			reg11: reg11 {
				regulator-name = "reg11";
				regulator-min-microvolt = <1100000>;
				regulator-max-microvolt = <1100000>;
			};

			reg18: reg18 {
				regulator-name = "reg18";
				regulator-min-microvolt = <1800000>;
				regulator-max-microvolt = <1800000>;
			};


			usb33: usb33 {
				regulator-name = "usb33";
				regulator-min-microvolt = <3300000>;
				regulator-max-microvolt = <3300000>;
			};

			sdmmc1_io: sdmmc1_io {
				compatible = "st,stm32mp13-iod";
				regulator-name = "sdmmc1_io";
				regulator-min-microvolt = <1800000>;
				regulator-max-microvolt = <3300000>;
				vddsd1-supply = <&vddsd1>;
				regulator-always-on;
			};

			sdmmc2_io: sdmmc2_io {
				compatible = "st,stm32mp13-iod";
				regulator-name = "sdmmc2_io";
				regulator-min-microvolt = <1800000>;
				regulator-max-microvolt = <3300000>;
				vddsd2-supply = <&vdd>;
				regulator-always-on;
			};
		};


And the sdmmc1_io/sdmmc2_io nodes allow to select the IOD for ultra high speed,

for example with "vqmmc-supply = <&sdmmc1_io>;"


To conclude:

Adding a separate compatible is mandatory for addition of SD IO domain (to manage new sub nodes in yaml),
and the PWR fallback is not needed as PWR not yet used by any board in non secure worl (Linux/U-Boot).

Patrick


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-13 18:02       ` Patrick DELAUNAY
@ 2024-05-14 20:08         ` Conor Dooley
  2024-05-15 14:33           ` Patrick DELAUNAY
  0 siblings, 1 reply; 11+ messages in thread
From: Conor Dooley @ 2024-05-14 20:08 UTC (permalink / raw)
  To: Patrick DELAUNAY
  Cc: Marek Vasut, Alexandre TORGUE, Liam Girdwood, Mark Brown,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

[-- Attachment #1: Type: text/plain, Size: 3212 bytes --]

On Mon, May 13, 2024 at 08:02:21PM +0200, Patrick DELAUNAY wrote:
> Hi,
> 
> On 5/13/24 17:16, Conor Dooley wrote:
> > On Mon, May 13, 2024 at 04:34:20PM +0200, Marek Vasut wrote:
> > > On 5/13/24 11:56 AM, Patrick Delaunay wrote:
> > > > Add new compatible "st,stm32mp13-pwr-reg" for STM32MP13 SoC family.
> > > > 
> > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > > > ---
> > > > 
> > > > Changes in v3:
> > > > - Replace oneOf/const by enum; solve the V2 issues for dt_binding_check
> > > > 
> > > > Changes in v2:
> > > > - update for Rob review, only add compatible for STM32MP13 family
> > > > 
> > > >    .../devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml    | 4 +++-
> > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> > > > index c9586d277f41..c766f0a15a31 100644
> > > > --- a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> > > > +++ b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
> > > > @@ -11,7 +11,9 @@ maintainers:
> > > >    properties:
> > > >      compatible:
> > > > -    const: st,stm32mp1,pwr-reg
> > > > +    enum:
> > > > +      - st,stm32mp1,pwr-reg
> > > > +      - st,stm32mp13-pwr-reg
> > > Should the st,stm32mp1,pwr-reg be treated as fallback compatible for
> > > st,stm32mp13-pwr-reg or not ?
> > > 
> > > In other words, should the DT contain:
> > > compatible = "st,stm32mp13-pwr-reg", "st,stm32mp1,pwr-reg";
> > > or
> > > compatible = "st,stm32mp13-pwr-reg";
> > > ? Which one is preferable ?
> > > 
> > > I think the former one, since the MP13 PWR block could also be operated by
> > > older MP1(5) PWR block driver(s) without any adverse effects, except the SD
> > > IO domain configuration won't be available, right ?
> > Aye, the fallback sounds like what should be being used here, especially
> > if another user of the DT might not need to implement the extra domain.
> 
> 
> Yes it is the the only difference but I think that type of fallback is no
> more recommended for different device and
> 
> the PWR device on STM32MP13 and on STM32MP15 are different.
> 
> 
> The other user of the non-secure device tree don't use the yet the PWR
> driver for STM32MP13,
> 
> so for me the fallback is not needed for non secure world (Linux/U-Boot).
> 
> 
> So I prefer to introduce a new compatible in Linux kernel before the
> STM32MP13 PWR node is really used  to avoid ABI break in futur.

How is it going to break an ABI? If the mp13 implements a functional
subset of what the mp1 does, then that's what fallback compatibles
are intended for. Marek's mail suggests that this is the case, and
therefore a fallback should be used.

If code written for the mp1 would not work on the mp13, then a fallback
should not be used.

Neither I nor Marek are saying that a new compatible should not be
created, in case you misunderstood that. This only affects the binding
patch, and your driver etc are free to use the mp13 compatible.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-14 20:08         ` Conor Dooley
@ 2024-05-15 14:33           ` Patrick DELAUNAY
  2024-05-15 15:35             ` Conor Dooley
  0 siblings, 1 reply; 11+ messages in thread
From: Patrick DELAUNAY @ 2024-05-15 14:33 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Marek Vasut, Alexandre TORGUE, Liam Girdwood, Mark Brown,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

Hi,

On 5/14/24 22:08, Conor Dooley wrote:
> On Mon, May 13, 2024 at 08:02:21PM +0200, Patrick DELAUNAY wrote:
>> Hi,
>>
>> On 5/13/24 17:16, Conor Dooley wrote:
>>> On Mon, May 13, 2024 at 04:34:20PM +0200, Marek Vasut wrote:
>>>> On 5/13/24 11:56 AM, Patrick Delaunay wrote:
>>>>> Add new compatible "st,stm32mp13-pwr-reg" for STM32MP13 SoC family.
>>>>>
>>>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - Replace oneOf/const by enum; solve the V2 issues for dt_binding_check
>>>>>
>>>>> Changes in v2:
>>>>> - update for Rob review, only add compatible for STM32MP13 family
>>>>>
>>>>>     .../devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml    | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
>>>>> index c9586d277f41..c766f0a15a31 100644
>>>>> --- a/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
>>>>> +++ b/Documentation/devicetree/bindings/regulator/st,stm32mp1-pwr-reg.yaml
>>>>> @@ -11,7 +11,9 @@ maintainers:
>>>>>     properties:
>>>>>       compatible:
>>>>> -    const: st,stm32mp1,pwr-reg
>>>>> +    enum:
>>>>> +      - st,stm32mp1,pwr-reg
>>>>> +      - st,stm32mp13-pwr-reg
>>>> Should the st,stm32mp1,pwr-reg be treated as fallback compatible for
>>>> st,stm32mp13-pwr-reg or not ?
>>>>
>>>> In other words, should the DT contain:
>>>> compatible = "st,stm32mp13-pwr-reg", "st,stm32mp1,pwr-reg";
>>>> or
>>>> compatible = "st,stm32mp13-pwr-reg";
>>>> ? Which one is preferable ?
>>>>
>>>> I think the former one, since the MP13 PWR block could also be operated by
>>>> older MP1(5) PWR block driver(s) without any adverse effects, except the SD
>>>> IO domain configuration won't be available, right ?
>>> Aye, the fallback sounds like what should be being used here, especially
>>> if another user of the DT might not need to implement the extra domain.
>>
>> Yes it is the the only difference but I think that type of fallback is no
>> more recommended for different device and
>>
>> the PWR device on STM32MP13 and on STM32MP15 are different.
>>
>>
>> The other user of the non-secure device tree don't use the yet the PWR
>> driver for STM32MP13,
>>
>> so for me the fallback is not needed for non secure world (Linux/U-Boot).
>>
>>
>> So I prefer to introduce a new compatible in Linux kernel before the
>> STM32MP13 PWR node is really used  to avoid ABI break in futur.
> How is it going to break an ABI? If the mp13 implements a functional
> subset of what the mp1 does, then that's what fallback compatibles
> are intended for. Marek's mail suggests that this is the case, and
> therefore a fallback should be used.
>
> If code written for the mp1 would not work on the mp13, then a fallback
> should not be used.
>
> Neither I nor Marek are saying that a new compatible should not be
> created, in case you misunderstood that. This only affects the binding
> patch, and your driver etc are free to use the mp13 compatible.
>
> Thanks,
> Conor.


The IP PWR change with STM32MP13 and it is not just a subset

but for the range used by the compatible <0x50001000 0x10>

I agreed that feature are just added.


Other feature are also removed but for next registers (the associated

registers or bits becomes reserved, for PWR_MCUCR for example).


So I can change in binding to support the DT:


compatible = "st,stm32mp1,pwr-reg";

or for STM32MP13:

compatible = "st,stm32mp13-pwr-reg", "st,stm32mp1,pwr-reg";

with

   compatible:
     oneOf:
         - items:
            - const: st,stm32mp1,pwr-reg
         - items:
            - const: st,stm32mp13-pwr-reg
            - const: st,stm32mp1,pwr-reg


But for me this fallback in device tree is a little confusing for PWR.


Patrick


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-15 14:33           ` Patrick DELAUNAY
@ 2024-05-15 15:35             ` Conor Dooley
  2024-05-15 15:37               ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Conor Dooley @ 2024-05-15 15:35 UTC (permalink / raw)
  To: Patrick DELAUNAY
  Cc: Marek Vasut, Alexandre TORGUE, Liam Girdwood, Mark Brown,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Wed, May 15, 2024 at 04:33:22PM +0200, Patrick DELAUNAY wrote:
> with
> 
>   compatible:
>     oneOf:
>         - items:
>            - const: st,stm32mp1,pwr-reg
>         - items:
>            - const: st,stm32mp13-pwr-reg
>            - const: st,stm32mp1,pwr-reg

Other than the extra ,s this looks okay, thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-15 15:35             ` Conor Dooley
@ 2024-05-15 15:37               ` Marek Vasut
  2024-05-15 16:15                 ` Conor Dooley
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2024-05-15 15:37 UTC (permalink / raw)
  To: Conor Dooley, Patrick DELAUNAY
  Cc: Alexandre TORGUE, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

On 5/15/24 5:35 PM, Conor Dooley wrote:
> On Wed, May 15, 2024 at 04:33:22PM +0200, Patrick DELAUNAY wrote:
>> with
>>
>>    compatible:
>>      oneOf:
>>          - items:
>>             - const: st,stm32mp1,pwr-reg
>>          - items:
>>             - const: st,stm32mp13-pwr-reg
>>             - const: st,stm32mp1,pwr-reg
> 
> Other than the extra ,s this looks okay, thanks.

I think the extra ,s are actually correct, those are the ones from the 
original compatible which had TWO ,s (it does look a bit unusual).

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-15 15:37               ` Marek Vasut
@ 2024-05-15 16:15                 ` Conor Dooley
  2024-05-16 16:32                   ` Patrick DELAUNAY
  0 siblings, 1 reply; 11+ messages in thread
From: Conor Dooley @ 2024-05-15 16:15 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Patrick DELAUNAY, Alexandre TORGUE, Liam Girdwood, Mark Brown,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Wed, May 15, 2024 at 05:37:49PM +0200, Marek Vasut wrote:
> On 5/15/24 5:35 PM, Conor Dooley wrote:
> > On Wed, May 15, 2024 at 04:33:22PM +0200, Patrick DELAUNAY wrote:
> > > with
> > > 
> > >    compatible:
> > >      oneOf:
> > >          - items:
> > >             - const: st,stm32mp1,pwr-reg
> > >          - items:
> > >             - const: st,stm32mp13-pwr-reg
> > >             - const: st,stm32mp1,pwr-reg
> > 
> > Other than the extra ,s this looks okay, thanks.
> 
> I think the extra ,s are actually correct, those are the ones from the
> original compatible which had TWO ,s (it does look a bit unusual).

Oh my bad then.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13
  2024-05-15 16:15                 ` Conor Dooley
@ 2024-05-16 16:32                   ` Patrick DELAUNAY
  0 siblings, 0 replies; 11+ messages in thread
From: Patrick DELAUNAY @ 2024-05-16 16:32 UTC (permalink / raw)
  To: Conor Dooley, Marek Vasut
  Cc: Alexandre TORGUE, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
	Pascal Paillet, devicetree, linux-arm-kernel, linux-kernel,
	linux-stm32

Hi,

On 5/15/24 18:15, Conor Dooley wrote:
> On Wed, May 15, 2024 at 05:37:49PM +0200, Marek Vasut wrote:
>> On 5/15/24 5:35 PM, Conor Dooley wrote:
>>> On Wed, May 15, 2024 at 04:33:22PM +0200, Patrick DELAUNAY wrote:
>>>> with
>>>>
>>>>     compatible:
>>>>       oneOf:
>>>>           - items:
>>>>              - const: st,stm32mp1,pwr-reg
>>>>           - items:
>>>>              - const: st,stm32mp13-pwr-reg
>>>>              - const: st,stm32mp1,pwr-reg
>>> Other than the extra ,s this looks okay, thanks.
>> I think the extra ,s are actually correct, those are the ones from the
>> original compatible which had TWO ,s (it does look a bit unusual).
> Oh my bad then.


Ok, I prepare a V4 with fallback


and comma in compatible "st,stm32mp1,pwr-reg" was clearly an error

but it is too late to change it, see [1]

[1] ARM: st: use a correct pwr compatible for stm32mp15

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=847733&state=* 


regards

Patrick



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-05-16 16:33 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-13  9:56 [PATCH v3 0/2] ARM: st: add new compatible for PWR regulators on STM32MP13 Patrick Delaunay
2024-05-13  9:56 ` [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13 Patrick Delaunay
2024-05-13 14:34   ` Marek Vasut
2024-05-13 15:16     ` Conor Dooley
2024-05-13 18:02       ` Patrick DELAUNAY
2024-05-14 20:08         ` Conor Dooley
2024-05-15 14:33           ` Patrick DELAUNAY
2024-05-15 15:35             ` Conor Dooley
2024-05-15 15:37               ` Marek Vasut
2024-05-15 16:15                 ` Conor Dooley
2024-05-16 16:32                   ` Patrick DELAUNAY

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).