Re: [PATCH v2 2/2] remoteproc: Add Renesas rcar driver

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

Hi Geert,
Thanks for taking time to review my patch.

On 12/2/21 14:40, Geert Uytterhoeven wrote:
> Hi Julien,
> 
> Thanks for your patch!
> 
> On Tue, Nov 30, 2021 at 11:01 AM Julien Massot <julien.massot@iot.bzh> 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
> 
> RAM
fixed
> 
>> - 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
>> @@ -283,6 +283,17 @@ config QCOM_WCNSS_PIL
>>            verified and booted with the help of the Peripheral Authentication
>>            System (PAS) in TrustZone.
>>
>> +config RCAR_REMOTEPROC
>> +       tristate "Renesas R-CAR Gen3 remoteproc support"
> 
> R-Car
fixed
> 
>> +       depends on ARCH_RENESAS
> 
> || COMPILE_TEST?
COMPILE_TEST has been added to v3

> 
>> +       help
>> +         Say y here to support R-Car realtime processor via the
>> +         remote processor framework. An elf firmware can be loaded
> 
> ELF
ok
> 
>> +         thanks to sysfs remoteproc entries. The remote processor
>> +         can be started and stopped.
>> +         This can be either built-in or a loadable module.
>> +         If compiled as module (M), the module name is rcar_rproc.
>> +
>>   config ST_REMOTEPROC
>>          tristate "ST remoteproc support"
>>          depends on ARCH_STI
> 
>> --- /dev/null
>> +++ b/drivers/remoteproc/rcar_rproc.c
> 
>> +static int rcar_rproc_mem_alloc(struct rproc *rproc,
>> +                                struct rproc_mem_entry *mem)
>> +{
>> +       struct device *dev = &rproc->dev;
>> +       void *va;
>> +
>> +       dev_dbg(dev, "map memory: %pa+%zx\n", &mem->dma, mem->len);
>> +       va = ioremap_wc(mem->dma, mem->len);
>> +       if (IS_ERR_OR_NULL(va)) {
> 
> I think ioremap_*() never returns an error code, only NULL for failure.
Changed to check against NULL.
> 
>> +               dev_err(dev, "Unable to map memory region: %pa+%zx\n",
>> +                       &mem->dma, mem->len);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       /* Update memory entry va */
>> +       mem->va = va;
>> +
>> +       return 0;
>> +}
> 
>> +static int rcar_rproc_prepare(struct rproc *rproc)
>> +{
>> +       struct device *dev = rproc->dev.parent;
>> +       struct device_node *np = dev->of_node;
>> +       struct of_phandle_iterator it;
>> +       struct rproc_mem_entry *mem;
>> +       struct reserved_mem *rmem;
>> +       u64 da;
>> +
>> +       /* Register associated reserved memory regions */
>> +       of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>> +       while (of_phandle_iterator_next(&it) == 0) {
>> +
>> +               rmem = of_reserved_mem_lookup(it.node);
>> +               if (!rmem) {
>> +                       dev_err(&rproc->dev,
>> +                               "unable to acquire memory-region\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               /* No need to translate pa to da, R-Car use same map */
>> +               da = rmem->base;
>> +
>> +               mem = rproc_mem_entry_init(dev, NULL,
>> +                                          (dma_addr_t)rmem->base,
> 
> Do you need this cast?
Looks like not. removed in v3

> 
>> +                                          rmem->size, da,
> 
> da is u64, and thus truncated to u32.
Ok thats indeed a limitation between the AP cores and the realtime core.
In v3 there is a check for bad input from device tree.

> 
>> +                                          rcar_rproc_mem_alloc,
>> +                                          rcar_rproc_mem_release,
>> +                                          it.node->name);
>> +
>> +               if (!mem)
>> +                       return -ENOMEM;
>> +
>> +               rproc_add_carveout(rproc, mem);
>> +       }
>> +
>> +       return 0;
>> +}
> 
>> +static int rcar_rproc_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct rcar_rproc *priv;
>> +       struct rproc *rproc;
>> +       int ret;
>> +
>> +       rproc = rproc_alloc(dev, np->name, &rcar_rproc_ops,
>> +                           NULL, sizeof(*priv));
> 
> devm_rproc_alloc(), to simplify cleanup?
Indeed devm_rproc_alloc is used in v3.
> 
>> +       if (!rproc)
>> +               return -ENOMEM;
>> +
>> +       priv = rproc->priv;
>> +
>> +       priv->rst = devm_reset_control_get_exclusive(dev, NULL);
>> +       if (IS_ERR(priv->rst)) {
>> +               ret = PTR_ERR(priv->rst);
>> +               dev_err(dev, "fail to acquire rproc reset\n");
> 
> dev_err_probe() (which handles -EPROBE_DEFER, too)
ok
> 
>> +               goto free_rproc;
>> +       }
>> +
>> +       pm_runtime_enable(dev);
>> +       ret = pm_runtime_get_sync(dev);
>> +       if (ret) {
>> +               dev_err(dev, "failed to power up\n");
>> +               goto free_rproc;
>> +       }
>> +
>> +       dev_set_drvdata(dev, rproc);
>> +
>> +       /* Manually start the rproc */
>> +       rproc->auto_boot = false;
>> +
>> +       ret = rproc_add(rproc);
> 
> devm_rproc_add()?
devm_rproc_add is now used in v3.

> 
>> +       if (ret) {
>> +               dev_err(dev, "rproc_add failed\n");
>> +               goto pm_disable;
>> +       }
>> +
>> +       return 0;
>> +
>> +pm_disable:
>> +       pm_runtime_disable(dev);
>> +free_rproc:
>> +       rproc_free(rproc);
>> +
>> +       return ret;
>> +}
> 
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Renesas Gen3 R-Car remote processor control driver");
> 
> R-Car Gen3
Ok

All requested changes should be addressed in v3.

Regards,
-- 
Julien Massot [IoT.bzh]