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

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

 



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]




[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Photo Sharing]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux