Re: [PATCH 05/14] dt-bindings: remoteproc: Add Qualcomm RPM processor/subsystem

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 06/06/2023 10:55, Stephan Gerhold wrote:
> <On Tue, Jun 06, 2023 at 08:36:10AM +0200, Krzysztof Kozlowski wrote:
>> On 05/06/2023 09:08, Stephan Gerhold wrote:
>>> On Qualcomm platforms, most subsystems (e.g. audio/modem DSP) are
>>> described as remote processors in the device tree, with a dedicated
>>> node where properties and services related to them can be described.
>>>
>>> The Resource Power Manager (RPM) is also such a subsystem, with a
>>> remote processor that is running a special firmware. Unfortunately,
>>> the RPM never got a dedicated node representing it properly in the
>>> device tree. Most of the RPM services are described below a top-level
>>> /smd or /rpm-glink node.
>>
>> Then what is rpm-requests? This is the true RPM. It looks like you now
>> duplicate half of it in a node above. Unless you want here to describe
>> ways to communicate with the RPM, not the RPM itself.
>>
> 
> I think you're confusing hardware and firmware here. The rpm-proc node
> represents the RPM hardware block (or perhaps the RPM firmware overall),
> while rpm-requests is just *one* communication interface provided by the
> RPM firmware. Here is a rough picture of the RPM "subsystem" I'm trying
> to describe:
> 
>                 +--------------------------------------------+      
>                 |       RPM subsystem (qcom,rpm-proc)        |      
>                 |                                            |      
>           reset | +---------------+     +-----+  +-----+     |      
>         --------->|               |     | MPM |  | CPR | ... |      
>  IPC interrupts | | ARM Cortex-M3 |     +-----+  +-----+     |      
> ----------------->|               |---     |        |        |      
>                 | +---------------+  |---------------------- |      
>                 | +---------------+  |                       |      
>                 | |   Code RAM    |--|  +------------------+ |      
>                 | +---------------+  |  |                  | |      
>                 | +---------------+  |  |   Message RAM    | |      
>                 | |   Data RAM    |--|--|                  | |      
>                 | +---------------+  |  +------------------+ |      
>                 +--------------------|-----------------------+      
>                                      v                              
>                                     NoC                             
> 
> (Somewhat adapted from Figure 8-1 RPM overview in the APQ8016E Technical
>  Reference Manual, but I added some extra stuff.)
> 
> As you can see neither "SMD" nor "rpm-requests" exist in hardware.
> Again, it's just one communication protocol implemented by the RPM
> firmware running on the Cortex-M3 processor, much like a webserver
> running on a PC.
> 
> When the SoC is started only the hardware block exists. Usually a
> component in the boot chain loads firmware into the code/data RAM and
> then de-asserts the reset line to start the Cortex-M3 processor.
> 
> This is not guaranteed to happen. For example, I have a special firmware
> version where the firmware is only loaded but not brought out of reset.
> In this case Linux is responsible to de-assert the reset line.
> In follow-up patches I add that to the outer qcom,rpm-proc node:
> 
> 	remoteproc {
> 		compatible = "qcom,msm8916-rpm-proc", "qcom,rpm-proc";
> 		resets = <&gcc GCC_RPM_RESET>;
> 		iommus = <&apps_smmu 0x0040>;
> 
> 		smd-edge {
> 			/* ... */
> 			rpm-requests {
> 				qcom,smd-channels = "rpm_requests";
> 			};
> 		};
> 	};
> 
> This reset line cannot be described on the rpm-requests node. Until the
> firmware is started there is no such thing as "SMD" or "rpm-requests".

OK, that makes sense, thank you for clarifying. It would be nice to
include it in the binding description (especially that you already wrote
it for me in the email :) ).

> 
> When the RPM firmware is started it sets up some data structures in the
> message RAM and then begins serving requests, perhaps like this:
> 
>                +----------------------------------+                                    
>                |          ARM Cortex-M3           |                                    
>                | +------------------------------+ |  +--------------------------------+
>                | |  RPM Firmware                | |  |          Message RAM           |
>  IPC Interrupt | | +----------------------+     | |  | +----------------------------+ |
> ------------------>| SMD Server           |     | |  | | SMD Data Structures & FIFO | |
>                | | | +--------------+     |     | |  | | +--------------+           | |
>                | | | | rpm_requests | ... |     | |  | | | rpm_requests |    ...    | |
>                | | | +--------------+     | ... | |  | | +--------------+           | |
>                | | +----------------------+     | |  | +----------------------------+ |
>                | +------------------------------+ |  |                ...             |
>                +----------------------------------+  +--------------------------------+
> 
> The "smd-edge" node represents the SMD part and "rpm_requests" is one
> of the communication channels that can be used to talk to the RPM firmware.
> 
> Everything below rpm-requests: clocks, regulators, power domains are
> pure firmware constructions. They do not exist in the RPM hardware block,
> the firmware just acts as a proxy to collect votes from different
> clients and then configures the actual hardware.

OK

>  
>>
>>> However, SMD/GLINK is just one of the communication channels to the RPM
>>> firmware. For example, the MPM interrupt functionality provided by the
>>> RPM does not use SMD/GLINK but writes directly to a special memory
>>> region allocated by the RPM firmware in combination with a mailbox.
>>> Currently there is no good place in the device tree to describe this
>>> functionality. It doesn't belong below SMD/GLINK but it's not an
>>> independent top-level device either.
>>>
>>> Introduce a new "qcom,rpm-proc" compatible that allows describing the
>>> RPM as a remote processor/subsystem like all others. The SMD/GLINK node
>>> is moved to a "smd-edge"/"glink-edge" subnode consistent with other
>>> existing bindings. Additional subnodes (e.g. interrupt-controller for
>>> MPM, rpm-master-stats) can be also added there.
>>
>> If this was about to stay, you should also update the qcom,smd.yaml, so
>> there will be no duplication.
>>
> 
> qcom,smd.yaml is deprecated in the next patch (PATCH 07/14).

Please squash it, because it is logically one change - you rework one
solution and deprecate other.

> 
>>>
>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>> ---
>>>  .../bindings/remoteproc/qcom,rpm-proc.yaml         | 125 +++++++++++++++++++++
>>>  1 file changed, 125 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,rpm-proc.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,rpm-proc.yaml
>>> new file mode 100644
>>> index 000000000000..c06dd4f66503
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,rpm-proc.yaml
>>> @@ -0,0 +1,125 @@
>>> [...]
>>> +  interrupt-controller:
>>> +    type: object
>>> +    $ref: /schemas/interrupt-controller/qcom,mpm.yaml#
>>> +    description:
>>> +      MSM Power Manager (MPM) interrupt controller that monitors interrupts
>>> +      when the system is asleep.
>>
>> Isn't this a service of RPM?
>>
>>> +
>>> +  master-stats:
>>> +    $ref: /schemas/soc/qcom/qcom,rpm-master-stats.yaml#
>>> +    description:
>>> +      Subsystem-level low-power mode statistics provided by RPM.
>>
>> The same question.
>>
> 
> Yes, they are services of the RPM firmware. But they have no relation to
> the SMD/GLINK channel.
> 
> To clarify this I extend my picture from above with MPM:
> 
>                  +----------------------------------+                                    
>                  |          ARM Cortex-M3           |                                    
>                  | +------------------------------+ |  +--------------------------------+
>                  | |  RPM Firmware                | |  |          Message RAM           |
>  IPC Interrupt 0 | | +----------------------+     | |  | +----------------------------+ |
>  ------------------->| SMD Server           |     | |  | | SMD Data Structures & FIFO | |
>                  | | | +--------------+     |     | |  | | +--------------+           | |
>                  | | | | rpm_requests | ... |     | |  | | | rpm_requests |    ...    | |
>                  | | | +--------------+     | ... | |  | | +--------------+           | |
>                  | | +----------------------+     | |  | +----------------------------+ |
>  IPC Interrupt 1 | | +----------------------+     | |  | +----------------------------+ |
>  ------------------->| MPM virtualization   |<-----------| MPM register copy (vMPM)   | |
>                  | | +----------------------+     | |  | +----------------------------+ |
>                  | +-----------|------------------+ |  |              ...               |
>                  +-------------v--------------------+  +--------------------------------+
>                        +--------------+                                                  
>                        | MPM Hardware |                                                  
>                        +--------------+                                                  
> 
> As you can see, SMD and MPM are siblings. The MPM functionality
> implemented by the RPM firmware is not a child of the SMD server.

OK, also please include it in the description.

> 
> It's a bit like a webserver and emailserver running on the same PC.
> Two separate services accessible via different ports and protocols.
> 
> Thanks,
> Stephan
> 
> NOTE: All of this is just my interpretation based on the public hints
> that exist. I have no access to internal documentation or source code
> that would prove that all of this is correct.

Sounds good enough for me :). Thank you.

Best regards,
Krzysztof