Re: [RFC PATCH 3/3] remoteproc: Add Renesas rcar driver

[Julien Massot <julien.massot@iot.bzh>]

Hi Geert, Mathieu,

Thanks for the review

On 11/9/21 09:09, Geert Uytterhoeven wrote:
> On Mon, Nov 8, 2021 at 7:42 PM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
>> On Wed, Oct 27, 2021 at 09:30:20AM +0200, Julien Massot wrote:
>>> Renesas Gen3 platform includes a Cortex-r7 processor.
>>>
>>> Both: the application cores (A5x) and the realtime core (CR7)
>>> share access to the RAM and devices with the same address map,
>>> so device addresses are equal to the Linux physical addresses.
>>>
>>> In order to initialize this remote processor we need to:
>>> - power on the realtime core
>>> - put the firmware in a ram area
>>> - set the boot address for this firmware (reset vector)
>>> - Deassert the reset
>>>
>>> This initial driver allows to start and stop the Cortex R7
>>> processor.
>>>
>>> Signed-off-by: Julien Massot <julien.massot@iot.bzh>
> 
>>> --- a/drivers/remoteproc/Kconfig
>>> +++ b/drivers/remoteproc/Kconfig
>>> @@ -261,6 +261,17 @@ config QCOM_WCNSS_PIL
>>>          verified and booted with the help of the Peripheral Authentication
>>>          System (PAS) in TrustZone.
>>>
>>> +config RCAR_REMOTEPROC
>>> +     tristate "Renesas RCAR remoteproc support"
>>
>> It is probably a good idea to include the type of SoC being supported, something
>> like:
>>
>>          tristate "Renesas Gen3 RCAR remoteproc support"
> 
> R-Car Gen3 please
Thanks changed to "Renesas R-Car Gen3 remoteproc support".

>>
>>> +     priv->dev = dev;
>>> +
>>> +     priv->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>>> +     if (IS_ERR(priv->rst)) {
>>> +             ret = PTR_ERR(priv->rst);
>>> +             dev_err(dev, "fail to acquire rproc reset\n");
>>> +             goto free_rproc;
>>> +     }
>>> +
>>> +     pm_runtime_enable(priv->dev);
>>> +     ret = pm_runtime_get_sync(priv->dev);
>>
>> There is no dev_pm_ops for the platform driver nor clocks to manage for this
>> device - is there something that requires pm_runtime operations to be called?
> 
> Given
> 
>      cr7_rproc: cr7 {
>          compatible = "renesas,rcar-cr7";
>          memory-region = <&cr7_ram>;
>          power-domains = <&sysc R8A7795_PD_CR7>;
>          resets = <&cpg 222>;
>          status = "okay";
>      };
> 
> the pm_runtime_get_sync() is intended to power the CR7 power domain,
> right?

That's exactly why I'm calling pm_runtime_get_sync. The Cortex R7 power domain
needs to be enabled.
  
> However, I have my doubt about the (bindings for) that node, as it
> does not represent the hardware.  Shouldn't the Cortex R7 have its
> own CPU node instead, with an appropriate enable-method?

Yes, representation is really from Remoteproc point of view, the Cortex R7 is
represented as other devices, and doesn't have a cpu node. As far I know it's
how others remoteproc CPUs are described in other SoCs such as ST, Qcom, TI, or NXP
SoCs. From what I see, CPU nodes are given for binary compatible CPUs where Linux can
execute on.

Regards,

-- 
Julien Massot [IoT.bzh]