From: Tero Kristo <t-kristo@ti.com>
To: Suman Anna <s-anna@ti.com>, Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: <bjorn.andersson@linaro.org>, <ohad@wizery.com>,
<linux-remoteproc@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-omap@vger.kernel.org>,
Tony Lindgren <tony@atomide.com>
Subject: Re: [PATCHv3 02/15] remoteproc/omap: Add device tree support
Date: Fri, 20 Dec 2019 11:36:01 +0200 [thread overview]
Message-ID: <d0f58297-a722-f5b8-c3f1-7868383cef00@ti.com> (raw)
In-Reply-To: <1d852c78-be6c-ed43-98c9-e5701a772746@ti.com>
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@ti.com>
>>>>
>>>> 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@atomide.com>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> [t-kristo@ti.com: converted to use ti-sysc framework]
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> ---
>>>> 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@wizery.com>
>>>> @@ -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
next prev parent reply other threads:[~2019-12-20 9:36 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-13 12:55 [PATCHv3 00/15] remoteproc: updates for omap remoteproc support Tero Kristo
2019-12-13 12:55 ` [PATCHv3 01/15] dt-bindings: remoteproc: Add OMAP remoteproc bindings Tero Kristo
2019-12-20 3:38 ` Suman Anna
2019-12-13 12:55 ` [PATCHv3 02/15] remoteproc/omap: Add device tree support Tero Kristo
2019-12-17 23:01 ` Mathieu Poirier
2019-12-19 11:54 ` Tero Kristo
2019-12-20 2:08 ` Suman Anna
2019-12-20 9:36 ` Tero Kristo [this message]
2019-12-20 18:17 ` Suman Anna
2019-12-13 12:55 ` [PATCHv3 03/15] remoteproc/omap: Add a sanity check for DSP boot address alignment Tero Kristo
2019-12-13 12:55 ` [PATCHv3 04/15] remoteproc/omap: Add support to parse internal memories from DT Tero Kristo
2019-12-18 0:22 ` Mathieu Poirier
2019-12-19 12:31 ` Tero Kristo
2019-12-19 17:37 ` Mathieu Poirier
2019-12-20 2:15 ` Suman Anna
2019-12-13 12:55 ` [PATCHv3 05/15] remoteproc/omap: Add the rproc ops .da_to_va() implementation Tero Kristo
2019-12-18 0:38 ` Mathieu Poirier
2019-12-19 13:18 ` Tero Kristo
2019-12-20 0:12 ` Mathieu Poirier
2019-12-20 2:34 ` Suman Anna
2019-12-20 22:15 ` Mathieu Poirier
2019-12-13 12:55 ` [PATCHv3 06/15] remoteproc/omap: Initialize and assign reserved memory node Tero Kristo
2019-12-13 12:55 ` [PATCHv3 07/15] remoteproc/omap: Add support for DRA7xx remote processors Tero Kristo
2019-12-18 21:31 ` Mathieu Poirier
2019-12-19 13:29 ` Tero Kristo
2019-12-13 12:55 ` [PATCHv3 08/15] remoteproc/omap: Remove the unused fields from platform data Tero Kristo
2019-12-20 2:44 ` Suman Anna
2019-12-20 9:48 ` Tero Kristo
2019-12-13 12:55 ` [PATCHv3 09/15] remoteproc/omap: Remove the omap_rproc_reserve_cma declaration Tero Kristo
2019-12-20 2:47 ` Suman Anna
2019-12-20 9:49 ` Tero Kristo
2019-12-13 12:55 ` [PATCHv3 10/15] remoteproc/omap: Check for undefined mailbox messages Tero Kristo
2019-12-13 12:55 ` [PATCHv3 11/15] remoteproc/omap: Request a timer(s) for remoteproc usage Tero Kristo
2019-12-18 22:43 ` Mathieu Poirier
2019-12-19 13:43 ` Tero Kristo
2019-12-13 12:55 ` [PATCHv3 12/15] remoteproc/omap: add support for system suspend/resume Tero Kristo
2019-12-19 21:46 ` Mathieu Poirier
2019-12-20 3:11 ` Suman Anna
2019-12-20 11:04 ` Tero Kristo
2019-12-20 18:23 ` Suman Anna
2019-12-20 21:58 ` Mathieu Poirier
2019-12-13 12:55 ` [PATCHv3 13/15] remoteproc/omap: add support for runtime auto-suspend/resume Tero Kristo
2019-12-19 23:43 ` Mathieu Poirier
2019-12-20 3:24 ` Suman Anna
2019-12-20 11:24 ` Tero Kristo
2019-12-20 18:44 ` Suman Anna
2020-01-02 12:50 ` Tero Kristo
2019-12-20 11:08 ` Tero Kristo
2019-12-13 12:55 ` [PATCHv3 14/15] remoteproc/omap: report device exceptions and trigger recovery Tero Kristo
2019-12-13 12:55 ` [PATCHv3 15/15] remoteproc/omap: add watchdog functionality for remote processors Tero Kristo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d0f58297-a722-f5b8-c3f1-7868383cef00@ti.com \
--to=t-kristo@ti.com \
--cc=bjorn.andersson@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=ohad@wizery.com \
--cc=s-anna@ti.com \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).