Re: [PATCH/RFC 10/14] dt-bindings: power: Document Renesas R-Car X5H Module Controller

[Marek Vasut <marek.vasut@mailbox.org>]

On 5/8/26 10:26 AM, Geert Uytterhoeven wrote:

Hello Geert,

>> Since there are up to 32 MPDG registers, and 256 resets, can we encode
>> both into a single cell ?
>>
>> (mpdg_register_offset << 16) | (reset_bit_offset << 0)
> 
> We could.  I did consider it (with a shift of 8 cfr. 256 modules),
> but see below...
> 
>> I cannot tell whether this is much better, but it at least ties the PD
>> components (power domain and clock domain) into a single value, which
>> matches reality a bit better. The current split power domain and clock
>> domain description in two cells gives me the illusion that it is
>> possible to mix and match power domains and clock domains in DT
>> description, but in fact the two cells are strongly tied together.
> 
> They are only tied together in the sense that a module (hardware block)
> is part of a power domain, and has module standby (clock) control.
> Some power domains are backed by MDLC hardware registers,
> others are not, hence the need for the additional definitions in
> <dt-bindings/power/renesas,r8a78000-mdlc.h>.
> I am not aware (yet) of modules that are part of a power domain,
> but do not have module standby control. If these exist, we
> need an additional definition (R8A78000_MDLC_MODULE_NONE?) in
> <dt-bindings/power/renesas,r8a78000-mdlc.h>.

Maybe TAUJ3 in AON domain? I think HSCN also has abundance of examples.

> Due to this separation, and due to a possible future need for expansion
> (R8A78000_MDLC_MODULE_NONE, MDLCs with more than 256 modules, ...),
> I went for two cells.

I think I won't push for a single cell either, two cells are OK with me too.

>> If we cannot encode the two into a single cell, maybe we can at least
>> have some sort of macro for this, e.g. this (0xff as no MPDG register
>> bits for this block):
>> #define R8A78000_MDLC_PD_HSCIF0 (0xff << 16) ((0x5 << 4) | (0x3 << 0))
>>
>> What do you think ?
> 
> I (and I believe the DT maintainers) are not so fond of defines for
> numbers that can be (more or less) just read from the documentation.

OK

> (and 0xff should be R8A78000_MDLC_PD_APL?)

I think AON would have to be 0xff , since it is superdomain of APL ?

>>> So perhaps I will clarify like this:
>>>
>>>         - The first power domain specifier cell is the power domain part, and
>>>           must be either the Module Power Domain Gating (MPDG) register index
>>
>> ... for power domains which are backed by MDPG bits, and which can be
>> controlled in that manner ...
> 
> OK.
> 
>>>           (0x00-0x3f) from the datasheet, or a Power Domain number, as defined in
>>>           <dt-bindings/power/renesas,r8a78000-mdlc.h>,
>>
>> ... for power domains which are always on, and for which there are no
>> MPDG bits which can be used to control them ...
> 
> OK,
> 
>>
>>>         - The second power domain specifier cell is the clock domain part, and
> 
> Upon second thought: s/clock domain/module standby/

If you could even mention that this refers to "Module STOP" column bit, 
that would even clearer.

>>>           must be the module number (0x00-0xff), composed of the Module System
>>>           Reset (MSRES) register index in the high nibble, and the Module Reset
>>>           Destination bitfield index in the low nibble.
>>
>> I can understand this.
>>
>>>>> +  '#reset-cells':
>>>>> +    description:
>>>>> +      The single reset specifier cell must be the module number (0x00-0xff).
>>>>> +    const: 1
>>>>
>>>> [...]
>>>>
>>>>> +#ifndef __DT_BINDINGS_POWER_RENESAS_R8A78000_MDLC_H__
>>>>> +#define __DT_BINDINGS_POWER_RENESAS_R8A78000_MDLC_H__
>>>>> +
>>>>> +/* R-Car X5H MDLC Power Domains */
>>>>> +
>>>>> +#define R8A78000_MDLC_PD_AON                 0x40
>>>>> +#define R8A78000_MDLC_PD_SCP                 0x41
>>>>> +#define R8A78000_MDLC_PD_APL                 0x42
>>>>> +#define R8A78000_MDLC_PD_CMN                 0x43
>>>>> +#define R8A78000_MDLC_PD_ACL                 0x44
>>>> ... what do these numbers represent ? Shouldn't those be register
>>>> offsets from MDLC MPDG00 according to power-domain-cells ?
>>>
>>> These are Power Domains that are not backed by any of the 64 Module
>>> Power Domain Gating (MPDG) registers in MDLC blocks.
>>
>> I suspect that might not be entirely correct for all of them, please
>> read on and see CMN below.
> 
> Thanks, looks like R8A78000_MDLC_PD_CMN should be dropped.
> 
>> Let's take PD_AC00 , AP core 0 , as a domain of interest. My
>> understanding is, that the domain structure for PD_AC00 looks as follows:
>>
>> PD_AON {
>>     PD_SCP { };
>>     PD_APL {
>>       hierarchy is SYSSS
>>       always-power-on
>>       PD_CMN {
>>         hierarchy is CMNN
>>         power-gating-bit is MDLC_CMNN 20
>>         PD_APU0 {
>>           hierarchy is SYSSS
>>           power-gating is done by APMU
>>           PD_ACL0 {
>>             hierarchy is CMNN
>>             power-gating-bit is MDLC_CMNN 16
>>             PD_AC00 {
>>               hierarchy is CMNN
>>               power-gating-bit is MDLC_CMNN 0
>>             };
>>             ...
>>           };
>>           ...
>>         };
>>         ...
>>       };
>>       ...
>>       PD_HSCIF0 {
>>         hierarchy is PERW
>>         power-gating-bit is MDLC_PERW 23
>>       };
>>     };
>>     ...
>> };
>>
>> With this in mind, I think CPU 0 DT node should refer to the PD_AC00
>> power domain this way:
>>
>> cpu@0 {
>>     ...
>>     power-domains = <&mdlc_cmnn R8A78000_MDLC_PD_AC00>;
>>     ...
>> };
> 
> So we do have a few modules (I found a few more) that are part of
> power domains, but do no support module standby.  One more reason to
> decouple them in power-domains.

I think HSCN is a really good example ?

> However, CPU cores are controlled through PSCI (the slightly less evil
> brother of SCMI? ;-), so
> Documentation/devicetree/bindings/arm/psci.yaml applies, too?

We can indeed control cores purely via PSCI , yes.

(Upstream TFA needs one more platform patch to make this operable)

>> The MDLC driver would pass the PD_AC00 domain ID to matching SCMI power
>> domain management protocol call, or, for bare-metal MDLC driver, would
>> have to internally encode PD hierarchy, walk it, and apply PD operations
>> in each step.
>>
>> I think even for SCIF/HSCIF, the power domain reference should be
>> something along the lines of the following description. The MDLC driver
>> should internally encode that R8A78000_MLDC_PD_HSCIF0 is a sub-domain of
>> R8A78000_MDLC_PD_APL .
>>
>> serial@c0710000 {
>>     ...
>>     power-domains = <&mdlc_perw R8A78000_MDLC_PD_HSCIF0>;
>>     ...
>> };
> 
> R8A78000_MLDC_PD_HSCIF0 is a not a full sub-domain, but merely standby
> (clock) control inside the PD_APL clock domain?
Do you consider R8A78000_MDLC_PD_AC00 a full sub-domain ? Because that 
one looks very similar to R8A78000_MDLC_PD_HSCIF0 , except the former 
controls a core, the later an UART IP.