From: Adrian Ratiu <adrian.ratiu@collabora.com> To: Ezequiel Garcia <ezequiel@collabora.com>, Philipp Zabel <p.zabel@pengutronix.de> Cc: Mark Brown <broonie@kernel.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, Fruehberger Peter <Peter.Fruehberger@de.bosch.com>, kuhanh.murugasen.krishnan@intel.com, Daniel Vetter <daniel@ffwll.ch>, kernel@collabora.com, linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH 06/18] media: hantro: imx8mq: simplify ctrlblk reset logic Date: Mon, 12 Oct 2020 23:59:45 +0300 [thread overview] Message-ID: <20201012205957.889185-7-adrian.ratiu@collabora.com> (raw) In-Reply-To: <20201012205957.889185-1-adrian.ratiu@collabora.com> The G1 and G2 cores on imx8mq share a common "control block" used to reset and enable the core clocks as well as enable functioning via ctrl FUSE registers (these are not the FUSEs on the VPU cores, they are just used to enable/disable the cores and allow the real VPU FUSE regs to become available). The problem is that, while the cores can be operated independently from one another (different config reg mem regions, separate IRQs), they can not be reset or powered down independently as the current code implies. This has been a source for many bugs and frustration when trying to enable G2 which this driver does not support yet. So we simplify the ctrlblk reset logic to always reset both cores, exactly like the vendor linux-imx provided driver "hantrodec" does for this SoC. Going forward, this simplified code should be moved in the future to its own reset controller driver as the reset framework also supports shared reset resources so the runtime PM logic can disable both cores when none of them are in use (this is not done yet because only G1 is supported in the driver so there is no need to account for G2). Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> --- drivers/staging/media/hantro/hantro.h | 2 - drivers/staging/media/hantro/imx8m_vpu_hw.c | 74 +++++++-------------- 2 files changed, 24 insertions(+), 52 deletions(-) diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index bb442eb1974e..2dd4362d4080 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -167,7 +167,6 @@ hantro_vdev_to_func(struct video_device *vdev) * @reg_bases: Mapped addresses of VPU registers. * @enc_base: Mapped address of VPU encoder register for convenience. * @dec_base: Mapped address of VPU decoder register for convenience. - * @ctrl_base: Mapped address of VPU control block. * @vpu_mutex: Mutex to synchronize V4L2 calls. * @irqlock: Spinlock to synchronize access to data structures * shared with interrupt handlers. @@ -187,7 +186,6 @@ struct hantro_dev { void __iomem **reg_bases; void __iomem *enc_base; void __iomem *dec_base; - void __iomem *ctrl_base; struct mutex vpu_mutex; /* video_device lock */ spinlock_t irqlock; diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c index c222de075ef4..b2a401a33992 100644 --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c @@ -24,34 +24,13 @@ #define CTRL_G1_PP_FUSE 0x0c #define CTRL_G2_DEC_FUSE 0x10 -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits) -{ - u32 val; - - /* Assert */ - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); - val &= ~reset_bits; - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); - - udelay(2); - - /* Release */ - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); - val |= reset_bits; - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); -} - -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits) -{ - u32 val; - - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE); - val |= clock_bits; - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE); -} - -static int imx8mq_runtime_resume(struct hantro_dev *vpu) +/* + * Due to a HW limitation, both G1 and G2 VPU cores on imx8mq need to be reset + * together via their unified ctrl block. + */ +static int imx8mq_ctrlblk_reset(struct hantro_dev *vpu) { + void __iomem *ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; int ret; ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks); @@ -60,13 +39,18 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu) return ret; } - imx8m_soft_reset(vpu, RESET_G1 | RESET_G2); - imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2); + /* reset HW and ungate clocks via ctrl block */ + writel(RESET_G1 | RESET_G2, ctrl_base + CTRL_SOFT_RESET); + writel(CLOCK_G1 | CLOCK_G2, ctrl_base + CTRL_CLOCK_ENABLE); - /* Set values of the fuse registers */ - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE); - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE); - writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE); + /* + * enable fuse functionalities for each core, these are not real fuses + * but registers which enable the cores and makes accesible their real + * read-only fuse registers describing supported features. + */ + writel(0xffffffff, ctrl_base + CTRL_G1_DEC_FUSE); + writel(0xffffffff, ctrl_base + CTRL_G1_PP_FUSE); + writel(0xffffffff, ctrl_base + CTRL_G2_DEC_FUSE); clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks); @@ -148,19 +132,9 @@ static irqreturn_t imx8m_vpu_g1_irq(int irq, void *dev_id) return IRQ_HANDLED; } -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu) +static void imx8m_vpu_reset(struct hantro_ctx *ctx) { - vpu->dec_base = vpu->reg_bases[0]; - vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; - - return 0; -} - -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx) -{ - struct hantro_dev *vpu = ctx->dev; - - imx8m_soft_reset(vpu, RESET_G1); + imx8mq_ctrlblk_reset(ctx->dev); } /* @@ -170,19 +144,19 @@ static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx) static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { [HANTRO_MODE_MPEG2_DEC] = { .run = hantro_g1_mpeg2_dec_run, - .reset = imx8m_vpu_g1_reset, + .reset = imx8m_vpu_reset, .init = hantro_mpeg2_dec_init, .exit = hantro_mpeg2_dec_exit, }, [HANTRO_MODE_VP8_DEC] = { .run = hantro_g1_vp8_dec_run, - .reset = imx8m_vpu_g1_reset, + .reset = imx8m_vpu_reset, .init = hantro_vp8_dec_init, .exit = hantro_vp8_dec_exit, }, [HANTRO_MODE_H264_DEC] = { .run = hantro_g1_h264_dec_run, - .reset = imx8m_vpu_g1_reset, + .reset = imx8m_vpu_reset, .init = hantro_h264_dec_init, .exit = hantro_h264_dec_exit, }, @@ -209,8 +183,8 @@ const struct hantro_variant imx8mq_vpu_variant = { .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER | HANTRO_H264_DECODER, .codec_ops = imx8mq_vpu_codec_ops, - .init = imx8mq_vpu_hw_init, - .runtime_resume = imx8mq_runtime_resume, + .init = imx8mq_ctrlblk_reset, + .runtime_resume = imx8mq_ctrlblk_reset, .irqs = imx8mq_irqs, .num_irqs = ARRAY_SIZE(imx8mq_irqs), .clk_names = imx8mq_clk_names, -- 2.28.0
WARNING: multiple messages have this Message-ID (diff)
From: Adrian Ratiu <adrian.ratiu@collabora.com> To: Ezequiel Garcia <ezequiel@collabora.com>, Philipp Zabel <p.zabel@pengutronix.de> Cc: Fruehberger Peter <Peter.Fruehberger@de.bosch.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, Mark Brown <broonie@kernel.org>, kuhanh.murugasen.krishnan@intel.com, Daniel Vetter <daniel@ffwll.ch>, kernel@collabora.com, linux-media@vger.kernel.org Subject: [PATCH 06/18] media: hantro: imx8mq: simplify ctrlblk reset logic Date: Mon, 12 Oct 2020 23:59:45 +0300 [thread overview] Message-ID: <20201012205957.889185-7-adrian.ratiu@collabora.com> (raw) In-Reply-To: <20201012205957.889185-1-adrian.ratiu@collabora.com> The G1 and G2 cores on imx8mq share a common "control block" used to reset and enable the core clocks as well as enable functioning via ctrl FUSE registers (these are not the FUSEs on the VPU cores, they are just used to enable/disable the cores and allow the real VPU FUSE regs to become available). The problem is that, while the cores can be operated independently from one another (different config reg mem regions, separate IRQs), they can not be reset or powered down independently as the current code implies. This has been a source for many bugs and frustration when trying to enable G2 which this driver does not support yet. So we simplify the ctrlblk reset logic to always reset both cores, exactly like the vendor linux-imx provided driver "hantrodec" does for this SoC. Going forward, this simplified code should be moved in the future to its own reset controller driver as the reset framework also supports shared reset resources so the runtime PM logic can disable both cores when none of them are in use (this is not done yet because only G1 is supported in the driver so there is no need to account for G2). Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> --- drivers/staging/media/hantro/hantro.h | 2 - drivers/staging/media/hantro/imx8m_vpu_hw.c | 74 +++++++-------------- 2 files changed, 24 insertions(+), 52 deletions(-) diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index bb442eb1974e..2dd4362d4080 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -167,7 +167,6 @@ hantro_vdev_to_func(struct video_device *vdev) * @reg_bases: Mapped addresses of VPU registers. * @enc_base: Mapped address of VPU encoder register for convenience. * @dec_base: Mapped address of VPU decoder register for convenience. - * @ctrl_base: Mapped address of VPU control block. * @vpu_mutex: Mutex to synchronize V4L2 calls. * @irqlock: Spinlock to synchronize access to data structures * shared with interrupt handlers. @@ -187,7 +186,6 @@ struct hantro_dev { void __iomem **reg_bases; void __iomem *enc_base; void __iomem *dec_base; - void __iomem *ctrl_base; struct mutex vpu_mutex; /* video_device lock */ spinlock_t irqlock; diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c index c222de075ef4..b2a401a33992 100644 --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c @@ -24,34 +24,13 @@ #define CTRL_G1_PP_FUSE 0x0c #define CTRL_G2_DEC_FUSE 0x10 -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits) -{ - u32 val; - - /* Assert */ - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); - val &= ~reset_bits; - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); - - udelay(2); - - /* Release */ - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); - val |= reset_bits; - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); -} - -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits) -{ - u32 val; - - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE); - val |= clock_bits; - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE); -} - -static int imx8mq_runtime_resume(struct hantro_dev *vpu) +/* + * Due to a HW limitation, both G1 and G2 VPU cores on imx8mq need to be reset + * together via their unified ctrl block. + */ +static int imx8mq_ctrlblk_reset(struct hantro_dev *vpu) { + void __iomem *ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; int ret; ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks); @@ -60,13 +39,18 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu) return ret; } - imx8m_soft_reset(vpu, RESET_G1 | RESET_G2); - imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2); + /* reset HW and ungate clocks via ctrl block */ + writel(RESET_G1 | RESET_G2, ctrl_base + CTRL_SOFT_RESET); + writel(CLOCK_G1 | CLOCK_G2, ctrl_base + CTRL_CLOCK_ENABLE); - /* Set values of the fuse registers */ - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE); - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE); - writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE); + /* + * enable fuse functionalities for each core, these are not real fuses + * but registers which enable the cores and makes accesible their real + * read-only fuse registers describing supported features. + */ + writel(0xffffffff, ctrl_base + CTRL_G1_DEC_FUSE); + writel(0xffffffff, ctrl_base + CTRL_G1_PP_FUSE); + writel(0xffffffff, ctrl_base + CTRL_G2_DEC_FUSE); clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks); @@ -148,19 +132,9 @@ static irqreturn_t imx8m_vpu_g1_irq(int irq, void *dev_id) return IRQ_HANDLED; } -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu) +static void imx8m_vpu_reset(struct hantro_ctx *ctx) { - vpu->dec_base = vpu->reg_bases[0]; - vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; - - return 0; -} - -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx) -{ - struct hantro_dev *vpu = ctx->dev; - - imx8m_soft_reset(vpu, RESET_G1); + imx8mq_ctrlblk_reset(ctx->dev); } /* @@ -170,19 +144,19 @@ static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx) static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { [HANTRO_MODE_MPEG2_DEC] = { .run = hantro_g1_mpeg2_dec_run, - .reset = imx8m_vpu_g1_reset, + .reset = imx8m_vpu_reset, .init = hantro_mpeg2_dec_init, .exit = hantro_mpeg2_dec_exit, }, [HANTRO_MODE_VP8_DEC] = { .run = hantro_g1_vp8_dec_run, - .reset = imx8m_vpu_g1_reset, + .reset = imx8m_vpu_reset, .init = hantro_vp8_dec_init, .exit = hantro_vp8_dec_exit, }, [HANTRO_MODE_H264_DEC] = { .run = hantro_g1_h264_dec_run, - .reset = imx8m_vpu_g1_reset, + .reset = imx8m_vpu_reset, .init = hantro_h264_dec_init, .exit = hantro_h264_dec_exit, }, @@ -209,8 +183,8 @@ const struct hantro_variant imx8mq_vpu_variant = { .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER | HANTRO_H264_DECODER, .codec_ops = imx8mq_vpu_codec_ops, - .init = imx8mq_vpu_hw_init, - .runtime_resume = imx8mq_runtime_resume, + .init = imx8mq_ctrlblk_reset, + .runtime_resume = imx8mq_ctrlblk_reset, .irqs = imx8mq_irqs, .num_irqs = ARRAY_SIZE(imx8mq_irqs), .clk_names = imx8mq_clk_names, -- 2.28.0 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
next prev parent reply other threads:[~2020-10-12 21:00 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-12 20:59 [PATCH 00/18] Add Hantro regmap and VC8000 h264 decode support Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 01/18] media: hantro: document all int reg bits up to vc8000 Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 02/18] media: hantro: make consistent use of decimal register notation Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 03/18] media: hantro: make G1_REG_SOFT_RESET Rockchip specific Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 04/18] media: hantro: add reset controller support Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-13 8:11 ` Philipp Zabel 2020-10-13 8:11 ` Philipp Zabel 2020-10-12 20:59 ` [PATCH 05/18] media: hantro: prepare clocks before variant inits are run Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu [this message] 2020-10-12 20:59 ` [PATCH 06/18] media: hantro: imx8mq: simplify ctrlblk reset logic Adrian Ratiu 2020-10-12 20:59 ` [PATCH 07/18] regmap: mmio: add config option to allow relaxed MMIO accesses Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-13 10:26 ` Mark Brown 2020-10-13 10:26 ` Mark Brown 2020-10-14 11:51 ` Adrian Ratiu 2020-10-14 11:51 ` Adrian Ratiu 2020-10-14 12:12 ` Mark Brown 2020-10-14 12:12 ` Mark Brown 2020-10-14 13:00 ` Adrian Ratiu 2020-10-14 13:00 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 08/18] media: hantro: add initial MMIO regmap infrastructure Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 09/18] media: hantro: default regmap to relaxed MMIO Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 10/18] media: hantro: convert G1 h264 decoder to regmap fields Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 11/18] media: hantro: convert G1 postproc to regmap Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 12/18] media: hantro: add VC8000D h264 decoding Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 13/18] media: hantro: add VC8000D postproc support Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 14/18] media: hantro: make PP enablement logic a bit smarter Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 15/18] media: hantro: add user-selectable, platform-selectable H264 High10 Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 16/18] media: hantro: rename h264_dec as it's not G1 specific anymore Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 17/18] media: hantro: add dump registers debug option before decode start Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 20:59 ` [PATCH 18/18] media: hantro: document encoder reg fields Adrian Ratiu 2020-10-12 20:59 ` Adrian Ratiu 2020-10-12 23:39 ` [PATCH 00/18] Add Hantro regmap and VC8000 h264 decode support Jonas Karlman 2020-10-12 23:39 ` Jonas Karlman 2020-10-13 6:48 ` Adrian Ratiu 2020-10-13 6:48 ` Adrian Ratiu 2020-10-29 12:38 ` Ezequiel Garcia 2020-10-29 12:38 ` Ezequiel Garcia 2020-10-29 16:21 ` Jonas Karlman 2020-10-29 16:21 ` Jonas Karlman 2020-11-03 15:27 ` Ezequiel Garcia 2020-11-03 15:27 ` Ezequiel Garcia 2020-10-29 13:07 ` Ezequiel Garcia 2020-10-29 13:07 ` Ezequiel Garcia 2020-10-29 14:15 ` Robin Murphy 2020-10-29 14:15 ` Robin Murphy 2020-10-29 14:48 ` Mark Brown 2020-10-29 14:48 ` Mark Brown 2020-10-29 16:27 ` Ezequiel Garcia 2020-10-29 16:27 ` Ezequiel Garcia 2020-10-29 17:59 ` Mark Brown 2020-10-29 17:59 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201012205957.889185-7-adrian.ratiu@collabora.com \ --to=adrian.ratiu@collabora.com \ --cc=Peter.Fruehberger@de.bosch.com \ --cc=broonie@kernel.org \ --cc=daniel@ffwll.ch \ --cc=ezequiel@collabora.com \ --cc=kernel@collabora.com \ --cc=kuhanh.murugasen.krishnan@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=mchehab@kernel.org \ --cc=p.zabel@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.