Messages in this thread | | | Subject | Re: [PATCH 2/6] remoteproc: Add a remoteproc driver for the MT8183's APU | From | Alexandre Bailon <> | Date | Thu, 6 Aug 2020 15:25:38 +0200 |
| |
Hi Mathieu,
On 7/21/20 12:17 AM, Mathieu Poirier wrote: > On Mon, Jul 13, 2020 at 03:29:23PM +0200, Alexandre Bailon wrote: >> This adds a driver to control the APU present in the MT8183. >> This loads the firmware and start the DSP. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> --- >> drivers/remoteproc/Kconfig | 10 + >> drivers/remoteproc/Makefile | 1 + >> drivers/remoteproc/mtk_apu_rproc.c | 308 +++++++++++++++++++++++++++++ > I would name the file mtk_apu.c to be consistent with the existing mtk_scp.c I will rename it > >> 3 files changed, 319 insertions(+) >> create mode 100644 drivers/remoteproc/mtk_apu_rproc.c >> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig >> index c4d1731295eb..e116d4a12ac3 100644 >> --- a/drivers/remoteproc/Kconfig >> +++ b/drivers/remoteproc/Kconfig >> @@ -42,6 +42,16 @@ config MTK_SCP >> >> It's safe to say N here. >> >> +config MTK_APU >> + tristate "Mediatek APU remoteproc support" >> + depends on ARCH_MEDIATEK >> + depends on MTK_IOMMU >> + help >> + Say y to support the Mediatek's Accelerated Processing Unit (APU) via >> + the remote processor framework. >> + >> + It's safe to say N here. >> + >> config OMAP_REMOTEPROC >> tristate "OMAP remoteproc support" >> depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX >> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile >> index e8b886e511f0..2ea231b75fa6 100644 >> --- a/drivers/remoteproc/Makefile >> +++ b/drivers/remoteproc/Makefile >> @@ -12,6 +12,7 @@ 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_MTK_APU) += mtk_apu_rproc.o >> obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o >> obj-$(CONFIG_WKUP_M3_RPROC) += wkup_m3_rproc.o >> obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o >> diff --git a/drivers/remoteproc/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c >> new file mode 100644 >> index 000000000000..fb416a817ef3 >> --- /dev/null >> +++ b/drivers/remoteproc/mtk_apu_rproc.c >> @@ -0,0 +1,308 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2020 BayLibre SAS >> + */ >> + >> +#include <linux/bitops.h> >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/iommu.h> >> +#include <linux/irq.h> >> +#include <linux/kernel.h> >> +#include <linux/highmem.h> > Move this below "delay.h" > >> +#include <linux/module.h> >> +#include <linux/of_reserved_mem.h> >> +#include <linux/platform_device.h> >> +#include <linux/remoteproc.h> >> + >> +#include "remoteproc_internal.h" >> + >> +/* From MT8183 4.5 Vision Processor Unit (VPU).pdf datasheet */ >> +#define SW_RST (0x0000000C) >> +#define SW_RST_OCD_HALT_ON_RST BIT(12) >> +#define SW_RST_IPU_D_RST BIT(8) >> +#define SW_RST_IPU_B_RST BIT(4) > Please don't indent defines. > >> +#define CORE_CTRL (0x00000110) >> +#define CORE_CTRL_PDEBUG_ENABLE BIT(31) >> +#define CORE_CTRL_SRAM_64K_iMEM (0x00 << 27) >> +#define CORE_CTRL_SRAM_96K_iMEM (0x01 << 27) >> +#define CORE_CTRL_SRAM_128K_iMEM (0x02 << 27) >> +#define CORE_CTRL_SRAM_192K_iMEM (0x03 << 27) >> +#define CORE_CTRL_SRAM_256K_iMEM (0x04 << 27) >> +#define CORE_CTRL_PBCLK_ENABLE BIT(26) >> +#define CORE_CTRL_RUN_STALL BIT(23) >> +#define CORE_CTRL_STATE_VECTOR_SELECT BIT(19) >> +#define CORE_CTRL_PIF_GATED BIT(17) >> +#define CORE_CTRL_NMI BIT(0) >> +#define CORE_XTENSA_INT (0x00000114) >> +#define CORE_CTL_XTENSA_INT (0x00000118) >> +#define CORE_DEFAULT0 (0x0000013C) >> +#define CORE_DEFAULT0_QOS_SWAP_0 (0x00 << 28) >> +#define CORE_DEFAULT0_QOS_SWAP_1 (0x01 << 28) >> +#define CORE_DEFAULT0_QOS_SWAP_2 (0x02 << 28) >> +#define CORE_DEFAULT0_QOS_SWAP_3 (0x03 << 28) >> +#define CORE_DEFAULT0_ARUSER_USE_IOMMU (0x10 << 23) >> +#define CORE_DEFAULT0_AWUSER_USE_IOMMU (0x10 << 18) >> +#define CORE_DEFAULT1 (0x00000140) >> +#define CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU (0x10 << 0) >> +#define CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU (0x10 << 5) >> +#define CORE_XTENSA_ALTRESETVEC (0x000001F8) >> + >> +struct mtk_vpu_rproc { >> + struct device *dev; >> + struct rproc *rproc; >> + >> + void __iomem *base; >> + int irq; >> + struct clk *axi; >> + struct clk *ipu; >> + struct clk *jtag; >> +}; >> + >> +static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off) >> +{ >> + return readl(vpu_rproc->base + off); >> +} >> + >> +static void vpu_write32(struct mtk_vpu_rproc *vpu_rproc, u32 off, u32 value) >> +{ >> + writel(value, vpu_rproc->base + off); >> +} > Not sure that much is gained by adding the above two functions. Just using > readl/writel would suit me just fine. I though this was more convenient to use but using readl/writel is also fine for me. >> + >> +static int mtk_vpu_rproc_start(struct rproc *rproc) >> +{ >> + struct mtk_vpu_rproc *vpu_rproc = rproc->priv; >> + u32 core_ctrl; >> + >> + vpu_write32(vpu_rproc, CORE_XTENSA_ALTRESETVEC, rproc->bootaddr); >> + >> + core_ctrl = vpu_read32(vpu_rproc, CORE_CTRL); >> + core_ctrl |= CORE_CTRL_PDEBUG_ENABLE | CORE_CTRL_PBCLK_ENABLE | >> + CORE_CTRL_STATE_VECTOR_SELECT | CORE_CTRL_RUN_STALL | >> + CORE_CTRL_PIF_GATED; >> + vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl); >> + >> + vpu_write32(vpu_rproc, SW_RST, SW_RST_OCD_HALT_ON_RST | >> + SW_RST_IPU_B_RST | SW_RST_IPU_D_RST); >> + ndelay(27); > What is this for? The state of the VPU can't be polled?
TBH, I don't know. I got the programming model from Mediatek's kernel.
I assumed that was the minimum time required to maintain reset asserted to make it effective.
> >> + vpu_write32(vpu_rproc, SW_RST, 0); >> + >> + core_ctrl &= ~CORE_CTRL_PIF_GATED; >> + vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl); >> + >> + vpu_write32(vpu_rproc, CORE_DEFAULT0, CORE_DEFAULT0_AWUSER_USE_IOMMU | >> + CORE_DEFAULT0_ARUSER_USE_IOMMU | >> + CORE_DEFAULT0_QOS_SWAP_1); >> + vpu_write32(vpu_rproc, CORE_DEFAULT1, >> + CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU | >> + CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU); >> + >> + core_ctrl &= ~CORE_CTRL_RUN_STALL; >> + vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl); > I would certainly appreciate more comments that describe that is going on in > this function. I will try to comment a little more but again, this come from Mediatek's kernel. Even if I have access to the datasheet, there are no much details. > >> + >> + return 0; >> +} >> + >> +static int mtk_vpu_rproc_stop(struct rproc *rproc) >> +{ >> + struct mtk_vpu_rproc *vpu_rproc = rproc->priv; >> + u32 core_ctrl; >> + >> + core_ctrl = vpu_read32(vpu_rproc, CORE_CTRL); >> + vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl | CORE_CTRL_RUN_STALL); >> + >> + return 0; >> +} >> + >> +static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid) >> +{ >> + struct mtk_vpu_rproc *vpu_rproc = rproc->priv; >> + >> + vpu_write32(vpu_rproc, CORE_CTL_XTENSA_INT, 1 << vqid); >> +} >> + >> +static const struct rproc_ops mtk_vpu_rproc_ops = { >> + .start = mtk_vpu_rproc_start, >> + .stop = mtk_vpu_rproc_stop, >> + .kick = mtk_vpu_rproc_kick, >> +}; >> + >> +static irqreturn_t mtk_vpu_rproc_callback(int irq, void *data) >> +{ >> + struct rproc *rproc = (struct rproc *)data; > There is no need to cast when working with a void pointer. The same comment > applies throughout. > >> + struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv; >> + >> + vpu_write32(vpu_rproc, CORE_XTENSA_INT, 1); >> + >> + return IRQ_WAKE_THREAD; >> +} >> + >> +static irqreturn_t handle_event(int irq, void *data) >> +{ >> + struct rproc *rproc = (struct rproc *)data; >> + >> + rproc_vq_interrupt(rproc, 0); >> + rproc_vq_interrupt(rproc, 1); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int mtk_vpu_rproc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct mtk_vpu_rproc *vpu_rproc; >> + struct rproc *rproc; >> + struct resource *res; >> + int ret; >> + >> + rproc = rproc_alloc(dev, "apu", &mtk_vpu_rproc_ops, NULL, >> + sizeof(*vpu_rproc)); > The problem with hard coding the name of the remote process is that it work on > only when there is a single processor. Based on the DTS extention sent with > this serie, there seems to be a possibility of having more the one. As such > both remote processor will be called "apu", mandating you to look at the > platform resources to know which is which. Consider using dev_name() or > dev->of_node->name. > >> + if (!rproc) >> + return -ENOMEM; >> + >> + rproc->recovery_disabled = true; >> + rproc->has_iommu = false; >> + >> + vpu_rproc = rproc->priv; >> + vpu_rproc->rproc = rproc; >> + vpu_rproc->dev = dev; >> + >> + platform_set_drvdata(pdev, rproc); >> + >> + rproc->domain = iommu_get_domain_for_dev(dev); >> + if (!rproc->domain) { >> + dev_err(dev, "Failed to get the IOMMU domain\n"); >> + ret = -EINVAL; >> + goto free_rproc; >> + } >> + >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + vpu_rproc->base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(vpu_rproc->base)) { >> + dev_err(&pdev->dev, "Failed to map mmio\n"); > Above dev_err() is used with @dev while here @pdev->dev is. Please pick one you > like and stick with it. > >> + ret = PTR_ERR(vpu_rproc->base); >> + goto free_rproc; >> + } >> + >> + vpu_rproc->irq = platform_get_irq(pdev, 0); >> + if (vpu_rproc->irq < 0) { >> + ret = vpu_rproc->irq; >> + goto free_rproc; >> + } >> + >> + ret = devm_request_threaded_irq(dev, vpu_rproc->irq, >> + mtk_vpu_rproc_callback, handle_event, >> + IRQF_SHARED | IRQF_ONESHOT, >> + "mtk_vpu-remoteproc", rproc); > Same problem as above, i.e hard coding the name of the interrupt will be > confusing when probing sysfs. Here rproc->index holds the value that > corresponds to 'X' in /sys/dev/class/remoteproc/remoteprocX. Simply build a > string using that and feed it to devm_request_threaded_ifq(). > > >> + if (ret) { >> + dev_err(dev, "devm_request_threaded_irq error: %d\n", ret); >> + goto free_rproc; >> + } >> + >> + vpu_rproc->ipu = devm_clk_get(dev, "ipu"); >> + if (IS_ERR(vpu_rproc->ipu)) { >> + dev_err(dev, "Failed to get ipu clock\n"); >> + ret = PTR_ERR(vpu_rproc->ipu); >> + goto free_rproc; >> + } >> + >> + ret = clk_prepare_enable(vpu_rproc->ipu); >> + if (ret) { >> + dev_err(dev, "Failed to enable ipu clock\n"); >> + goto free_rproc; >> + } >> + >> + vpu_rproc->axi = devm_clk_get(dev, "axi"); >> + if (IS_ERR(vpu_rproc->axi)) { >> + dev_err(dev, "Failed to get axi clock\n"); >> + ret = PTR_ERR(vpu_rproc->axi); >> + goto clk_disable_ipu; >> + } >> + >> + ret = clk_prepare_enable(vpu_rproc->axi); >> + if (ret) { >> + dev_err(dev, "Failed to enable axi clock\n"); >> + goto clk_disable_ipu; >> + }a > Please look at how Paul use the clock bulk API to deal with multiple clocs in > ingenic_rproc.c and see if it is possible to use the same scheme. I did not knew the bulk API. I think it should work. > >> + >> + vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag"); > Why is the jtag clock optional when the binding document says that it "seems to > be required to run the DSP, even when JTAG is not in use"?
I forget to change it to devm_clk_get when I figured out this was actually not optional.
Thanks, Alexandre
> >> + if (IS_ERR(vpu_rproc->jtag)) { >> + dev_err(dev, "Failed to enable jtag clock\n"); >> + ret = PTR_ERR(vpu_rproc->jtag); >> + goto clk_disable_axi; >> + } >> + >> + ret = clk_prepare_enable(vpu_rproc->jtag); >> + if (ret) { >> + dev_err(dev, "Failed to enable jtag clock\n"); >> + goto clk_disable_axi; >> + } >> + >> + ret = of_reserved_mem_device_init(dev); >> + if (ret) { >> + dev_err(dev, "device does not have specific CMA pool\n"); >> + goto clk_disable_jtag; >> + } >> + >> + ret = rproc_add(rproc); >> + if (ret) { >> + dev_err(dev, "rproc_add failed: %d\n", ret); >> + goto free_mem; >> + } >> + >> + return 0; >> + >> +free_mem: >> + of_reserved_mem_device_release(dev); >> +clk_disable_jtag: >> + clk_disable_unprepare(vpu_rproc->jtag); >> +clk_disable_axi: >> + clk_disable_unprepare(vpu_rproc->axi); >> +clk_disable_ipu: >> + clk_disable_unprepare(vpu_rproc->ipu); >> +free_rproc: >> + rproc_free(rproc); >> + >> + return ret; >> +} >> + >> +static int mtk_vpu_rproc_remove(struct platform_device *pdev) >> +{ >> + struct rproc *rproc = platform_get_drvdata(pdev); >> + struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv; >> + struct device *dev = &pdev->dev; >> + >> + disable_irq(vpu_rproc->irq); >> + >> + rproc_del(rproc); >> + of_reserved_mem_device_release(dev); >> + clk_disable_unprepare(vpu_rproc->jtag); >> + clk_disable_unprepare(vpu_rproc->axi); >> + clk_disable_unprepare(vpu_rproc->ipu); >> + rproc_free(rproc); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id mtk_vpu_rproc_of_match[] __maybe_unused = { > Why is "__maybe_unused" needed? > > Thanks, > Mathieu > > >> + { .compatible = "mediatek,mt8183-apu", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, mtk_vpu_rproc_of_match); >> + >> +static struct platform_driver mtk_vpu_rproc_driver = { >> + .probe = mtk_vpu_rproc_probe, >> + .remove = mtk_vpu_rproc_remove, >> + .driver = { >> + .name = "mtk_vpu-rproc", >> + .of_match_table = of_match_ptr(mtk_vpu_rproc_of_match), >> + }, >> +}; >> +module_platform_driver(mtk_vpu_rproc_driver); >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_AUTHOR("Alexandre Bailon"); >> +MODULE_DESCRIPTION("Mt8183 VPU Remote Processor control driver"); >> -- >> 2.26.2 >>
| |