[13/13] media: qcom: camss: Add support for VFE hardware version Titan 780

Add support for VFE found on SM8550 (Titan 780). This implementation is
based on the titan 480 implementation. It supports the normal and lite
VFE.

Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
 drivers/media/platform/qcom/camss/Makefile    |   1 +
 .../media/platform/qcom/camss/camss-vfe-780.c | 148 ++++++++++++++++++
 drivers/media/platform/qcom/camss/camss-vfe.c |  33 ++--
 drivers/media/platform/qcom/camss/camss-vfe.h |   1 +
 drivers/media/platform/qcom/camss/camss.c     | 132 ++++++++++++++++
 drivers/media/platform/qcom/camss/camss.h     |   2 +
 6 files changed, 304 insertions(+), 13 deletions(-)
 create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c

Hi Depeng,

please find a few review comments, all asked changes are non-functional.

On 8/12/24 17:41, Depeng Shao wrote:
> Add support for VFE found on SM8550 (Titan 780). This implementation is
> based on the titan 480 implementation. It supports the normal and lite
> VFE.
> 
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>   .../media/platform/qcom/camss/camss-vfe-780.c | 148 ++++++++++++++++++
>   drivers/media/platform/qcom/camss/camss-vfe.c |  33 ++--
>   drivers/media/platform/qcom/camss/camss-vfe.h |   1 +
>   drivers/media/platform/qcom/camss/camss.c     | 132 ++++++++++++++++
>   drivers/media/platform/qcom/camss/camss.h     |   2 +
>   6 files changed, 304 insertions(+), 13 deletions(-)
>   create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c
> 
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index c336e4c1a399..a83b7a8dcef7 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-8.o \
>   		camss-vfe-17x.o \
>   		camss-vfe-480.o \
> +		camss-vfe-780.o \
>   		camss-vfe-gen1.o \
>   		camss-vfe.o \
>   		camss-video.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> new file mode 100644
> index 000000000000..e1c4d25cdc40
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-vfe-780.c

I understand that a file name copied from a previous file and updated,
let's just remove it, it serves no purpose, but adds this unnecessary
work on every next copy.

> + *
> + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module v780 (SM8550)
> + *
> + * Copyright (c) 2024 Qualcomm Technologies, Inc.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +
> +#include "camss.h"
> +#include "camss-vfe.h"
> +
> +#define BUS_REG_BASE			(vfe_is_lite(vfe) ? 0x200 : 0xC00)
> +
> +#define VFE_BUS_WM_CGC_OVERRIDE		(BUS_REG_BASE + 0x08)
> +#define		WM_CGC_OVERRIDE_ALL		(0x7FFFFFF)
> +
> +#define VFE_BUS_WM_TEST_BUS_CTRL	(BUS_REG_BASE + 0xDC)
> +
> +#define VFE_BUS_WM_CFG(n)		(BUS_REG_BASE + 0x200 + (n) * 0x100)
> +#define		WM_CFG_EN			BIT(0)
> +#define		WM_VIR_FRM_EN			BIT(1)
> +#define		WM_CFG_MODE			BIT(16)
> +#define VFE_BUS_WM_IMAGE_ADDR(n)	(BUS_REG_BASE + 0x204 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAME_INCR(n)	(BUS_REG_BASE + 0x208 + (n) * 0x100)
> +#define VFE_BUS_WM_IMAGE_CFG_0(n)	(BUS_REG_BASE + 0x20c + (n) * 0x100)
> +#define		WM_IMAGE_CFG_0_DEFAULT_WIDTH	(0xFFFF)
> +#define VFE_BUS_WM_IMAGE_CFG_1(n)	(BUS_REG_BASE + 0x210 + (n) * 0x100)
> +#define VFE_BUS_WM_IMAGE_CFG_2(n)	(BUS_REG_BASE + 0x214 + (n) * 0x100)
> +#define		WM_IMAGE_CFG_2_DEFAULT_STRIDE	(0xFFFF)
> +#define VFE_BUS_WM_PACKER_CFG(n)	(BUS_REG_BASE + 0x218 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_ADDR(n)	(BUS_REG_BASE + 0x220 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_INCR(n)	(BUS_REG_BASE + 0x224 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_CFG(n)	(BUS_REG_BASE + 0x228 + (n) * 0x100)

Three VFE_BUS_WM_HEADER_* macra above are not used, please remove.

> +
> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(n)	(BUS_REG_BASE + 0x230 + (n) * 0x100)
> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(n)	(BUS_REG_BASE + 0x234 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAMEDROP_PERIOD(n)		(BUS_REG_BASE + 0x238 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAMEDROP_PATTERN(n)		(BUS_REG_BASE + 0x23c + (n) * 0x100)
> +
> +#define VFE_BUS_WM_MMU_PREFETCH_CFG(n)		(BUS_REG_BASE + 0x260 + (n) * 0x100)
> +#define VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(n)	(BUS_REG_BASE + 0x264 + (n) * 0x100)
> +#define VFE_BUS_WM_SYSTEM_CACHE_CFG(n)		(BUS_REG_BASE + 0x268 + (n) * 0x100)

Good to know that there is such a register, but it's not used,
please remove the macro.

> +
> +/* for titan 780, each bus client is hardcoded to a specific path */
> +#define RDI_WM(n)			((vfe_is_lite(vfe) ? 0x0 : 0x17) + (n))
> +
> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
> +{
> +	struct v4l2_pix_format_mplane *pix =
> +		&line->video_out.active_fmt.fmt.pix_mp;
> +
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */

Please move the comment on its own line.

> +
> +	/* no clock gating at bus input */
> +	writel(WM_CGC_OVERRIDE_ALL, vfe->base + VFE_BUS_WM_CGC_OVERRIDE);
> +
> +	writel(0x0, vfe->base + VFE_BUS_WM_TEST_BUS_CTRL);
> +
> +	writel(ALIGN(pix->plane_fmt[0].bytesperline, 16) * pix->height >> 8,
> +	       vfe->base + VFE_BUS_WM_FRAME_INCR(wm));
> +	writel((WM_IMAGE_CFG_0_DEFAULT_WIDTH & 0xFFFF),
> +	       vfe->base + VFE_BUS_WM_IMAGE_CFG_0(wm));
> +	writel(WM_IMAGE_CFG_2_DEFAULT_STRIDE,
> +	       vfe->base + VFE_BUS_WM_IMAGE_CFG_2(wm));
> +	writel(0, vfe->base + VFE_BUS_WM_PACKER_CFG(wm));
> +
> +	/* no dropped frames, one irq per frame */
> +	writel(0, vfe->base + VFE_BUS_WM_FRAMEDROP_PERIOD(wm));
> +	writel(1, vfe->base + VFE_BUS_WM_FRAMEDROP_PATTERN(wm));
> +	writel(0, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(wm));
> +	writel(1, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(wm));
> +
> +	writel(1, vfe->base + VFE_BUS_WM_MMU_PREFETCH_CFG(wm));
> +	writel(0xFFFFFFFF, vfe->base + VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(wm));
> +
> +	writel(WM_CFG_EN | WM_CFG_MODE, vfe->base + VFE_BUS_WM_CFG(wm));
> +}
> +
> +static void vfe_wm_stop(struct vfe_device *vfe, u8 wm)
> +{
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */

Please move the comment on its own line or remove it as obvious one.

> +	writel(0, vfe->base + VFE_BUS_WM_CFG(wm));
> +}
> +
> +static void vfe_wm_update(struct vfe_device *vfe, u8 wm, u32 addr,
> +			  struct vfe_line *line)
> +{
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */

Please move the comment on its own line or remove it as obvious one.

> +	writel((addr >> 8) & 0xFFFFFFFF, vfe->base + VFE_BUS_WM_IMAGE_ADDR(wm));
> +
> +	dev_dbg(vfe->camss->dev, "%s wm:%d, image buf addr:0x%x\n",
> +		__func__, wm, addr);

There will be no confusion in runtime about a source of the debug
message, please remove that __func__ information.

> +}
> +
> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.

Huh, it's unusual to see a network subsystem style comment formatting here.

There is a typo, s/beem/been/

> +	 * Notify the event of trigger RUP.
> +	 */

I suppose it would be good enough to remove the comment completely as
an obvious one.

> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
> +					enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP clear.
> +	 */

Same as above.

> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
> +}
> +
> +static const struct camss_video_ops vfe_video_ops_780 = {
> +	.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_780;
> +}
> +
> +const struct vfe_hw_ops vfe_ops_780 = {
> +	.enable_irq = NULL,
> +	.global_reset = NULL,
> +	.hw_version = vfe_hw_version,
> +	.isr = NULL,
> +	.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 = NULL,
> +	.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.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 71bd55e854bb..507fc7785ac8 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -343,6 +343,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_8550:
>   		switch (sink_code) {
>   		case MEDIA_BUS_FMT_YUYV8_1X16:
>   		{
> @@ -674,15 +675,17 @@ int vfe_reset(struct vfe_device *vfe)
>   {
>   	unsigned long time;
>   
> -	reinit_completion(&vfe->reset_complete);
> +	if (vfe->res->hw_ops->global_reset) {
> +		reinit_completion(&vfe->reset_complete);
>   
> -	vfe->res->hw_ops->global_reset(vfe);
> +		vfe->res->hw_ops->global_reset(vfe);
>   
> -	time = wait_for_completion_timeout(&vfe->reset_complete,
> -		msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
> -	if (!time) {
> -		dev_err(vfe->camss->dev, "VFE reset timeout\n");
> -		return -EIO;
> +		time = wait_for_completion_timeout(&vfe->reset_complete,
> +			msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
> +		if (!time) {
> +			dev_err(vfe->camss->dev, "VFE reset timeout\n");
> +			return -EIO;
> +		}

This goes to some other preceding change, since it's unrelated to Titan 780
support, but the latter depends on it.

>   	}
>   
>   	return 0;
> @@ -1120,7 +1123,8 @@ void vfe_put(struct vfe_device *vfe)
>   	} else if (vfe->power_count == 1) {
>   		if (vfe->was_streaming) {
>   			vfe->was_streaming = 0;
> -			vfe->res->hw_ops->vfe_halt(vfe);
> +			if (vfe->res->hw_ops->vfe_halt)
> +				vfe->res->hw_ops->vfe_halt(vfe);

This goes to some other change, since it's unrelated to Titan 780 support.

>   		}
>   		camss_disable_clocks(vfe->nclocks, vfe->clock);
>   		pm_runtime_put_sync(vfe->camss->dev);
> @@ -1807,11 +1811,13 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>   	vfe->irq = ret;
>   	snprintf(vfe->irq_name, sizeof(vfe->irq_name), "%s_%s%d",
>   		 dev_name(dev), MSM_VFE_NAME, id);
> -	ret = devm_request_irq(dev, vfe->irq, vfe->res->hw_ops->isr,
> -			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
> -	if (ret < 0) {
> -		dev_err(dev, "request_irq failed: %d\n", ret);
> -		return ret;
> +	if (vfe->res->hw_ops->isr) {
> +		ret = devm_request_irq(dev, vfe->irq, vfe->res->hw_ops->isr,
> +				       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
> +		if (ret < 0) {
> +			dev_err(dev, "request_irq failed: %d\n", ret);
> +			return ret;
> +		}

This change shall be done in a seperate preceding commit, since it's
unrelated to Titan 780 support.

>   	}
>   
>   	/* Clocks */
> @@ -1963,6 +1969,7 @@ static int vfe_bpl_align(struct vfe_device *vfe)
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_8550:
>   		ret = 16;
>   		break;
>   	default:
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index fcbf4f609129..9dec5bc0d1b1 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -243,6 +243,7 @@ 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_480;
> +extern const struct vfe_hw_ops vfe_ops_780;
>   
>   int vfe_get(struct vfe_device *vfe);
>   void vfe_put(struct vfe_device *vfe);
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 7ee102948dc4..92a0fa02e415 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1666,6 +1666,125 @@ static const struct camss_subdev_resources csid_res_8550[] = {
>   	}
>   };
>   
> +static const struct camss_subdev_resources vfe_res_8550[] = {
> +	/* VFE0 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe0_fast_ahb",
> +			   "vfe0", "cpas_vfe0", "camnoc_axi" },
> +		.clock_rate = { { 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe0" },
> +		.interrupt = { "vfe0" },
> +		.vfe = {
> +			.line_num = 3,
> +			.is_lite = false,
> +			.has_pd = true,
> +			.pd_name = "ife0",
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE1 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe1_fast_ahb",
> +			   "vfe1", "cpas_vfe1", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe1" },
> +		.interrupt = { "vfe1" },
> +		.vfe = {
> +			.line_num = 3,
> +			.is_lite = false,
> +			.has_pd = true,
> +			.pd_name = "ife1",
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE2 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe2_fast_ahb",
> +			   "vfe2", "cpas_vfe2", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe2" },
> +		.interrupt = { "vfe2" },
> +		.vfe = {
> +			.line_num = 3,
> +			.is_lite = false,
> +			.has_pd = true,
> +			.pd_name = "ife2",
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE3 lite */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe_lite_ahb",
> +			   "vfe_lite", "cpas_ife_lite", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe_lite0" },
> +		.interrupt = { "vfe_lite0" },
> +		.vfe = {
> +			.line_num = 4,
> +			.is_lite = true,
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE4 lite */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe_lite_ahb",
> +			   "vfe_lite", "cpas_ife_lite", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe_lite1" },
> +		.interrupt = { "vfe_lite1" },
> +		.vfe = {
> +			.line_num = 4,
> +			.is_lite = true,
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +};
> +
>   static const struct resources_icc icc_res_sm8550[] = {
>   	{
>   		.name = "ahb",
> @@ -1846,6 +1965,17 @@ void camss_pm_domain_off(struct camss *camss, int id)
>   	}
>   }
>   
> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, bool is_clear)

Please let it be just a declarative 'clear' instead of questioning 'is_clear'.

> +{
> +	struct csid_device *csid;
> +
> +	if (hw_id < camss->res->csid_num) {
> +		csid = &(camss->csid[hw_id]);
> +
> +		csid->res->hw_ops->reg_update(csid, port_id, is_clear);
> +	}
> +}
> +

Please add the new exported function camss_reg_update() in a separate
preceding commit.

>   void camss_buf_done(struct camss *camss, int hw_id, int port_id)
>   {
>   	struct vfe_device *vfe;
> @@ -2668,10 +2798,12 @@ static const struct camss_resources sm8550_resources = {
>   	.pd_name = "top",
>   	.csiphy_res = csiphy_res_8550,
>   	.csid_res = csid_res_8550,
> +	.vfe_res = vfe_res_8550,
>   	.icc_res = icc_res_sm8550,
>   	.icc_path_num = ARRAY_SIZE(icc_res_sm8550),
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_8550),
>   	.csid_num = ARRAY_SIZE(csid_res_8550),
> +	.vfe_num = ARRAY_SIZE(vfe_res_8550),
>   	.link_entities = camss_link_entities
>   };
>   
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index d6b6558a82b9..697846e70e78 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -157,5 +157,7 @@ int camss_vfe_get(struct camss *camss, int id);
>   void camss_vfe_put(struct camss *camss, int id);
>   void camss_delete(struct camss *camss);
>   void camss_buf_done(struct camss *camss, int hw_id, int port_id);
> +void camss_reg_update(struct camss *camss, int hw_id,
> +		      int port_id, bool is_clear);
>   
>   #endif /* QC_MSM_CAMSS_H */

Thank you for the efforts to get support of Titan 780 in the upstream.

--
Best wishes,
Vladimir

Hi Vladimir,

On 8/14/2024 7:13 PM, Vladimir Zapolskiy wrote:
> Hi Depeng,
> 
> please find a few review comments, all asked changes are non-functional.
> 

>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, 
>> bool is_clear)
> 
> Please let it be just a declarative 'clear' instead of questioning 
> 'is_clear'.
> 
>> +{
>> +    struct csid_device *csid;
>> +
>> +    if (hw_id < camss->res->csid_num) {
>> +        csid = &(camss->csid[hw_id]);
>> +
>> +        csid->res->hw_ops->reg_update(csid, port_id, is_clear);
>> +    }
>> +}
>> +
> 
> Please add the new exported function camss_reg_update() in a separate
> preceding commit.
> 
>>   void camss_buf_done(struct camss *camss, int hw_id, int port_id)
>>   {
>>       struct vfe_device *vfe;

Thanks for your comments, I will address them in new series.

But I have some concern about above comment, you want to add a separate 
commit for camss_reg_update, maybe camss_buf_done also need to do this, 
but I guess I will get new comments from Krzysztof if I make a separate 
change, Krzysztof posted few comments in v3 series, he asked, "must 
organize your patches in logical junks" and the code must have a user.

Please check below comments.

https://lore.kernel.org/all/e1b298df-05da-4881-a628-149a8a625544@kernel.org/

https://lore.kernel.org/all/d0f8b72d-4355-43cd-a5f9-c44aab8147e5@kernel.org/


Or I don't add reg update and buf done functionality in 
camss-csid-gen3.c and camss-vfe-780.c firstly, then add them in a later 
commit.

Could you please comment on whether this is acceptable? Please also help 
to common on if one commit to add them or need two separate commits, one 
is for reg update and the other one is for buf done.


Thanks,
Depeng

On 12/08/2024 15:41, Depeng Shao wrote:
> Add support for VFE found on SM8550 (Titan 780). This implementation is
> based on the titan 480 implementation. It supports the normal and lite
> VFE.
> 
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>   .../media/platform/qcom/camss/camss-vfe-780.c | 148 ++++++++++++++++++
>   drivers/media/platform/qcom/camss/camss-vfe.c |  33 ++--
>   drivers/media/platform/qcom/camss/camss-vfe.h |   1 +
>   drivers/media/platform/qcom/camss/camss.c     | 132 ++++++++++++++++
>   drivers/media/platform/qcom/camss/camss.h     |   2 +
>   6 files changed, 304 insertions(+), 13 deletions(-)
>   create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c
> 
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index c336e4c1a399..a83b7a8dcef7 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-8.o \
>   		camss-vfe-17x.o \
>   		camss-vfe-480.o \
> +		camss-vfe-780.o \
>   		camss-vfe-gen1.o \
>   		camss-vfe.o \
>   		camss-video.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> new file mode 100644
> index 000000000000..e1c4d25cdc40
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-vfe-780.c
> + *
> + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module v780 (SM8550)
> + *
> + * Copyright (c) 2024 Qualcomm Technologies, Inc.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +
> +#include "camss.h"
> +#include "camss-vfe.h"
> +
> +#define BUS_REG_BASE			(vfe_is_lite(vfe) ? 0x200 : 0xC00)
> +
> +#define VFE_BUS_WM_CGC_OVERRIDE		(BUS_REG_BASE + 0x08)
> +#define		WM_CGC_OVERRIDE_ALL		(0x7FFFFFF)
> +
> +#define VFE_BUS_WM_TEST_BUS_CTRL	(BUS_REG_BASE + 0xDC)
> +
> +#define VFE_BUS_WM_CFG(n)		(BUS_REG_BASE + 0x200 + (n) * 0x100)
> +#define		WM_CFG_EN			BIT(0)
> +#define		WM_VIR_FRM_EN			BIT(1)
> +#define		WM_CFG_MODE			BIT(16)
> +#define VFE_BUS_WM_IMAGE_ADDR(n)	(BUS_REG_BASE + 0x204 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAME_INCR(n)	(BUS_REG_BASE + 0x208 + (n) * 0x100)
> +#define VFE_BUS_WM_IMAGE_CFG_0(n)	(BUS_REG_BASE + 0x20c + (n) * 0x100)
> +#define		WM_IMAGE_CFG_0_DEFAULT_WIDTH	(0xFFFF)
> +#define VFE_BUS_WM_IMAGE_CFG_1(n)	(BUS_REG_BASE + 0x210 + (n) * 0x100)
> +#define VFE_BUS_WM_IMAGE_CFG_2(n)	(BUS_REG_BASE + 0x214 + (n) * 0x100)
> +#define		WM_IMAGE_CFG_2_DEFAULT_STRIDE	(0xFFFF)
> +#define VFE_BUS_WM_PACKER_CFG(n)	(BUS_REG_BASE + 0x218 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_ADDR(n)	(BUS_REG_BASE + 0x220 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_INCR(n)	(BUS_REG_BASE + 0x224 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_CFG(n)	(BUS_REG_BASE + 0x228 + (n) * 0x100)
> +
> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(n)	(BUS_REG_BASE + 0x230 + (n) * 0x100)
> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(n)	(BUS_REG_BASE + 0x234 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAMEDROP_PERIOD(n)		(BUS_REG_BASE + 0x238 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAMEDROP_PATTERN(n)		(BUS_REG_BASE + 0x23c + (n) * 0x100)
> +
> +#define VFE_BUS_WM_MMU_PREFETCH_CFG(n)		(BUS_REG_BASE + 0x260 + (n) * 0x100)
> +#define VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(n)	(BUS_REG_BASE + 0x264 + (n) * 0x100)
> +#define VFE_BUS_WM_SYSTEM_CACHE_CFG(n)		(BUS_REG_BASE + 0x268 + (n) * 0x100)
> +
> +/* for titan 780, each bus client is hardcoded to a specific path */
> +#define RDI_WM(n)			((vfe_is_lite(vfe) ? 0x0 : 0x17) + (n))
> +
> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
> +{
> +	struct v4l2_pix_format_mplane *pix =
> +		&line->video_out.active_fmt.fmt.pix_mp;
> +
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> +
> +	/* no clock gating at bus input */
> +	writel(WM_CGC_OVERRIDE_ALL, vfe->base + VFE_BUS_WM_CGC_OVERRIDE);
> +
> +	writel(0x0, vfe->base + VFE_BUS_WM_TEST_BUS_CTRL);
> +
> +	writel(ALIGN(pix->plane_fmt[0].bytesperline, 16) * pix->height >> 8,
> +	       vfe->base + VFE_BUS_WM_FRAME_INCR(wm));
> +	writel((WM_IMAGE_CFG_0_DEFAULT_WIDTH & 0xFFFF),
> +	       vfe->base + VFE_BUS_WM_IMAGE_CFG_0(wm));
> +	writel(WM_IMAGE_CFG_2_DEFAULT_STRIDE,
> +	       vfe->base + VFE_BUS_WM_IMAGE_CFG_2(wm));
> +	writel(0, vfe->base + VFE_BUS_WM_PACKER_CFG(wm));
> +
> +	/* no dropped frames, one irq per frame */
> +	writel(0, vfe->base + VFE_BUS_WM_FRAMEDROP_PERIOD(wm));
> +	writel(1, vfe->base + VFE_BUS_WM_FRAMEDROP_PATTERN(wm));
> +	writel(0, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(wm));
> +	writel(1, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(wm));
> +
> +	writel(1, vfe->base + VFE_BUS_WM_MMU_PREFETCH_CFG(wm));
> +	writel(0xFFFFFFFF, vfe->base + VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(wm));
> +
> +	writel(WM_CFG_EN | WM_CFG_MODE, vfe->base + VFE_BUS_WM_CFG(wm));
> +}
> +
> +static void vfe_wm_stop(struct vfe_device *vfe, u8 wm)
> +{
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> +	writel(0, vfe->base + VFE_BUS_WM_CFG(wm));
> +}
> +
> +static void vfe_wm_update(struct vfe_device *vfe, u8 wm, u32 addr,
> +			  struct vfe_line *line)
> +{
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> +	writel((addr >> 8) & 0xFFFFFFFF, vfe->base + VFE_BUS_WM_IMAGE_ADDR(wm));
> +
> +	dev_dbg(vfe->camss->dev, "%s wm:%d, image buf addr:0x%x\n",
> +		__func__, wm, addr);
> +}
> +
> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
> +					enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP clear.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
> +}
> +
> +static const struct camss_video_ops vfe_video_ops_780 = {
> +	.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_780;
> +}
> +
> +const struct vfe_hw_ops vfe_ops_780 = {
> +	.enable_irq = NULL,
> +	.global_reset = NULL,
> +	.hw_version = vfe_hw_version,
> +	.isr = NULL,
> +	.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 = NULL,
> +	.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.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 71bd55e854bb..507fc7785ac8 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -343,6 +343,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_8550:
>   		switch (sink_code) {
>   		case MEDIA_BUS_FMT_YUYV8_1X16:
>   		{
> @@ -674,15 +675,17 @@ int vfe_reset(struct vfe_device *vfe)
>   {
>   	unsigned long time;
>   
> -	reinit_completion(&vfe->reset_complete);
> +	if (vfe->res->hw_ops->global_reset) {
> +		reinit_completion(&vfe->reset_complete);
>   
> -	vfe->res->hw_ops->global_reset(vfe);
> +		vfe->res->hw_ops->global_reset(vfe);
>   
> -	time = wait_for_completion_timeout(&vfe->reset_complete,
> -		msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
> -	if (!time) {
> -		dev_err(vfe->camss->dev, "VFE reset timeout\n");
> -		return -EIO;
> +		time = wait_for_completion_timeout(&vfe->reset_complete,
> +			msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
> +		if (!time) {
> +			dev_err(vfe->camss->dev, "VFE reset timeout\n");
> +			return -EIO;
> +		}

Per my comment on the CSID - this feels like a fix you are introducing 
here in the guise of a silicon add.

Please break it up.

If you have a number of fixes to core functionality they need to be

1. Granular and individual
2. Indivdually scrutable with their own patch and descritption
3. git cherry-pickable
4. Have a Fixes tag
5. And be cc'd to stable@vger.kernel.org

Can't accept either the fixes or the silicon add if the two live mixed 
up in one patch.

>   	}
>   
>   	return 0;
> @@ -1120,7 +1123,8 @@ void vfe_put(struct vfe_device *vfe)
>   	} else if (vfe->power_count == 1) {
>   		if (vfe->was_streaming) {
>   			vfe->was_streaming = 0;
> -			vfe->res->hw_ops->vfe_halt(vfe);
> +			if (vfe->res->hw_ops->vfe_halt)
> +				vfe->res->hw_ops->vfe_halt(vfe);

Similar comment.

>   		}
>   		camss_disable_clocks(vfe->nclocks, vfe->clock);
>   		pm_runtime_put_sync(vfe->camss->dev);
> @@ -1807,11 +1811,13 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>   	vfe->irq = ret;
>   	snprintf(vfe->irq_name, sizeof(vfe->irq_name), "%s_%s%d",
>   		 dev_name(dev), MSM_VFE_NAME, id);
> -	ret = devm_request_irq(dev, vfe->irq, vfe->res->hw_ops->isr,
> -			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
> -	if (ret < 0) {
> -		dev_err(dev, "request_irq failed: %d\n", ret);
> -		return ret;
> +	if (vfe->res->hw_ops->isr) {

And again.

> +		ret = devm_request_irq(dev, vfe->irq, vfe->res->hw_ops->isr,
> +				       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
> +		if (ret < 0) {
> +			dev_err(dev, "request_irq failed: %d\n", ret);
> +			return ret;
> +		}
>   	}
>   
>   	/* Clocks */
> @@ -1963,6 +1969,7 @@ static int vfe_bpl_align(struct vfe_device *vfe)
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_8550:
>   		ret = 16;
>   		break;
>   	default:
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index fcbf4f609129..9dec5bc0d1b1 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -243,6 +243,7 @@ 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_480;
> +extern const struct vfe_hw_ops vfe_ops_780;
>   
>   int vfe_get(struct vfe_device *vfe);
>   void vfe_put(struct vfe_device *vfe);
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 7ee102948dc4..92a0fa02e415 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1666,6 +1666,125 @@ static const struct camss_subdev_resources csid_res_8550[] = {
>   	}
>   };
>   
> +static const struct camss_subdev_resources vfe_res_8550[] = {
> +	/* VFE0 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe0_fast_ahb",
> +			   "vfe0", "cpas_vfe0", "camnoc_axi" },

Should the camnoc AXI clock go here or in the CSID ?


> +		.clock_rate = { { 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe0" },
> +		.interrupt = { "vfe0" },
> +		.vfe = {
> +			.line_num = 3,
> +			.is_lite = false,
> +			.has_pd = true,
> +			.pd_name = "ife0",
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE1 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe1_fast_ahb",
> +			   "vfe1", "cpas_vfe1", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe1" },
> +		.interrupt = { "vfe1" },
> +		.vfe = {
> +			.line_num = 3,
> +			.is_lite = false,
> +			.has_pd = true,
> +			.pd_name = "ife1",
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE2 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe2_fast_ahb",
> +			   "vfe2", "cpas_vfe2", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe2" },
> +		.interrupt = { "vfe2" },
> +		.vfe = {
> +			.line_num = 3,
> +			.is_lite = false,
> +			.has_pd = true,
> +			.pd_name = "ife2",
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE3 lite */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe_lite_ahb",
> +			   "vfe_lite", "cpas_ife_lite", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe_lite0" },
> +		.interrupt = { "vfe_lite0" },
> +		.vfe = {
> +			.line_num = 4,
> +			.is_lite = true,
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE4 lite */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", "vfe_lite_ahb",
> +			   "vfe_lite", "cpas_ife_lite", "camnoc_axi" },
> +		.clock_rate = {	{ 0, 0, 0, 0, 0 },
> +				{ 0, 0, 0, 0, 80000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },

I realise you're specifying all of the operating points here but the 
clock only needs to appear once i.e.

1 x 300 MHz
1 x 400 MHz
1 x 480 MHz

etc.

> +				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
> +				{ 300000000, 300000000, 400000000, 400000000, 400000000 } },
> +		.reg = { "vfe_lite1" },
> +		.interrupt = { "vfe_lite1" },
> +		.vfe = {
> +			.line_num = 4,
> +			.is_lite = true,
> +			.hw_ops = &vfe_ops_780,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +};
> +
>   static const struct resources_icc icc_res_sm8550[] = {
>   	{
>   		.name = "ahb",
> @@ -1846,6 +1965,17 @@ void camss_pm_domain_off(struct camss *camss, int id)
>   	}
>   }
>   
> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, bool is_clear)
> +{
> +	struct csid_device *csid;
> +
> +	if (hw_id < camss->res->csid_num) {

Does this cause do anything ? Is it just defensive programming ? Can the 
hw_id index exceed the number of CSIDs defined and if so why ?

Smells wrong.

> +		csid = &(camss->csid[hw_id]);
> +
> +		csid->res->hw_ops->reg_update(csid, port_id, is_clear);
> +	}
> +}
> +
>   void camss_buf_done(struct camss *camss, int hw_id, int port_id)
>   {
>   	struct vfe_device *vfe;
> @@ -2668,10 +2798,12 @@ static const struct camss_resources sm8550_resources = {
>   	.pd_name = "top",
>   	.csiphy_res = csiphy_res_8550,
>   	.csid_res = csid_res_8550,
> +	.vfe_res = vfe_res_8550,
>   	.icc_res = icc_res_sm8550,
>   	.icc_path_num = ARRAY_SIZE(icc_res_sm8550),
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_8550),
>   	.csid_num = ARRAY_SIZE(csid_res_8550),
> +	.vfe_num = ARRAY_SIZE(vfe_res_8550),
>   	.link_entities = camss_link_entities
>   };
>   
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index d6b6558a82b9..697846e70e78 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -157,5 +157,7 @@ int camss_vfe_get(struct camss *camss, int id);
>   void camss_vfe_put(struct camss *camss, int id);
>   void camss_delete(struct camss *camss);
>   void camss_buf_done(struct camss *camss, int hw_id, int port_id);
> +void camss_reg_update(struct camss *camss, int hw_id,
> +		      int port_id, bool is_clear);
>   
>   #endif /* QC_MSM_CAMSS_H */

All in all good work, looks like good progress and thanks for sticking 
with your submissions.

Please follow up on the points raised.

---
bod

Hi Depeng,

On 8/14/24 16:10, Depeng Shao wrote:
> Hi Vladimir,
> 
> On 8/14/2024 7:13 PM, Vladimir Zapolskiy wrote:
>> Hi Depeng,
>>
>> please find a few review comments, all asked changes are non-functional.
>>
> 
>>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id,
>>> bool is_clear)
>>
>> Please let it be just a declarative 'clear' instead of questioning
>> 'is_clear'.
>>
>>> +{
>>> +    struct csid_device *csid;
>>> +
>>> +    if (hw_id < camss->res->csid_num) {
>>> +        csid = &(camss->csid[hw_id]);
>>> +
>>> +        csid->res->hw_ops->reg_update(csid, port_id, is_clear);
>>> +    }
>>> +}
>>> +
>>
>> Please add the new exported function camss_reg_update() in a separate
>> preceding commit.
>>
>>>    void camss_buf_done(struct camss *camss, int hw_id, int port_id)
>>>    {
>>>        struct vfe_device *vfe;
> 
> Thanks for your comments, I will address them in new series.
> 
> But I have some concern about above comment, you want to add a separate
> commit for camss_reg_update, maybe camss_buf_done also need to do this,
> but I guess I will get new comments from Krzysztof if I make a separate
> change, Krzysztof posted few comments in v3 series, he asked, "must
> organize your patches in logical junks" and the code must have a user.
> 
> Please check below comments.
> 
> https://lore.kernel.org/all/e1b298df-05da-4881-a628-149a8a625544@kernel.org/
> 
> https://lore.kernel.org/all/d0f8b72d-4355-43cd-a5f9-c44aab8147e5@kernel.org/

Krzysztof is absolutely right in his two comments.

 From what I see there is a difference between his concerns and mine ones
though, Krzysztof points to unused data, which should raise a build time
warning, and I asked to make a separate commit for a non-static function,
I believe it'll be removed by the linker silently...

The potential runtime logic change introduced by camss_reg_update() in the
generic code is not trivial, which opens an option to update/fix it lately
referencing a commit from generic domain rather than platform specific one.

If someone for whatever reasons wants to merge a new generic and shared
camss_reg_update() function within a the platform specific code/commit,
I won't strongly object, let it be merged together then.

> 
> Or I don't add reg update and buf done functionality in
> camss-csid-gen3.c and camss-vfe-780.c firstly, then add them in a later
> commit.
> 
> Could you please comment on whether this is acceptable? Please also help
> to common on if one commit to add them or need two separate commits, one
> is for reg update and the other one is for buf done.
> 

I would prefer to see two more separate commits within non-platform specific
code, however as I stated above if it causes anyone's concerns, including
your own, let it be kept as it is done today. Eventually we do discuss
a non-functional change.

--
Best wishes,
Vladimir

On 12/08/2024 15:41, Depeng Shao wrote:
> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
> +					enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP clear.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
> +}

Hmm, so another thought here.

camss_reg_update() is not an accurate name -> camss_rup_update() because 
in this case we only update the RUP register, not the AUP or MUP.

reg is an abbreviation for register - but RUP has a defined meaning in 
the camera namespace i.e. RUP = register update and its job is to latch 
shadow registers to real registers.

camss_rup_update() please.

---
bod

On 12/08/2024 15:41, Depeng Shao wrote:
> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, bool is_clear)
> +{
> +	struct csid_device *csid;
> +
> +	if (hw_id < camss->res->csid_num) {
> +		csid = &(camss->csid[hw_id]);
> +
> +		csid->res->hw_ops->reg_update(csid, port_id, is_clear);
> +	}
> +}

The naming here doesn't make the action clear

hw_ops->rup_update(csid, port, clear);

"is_clear" is not required since the type is a bool the "is" is implied 
in the the logical state so just "clear" will do.

But re: my previous comment on having the ISR do the clear as is done in 
the VFE 480, I don't think this is_clear parameter is warranted.

We want the calling function to request the rup_update() for the 
rup_update() function to wait on completion and the ISR() to do the 
clear once the RUP interrupt has been raised.

At least I think that's how it should work - could you please experiment 
with your code for the flow - as it appears to match the VFE 480 logic.

---
bod

Hi Bryan,

On 8/15/2024 12:23 AM, Bryan O'Donoghue wrote:

>> @@ -674,15 +675,17 @@ int vfe_reset(struct vfe_device *vfe)
>>   {
>>       unsigned long time;
>> -    reinit_completion(&vfe->reset_complete);
>> +    if (vfe->res->hw_ops->global_reset) {
>> +        reinit_completion(&vfe->reset_complete);
>> -    vfe->res->hw_ops->global_reset(vfe);
>> +        vfe->res->hw_ops->global_reset(vfe);
>> -    time = wait_for_completion_timeout(&vfe->reset_complete,
>> -        msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
>> -    if (!time) {
>> -        dev_err(vfe->camss->dev, "VFE reset timeout\n");
>> -        return -EIO;
>> +        time = wait_for_completion_timeout(&vfe->reset_complete,
>> +            msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
>> +        if (!time) {
>> +            dev_err(vfe->camss->dev, "VFE reset timeout\n");
>> +            return -EIO;
>> +        }
> 
> Per my comment on the CSID - this feels like a fix you are introducing 
> here in the guise of a silicon add.
> 
> Please break it up.
> 
> If you have a number of fixes to core functionality they need to be
> 
> 1. Granular and individual
> 2. Indivdually scrutable with their own patch and descritption
> 3. git cherry-pickable
> 4. Have a Fixes tag
> 5. And be cc'd to stable@vger.kernel.org
> 
> Can't accept either the fixes or the silicon add if the two live mixed 
> up in one patch.
> 

This isn't a bug fix, adding a null pointer checking just because vfe780 
doesn't have enable_irq/global_reset/isr/vfe_halt hw_ops, so adding the 
null checking for these hw_ops in this patch and adding them in one patch.
The original code doesn't have any bug.



>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/ 
>> media/platform/qcom/camss/camss-vfe.h
>> index fcbf4f609129..9dec5bc0d1b1 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>> @@ -243,6 +243,7 @@ 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_480;
>> +extern const struct vfe_hw_ops vfe_ops_780;
>>   int vfe_get(struct vfe_device *vfe);
>>   void vfe_put(struct vfe_device *vfe);
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/ 
>> media/platform/qcom/camss/camss.c
>> index 7ee102948dc4..92a0fa02e415 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -1666,6 +1666,125 @@ static const struct camss_subdev_resources 
>> csid_res_8550[] = {
>>       }
>>   };
>> +static const struct camss_subdev_resources vfe_res_8550[] = {
>> +    /* VFE0 */
>> +    {
>> +        .regulators = {},
>> +        .clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", 
>> "vfe0_fast_ahb",
>> +               "vfe0", "cpas_vfe0", "camnoc_axi" },
> 
> Should the camnoc AXI clock go here or in the CSID ?
> 

camnoc is responsible for ddr writing, so it is needed for the WM in vfe.


>> +    /* VFE4 lite */
>> +    {
>> +        .regulators = {},
>> +        .clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", 
>> "vfe_lite_ahb",
>> +               "vfe_lite", "cpas_ife_lite", "camnoc_axi" },
>> +        .clock_rate = {    { 0, 0, 0, 0, 0 },
>> +                { 0, 0, 0, 0, 80000000 },
>> +                { 300000000, 300000000, 400000000, 400000000, 
>> 400000000 },
>> +                { 300000000, 300000000, 400000000, 400000000, 
>> 400000000 },
> 
> I realise you're specifying all of the operating points here but the 
> clock only needs to appear once i.e.
> 
> 1 x 300 MHz
> 1 x 400 MHz
> 1 x 480 MHz
> 
> etc.
> 

Sure, will update in next series.

>> +                { 400000000, 480000000, 480000000, 480000000, 
>> 480000000 },
>> +                { 300000000, 300000000, 400000000, 400000000, 
>> 400000000 },
>> +                { 300000000, 300000000, 400000000, 400000000, 
>> 400000000 } },
>> +        .reg = { "vfe_lite1" },
>> +        .interrupt = { "vfe_lite1" },
>> +        .vfe = {
>> +            .line_num = 4,
>> +            .is_lite = true,
>> +            .hw_ops = &vfe_ops_780,
>> +            .formats_rdi = &vfe_formats_rdi_845,
>> +            .formats_pix = &vfe_formats_pix_845
>> +        }
>> +    },
>> +};

>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, 
>> bool is_clear)
>> +{
>> +    struct csid_device *csid;
>> +
>> +    if (hw_id < camss->res->csid_num) {
> 
> Does this cause do anything ? Is it just defensive programming ? Can the 
> hw_id index exceed the number of CSIDs defined and if so why ?
> 
> Smells wrong.
> 

It is just a defensive programming, just like some null pointer checking.


Thanks,
Depeng

Hi Bryan,

On 8/15/2024 8:25 AM, Bryan O'Donoghue wrote:
> On 12/08/2024 15:41, Depeng Shao wrote:
>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, 
>> bool is_clear)
>> +{
>> +    struct csid_device *csid;
>> +
>> +    if (hw_id < camss->res->csid_num) {
>> +        csid = &(camss->csid[hw_id]);
>> +
>> +        csid->res->hw_ops->reg_update(csid, port_id, is_clear);
>> +    }
>> +}
> 
> The naming here doesn't make the action clear
> 
> hw_ops->rup_update(csid, port, clear);
> 
> "is_clear" is not required since the type is a bool the "is" is implied 
> in the the logical state so just "clear" will do.
> 
> But re: my previous comment on having the ISR do the clear as is done in 
> the VFE 480, I don't think this is_clear parameter is warranted.
> 
> We want the calling function to request the rup_update() for the 
> rup_update() function to wait on completion and the ISR() to do the 
> clear once the RUP interrupt has been raised.
> 
> At least I think that's how it should work - could you please experiment 
> with your code for the flow - as it appears to match the VFE 480 logic.
> 

Thanks for catching this, I forget to add the rup irq, so this logic is 
also missed. I have tried it just now, the logic works good, will add it 
in next version patch.

Thanks,
Depeng

Hi Bryan,

>> +
>> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
>> +                    enum vfe_line_id line_id)
>> +{
>> +    int port_id = line_id;
>> +
>> +    /* RUP(register update) registers has beem moved to CSID in Titan 
>> 780.
>> +     * Notify the event of trigger RUP clear.
>> +     */
>> +    camss_reg_update(vfe->camss, vfe->id, port_id, true);
>> +}
> 
> Hmm, so another thought here.
> 
> camss_reg_update() is not an accurate name -> camss_rup_update() because 
> in this case we only update the RUP register, not the AUP or MUP.
> 
> reg is an abbreviation for register - but RUP has a defined meaning in 
> the camera namespace i.e. RUP = register update and its job is to latch 
> shadow registers to real registers.
> 
> camss_rup_update() please.
> 

Yes, you are right, the rup_update is reasonable, I will update it in 
next version patch.

Thanks,
Depeng

Hi Vladimir,

On 8/15/2024 7:20 AM, Vladimir Zapolskiy wrote:

>>
>>>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id,
>>>> bool is_clear)
>>>
>>>> +{
>>>> +    struct csid_device *csid;
>>>> +
>>>> +    if (hw_id < camss->res->csid_num) {
>>>> +        csid = &(camss->csid[hw_id]);
>>>> +
>>>> +        csid->res->hw_ops->reg_update(csid, port_id, is_clear);
>>>> +    }
>>>> +}
>>>> +
>>>
>>> Please add the new exported function camss_reg_update() in a separate
>>> preceding commit.

>>
>> Thanks for your comments, I will address them in new series.
>>
>> But I have some concern about above comment, you want to add a separate
>> commit for camss_reg_update, maybe camss_buf_done also need to do this,
>> but I guess I will get new comments from Krzysztof if I make a separate
>> change, Krzysztof posted few comments in v3 series, he asked, "must
>> organize your patches in logical junks" and the code must have a user.
>>
>> Please check below comments.
>>
>> https://lore.kernel.org/all/e1b298df-05da-4881- 
>> a628-149a8a625544@kernel.org/
>>
>> https://lore.kernel.org/all/d0f8b72d-4355-43cd-a5f9- 
>> c44aab8147e5@kernel.org/
> 
> Krzysztof is absolutely right in his two comments.
> 
>  From what I see there is a difference between his concerns and mine ones
> though, Krzysztof points to unused data, which should raise a build time
> warning, and I asked to make a separate commit for a non-static function,
> I believe it'll be removed by the linker silently...
> 
> The potential runtime logic change introduced by camss_reg_update() in the
> generic code is not trivial, which opens an option to update/fix it lately
> referencing a commit from generic domain rather than platform specific one.
> 
> If someone for whatever reasons wants to merge a new generic and shared
> camss_reg_update() function within a the platform specific code/commit,
> I won't strongly object, let it be merged together then.
> 
>>
>> Or I don't add reg update and buf done functionality in
>> camss-csid-gen3.c and camss-vfe-780.c firstly, then add them in a later
>> commit.
>>
>> Could you please comment on whether this is acceptable? Please also help
>> to common on if one commit to add them or need two separate commits, one
>> is for reg update and the other one is for buf done.
>>
> 
> I would prefer to see two more separate commits within non-platform 
> specific
> code, however as I stated above if it causes anyone's concerns, including
> your own, let it be kept as it is done today. Eventually we do discuss
> a non-functional change.
> 

Thanks for the confirmation, even though I add the rup_update and 
buf_done function in later commits, it is still called in platform 
specific code(camss-vfe-780.c), so I will keep as it is done today.

Thanks,
Depeng

Hi Depeng,

On 8/15/24 17:42, Depeng Shao wrote:
> Hi Vladimir,
> 
> On 8/15/2024 7:20 AM, Vladimir Zapolskiy wrote:
> 
>>>
>>>>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id,
>>>>> bool is_clear)
>>>>
>>>>> +{
>>>>> +    struct csid_device *csid;
>>>>> +
>>>>> +    if (hw_id < camss->res->csid_num) {
>>>>> +        csid = &(camss->csid[hw_id]);
>>>>> +
>>>>> +        csid->res->hw_ops->reg_update(csid, port_id, is_clear);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>
>>>> Please add the new exported function camss_reg_update() in a separate
>>>> preceding commit.
> 
>>>
>>> Thanks for your comments, I will address them in new series.
>>>
>>> But I have some concern about above comment, you want to add a separate
>>> commit for camss_reg_update, maybe camss_buf_done also need to do this,
>>> but I guess I will get new comments from Krzysztof if I make a separate
>>> change, Krzysztof posted few comments in v3 series, he asked, "must
>>> organize your patches in logical junks" and the code must have a user.
>>>
>>> Please check below comments.
>>>
>>> https://lore.kernel.org/all/e1b298df-05da-4881-
>>> a628-149a8a625544@kernel.org/
>>>
>>> https://lore.kernel.org/all/d0f8b72d-4355-43cd-a5f9-
>>> c44aab8147e5@kernel.org/
>>
>> Krzysztof is absolutely right in his two comments.
>>
>>   From what I see there is a difference between his concerns and mine ones
>> though, Krzysztof points to unused data, which should raise a build time
>> warning, and I asked to make a separate commit for a non-static function,
>> I believe it'll be removed by the linker silently...
>>
>> The potential runtime logic change introduced by camss_reg_update() in the
>> generic code is not trivial, which opens an option to update/fix it lately
>> referencing a commit from generic domain rather than platform specific one.
>>
>> If someone for whatever reasons wants to merge a new generic and shared
>> camss_reg_update() function within a the platform specific code/commit,
>> I won't strongly object, let it be merged together then.
>>
>>>
>>> Or I don't add reg update and buf done functionality in
>>> camss-csid-gen3.c and camss-vfe-780.c firstly, then add them in a later
>>> commit.
>>>
>>> Could you please comment on whether this is acceptable? Please also help
>>> to common on if one commit to add them or need two separate commits, one
>>> is for reg update and the other one is for buf done.
>>>
>>
>> I would prefer to see two more separate commits within non-platform
>> specific
>> code, however as I stated above if it causes anyone's concerns, including
>> your own, let it be kept as it is done today. Eventually we do discuss
>> a non-functional change.
>>
> 
> Thanks for the confirmation, even though I add the rup_update and
> buf_done function in later commits, it is still called in platform
> specific code(camss-vfe-780.c), so I will keep as it is done today.

let it be so.

I have another ask about it, please move new camss_reg_update() out from
camss.c into camss-csid.c, and camss_buf_done() from camss.c into camss-vfe.c

Thank you.

--
Best wishes,
Vladimir

Hi Vladimir,

>>
>> Thanks for the confirmation, even though I add the rup_update and
>> buf_done function in later commits, it is still called in platform
>> specific code(camss-vfe-780.c), so I will keep as it is done today.
> 
> let it be so.
> 
> I have another ask about it, please move new camss_reg_update() out from
> camss.c into camss-csid.c, and camss_buf_done() from camss.c into camss- 
> vfe.c
> 

The cross direct call has been removed by below commit, so it looks 
strange if I add the cross direct call.

media: qcom: camss: Decouple VFE from CSID
https://lore.kernel.org/lkml/20240522154659.510-9-quic_grosikop@quicinc.com/

I use the v4l2_subdev_notify to do the cross communication in v1 and v2 
series, but Bryan said, "The subdev notify is I think not the right fit 
for this purpose within our driver.".
Then I add an internal notify interface in camss structure, but Bryan 
suggested to use direct call, so I add these functions directly in camss.c

https://lore.kernel.org/all/236cfe43-8321-4168-8630-fb9528f581bd@linaro.org/

Thanks,
Depeng

On 15/08/2024 14:33, Depeng Shao wrote:
> Hi Bryan,
> 
> On 8/15/2024 12:23 AM, Bryan O'Donoghue wrote:
> 
>>> @@ -674,15 +675,17 @@ int vfe_reset(struct vfe_device *vfe)
>>>   {
>>>       unsigned long time;
>>> -    reinit_completion(&vfe->reset_complete);
>>> +    if (vfe->res->hw_ops->global_reset) {
>>> +        reinit_completion(&vfe->reset_complete);
>>> -    vfe->res->hw_ops->global_reset(vfe);
>>> +        vfe->res->hw_ops->global_reset(vfe);
>>> -    time = wait_for_completion_timeout(&vfe->reset_complete,
>>> -        msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
>>> -    if (!time) {
>>> -        dev_err(vfe->camss->dev, "VFE reset timeout\n");
>>> -        return -EIO;
>>> +        time = wait_for_completion_timeout(&vfe->reset_complete,
>>> +            msecs_to_jiffies(VFE_RESET_TIMEOUT_MS));
>>> +        if (!time) {
>>> +            dev_err(vfe->camss->dev, "VFE reset timeout\n");
>>> +            return -EIO;
>>> +        }
>>
>> Per my comment on the CSID - this feels like a fix you are introducing 
>> here in the guise of a silicon add.
>>
>> Please break it up.
>>
>> If you have a number of fixes to core functionality they need to be
>>
>> 1. Granular and individual
>> 2. Indivdually scrutable with their own patch and descritption
>> 3. git cherry-pickable
>> 4. Have a Fixes tag
>> 5. And be cc'd to stable@vger.kernel.org
>>
>> Can't accept either the fixes or the silicon add if the two live mixed 
>> up in one patch.
>>
> 
> This isn't a bug fix, adding a null pointer checking just because vfe780 
> doesn't have enable_irq/global_reset/isr/vfe_halt hw_ops, so adding the 
> null checking for these hw_ops in this patch and adding them in one patch.
> The original code doesn't have any bug.

Right but you could just have

static void vfe_global_reset(struct vfe_device *vfe)
{
         /* VFE780 has no global reset, simply report a completion */
         complete(&vfe->reset_complete);
}

const struct vfe_hw_ops vfe_ops_780 = {
	.global_reset = vfe_global_reset,
}

Instead of having a bunch of special cases in the top level code.

> 
>>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/ 
>>> media/platform/qcom/camss/camss-vfe.h
>>> index fcbf4f609129..9dec5bc0d1b1 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>>> @@ -243,6 +243,7 @@ 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_480;
>>> +extern const struct vfe_hw_ops vfe_ops_780;
>>>   int vfe_get(struct vfe_device *vfe);
>>>   void vfe_put(struct vfe_device *vfe);
>>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/ 
>>> media/platform/qcom/camss/camss.c
>>> index 7ee102948dc4..92a0fa02e415 100644
>>> --- a/drivers/media/platform/qcom/camss/camss.c
>>> +++ b/drivers/media/platform/qcom/camss/camss.c
>>> @@ -1666,6 +1666,125 @@ static const struct camss_subdev_resources 
>>> csid_res_8550[] = {
>>>       }
>>>   };
>>> +static const struct camss_subdev_resources vfe_res_8550[] = {
>>> +    /* VFE0 */
>>> +    {
>>> +        .regulators = {},
>>> +        .clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", 
>>> "vfe0_fast_ahb",
>>> +               "vfe0", "cpas_vfe0", "camnoc_axi" },
>>
>> Should the camnoc AXI clock go here or in the CSID ?
>>
> 
> camnoc is responsible for ddr writing, so it is needed for the WM in vfe.

Right, I don't recall if you specified the clock for CSID and VFE but 
just for the record it should appear in only the one block.. VFE sounds 
good to me.

> 
> 
>>> +    /* VFE4 lite */
>>> +    {
>>> +        .regulators = {},
>>> +        .clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb_clk", 
>>> "vfe_lite_ahb",
>>> +               "vfe_lite", "cpas_ife_lite", "camnoc_axi" },
>>> +        .clock_rate = {    { 0, 0, 0, 0, 0 },
>>> +                { 0, 0, 0, 0, 80000000 },
>>> +                { 300000000, 300000000, 400000000, 400000000, 
>>> 400000000 },
>>> +                { 300000000, 300000000, 400000000, 400000000, 
>>> 400000000 },
>>
>> I realise you're specifying all of the operating points here but the 
>> clock only needs to appear once i.e.
>>
>> 1 x 300 MHz
>> 1 x 400 MHz
>> 1 x 480 MHz
>>
>> etc.
>>
> 
> Sure, will update in next series.
> 
>>> +                { 400000000, 480000000, 480000000, 480000000, 
>>> 480000000 },
>>> +                { 300000000, 300000000, 400000000, 400000000, 
>>> 400000000 },
>>> +                { 300000000, 300000000, 400000000, 400000000, 
>>> 400000000 } },
>>> +        .reg = { "vfe_lite1" },
>>> +        .interrupt = { "vfe_lite1" },
>>> +        .vfe = {
>>> +            .line_num = 4,
>>> +            .is_lite = true,
>>> +            .hw_ops = &vfe_ops_780,
>>> +            .formats_rdi = &vfe_formats_rdi_845,
>>> +            .formats_pix = &vfe_formats_pix_845
>>> +        }
>>> +    },
>>> +};
> 
>>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, 
>>> bool is_clear)
>>> +{
>>> +    struct csid_device *csid;
>>> +
>>> +    if (hw_id < camss->res->csid_num) {
>>
>> Does this cause do anything ? Is it just defensive programming ? Can 
>> the hw_id index exceed the number of CSIDs defined and if so why ?
>>
>> Smells wrong.
>>
> 
> It is just a defensive programming, just like some null pointer checking.

Right so, please drop then. We require the indexes to be in range in 
order to merge, our job is to make sure this is so.

Lets just reason about the code and make sure the indexes are right.

---
bod

On 15/08/2024 15:21, Depeng Shao wrote:
> Thanks for catching this, I forget to add the rup irq, so this logic is 
> also missed. I have tried it just now, the logic works good, will add it 
> in next version patch.

Cool.

Thanks for sticking with this.

---
bod

Hi Depeng.

On 8/15/24 18:43, Depeng Shao wrote:
> Hi Vladimir,
> 
>>>
>>> Thanks for the confirmation, even though I add the rup_update and
>>> buf_done function in later commits, it is still called in platform
>>> specific code(camss-vfe-780.c), so I will keep as it is done today.
>>
>> let it be so.
>>
>> I have another ask about it, please move new camss_reg_update() out from
>> camss.c into camss-csid.c, and camss_buf_done() from camss.c into camss-
>> vfe.c
>>
> 
> The cross direct call has been removed by below commit, so it looks
> strange if I add the cross direct call.
> 
> media: qcom: camss: Decouple VFE from CSID
> https://lore.kernel.org/lkml/20240522154659.510-9-quic_grosikop@quicinc.com/

This I don't understand, please elaborate. I don't ask for a "cross direct
call", but you do introduce a CSID specific function in the generic camss.c
and another VFE specific function in the same camss.c

What I ask is just move the current versions of camss_buf_done() and
camss_reg_update() out from camss.c to the files, which are related to the
sub-IP blocks, and of course move the function declarations from camss.h
into camss-vfe.h and camss-csid.h respectively.

If possible there shall be no CSID or VFE specific specific code in camss.c,
and that fact is that it's possible.

> I use the v4l2_subdev_notify to do the cross communication in v1 and v2
> series, but Bryan said, "The subdev notify is I think not the right fit
> for this purpose within our driver.".

As far as I see all of that is irrelevant.

> Then I add an internal notify interface in camss structure, but Bryan
> suggested to use direct call, so I add these functions directly in camss.c
> 
> https://lore.kernel.org/all/236cfe43-8321-4168-8630-fb9528f581bd@linaro.org/
> 

--
Best wishes,
Vladimir

Hi Vladimir,

On 8/16/2024 5:31 AM, Vladimir Zapolskiy wrote:
> Hi Depeng.
> 
> On 8/15/24 18:43, Depeng Shao wrote:
>> Hi Vladimir,
>>
>>>>
>>>> Thanks for the confirmation, even though I add the rup_update and
>>>> buf_done function in later commits, it is still called in platform
>>>> specific code(camss-vfe-780.c), so I will keep as it is done today.
>>>
>>> let it be so.
>>>
>>> I have another ask about it, please move new camss_reg_update() out from
>>> camss.c into camss-csid.c, and camss_buf_done() from camss.c into camss-
>>> vfe.c
>>>
>>
>> The cross direct call has been removed by below commit, so it looks
>> strange if I add the cross direct call.
>>
>> media: qcom: camss: Decouple VFE from CSID
>> https://lore.kernel.org/lkml/20240522154659.510-9- 
>> quic_grosikop@quicinc.com/
> 
> This I don't understand, please elaborate. I don't ask for a "cross direct
> call", but you do introduce a CSID specific function in the generic camss.c
> and another VFE specific function in the same camss.c
> 

CSID calls vfe_get/vfe_put to power up/reset vfe hw in old code, but 
above decouple commit removes this cross direct call, this commit has 
been merged recently.

> What I ask is just move the current versions of camss_buf_done() and
> camss_reg_update() out from camss.c to the files, which are related to the
> sub-IP blocks, and of course move the function declarations from camss.h
> into camss-vfe.h and camss-csid.h respectively.
> 
> If possible there shall be no CSID or VFE specific specific code in 
> camss.c,
> and that fact is that it's possible.
> 

Yes, I understand what you mean. Let's take camss_buf_done as example, 
if we move camss_buf_done to camss-vfe.c, but this function is called in 
csid csid driver, so here will have a cross direct call again, 
camss_reg_update is same. Since the cross call is removed in above 
commit, then it will be strange if I do this again.

So, I moved them to camss.c

>> I use the v4l2_subdev_notify to do the cross communication in v1 and v2
>> series, but Bryan said, "The subdev notify is I think not the right fit
>> for this purpose within our driver.".
> 
> As far as I see all of that is irrelevant.
> 
>> Then I add an internal notify interface in camss structure, but Bryan
>> suggested to use direct call, so I add these functions directly in 
>> camss.c
>>
>> https://lore.kernel.org/all/236cfe43-8321-4168-8630- 
>> fb9528f581bd@linaro.org/
>>
> 

Thanks,
Depeng

On 12/08/2024 15:41, Depeng Shao wrote:
> +#define VFE_BUS_WM_CFG(n)		(BUS_REG_BASE + 0x200 + (n) * 0x100)

<snip>

> +#define RDI_WM(n)			((vfe_is_lite(vfe) ? 0x0 : 0x17) + (n))
> +
> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
> +{
> +	struct v4l2_pix_format_mplane *pix =
> +		&line->video_out.active_fmt.fmt.pix_mp;
> +
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */

OK so one more point here.

The non-lite VFE has I think in the case of sm8550 twenty seven 
different bus clients.

The above code takes a given index - take the example of index 0 meaning 
RDI0 and

1. Determines if is_lite() is true deriving a jump of 0 or 0x17
2. Uses this index as a further offset to functions such as
    VFE_BUS_WM_CFG(n)
3. In no way articulates which bus client is which.

So for a non lite case -> RDI0 is bus client # 23

The code we have for CAMSS just assumes RDI is the only client we are 
programming - which I'm not proposing to change for now, however the 
code is very not obvious in what it is doing here.

This BTW isn't a criticism of what you've done here but, even though I 
have access to the registers in front of me, I had to spend about 30 
minutes looking up and verifying these offsets.

That's not sustainable.

Could you please add a comment which details what each index relates to.

/*
  * Bus client mapping
  *
  * 0 = VID_Y ?
  * 1 = VID_C
  * .. etc
  * .. etc
  * 23 = RDI0
  * 24 = RDI1
  */

I'll try to apply a similar level of index documentation for existing 
upstream submissions so that working out client mappings is less tedious 
and will be requiring these mappings for new VFE silicon enabling code 
upstream.

---
bod

Hi Bryan,


On 8/19/2024 7:05 PM, Bryan O'Donoghue wrote:
> On 12/08/2024 15:41, Depeng Shao wrote:
>> +#define VFE_BUS_WM_CFG(n)        (BUS_REG_BASE + 0x200 + (n) * 0x100)
> 
> <snip>
> 
>> +#define RDI_WM(n)            ((vfe_is_lite(vfe) ? 0x0 : 0x17) + (n))
>> +
>> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct 
>> vfe_line *line)
>> +{
>> +    struct v4l2_pix_format_mplane *pix =
>> +        &line->video_out.active_fmt.fmt.pix_mp;
>> +
>> +    wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> 
> OK so one more point here.
> 
> The non-lite VFE has I think in the case of sm8550 twenty seven 
> different bus clients.
> 
> The above code takes a given index - take the example of index 0 meaning 
> RDI0 and
> 
> 1. Determines if is_lite() is true deriving a jump of 0 or 0x17
> 2. Uses this index as a further offset to functions such as
>     VFE_BUS_WM_CFG(n)
> 3. In no way articulates which bus client is which.
> 
> So for a non lite case -> RDI0 is bus client # 23
> 
> The code we have for CAMSS just assumes RDI is the only client we are 
> programming - which I'm not proposing to change for now, however the 
> code is very not obvious in what it is doing here.
> 
> This BTW isn't a criticism of what you've done here but, even though I 
> have access to the registers in front of me, I had to spend about 30 
> minutes looking up and verifying these offsets.
> 
> That's not sustainable.
> 
> Could you please add a comment which details what each index relates to.
> 
> /*
>   * Bus client mapping
>   *
>   * 0 = VID_Y ?
>   * 1 = VID_C
>   * .. etc
>   * .. etc
>   * 23 = RDI0
>   * 24 = RDI1
>   */
> 
> I'll try to apply a similar level of index documentation for existing 
> upstream submissions so that working out client mappings is less tedious 
> and will be requiring these mappings for new VFE silicon enabling code 
> upstream.
> 

Sure, I will add the comment for the bus client mapping in next version 
patch.

But the comment will occupy too many lines, I will fold the comment, e.g.,

/*
  * Bus client mapping
  *
  * Full VFE:
  * 0 = VID_Y, 1 = VID_C, 2 = VID 4:1, 3 = VID 16:1, 4 = DISP Y, 5 = 
DISP C, 6 = DISP 4:1,
  * 7 = DISP 16:1, 8 = FD_Y, 9 = FD_C, ...
  * ...
  * 23 = RDI0, 24 = RDI1, 25 = RDI2, 26 = LTM STATS
  *
  * VFE LITE:
  * 0 = RDI0, 1 = RDI1, 2 = RDI3, 4 = RDI4
  */

Since the full VFE has many ports, can we just add comment for the RDI 
client?


Thanks,
Depeng

Hi Depeng.

On 8/16/24 15:42, Depeng Shao wrote:
> Hi Vladimir,
> 
> On 8/16/2024 5:31 AM, Vladimir Zapolskiy wrote:
>> Hi Depeng.
>>
>> On 8/15/24 18:43, Depeng Shao wrote:
>>> Hi Vladimir,
>>>
>>>>>
>>>>> Thanks for the confirmation, even though I add the rup_update and
>>>>> buf_done function in later commits, it is still called in platform
>>>>> specific code(camss-vfe-780.c), so I will keep as it is done today.
>>>>
>>>> let it be so.
>>>>
>>>> I have another ask about it, please move new camss_reg_update() out from
>>>> camss.c into camss-csid.c, and camss_buf_done() from camss.c into camss-
>>>> vfe.c
>>>>
>>>
>>> The cross direct call has been removed by below commit, so it looks
>>> strange if I add the cross direct call.
>>>
>>> media: qcom: camss: Decouple VFE from CSID
>>> https://lore.kernel.org/lkml/20240522154659.510-9-
>>> quic_grosikop@quicinc.com/
>>
>> This I don't understand, please elaborate. I don't ask for a "cross direct
>> call", but you do introduce a CSID specific function in the generic camss.c
>> and another VFE specific function in the same camss.c
>>
> 
> CSID calls vfe_get/vfe_put to power up/reset vfe hw in old code, but
> above decouple commit removes this cross direct call, this commit has
> been merged recently.

Apparently I was imprecise, this is not the thing, which I don't understand,
for me it was the wording of "cross direct call".

>> What I ask is just move the current versions of camss_buf_done() and
>> camss_reg_update() out from camss.c to the files, which are related to the
>> sub-IP blocks, and of course move the function declarations from camss.h
>> into camss-vfe.h and camss-csid.h respectively.
>>
>> If possible there shall be no CSID or VFE specific specific code in
>> camss.c,
>> and that fact is that it's possible.
>>
> 
> Yes, I understand what you mean. Let's take camss_buf_done as example,
> if we move camss_buf_done to camss-vfe.c, but this function is called in
> csid csid driver, so here will have a cross direct call again,


> camss_reg_update is same. Since the cross call is removed in above
> commit, then it will be strange if I do this again.

It might be strange, but what I ask is to make the code way less convoluted.

> So, I moved them to camss.c

I'm still missing the essence of having two layers of indirection instead
of just one. Previous code was a function call from csid to vfe, now it's
csid to camss to vfe, I don't understand why there is a need to introduce
just an additional layer, it greatly complicates the code, also it slightly
drops the performance.

Previously there was no 'struct vfe_device *' or 'struct csid_device *'
types in the generic camss.c, now these "new" types leaked from camss-csid.h
and camss-vfe.h into camss.c, and the reason why remains unknown.

Okay, please ignore this one review request, let it be kept as is for a while.

As a side note, generally there might be various reasons to revert the code
changes or to return to the previous logic.

Thank you.

--
Best wishes,
Vladimir

On 8/12/24 17:41, Depeng Shao wrote:
> Add support for VFE found on SM8550 (Titan 780). This implementation is
> based on the titan 480 implementation. It supports the normal and lite
> VFE.
> 
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---

<snip>

> +
> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
> +					enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;

Once I said that the comment with a typo can be removed from these two
functions, however the functions can be removed, since they are trivial,
use camss_reg_update(vfe->camss, vfe->id, port_id, ...) directly in the code.

> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP clear.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
> +}
> +


--
Best wishes,
Vladimir

On 8/21/24 14:11, Vladimir Zapolskiy wrote:
> On 8/12/24 17:41, Depeng Shao wrote:
>> Add support for VFE found on SM8550 (Titan 780). This implementation is
>> based on the titan 480 implementation. It supports the normal and lite
>> VFE.
>>
>> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>> ---
> 
> <snip>
> 
>> +
>> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
>> +{
>> +	int port_id = line_id;
>> +
>> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
>> +	 * Notify the event of trigger RUP.
>> +	 */
>> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);
>> +}
>> +
>> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
>> +					enum vfe_line_id line_id)
>> +{
>> +	int port_id = line_id;
> 
> Once I said that the comment with a typo can be removed from these two
> functions, however the functions can be removed, since they are trivial,
> use camss_reg_update(vfe->camss, vfe->id, port_id, ...) directly in the code.
> 

I see that these new vfe_reg_update() and vfe_reg_update_clear() are
callback functions now, so, without making a step back, now it will not
be straightforward to get rid of one more level of indirection.

Just remove the comments then.

>> +
>> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
>> +	 * Notify the event of trigger RUP clear.
>> +	 */
>> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
>> +}
>> +

--
Best wishes,
Vladimir

On 12/08/2024 15:41, Depeng Shao wrote:
> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
> +					enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* RUP(register update) registers has beem moved to CSID in Titan 780.
> +	 * Notify the event of trigger RUP clear.
> +	 */
> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
> +}

I think I tend to agree with Vlad here, that this is a needless layer of 
wrappering.

I'm not sure that's exactly what you guys where talking about but, I 
rebased my x1e80100 stuff on top of your stuff

https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-24-08-15-x1e80100-camss-debufsoff-cleanup?ref_type=heads

and anyway the above wrapper didn't make alot of sense to me.

---
bod