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

From: claudiu beznea
Date: Mon Feb 12 2024 - 03:04:02 EST


Hi, Biju,

On 08.02.2024 21:20, Biju Das wrote:
>
>
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@xxxxxxxxx>
>> Sent: Thursday, February 8, 2024 4:53 PM
>> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; geert+renesas@xxxxxxxxx;
>> mturquette@xxxxxxxxxxxx; sboyd@xxxxxxxxxx; robh@xxxxxxxxxx;
>> krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
>> magnus.damm@xxxxxxxxx; paul.walmsley@xxxxxxxxxx; palmer@xxxxxxxxxxx;
>> aou@xxxxxxxxxxxxxxxxx
>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx;
>> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
>> riscv@xxxxxxxxxxxxxxxxxxx; Claudiu Beznea
>> <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>> 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@xxxxxxxxx>
>>>> 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@xxxxxxxxx>
>>>>>> 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@xxxxxxxxxxxxxx>
>>>>>>
>>>>>> Add power domain IDs for RZ/G2UL (R9A07G043) SoC.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> 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