| 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 |
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 ?
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.
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
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.
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
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.
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).
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.
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 --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
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(-)