Re: [PATCH 07/10] arm64: dts: st: add RIFSC as a domain controller for STM32MP25x boards

[Gatien CHEVALLIER]

Hi Alex,

On 7/6/23 11:30, Gatien CHEVALLIER wrote:
> Hi Alex,
>
> On 7/6/23 11:25, Alexandre TORGUE wrote:
> > Hi Gatien
> >
> > On 7/5/23 19:27, Gatien Chevallier wrote:
> > > RIFSC is a firewall controller. Change its compatible so that is matches
> > > the documentation and reference RIFSC as a feature-domain-controller.
> > >
> > > Signed-off-by: Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx>
> > > ---
> > >   arch/arm64/boot/dts/st/stm32mp251.dtsi | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi 
> > > b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> > > index 5268a4321841..62101084cab8 100644
> > > --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
> > > +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> > > @@ -106,17 +106,20 @@ soc@0 {
> > >           ranges = <0x0 0x0 0x0 0x80000000>;
> > >           rifsc: rifsc-bus@42080000 {
> > > -            compatible = "simple-bus";
> > > +            compatible = "st,stm32mp25-rifsc";
> >
> > You could keep "simple-bus" compatible (in second position). In case 
> > of the RIFSC is not probed, the platform will be able to boot. If you 
> > agree you can use the same for ETZPC.
> >
> > Cheers
> > Alex
>
> Sure, good point.
>
> I'll change that in V2
>
> Best regards,
> Gatien

Actually, it would be a bad idea to keep "simple-bus" as a compatible.

Practical example:
1) Firewall controller forbids a device probe by marking the device's
node as populated (OF_POPULATED flag).
2) The simple-bus, which is simple, populates all the devices
from the device tree data, overriding what the firewall bus has done.
3)=>Forbidden device's driver will be probed.

I think it's best to keep one compatible. If someone wants these drivers
as external modules, then it'll be best to handle this differently.
I'll resubmit with a single compatible for V2, please do not
hesitate to comment on the V2 if you're not okay with this.

Best regards,
Gatien

> > >               reg = <0x42080000 0x1000>;
> > >               #address-cells = <1>;
> > >               #size-cells = <1>;
> > >               ranges;
> > > +            feature-domain-controller;
> > > +            #feature-domain-cells = <1>;
> > >               usart2: serial@400e0000 {
> > >                   compatible = "st,stm32h7-uart";
> > >                   reg = <0x400e0000 0x400>;
> > >                   interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
> > >                   clocks = <&ck_flexgen_08>;
> > > +                feature-domains = <&rifsc 32>;
> > >                   status = "disabled";
> > >               };
> > >           };