Re: [PATCH 1/6] media: qcom: camss: Add support for TFE (Spectra 340)

Bryan O'Donoghue
2025-04-16T12:27:48Z

On 16/04/2025 13:09, Loic Poulain wrote:
> Add support for TFE (Thin Front End) found in QCM2290.
>
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
> drivers/media/platform/qcom/camss/Makefile | 1 +
> .../media/platform/qcom/camss/camss-vfe-340.c | 281 ++++++++++++++++++
> drivers/media/platform/qcom/camss/camss-vfe.h | 1 +
> 3 files changed, 283 insertions(+)
> create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-340.c
>
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index d26a9c24a430..719898f5d32b 100644
> --- a/drivers/media/platform/qcom/camss/Makefile
> +++ b/drivers/media/platform/qcom/camss/Makefile
> @@ -17,6 +17,7 @@ qcom-camss-objs += \
> camss-vfe-4-7.o \
> camss-vfe-4-8.o \
> camss-vfe-17x.o \
> + camss-vfe-340.o \
> camss-vfe-480.o \
> camss-vfe-680.o \
> camss-vfe-780.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-340.c b/drivers/media/platform/qcom/camss/camss-vfe-340.c
> new file mode 100644
> index 000000000000..fc454d66e1d2
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-340.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module 340 (TFE)
> + *
> + * Copyright (c) 2025 Qualcomm Technologies, Inc.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +
> +#include "camss.h"
> +#include "camss-vfe.h"
> +
> +#define TFE_GLOBAL_RESET_CMD (0x014)
> +#define TFE_GLOBAL_RESET_CMD_CORE BIT(0)
> +
> +#define TFE_REG_UPDATE_CMD (0x02C)
> +
> +#define TFE_IRQ_CMD (0x030)
> +#define TFE_IRQ_CMD_CLEAR BIT(0)
> +#define TFE_IRQ_MASK_0 (0x034)
> +#define TFE_IRQ_MASK_0_RST_DONE BIT(0)
> +#define TFE_IRQ_MASK_0_BUS_WR BIT(1)
> +#define TFE_IRQ_MASK_1 (0x038)
> +#define TFE_IRQ_MASK_2 (0x03c)
> +#define TFE_IRQ_CLEAR_0 (0x040)
> +
> +#define TFE_IRQ_STATUS_0 (0x04c)
> +
> +#define BUS_REG(a) (0xa00 + (a))
> +
> +#define TFE_BUS_IRQ_MASK_0 BUS_REG(0x18)
> +#define TFE_BUS_IRQ_MASK_RUP_DONE_ALL 0x000f
> +#define TFE_BUS_IRQ_MASK_RUP_DONE(src) BIT(src)
> +#define TFE_BUS_IRQ_MASK_BUF_DONE_ALL 0xff00
> +#define TFE_BUS_IRQ_MASK_BUF_DONE(sg) BIT((sg) + 8)
> +#define TFE_BUS_IRQ_MASK_0_CONS_VIOL BIT(28)
> +#define TFE_BUS_IRQ_MASK_0_VIOL BIT(30)
> +#define TFE_BUS_IRQ_MASK_0_IMG_VIOL BIT(31)
> +
> +#define TFE_BUS_IRQ_MASK_1 BUS_REG(0x1C)
> +#define TFE_BUS_IRQ_CLEAR_0 BUS_REG(0x20)
> +#define TFE_BUS_IRQ_STATUS_0 BUS_REG(0x28)
> +#define TFE_BUS_IRQ_CMD BUS_REG(0x30)
> +#define TFE_BUS_IRQ_CMD_CLEAR BIT(0)
> +
> +#define TFE_BUS_STATUS_CLEAR BUS_REG(0x60)
> +#define TFE_BUS_VIOLATION_STATUS BUS_REG(0x64)
> +#define TFE_BUS_OVERFLOW_STATUS BUS_REG(0x68)
> +#define TFE_BUS_IMAGE_SZ_VIOLATION_STATUS BUS_REG(0x70)
> +
> +#define TFE_BUS_CLIENT_CFG(c) BUS_REG(0x200 + (c) * 0x100)
> +#define TFE_BUS_CLIENT_CFG_EN BIT(0)
> +#define TFE_BUS_CLIENT_CFG_MODE_FRAME BIT(16)
> +#define TFE_BUS_IMAGE_ADDR(c) BUS_REG(0x204 + (c) * 0x100)
> +#define TFE_BUS_FRAME_INCR(c) BUS_REG(0x208 + (c) * 0x100)
> +#define TFE_BUS_IMAGE_CFG_0(c) BUS_REG(0x20C + (c) * 0x100)
> +#define TFE_BUS_IMAGE_CFG_0_DEFAULT 0xffff
> +#define TFE_BUS_IMAGE_CFG_1(c) BUS_REG(0x210 + (c) * 0x100)
> +#define TFE_BUS_IMAGE_CFG_2(c) BUS_REG(0x214 + (c) * 0x100)
> +#define TFE_BUS_IMAGE_CFG_2_DEFAULT 0xffff
> +#define TFE_BUS_PACKER_CFG(c) BUS_REG(0x218 + (c) * 0x100)
> +#define TFE_BUS_PACKER_CFG_FMT_PLAIN64 0xa
> +#define TFE_BUS_IRQ_SUBSAMPLE_CFG_0(c) BUS_REG(0x230 + (c) * 0x100)
> +#define TFE_BUS_IRQ_SUBSAMPLE_CFG_1(c) BUS_REG(0x234 + (c) * 0x100)
> +#define TFE_BUS_FRAMEDROP_CFG_0(c) BUS_REG(0x238 + (c) * 0x100)
> +#define TFE_BUS_FRAMEDROP_CFG_1(c) BUS_REG(0x23c + (c) * 0x100)
> +
> +#define RDI_CLIENT(n) (7 + (n))

Should be RDI_WM for consistency.

Please add a comment listing what those clients are

https://gitlab.freedesktop.org/linux-media/media-committers/-/commit/727970e9725c35366f501a890584e466ae34224a#c0063099c8260d8c4b3dc9ea650b6d0a4db47518_0_59

> +#define TFE_SOURCES_NUM 4
> +#define TFE_SUBGROUPS_NUM 8
> +#define TFE_CLIENTS_NUM 10
> +
> +static inline unsigned int __regupdate_iface(enum vfe_line_id line_id)
> +{
> + switch (line_id) {
> + case VFE_LINE_RDI0:
> + return 1;
> + case VFE_LINE_RDI1:
> + return 2;
> + case VFE_LINE_RDI2:
> + return 3;
> + case VFE_LINE_PIX:
> + default:
> + return 0;
> + }

An enum or a define instead of hard-coded values on the return please.

> +}
> +
> +static inline unsigned int __regupdate_line(unsigned int iface)
> +{
> + if (!iface)
> + return VFE_LINE_PIX;
> + if (iface < 4)
> + return VFE_LINE_RDI0 + (iface - 1);
> +
> + return VFE_LINE_NONE;
> +}
> +
> +static inline unsigned int __subgroup_line(unsigned int subgroup)
> +{
> + switch (subgroup) {
> + case 5:
> + return VFE_LINE_RDI0;
> + case 6:
> + return VFE_LINE_RDI1;
> + case 7:
> + return VFE_LINE_RDI2;
> + default:
> + return VFE_LINE_PIX;
> + }
> +
> + return VFE_LINE_NONE;
> +}
> +
> +static void vfe_global_reset(struct vfe_device *vfe)
> +{
> + writel_relaxed(TFE_IRQ_MASK_0_RST_DONE, vfe->base + TFE_IRQ_MASK_0);
> + writel(TFE_GLOBAL_RESET_CMD_CORE, vfe->base + TFE_GLOBAL_RESET_CMD);

Why is one relaxed and one not ?

> +}
> +
> +static irqreturn_t vfe_isr(int irq, void *dev)
> +{
> + struct vfe_device *vfe = dev;
> + u32 status;
> + int i;
> +
> + status = readl_relaxed(vfe->base + TFE_IRQ_STATUS_0);
> + writel_relaxed(status, vfe->base + TFE_IRQ_CLEAR_0);
> + writel_relaxed(TFE_IRQ_CMD_CLEAR, vfe->base + TFE_IRQ_CMD);
> +
> +
> + if (status & TFE_IRQ_MASK_0_RST_DONE) {
> + dev_dbg(vfe->camss->dev, "VFE%u: Reset done!", vfe->id);
> + vfe_isr_reset_ack(vfe);
> + }
> +
> + if (status & TFE_IRQ_MASK_0_BUS_WR) {
> + u32 bus_status = readl_relaxed(vfe->base + TFE_BUS_IRQ_STATUS_0);
> +
> + writel_relaxed(bus_status, vfe->base + TFE_BUS_IRQ_CLEAR_0);
> + writel_relaxed(TFE_BUS_IRQ_CMD_CLEAR, vfe->base + TFE_BUS_IRQ_CMD);
> +
> + for (i = 0; i < TFE_SOURCES_NUM; i++) {
> + if (bus_status & TFE_BUS_IRQ_MASK_RUP_DONE(i))
> + vfe->res->hw_ops->reg_update_clear(vfe, __regupdate_line(i));
> + }
> +
> + for (i = 0; i < TFE_SUBGROUPS_NUM; i++) {
> + if (bus_status & TFE_BUS_IRQ_MASK_BUF_DONE(i))
> + vfe_buf_done(vfe, __subgroup_line(i));
> + }
> +
> + if (bus_status & TFE_BUS_IRQ_MASK_0_CONS_VIOL)
> + dev_err_ratelimited(vfe->camss->dev, "VFE%u: Bad config violation",
> + vfe->id);
> +
> + if (bus_status & TFE_BUS_IRQ_MASK_0_VIOL)
> + dev_err_ratelimited(vfe->camss->dev, "VFE%u: Input data violation",
> + vfe->id);
> +
> + if (bus_status & TFE_BUS_IRQ_MASK_0_IMG_VIOL)
> + dev_err_ratelimited(vfe->camss->dev, "VFE%u: Image size violation",
> + vfe->id);
> + }
> +
> + status = readl_relaxed(vfe->base + TFE_BUS_OVERFLOW_STATUS);
> + if (status) {
> + writel_relaxed(status, vfe->base + TFE_BUS_STATUS_CLEAR);
> + for (i = 0; i < TFE_CLIENTS_NUM; i++) {
> + if (status & BIT(i))
> + dev_err_ratelimited(vfe->camss->dev,
> + "VFE%u: bus overflow for client %u\n",
> + vfe->id, i);
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}

looks ok

> +
> +static int vfe_halt(struct vfe_device *vfe)
> +{
> + /* rely on vfe_disable_output() to stop the VFE */
> + return 0;
> +}
> +
> +static void vfe_enable_irq(struct vfe_device *vfe)
> +{
> + writel_relaxed(TFE_IRQ_MASK_0_RST_DONE | TFE_IRQ_MASK_0_BUS_WR,
> + vfe->base + TFE_IRQ_MASK_0);
> + writel_relaxed(TFE_BUS_IRQ_MASK_RUP_DONE_ALL | TFE_BUS_IRQ_MASK_BUF_DONE_ALL |
> + TFE_BUS_IRQ_MASK_0_CONS_VIOL | TFE_BUS_IRQ_MASK_0_VIOL |
> + TFE_BUS_IRQ_MASK_0_IMG_VIOL, vfe->base + TFE_BUS_IRQ_MASK_0);
> +}
> +
> +static void vfe_wm_update(struct vfe_device *vfe, u8 rdi, u32 addr,
> + struct vfe_line *line)
> +{
> + u8 client = RDI_CLIENT(rdi);
> +
> + writel_relaxed(addr, vfe->base + TFE_BUS_IMAGE_ADDR(client));
> +}
> +
> +static void vfe_wm_start(struct vfe_device *vfe, u8 rdi, struct vfe_line *line)
> +{
> + struct v4l2_pix_format_mplane *pix =
> + &line->video_out.active_fmt.fmt.pix_mp;
> + u32 stride = pix->plane_fmt[0].bytesperline;
> + u8 client = RDI_CLIENT(rdi);
> +
> + /* Configuration for plain RDI frames */
> + writel_relaxed(TFE_BUS_IMAGE_CFG_0_DEFAULT, vfe->base + TFE_BUS_IMAGE_CFG_0(client));
> + writel_relaxed(0u, vfe->base + TFE_BUS_IMAGE_CFG_1(client));
> + writel_relaxed(TFE_BUS_IMAGE_CFG_2_DEFAULT, vfe->base + TFE_BUS_IMAGE_CFG_2(client));
> + writel_relaxed(stride * pix->height, vfe->base + TFE_BUS_FRAME_INCR(client));
> + writel_relaxed(TFE_BUS_PACKER_CFG_FMT_PLAIN64, vfe->base + TFE_BUS_PACKER_CFG(client));
> +
> + /* No dropped frames, one irq per frame */
> + writel_relaxed(0, vfe->base + TFE_BUS_FRAMEDROP_CFG_0(client));
> + writel_relaxed(1, vfe->base + TFE_BUS_FRAMEDROP_CFG_1(client));
> + writel_relaxed(0, vfe->base + TFE_BUS_IRQ_SUBSAMPLE_CFG_0(client));
> + writel_relaxed(1, vfe->base + TFE_BUS_IRQ_SUBSAMPLE_CFG_1(client));
> +
> + vfe_enable_irq(vfe);
> +
> + writel(TFE_BUS_CLIENT_CFG_EN | TFE_BUS_CLIENT_CFG_MODE_FRAME,
> + vfe->base + TFE_BUS_CLIENT_CFG(client));
> +
> + dev_dbg(vfe->camss->dev, "VFE%u: Started RDI%u width %u height %u stride %u\n",
> + vfe->id, rdi, pix->width, pix->height, stride);
> +}
> +
> +static void vfe_wm_stop(struct vfe_device *vfe, u8 rdi)
> +{
> + u8 client = RDI_CLIENT(rdi);
> +
> + writel(0, vfe->base + TFE_BUS_CLIENT_CFG(client));
> +
> + dev_dbg(vfe->camss->dev, "VFE%u: Stopped RDI%u\n", vfe->id, rdi);
> +}
> +
> +static const struct camss_video_ops vfe_video_ops_520 = {
> + .queue_buffer = vfe_queue_buffer_v2,
> + .flush_buffers = vfe_flush_buffers,
> +};
> +
> +static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
> +{
> + vfe->video_ops = vfe_video_ops_520;
> +}
> +
> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> + vfe->reg_update |= BIT(__regupdate_iface(line_id));
> + writel_relaxed(vfe->reg_update, vfe->base + TFE_REG_UPDATE_CMD);
> +}
> +
> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
> + enum vfe_line_id line_id)
> +{
> + vfe->reg_update &= ~BIT(__regupdate_iface(line_id));
> +}
> +
> +const struct vfe_hw_ops vfe_ops_340 = {
> + .global_reset = vfe_global_reset,
> + .hw_version = vfe_hw_version,
> + .isr = vfe_isr,
> + .pm_domain_off = vfe_pm_domain_off,
> + .pm_domain_on = vfe_pm_domain_on,
> + .subdev_init = vfe_subdev_init,
> + .vfe_disable = vfe_disable,
> + .vfe_enable = vfe_enable_v2,
> + .vfe_halt = vfe_halt,
> + .vfe_wm_start = vfe_wm_start,
> + .vfe_wm_stop = vfe_wm_stop,
> + .vfe_buf_done = vfe_buf_done,
> + .vfe_wm_update = vfe_wm_update,
> + .reg_update = vfe_reg_update,
> + .reg_update_clear = vfe_reg_update_clear,
> +};
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index a23f666be753..9b138849caca 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -242,6 +242,7 @@ extern const struct vfe_hw_ops vfe_ops_4_1;
> extern const struct vfe_hw_ops vfe_ops_4_7;
> extern const struct vfe_hw_ops vfe_ops_4_8;
> extern const struct vfe_hw_ops vfe_ops_170;
> +extern const struct vfe_hw_ops vfe_ops_340;
> extern const struct vfe_hw_ops vfe_ops_480;
> extern const struct vfe_hw_ops vfe_ops_680;
> extern const struct vfe_hw_ops vfe_ops_780;