Re: [PATCHv3 02/15] remoteproc/omap: Add device tree support

From: Tero Kristo
Date: Fri Dec 20 2019 - 04:36:13 EST

On 20/12/2019 04:08, Suman Anna wrote:
Hi Tero, Mathieu,

On 12/19/19 5:54 AM, Tero Kristo wrote:
On 18/12/2019 01:01, Mathieu Poirier wrote:
Hi Tero,

On Fri, Dec 13, 2019 at 02:55:24PM +0200, Tero Kristo wrote:
From: Suman Anna <s-anna@xxxxxx>

OMAP4+ SoCs support device tree boot only. The OMAP remoteproc
driver is enhanced to support remoteproc devices created through
Device Tree, support for legacy platform devices has been
deprecated. The current DT support handles the IPU and DSP
processor subsystems on OMAP4 and OMAP5 SoCs.

The OMAP remoteproc driver relies on the ti-sysc, reset, and
syscon layers for performing clock, reset and boot vector
management (DSP remoteprocs only) of the devices, but some of
these are limited only to the machine-specific layers
in arch/arm. The dependency against control module API for boot
vector management of the DSP remoteprocs has now been removed
with added logic to parse the boot register from the DT node
and program it appropriately directly within the driver.

The OMAP remoteproc driver expects the firmware names to be
provided via device tree entries (firmware-name.) These are used
to load the proper firmware during boot of the remote processor.

Cc: Tony Lindgren <tony@xxxxxxxxxxx>
Signed-off-by: Suman Anna <s-anna@xxxxxx>
[t-kristo@xxxxxx: converted to use ti-sysc framework]
Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
---
 drivers/remoteproc/omap_remoteproc.c | 191 +++++++++++++++++++++++----
 1 file changed, 168 insertions(+), 23 deletions(-)

diff --git a/drivers/remoteproc/omap_remoteproc.c
b/drivers/remoteproc/omap_remoteproc.c
index 6398194075aa..558634624590 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -2,7 +2,7 @@
 /*
ÂÂ * OMAP Remote Processor driver
ÂÂ *
- * Copyright (C) 2011 Texas Instruments, Inc.
+ * Copyright (C) 2011-2019 Texas Instruments Incorporated -
http://www.ti.com/
ÂÂ * Copyright (C) 2011 Google, Inc.
ÂÂ *
ÂÂ * Ohad Ben-Cohen <ohad@xxxxxxxxxx>
@@ -16,27 +16,53 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/err.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
 #include <linux/remoteproc.h>
 #include <linux/mailbox_client.h>
 #include <linux/omap-mailbox.h>
-
-#include <linux/platform_data/remoteproc-omap.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/reset.h>
  #include "omap_remoteproc.h"
 #include "remoteproc_internal.h"
 +/**
+ * struct omap_rproc_boot_data - boot data structure for the DSP
omap rprocs
+ * @syscon: regmap handle for the system control configuration module
+ * @boot_reg: boot register offset within the @syscon regmap
+ */
+struct omap_rproc_boot_data {
+ÂÂÂ struct regmap *syscon;
+ÂÂÂ unsigned int boot_reg;
+};
+
 /**
ÂÂ * struct omap_rproc - omap remote processor state
ÂÂ * @mbox: mailbox channel handle
ÂÂ * @client: mailbox client to request the mailbox channel
+ * @boot_data: boot data structure for setting processor boot address
ÂÂ * @rproc: rproc handle
+ * @reset: reset handle
ÂÂ */
 struct omap_rproc {
ÂÂÂÂÂ struct mbox_chan *mbox;
ÂÂÂÂÂ struct mbox_client client;
+ÂÂÂ struct omap_rproc_boot_data *boot_data;
ÂÂÂÂÂ struct rproc *rproc;
+ÂÂÂ struct reset_control *reset;
+};
+
+/**
+ * struct omap_rproc_dev_data - device data for the omap remote
processor
+ * @device_name: device name of the remote processor
+ * @has_bootreg: true if this remote processor has boot register
+ */
+struct omap_rproc_dev_data {
+ÂÂÂ const char *device_name;
+ÂÂÂ bool has_bootreg;
 };
  /**
@@ -92,6 +118,21 @@ static void omap_rproc_kick(struct rproc *rproc,
int vqid)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ ret);
 }
 +/**
+ * omap_rproc_write_dsp_boot_addr - set boot address for a DSP
remote processor
+ * @rproc: handle of a remote processor
+ *
+ * Set boot address for a supported DSP remote processor.
+ */
+static void omap_rproc_write_dsp_boot_addr(struct rproc *rproc)
+{
+ÂÂÂ struct omap_rproc *oproc = rproc->priv;
+ÂÂÂ struct omap_rproc_boot_data *bdata = oproc->boot_data;
+ÂÂÂ u32 offset = bdata->boot_reg;
+
+ÂÂÂ regmap_write(bdata->syscon, offset, rproc->bootaddr);
+}
+
 /*
ÂÂ * Power up the remote processor.
ÂÂ *
@@ -103,13 +144,11 @@ static int omap_rproc_start(struct rproc *rproc)
 {
ÂÂÂÂÂ struct omap_rproc *oproc = rproc->priv;
ÂÂÂÂÂ struct device *dev = rproc->dev.parent;
-ÂÂÂ struct platform_device *pdev = to_platform_device(dev);
-ÂÂÂ struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
ÂÂÂÂÂ int ret;
ÂÂÂÂÂ struct mbox_client *client = &oproc->client;
 - if (pdata->set_bootaddr)
-ÂÂÂÂÂÂÂ pdata->set_bootaddr(rproc->bootaddr);
+ÂÂÂ if (oproc->boot_data)
+ÂÂÂÂÂÂÂ omap_rproc_write_dsp_boot_addr(rproc);
 Â client->dev = dev;
ÂÂÂÂÂ client->tx_done = NULL;
@@ -117,7 +156,7 @@ static int omap_rproc_start(struct rproc *rproc)
ÂÂÂÂÂ client->tx_block = false;
ÂÂÂÂÂ client->knows_txdone = false;
 - oproc->mbox = omap_mbox_request_channel(client,
pdata->mbox_name);
+ÂÂÂ oproc->mbox = mbox_request_channel(client, 0);
ÂÂÂÂÂ if (IS_ERR(oproc->mbox)) {
ÂÂÂÂÂÂÂÂÂ ret = -EBUSY;
ÂÂÂÂÂÂÂÂÂ dev_err(dev, "mbox_request_channel failed: %ld\n",
@@ -138,11 +177,7 @@ static int omap_rproc_start(struct rproc *rproc)
ÂÂÂÂÂÂÂÂÂ goto put_mbox;
ÂÂÂÂÂ }
 - ret = pdata->device_enable(pdev);
-ÂÂÂ if (ret) {
-ÂÂÂÂÂÂÂ dev_err(dev, "omap_device_enable failed: %d\n", ret);
-ÂÂÂÂÂÂÂ goto put_mbox;
-ÂÂÂ }
+ÂÂÂ reset_control_deassert(oproc->reset);
 Â return 0;
 @@ -154,15 +189,9 @@ static int omap_rproc_start(struct rproc *rproc)
 /* power off the remote processor */
 static int omap_rproc_stop(struct rproc *rproc)
 {
-ÂÂÂ struct device *dev = rproc->dev.parent;
-ÂÂÂ struct platform_device *pdev = to_platform_device(dev);
-ÂÂÂ struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
ÂÂÂÂÂ struct omap_rproc *oproc = rproc->priv;
-ÂÂÂ int ret;
 - ret = pdata->device_shutdown(pdev);
-ÂÂÂ if (ret)
-ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ reset_control_assert(oproc->reset);

Any reasons for dropping the checks for the return status and wherever
you replaced the pdata callbacks with the desired reset API?

Ok, let me try to add the checks back.


 Â mbox_free_channel(oproc->mbox);
 @@ -175,12 +204,122 @@ static const struct rproc_ops omap_rproc_ops
= {
ÂÂÂÂÂ .kickÂÂÂÂÂÂÂ = omap_rproc_kick,
 };
 +static const struct omap_rproc_dev_data omap4_dsp_dev_data = {
+ÂÂÂ .device_nameÂÂÂ = "dsp",
+ÂÂÂ .has_bootregÂÂÂ = true,
+};
+
+static const struct omap_rproc_dev_data omap4_ipu_dev_data = {
+ÂÂÂ .device_nameÂÂÂ = "ipu",
+};
+
+static const struct omap_rproc_dev_data omap5_dsp_dev_data = {
+ÂÂÂ .device_nameÂÂÂ = "dsp",
+ÂÂÂ .has_bootregÂÂÂ = true,
+};
+
+static const struct omap_rproc_dev_data omap5_ipu_dev_data = {
+ÂÂÂ .device_nameÂÂÂ = "ipu",
+};
+
+static const struct of_device_id omap_rproc_of_match[] = {
+ÂÂÂ {
+ÂÂÂÂÂÂÂ .compatibleÂÂÂÂ = "ti,omap4-dsp",
+ÂÂÂÂÂÂÂ .dataÂÂÂÂÂÂÂÂÂÂ = &omap4_dsp_dev_data,
+ÂÂÂ },
+ÂÂÂ {
+ÂÂÂÂÂÂÂ .compatibleÂÂÂÂ = "ti,omap4-ipu",
+ÂÂÂÂÂÂÂ .dataÂÂÂÂÂÂÂÂÂÂ = &omap4_ipu_dev_data,
+ÂÂÂ },
+ÂÂÂ {
+ÂÂÂÂÂÂÂ .compatibleÂÂÂÂ = "ti,omap5-dsp",
+ÂÂÂÂÂÂÂ .dataÂÂÂÂÂÂÂÂÂÂ = &omap5_dsp_dev_data,
+ÂÂÂ },
+ÂÂÂ {
+ÂÂÂÂÂÂÂ .compatibleÂÂÂÂ = "ti,omap5-ipu",
+ÂÂÂÂÂÂÂ .dataÂÂÂÂÂÂÂÂÂÂ = &omap5_ipu_dev_data,
+ÂÂÂ },
+ÂÂÂ {
+ÂÂÂÂÂÂÂ /* end */
+ÂÂÂ },
+};
+MODULE_DEVICE_TABLE(of, omap_rproc_of_match);
+
+static const char *omap_rproc_get_firmware(struct platform_device
*pdev)
+{
+ÂÂÂ const char *fw_name;
+ÂÂÂ int ret;
+
+ÂÂÂ ret = of_property_read_string(pdev->dev.of_node, "firmware-name",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &fw_name);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ return ERR_PTR(ret);
+
+ÂÂÂ return fw_name;
+}
+
+static int omap_rproc_get_boot_data(struct platform_device *pdev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct rproc *rproc)
+{
+ÂÂÂ struct device_node *np = pdev->dev.of_node;
+ÂÂÂ struct omap_rproc *oproc = rproc->priv;
+ÂÂÂ const struct omap_rproc_dev_data *data;
+ÂÂÂ int ret;
+
+ÂÂÂ data = of_device_get_match_data(&pdev->dev);
+ÂÂÂ if (!data)
+ÂÂÂÂÂÂÂ return -ENODEV;
+
+ÂÂÂ if (!data->has_bootreg)
+ÂÂÂÂÂÂÂ return 0;
+
+ÂÂÂ oproc->boot_data = devm_kzalloc(&pdev->dev,
sizeof(*oproc->boot_data),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GFP_KERNEL);
+ÂÂÂ if (!oproc->boot_data)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+
+ÂÂÂ if (!of_property_read_bool(np, "ti,bootreg")) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "ti,bootreg property is missing\n");
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ oproc->boot_data->syscon =
+ÂÂÂÂÂÂÂÂÂÂÂ syscon_regmap_lookup_by_phandle(np, "ti,bootreg");
+ÂÂÂ if (IS_ERR(oproc->boot_data->syscon)) {
+ÂÂÂÂÂÂÂ ret = PTR_ERR(oproc->boot_data->syscon);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
+ÂÂÂ if (of_property_read_u32_index(np, "ti,bootreg", 1,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &oproc->boot_data->boot_reg)) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "couldn't get the boot register\n");
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ return 0;
+}
+
 static int omap_rproc_probe(struct platform_device *pdev)
 {
-ÂÂÂ struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
+ÂÂÂ struct device_node *np = pdev->dev.of_node;
ÂÂÂÂÂ struct omap_rproc *oproc;
ÂÂÂÂÂ struct rproc *rproc;
+ÂÂÂ const char *firmware;
ÂÂÂÂÂ int ret;
+ÂÂÂ struct reset_control *reset;
+
+ÂÂÂ if (!np) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "only DT-based devices are supported\n");
+ÂÂÂÂÂÂÂ return -ENODEV;
+ÂÂÂ }
+
+ÂÂÂ reset =
devm_reset_control_array_get_optional_exclusive(&pdev->dev);
+ÂÂÂ if (IS_ERR(reset))
+ÂÂÂÂÂÂÂ return PTR_ERR(reset);

Definition of a reset is listed as "required" in the bindings but here
it is
optional. If this is really what you want then adding a comment to
exlain your
choice is probably a good idea.

Right, I think I updated the binding to require this but forgot to
update the driver for this part. Will fix this.

-Tero


+
+ÂÂÂ firmware = omap_rproc_get_firmware(pdev);
+ÂÂÂ if (IS_ERR(firmware))
+ÂÂÂÂÂÂÂ return PTR_ERR(firmware);
 Â ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
ÂÂÂÂÂ if (ret) {
@@ -188,16 +327,21 @@ static int omap_rproc_probe(struct
platform_device *pdev)
ÂÂÂÂÂÂÂÂÂ return ret;
ÂÂÂÂÂ }
 - rproc = rproc_alloc(&pdev->dev, pdata->name, &omap_rproc_ops,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pdata->firmware, sizeof(*oproc));
+ÂÂÂ rproc = rproc_alloc(&pdev->dev, dev_name(&pdev->dev),
&omap_rproc_ops,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ firmware, sizeof(*oproc));
ÂÂÂÂÂ if (!rproc)
ÂÂÂÂÂÂÂÂÂ return -ENOMEM;
 Â oproc = rproc->priv;
ÂÂÂÂÂ oproc->rproc = rproc;
+ÂÂÂ oproc->reset = reset;
ÂÂÂÂÂ /* All existing OMAP IPU and DSP processors have an MMU */
ÂÂÂÂÂ rproc->has_iommu = true;
 + ret = omap_rproc_get_boot_data(pdev, rproc);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ goto free_rproc;
+
ÂÂÂÂÂ platform_set_drvdata(pdev, rproc);
 Â ret = rproc_add(rproc);
@@ -226,6 +370,7 @@ static struct platform_driver omap_rproc_driver = {
ÂÂÂÂÂ .remove = omap_rproc_remove,
ÂÂÂÂÂ .driver = {
ÂÂÂÂÂÂÂÂÂ .name = "omap-rproc",
+ÂÂÂÂÂÂÂ .of_match_table = omap_rproc_of_match,

ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .of_match_table = of_match_ptr(omap_rproc_of_match),

I had dropped this sometime back intentionally as all our platforms are
DT-only.

Hmm, dropped what?

-Tero


regards
Suman


Thanks,
Mathieu

ÂÂÂÂÂ },
 };
 --
2.17.1

--

--


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki