Date:   Fri, 12 Jun 2020 13:47:40 +0200
From:   Paul Cercueil <paul@...pouillou.net>
To:     Suman Anna <s-anna@...com>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        Arnaud Pouliquen <arnaud.pouliquen@...com>, od@...c.me,
        linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Mathieu Poirier <mathieu.poirier@...aro.org>
Subject: Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

Hi Suman,

Le jeu. 11 juin 2020 à 19:21, Suman Anna <s-anna@...com> a écrit :
> Hi Paul,
> 
> On 6/11/20 5:21 PM, Paul Cercueil wrote:
>> Hi Suman,
>> 
>> Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@...com> a écrit :
>>> Hi Paul,
>>> 
>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>> This driver is used to boot, communicate with and load firmwares 
>>>> to the
>>>> MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
>>>> Ingenic.
>>> 
>>> I have a few comments w.r.t pm_runtime usage in this driver.
>>> 
>>>> 
>>>> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>>>> Acked-by: Mathieu Poirier <mathieu.poirier@...aro.org>
>>>> ---
>>>> 
>>>> Notes:
>>>>      v2: Remove exception for always-mapped memories
>>>>      v3: - Use clk_bulk API
>>>>          - Move device-managed code to its own patch [3/4]
>>>>          - Move devicetree table right above ingenic_rproc_driver
>>>>          - Removed #ifdef CONFIG_OF around devicetree table
>>>>          - Removed .owner = THIS_MODULE in ingenic_rproc_driver
>>>>          - Removed useless platform_set_drvdata()
>>>>      v4: - Add fix reported by Julia
>>>>          - Change Kconfig symbol to INGENIC_VPU_RPROC
>>>>          - Add documentation to struct vpu
>>>>          - disable_irq_nosync() -> disable_irq()
>>>>      v5: No change
>>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>>> callbacks
>>>>      v7: - Remove use of of_match_ptr()
>>>>          - Move Kconfig symbol so that it's in alphabetical order
>>>>          - Add missing doc for private structure field aux_base
>>>>          - Don't check for (len <= 0) in da_to_va()
>>>>          - Add missing \n in dev_info/dev_err messages
>>>> 
>>>>   drivers/remoteproc/Kconfig         |   9 +
>>>>   drivers/remoteproc/Makefile        |   1 +
>>>>   drivers/remoteproc/ingenic_rproc.c | 280 
>>>> +++++++++++++++++++++++++++++
>>>>   3 files changed, 290 insertions(+)
>>>>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
>>>> 
>>>> diff --git a/drivers/remoteproc/Kconfig 
>>>> b/drivers/remoteproc/Kconfig
>>>> index fbaed079b299..c4d1731295eb 100644
>>>> --- a/drivers/remoteproc/Kconfig
>>>> +++ b/drivers/remoteproc/Kconfig
>>>> @@ -23,6 +23,15 @@ config IMX_REMOTEPROC
>>>>           It's safe to say N here.
>>>>   +config INGENIC_VPU_RPROC
>>>> +    tristate "Ingenic JZ47xx VPU remoteproc support"
>>>> +    depends on MIPS || COMPILE_TEST
>>>> +    help
>>>> +      Say y or m here to support the VPU in the JZ47xx SoCs from 
>>>> Ingenic.
>>>> +
>>>> +      This can be either built-in or a loadable module.
>>>> +      If unsure say N.
>>>> +
>>>>   config MTK_SCP
>>>>       tristate "Mediatek SCP support"
>>>>       depends on ARCH_MEDIATEK
>>>> diff --git a/drivers/remoteproc/Makefile 
>>>> b/drivers/remoteproc/Makefile
>>>> index 0effd3825035..e8b886e511f0 100644
>>>> --- a/drivers/remoteproc/Makefile
>>>> +++ b/drivers/remoteproc/Makefile
>>>> @@ -10,6 +10,7 @@ remoteproc-y                += remoteproc_sysfs.o
>>>>   remoteproc-y                += remoteproc_virtio.o
>>>>   remoteproc-y                += remoteproc_elf_loader.o
>>>>   obj-$(CONFIG_IMX_REMOTEPROC)        += imx_rproc.o
>>>> +obj-$(CONFIG_INGENIC_VPU_RPROC)        += ingenic_rproc.o
>>>>   obj-$(CONFIG_MTK_SCP)            += mtk_scp.o mtk_scp_ipi.o
>>>>   obj-$(CONFIG_OMAP_REMOTEPROC)        += omap_remoteproc.o
>>>>   obj-$(CONFIG_WKUP_M3_RPROC)        += wkup_m3_rproc.o
>>>> diff --git a/drivers/remoteproc/ingenic_rproc.c 
>>>> b/drivers/remoteproc/ingenic_rproc.c
>>>> new file mode 100644
>>>> index 000000000000..189020d77b25
>>>> --- /dev/null
>>>> +++ b/drivers/remoteproc/ingenic_rproc.c
>>>> @@ -0,0 +1,280 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Ingenic JZ47xx remoteproc driver
>>>> + * Copyright 2019, Paul Cercueil <paul@...pouillou.net>
>>>> + */
>>>> +
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/remoteproc.h>
>>>> +
>>>> +#include "remoteproc_internal.h"
>>>> +
>>>> +#define REG_AUX_CTRL        0x0
>>>> +#define REG_AUX_MSG_ACK        0x10
>>>> +#define REG_AUX_MSG        0x14
>>>> +#define REG_CORE_MSG_ACK    0x18
>>>> +#define REG_CORE_MSG        0x1C
>>>> +
>>>> +#define AUX_CTRL_SLEEP        BIT(31)
>>>> +#define AUX_CTRL_MSG_IRQ_EN    BIT(3)
>>>> +#define AUX_CTRL_NMI_RESETS    BIT(2)
>>>> +#define AUX_CTRL_NMI        BIT(1)
>>>> +#define AUX_CTRL_SW_RESET    BIT(0)
>>>> +
>>>> +struct vpu_mem_map {
>>>> +    const char *name;
>>>> +    unsigned int da;
>>>> +};
>>>> +
>>>> +struct vpu_mem_info {
>>>> +    const struct vpu_mem_map *map;
>>>> +    unsigned long len;
>>>> +    void __iomem *base;
>>>> +};
>>>> +
>>>> +static const struct vpu_mem_map vpu_mem_map[] = {
>>>> +    { "tcsm0", 0x132b0000 },
>>>> +    { "tcsm1", 0xf4000000 },
>>>> +    { "sram",  0x132f0000 },
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct vpu - Ingenic VPU remoteproc private structure
>>>> + * @irq: interrupt number
>>>> + * @clks: pointers to the VPU and AUX clocks
>>>> + * @aux_base: raw pointer to the AUX interface registers
>>>> + * @mem_info: array of struct vpu_mem_info, which contain the 
>>>> mapping info of
>>>> + *            each of the external memories
>>>> + * @dev: private pointer to the device
>>>> + */
>>>> +struct vpu {
>>>> +    int irq;
>>>> +    struct clk_bulk_data clks[2];
>>>> +    void __iomem *aux_base;
>>>> +    struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
>>>> +    struct device *dev;
>>>> +};
>>>> +
>>>> +static int ingenic_rproc_start(struct rproc *rproc)
>>>> +{
>>>> +    struct vpu *vpu = rproc->priv;
>>>> +    u32 ctrl;
>>>> +
>>>> +    enable_irq(vpu->irq);
>>>> +
>>>> +    /* Reset the AUX and enable message IRQ */
>>>> +    ctrl = AUX_CTRL_NMI_RESETS | AUX_CTRL_NMI | 
>>>> AUX_CTRL_MSG_IRQ_EN;
>>>> +    writel(ctrl, vpu->aux_base + REG_AUX_CTRL);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ingenic_rproc_stop(struct rproc *rproc)
>>>> +{
>>>> +    struct vpu *vpu = rproc->priv;
>>>> +
>>>> +    disable_irq(vpu->irq);
>>>> +
>>>> +    /* Keep AUX in reset mode */
>>>> +    writel(AUX_CTRL_SW_RESET, vpu->aux_base + REG_AUX_CTRL);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void ingenic_rproc_kick(struct rproc *rproc, int vqid)
>>>> +{
>>>> +    struct vpu *vpu = rproc->priv;
>>>> +
>>>> +    writel(vqid, vpu->aux_base + REG_CORE_MSG);
>>>> +}
>>>> +
>>>> +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, 
>>>> size_t len)
>>>> +{
>>>> +    struct vpu *vpu = rproc->priv;
>>>> +    void __iomem *va = NULL;
>>>> +    unsigned int i;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
>>>> +        const struct vpu_mem_info *info = &vpu->mem_info[i];
>>>> +        const struct vpu_mem_map *map = info->map;
>>>> +
>>>> +        if (da >= map->da && (da + len) < (map->da + info->len)) {
>>>> +            va = info->base + (da - map->da);
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return (__force void *)va;
>>>> +}
>>>> +
>>>> +static struct rproc_ops ingenic_rproc_ops = {
>>>> +    .start = ingenic_rproc_start,
>>>> +    .stop = ingenic_rproc_stop,
>>>> +    .kick = ingenic_rproc_kick,
>>>> +    .da_to_va = ingenic_rproc_da_to_va,
>>>> +};
>>>> +
>>>> +static irqreturn_t vpu_interrupt(int irq, void *data)
>>>> +{
>>>> +    struct rproc *rproc = data;
>>>> +    struct vpu *vpu = rproc->priv;
>>>> +    u32 vring;
>>>> +
>>>> +    vring = readl(vpu->aux_base + REG_AUX_MSG);
>>>> +
>>>> +    /* Ack the interrupt */
>>>> +    writel(0, vpu->aux_base + REG_AUX_MSG_ACK);
>>>> +
>>>> +    return rproc_vq_interrupt(rproc, vring);
>>>> +}
>>>> +
>>>> +static void ingenic_rproc_disable_clks(void *data)
>>>> +{
>>>> +    struct vpu *vpu = data;
>>>> +
>>>> +    pm_runtime_resume(vpu->dev);
>>>> +    pm_runtime_disable(vpu->dev);
>>>> +
>>>> +    clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>> +}
>>>> +
>>>> +static int ingenic_rproc_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct resource *mem;
>>>> +    struct rproc *rproc;
>>>> +    struct vpu *vpu;
>>>> +    unsigned int i;
>>>> +    int ret;
>>>> +
>>>> +    rproc = devm_rproc_alloc(dev, "ingenic-vpu",
>>>> +                 &ingenic_rproc_ops, NULL, sizeof(*vpu));
>>>> +    if (!rproc)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    vpu = rproc->priv;
>>>> +    vpu->dev = &pdev->dev;
>>>> +    platform_set_drvdata(pdev, vpu);
>>>> +
>>>> +    mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, 
>>>> "aux");
>>>> +    vpu->aux_base = devm_ioremap_resource(dev, mem);
>>>> +    if (IS_ERR(vpu->aux_base)) {
>>>> +        dev_err(dev, "Failed to ioremap\n");
>>>> +        return PTR_ERR(vpu->aux_base);
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
>>>> +        mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>>> +                           vpu_mem_map[i].name);
>>>> +
>>>> +        vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
>>>> +        if (IS_ERR(vpu->mem_info[i].base)) {
>>>> +            ret = PTR_ERR(vpu->mem_info[i].base);
>>>> +            dev_err(dev, "Failed to ioremap\n");
>>>> +            return ret;
>>>> +        }
>>>> +
>>>> +        vpu->mem_info[i].len = resource_size(mem);
>>>> +        vpu->mem_info[i].map = &vpu_mem_map[i];
>>>> +    }
>>>> +
>>>> +    vpu->clks[0].id = "vpu";
>>>> +    vpu->clks[1].id = "aux";
>>>> +
>>>> +    ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), 
>>>> vpu->clks);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Failed to get clocks\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    vpu->irq = platform_get_irq(pdev, 0);
>>>> +    if (vpu->irq < 0)
>>>> +        return vpu->irq;
>>>> +
>>>> +    ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, 
>>>> "VPU", rproc);
>>>> +    if (ret < 0) {
>>>> +        dev_err(dev, "Failed to request IRQ\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    disable_irq(vpu->irq);
>>>> +
>>>> +    /* The clocks must be enabled for the firmware to be loaded 
>>>> in TCSM */
>>>> +    ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), 
>>>> vpu->clks);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Unable to start clocks\n");
>>>> +        return ret;
>>>> +    }
>>> 
>>> You are enabling the clocks directly here and also trying to manage 
>>> them through pm_runtime callbacks again.
>> 
>> Yes. The clocks need to be enabled in the probe.
> 
> For the preferred non CONFIG_PM case now and lack of 
> prepare/unprepare().

I want to make it clear that I'm not against having 
.prepare/.unprepare, but I want to see what maintainers have to say.

>> 
>>>> +
>>>> +    pm_runtime_irq_safe(dev);
>>> 
>>> Nothing wrong with this, but this does take an additional reference 
>>> count on the parent device (a bus device for you??), and also 
>>> implies that your clk driver code can all run in atomic context 
>>> so unless you have a strong reason, it is safe to drop this.
>> 
>> The clock driver code can run in atomic context, it is guaranteed by 
>> the clk API.
> 
> OK.
> 
>> 
>>> 
>>>> +    pm_runtime_set_active(dev);
>>> 
>>> The get_sync below would have been sufficient if you had either 
>>> limited the clk API above to just clk_prepare, or you could have 
>>> moved the whole clk API above into your runtime resume callback.
>> 
>> You assume that pm_runtime_get() will enable the clocks, but that's 
>> only true if CONFIG_PM is set.
>> 
>> The reason the clocks are enabled in the probe, and 
>> pm_runtime_set_active() is called, is that it works whether or not 
>> CONFIG_PM is set.
> 
> As I said, if the intention is to reflect the clocks active state in 
> rpm status, then pm_runtime_get_noresume() does the job for you 
> instead of get_sync(). pm_runtime_get_sync() typically does 3 things 
> - increment the usage count, invoke your callbacks (enable clocks for 
> you), and sets the status to active, with the last two steps optional 
> depending on usage count.

So pm_runtime_get_noresume() instead of pm_runtime_set_active() + 
pm_runtime_get_sync()?

>> 
>>>> +    pm_runtime_enable(dev);
>>>> +    pm_runtime_get_sync(dev);
>>> 
>>> If the intention was to increment the usage count with the above 
>>> sequence, pm_runtime_get_noresume() is better. But dropping all 
>>> of the above and just using get_sync would have been sufficient.
>>> 
>>>> +    pm_runtime_use_autosuspend(dev);
>>> 
>>> I don't see any setting of the autosuspend delay (default value is 
>>> 0). So, you might have as well just not used this at all, and 
>>> just used pm_runtime_put() below.
>> 
>> Autosuspend delay value can be set from userspace.
> 
> Yes, but I don't see a specific purpose for it in your driver. Either 
> you have remoteproc running (and so clocks enabled always), or you 
> don't have a firmware loaded and want clocks disabled (not sure you 
> would want to waste power for certain of amount of time).

What I had in mind is that it would prevent the hardware from being 
quickly disabled then re-enabled when switching firmwares. But it's not 
like it takes a long time to stop/start the clock, so this could go 
away, yes.

>> 
>>>> +
>>>> +    ret = devm_add_action_or_reset(dev, 
>>>> ingenic_rproc_disable_clks, vpu);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Unable to register action\n");
>>>> +        goto out_pm_put;
>>>> +    }
>>>> +
>>>> +    ret = devm_rproc_add(dev, rproc);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Failed to register remote processor\n");
>>>> +        goto out_pm_put;
>>>> +    }
>>> 
>>> You are using auto-boot, so the firmware loading is an asynchronous 
>>> event and most probably you would run through below sequence 
>>> first, and end up disabling the clocks with an incorrect rpm 
>>> status.
>> 
>> The driver can auto-load the firmware, but that does not mean it 
>> will. We actually don't do that, and load a task-specific firmware 
>> onto the remote processor on demand.
> 
> Yeah OK, depends on what is preferred by default. If it is more 
> standard practise that the remoteproc is booted by userspace always, 
> then I suggest setting auto_boot as false. But nothing wrong with 
> expecting to boot by default with a starting firmware.

Ok, I didn't know there was a way to disable auto-boot.

>> 
>>>> +
>>>> +out_pm_put:
>>>> +    pm_runtime_put_autosuspend(dev);
>>> 
>>> And finally, with the remoteproc core rpm patch, this would all 
>>> have been unnecessary.
>>> 
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct of_device_id ingenic_rproc_of_matches[] = {
>>>> +    { .compatible = "ingenic,jz4770-vpu-rproc", },
>>>> +    {}
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>>>> +
>>>> +static int __maybe_unused ingenic_rproc_suspend(struct device 
>>>> *dev)
>>>> +{
>>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>>> +
>>>> +    clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused ingenic_rproc_resume(struct device *dev)
>>>> +{
>>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>>> +
>>>> +    return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
>>>> +    SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, 
>>>> ingenic_rproc_resume, NULL)
>>>> +};
>>>> +
>>>> +static struct platform_driver ingenic_rproc_driver = {
>>>> +    .probe = ingenic_rproc_probe,
>>>> +    .driver = {
>>>> +        .name = "ingenic-vpu",
>>>> +#ifdef CONFIG_PM
>>> 
>>> Not sure why you would want to maintain this conditional, because 
>>> runtime_pm is a core dependency now for your driver to work 
>>> properly.
>> 
>> No, it is not - the driver works perfectly fine with CONFIG_PM being 
>> disabled, and having a hardcoded dependency on CONFIG_PM is not 
>> something we want.
> 
> OK, so if IIUC, in general CONFIG_PM is not a typical usage for your 
> MIPS platforms. Your driver is the first non-ARM remoteproc driver 
> :), CONFIG_PM is almost a given on most ARM platforms.
> 
> So, I fail to see how your clocks can stay disabled then when 
> CONFIG_PM=n if the remoteproc fails to load with the current code, 
> which was your primary argument for using prepare/unprepare (based on 
> comments on prior versions). It looks to me that your needs are 
> indeed better suited with the prepare/unprepare ops as in your 
> initial series.

Yes, I find the rpm in remoteproc core solution more elegant, but that 
doesn't work as well for me in the CONFIG_PM=n case.

> And in the case of CONFIG_PM=y, you have three levels of code that 
> enables the clocks (the bare clk API, the pm_runtime_get in probe, 
> and the pm_runtime_get in the remoteproc core). Depending on the rpm 
> status, it may or may not invoke the callbacks.

pm_runtime_get() in the probe won't call pm_resume, since the rpm 
status is marked as active with pm_runtime_set_active(), so the clocks 
are enabled only once in the probe. And even if the remoteproc core 
calls pm_runtime_get() in the middle of the probe (why would it?) it 
would still work fine since the clock enable/disable API is 
reference-counted.

Cheers,
-Paul

> regards
> Suman
> 
>> 
>> Cheers,
>> -Paul
>> 
>>> regards
>>> Suman
>>> 
>>>> +        .pm = &ingenic_rproc_pm,
>>>> +#endif
>>>> +        .of_match_table = ingenic_rproc_of_matches,
>>>> +    },
>>>> +};
>>>> +module_platform_driver(ingenic_rproc_driver);
>>>> +
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_AUTHOR("Paul Cercueil <paul@...pouillou.net>");
>>>> +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control 
>>>> driver");
>>>> 
>>> 
>> 
>> 
> 

Powered by blists - more mailing lists