diff mbox series

[01/17] dt-bindings: clock: r9a07g043-cpg: Add power domain IDs

Message ID 20240208124300.2740313-2-claudiu.beznea.uj@bp.renesas.com
State New
Headers
Series clk: renesas: rzg2l: Add support for power domains |

Commit Message

claudiu beznea Feb. 8, 2024, 12:42 p.m. UTC 
  From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add power domain IDs for RZ/G2UL (R9A07G043) SoC.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 include/dt-bindings/clock/r9a07g043-cpg.h | 48 +++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

claudiu beznea Feb. 8, 2024, 4:53 p.m. UTC | #1 
On 08.02.2024 18:28, Biju Das wrote:
> 
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Thursday, February 8, 2024 3:46 PM
>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
>> domain IDs
>>
>> Hi, Biju,
>>
>> On 08.02.2024 16:30, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>> Thanks for the patch.
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: Thursday, February 8, 2024 12:43 PM
>>>> Subject: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
>>>> domain IDs
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>  include/dt-bindings/clock/r9a07g043-cpg.h | 48
>>>> +++++++++++++++++++++++
>>>>  1 file changed, 48 insertions(+)
>>>>
>>>> diff --git a/include/dt-bindings/clock/r9a07g043-cpg.h b/include/dt-
>>>> bindings/clock/r9a07g043-cpg.h index 77cde8effdc7..eabfeec7ac37
>>>> 100644
>>>> --- a/include/dt-bindings/clock/r9a07g043-cpg.h
>>>> +++ b/include/dt-bindings/clock/r9a07g043-cpg.h
>>>> @@ -200,5 +200,53 @@
>>>>  #define R9A07G043_AX45MP_CORE0_RESETN	78	/* RZ/Five Only */
>>>>  #define R9A07G043_IAX45_RESETN		79	/* RZ/Five Only */
>>>>
>>>> +/* Power domain IDs. */
>>>> +#define R9A07G043_PD_ALWAYS_ON		0
>>>> +#define R9A07G043_PD_GIC		1
>>>> +#define R9A07G043_PD_IA55		2
>>>> +#define R9A07G043_PD_MHU		3
>>>> +#define R9A07G043_PD_CORESIGHT		4
>>>> +#define R9A07G043_PD_SYC		5
>>>> +#define R9A07G043_PD_DMAC		6
>>>> +#define R9A07G043_PD_GTM0		7
>>>> +#define R9A07G043_PD_GTM1		8
>>>> +#define R9A07G043_PD_GTM2		9
>>>> +#define R9A07G043_PD_MTU		10
>>>> +#define R9A07G043_PD_POE3		11
>>>> +#define R9A07G043_PD_WDT0		12
>>>> +#define R9A07G043_PD_SPI		13
>>>> +#define R9A07G043_PD_SDHI0		14
>>>> +#define R9A07G043_PD_SDHI1		15
>>>> +#define R9A07G043_PD_ISU		16
>>>> +#define R9A07G043_PD_CRU		17
>>>> +#define R9A07G043_PD_LCDC		18
>>>> +#define R9A07G043_PD_SSI0		19
>>>> +#define R9A07G043_PD_SSI1		20
>>>> +#define R9A07G043_PD_SSI2		21
>>>> +#define R9A07G043_PD_SSI3		22
>>>> +#define R9A07G043_PD_SRC		23
>>>> +#define R9A07G043_PD_USB0		24
>>>> +#define R9A07G043_PD_USB1		25
>>>> +#define R9A07G043_PD_USB_PHY		26
>>>> +#define R9A07G043_PD_ETHER0		27
>>>> +#define R9A07G043_PD_ETHER1		28
>>>> +#define R9A07G043_PD_I2C0		29
>>>> +#define R9A07G043_PD_I2C1		30
>>>> +#define R9A07G043_PD_I2C2		31
>>>> +#define R9A07G043_PD_I2C3		32
>>>> +#define R9A07G043_PD_SCIF0		33
>>>> +#define R9A07G043_PD_SCIF1		34
>>>> +#define R9A07G043_PD_SCIF2		35
>>>> +#define R9A07G043_PD_SCIF3		36
>>>> +#define R9A07G043_PD_SCIF4		37
>>>> +#define R9A07G043_PD_SCI0		38
>>>> +#define R9A07G043_PD_SCI1		39
>>>> +#define R9A07G043_PD_IRDA		40
>>>> +#define R9A07G043_PD_RSPI0		41
>>>> +#define R9A07G043_PD_RSPI1		42
>>>> +#define R9A07G043_PD_RSPI2		43
>>>> +#define R9A07G043_PD_CANFD		44
>>>> +#define R9A07G043_PD_ADC		45
>>>> +#define R9A07G043_PD_TSU		46
>>>
>>> Not sure from "Table 42.3 Registers for Module Standby Mode"
>>>
>>> Power domain ID has to be based on CPG_BUS_***_MSTOP or CPG_CLKON_***
>>> As former reduces number of IDs??
>>
>> If I understand correctly your point here, you want me to describe PM
>> domain in DT with something like:
>>
>> power-domains = <&cpg CPG_BUS_X_MSTOP>;
> 
> MSTOP bits are distinct for each IP.
> 
> <&cpg CPG_BUS_MCPU1_MSTOP x>; x =1..9
> 
> 2=MTU IP
> 
> 4= GPT
> 
> etc...
> 
> Is it something work??

It might work. But:

- you have to consider that some IPs have more than one MSTOP bit, thus, do
  we want to uniquely identify these with all MSTOP bits (thus the 2nd cell
  being a bitmask) or only one is enough?
- some HW blocks (e.g. OTFDE_DDR) have no MSTOP bits associated (as of my
  current research), so, only PWRDN
- some HW blocks have both MSTOP and PWRDN
- if future hardware implementation will spread the MSTOP bits for one IP
  to more than one register then this proposal will not work

Having a unique identified decoupled from MSTOP registers or PWRDN offers
support to use the same code base for future usage. This is what I can tell
at the moment.

> 
>>
>> where X={ACPU, PERI_CPU, PERI_CPU2, REG0, REG1} ?
>>
>> With this, I still see the necessity of a 3rd identifier that will be IP
>> specific to be able to uniquely match b/w DT description and registered
>> power domain. FMPOV, this will lead to a more complicated implementation.
>>
>> We need a unique ID that the pm domain xlate will use to xlate the DT
>> binding to driver data structures.
> 
> Ok.
> 
> Cheers,
> Biju
>
claudiu beznea Feb. 12, 2024, 8:02 a.m. UTC | #2 
Hi, Biju,

On 08.02.2024 21:20, Biju Das wrote:
> 
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Thursday, February 8, 2024 4:53 PM
>> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be;
>> mturquette@baylibre.com; sboyd@kernel.org; robh@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> magnus.damm@gmail.com; paul.walmsley@sifive.com; palmer@dabbelt.com;
>> aou@eecs.berkeley.edu
>> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>> riscv@lists.infradead.org; Claudiu Beznea
>> <claudiu.beznea.uj@bp.renesas.com>
>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
>> domain IDs
>>
>>
>>
>> On 08.02.2024 18:28, Biju Das wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>>> Sent: Thursday, February 8, 2024 3:46 PM
>>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
>>>> power domain IDs
>>>>
>>>> Hi, Biju,
>>>>
>>>> On 08.02.2024 16:30, Biju Das wrote:
>>>>> Hi Claudiu,
>>>>>
>>>>> Thanks for the patch.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>>>> Sent: Thursday, February 8, 2024 12:43 PM
>>>>>> Subject: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
>>>>>> domain IDs
>>>>>>
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>
>>>>>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>> ---
>>>>>>  include/dt-bindings/clock/r9a07g043-cpg.h | 48
>>>>>> +++++++++++++++++++++++
>>>>>>  1 file changed, 48 insertions(+)
>>>>>>
>>>>>> diff --git a/include/dt-bindings/clock/r9a07g043-cpg.h
>>>>>> b/include/dt- bindings/clock/r9a07g043-cpg.h index
>>>>>> 77cde8effdc7..eabfeec7ac37
>>>>>> 100644
>>>>>> --- a/include/dt-bindings/clock/r9a07g043-cpg.h
>>>>>> +++ b/include/dt-bindings/clock/r9a07g043-cpg.h
>>>>>> @@ -200,5 +200,53 @@
>>>>>>  #define R9A07G043_AX45MP_CORE0_RESETN	78	/* RZ/Five Only */
>>>>>>  #define R9A07G043_IAX45_RESETN		79	/* RZ/Five Only */
>>>>>>
>>>>>> +/* Power domain IDs. */
>>>>>> +#define R9A07G043_PD_ALWAYS_ON		0
>>>>>> +#define R9A07G043_PD_GIC		1
>>>>>> +#define R9A07G043_PD_IA55		2
>>>>>> +#define R9A07G043_PD_MHU		3
>>>>>> +#define R9A07G043_PD_CORESIGHT		4
>>>>>> +#define R9A07G043_PD_SYC		5
>>>>>> +#define R9A07G043_PD_DMAC		6
>>>>>> +#define R9A07G043_PD_GTM0		7
>>>>>> +#define R9A07G043_PD_GTM1		8
>>>>>> +#define R9A07G043_PD_GTM2		9
>>>>>> +#define R9A07G043_PD_MTU		10
>>>>>> +#define R9A07G043_PD_POE3		11
>>>>>> +#define R9A07G043_PD_WDT0		12
>>>>>> +#define R9A07G043_PD_SPI		13
>>>>>> +#define R9A07G043_PD_SDHI0		14
>>>>>> +#define R9A07G043_PD_SDHI1		15
>>>>>> +#define R9A07G043_PD_ISU		16
>>>>>> +#define R9A07G043_PD_CRU		17
>>>>>> +#define R9A07G043_PD_LCDC		18
>>>>>> +#define R9A07G043_PD_SSI0		19
>>>>>> +#define R9A07G043_PD_SSI1		20
>>>>>> +#define R9A07G043_PD_SSI2		21
>>>>>> +#define R9A07G043_PD_SSI3		22
>>>>>> +#define R9A07G043_PD_SRC		23
>>>>>> +#define R9A07G043_PD_USB0		24
>>>>>> +#define R9A07G043_PD_USB1		25
>>>>>> +#define R9A07G043_PD_USB_PHY		26
>>>>>> +#define R9A07G043_PD_ETHER0		27
>>>>>> +#define R9A07G043_PD_ETHER1		28
>>>>>> +#define R9A07G043_PD_I2C0		29
>>>>>> +#define R9A07G043_PD_I2C1		30
>>>>>> +#define R9A07G043_PD_I2C2		31
>>>>>> +#define R9A07G043_PD_I2C3		32
>>>>>> +#define R9A07G043_PD_SCIF0		33
>>>>>> +#define R9A07G043_PD_SCIF1		34
>>>>>> +#define R9A07G043_PD_SCIF2		35
>>>>>> +#define R9A07G043_PD_SCIF3		36
>>>>>> +#define R9A07G043_PD_SCIF4		37
>>>>>> +#define R9A07G043_PD_SCI0		38
>>>>>> +#define R9A07G043_PD_SCI1		39
>>>>>> +#define R9A07G043_PD_IRDA		40
>>>>>> +#define R9A07G043_PD_RSPI0		41
>>>>>> +#define R9A07G043_PD_RSPI1		42
>>>>>> +#define R9A07G043_PD_RSPI2		43
>>>>>> +#define R9A07G043_PD_CANFD		44
>>>>>> +#define R9A07G043_PD_ADC		45
>>>>>> +#define R9A07G043_PD_TSU		46
>>>>>
>>>>> Not sure from "Table 42.3 Registers for Module Standby Mode"
>>>>>
>>>>> Power domain ID has to be based on CPG_BUS_***_MSTOP or
>>>>> CPG_CLKON_*** As former reduces number of IDs??
>>>>
>>>> If I understand correctly your point here, you want me to describe PM
>>>> domain in DT with something like:
>>>>
>>>> power-domains = <&cpg CPG_BUS_X_MSTOP>;
>>>
>>> MSTOP bits are distinct for each IP.
>>>
>>> <&cpg CPG_BUS_MCPU1_MSTOP x>; x =1..9
>>>
>>> 2=MTU IP
>>>
>>> 4= GPT
>>>
>>> etc...
>>>
>>> Is it something work??
>>
>> It might work. But:
>>
>> - you have to consider that some IPs have more than one MSTOP bit, thus,
>> do
>>   we want to uniquely identify these with all MSTOP bits (thus the 2nd
>> cell
>>   being a bitmask) or only one is enough?
> 
> We can have an encoding in that case 8:16 24 bit entries

I consider this complicates the bindings. I don't consider this is the way
going forward. But I may be wrong. I'll let Geert to give his opinion on it
and change it afterwards, if any.

> 
>> - some HW blocks (e.g. OTFDE_DDR) have no MSTOP bits associated (as of my
>>   current research), so, only PWRDN
> 
> Why do we want to add power domain support for DDR?

To power it up (in case bootloader does any settings in this area) such
that the system will not block while booting.

It is explained in cover letter:

The current DT
bindings were updated with module IDs for the modules listed in tables
with name "Registers for Module Standby Mode" (see HW manual) exception
being RZ/G3S where, *due to the power down functionality*, the DDR,
TZCDDR, OTFDE_DDR were also added, to avoid system being blocked due
to the following lines of code from patch 7/17.

+       /* Prepare for power down the BUSes in power down mode. */
+       if (info->pm_domain_pwrdn_mstop)
+               writel(CPG_PWRDN_MSTOP_ENABLE, priv->base + CPG_PWRDN_MSTOP);

> 
>> - some HW blocks have both MSTOP and PWRDN
> 
> That will be an array right?

I'm not sure what you want to say here.

> 
>> - if future hardware implementation will spread the MSTOP bits for one IP
>>   to more than one register then this proposal will not work
> 
> That will be an array right?

Same here.

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
Biju Das Feb. 12, 2024, 8:59 a.m. UTC | #3 
Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Monday, February 12, 2024 8:02 AM
> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
> domain IDs
> 
> Hi, Biju,
> 
> On 08.02.2024 21:20, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >> Sent: Thursday, February 8, 2024 4:53 PM
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be;
> >> mturquette@baylibre.com; sboyd@kernel.org; robh@kernel.org;
> >> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> >> magnus.damm@gmail.com; paul.walmsley@sifive.com; palmer@dabbelt.com;
> >> aou@eecs.berkeley.edu
> >> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> >> riscv@lists.infradead.org; Claudiu Beznea
> >> <claudiu.beznea.uj@bp.renesas.com>
> >> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
> >> power domain IDs
> >>
> >>
> >>
> >> On 08.02.2024 18:28, Biju Das wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >>>> Sent: Thursday, February 8, 2024 3:46 PM
> >>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
> >>>> power domain IDs
> >>>>
> >>>> Hi, Biju,
> >>>>
> >>>> On 08.02.2024 16:30, Biju Das wrote:
> >>>>> Hi Claudiu,
> >>>>>
> >>>>> Thanks for the patch.
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
> >>>>>> Sent: Thursday, February 8, 2024 12:43 PM
> >>>>>> Subject: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
> >>>>>> power domain IDs
> >>>>>>
> >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>
> >>>>>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
> >>>>>>
> >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>> ---
> >>>>>>  include/dt-bindings/clock/r9a07g043-cpg.h | 48
> >>>>>> +++++++++++++++++++++++
> >>>>>>  1 file changed, 48 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/dt-bindings/clock/r9a07g043-cpg.h
> >>>>>> b/include/dt- bindings/clock/r9a07g043-cpg.h index
> >>>>>> 77cde8effdc7..eabfeec7ac37
> >>>>>> 100644
> >>>>>> --- a/include/dt-bindings/clock/r9a07g043-cpg.h
> >>>>>> +++ b/include/dt-bindings/clock/r9a07g043-cpg.h
> >>>>>> @@ -200,5 +200,53 @@
> >>>>>>  #define R9A07G043_AX45MP_CORE0_RESETN	78	/* RZ/Five Only */
> >>>>>>  #define R9A07G043_IAX45_RESETN		79	/* RZ/Five Only */
> >>>>>>
> >>>>>> +/* Power domain IDs. */
> >>>>>> +#define R9A07G043_PD_ALWAYS_ON		0
> >>>>>> +#define R9A07G043_PD_GIC		1
> >>>>>> +#define R9A07G043_PD_IA55		2
> >>>>>> +#define R9A07G043_PD_MHU		3
> >>>>>> +#define R9A07G043_PD_CORESIGHT		4
> >>>>>> +#define R9A07G043_PD_SYC		5
> >>>>>> +#define R9A07G043_PD_DMAC		6
> >>>>>> +#define R9A07G043_PD_GTM0		7
> >>>>>> +#define R9A07G043_PD_GTM1		8
> >>>>>> +#define R9A07G043_PD_GTM2		9
> >>>>>> +#define R9A07G043_PD_MTU		10
> >>>>>> +#define R9A07G043_PD_POE3		11
> >>>>>> +#define R9A07G043_PD_WDT0		12
> >>>>>> +#define R9A07G043_PD_SPI		13
> >>>>>> +#define R9A07G043_PD_SDHI0		14
> >>>>>> +#define R9A07G043_PD_SDHI1		15
> >>>>>> +#define R9A07G043_PD_ISU		16
> >>>>>> +#define R9A07G043_PD_CRU		17
> >>>>>> +#define R9A07G043_PD_LCDC		18
> >>>>>> +#define R9A07G043_PD_SSI0		19
> >>>>>> +#define R9A07G043_PD_SSI1		20
> >>>>>> +#define R9A07G043_PD_SSI2		21
> >>>>>> +#define R9A07G043_PD_SSI3		22
> >>>>>> +#define R9A07G043_PD_SRC		23
> >>>>>> +#define R9A07G043_PD_USB0		24
> >>>>>> +#define R9A07G043_PD_USB1		25
> >>>>>> +#define R9A07G043_PD_USB_PHY		26
> >>>>>> +#define R9A07G043_PD_ETHER0		27
> >>>>>> +#define R9A07G043_PD_ETHER1		28
> >>>>>> +#define R9A07G043_PD_I2C0		29
> >>>>>> +#define R9A07G043_PD_I2C1		30
> >>>>>> +#define R9A07G043_PD_I2C2		31
> >>>>>> +#define R9A07G043_PD_I2C3		32
> >>>>>> +#define R9A07G043_PD_SCIF0		33
> >>>>>> +#define R9A07G043_PD_SCIF1		34
> >>>>>> +#define R9A07G043_PD_SCIF2		35
> >>>>>> +#define R9A07G043_PD_SCIF3		36
> >>>>>> +#define R9A07G043_PD_SCIF4		37
> >>>>>> +#define R9A07G043_PD_SCI0		38
> >>>>>> +#define R9A07G043_PD_SCI1		39
> >>>>>> +#define R9A07G043_PD_IRDA		40
> >>>>>> +#define R9A07G043_PD_RSPI0		41
> >>>>>> +#define R9A07G043_PD_RSPI1		42
> >>>>>> +#define R9A07G043_PD_RSPI2		43
> >>>>>> +#define R9A07G043_PD_CANFD		44
> >>>>>> +#define R9A07G043_PD_ADC		45
> >>>>>> +#define R9A07G043_PD_TSU		46
> >>>>>
> >>>>> Not sure from "Table 42.3 Registers for Module Standby Mode"
> >>>>>
> >>>>> Power domain ID has to be based on CPG_BUS_***_MSTOP or
> >>>>> CPG_CLKON_*** As former reduces number of IDs??
> >>>>
> >>>> If I understand correctly your point here, you want me to describe
> >>>> PM domain in DT with something like:
> >>>>
> >>>> power-domains = <&cpg CPG_BUS_X_MSTOP>;
> >>>
> >>> MSTOP bits are distinct for each IP.
> >>>
> >>> <&cpg CPG_BUS_MCPU1_MSTOP x>; x =1..9
> >>>
> >>> 2=MTU IP
> >>>
> >>> 4= GPT
> >>>
> >>> etc...
> >>>
> >>> Is it something work??
> >>
> >> It might work. But:
> >>
> >> - you have to consider that some IPs have more than one MSTOP bit,
> >> thus, do
> >>   we want to uniquely identify these with all MSTOP bits (thus the
> >> 2nd cell
> >>   being a bitmask) or only one is enough?
> >
> > We can have an encoding in that case 8:16 24 bit entries
> 
> I consider this complicates the bindings. I don't consider this is the way
> going forward. But I may be wrong. I'll let Geert to give his opinion on
> it and change it afterwards, if any.
> 
> >
> >> - some HW blocks (e.g. OTFDE_DDR) have no MSTOP bits associated (as of
> my
> >>   current research), so, only PWRDN
> >
> > Why do we want to add power domain support for DDR?
> 
> To power it up (in case bootloader does any settings in this area) such
> that the system will not block while booting.

DDR is enabled by TF_A and is not touched by linux, so why are we adding
Power domain at all in first place. TZC DDR is not accessible in normal world.

So if you don't add DDR power domains, linux doesn't know about any thing about
and it should work like current case.

> 
> It is explained in cover letter:
> 
> The current DT
> bindings were updated with module IDs for the modules listed in tables
> with name "Registers for Module Standby Mode" (see HW manual) exception
> being RZ/G3S where, *due to the power down functionality*, the DDR,
> TZCDDR, OTFDE_DDR were also added, to avoid system being blocked due to
> the following lines of code from patch 7/17.
> 
> +       /* Prepare for power down the BUSes in power down mode. */
> +       if (info->pm_domain_pwrdn_mstop)
> +               writel(CPG_PWRDN_MSTOP_ENABLE, priv->base +
> + CPG_PWRDN_MSTOP);
> 
> >
> >> - some HW blocks have both MSTOP and PWRDN
> >
> > That will be an array right?
> 
> I'm not sure what you want to say here.

This has to be an array PM domains(multi PM domain) like clocks?

Or 

It can be  handled as sibliling power domain like sibling clocks in RZ/G2L Gbether.

Cheers,
Biju
claudiu beznea Feb. 12, 2024, 10:17 a.m. UTC | #4 
On 12.02.2024 10:59, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Monday, February 12, 2024 8:02 AM
>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
>> domain IDs
>>
>> Hi, Biju,
>>
>> On 08.02.2024 21:20, Biju Das wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>>> Sent: Thursday, February 8, 2024 4:53 PM
>>>> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be;
>>>> mturquette@baylibre.com; sboyd@kernel.org; robh@kernel.org;
>>>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>>> magnus.damm@gmail.com; paul.walmsley@sifive.com; palmer@dabbelt.com;
>>>> aou@eecs.berkeley.edu
>>>> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org;
>>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>>>> riscv@lists.infradead.org; Claudiu Beznea
>>>> <claudiu.beznea.uj@bp.renesas.com>
>>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
>>>> power domain IDs
>>>>
>>>>
>>>>
>>>> On 08.02.2024 18:28, Biju Das wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>>>>> Sent: Thursday, February 8, 2024 3:46 PM
>>>>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
>>>>>> power domain IDs
>>>>>>
>>>>>> Hi, Biju,
>>>>>>
>>>>>> On 08.02.2024 16:30, Biju Das wrote:
>>>>>>> Hi Claudiu,
>>>>>>>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>>>>>> Sent: Thursday, February 8, 2024 12:43 PM
>>>>>>>> Subject: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
>>>>>>>> power domain IDs
>>>>>>>>
>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>
>>>>>>>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
>>>>>>>>
>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>> ---
>>>>>>>>  include/dt-bindings/clock/r9a07g043-cpg.h | 48
>>>>>>>> +++++++++++++++++++++++
>>>>>>>>  1 file changed, 48 insertions(+)
[ ... ]

>>>>>>>> +#define R9A07G043_PD_TSU		46
>>>>>>>
>>>>>>> Not sure from "Table 42.3 Registers for Module Standby Mode"
>>>>>>>
>>>>>>> Power domain ID has to be based on CPG_BUS_***_MSTOP or
>>>>>>> CPG_CLKON_*** As former reduces number of IDs??
>>>>>>
>>>>>> If I understand correctly your point here, you want me to describe
>>>>>> PM domain in DT with something like:
>>>>>>
>>>>>> power-domains = <&cpg CPG_BUS_X_MSTOP>;
>>>>>
>>>>> MSTOP bits are distinct for each IP.
>>>>>
>>>>> <&cpg CPG_BUS_MCPU1_MSTOP x>; x =1..9
>>>>>
>>>>> 2=MTU IP
>>>>>
>>>>> 4= GPT
>>>>>
>>>>> etc...
>>>>>
>>>>> Is it something work??
>>>>
>>>> It might work. But:
>>>>
>>>> - you have to consider that some IPs have more than one MSTOP bit,
>>>> thus, do
>>>>   we want to uniquely identify these with all MSTOP bits (thus the
>>>> 2nd cell
>>>>   being a bitmask) or only one is enough?
>>>
>>> We can have an encoding in that case 8:16 24 bit entries
>>
>> I consider this complicates the bindings. I don't consider this is the way
>> going forward. But I may be wrong. I'll let Geert to give his opinion on
>> it and change it afterwards, if any.
>>
>>>
>>>> - some HW blocks (e.g. OTFDE_DDR) have no MSTOP bits associated (as of
>> my
>>>>   current research), so, only PWRDN
>>>
>>> Why do we want to add power domain support for DDR?
>>
>> To power it up (in case bootloader does any settings in this area) such
>> that the system will not block while booting.
> 
> DDR is enabled by TF_A and is not touched by linux, so why are we adding
> Power domain at all in first place. TZC DDR is not accessible in normal world.
> 
> So if you don't add DDR power domains, linux doesn't know about any thing about
> and it should work like current case.

It is related to the way MSTOP and PWRDN hardware features works together.
PWRDN allows you to save more power by setting IP specific bits in this
registers after you set the MSTOP.

OFTDE_DDR and TZCDDR have PWRDN bits dedicated as well as other IPs (e.g.
serial, ethernet, etc) in CPG_PWRDN_IP2. Setting CPG_PWRDN_MSTOP_ENABLE to
CPG_PWRDN_MSTOP applies the power down for the IPs specified in
CPG_PWRDN_{IP1, IP2}.

It may happen (as in my case) to have a bootloader that sets all the bits
in CPG_PWRD_{IP1,IP2}.

If you want to save power for the other IPs listed in CPG_PWRD_{IP1,IP2}
you have to instantiate power domains for the blocks that you don't want to
be powered down due to setting CPG_PWRDN_MSTOP_ENABLE to CPG_PWRDN_MSTOP,
to power them up. Otherwise the system will block when setting
CPG_PWRDN_MSTOP_ENABLE to CPG_PWRDN_MSTOP (if bootloaders previously did
some settings in the above specified registers).

Hope it was clear.

Thank you,
Claudiu Beznea

> 
>>
>> It is explained in cover letter:
>>
>> The current DT
>> bindings were updated with module IDs for the modules listed in tables
>> with name "Registers for Module Standby Mode" (see HW manual) exception
>> being RZ/G3S where, *due to the power down functionality*, the DDR,
>> TZCDDR, OTFDE_DDR were also added, to avoid system being blocked due to
>> the following lines of code from patch 7/17.
>>
>> +       /* Prepare for power down the BUSes in power down mode. */
>> +       if (info->pm_domain_pwrdn_mstop)
>> +               writel(CPG_PWRDN_MSTOP_ENABLE, priv->base +
>> + CPG_PWRDN_MSTOP);
>>
>>>
>>>> - some HW blocks have both MSTOP and PWRDN
>>>
>>> That will be an array right?
>>
>> I'm not sure what you want to say here.
> 
> This has to be an array PM domains(multi PM domain) like clocks?
> 
> Or 
> 
> It can be  handled as sibliling power domain like sibling clocks in RZ/G2L Gbether.
> 
> Cheers,
> Biju
Biju Das Feb. 12, 2024, 10:32 a.m. UTC | #5 
Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Monday, February 12, 2024 10:17 AM
> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
> domain IDs
> 
> 
> 
> On 12.02.2024 10:59, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >> Sent: Monday, February 12, 2024 8:02 AM
> >> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
> >> power domain IDs
> >>
> >> Hi, Biju,
> >>
> >> On 08.02.2024 21:20, Biju Das wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >>>> Sent: Thursday, February 8, 2024 4:53 PM
> >>>> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be;
> >>>> mturquette@baylibre.com; sboyd@kernel.org; robh@kernel.org;
> >>>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> >>>> magnus.damm@gmail.com; paul.walmsley@sifive.com;
> >>>> palmer@dabbelt.com; aou@eecs.berkeley.edu
> >>>> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org;
> >>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> >>>> riscv@lists.infradead.org; Claudiu Beznea
> >>>> <claudiu.beznea.uj@bp.renesas.com>
> >>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
> >>>> power domain IDs
> >>>>
> >>>>
> >>>>
> >>>> On 08.02.2024 18:28, Biju Das wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >>>>>> Sent: Thursday, February 8, 2024 3:46 PM
> >>>>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
> >>>>>> power domain IDs
> >>>>>>
> >>>>>> Hi, Biju,
> >>>>>>
> >>>>>> On 08.02.2024 16:30, Biju Das wrote:
> >>>>>>> Hi Claudiu,
> >>>>>>>
> >>>>>>> Thanks for the patch.
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
> >>>>>>>> Sent: Thursday, February 8, 2024 12:43 PM
> >>>>>>>> Subject: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
> >>>>>>>> power domain IDs
> >>>>>>>>
> >>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>>
> >>>>>>>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Claudiu Beznea
> >>>>>>>> <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>> ---
> >>>>>>>>  include/dt-bindings/clock/r9a07g043-cpg.h | 48
> >>>>>>>> +++++++++++++++++++++++
> >>>>>>>>  1 file changed, 48 insertions(+)
> [ ... ]
> 
> >>>>>>>> +#define R9A07G043_PD_TSU		46
> >>>>>>>
> >>>>>>> Not sure from "Table 42.3 Registers for Module Standby Mode"
> >>>>>>>
> >>>>>>> Power domain ID has to be based on CPG_BUS_***_MSTOP or
> >>>>>>> CPG_CLKON_*** As former reduces number of IDs??
> >>>>>>
> >>>>>> If I understand correctly your point here, you want me to
> >>>>>> describe PM domain in DT with something like:
> >>>>>>
> >>>>>> power-domains = <&cpg CPG_BUS_X_MSTOP>;
> >>>>>
> >>>>> MSTOP bits are distinct for each IP.
> >>>>>
> >>>>> <&cpg CPG_BUS_MCPU1_MSTOP x>; x =1..9
> >>>>>
> >>>>> 2=MTU IP
> >>>>>
> >>>>> 4= GPT
> >>>>>
> >>>>> etc...
> >>>>>
> >>>>> Is it something work??
> >>>>
> >>>> It might work. But:
> >>>>
> >>>> - you have to consider that some IPs have more than one MSTOP bit,
> >>>> thus, do
> >>>>   we want to uniquely identify these with all MSTOP bits (thus the
> >>>> 2nd cell
> >>>>   being a bitmask) or only one is enough?
> >>>
> >>> We can have an encoding in that case 8:16 24 bit entries
> >>
> >> I consider this complicates the bindings. I don't consider this is
> >> the way going forward. But I may be wrong. I'll let Geert to give his
> >> opinion on it and change it afterwards, if any.
> >>
> >>>
> >>>> - some HW blocks (e.g. OTFDE_DDR) have no MSTOP bits associated (as
> >>>> of
> >> my
> >>>>   current research), so, only PWRDN
> >>>
> >>> Why do we want to add power domain support for DDR?
> >>
> >> To power it up (in case bootloader does any settings in this area)
> >> such that the system will not block while booting.
> >
> > DDR is enabled by TF_A and is not touched by linux, so why are we
> > adding Power domain at all in first place. TZC DDR is not accessible in
> normal world.
> >
> > So if you don't add DDR power domains, linux doesn't know about any
> > thing about and it should work like current case.
> 
> It is related to the way MSTOP and PWRDN hardware features works together.
> PWRDN allows you to save more power by setting IP specific bits in this
> registers after you set the MSTOP.
> 
> OFTDE_DDR and TZCDDR have PWRDN bits dedicated as well as other IPs (e.g.
> serial, ethernet, etc) in CPG_PWRDN_IP2. Setting CPG_PWRDN_MSTOP_ENABLE to
> CPG_PWRDN_MSTOP applies the power down for the IPs specified in
> CPG_PWRDN_{IP1, IP2}.
> 
> It may happen (as in my case) to have a bootloader that sets all the bits
> in CPG_PWRD_{IP1,IP2}.
> 
> If you want to save power for the other IPs listed in CPG_PWRD_{IP1,IP2}
> you have to instantiate power domains for the blocks that you don't want
> to be powered down due to setting CPG_PWRDN_MSTOP_ENABLE to
> CPG_PWRDN_MSTOP, to power them up. Otherwise the system will block when
> setting CPG_PWRDN_MSTOP_ENABLE to CPG_PWRDN_MSTOP (if bootloaders
> previously did some settings in the above specified registers).
> 
> Hope it was clear.

OK got it, Basically you are saying linux PM messes up the CPG_PWRDN_IP2 bits({0,1}
set by TF_A for DDR/TZC DDR and leading to boot failure.

Cheers,
Biju
claudiu beznea Feb. 12, 2024, 11:08 a.m. UTC | #6 
On 12.02.2024 10:59, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Monday, February 12, 2024 8:02 AM
>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add power
>> domain IDs
>>
>> Hi, Biju,
>>
>> On 08.02.2024 21:20, Biju Das wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>>> Sent: Thursday, February 8, 2024 4:53 PM
>>>> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be;
>>>> mturquette@baylibre.com; sboyd@kernel.org; robh@kernel.org;
>>>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>>>> magnus.damm@gmail.com; paul.walmsley@sifive.com; palmer@dabbelt.com;
>>>> aou@eecs.berkeley.edu
>>>> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org;
>>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>>>> riscv@lists.infradead.org; Claudiu Beznea
>>>> <claudiu.beznea.uj@bp.renesas.com>
>>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
>>>> power domain IDs
>>>>
>>>>
>>>>
>>>> On 08.02.2024 18:28, Biju Das wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>>>>> Sent: Thursday, February 8, 2024 3:46 PM
>>>>>> Subject: Re: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
>>>>>> power domain IDs
>>>>>>
>>>>>> Hi, Biju,
>>>>>>
>>>>>> On 08.02.2024 16:30, Biju Das wrote:
>>>>>>> Hi Claudiu,
>>>>>>>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>>>>>> Sent: Thursday, February 8, 2024 12:43 PM
>>>>>>>> Subject: [PATCH 01/17] dt-bindings: clock: r9a07g043-cpg: Add
>>>>>>>> power domain IDs
>>>>>>>>
>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>
>>>>>>>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
>>>>>>>>
>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>> ---
>>>>>>>>  include/dt-bindings/clock/r9a07g043-cpg.h | 48
>>>>>>>> +++++++++++++++++++++++
>>>>>>>>  1 file changed, 48 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/dt-bindings/clock/r9a07g043-cpg.h
>>>>>>>> b/include/dt- bindings/clock/r9a07g043-cpg.h index
>>>>>>>> 77cde8effdc7..eabfeec7ac37
>>>>>>>> 100644
>>>>>>>> --- a/include/dt-bindings/clock/r9a07g043-cpg.h
>>>>>>>> +++ b/include/dt-bindings/clock/r9a07g043-cpg.h
>>>>>>>> @@ -200,5 +200,53 @@
>>>>>>>>  #define R9A07G043_AX45MP_CORE0_RESETN	78	/* RZ/Five Only */
>>>>>>>>  #define R9A07G043_IAX45_RESETN		79	/* RZ/Five Only */
>>>>>>>>
>>>>>>>> +/* Power domain IDs. */
>>>>>>>> +#define R9A07G043_PD_ALWAYS_ON		0
>>>>>>>> +#define R9A07G043_PD_GIC		1
>>>>>>>> +#define R9A07G043_PD_IA55		2
>>>>>>>> +#define R9A07G043_PD_MHU		3
>>>>>>>> +#define R9A07G043_PD_CORESIGHT		4
>>>>>>>> +#define R9A07G043_PD_SYC		5
>>>>>>>> +#define R9A07G043_PD_DMAC		6
>>>>>>>> +#define R9A07G043_PD_GTM0		7
>>>>>>>> +#define R9A07G043_PD_GTM1		8
>>>>>>>> +#define R9A07G043_PD_GTM2		9
>>>>>>>> +#define R9A07G043_PD_MTU		10
>>>>>>>> +#define R9A07G043_PD_POE3		11
>>>>>>>> +#define R9A07G043_PD_WDT0		12
>>>>>>>> +#define R9A07G043_PD_SPI		13
>>>>>>>> +#define R9A07G043_PD_SDHI0		14
>>>>>>>> +#define R9A07G043_PD_SDHI1		15
>>>>>>>> +#define R9A07G043_PD_ISU		16
>>>>>>>> +#define R9A07G043_PD_CRU		17
>>>>>>>> +#define R9A07G043_PD_LCDC		18
>>>>>>>> +#define R9A07G043_PD_SSI0		19
>>>>>>>> +#define R9A07G043_PD_SSI1		20
>>>>>>>> +#define R9A07G043_PD_SSI2		21
>>>>>>>> +#define R9A07G043_PD_SSI3		22
>>>>>>>> +#define R9A07G043_PD_SRC		23
>>>>>>>> +#define R9A07G043_PD_USB0		24
>>>>>>>> +#define R9A07G043_PD_USB1		25
>>>>>>>> +#define R9A07G043_PD_USB_PHY		26
>>>>>>>> +#define R9A07G043_PD_ETHER0		27
>>>>>>>> +#define R9A07G043_PD_ETHER1		28
>>>>>>>> +#define R9A07G043_PD_I2C0		29
>>>>>>>> +#define R9A07G043_PD_I2C1		30
>>>>>>>> +#define R9A07G043_PD_I2C2		31
>>>>>>>> +#define R9A07G043_PD_I2C3		32
>>>>>>>> +#define R9A07G043_PD_SCIF0		33
>>>>>>>> +#define R9A07G043_PD_SCIF1		34
>>>>>>>> +#define R9A07G043_PD_SCIF2		35
>>>>>>>> +#define R9A07G043_PD_SCIF3		36
>>>>>>>> +#define R9A07G043_PD_SCIF4		37
>>>>>>>> +#define R9A07G043_PD_SCI0		38
>>>>>>>> +#define R9A07G043_PD_SCI1		39
>>>>>>>> +#define R9A07G043_PD_IRDA		40
>>>>>>>> +#define R9A07G043_PD_RSPI0		41
>>>>>>>> +#define R9A07G043_PD_RSPI1		42
>>>>>>>> +#define R9A07G043_PD_RSPI2		43
>>>>>>>> +#define R9A07G043_PD_CANFD		44
>>>>>>>> +#define R9A07G043_PD_ADC		45
>>>>>>>> +#define R9A07G043_PD_TSU		46
>>>>>>>
>>>>>>> Not sure from "Table 42.3 Registers for Module Standby Mode"
>>>>>>>
>>>>>>> Power domain ID has to be based on CPG_BUS_***_MSTOP or
>>>>>>> CPG_CLKON_*** As former reduces number of IDs??
>>>>>>
>>>>>> If I understand correctly your point here, you want me to describe
>>>>>> PM domain in DT with something like:
>>>>>>
>>>>>> power-domains = <&cpg CPG_BUS_X_MSTOP>;
>>>>>
>>>>> MSTOP bits are distinct for each IP.
>>>>>
>>>>> <&cpg CPG_BUS_MCPU1_MSTOP x>; x =1..9
>>>>>
>>>>> 2=MTU IP
>>>>>
>>>>> 4= GPT
>>>>>
>>>>> etc...
>>>>>
>>>>> Is it something work??
>>>>
>>>> It might work. But:
>>>>
>>>> - you have to consider that some IPs have more than one MSTOP bit,
>>>> thus, do
>>>>   we want to uniquely identify these with all MSTOP bits (thus the
>>>> 2nd cell
>>>>   being a bitmask) or only one is enough?
>>>
>>> We can have an encoding in that case 8:16 24 bit entries
>>
>> I consider this complicates the bindings. I don't consider this is the way
>> going forward. But I may be wrong. I'll let Geert to give his opinion on
>> it and change it afterwards, if any.
>>
>>>
>>>> - some HW blocks (e.g. OTFDE_DDR) have no MSTOP bits associated (as of
>> my
>>>>   current research), so, only PWRDN
>>>
>>> Why do we want to add power domain support for DDR?
>>
>> To power it up (in case bootloader does any settings in this area) such
>> that the system will not block while booting.
> 
> DDR is enabled by TF_A and is not touched by linux, so why are we adding
> Power domain at all in first place. TZC DDR is not accessible in normal world.
> 
> So if you don't add DDR power domains, linux doesn't know about any thing about
> and it should work like current case.
> 
>>
>> It is explained in cover letter:
>>
>> The current DT
>> bindings were updated with module IDs for the modules listed in tables
>> with name "Registers for Module Standby Mode" (see HW manual) exception
>> being RZ/G3S where, *due to the power down functionality*, the DDR,
>> TZCDDR, OTFDE_DDR were also added, to avoid system being blocked due to
>> the following lines of code from patch 7/17.
>>
>> +       /* Prepare for power down the BUSes in power down mode. */
>> +       if (info->pm_domain_pwrdn_mstop)
>> +               writel(CPG_PWRDN_MSTOP_ENABLE, priv->base +
>> + CPG_PWRDN_MSTOP);
>>
>>>
>>>> - some HW blocks have both MSTOP and PWRDN
>>>
>>> That will be an array right?
>>
>> I'm not sure what you want to say here.
> 
> This has to be an array PM domains(multi PM domain) like clocks?

Forgot to reply to this...

Yes, this should work for IPs having both MSTOP and PWRDN. It is an
alternative to the current implementation. I kept both MSTOP and PWRDN
under the control of the same PM domain in the current implementation.

But if future hardware implementation will spread the MSTOP bits for one IP
to more than one register (I don't know if this is likely to happen but it
may worth considering) then multiple MSTOP bits for the same power domain
cannot be handled by this approach and describing the domain with register
offset and bitmask.

> 
> Or 
> 
> It can be  handled as sibliling power domain like sibling clocks in RZ/G2L Gbether.

Kind of this implementation was proposed initially (linking the MSTOP to
the IP clocks inside the clock driver).

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
Geert Uytterhoeven Feb. 16, 2024, 2:01 p.m. UTC | #7 
Hi Claudiu,

On Thu, Feb 8, 2024 at 1:43 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/include/dt-bindings/clock/r9a07g043-cpg.h
> +++ b/include/dt-bindings/clock/r9a07g043-cpg.h
> @@ -200,5 +200,53 @@
>  #define R9A07G043_AX45MP_CORE0_RESETN  78      /* RZ/Five Only */
>  #define R9A07G043_IAX45_RESETN         79      /* RZ/Five Only */
>
> +/* Power domain IDs. */
> +#define R9A07G043_PD_ALWAYS_ON         0
> +#define R9A07G043_PD_GIC               1

As this file is shared between RZ/G2UL and RZ/Five, R9A07G043_PD_GIC
needs an "/* RZ/G2UL Only */" comment

> +#define R9A07G043_PD_IA55              2
> +#define R9A07G043_PD_MHU               3
> +#define R9A07G043_PD_CORESIGHT         4
> +#define R9A07G043_PD_SYC               5

Likewise for the four above.

> +#define R9A07G043_PD_DMAC              6
> +#define R9A07G043_PD_GTM0              7
> +#define R9A07G043_PD_GTM1              8
> +#define R9A07G043_PD_GTM2              9
> +#define R9A07G043_PD_MTU               10
> +#define R9A07G043_PD_POE3              11
> +#define R9A07G043_PD_WDT0              12
> +#define R9A07G043_PD_SPI               13
> +#define R9A07G043_PD_SDHI0             14
> +#define R9A07G043_PD_SDHI1             15
> +#define R9A07G043_PD_ISU               16
> +#define R9A07G043_PD_CRU               17
> +#define R9A07G043_PD_LCDC              18

Likewise for the three above.

> +#define R9A07G043_PD_SSI0              19
> +#define R9A07G043_PD_SSI1              20
> +#define R9A07G043_PD_SSI2              21
> +#define R9A07G043_PD_SSI3              22
> +#define R9A07G043_PD_SRC               23
> +#define R9A07G043_PD_USB0              24
> +#define R9A07G043_PD_USB1              25
> +#define R9A07G043_PD_USB_PHY           26
> +#define R9A07G043_PD_ETHER0            27
> +#define R9A07G043_PD_ETHER1            28
> +#define R9A07G043_PD_I2C0              29
> +#define R9A07G043_PD_I2C1              30
> +#define R9A07G043_PD_I2C2              31
> +#define R9A07G043_PD_I2C3              32
> +#define R9A07G043_PD_SCIF0             33
> +#define R9A07G043_PD_SCIF1             34
> +#define R9A07G043_PD_SCIF2             35
> +#define R9A07G043_PD_SCIF3             36
> +#define R9A07G043_PD_SCIF4             37
> +#define R9A07G043_PD_SCI0              38
> +#define R9A07G043_PD_SCI1              39
> +#define R9A07G043_PD_IRDA              40
> +#define R9A07G043_PD_RSPI0             41
> +#define R9A07G043_PD_RSPI1             42
> +#define R9A07G043_PD_RSPI2             43
> +#define R9A07G043_PD_CANFD             44
> +#define R9A07G043_PD_ADC               45
> +#define R9A07G043_PD_TSU               46
>
>  #endif /* __DT_BINDINGS_CLOCK_R9A07G043_CPG_H__ */

In addition, you need definitions for the modules that are only
present on RZ/Five, e.g.

    #define R9A07G043_PD_PLIC               47    /* RZ/Five Only */

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
claudiu beznea Feb. 19, 2024, 7:36 a.m. UTC | #8 
Hi, Geert,

On 16.02.2024 16:01, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, Feb 8, 2024 at 1:43 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/include/dt-bindings/clock/r9a07g043-cpg.h
>> +++ b/include/dt-bindings/clock/r9a07g043-cpg.h
>> @@ -200,5 +200,53 @@
>>  #define R9A07G043_AX45MP_CORE0_RESETN  78      /* RZ/Five Only */
>>  #define R9A07G043_IAX45_RESETN         79      /* RZ/Five Only */
>>
>> +/* Power domain IDs. */
>> +#define R9A07G043_PD_ALWAYS_ON         0
>> +#define R9A07G043_PD_GIC               1
> 
> As this file is shared between RZ/G2UL and RZ/Five, R9A07G043_PD_GIC
> needs an "/* RZ/G2UL Only */" comment

Ok. I'll re-checked it and update. Likewise for the rest of your below points.

> 
>> +#define R9A07G043_PD_IA55              2
>> +#define R9A07G043_PD_MHU               3
>> +#define R9A07G043_PD_CORESIGHT         4
>> +#define R9A07G043_PD_SYC               5
> 
> Likewise for the four above.
> 
>> +#define R9A07G043_PD_DMAC              6
>> +#define R9A07G043_PD_GTM0              7
>> +#define R9A07G043_PD_GTM1              8
>> +#define R9A07G043_PD_GTM2              9
>> +#define R9A07G043_PD_MTU               10
>> +#define R9A07G043_PD_POE3              11
>> +#define R9A07G043_PD_WDT0              12
>> +#define R9A07G043_PD_SPI               13
>> +#define R9A07G043_PD_SDHI0             14
>> +#define R9A07G043_PD_SDHI1             15
>> +#define R9A07G043_PD_ISU               16
>> +#define R9A07G043_PD_CRU               17
>> +#define R9A07G043_PD_LCDC              18
> 
> Likewise for the three above.
> 
>> +#define R9A07G043_PD_SSI0              19
>> +#define R9A07G043_PD_SSI1              20
>> +#define R9A07G043_PD_SSI2              21
>> +#define R9A07G043_PD_SSI3              22
>> +#define R9A07G043_PD_SRC               23
>> +#define R9A07G043_PD_USB0              24
>> +#define R9A07G043_PD_USB1              25
>> +#define R9A07G043_PD_USB_PHY           26
>> +#define R9A07G043_PD_ETHER0            27
>> +#define R9A07G043_PD_ETHER1            28
>> +#define R9A07G043_PD_I2C0              29
>> +#define R9A07G043_PD_I2C1              30
>> +#define R9A07G043_PD_I2C2              31
>> +#define R9A07G043_PD_I2C3              32
>> +#define R9A07G043_PD_SCIF0             33
>> +#define R9A07G043_PD_SCIF1             34
>> +#define R9A07G043_PD_SCIF2             35
>> +#define R9A07G043_PD_SCIF3             36
>> +#define R9A07G043_PD_SCIF4             37
>> +#define R9A07G043_PD_SCI0              38
>> +#define R9A07G043_PD_SCI1              39
>> +#define R9A07G043_PD_IRDA              40
>> +#define R9A07G043_PD_RSPI0             41
>> +#define R9A07G043_PD_RSPI1             42
>> +#define R9A07G043_PD_RSPI2             43
>> +#define R9A07G043_PD_CANFD             44
>> +#define R9A07G043_PD_ADC               45
>> +#define R9A07G043_PD_TSU               46
>>
>>  #endif /* __DT_BINDINGS_CLOCK_R9A07G043_CPG_H__ */
> 
> In addition, you need definitions for the modules that are only
> present on RZ/Five, e.g.
> 
>     #define R9A07G043_PD_PLIC               47    /* RZ/Five Only */
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
diff mbox series

Patch

diff --git a/include/dt-bindings/clock/r9a07g043-cpg.h b/include/dt-bindings/clock/r9a07g043-cpg.h
index 77cde8effdc7..eabfeec7ac37 100644
--- a/include/dt-bindings/clock/r9a07g043-cpg.h
+++ b/include/dt-bindings/clock/r9a07g043-cpg.h
@@ -200,5 +200,53 @@ 
 #define R9A07G043_AX45MP_CORE0_RESETN	78	/* RZ/Five Only */
 #define R9A07G043_IAX45_RESETN		79	/* RZ/Five Only */
 
+/* Power domain IDs. */
+#define R9A07G043_PD_ALWAYS_ON		0
+#define R9A07G043_PD_GIC		1
+#define R9A07G043_PD_IA55		2
+#define R9A07G043_PD_MHU		3
+#define R9A07G043_PD_CORESIGHT		4
+#define R9A07G043_PD_SYC		5
+#define R9A07G043_PD_DMAC		6
+#define R9A07G043_PD_GTM0		7
+#define R9A07G043_PD_GTM1		8
+#define R9A07G043_PD_GTM2		9
+#define R9A07G043_PD_MTU		10
+#define R9A07G043_PD_POE3		11
+#define R9A07G043_PD_WDT0		12
+#define R9A07G043_PD_SPI		13
+#define R9A07G043_PD_SDHI0		14
+#define R9A07G043_PD_SDHI1		15
+#define R9A07G043_PD_ISU		16
+#define R9A07G043_PD_CRU		17
+#define R9A07G043_PD_LCDC		18
+#define R9A07G043_PD_SSI0		19
+#define R9A07G043_PD_SSI1		20
+#define R9A07G043_PD_SSI2		21
+#define R9A07G043_PD_SSI3		22
+#define R9A07G043_PD_SRC		23
+#define R9A07G043_PD_USB0		24
+#define R9A07G043_PD_USB1		25
+#define R9A07G043_PD_USB_PHY		26
+#define R9A07G043_PD_ETHER0		27
+#define R9A07G043_PD_ETHER1		28
+#define R9A07G043_PD_I2C0		29
+#define R9A07G043_PD_I2C1		30
+#define R9A07G043_PD_I2C2		31
+#define R9A07G043_PD_I2C3		32
+#define R9A07G043_PD_SCIF0		33
+#define R9A07G043_PD_SCIF1		34
+#define R9A07G043_PD_SCIF2		35
+#define R9A07G043_PD_SCIF3		36
+#define R9A07G043_PD_SCIF4		37
+#define R9A07G043_PD_SCI0		38
+#define R9A07G043_PD_SCI1		39
+#define R9A07G043_PD_IRDA		40
+#define R9A07G043_PD_RSPI0		41
+#define R9A07G043_PD_RSPI1		42
+#define R9A07G043_PD_RSPI2		43
+#define R9A07G043_PD_CANFD		44
+#define R9A07G043_PD_ADC		45
+#define R9A07G043_PD_TSU		46
 
 #endif /* __DT_BINDINGS_CLOCK_R9A07G043_CPG_H__ */