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

[Patrick DELAUNAY]

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@xxxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > 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