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

[Suman Anna]

On 12/20/19 3:36 AM, Tero Kristo wrote:
> 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?

Dropped the of_match_ptr.

regards
Suman

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