diff mbox series

[09/13] media: qcom: camss: Add CSID Gen3 support for SM8550

Message ID 20240709160656.31146-10-quic_depengs@quicinc.com (mailing list archive)
State New
Headers show
Series media: qcom: camss: Add sm8550 support | expand

Commit Message

Depeng Shao July 9, 2024, 4:06 p.m. UTC
The CSID in SM8550 is gen3, it has new register offset and new
functionality. The buf done irq,register update and reset are
moved to CSID gen3. And CSID gen3 has a new register block which
is named as CSID top, it controls the output of CSID, since the
CSID can connect to Sensor Front End (SFE) or original VFE, the
register in top block is used to control the HW connection.

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 +
 .../platform/qcom/camss/camss-csid-gen3.c     | 445 ++++++++++++++++++
 .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
 .../media/platform/qcom/camss/camss-csid.h    |   2 +
 4 files changed, 474 insertions(+)
 create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c
 create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h

Comments

Krzysztof Kozlowski July 10, 2024, 11:20 a.m. UTC | #1
On 09/07/2024 18:06, Depeng Shao wrote:
> The CSID in SM8550 is gen3, it has new register offset and new
> functionality. The buf done irq,register update and reset are
> moved to CSID gen3. And CSID gen3 has a new register block which
> is named as CSID top, it controls the output of CSID, since the
> CSID can connect to Sensor Front End (SFE) or original VFE, the
> register in top block is used to control the HW connection.
> 
> 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 +
>  .../platform/qcom/camss/camss-csid-gen3.c     | 445 ++++++++++++++++++
>  .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
>  .../media/platform/qcom/camss/camss-csid.h    |   2 +
>  4 files changed, 474 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c
>  create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h
> 
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index e636968a1126..c336e4c1a399 100644
> --- a/drivers/media/platform/qcom/camss/Makefile
> +++ b/drivers/media/platform/qcom/camss/Makefile
> @@ -7,6 +7,7 @@ qcom-camss-objs += \
>  		camss-csid-4-1.o \
>  		camss-csid-4-7.o \
>  		camss-csid-gen2.o \
> +		camss-csid-gen3.o \
>  		camss-csiphy-2ph-1-0.o \
>  		camss-csiphy-3ph-1-0.o \
>  		camss-csiphy.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> new file mode 100644
> index 000000000000..17fd7c5499de
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> @@ -0,0 +1,445 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-csid-gen3.c
> + *
> + * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module
> + *
> + * Copyright (c) 2024 Qualcomm Technologies, Inc.
> + */
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +
> +#include "camss.h"
> +#include "camss-csid.h"
> +#include "camss-csid-gen3.h"
> +
> +
> +#define CSID_TOP_IO_PATH_CFG0(csid)	(0x4 * (csid))
> +#define		OUTPUT_IFE_EN 0x100
> +#define		INTERNAL_CSID 1
> +
> +#define CSID_HW_VERSION		0x0
> +#define		HW_VERSION_STEPPING	0
> +#define		HW_VERSION_REVISION	16
> +#define		HW_VERSION_GENERATION	28
> +
> +#define CSID_RST_CFG	0xC
> +#define		RST_MODE		0
> +#define		RST_LOCATION	4
> +
> +#define CSID_RST_CMD	0x10
> +#define		SELECT_HW_RST	0
> +#define		SELECT_SW_RST	1
> +#define		SELECT_IRQ_RST	2
> +
> +#define CSID_CSI2_RX_IRQ_STATUS	0x9C
> +#define	CSID_CSI2_RX_IRQ_MASK	0xA0
> +#define CSID_CSI2_RX_IRQ_CLEAR	0xA4
> +#define CSID_CSI2_RX_IRQ_SET	0xA8
> +
> +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)		(0xEC + 0x10 * (rdi))
> +#define CSID_CSI2_RDIN_IRQ_MASK(rdi)		(0xF0 + 0x10 * (rdi))
> +#define   CSID_CSI2_RDIN_INFO_FIFO_FULL 2

That's a random set of indentations.

> +#define   CSID_CSI2_RDIN_INFO_CAMIF_EOF 3
> +#define   CSID_CSI2_RDIN_INFO_CAMIF_SOF 4
> +#define   CSID_CSI2_RDIN_INFO_INPUT_EOF 9
> +#define   CSID_CSI2_RDIN_INFO_INPUT_SOF 12


...

> +
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
> +
> +	val = 1 << RDI_CFG1_PACKING_FORMAT;
> +	val |= 1 << RDI_CFG1_PIX_STORE;
> +	val |= 1 << RDI_CFG1_DROP_H_EN;
> +	val |= 1 << RDI_CFG1_DROP_V_EN;
> +	val |= 1 << RDI_CFG1_CROP_H_EN;
> +	val |= 1 << RDI_CFG1_CROP_V_EN;
> +	val |= RDI_CFG1_EARLY_EOF_EN;
> +
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
> +
> +	val = 0;
> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
> +
> +	val = 1;
> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
> +
> +	val = 0;
> +	writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
> +
> +	val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
> +	val |=  enable << RDI_CFG0_EN;
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
> +}
> +

such patterns and...

> + */
> +static int csid_reset(struct csid_device *csid)
> +{
> +	unsigned long time;
> +	u32 val;
> +	int i;
> +
> +	reinit_completion(&csid->reset_complete);
> +
> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
> +	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
> +		if (csid->phy.en_vc & BIT(i)) {
> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> +						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> +			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> +						csid->base + CSID_BUF_DONE_IRQ_MASK);
> +		}
> +
> +	/* preserve registers */
> +	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
> +	writel_relaxed(val, csid->base + CSID_RST_CFG);

... here - using everywhere relaxed here is odd and looks racy. These
looks like some strict sequences.


> +
> +	val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
> +	writel_relaxed(val, csid->base + CSID_RST_CMD);
> +
> +	time = wait_for_completion_timeout(&csid->reset_complete,
> +					   msecs_to_jiffies(CSID_RESET_TIMEOUT_MS));
> +	if (!time) {
> +		dev_err(csid->camss->dev, "CSID reset timeout\n");
> +		return -EIO;
> +	}
> +


> +
> +static void csid_subdev_init(struct csid_device *csid)
> +{
> +	csid->testgen.modes = csid_testgen_modes;
> +	csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
> +}
> +
> +const struct csid_hw_ops csid_ops_gen3 = {

Isn't there a warning here?

> +	.configure_stream = csid_configure_stream,
> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
> +	.hw_version = csid_hw_version,
> +	.isr = csid_isr,
> +	.reset = csid_reset,
> +	.src_pad_code = csid_src_pad_code,
> +	.subdev_init = csid_subdev_init,
> +};

Your patchset does not apply at all. Tried v6.9, v6.10, next. I see some
dependency above, but that means no one can test it and no one can apply it.

Fix the warnings, I cannot verify it but I am sure you have them.

Best regards,
Krzysztof
Bryan O'Donoghue July 10, 2024, 11:28 a.m. UTC | #2
On 09/07/2024 17:06, Depeng Shao wrote:
> The CSID in SM8550 is gen3, it has new register offset and new
> functionality. The buf done irq,register update and reset are
> moved to CSID gen3. And CSID gen3 has a new register block which
> is named as CSID top, it controls the output of CSID, since the
> CSID can connect to Sensor Front End (SFE) or original VFE, the
> register in top block is used to control the HW connection.
> 
> 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 +
>   .../platform/qcom/camss/camss-csid-gen3.c     | 445 ++++++++++++++++++
>   .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
>   .../media/platform/qcom/camss/camss-csid.h    |   2 +
>   4 files changed, 474 insertions(+)
>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c
>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h
> 
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index e636968a1126..c336e4c1a399 100644
> --- a/drivers/media/platform/qcom/camss/Makefile
> +++ b/drivers/media/platform/qcom/camss/Makefile
> @@ -7,6 +7,7 @@ qcom-camss-objs += \
>   		camss-csid-4-1.o \
>   		camss-csid-4-7.o \
>   		camss-csid-gen2.o \
> +		camss-csid-gen3.o \
>   		camss-csiphy-2ph-1-0.o \
>   		camss-csiphy-3ph-1-0.o \
>   		camss-csiphy.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> new file mode 100644
> index 000000000000..17fd7c5499de
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> @@ -0,0 +1,445 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-csid-gen3.c
> + *
> + * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module
> + *
> + * Copyright (c) 2024 Qualcomm Technologies, Inc.
> + */
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +
> +#include "camss.h"
> +#include "camss-csid.h"
> +#include "camss-csid-gen3.h"
> +
> +
> +#define CSID_TOP_IO_PATH_CFG0(csid)	(0x4 * (csid))
> +#define		OUTPUT_IFE_EN 0x100
> +#define		INTERNAL_CSID 1
> +
> +#define CSID_HW_VERSION		0x0
> +#define		HW_VERSION_STEPPING	0
> +#define		HW_VERSION_REVISION	16
> +#define		HW_VERSION_GENERATION	28
> +
> +#define CSID_RST_CFG	0xC
> +#define		RST_MODE		0
> +#define		RST_LOCATION	4
> +
> +#define CSID_RST_CMD	0x10
> +#define		SELECT_HW_RST	0
> +#define		SELECT_SW_RST	1
> +#define		SELECT_IRQ_RST	2
> +
> +#define CSID_CSI2_RX_IRQ_STATUS	0x9C
> +#define	CSID_CSI2_RX_IRQ_MASK	0xA0
> +#define CSID_CSI2_RX_IRQ_CLEAR	0xA4
> +#define CSID_CSI2_RX_IRQ_SET	0xA8
> +
> +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)		(0xEC + 0x10 * (rdi))
> +#define CSID_CSI2_RDIN_IRQ_MASK(rdi)		(0xF0 + 0x10 * (rdi))
> +#define   CSID_CSI2_RDIN_INFO_FIFO_FULL 2
> +#define   CSID_CSI2_RDIN_INFO_CAMIF_EOF 3
> +#define   CSID_CSI2_RDIN_INFO_CAMIF_SOF 4
> +#define   CSID_CSI2_RDIN_INFO_INPUT_EOF 9
> +#define   CSID_CSI2_RDIN_INFO_INPUT_SOF 12
> +#define   CSID_CSI2_RDIN_ERROR_REC_FRAME_DROP 18
> +#define   CSID_CSI2_RDIN_ERROR_REC_OVERFLOW_IRQ 19
> +#define   CSID_CSI2_RDIN_ERROR_REC_CCIF_VIOLATION 20
> +#define   CSID_CSI2_RDIN_EPOCH0 21
> +#define   CSID_CSI2_RDIN_EPOCH1 22
> +#define   CSID_CSI2_RDIN_RUP_DONE 23
> +#define   CSID_CSI2_RDIN_CCIF_VIOLATION 29
> +
> +#define CSID_CSI2_RDIN_IRQ_CLEAR(rdi)		(0xF4 + 0x10 * (rdi))
> +#define CSID_CSI2_RDIN_IRQ_SET(rdi)			(0xF8 + 0x10 * (rdi))
> +
> +#define CSID_TOP_IRQ_STATUS	0x7C
> +#define		TOP_IRQ_STATUS_RESET_DONE 0
> +#define CSID_TOP_IRQ_MASK	0x80
> +#define CSID_TOP_IRQ_CLEAR	0x84
> +#define CSID_TOP_IRQ_SET	0x88
> +#define CSID_IRQ_CMD		0x14
> +#define		IRQ_CMD_CLEAR	0
> +#define		IRQ_CMD_SET	4
> +
> +#define CSID_BUF_DONE_IRQ_STATUS	0x8C
> +#define		BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 0x1 : 0xE)
> +#define CSID_BUF_DONE_IRQ_MASK	0x90
> +#define CSID_BUF_DONE_IRQ_CLEAR	0x94
> +#define CSID_BUF_DONE_IRQ_SET	0x98
> +
> +#define CSID_CSI2_RX_CFG0	0x200
> +#define		CSI2_RX_CFG0_NUM_ACTIVE_LANES	0
> +#define		CSI2_RX_CFG0_DL0_INPUT_SEL	4
> +#define		CSI2_RX_CFG0_DL1_INPUT_SEL	8
> +#define		CSI2_RX_CFG0_DL2_INPUT_SEL	12
> +#define		CSI2_RX_CFG0_DL3_INPUT_SEL	16
> +#define		CSI2_RX_CFG0_PHY_NUM_SEL	20
> +#define			CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
> +#define		CSI2_RX_CFG0_PHY_TYPE_SEL	24
> +#define		CSI2_RX_CFG0_TPG_MUX_EN		27
> +#define		CSI2_RX_CFG0_TPG_NUM_SEL	28
> +
> +#define CSID_CSI2_RX_CFG1	0x204
> +#define		CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN		0
> +#define		CSI2_RX_CFG1_DE_SCRAMBLE_EN			1
> +#define		CSI2_RX_CFG1_VC_MODE				2
> +#define		CSI2_RX_CFG1_COMPLETE_STREAM_EN			4
> +#define		CSI2_RX_CFG1_COMPLETE_STREAM_FRAME_TIMING	5
> +#define		CSI2_RX_CFG1_MISR_EN				6
> +#define		CSI2_RX_CFG1_PHY_BIST_EN			7
> +#define		CSI2_RX_CFG1_EPD_MODE				8
> +#define		CSI2_RX_CFG1_EOTP_EN				9
> +#define		CSI2_RX_CFG1_DYN_SWITCH_EN			10
> +#define		CSI2_RX_CFG1_RUP_AUP_LATCH_DIS		11
> +
> +#define CSID_CSI2_RX_CAPTURE_CTRL	0x208
> +#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_EN	0
> +#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_EN	1
> +#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_EN	3
> +#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_DT		4
> +#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_VC		10
> +#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_VC	15
> +#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_DT		20
> +#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_VC		26
> +
> +#define CSID_RDI_CFG0(rdi)			(0x500 + 0x100 * (rdi))
> +#define		RDI_CFG0_VFR_EN				0
> +#define		RDI_CFG0_FRAME_ID_DEC_EN	1
> +#define		RDI_CFG0_RETIME_DIS			5
> +#define		RDI_CFG0_TIMESTAMP_EN		6
> +#define		RDI_CFG0_TIMESTAMP_STB_SEL	8
> +#define		RDI_CFG0_DECODE_FORMAT		12
> +#define		RDI_CFG0_DT					16
> +#define		RDI_CFG0_VC					22
> +#define		RDI_CFG0_DT_ID				27
> +#define		RDI_CFG0_EN					31
> +
> +#define CSID_RDI_CFG1(rdi)			(0x510 + 0x100 * (rdi))
> +#define		RDI_CFG1_BYTE_CNTR_EN	2
> +#define		RDI_CFG1_DROP_H_EN	5
> +#define		RDI_CFG1_DROP_V_EN	6
> +#define		RDI_CFG1_CROP_H_EN	7
> +#define		RDI_CFG1_CROP_V_EN	8
> +#define		RDI_CFG1_MISR_EN	9
> +#define		RDI_CFG1_PIX_STORE  10
> +#define		RDI_CFG1_PLAIN_ALIGNMENT	11
> +#define		RDI_CFG1_PLAIN_FORMAT	12
> +#define		RDI_CFG1_EARLY_EOF_EN	14
> +#define		RDI_CFG1_PACKING_FORMAT	15
> +
> +#define CSID_RDI_CTRL(rdi)			(0x504 + 0x100 * (rdi))
> +#define		RDI_CTRL_START_CMD		0
> +#define		RDI_CTRL_START_MODE		2
> +
> +#define CSID_RDI_EPOCH_IRQ_CFG(rdi) (0x52C + 0x100 * (rdi))
> +
> +#define CSID_RDI_FRM_DROP_PATTERN(rdi)			(0x540 + 0x100 * (rdi))
> +#define CSID_RDI_FRM_DROP_PERIOD(rdi)			(0x544 + 0x100 * (rdi))
> +#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi)		(0x548 + 0x100 * (rdi))
> +#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi)		(0x54C + 0x100 * (rdi))
> +#define CSID_RDI_RPP_PIX_DROP_PATTERN(rdi)		(0x558 + 0x100 * (rdi))
> +#define CSID_RDI_RPP_PIX_DROP_PERIOD(rdi)		(0x55C + 0x100 * (rdi))
> +#define CSID_RDI_RPP_LINE_DROP_PATTERN(rdi)		(0x560 + 0x100 * (rdi))
> +#define CSID_RDI_RPP_LINE_DROP_PERIOD(rdi)		(0x564 + 0x100 * (rdi))
> +
> +#define CSID_RDI_RPP_HCROP(rdi)                 (0x550 + 0x100 * (rdi))
> +#define CSID_RDI_RPP_VCROP(rdi)                 (0x554 + 0x100 * (rdi))
> +
> +#define CSID_RDI_ERROR_RECOVERY_CFG0(rdi)       (0x514 + 0x100 * (rdi))
> +
> +#define CSID_REG_UPDATE_CMD		0x18
> +static inline int reg_update_rdi(struct csid_device *csid, int n)
> +{
> +	return BIT(n + 4) + BIT(20 + n);
> +}
> +
> +#define	    REG_UPDATE_RDI		reg_update_rdi
> +
> +static void __csid_configure_rx(struct csid_device *csid,
> +				struct csid_phy_config *phy, int vc)
> +{
> +	int val;
> +
> +	val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
> +	val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
> +	val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX) << CSI2_RX_CFG0_PHY_NUM_SEL;
> +
> +	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
> +
> +	val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
> +	if (vc > 3)
> +		val |= 1 << CSI2_RX_CFG1_VC_MODE;

I realise downstream does these shifts but, I think in upstream we 
should just define a BIT(x)

#define CSI2_RX_CFG1_VC_MODE BIT(2)

val |= CSI2_RX_CFG1_VC_MODE;

> +	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
> +}
> +
> +static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8 rdi)
> +{
> +	int val;
> +
> +	if (enable)
> +		val = 1 << RDI_CTRL_START_CMD;
> +	else
> +		val = 0 << RDI_CTRL_START_CMD;

Here is an example of how the bit shifting is weird

(0 << anything) is still zero

> +	writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
> +}
> +
> +static void __csid_configure_top(struct csid_device *csid)
> +{
> +	u32 val;
> +
> +	/* CSID "top" is a new function in Titan780.
> +	 * CSID can connect to VFE & SFE(Sensor Front End).
> +	 * This connection is ontrolled by CSID "top".
> +	 * Only enable VFE path in current driver.
> +	 */
> +	if (csid->top_base) {

When is csid->top_base NULL ?

Its required in your yaml.

> +		val = OUTPUT_IFE_EN | INTERNAL_CSID;
> +		writel_relaxed(val, csid->top_base + CSID_TOP_IO_PATH_CFG0(csid->id));
> +	}
> +}
> +
> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 vc)
> +{
> +	u32 val;
> +	u8 lane_cnt = csid->phy.lane_cnt;
> +	/* Source pads matching RDI channels on hardware. Pad 1 -> RDI0, Pad 2 -> RDI1, etc. */
> +	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + vc];
> +	const struct csid_format_info *format = csid_get_fmt_entry(csid->res->formats->formats,
> +								   csid->res->formats->nformats,
> +								   input_format->code);
> +
> +	if (!lane_cnt)
> +		lane_cnt = 4;
> +
> +	/*
> +	 * DT_ID is a two bit bitfield that is concatenated with
> +	 * the four least significant bits of the five bit VC
> +	 * bitfield to generate an internal CID value.
> +	 *
> +	 * CSID_RDI_CFG0(vc)
> +	 * DT_ID : 28:27
> +	 * VC    : 26:22
> +	 * DT    : 21:16
> +	 *
> +	 * CID   : VC 3:0 << 2 | DT_ID 1:0
> +	 */
> +	u8 dt_id = vc & 0x03;
> +
> +	val = 1 << RDI_CFG0_TIMESTAMP_EN;
> +	val |= 2 << RDI_CFG0_TIMESTAMP_STB_SEL;
> +	/* note: for non-RDI path, this should be format->decode_format */
> +	val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
> +	val |= vc << RDI_CFG0_VC;
> +	val |= format->data_type << RDI_CFG0_DT;
> +	val |= dt_id << RDI_CFG0_DT_ID;
> +
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
> +
> +	val = 1 << RDI_CFG1_PACKING_FORMAT;
> +	val |= 1 << RDI_CFG1_PIX_STORE;
> +	val |= 1 << RDI_CFG1_DROP_H_EN;
> +	val |= 1 << RDI_CFG1_DROP_V_EN;
> +	val |= 1 << RDI_CFG1_CROP_H_EN;
> +	val |= 1 << RDI_CFG1_CROP_V_EN;
> +	val |= RDI_CFG1_EARLY_EOF_EN;
> +
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
> +
> +	val = 0;
> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
> +
> +	val = 1;
> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
> +
> +	val = 0;
> +	writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
> +
> +	val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
> +	val |=  enable << RDI_CFG0_EN;
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
> +}
> +
> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
> +{
> +	u8 i;
> +
> +	/* Loop through all enabled VCs and configure stream for each */
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
> +		if (csid->phy.en_vc & BIT(i)) {
> +			/* Configure CSID "top" */
> +			__csid_configure_top(csid);
> +			__csid_configure_rdi_stream(csid, enable, i);
> +			__csid_configure_rx(csid, &csid->phy, i);
> +			__csid_ctrl_rdi(csid, enable, i);
> +		}
> +}

I really like this breakdown
> +
> +static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
> +{
> +	if (val > 0 && val <= csid->testgen.nmodes)
> +		csid->testgen.mode = val;

Surely you should just set the val parameter directly ?

Also why is this a signed integer and how does it get assigned a 
negative value which we need to mitigate against here ?

> +
> +	return 0;
> +}
> +
> +/*
> + * csid_hw_version - CSID hardware version query
> + * @csid: CSID device
> + *
> + * Return HW version or error
> + */
> +static u32 csid_hw_version(struct csid_device *csid)
> +{
> +	u32 hw_version;
> +	u32 hw_gen;
> +	u32 hw_rev;
> +	u32 hw_step;
> +
> +	hw_version = readl_relaxed(csid->base + CSID_HW_VERSION);
> +	hw_gen = (hw_version >> HW_VERSION_GENERATION) & 0xF;
> +	hw_rev = (hw_version >> HW_VERSION_REVISION) & 0xFFF;
> +	hw_step = (hw_version >> HW_VERSION_STEPPING) & 0xFFFF;
> +	dev_info(csid->camss->dev, "CSID HW Version = %u.%u.%u\n",
> +		hw_gen, hw_rev, hw_step);
> +
> +	return hw_version;
> +}
> +
> +/*
> + * csid_isr - CSID module interrupt service routine
> + * @irq: Interrupt line
> + * @dev: CSID device
> + *
> + * Return IRQ_HANDLED on success
> + */
> +static irqreturn_t csid_isr(int irq, void *dev)
> +{
> +	struct csid_device *csid = dev;
> +	u32 val, buf_done_val;
> +	u8 reset_done;
> +	int i;
> +
> +	val = readl_relaxed(csid->base + CSID_TOP_IRQ_STATUS);
> +	writel_relaxed(val, csid->base + CSID_TOP_IRQ_CLEAR);
> +	reset_done = val & BIT(TOP_IRQ_STATUS_RESET_DONE);
> +
> +	val = readl_relaxed(csid->base + CSID_CSI2_RX_IRQ_STATUS);
> +	writel_relaxed(val, csid->base + CSID_CSI2_RX_IRQ_CLEAR);
> +
> +	buf_done_val = readl_relaxed(csid->base + CSID_BUF_DONE_IRQ_STATUS);
> +	writel_relaxed(buf_done_val, csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> +
> +	/* Read and clear IRQ status for each enabled RDI channel */
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
> +		if (csid->phy.en_vc & BIT(i)) {
> +			val = readl_relaxed(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
> +			writel_relaxed(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
> +		}
> +
> +	val = 1 << IRQ_CMD_CLEAR;
> +	writel_relaxed(val, csid->base + CSID_IRQ_CMD);
> +
> +	if (reset_done)
> +		complete(&csid->reset_complete);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * csid_reset - Trigger reset on CSID module and wait to complete
> + * @csid: CSID device
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +static int csid_reset(struct csid_device *csid)
> +{
> +	unsigned long time;
> +	u32 val;
> +	int i;
> +
> +	reinit_completion(&csid->reset_complete);
> +
> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
> +	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
> +		if (csid->phy.en_vc & BIT(i)) {
> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> +						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> +			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> +						csid->base + CSID_BUF_DONE_IRQ_MASK);
> +		}
> +
> +	/* preserve registers */
> +	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
> +	writel_relaxed(val, csid->base + CSID_RST_CFG);
> +
> +	val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
> +	writel_relaxed(val, csid->base + CSID_RST_CMD);
> +
> +	time = wait_for_completion_timeout(&csid->reset_complete,
> +					   msecs_to_jiffies(CSID_RESET_TIMEOUT_MS));
> +	if (!time) {
> +		dev_err(csid->camss->dev, "CSID reset timeout\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 csid_src_pad_code(struct csid_device *csid, u32 sink_code,
> +			     unsigned int match_format_idx, u32 match_code)
> +{
> +	switch (sink_code) {
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	{
> +		u32 src_code[] = {
> +			MEDIA_BUS_FMT_SBGGR10_1X10,
> +			MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE,
> +		};
> +
> +		return csid_find_code(src_code, ARRAY_SIZE(src_code),
> +				      match_format_idx, match_code);
> +	}
> +	case MEDIA_BUS_FMT_Y10_1X10:
> +	{
> +		u32 src_code[] = {
> +			MEDIA_BUS_FMT_Y10_1X10,
> +			MEDIA_BUS_FMT_Y10_2X8_PADHI_LE,
> +		};
> +
> +		return csid_find_code(src_code, ARRAY_SIZE(src_code),
> +				      match_format_idx, match_code);
> +	}
> +	default:
> +		if (match_format_idx > 0)
> +			return 0;
> +
> +		return sink_code;
> +	}
> +}

Same code as in gen2.

You could move the gen2 version of this into camss-csid.c and just reuse 
in both.

---
bod
Depeng Shao July 11, 2024, 11:08 a.m. UTC | #3
Hi Krzysztof,

On 7/10/2024 7:20 PM, Krzysztof Kozlowski wrote:
> On 09/07/2024 18:06, Depeng Shao wrote:
>> The CSID in SM8550 is gen3, it has new register offset and new
>> functionality. The buf done irq,register update and reset are
>> moved to CSID gen3. And CSID gen3 has a new register block which
>> is named as CSID top, it controls the output of CSID, since the
>> CSID can connect to Sensor Front End (SFE) or original VFE, the
>> register in top block is used to control the HW connection.
>>
>> 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 +
>>   .../platform/qcom/camss/camss-csid-gen3.c     | 445 ++++++++++++++++++
>>   .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
>>   .../media/platform/qcom/camss/camss-csid.h    |   2 +
>>   4 files changed, 474 insertions(+)
>>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c
>>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h
>>
>> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
>> index e636968a1126..c336e4c1a399 100644
>> --- a/drivers/media/platform/qcom/camss/Makefile
>> +++ b/drivers/media/platform/qcom/camss/Makefile
>> @@ -7,6 +7,7 @@ qcom-camss-objs += \
>>   		camss-csid-4-1.o \
>>   		camss-csid-4-7.o \
>>   		camss-csid-gen2.o \
>> +		camss-csid-gen3.o \
>>   		camss-csiphy-2ph-1-0.o \
>>   		camss-csiphy-3ph-1-0.o \
>>   		camss-csiphy.o \
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>> new file mode 100644
>> index 000000000000..17fd7c5499de
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>> @@ -0,0 +1,445 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * camss-csid-gen3.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module
>> + *
>> + * Copyright (c) 2024 Qualcomm Technologies, Inc.
>> + */
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of.h>
>> +
>> +#include "camss.h"
>> +#include "camss-csid.h"
>> +#include "camss-csid-gen3.h"
>> +
>> +
>> +#define CSID_TOP_IO_PATH_CFG0(csid)	(0x4 * (csid))
>> +#define		OUTPUT_IFE_EN 0x100
>> +#define		INTERNAL_CSID 1
>> +
>> +#define CSID_HW_VERSION		0x0
>> +#define		HW_VERSION_STEPPING	0
>> +#define		HW_VERSION_REVISION	16
>> +#define		HW_VERSION_GENERATION	28
>> +
>> +#define CSID_RST_CFG	0xC
>> +#define		RST_MODE		0
>> +#define		RST_LOCATION	4
>> +
>> +#define CSID_RST_CMD	0x10
>> +#define		SELECT_HW_RST	0
>> +#define		SELECT_SW_RST	1
>> +#define		SELECT_IRQ_RST	2
>> +
>> +#define CSID_CSI2_RX_IRQ_STATUS	0x9C
>> +#define	CSID_CSI2_RX_IRQ_MASK	0xA0
>> +#define CSID_CSI2_RX_IRQ_CLEAR	0xA4
>> +#define CSID_CSI2_RX_IRQ_SET	0xA8
>> +
>> +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)		(0xEC + 0x10 * (rdi))
>> +#define CSID_CSI2_RDIN_IRQ_MASK(rdi)		(0xF0 + 0x10 * (rdi))
>> +#define   CSID_CSI2_RDIN_INFO_FIFO_FULL 2
> 
> That's a random set of indentations.
> 

Thanks, will fixes this.

>> +#define   CSID_CSI2_RDIN_INFO_CAMIF_EOF 3
>> +#define   CSID_CSI2_RDIN_INFO_CAMIF_SOF 4
>> +#define   CSID_CSI2_RDIN_INFO_INPUT_EOF 9
>> +#define   CSID_CSI2_RDIN_INFO_INPUT_SOF 12
> 
> 
> ...
> 
>> +
>> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
>> +
>> +	val = 1 << RDI_CFG1_PACKING_FORMAT;
>> +	val |= 1 << RDI_CFG1_PIX_STORE;
>> +	val |= 1 << RDI_CFG1_DROP_H_EN;
>> +	val |= 1 << RDI_CFG1_DROP_V_EN;
>> +	val |= 1 << RDI_CFG1_CROP_H_EN;
>> +	val |= 1 << RDI_CFG1_CROP_V_EN;
>> +	val |= RDI_CFG1_EARLY_EOF_EN;
>> +
>> +	writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
>> +
>> +	val = 0;
>> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
>> +
>> +	val = 1;
>> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
>> +
>> +	val = 0;
>> +	writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
>> +
>> +	val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
>> +	val |=  enable << RDI_CFG0_EN;
>> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
>> +}
>> +
> 
> such patterns and...
> 
>> + */
>> +static int csid_reset(struct csid_device *csid)
>> +{
>> +	unsigned long time;
>> +	u32 val;
>> +	int i;
>> +
>> +	reinit_completion(&csid->reset_complete);
>> +
>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
>> +	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
>> +
>> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>> +		if (csid->phy.en_vc & BIT(i)) {
>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>> +						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
>> +			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>> +						csid->base + CSID_BUF_DONE_IRQ_MASK);
>> +		}
>> +
>> +	/* preserve registers */
>> +	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
>> +	writel_relaxed(val, csid->base + CSID_RST_CFG);
> 
> ... here - using everywhere relaxed here is odd and looks racy. These
> looks like some strict sequences.
> 
Yes, these are some sequences to initialize the HW.

> 
>> +
>> +	val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
>> +	writel_relaxed(val, csid->base + CSID_RST_CMD);
>> +
>> +	time = wait_for_completion_timeout(&csid->reset_complete,
>> +					   msecs_to_jiffies(CSID_RESET_TIMEOUT_MS));
>> +	if (!time) {
>> +		dev_err(csid->camss->dev, "CSID reset timeout\n");
>> +		return -EIO;
>> +	}
>> +
> 
> 
>> +
>> +static void csid_subdev_init(struct csid_device *csid)
>> +{
>> +	csid->testgen.modes = csid_testgen_modes;
>> +	csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
>> +}
>> +
>> +const struct csid_hw_ops csid_ops_gen3 = {
> 
> Isn't there a warning here?
> 
>> +	.configure_stream = csid_configure_stream,
>> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
>> +	.hw_version = csid_hw_version,
>> +	.isr = csid_isr,
>> +	.reset = csid_reset,
>> +	.src_pad_code = csid_src_pad_code,
>> +	.subdev_init = csid_subdev_init,
>> +};
> 
> Your patchset does not apply at all. Tried v6.9, v6.10, next. I see some
> dependency above, but that means no one can test it and no one can apply it.
> 
> Fix the warnings, I cannot verify it but I am sure you have them.
> 

My code base is next master branch, do you mean the 'declared and not 
used' warning? I don't see this warning with below two version compiler 
even though I just pick this patch and pull the code the latest version. 
But it indeed have this issue, these structures are declared and will be 
used later in "media: qcom: camss: Add sm8550 resources" patch, will 
think about how to avoid this.

aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
aarch64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

---

  CALL    scripts/checksyscalls.sh
   CC [M]  drivers/media/platform/qcom/camss/camss-csid.o
   CC [M]  drivers/media/platform/qcom/camss/camss-csid-4-1.o
   CC [M]  drivers/media/platform/qcom/camss/camss-csid-4-7.o
   CC [M]  drivers/media/platform/qcom/camss/camss-csid-gen2.o
   CC [M]  drivers/media/platform/qcom/camss/camss-csid-gen3.o
   CC [M]  drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.o
   CC [M]  drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.o

---

89a4a25b7b56 (HEAD -> master) media: qcom: camss: Add CSID Gen3 support 
for SM8550
d0cfd24496d3 media: qcom: camss: csiphy-3ph: Add Gen2 v1.2 two-phase 
MIPI CSI-2 DPHY init
5795fd39a8ee dt-bindings: media: camss: Add qcom,sm8550-camss binding

> Best regards,
> Krzysztof
> 

Thanks,
Depeng
Krzysztof Kozlowski July 11, 2024, 11:12 a.m. UTC | #4
On 11/07/2024 13:08, Depeng Shao wrote:


>>
>>> + */
>>> +static int csid_reset(struct csid_device *csid)
>>> +{
>>> +	unsigned long time;
>>> +	u32 val;
>>> +	int i;
>>> +
>>> +	reinit_completion(&csid->reset_complete);
>>> +
>>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
>>> +	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
>>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
>>> +
>>> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>>> +		if (csid->phy.en_vc & BIT(i)) {
>>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>>> +						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
>>> +			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
>>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>>> +						csid->base + CSID_BUF_DONE_IRQ_MASK);
>>> +		}
>>> +
>>> +	/* preserve registers */
>>> +	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
>>> +	writel_relaxed(val, csid->base + CSID_RST_CFG);
>>
>> ... here - using everywhere relaxed here is odd and looks racy. These
>> looks like some strict sequences.
>>
> Yes, these are some sequences to initialize the HW.

Hm? It's like you ignore the problem and just answer with whatever to
shut up the reviewer. Instead of replying with the same, address the
problem. Why ordering is not a problem here?

> 
>>
>>> +
>>> +	val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
>>> +	writel_relaxed(val, csid->base + CSID_RST_CMD);
>>> +
>>> +	time = wait_for_completion_timeout(&csid->reset_complete,
>>> +					   msecs_to_jiffies(CSID_RESET_TIMEOUT_MS));
>>> +	if (!time) {
>>> +		dev_err(csid->camss->dev, "CSID reset timeout\n");
>>> +		return -EIO;
>>> +	}
>>> +
>>
>>
>>> +
>>> +static void csid_subdev_init(struct csid_device *csid)
>>> +{
>>> +	csid->testgen.modes = csid_testgen_modes;
>>> +	csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
>>> +}
>>> +
>>> +const struct csid_hw_ops csid_ops_gen3 = {
>>
>> Isn't there a warning here?
>>
>>> +	.configure_stream = csid_configure_stream,
>>> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
>>> +	.hw_version = csid_hw_version,
>>> +	.isr = csid_isr,
>>> +	.reset = csid_reset,
>>> +	.src_pad_code = csid_src_pad_code,
>>> +	.subdev_init = csid_subdev_init,
>>> +};
>>
>> Your patchset does not apply at all. Tried v6.9, v6.10, next. I see some
>> dependency above, but that means no one can test it and no one can apply it.
>>
>> Fix the warnings, I cannot verify it but I am sure you have them.
>>
> 
> My code base is next master branch, do you mean the 'declared and not 
> used' warning? I don't see this warning with below two version compiler 
> even though I just pick this patch and pull the code the latest version. 
> But it indeed have this issue, these structures are declared and will be 
> used later in "media: qcom: camss: Add sm8550 resources" patch, will 
> think about how to avoid this.
> 
> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
> aarch64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

That's some old compilers... I am talking about recent GCC, recent clang
and of course W=1.



Best regards,
Krzysztof
Depeng Shao July 11, 2024, 11:41 a.m. UTC | #5
Hi Krzysztof,

On 7/11/2024 7:12 PM, Krzysztof Kozlowski wrote:
> On 11/07/2024 13:08, Depeng Shao wrote:
> 
> 
>>>
>>>> + */
>>>> +static int csid_reset(struct csid_device *csid)
>>>> +{
>>>> +	unsigned long time;
>>>> +	u32 val;
>>>> +	int i;
>>>> +
>>>> +	reinit_completion(&csid->reset_complete);
>>>> +
>>>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
>>>> +	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
>>>> +	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
>>>> +
>>>> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>>>> +		if (csid->phy.en_vc & BIT(i)) {
>>>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>>>> +						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
>>>> +			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
>>>> +			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>>>> +						csid->base + CSID_BUF_DONE_IRQ_MASK);
>>>> +		}
>>>> +
>>>> +	/* preserve registers */
>>>> +	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
>>>> +	writel_relaxed(val, csid->base + CSID_RST_CFG);
>>>
>>> ... here - using everywhere relaxed here is odd and looks racy. These
>>> looks like some strict sequences.
>>>
>> Yes, these are some sequences to initialize the HW.
> 
> Hm? It's like you ignore the problem and just answer with whatever to
> shut up the reviewer. Instead of replying with the same, address the
> problem. Why ordering is not a problem here?
> 

Sorry, I didn't mean that, was trying to understand the problem, then 
just sent out the mail by mistake.
Do you mean we should use writel to ensure the strict sequences?
Thanks for catching this problem, this problem is also in the the 
existing camss driver. I will check all of them in this series, but the 
problem in some existing camss drivers, maybe Bryan from Linaro can help 
to fix them, since I don't have these devices to verify the modifications.

>>>> +
>>>> +const struct csid_hw_ops csid_ops_gen3 = {
>>>
>>> Isn't there a warning here?
>>>
>>>> +	.configure_stream = csid_configure_stream,
>>>> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
>>>> +	.hw_version = csid_hw_version,
>>>> +	.isr = csid_isr,
>>>> +	.reset = csid_reset,
>>>> +	.src_pad_code = csid_src_pad_code,
>>>> +	.subdev_init = csid_subdev_init,
>>>> +};
>>>
>>> Your patchset does not apply at all. Tried v6.9, v6.10, next. I see some
>>> dependency above, but that means no one can test it and no one can apply it.
>>>
>>> Fix the warnings, I cannot verify it but I am sure you have them.
>>>
>>
>> My code base is next master branch, do you mean the 'declared and not
>> used' warning? I don't see this warning with below two version compiler
>> even though I just pick this patch and pull the code the latest version.
>> But it indeed have this issue, these structures are declared and will be
>> used later in "media: qcom: camss: Add sm8550 resources" patch, will
>> think about how to avoid this.
>>
>> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
>> aarch64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
> 
> That's some old compilers... I am talking about recent GCC, recent clang
> and of course W=1.
> 

Thanks for the sharing, I will try to upgrade to latest compiler to 
avoid other potential issues.


Thanks,
Depeng
Bryan O'Donoghue July 11, 2024, noon UTC | #6
On 11/07/2024 12:41, Depeng Shao wrote:
>>> Yes, these are some sequences to initialize the HW.
>>
>> Hm? It's like you ignore the problem and just answer with whatever to
>> shut up the reviewer. Instead of replying with the same, address the
>> problem. Why ordering is not a problem here?
>>
> 
> Sorry, I didn't mean that, was trying to understand the problem, then 
> just sent out the mail by mistake.
> Do you mean we should use writel to ensure the strict sequences?
> Thanks for catching this problem, this problem is also in the the 
> existing camss driver. I will check all of them in this series, but the 
> problem in some existing camss drivers, maybe Bryan from Linaro can help 
> to fix them, since I don't have these devices to verify the modifications.

_relaxed is used I'm sure because that's what's always been used and 
what downstream does.

Is there a good reason for it ? None that I can think of.

Krzysztof is right, there's no good reason to use relaxed() here at all, 
you should drop it.

---
bod
Depeng Shao July 11, 2024, 12:14 p.m. UTC | #7
Hi Bryan,

On 7/11/2024 8:00 PM, Bryan O'Donoghue wrote:
> On 11/07/2024 12:41, Depeng Shao wrote:
>>>> Yes, these are some sequences to initialize the HW.
>>>
>>> Hm? It's like you ignore the problem and just answer with whatever to
>>> shut up the reviewer. Instead of replying with the same, address the
>>> problem. Why ordering is not a problem here?
>>>
>>
>> Sorry, I didn't mean that, was trying to understand the problem, then 
>> just sent out the mail by mistake.
>> Do you mean we should use writel to ensure the strict sequences?
>> Thanks for catching this problem, this problem is also in the the 
>> existing camss driver. I will check all of them in this series, but 
>> the problem in some existing camss drivers, maybe Bryan from Linaro 
>> can help to fix them, since I don't have these devices to verify the 
>> modifications.
> 
> _relaxed is used I'm sure because that's what's always been used and 
> what downstream does.
> 
> Is there a good reason for it ? None that I can think of.
> 
> Krzysztof is right, there's no good reason to use relaxed() here at all, 
> you should drop it.
> 
> ---
> bod

Sure, I will drop the _relaxed.

Most io_write don't use _relaxed in the Qualcomm downstream camera 
driver, but few strict timing required code really have this problem, I 
will fix them.

Thanks for highlight it.

Thanks,
Depeng
Krzysztof Kozlowski July 11, 2024, 12:17 p.m. UTC | #8
On 11/07/2024 14:00, Bryan O'Donoghue wrote:
> On 11/07/2024 12:41, Depeng Shao wrote:
>>>> Yes, these are some sequences to initialize the HW.
>>>
>>> Hm? It's like you ignore the problem and just answer with whatever to
>>> shut up the reviewer. Instead of replying with the same, address the
>>> problem. Why ordering is not a problem here?
>>>
>>
>> Sorry, I didn't mean that, was trying to understand the problem, then 
>> just sent out the mail by mistake.
>> Do you mean we should use writel to ensure the strict sequences?
>> Thanks for catching this problem, this problem is also in the the 
>> existing camss driver. I will check all of them in this series, but the 
>> problem in some existing camss drivers, maybe Bryan from Linaro can help 
>> to fix them, since I don't have these devices to verify the modifications.
> 
> _relaxed is used I'm sure because that's what's always been used and 
> what downstream does.
> 
> Is there a good reason for it ? None that I can think of.
> 
> Krzysztof is right, there's no good reason to use relaxed() here at all, 
> you should drop it.
> 

In many cases relaxed will be fine, but in few might lead to tricky to
debug issues thus people introducing msleep() or other workarounds.
Usually init sequences are "sequences" for a reason, but of course here
maybe it does not matter.

Best regards,
Krzysztof
Depeng Shao July 11, 2024, 3:33 p.m. UTC | #9
Hi Bryan,

On 7/10/2024 7:28 PM, Bryan O'Donoghue wrote:
> On 09/07/2024 17:06, Depeng Shao wrote:
>> The CSID in SM8550 is gen3, it has new register offset and new
>> functionality. The buf done irq,register update and reset are
>> moved to CSID gen3. And CSID gen3 has a new register block which
>> is named as CSID top, it controls the output of CSID, since the
>> CSID can connect to Sensor Front End (SFE) or original VFE, the
>> register in top block is used to control the HW connection.
>>
>> 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 +
>>   .../platform/qcom/camss/camss-csid-gen3.c     | 445 ++++++++++++++++++
>>   .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
>>   .../media/platform/qcom/camss/camss-csid.h    |   2 +
>>   4 files changed, 474 insertions(+)
>>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c
>>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h
>>

>> +
>> +#define        REG_UPDATE_RDI        reg_update_rdi
>> +
>> +static void __csid_configure_rx(struct csid_device *csid,
>> +                struct csid_phy_config *phy, int vc)
>> +{
>> +    int val;
>> +
>> +    val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
>> +    val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
>> +    val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX) << 
>> CSI2_RX_CFG0_PHY_NUM_SEL;
>> +
>> +    writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
>> +
>> +    val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
>> +    if (vc > 3)
>> +        val |= 1 << CSI2_RX_CFG1_VC_MODE;
> 
> I realise downstream does these shifts but, I think in upstream we 
> should just define a BIT(x)
> 
> #define CSI2_RX_CFG1_VC_MODE BIT(2)
> 
> val |= CSI2_RX_CFG1_VC_MODE;
> 

Here CSI2_RX_CFG1_VC_MODE just means a register bit offset, not a 
register configuration.

Some fixed configuration can do this, but some register bits value are 
configured based on actual parameter.
e.g.;  val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;

If we want to use macro definition, maybe we can do like below.

#define CSI2_RX_CFG1_VC_MODE(n) ((n) << 2)
val |= CSI2_RX_CFG1_VC_MODE(1);


#define CSI2_RX_CFG0_DL0_INPUT_SEL(n) ((n) << 4)
val |= CSI2_RX_CFG0_DL0_INPUT_SEL(phy->lane_assign)

Could you please comment if we really need to do like this?


>> +    writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
>> +}
>> +
>> +static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8 
>> rdi)
>> +{
>> +    int val;
>> +
>> +    if (enable)
>> +        val = 1 << RDI_CTRL_START_CMD;
>> +    else
>> +        val = 0 << RDI_CTRL_START_CMD;
> 
> Here is an example of how the bit shifting is weird
> 
> (0 << anything) is still zero
> 

Understood, the value is same, but we can know the configuration clearly 
on this register bit. If we do like above way, then it likes below.

#define RDI_CTRL_START_CMD(n) ((n) << 0)  --> it is same with (n), but 
we don't know the register bit offset clearly if we use (n).

if (enable)
	val = RDI_CTRL_START_CMD(1);
else
	val = RDI_CTRL_START_CMD(0);

>> +    writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
>> +}
>> +
>> +static void __csid_configure_top(struct csid_device *csid)
>> +{
>> +    u32 val;
>> +
>> +    /* CSID "top" is a new function in Titan780.
>> +     * CSID can connect to VFE & SFE(Sensor Front End).
>> +     * This connection is ontrolled by CSID "top".
>> +     * Only enable VFE path in current driver.
>> +     */
>> +    if (csid->top_base) {
> 
> When is csid->top_base NULL ?
> 
> Its required in your yaml.
> 

csid->top_base is NULL when it is csid lite, I will add this info in yaml.


>> +
>> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
>> +{
>> +    u8 i;
>> +
>> +    /* Loop through all enabled VCs and configure stream for each */
>> +    for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>> +        if (csid->phy.en_vc & BIT(i)) {
>> +            /* Configure CSID "top" */
>> +            __csid_configure_top(csid);
>> +            __csid_configure_rdi_stream(csid, enable, i);
>> +            __csid_configure_rx(csid, &csid->phy, i);
>> +            __csid_ctrl_rdi(csid, enable, i);
>> +        }
>> +}
> 
> I really like this breakdown

Sorry, I don't get it, do you mean you like that configuring the 
different block use different functions, and no other meaning?

>> +
>> +static int csid_configure_testgen_pattern(struct csid_device *csid, 
>> s32 val)
>> +{
>> +    if (val > 0 && val <= csid->testgen.nmodes)
>> +        csid->testgen.mode = val;
> 
> Surely you should just set the val parameter directly ?
> 
> Also why is this a signed integer and how does it get assigned a 
> negative value which we need to mitigate against here  >

This was copied from csid-gen2 driver, they are same, so we can move to 
common file.

And the val comes from ctrl->val, so I guess this is the reason why this 
agrument is signed integer.

struct v4l2_ctrl {
	...
	s32 val;
	...
};



>> +
>> +static u32 csid_src_pad_code(struct csid_device *csid, u32 sink_code,
>> +                 unsigned int match_format_idx, u32 match_code)
>> +{
>> +    switch (sink_code) {
>> +    case MEDIA_BUS_FMT_SBGGR10_1X10:
>> +    {
>> +        u32 src_code[] = {
>> +            MEDIA_BUS_FMT_SBGGR10_1X10,
>> +            MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE,
>> +        };
>> +
>> +        return csid_find_code(src_code, ARRAY_SIZE(src_code),
>> +                      match_format_idx, match_code);
>> +    }
>> +    case MEDIA_BUS_FMT_Y10_1X10:
>> +    {
>> +        u32 src_code[] = {
>> +            MEDIA_BUS_FMT_Y10_1X10,
>> +            MEDIA_BUS_FMT_Y10_2X8_PADHI_LE,
>> +        };
>> +
>> +        return csid_find_code(src_code, ARRAY_SIZE(src_code),
>> +                      match_format_idx, match_code);
>> +    }
>> +    default:
>> +        if (match_format_idx > 0)
>> +            return 0;
>> +
>> +        return sink_code;
>> +    }
>> +}
> 
> Same code as in gen2.
> 
> You could move the gen2 version of this into camss-csid.c and just reuse 
> in both.
> 

Sure, it is same with the comments in vfe driver, I will try to move 
same code to camss-csid.c

Thanks,
Depeng
Depeng Shao July 31, 2024, 3:26 p.m. UTC | #10
On 7/11/2024 7:12 PM, Krzysztof Kozlowski wrote:
> On 11/07/2024 13:08, Depeng Shao wrote:

>>>
>>>> +
>>>> +static void csid_subdev_init(struct csid_device *csid)
>>>> +{
>>>> +	csid->testgen.modes = csid_testgen_modes;
>>>> +	csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
>>>> +}
>>>> +
>>>> +const struct csid_hw_ops csid_ops_gen3 = {
>>>
>>> Isn't there a warning here?
>>>
>>>> +	.configure_stream = csid_configure_stream,
>>>> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
>>>> +	.hw_version = csid_hw_version,
>>>> +	.isr = csid_isr,
>>>> +	.reset = csid_reset,
>>>> +	.src_pad_code = csid_src_pad_code,
>>>> +	.subdev_init = csid_subdev_init,
>>>> +};
>>>
>>> Your patchset does not apply at all. Tried v6.9, v6.10, next. I see some
>>> dependency above, but that means no one can test it and no one can apply it.
>>>
>>> Fix the warnings, I cannot verify it but I am sure you have them.
>>>
>>
>> My code base is next master branch, do you mean the 'declared and not
>> used' warning? I don't see this warning with below two version compiler
>> even though I just pick this patch and pull the code the latest version.
>> But it indeed have this issue, these structures are declared and will be
>> used later in "media: qcom: camss: Add sm8550 resources" patch, will
>> think about how to avoid this.
>>
>> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
>> aarch64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
> 
> That's some old compilers... I am talking about recent GCC, recent clang
> and of course W=1.
> 

Hi Krzysztof,

I'm preparing the next version patches, then I find it is hard to avoid 
such warning if only apply current patch, since this will be used in the 
below patch, it will be in structures csid_res_8550 -> sm8550_resources 
-> camss_dt_match, so I need to add all csid_res_8550, sm8550_resources, 
camss_dt_match into this patch if I want to avoid the compile warning,
then I also need to add compatible info for it to avoid sm8550_resources 
has unused variable warning, but the sm8550_resources structure also 
need to add other items to make it complete, then the driver will be 
incomplete but can be probed with this patch.

{ .compatible = "qcom,sm8550-camss", .data = &sm8550_resources },

https://lore.kernel.org/all/20240709160656.31146-14-quic_depengs@quicinc.com/

Could you please share some suggestions?


> 
> 
> Best regards,
> Krzysztof
> 

Thanks,
Depeng
Bryan O'Donoghue July 31, 2024, 4:12 p.m. UTC | #11
On 31/07/2024 16:26, Depeng Shao wrote:
> I'm preparing the next version patches, then I find it is hard to avoid 
> such warning if only apply current patch, since this will be used in the 
> below patch, it will be in structures csid_res_8550 -> sm8550_resources 
> -> camss_dt_match, so I need to add all csid_res_8550, sm8550_resources, 
> camss_dt_match into this patch if I want to avoid the compile warning,
> then I also need to add compatible info for it to avoid sm8550_resources 
> has unused variable warning, but the sm8550_resources structure also 
> need to add other items to make it complete, then the driver will be 
> incomplete but can be probed with this patch.
> 
> { .compatible = "qcom,sm8550-camss", .data = &sm8550_resources },
> 
> https://lore.kernel.org/all/20240709160656.31146-14-quic_depengs@quicinc.com/

Couldn't you just add the public structures at the same time they are 
referenced in &sm8550_resources ?

That way your patchset would progressively apply with no warnings.

---
bod
Depeng Shao Aug. 1, 2024, 1:53 a.m. UTC | #12
Hi Bryan,

On 8/1/2024 12:12 AM, Bryan O'Donoghue wrote:
> On 31/07/2024 16:26, Depeng Shao wrote:
>> I'm preparing the next version patches, then I find it is hard to 
>> avoid such warning if only apply current patch, since this will be 
>> used in the below patch, it will be in structures csid_res_8550 -> 
>> sm8550_resources -> camss_dt_match, so I need to add all 
>> csid_res_8550, sm8550_resources, camss_dt_match into this patch if I 
>> want to avoid the compile warning,
>> then I also need to add compatible info for it to avoid 
>> sm8550_resources has unused variable warning, but the sm8550_resources 
>> structure also need to add other items to make it complete, then the 
>> driver will be incomplete but can be probed with this patch.
>>
>> { .compatible = "qcom,sm8550-camss", .data = &sm8550_resources },
>>
>> https://lore.kernel.org/all/20240709160656.31146-14-quic_depengs@quicinc.com/
> 
> Couldn't you just add the public structures at the same time they are 
> referenced in &sm8550_resources ?
> 
> That way your patchset would progressively apply with no warnings.
> 

Sorry, I don't get it, but in my understanding, but looks like the only 
way to avoid the compile warning is that adding compatible change in 
early patch set.

I can add the compatible change and structure sm8550_resources in the 
early patch, but the structure sm8550_resources will just have very few 
info in this patch. Then fill the other elements in sm8550_resources in 
the following patches, this can avoid the warning, but the issue is that 
the sm8550 can be probed once having patch set 1, but the 
sm8550_resources isn't complete in patch set 1.
Could you please common if this is fine?

patch set 1
+{ .compatible = "qcom,sm8550-camss", .data = &sm8550_resources },


+static const struct camss_resources sm8550_resources = {
+	.version = CAMSS_8550,
+	.pd_name = "top",
+	.icc_res = icc_res_sm8550,
+	.icc_path_num = ARRAY_SIZE(icc_res_sm8550),
+	.link_entities = camss_link_entities
+};


patch set2 - Adding csiphy driver and csiphy res to sm8550_resources
static const struct camss_resources sm8550_resources = {
	.version = CAMSS_8550,
	.pd_name = "top",
+	.csiphy_res = csiphy_res_8550,
	.icc_res = icc_res_sm8550,
	.icc_path_num = ARRAY_SIZE(icc_res_sm8550),
+	.csiphy_num = ARRAY_SIZE(csiphy_res_8550),
	.link_entities = camss_link_entities
};


patch set 3 - Adding csid driver and csid res to sm8550_resources
static const struct camss_resources sm8550_resources = {
	.version = CAMSS_8550,
	.pd_name = "top",
	.csiphy_res = csiphy_res_8550,
+	.csid_res = csid_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),
	.link_entities = camss_link_entities
};

> ---
> bod

Thanks,
Depeng
Bryan O'Donoghue Aug. 1, 2024, 10:59 a.m. UTC | #13
On 01/08/2024 02:53, Depeng Shao wrote:
> but the issue is that the sm8550 can be probed once having patch set 1, 
> but the sm8550_resources isn't complete in patch set 1.

Doesn't matter until you _upstream_ a dts that operates on it.

Its not a massive deal if you start at patch #1 and and patch #1+n and a 
there are a few warnings in between as you add stuff in but, for 
preference every single patch applies and builds warning free.

If you

- Add bindings
- Do driver stuff
- Then do dts stuff

You don't need to worry about probe() doing mad things.

---
bod
Bryan O'Donoghue Aug. 1, 2024, 11:14 a.m. UTC | #14
On 01/08/2024 11:59, Bryan O'Donoghue wrote:
> for preference every single patch applies and builds warning free.

Oops mistyped

- Every patch must apply cleanly
- You could make an argument for some specific cases that
   a patch can generate a warning provided
- By the end of your set everything must be warning free

In this case though, I don't believe you need to make that case since, 
the problem you describe about probe() isn't a problem at all as you 
have no upstream dts that can drive the probe() at this point.

Just do the dts at the end and no problem.

---
bod
Depeng Shao Aug. 1, 2024, 1:49 p.m. UTC | #15
Hi Bryan,

On 8/1/2024 7:14 PM, Bryan O'Donoghue wrote:
> On 01/08/2024 11:59, Bryan O'Donoghue wrote:
>> for preference every single patch applies and builds warning free.
> 
> Oops mistyped
> 
> - Every patch must apply cleanly
> - You could make an argument for some specific cases that
>    a patch can generate a warning provided
> - By the end of your set everything must be warning free
> 
> In this case though, I don't believe you need to make that case since, 
> the problem you describe about probe() isn't a problem at all as you 
> have no upstream dts that can drive the probe() at this point.
> 
> Just do the dts at the end and no problem.
> 

Thanks for the confirmation, maybe also can add a checking for the res, 
probe returns fail if the .data->xxx_res is NULL.


Thanks,
Depeng
Depeng Shao Aug. 7, 2024, 3:10 p.m. UTC | #16
Hi Bryan,

Looks like you missed this mail, Could you please check again?


On 7/11/2024 11:33 PM, Depeng Shao wrote:
> Hi Bryan,
> 
> On 7/10/2024 7:28 PM, Bryan O'Donoghue wrote:
>> On 09/07/2024 17:06, Depeng Shao wrote:
>>> The CSID in SM8550 is gen3, it has new register offset and new
>>> functionality. The buf done irq,register update and reset are
>>> moved to CSID gen3. And CSID gen3 has a new register block which
>>> is named as CSID top, it controls the output of CSID, since the
>>> CSID can connect to Sensor Front End (SFE) or original VFE, the
>>> register in top block is used to control the HW connection.
>>>
>>> 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 +
>>>   .../platform/qcom/camss/camss-csid-gen3.c     | 445 ++++++++++++++++++
>>>   .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
>>>   .../media/platform/qcom/camss/camss-csid.h    |   2 +
>>>   4 files changed, 474 insertions(+)
>>>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c
>>>   create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h
>>>
> 
>>> +
>>> +#define        REG_UPDATE_RDI        reg_update_rdi
>>> +
>>> +static void __csid_configure_rx(struct csid_device *csid,
>>> +                struct csid_phy_config *phy, int vc)
>>> +{
>>> +    int val;
>>> +
>>> +    val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
>>> +    val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
>>> +    val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX) << 
>>> CSI2_RX_CFG0_PHY_NUM_SEL;
>>> +
>>> +    writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
>>> +
>>> +    val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
>>> +    if (vc > 3)
>>> +        val |= 1 << CSI2_RX_CFG1_VC_MODE;
>>
>> I realise downstream does these shifts but, I think in upstream we 
>> should just define a BIT(x)
>>
>> #define CSI2_RX_CFG1_VC_MODE BIT(2)
>>
>> val |= CSI2_RX_CFG1_VC_MODE;
>>
> 
> Here CSI2_RX_CFG1_VC_MODE just means a register bit offset, not a 
> register configuration.
> 
> Some fixed configuration can do this, but some register bits value are 
> configured based on actual parameter.
> e.g.;  val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
> 
> If we want to use macro definition, maybe we can do like below.
> 
> #define CSI2_RX_CFG1_VC_MODE(n) ((n) << 2)
> val |= CSI2_RX_CFG1_VC_MODE(1);
> 
> 
> #define CSI2_RX_CFG0_DL0_INPUT_SEL(n) ((n) << 4)
> val |= CSI2_RX_CFG0_DL0_INPUT_SEL(phy->lane_assign)
> 
> Could you please comment if we really need to do like this?
> 
> 
>>> +    writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
>>> +}
>>> +
>>> +static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8 
>>> rdi)
>>> +{
>>> +    int val;
>>> +
>>> +    if (enable)
>>> +        val = 1 << RDI_CTRL_START_CMD;
>>> +    else
>>> +        val = 0 << RDI_CTRL_START_CMD;
>>
>> Here is an example of how the bit shifting is weird
>>
>> (0 << anything) is still zero
>>
> 
> Understood, the value is same, but we can know the configuration clearly 
> on this register bit. If we do like above way, then it likes below.
> 
> #define RDI_CTRL_START_CMD(n) ((n) << 0)  --> it is same with (n), but 
> we don't know the register bit offset clearly if we use (n).
> 
> if (enable)
>      val = RDI_CTRL_START_CMD(1);
> else
>      val = RDI_CTRL_START_CMD(0);
> 
>>> +    writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
>>> +}
>>> +
>>> +static void __csid_configure_top(struct csid_device *csid)
>>> +{
>>> +    u32 val;
>>> +
>>> +    /* CSID "top" is a new function in Titan780.
>>> +     * CSID can connect to VFE & SFE(Sensor Front End).
>>> +     * This connection is ontrolled by CSID "top".
>>> +     * Only enable VFE path in current driver.
>>> +     */
>>> +    if (csid->top_base) {
>>
>> When is csid->top_base NULL ?
>>
>> Its required in your yaml.
>>
> 
> csid->top_base is NULL when it is csid lite, I will add this info in yaml.
> 
> 
>>> +
>>> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
>>> +{
>>> +    u8 i;
>>> +
>>> +    /* Loop through all enabled VCs and configure stream for each */
>>> +    for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>>> +        if (csid->phy.en_vc & BIT(i)) {
>>> +            /* Configure CSID "top" */
>>> +            __csid_configure_top(csid);
>>> +            __csid_configure_rdi_stream(csid, enable, i);
>>> +            __csid_configure_rx(csid, &csid->phy, i);
>>> +            __csid_ctrl_rdi(csid, enable, i);
>>> +        }
>>> +}
>>
>> I really like this breakdown
> 
> Sorry, I don't get it, do you mean you like that configuring the 
> different block use different functions, and no other meaning?
> 
>>> +
>>> +static int csid_configure_testgen_pattern(struct csid_device *csid, 
>>> s32 val)
>>> +{
>>> +    if (val > 0 && val <= csid->testgen.nmodes)
>>> +        csid->testgen.mode = val;
>>
>> Surely you should just set the val parameter directly ?
>>
>> Also why is this a signed integer and how does it get assigned a 
>> negative value which we need to mitigate against here  >
> 
> This was copied from csid-gen2 driver, they are same, so we can move to 
> common file.
> 
> And the val comes from ctrl->val, so I guess this is the reason why this 
> agrument is signed integer.
> 
> struct v4l2_ctrl {
>      ...
>      s32 val;
>      ...
> };
> 
> 
> 
>>> +
>>> +static u32 csid_src_pad_code(struct csid_device *csid, u32 sink_code,
>>> +                 unsigned int match_format_idx, u32 match_code)
>>> +{
>>> +    switch (sink_code) {
>>> +    case MEDIA_BUS_FMT_SBGGR10_1X10:
>>> +    {
>>> +        u32 src_code[] = {
>>> +            MEDIA_BUS_FMT_SBGGR10_1X10,
>>> +            MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE,
>>> +        };
>>> +
>>> +        return csid_find_code(src_code, ARRAY_SIZE(src_code),
>>> +                      match_format_idx, match_code);
>>> +    }
>>> +    case MEDIA_BUS_FMT_Y10_1X10:
>>> +    {
>>> +        u32 src_code[] = {
>>> +            MEDIA_BUS_FMT_Y10_1X10,
>>> +            MEDIA_BUS_FMT_Y10_2X8_PADHI_LE,
>>> +        };
>>> +
>>> +        return csid_find_code(src_code, ARRAY_SIZE(src_code),
>>> +                      match_format_idx, match_code);
>>> +    }
>>> +    default:
>>> +        if (match_format_idx > 0)
>>> +            return 0;
>>> +
>>> +        return sink_code;
>>> +    }
>>> +}
>>
>> Same code as in gen2.
>>
>> You could move the gen2 version of this into camss-csid.c and just 
>> reuse in both.
>>
> 
> Sure, it is same with the comments in vfe driver, I will try to move 
> same code to camss-csid.c
> 
> Thanks,
> Depeng

Thanks,
Depeng
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
index e636968a1126..c336e4c1a399 100644
--- a/drivers/media/platform/qcom/camss/Makefile
+++ b/drivers/media/platform/qcom/camss/Makefile
@@ -7,6 +7,7 @@  qcom-camss-objs += \
 		camss-csid-4-1.o \
 		camss-csid-4-7.o \
 		camss-csid-gen2.o \
+		camss-csid-gen3.o \
 		camss-csiphy-2ph-1-0.o \
 		camss-csiphy-3ph-1-0.o \
 		camss-csiphy.o \
diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
new file mode 100644
index 000000000000..17fd7c5499de
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
@@ -0,0 +1,445 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * camss-csid-gen3.c
+ *
+ * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module
+ *
+ * Copyright (c) 2024 Qualcomm Technologies, Inc.
+ */
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+
+#include "camss.h"
+#include "camss-csid.h"
+#include "camss-csid-gen3.h"
+
+
+#define CSID_TOP_IO_PATH_CFG0(csid)	(0x4 * (csid))
+#define		OUTPUT_IFE_EN 0x100
+#define		INTERNAL_CSID 1
+
+#define CSID_HW_VERSION		0x0
+#define		HW_VERSION_STEPPING	0
+#define		HW_VERSION_REVISION	16
+#define		HW_VERSION_GENERATION	28
+
+#define CSID_RST_CFG	0xC
+#define		RST_MODE		0
+#define		RST_LOCATION	4
+
+#define CSID_RST_CMD	0x10
+#define		SELECT_HW_RST	0
+#define		SELECT_SW_RST	1
+#define		SELECT_IRQ_RST	2
+
+#define CSID_CSI2_RX_IRQ_STATUS	0x9C
+#define	CSID_CSI2_RX_IRQ_MASK	0xA0
+#define CSID_CSI2_RX_IRQ_CLEAR	0xA4
+#define CSID_CSI2_RX_IRQ_SET	0xA8
+
+#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)		(0xEC + 0x10 * (rdi))
+#define CSID_CSI2_RDIN_IRQ_MASK(rdi)		(0xF0 + 0x10 * (rdi))
+#define   CSID_CSI2_RDIN_INFO_FIFO_FULL 2
+#define   CSID_CSI2_RDIN_INFO_CAMIF_EOF 3
+#define   CSID_CSI2_RDIN_INFO_CAMIF_SOF 4
+#define   CSID_CSI2_RDIN_INFO_INPUT_EOF 9
+#define   CSID_CSI2_RDIN_INFO_INPUT_SOF 12
+#define   CSID_CSI2_RDIN_ERROR_REC_FRAME_DROP 18
+#define   CSID_CSI2_RDIN_ERROR_REC_OVERFLOW_IRQ 19
+#define   CSID_CSI2_RDIN_ERROR_REC_CCIF_VIOLATION 20
+#define   CSID_CSI2_RDIN_EPOCH0 21
+#define   CSID_CSI2_RDIN_EPOCH1 22
+#define   CSID_CSI2_RDIN_RUP_DONE 23
+#define   CSID_CSI2_RDIN_CCIF_VIOLATION 29
+
+#define CSID_CSI2_RDIN_IRQ_CLEAR(rdi)		(0xF4 + 0x10 * (rdi))
+#define CSID_CSI2_RDIN_IRQ_SET(rdi)			(0xF8 + 0x10 * (rdi))
+
+#define CSID_TOP_IRQ_STATUS	0x7C
+#define		TOP_IRQ_STATUS_RESET_DONE 0
+#define CSID_TOP_IRQ_MASK	0x80
+#define CSID_TOP_IRQ_CLEAR	0x84
+#define CSID_TOP_IRQ_SET	0x88
+#define CSID_IRQ_CMD		0x14
+#define		IRQ_CMD_CLEAR	0
+#define		IRQ_CMD_SET	4
+
+#define CSID_BUF_DONE_IRQ_STATUS	0x8C
+#define		BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 0x1 : 0xE)
+#define CSID_BUF_DONE_IRQ_MASK	0x90
+#define CSID_BUF_DONE_IRQ_CLEAR	0x94
+#define CSID_BUF_DONE_IRQ_SET	0x98
+
+#define CSID_CSI2_RX_CFG0	0x200
+#define		CSI2_RX_CFG0_NUM_ACTIVE_LANES	0
+#define		CSI2_RX_CFG0_DL0_INPUT_SEL	4
+#define		CSI2_RX_CFG0_DL1_INPUT_SEL	8
+#define		CSI2_RX_CFG0_DL2_INPUT_SEL	12
+#define		CSI2_RX_CFG0_DL3_INPUT_SEL	16
+#define		CSI2_RX_CFG0_PHY_NUM_SEL	20
+#define			CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
+#define		CSI2_RX_CFG0_PHY_TYPE_SEL	24
+#define		CSI2_RX_CFG0_TPG_MUX_EN		27
+#define		CSI2_RX_CFG0_TPG_NUM_SEL	28
+
+#define CSID_CSI2_RX_CFG1	0x204
+#define		CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN		0
+#define		CSI2_RX_CFG1_DE_SCRAMBLE_EN			1
+#define		CSI2_RX_CFG1_VC_MODE				2
+#define		CSI2_RX_CFG1_COMPLETE_STREAM_EN			4
+#define		CSI2_RX_CFG1_COMPLETE_STREAM_FRAME_TIMING	5
+#define		CSI2_RX_CFG1_MISR_EN				6
+#define		CSI2_RX_CFG1_PHY_BIST_EN			7
+#define		CSI2_RX_CFG1_EPD_MODE				8
+#define		CSI2_RX_CFG1_EOTP_EN				9
+#define		CSI2_RX_CFG1_DYN_SWITCH_EN			10
+#define		CSI2_RX_CFG1_RUP_AUP_LATCH_DIS		11
+
+#define CSID_CSI2_RX_CAPTURE_CTRL	0x208
+#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_EN	0
+#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_EN	1
+#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_EN	3
+#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_DT		4
+#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_VC		10
+#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_VC	15
+#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_DT		20
+#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_VC		26
+
+#define CSID_RDI_CFG0(rdi)			(0x500 + 0x100 * (rdi))
+#define		RDI_CFG0_VFR_EN				0
+#define		RDI_CFG0_FRAME_ID_DEC_EN	1
+#define		RDI_CFG0_RETIME_DIS			5
+#define		RDI_CFG0_TIMESTAMP_EN		6
+#define		RDI_CFG0_TIMESTAMP_STB_SEL	8
+#define		RDI_CFG0_DECODE_FORMAT		12
+#define		RDI_CFG0_DT					16
+#define		RDI_CFG0_VC					22
+#define		RDI_CFG0_DT_ID				27
+#define		RDI_CFG0_EN					31
+
+#define CSID_RDI_CFG1(rdi)			(0x510 + 0x100 * (rdi))
+#define		RDI_CFG1_BYTE_CNTR_EN	2
+#define		RDI_CFG1_DROP_H_EN	5
+#define		RDI_CFG1_DROP_V_EN	6
+#define		RDI_CFG1_CROP_H_EN	7
+#define		RDI_CFG1_CROP_V_EN	8
+#define		RDI_CFG1_MISR_EN	9
+#define		RDI_CFG1_PIX_STORE  10
+#define		RDI_CFG1_PLAIN_ALIGNMENT	11
+#define		RDI_CFG1_PLAIN_FORMAT	12
+#define		RDI_CFG1_EARLY_EOF_EN	14
+#define		RDI_CFG1_PACKING_FORMAT	15
+
+#define CSID_RDI_CTRL(rdi)			(0x504 + 0x100 * (rdi))
+#define		RDI_CTRL_START_CMD		0
+#define		RDI_CTRL_START_MODE		2
+
+#define CSID_RDI_EPOCH_IRQ_CFG(rdi) (0x52C + 0x100 * (rdi))
+
+#define CSID_RDI_FRM_DROP_PATTERN(rdi)			(0x540 + 0x100 * (rdi))
+#define CSID_RDI_FRM_DROP_PERIOD(rdi)			(0x544 + 0x100 * (rdi))
+#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi)		(0x548 + 0x100 * (rdi))
+#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi)		(0x54C + 0x100 * (rdi))
+#define CSID_RDI_RPP_PIX_DROP_PATTERN(rdi)		(0x558 + 0x100 * (rdi))
+#define CSID_RDI_RPP_PIX_DROP_PERIOD(rdi)		(0x55C + 0x100 * (rdi))
+#define CSID_RDI_RPP_LINE_DROP_PATTERN(rdi)		(0x560 + 0x100 * (rdi))
+#define CSID_RDI_RPP_LINE_DROP_PERIOD(rdi)		(0x564 + 0x100 * (rdi))
+
+#define CSID_RDI_RPP_HCROP(rdi)                 (0x550 + 0x100 * (rdi))
+#define CSID_RDI_RPP_VCROP(rdi)                 (0x554 + 0x100 * (rdi))
+
+#define CSID_RDI_ERROR_RECOVERY_CFG0(rdi)       (0x514 + 0x100 * (rdi))
+
+#define CSID_REG_UPDATE_CMD		0x18
+static inline int reg_update_rdi(struct csid_device *csid, int n)
+{
+	return BIT(n + 4) + BIT(20 + n);
+}
+
+#define	    REG_UPDATE_RDI		reg_update_rdi
+
+static void __csid_configure_rx(struct csid_device *csid,
+				struct csid_phy_config *phy, int vc)
+{
+	int val;
+
+	val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
+	val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
+	val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX) << CSI2_RX_CFG0_PHY_NUM_SEL;
+
+	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
+
+	val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
+	if (vc > 3)
+		val |= 1 << CSI2_RX_CFG1_VC_MODE;
+	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
+}
+
+static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8 rdi)
+{
+	int val;
+
+	if (enable)
+		val = 1 << RDI_CTRL_START_CMD;
+	else
+		val = 0 << RDI_CTRL_START_CMD;
+	writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
+}
+
+static void __csid_configure_top(struct csid_device *csid)
+{
+	u32 val;
+
+	/* CSID "top" is a new function in Titan780.
+	 * CSID can connect to VFE & SFE(Sensor Front End).
+	 * This connection is ontrolled by CSID "top".
+	 * Only enable VFE path in current driver.
+	 */
+	if (csid->top_base) {
+		val = OUTPUT_IFE_EN | INTERNAL_CSID;
+		writel_relaxed(val, csid->top_base + CSID_TOP_IO_PATH_CFG0(csid->id));
+	}
+}
+
+static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 vc)
+{
+	u32 val;
+	u8 lane_cnt = csid->phy.lane_cnt;
+	/* Source pads matching RDI channels on hardware. Pad 1 -> RDI0, Pad 2 -> RDI1, etc. */
+	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + vc];
+	const struct csid_format_info *format = csid_get_fmt_entry(csid->res->formats->formats,
+								   csid->res->formats->nformats,
+								   input_format->code);
+
+	if (!lane_cnt)
+		lane_cnt = 4;
+
+	/*
+	 * DT_ID is a two bit bitfield that is concatenated with
+	 * the four least significant bits of the five bit VC
+	 * bitfield to generate an internal CID value.
+	 *
+	 * CSID_RDI_CFG0(vc)
+	 * DT_ID : 28:27
+	 * VC    : 26:22
+	 * DT    : 21:16
+	 *
+	 * CID   : VC 3:0 << 2 | DT_ID 1:0
+	 */
+	u8 dt_id = vc & 0x03;
+
+	val = 1 << RDI_CFG0_TIMESTAMP_EN;
+	val |= 2 << RDI_CFG0_TIMESTAMP_STB_SEL;
+	/* note: for non-RDI path, this should be format->decode_format */
+	val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
+	val |= vc << RDI_CFG0_VC;
+	val |= format->data_type << RDI_CFG0_DT;
+	val |= dt_id << RDI_CFG0_DT_ID;
+
+	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
+
+	val = 1 << RDI_CFG1_PACKING_FORMAT;
+	val |= 1 << RDI_CFG1_PIX_STORE;
+	val |= 1 << RDI_CFG1_DROP_H_EN;
+	val |= 1 << RDI_CFG1_DROP_V_EN;
+	val |= 1 << RDI_CFG1_CROP_H_EN;
+	val |= 1 << RDI_CFG1_CROP_V_EN;
+	val |= RDI_CFG1_EARLY_EOF_EN;
+
+	writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
+
+	val = 0;
+	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
+
+	val = 1;
+	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
+
+	val = 0;
+	writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
+
+	val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
+	val |=  enable << RDI_CFG0_EN;
+	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
+}
+
+static void csid_configure_stream(struct csid_device *csid, u8 enable)
+{
+	u8 i;
+
+	/* Loop through all enabled VCs and configure stream for each */
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i)) {
+			/* Configure CSID "top" */
+			__csid_configure_top(csid);
+			__csid_configure_rdi_stream(csid, enable, i);
+			__csid_configure_rx(csid, &csid->phy, i);
+			__csid_ctrl_rdi(csid, enable, i);
+		}
+}
+
+static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
+{
+	if (val > 0 && val <= csid->testgen.nmodes)
+		csid->testgen.mode = val;
+
+	return 0;
+}
+
+/*
+ * csid_hw_version - CSID hardware version query
+ * @csid: CSID device
+ *
+ * Return HW version or error
+ */
+static u32 csid_hw_version(struct csid_device *csid)
+{
+	u32 hw_version;
+	u32 hw_gen;
+	u32 hw_rev;
+	u32 hw_step;
+
+	hw_version = readl_relaxed(csid->base + CSID_HW_VERSION);
+	hw_gen = (hw_version >> HW_VERSION_GENERATION) & 0xF;
+	hw_rev = (hw_version >> HW_VERSION_REVISION) & 0xFFF;
+	hw_step = (hw_version >> HW_VERSION_STEPPING) & 0xFFFF;
+	dev_info(csid->camss->dev, "CSID HW Version = %u.%u.%u\n",
+		hw_gen, hw_rev, hw_step);
+
+	return hw_version;
+}
+
+/*
+ * csid_isr - CSID module interrupt service routine
+ * @irq: Interrupt line
+ * @dev: CSID device
+ *
+ * Return IRQ_HANDLED on success
+ */
+static irqreturn_t csid_isr(int irq, void *dev)
+{
+	struct csid_device *csid = dev;
+	u32 val, buf_done_val;
+	u8 reset_done;
+	int i;
+
+	val = readl_relaxed(csid->base + CSID_TOP_IRQ_STATUS);
+	writel_relaxed(val, csid->base + CSID_TOP_IRQ_CLEAR);
+	reset_done = val & BIT(TOP_IRQ_STATUS_RESET_DONE);
+
+	val = readl_relaxed(csid->base + CSID_CSI2_RX_IRQ_STATUS);
+	writel_relaxed(val, csid->base + CSID_CSI2_RX_IRQ_CLEAR);
+
+	buf_done_val = readl_relaxed(csid->base + CSID_BUF_DONE_IRQ_STATUS);
+	writel_relaxed(buf_done_val, csid->base + CSID_BUF_DONE_IRQ_CLEAR);
+
+	/* Read and clear IRQ status for each enabled RDI channel */
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i)) {
+			val = readl_relaxed(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
+			writel_relaxed(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
+		}
+
+	val = 1 << IRQ_CMD_CLEAR;
+	writel_relaxed(val, csid->base + CSID_IRQ_CMD);
+
+	if (reset_done)
+		complete(&csid->reset_complete);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * csid_reset - Trigger reset on CSID module and wait to complete
+ * @csid: CSID device
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int csid_reset(struct csid_device *csid)
+{
+	unsigned long time;
+	u32 val;
+	int i;
+
+	reinit_completion(&csid->reset_complete);
+
+	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
+	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
+	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
+
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i)) {
+			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
+			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
+			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+						csid->base + CSID_BUF_DONE_IRQ_MASK);
+		}
+
+	/* preserve registers */
+	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
+	writel_relaxed(val, csid->base + CSID_RST_CFG);
+
+	val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
+	writel_relaxed(val, csid->base + CSID_RST_CMD);
+
+	time = wait_for_completion_timeout(&csid->reset_complete,
+					   msecs_to_jiffies(CSID_RESET_TIMEOUT_MS));
+	if (!time) {
+		dev_err(csid->camss->dev, "CSID reset timeout\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static u32 csid_src_pad_code(struct csid_device *csid, u32 sink_code,
+			     unsigned int match_format_idx, u32 match_code)
+{
+	switch (sink_code) {
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+	{
+		u32 src_code[] = {
+			MEDIA_BUS_FMT_SBGGR10_1X10,
+			MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE,
+		};
+
+		return csid_find_code(src_code, ARRAY_SIZE(src_code),
+				      match_format_idx, match_code);
+	}
+	case MEDIA_BUS_FMT_Y10_1X10:
+	{
+		u32 src_code[] = {
+			MEDIA_BUS_FMT_Y10_1X10,
+			MEDIA_BUS_FMT_Y10_2X8_PADHI_LE,
+		};
+
+		return csid_find_code(src_code, ARRAY_SIZE(src_code),
+				      match_format_idx, match_code);
+	}
+	default:
+		if (match_format_idx > 0)
+			return 0;
+
+		return sink_code;
+	}
+}
+
+static void csid_subdev_init(struct csid_device *csid)
+{
+	csid->testgen.modes = csid_testgen_modes;
+	csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
+}
+
+const struct csid_hw_ops csid_ops_gen3 = {
+	.configure_stream = csid_configure_stream,
+	.configure_testgen_pattern = csid_configure_testgen_pattern,
+	.hw_version = csid_hw_version,
+	.isr = csid_isr,
+	.reset = csid_reset,
+	.src_pad_code = csid_src_pad_code,
+	.subdev_init = csid_subdev_init,
+};
diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.h b/drivers/media/platform/qcom/camss/camss-csid-gen3.h
new file mode 100644
index 000000000000..2241b91fd09e
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.h
@@ -0,0 +1,26 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * camss-csid-gen3.h
+ *
+ * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module Generation 1
+ *
+ * Copyright (c) 2024 Qualcomm Technologies, Inc.
+ */
+#ifndef QC_MSM_CAMSS_CSID_GEN3_H
+#define QC_MSM_CAMSS_CSID_GEN3_H
+
+#define DECODE_FORMAT_UNCOMPRESSED_8_BIT	0x1
+#define DECODE_FORMAT_UNCOMPRESSED_10_BIT	0x2
+#define DECODE_FORMAT_UNCOMPRESSED_12_BIT	0x3
+#define DECODE_FORMAT_UNCOMPRESSED_14_BIT	0x4
+#define DECODE_FORMAT_UNCOMPRESSED_16_BIT	0x5
+#define DECODE_FORMAT_UNCOMPRESSED_20_BIT	0x6
+#define DECODE_FORMAT_UNCOMPRESSED_24_BIT	0x7
+#define DECODE_FORMAT_PAYLOAD_ONLY		0xf
+
+
+#define PLAIN_FORMAT_PLAIN8	0x0 /* supports DPCM, UNCOMPRESSED_6/8_BIT */
+#define PLAIN_FORMAT_PLAIN16	0x1 /* supports DPCM, UNCOMPRESSED_10/16_BIT */
+#define PLAIN_FORMAT_PLAIN32	0x2 /* supports UNCOMPRESSED_20_BIT */
+
+#endif /* QC_MSM_CAMSS_CSID_GEN3_H */
diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index 8cdae98e4dca..ae5b6b0dc0ea 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -167,7 +167,9 @@  struct csid_device {
 	struct v4l2_subdev subdev;
 	struct media_pad pads[MSM_CSID_PADS_NUM];
 	void __iomem *base;
+	void __iomem *top_base;
 	u32 irq;
+	u32 reg_update;
 	char irq_name[30];
 	struct camss_clock *clock;
 	int nclocks;