Re: [PATCHv4 02/14] remoteproc/omap: Add device tree support

[Tero Kristo <t-kristo@xxxxxx>]

On 08/01/2020 19:55, Suman Anna wrote:
> Hi Tero,
>
> On 1/2/20 7:18 AM, 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>
> > ---
> > v4:
> >    - error handling improvements
>
> Thanks for taking care of these comments.
>
> >    - dropped has_bootreg from platform data (instead parsed from DT)
>
> So, I originally used this to ensure that the DT node is not missing the
> bootreg property on the required devices (since it is not present on
> all). I guess this is no longer needed since the idea is that it would
> be caught during the schema check, please confirm.

Yes, schema will complain if you don't have bootreg on the required nodes.

-Tero

> regards
> Suman
>
> >   drivers/remoteproc/omap_remoteproc.c | 177 ++++++++++++++++++++++++---
> >   1 file changed, 160 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c
> > index 6398194075aa..fb4902e4dacf 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-2020 Texas Instruments Incorporated - http://www.ti.com/
> >    * Copyright (C) 2011 Google, Inc.
> >    *
> >    * Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> > @@ -16,27 +16,51 @@
> >   #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
> > + */
> > +struct omap_rproc_dev_data {
> > +	const char *device_name;
> >   };
> >   
> >   /**
> > @@ -92,6 +116,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 +142,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 +154,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,9 +175,9 @@ static int omap_rproc_start(struct rproc *rproc)
> >   		goto put_mbox;
> >   	}
> >   
> > -	ret = pdata->device_enable(pdev);
> > +	ret = reset_control_deassert(oproc->reset);
> >   	if (ret) {
> > -		dev_err(dev, "omap_device_enable failed: %d\n", ret);
> > +		dev_err(dev, "reset control deassert failed: %d\n", ret);
> >   		goto put_mbox;
> >   	}
> >   
> > @@ -154,13 +191,10 @@ 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);
> > +	ret = reset_control_assert(oproc->reset);
> >   	if (ret)
> >   		return ret;
> >   
> > @@ -175,12 +209,115 @@ 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",
> > +};
> > +
> > +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",
> > +};
> > +
> > +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 (!of_property_read_bool(np, "ti,bootreg"))
> > +		return 0;
> > +
> > +	oproc->boot_data = devm_kzalloc(&pdev->dev, sizeof(*oproc->boot_data),
> > +					GFP_KERNEL);
> > +	if (!oproc->boot_data)
> > +		return -ENOMEM;
> > +
> > +	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_exclusive(&pdev->dev);
> > +	if (IS_ERR(reset))
> > +		return PTR_ERR(reset);
> > +
> > +	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 +325,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 +368,7 @@ static struct platform_driver omap_rproc_driver = {
> >   	.remove = omap_rproc_remove,
> >   	.driver = {
> >   		.name = "omap-rproc",
> > +		.of_match_table = omap_rproc_of_match,
> >   	},
> >   };
> >   

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