On 06/15/2017 02:01 PM, Suman Anna wrote:
> Hi Oleksij,
>
> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>> From: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
>>
>> this driver was tested on NXP imx7d but should work on
>> imx6sx as well.
>> It will upload firmware to OCRAM, which shared memory between
>> Cortex A7 and Cortex M4, then turn M4 on.
>
> Mostly looks fine, need to address few comments. I take it that you
> haven't added the binding since this is just an RFC.
>
>>
>> Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/remoteproc/Kconfig | 8 ++
>> drivers/remoteproc/Makefile | 1 +
>> drivers/remoteproc/imx_rproc.c | 264
>> +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 273 insertions(+)
>> create mode 100644 drivers/remoteproc/imx_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index faad69a1a597..8d33bff4f530 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>> tristate
>> depends on REMOTEPROC
>> +config IMX_REMOTEPROC
>> + tristate "IMX6/7 remoteproc support"
>> + depends on SOC_IMX6SX || SOC_IMX7D
>> + depends on REMOTEPROC
>> + help
>> + TODO, write some thing here
>> + This can be either built-in or a loadable module.
>> +
>> endif # REMOTEPROC
>> endmenu
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index ffc5e430df27..d351c25cdb4e 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y += qcom_wcnss.o
>> qcom_wcnss_pil-y += qcom_wcnss_iris.o
>> obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
>> obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
>> +obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
>> diff --git a/drivers/remoteproc/imx_rproc.c
>> b/drivers/remoteproc/imx_rproc.c
>> new file mode 100644
>> index 000000000000..6bef85237a27
>> --- /dev/null
>> +++ b/drivers/remoteproc/imx_rproc.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
>
> Please add a blank line here.
>
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define IMX7D_ENABLE_M4 BIT(3)
>> +#define IMX7D_SW_M4P_RST BIT(2)
>> +#define IMX7D_SW_M4C_RST BIT(1)
>> +#define IMX7D_SW_M4C_NON_SCLR_RST BIT(0)
>> +
>> +#define IMX7D_M4_RST_MASK 0xf
>> +
>> +
>> +#define IMX7D_RPROC_MEM_MAX 2
>> +enum {
>> + IMX7D_RPROC_IMEM,
>> + IMX7D_RPROC_DMEM,
>> +};
>> +
>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>> + [IMX7D_RPROC_IMEM] = "imem",
>> + [IMX7D_RPROC_DMEM] = "dmem",
>> +};
>
> Do you really need these to be globally defined? You only need them in
> the addr_init function, they can be made local to that function.
>
>> +
>> +/**
>> + * struct imx_rproc_mem - slim internal memory structure
>> + * @cpu_addr: MPU virtual address of the memory region
>> + * @bus_addr: Bus address used to access the memory region
>> + * @size: Size of the memory region
>> + */
>> +struct imx_rproc_mem {
>> + void __iomem *cpu_addr;
>> + phys_addr_t bus_addr;
>
> Are the rproc-view of these regions same as the bus addresses or
> do they have their own local addresses? If latter, images that are built
> for those will be using those addresses in which case you need some
> offset calculation in the da_to_va ops?
>
>> + size_t size;
>> +};
>> +
>> +struct imx_rproc_dcfg {
>> + int offset;
>> +};
>> +
>> +struct imx_rproc {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct rproc *rproc;
>> + const struct imx_rproc_dcfg *dcfg;
>
> No need of aligning the variables, you can get rid of the additional
> whitespaces and maintain consistent style.
>
>> + struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>> + struct clk *clk;
>> +};
>> +
>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>> + .offset = 0xc,
>> +};
>> +
>> +static int imx_rproc_start(struct rproc *rproc)
>> +{
>> + struct imx_rproc *priv = rproc->priv;
>> + const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> + struct device *dev = priv->dev;
>> + int ret;
>> +
>> + ret = clk_enable(priv->clk);
>> + if (ret) {
>> + dev_err(&rproc->dev, "Failed to enable clock\n");
>> + return ret;
>> + }
>> +
>> + ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> + IMX7D_M4_RST_MASK,
>> + IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | IMX7D_ENABLE_M4);
>> + if (ret) {
>> + dev_err(dev, "Filed to enable M4!\n");
typo for failed..
>> + clk_disable(priv->clk);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int imx_rproc_stop(struct rproc *rproc)
>> +{
>> + struct imx_rproc *priv = rproc->priv;
>> + const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> + struct device *dev = priv->dev;
>> + int ret;
>> +
>> + ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> + IMX7D_M4_RST_MASK,
>> + IMX7D_SW_M4C_NON_SCLR_RST);
>> + if (ret)
>> + dev_err(dev, "Filed to stop M4!\n");
>> +
>> + clk_disable(priv->clk);
>> +
>> + return ret;
>> +}
>> +
>> +static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>> +{
>> + struct imx_rproc *priv = rproc->priv;
>> + void *va = NULL;
>> + int i;
>> +
>> + for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
>> + if (da != priv->mem[i].bus_addr)
>> + continue;
>> +
>> + if (len <= priv->mem[i].size) {
>> + /* __force to make sparse happy with type conversion */
>> + va = (__force void *)priv->mem[i].cpu_addr;
>> + break;
>> + }
missed this yesterday, but this implementation is wrong, you need to be
doing a range check and translate here accordingly. See for an example a
patch on a driver I submitted recently,
https://patchwork.kernel.org/patch/9773687/
regards
Suman
>> + }
>> +
>> + dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da,
>> len, va);
>> +
>> + return va;
>> +}
>> +
>> +static const struct rproc_ops imx_rproc_ops = {
>> + .start = imx_rproc_start,
>> + .stop = imx_rproc_stop,
>> + .da_to_va = imx_rproc_da_to_va,
>> +};
>> +
>> +static const struct of_device_id imx_rproc_of_match[] = {
>> + { .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
>> +
>> +static int imx_rproc_addr_init(struct imx_rproc *priv,
>> + struct platform_device *pdev)
>> +{
>> + int i, err;
>> + struct resource *res;
>> +
>> + /* get imem and dmem */
>> + for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> + mem_names[i]);
>> + if (!res)
>> + continue;
>> +
>> + priv->mem[i].cpu_addr = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(priv->mem[i].cpu_addr)) {
>> + dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
>> + err = PTR_ERR(priv->mem[i].cpu_addr);
>> + return err;
>> + }
>> + priv->mem[i].bus_addr = res->start & 0xffff;
>> + priv->mem[i].size = resource_size(res);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int imx_rproc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct imx_rproc *priv;
>> + struct rproc *rproc;
>> + struct regmap_config config = { .name = "imx_rproc" };
>> + const struct imx_rproc_dcfg *dcfg;
>> + struct regmap *regmap;
>> + int ret;
>> +
>> + regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
>> + if (IS_ERR(regmap)) {
>> + dev_err(dev, "failed to find syscon\n");
>> + return PTR_ERR(regmap);
>> + }
>> + regmap_attach_dev(dev, regmap, &config);
>> +
>> + /* set some other name then imx */
>> + rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL,
>> sizeof(*priv));
>> + if (!rproc) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + dcfg = of_device_get_match_data(dev);
>> + if (!dcfg)
>> + return -EINVAL;
>
> move this up to the beginning of the function. You can avoid the cleanup
> (you are missing an rproc_free() here)
>
>> +
>> + priv = rproc->priv;
>> + priv->rproc = rproc;
>> + priv->regmap = regmap;
>> + priv->dcfg = dcfg;
>> +
>> + dev_set_drvdata(dev, rproc);
>> +
>> + ret = imx_rproc_addr_init(priv, pdev);
>> + if (ret) {
>> + dev_err(dev, "filed on imx_rproc_addr_init\n");
>> + goto err_put_rproc;
>> + }
>> +
>> + priv->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(priv->clk)) {
>> + dev_err(dev, "Failed to get clock\n");
>> + return PTR_ERR(priv->clk);
>> + }
>> +
>> + ret = rproc_add(rproc);
>> + if (ret) {
>> + dev_err(dev, "rproc_add failed\n");
>> + goto err_put_rproc;
>> + }
>> +
>> + ret = clk_prepare(priv->clk);
>> + if (ret)
>> + dev_err(dev, "failed to get clock\n");
>
> How are your internal memories clocked? If they need the same clock,
> rproc_add() would need the clock to be enabled for loading section data
> into those regions as part of rproc_boot(), and your current enable in
> imx_rproc_start() will be too late. Also, do not see a balancing
> clk_unprepare() call in remove, and a missing rproc_del() on failure here.
>
> regards
> Suman
>
>> +
>> + return ret;
>> +
>> +err_put_rproc:
>> + rproc_free(rproc);
>> +err:
>> + return ret;
>> +}
>> +
>> +static int imx_rproc_remove(struct platform_device *pdev)
>> +{
>> + struct rproc *rproc = platform_get_drvdata(pdev);
>> +
>> + rproc_del(rproc);
>> + rproc_free(rproc);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver imx_rproc_driver = {
>> + .probe = imx_rproc_probe,
>> + .remove = imx_rproc_remove,
>> + .driver = {
>> + .name = "imx_rproc",
>> + .of_match_table = imx_rproc_of_match,
>> + },
>> +};
>> +
>> +module_platform_driver(imx_rproc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("IMX6/7 remote processor control driver");
>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
