Messages in this thread | | | Date | Fri, 12 Jun 2020 13:47:40 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver |
| |
Hi Suman,
Le jeu. 11 juin 2020 à 19:21, Suman Anna <s-anna@ti.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@ti.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@crapouillou.net> >>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.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@crapouillou.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@crapouillou.net>"); >>>> +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control >>>> driver"); >>>> >>> >> >> >
| |