arm64: dts: renesas: r9a07g044: Add missing GICv3 node properties

Commit Message

Lad Prabhakar June 11, 2021, 3:21 p.m. UTC

Add the below missing properties into GIC node,
- clocks
- clock-names
- power-domains
- resets

Signed-off-by: Lad Prabhakar <[e-mail]>
Reviewed-by: Biju Das <[e-mail]>
---
Hi All,

This patch depends on patch [1] + patch series [2].

[1] https://lore.kernel.org/linux-devicetree/
    [e-mail]/
[2] https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=498915

Cheers,
Prabhakar
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Geert Uytterhoeven June 14, 2021, 12:48 p.m. UTC | #1

On Fri, Jun 11, 2021 at 5:21 PM Lad Prabhakar
<[e-mail]> wrote:
> Add the below missing properties into GIC node,
> - clocks
> - clock-names
> - power-domains
> - resets
>
> Signed-off-by: Lad Prabhakar <[e-mail]>
> Reviewed-by: Biju Das <[e-mail]>

Reviewed-by: Geert Uytterhoeven <[e-mail]>

Queueing pending on[1].

> [1] https://lore.kernel.org/linux-devicetree/
>     [e-mail]/


Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven July 13, 2021, 8:08 a.m. UTC | #2

Hi Prabhakar,

On Mon, Jun 14, 2021 at 2:48 PM Geert Uytterhoeven <[e-mail]> wrote:
> On Fri, Jun 11, 2021 at 5:21 PM Lad Prabhakar
> <[e-mail]> wrote:
> > Add the below missing properties into GIC node,
> > - clocks
> > - clock-names
> > - power-domains
> > - resets
> >
> > Signed-off-by: Lad Prabhakar <[e-mail]>
> > Reviewed-by: Biju Das <[e-mail]>
>
> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>
> Queueing pending on[1].
>
> > [1] https://lore.kernel.org/linux-devicetree/
> >     [e-mail]/

The dependency has been accepted, but this patch needs a respin
for the changed clocks.

Gr{oetje,eeting}s,

                        Geert

Lad, Prabhakar July 13, 2021, 8:22 a.m. UTC | #3

Hi Geert,

On Tue, Jul 13, 2021 at 9:08 AM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jun 14, 2021 at 2:48 PM Geert Uytterhoeven <[e-mail]> wrote:
> > On Fri, Jun 11, 2021 at 5:21 PM Lad Prabhakar
> > <[e-mail]> wrote:
> > > Add the below missing properties into GIC node,
> > > - clocks
> > > - clock-names
> > > - power-domains
> > > - resets
> > >
> > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > Reviewed-by: Biju Das <[e-mail]>
> >
> > Reviewed-by: Geert Uytterhoeven <[e-mail]>
> >
> > Queueing pending on[1].
> >
> > > [1] https://lore.kernel.org/linux-devicetree/
> > >     [e-mail]/
>
> The dependency has been accepted, but this patch needs a respin
> for the changed clocks.
>
Thank you for pointing this out. wrt resets the GIC has two signals
(which I learnt lately when the dependency path was accepted). Earlier
discussion in irc with Sudeep pointed out that there wouldn't be any
use case of having GIC resets in DTSI, so either we drop the resets
property in DT binding doc or correct it.

Let me know your thoughts on this and how we proceed further.

Cheers,
Prabhakar

Geert Uytterhoeven July 13, 2021, 8:30 a.m. UTC | #4

Hi Prabhakar,

On Tue, Jul 13, 2021 at 10:22 AM Lad, Prabhakar
<[e-mail]> wrote:
> On Tue, Jul 13, 2021 at 9:08 AM Geert Uytterhoeven <[e-mail]> wrote:
> > On Mon, Jun 14, 2021 at 2:48 PM Geert Uytterhoeven <[e-mail]> wrote:
> > > On Fri, Jun 11, 2021 at 5:21 PM Lad Prabhakar
> > > <[e-mail]> wrote:
> > > > Add the below missing properties into GIC node,
> > > > - clocks
> > > > - clock-names
> > > > - power-domains
> > > > - resets
> > > >
> > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > Reviewed-by: Biju Das <[e-mail]>
> > >
> > > Reviewed-by: Geert Uytterhoeven <[e-mail]>
> > >
> > > Queueing pending on[1].
> > >
> > > > [1] https://lore.kernel.org/linux-devicetree/
> > > >     [e-mail]/
> >
> > The dependency has been accepted, but this patch needs a respin
> > for the changed clocks.
> >
> Thank you for pointing this out. wrt resets the GIC has two signals
> (which I learnt lately when the dependency path was accepted). Earlier
> discussion in irc with Sudeep pointed out that there wouldn't be any
> use case of having GIC resets in DTSI, so either we drop the resets
> property in DT binding doc or correct it.
>
> Let me know your thoughts on this and how we proceed further.

DT Rule #1: DT describes hardware not software policy.

And a possible use case: the RT CPU core may want to reset the AP GIC.

Gr{oetje,eeting}s,

                        Geert

Sudeep Holla July 13, 2021, 8:55 a.m. UTC | #5

On Tue, Jul 13, 2021 at 10:30:36AM +0200, Geert Uytterhoeven wrote:
> Hi Prabhakar,
> 
> On Tue, Jul 13, 2021 at 10:22 AM Lad, Prabhakar
> <[e-mail]> wrote:
> > On Tue, Jul 13, 2021 at 9:08 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > On Mon, Jun 14, 2021 at 2:48 PM Geert Uytterhoeven <[e-mail]> wrote:
> > > > On Fri, Jun 11, 2021 at 5:21 PM Lad Prabhakar
> > > > <[e-mail]> wrote:
> > > > > Add the below missing properties into GIC node,
> > > > > - clocks
> > > > > - clock-names
> > > > > - power-domains
> > > > > - resets
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > Reviewed-by: Biju Das <[e-mail]>
> > > >
> > > > Reviewed-by: Geert Uytterhoeven <[e-mail]>
> > > >
> > > > Queueing pending on[1].
> > > >
> > > > > [1] https://lore.kernel.org/linux-devicetree/
> > > > >     [e-mail]/
> > >
> > > The dependency has been accepted, but this patch needs a respin
> > > for the changed clocks.
> > >
> > Thank you for pointing this out. wrt resets the GIC has two signals
> > (which I learnt lately when the dependency path was accepted). Earlier
> > discussion in irc with Sudeep pointed out that there wouldn't be any
> > use case of having GIC resets in DTSI, so either we drop the resets
> > property in DT binding doc or correct it.
> >
> > Let me know your thoughts on this and how we proceed further.
>
> DT Rule #1: DT describes hardware not software policy.
>

Completely agreed, no disagreement 

Geert Uytterhoeven July 13, 2021, 9:04 a.m. UTC | #6

Hi Sudeep,

On Tue, Jul 13, 2021 at 10:56 AM Sudeep Holla <[e-mail]> wrote:
> On Tue, Jul 13, 2021 at 10:30:36AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Jul 13, 2021 at 10:22 AM Lad, Prabhakar
> > <[e-mail]> wrote:
> > > On Tue, Jul 13, 2021 at 9:08 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > > On Mon, Jun 14, 2021 at 2:48 PM Geert Uytterhoeven <[e-mail]> wrote:
> > > > > On Fri, Jun 11, 2021 at 5:21 PM Lad Prabhakar
> > > > > <[e-mail]> wrote:
> > > > > > Add the below missing properties into GIC node,
> > > > > > - clocks
> > > > > > - clock-names
> > > > > > - power-domains
> > > > > > - resets
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > > Reviewed-by: Biju Das <[e-mail]>
> > > > >
> > > > > Reviewed-by: Geert Uytterhoeven <[e-mail]>
> > > > >
> > > > > Queueing pending on[1].
> > > > >
> > > > > > [1] https://lore.kernel.org/linux-devicetree/
> > > > > >     [e-mail]/
> > > >
> > > > The dependency has been accepted, but this patch needs a respin
> > > > for the changed clocks.
> > > >
> > > Thank you for pointing this out. wrt resets the GIC has two signals
> > > (which I learnt lately when the dependency path was accepted). Earlier
> > > discussion in irc with Sudeep pointed out that there wouldn't be any
> > > use case of having GIC resets in DTSI, so either we drop the resets
> > > property in DT binding doc or correct it.
> > >
> > > Let me know your thoughts on this and how we proceed further.
> >
> > DT Rule #1: DT describes hardware not software policy.
> >
>
> Completely agreed, no disagreement .

Good ;-)

> > And a possible use case: the RT CPU core may want to reset the AP GIC.
>
> I didn't want to add new bindings without details on the implementation
> to avoid possible issues with backward compatibility as this was not
> thought through completely and correctly before it was added.
>
> OK, now let us discuss your use-case: *RT CPU wants to reset AP GIC*
>
> 1. Will it just reset AP GIC or will it request the AP reset as a whole ?
>    I am not sure if we can handle former, if you think otherwise what is
>    the reset notification mechanism ?
>
> 2. Will that bypass secure world/PSCI ? Again more details on this would
>    be helpful to visualise the entire use-case end-to-end better.
>
> By GIC reset, I am assuming it will be complete GIC reset including it's
> CPU interface.
>
> I don't think we can reset GIC without actual CPU reset. Even if we get
> some notification magically to the CPU that its GIC alone needs to be
> reset, it needs to safely higher exceptions to get its GIC CPU interface
> reprogrammed to correct (saved) values before OS can reprogram the NS
> world values. All these seems overall complicated and may be unnecessary.

Probably both.  Might make sense to reset on wake-up, after having disabled
clocks and powered down the AP CPU, AP GIC, ...

If that bypasses PSCI: well, if the unsecure software can do it, it
means the hardware is not secure. Or at least Linux has to be trusted.

Gr{oetje,eeting}s,

                        Geert

Sudeep Holla July 13, 2021, 9:15 a.m. UTC | #7

On Tue, Jul 13, 2021 at 11:04:09AM +0200, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Tue, Jul 13, 2021 at 10:56 AM Sudeep Holla <[e-mail]> wrote:
> > On Tue, Jul 13, 2021 at 10:30:36AM +0200, Geert Uytterhoeven wrote:

[...]

> > > And a possible use case: the RT CPU core may want to reset the AP GIC.
> >
> > I didn't want to add new bindings without details on the implementation
> > to avoid possible issues with backward compatibility as this was not
> > thought through completely and correctly before it was added.
> >
> > OK, now let us discuss your use-case: *RT CPU wants to reset AP GIC*
> >
> > 1. Will it just reset AP GIC or will it request the AP reset as a whole ?
> >    I am not sure if we can handle former, if you think otherwise what is
> >    the reset notification mechanism ?
> >
> > 2. Will that bypass secure world/PSCI ? Again more details on this would
> >    be helpful to visualise the entire use-case end-to-end better.
> >
> > By GIC reset, I am assuming it will be complete GIC reset including it's
> > CPU interface.
> >
> > I don't think we can reset GIC without actual CPU reset. Even if we get
> > some notification magically to the CPU that its GIC alone needs to be
> > reset, it needs to safely higher exceptions to get its GIC CPU interface
> > reprogrammed to correct (saved) values before OS can reprogram the NS
> > world values. All these seems overall complicated and may be unnecessary.
>
> Probably both.  Might make sense to reset on wake-up, after having disabled
> clocks and powered down the AP CPU, AP GIC, ...
>

/me confused. If this is arm64 platform, then you have to use *PSCI* and
I expect the reset to be done as part of CPU wake-up in PSCI firmware.

> If that bypasses PSCI: well, if the unsecure software can do it, it
> means the hardware is not secure. Or at least Linux has to be trusted.
>

No, if the system has PSCI, then you simply can't bypass that for GIC
reset. Or at-least I am failing to understand the complete flow of that.

--
Regards,
Sudeep

Geert Uytterhoeven July 13, 2021, 9:24 a.m. UTC | #8

Hi Sudeep,

On Tue, Jul 13, 2021 at 11:16 AM Sudeep Holla <[e-mail]> wrote:
> On Tue, Jul 13, 2021 at 11:04:09AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Jul 13, 2021 at 10:56 AM Sudeep Holla <[e-mail]> wrote:
> > > On Tue, Jul 13, 2021 at 10:30:36AM +0200, Geert Uytterhoeven wrote:
>
> [...]
>
> > > > And a possible use case: the RT CPU core may want to reset the AP GIC.
> > >
> > > I didn't want to add new bindings without details on the implementation
> > > to avoid possible issues with backward compatibility as this was not
> > > thought through completely and correctly before it was added.
> > >
> > > OK, now let us discuss your use-case: *RT CPU wants to reset AP GIC*
> > >
> > > 1. Will it just reset AP GIC or will it request the AP reset as a whole ?
> > >    I am not sure if we can handle former, if you think otherwise what is
> > >    the reset notification mechanism ?
> > >
> > > 2. Will that bypass secure world/PSCI ? Again more details on this would
> > >    be helpful to visualise the entire use-case end-to-end better.
> > >
> > > By GIC reset, I am assuming it will be complete GIC reset including it's
> > > CPU interface.
> > >
> > > I don't think we can reset GIC without actual CPU reset. Even if we get
> > > some notification magically to the CPU that its GIC alone needs to be
> > > reset, it needs to safely higher exceptions to get its GIC CPU interface
> > > reprogrammed to correct (saved) values before OS can reprogram the NS
> > > world values. All these seems overall complicated and may be unnecessary.
> >
> > Probably both.  Might make sense to reset on wake-up, after having disabled
> > clocks and powered down the AP CPU, AP GIC, ...
> >
>
> /me confused. If this is arm64 platform, then you have to use *PSCI* and
> I expect the reset to be done as part of CPU wake-up in PSCI firmware.

DT Rule #1: DT describes hardware not software policy.

The fact that _Linux_ must use PSCI is a (unfortunate) software policy.
What about other OSes, or bare-metal software?

> > If that bypasses PSCI: well, if the unsecure software can do it, it
> > means the hardware is not secure. Or at least Linux has to be trusted.
>
> No, if the system has PSCI, then you simply can't bypass that for GIC
> reset. Or at-least I am failing to understand the complete flow of that.

PSCI can only prevent other software from bypassing GIC reset if PSCI
programs the hardware to prevent access to the GIC reset (if possible
at all).

Gr{oetje,eeting}s,

                        Geert

Sudeep Holla July 13, 2021, 10:23 a.m. UTC | #9

On Tue, Jul 13, 2021 at 11:24:24AM +0200, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Tue, Jul 13, 2021 at 11:16 AM Sudeep Holla <[e-mail]> wrote:
> > On Tue, Jul 13, 2021 at 11:04:09AM +0200, Geert Uytterhoeven wrote:

[...]

> > >
> > > Probably both.  Might make sense to reset on wake-up, after having disabled
> > > clocks and powered down the AP CPU, AP GIC, ...
> > >
> >
> > /me confused. If this is arm64 platform, then you have to use *PSCI* and
> > I expect the reset to be done as part of CPU wake-up in PSCI firmware.
>
> DT Rule #1: DT describes hardware not software policy.
>

As mentioned before I agree on that. But I assume you too agree that not
all bits and pieces of hardware are represented in DT. Only ones that are
essential for any software to understand the hardware and make it work.
So my opinion is that this GIC reset information is implicit like many
other hardware information.

> The fact that _Linux_ must use PSCI is a (unfortunate) software policy.
> What about other OSes, or bare-metal software?
>

Disagree. PSCI is OS agnostic and _Linux_ is not the sole user. Do you
have examples of other OS that deploy alternate to PSCI ? Or bare-metal
that uses DT ? Again if there is a use-case, you need to spell out details
on when this can be used and where it can't be of much use(of-course with
the mention of Linux). There are complaints that DT bindings are too Linux
specific, so please pull in the other OS folks or other users so that we
get details on use-cases. Based on your argument we should have loads of
other information in DT on CPU or CPU peripherals for example even if
PSCI hides them for OS. Do we really want to get down that path ?

I agree PSCI is software policy but definitely fortunate, helped prevent
lot of non-sense in OS 

Patch

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 2ffdaed6c9a5..81097f1e242c 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -157,6 +157,10 @@ 
 			reg = <0x0 0x11900000 0 0x40000>,
 			      <0x0 0x11940000 0 0x60000>;
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
+			clocks = <&cpg CPG_MOD R9A07G044_CLK_GIC600>;
+			clock-names = "aclk";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A07G044_CLK_GIC600>;
 		};
 
 		usbphyctrl: usbphyctrl@11c40000 {