diff mbox series

[v3,1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add compatible for STM32MP13

Message ID 20240513115601.v3.1.Ia0a99d90acb512aa020a6e7a8cca8cc1b71f1759@changeid (mailing list archive)
State New, archived
Headers show
Series ARM: st: add new compatible for PWR regulators on STM32MP13 | expand

Commit Message

Patrick Delaunay May 13, 2024, 9:56 a.m. UTC
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(-)

Comments

Marek Vasut May 13, 2024, 2:34 p.m. UTC | #1
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 ?
Conor Dooley May 13, 2024, 3:16 p.m. UTC | #2
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.
Patrick Delaunay May 13, 2024, 6:02 p.m. UTC | #3
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
Conor Dooley May 14, 2024, 8:08 p.m. UTC | #4
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.
Patrick Delaunay May 15, 2024, 2:33 p.m. UTC | #5
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
Conor Dooley May 15, 2024, 3:35 p.m. UTC | #6
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.
Marek Vasut May 15, 2024, 3:37 p.m. UTC | #7
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).
Conor Dooley May 15, 2024, 4:15 p.m. UTC | #8
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.
Patrick Delaunay May 16, 2024, 4:32 p.m. UTC | #9
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
diff mbox series

Patch

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