Received-SPF: pass (google.com: domain of linux-kernel+bounces-180012-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0c97408c-422d-46b3-8017-da9ebb0767e1@foss.st.com>
Date: Wed, 15 May 2024 16:33:22 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/2] dt-bindings: regulator: st,stm32mp1-pwr-reg: add
 compatible for STM32MP13
To: Conor Dooley <conor@kernel.org>
CC: Marek Vasut <marex@denx.de>,
        Alexandre TORGUE
	<alexandre.torgue@foss.st.com>,
        Liam Girdwood <lgirdwood@gmail.com>, Mark
 Brown <broonie@kernel.org>,
        Rob Herring <robh@kernel.org>,
        Krzysztof
 Kozlowski <krzk+dt@kernel.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Maxime
 Coquelin <mcoquelin.stm32@gmail.com>,
        Pascal Paillet <p.paillet@foss.st.com>, <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>
References: <20240513095605.218042-1-patrick.delaunay@foss.st.com>
 <20240513115601.v3.1.Ia0a99d90acb512aa020a6e7a8cca8cc1b71f1759@changeid>
 <615dfdcb-cbda-426f-895e-810f03a8ce60@denx.de>
 <20240513-stabilize-proofread-81f0f9ee38b9@spud>
 <d73d4435-75d6-4cea-b38e-07c7ceae3980@foss.st.com>
 <20240514-entryway-idealize-fcd5ed0e1de7@spud>
Content-Language: en-US
From: Patrick DELAUNAY <patrick.delaunay@foss.st.com>
In-Reply-To: <20240514-entryway-idealize-fcd5ed0e1de7@spud>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

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