Hi Mathieu,
Thanks for the review !
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 9a6eedc3994a..3e87eadbaf59 100644
--- 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"
That will make it easier to support future RCAR processors that may not share
the same architecture.
Ok, changed according to Geert's suggestion to:
"Renesas R-CAR Gen3 remoteproc support"
+ depends on ARCH_RENESAS
+ depends on REMOTEPROC
+ help
+ Say y here to support R-Car realtime processor via the
+ remote processor framework. An elf firmware can be loaded
+ thanks to sysfs remoteproc entries. The remote processor
+ can be started and stopped.
+ This can be either built-in or a loadable module.
Please add the name of the module when compiled as such.
Ok
+
+#include "remoteproc_internal.h"
+
+struct rcar_rproc {
+ struct device *dev;
+ struct rproc *rproc;
+ struct reset_control *rst;
+};
+
+static int rcar_rproc_mem_alloc(struct rproc *rproc,
+ struct rproc_mem_entry *mem)
+{
+ struct device *dev = rproc->dev.parent;
+ void *va;
+
+ dev_dbg(dev, "map memory: %p+%zx\n", &mem->dma, mem->len);
I think this should be "map memory: %pa+%lx\n" to be consistent with dev_err()
below and the original implementation in stm32_rproc.c.
Ok
..
+
+static int rcar_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+ int ret;
+
+ ret = rproc_elf_load_rsc_table(rproc, fw);
+ if (ret)
+ dev_info(&rproc->dev, "No resource table in elf\n");
In the above functions rproc->dev.parent is used for output. I don't have a
strong opinion on which of rproc->dev or rproc->dev.parent is used but I would
like to see consistency throughout the driver.
Thanks I choosed to use rproc->dev. Indeed logs are more consistent now.
+
+ return 0;
+}
+
+static int rcar_rproc_start(struct rproc *rproc)
+{
+ struct rcar_rproc *priv = rproc->priv;
+ int err;
+
+ if (!rproc->bootaddr)
+ return -EINVAL;
+
+ err = rcar_rst_set_rproc_boot_addr(rproc->bootaddr);
+ if (err) {
+ dev_err(&rproc->dev, "failed to set rproc boot addr\n");
Same comment as above.
ok
+
+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));
+ if (!rproc)
+ return -ENOMEM;
+
+ priv = rproc->priv;
+ priv->rproc = rproc;
I don't see rcar_rproc::rproc being used anywhere.
Indeed, rproc member will be removed in next version.
+ 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?
Will reply in Geert's reply.
+
+static int rcar_rproc_remove(struct platform_device *pdev)
+{
+ struct rproc *rproc = platform_get_drvdata(pdev);
+ struct rcar_rproc *priv = rproc->priv;
+
+ rproc_del(rproc);
+ pm_runtime_disable(priv->dev);
As far as I can tell rcar_rproc::dev is not required. It is only used in
rproc_probe() and rproc_remove() where pdev->dev is available.
Thanks rcar_rproc::dev will be removed in next version.
Regards,
--
Julien Massot [IoT.bzh]
