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

[Patrick DELAUNAY <patrick.delaunay@foss.st.com>]

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