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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx> 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@xxxxxxx>

--- 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]




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux