[12/13] media: qcom: camss: Add CSID Gen3 support for sm8550
Commit Message
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.
The sm8550 also has a new block is named as CSID top, CSID can
connect to VFE or SFE(Sensor Front End), the connection is controlled
by CSID top.
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 | 339 ++++++++++++++++++
.../platform/qcom/camss/camss-csid-gen3.h | 26 ++
.../media/platform/qcom/camss/camss-csid.c | 46 ++-
.../media/platform/qcom/camss/camss-csid.h | 10 +
drivers/media/platform/qcom/camss/camss.c | 91 +++++
drivers/media/platform/qcom/camss/camss.h | 2 +
7 files changed, 503 insertions(+), 12 deletions(-)
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
On 12/08/2024 15:41, 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.
>
> The sm8550 also has a new block is named as CSID top, CSID can
> connect to VFE or SFE(Sensor Front End), the connection is controlled
> by CSID top.
>
> 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 | 339 ++++++++++++++++++
> .../platform/qcom/camss/camss-csid-gen3.h | 26 ++
So this "gen2" and "gen3" stuff would make sense if we had a number of
SoCs based on gen2 and gen3 which were controlled from the upper-level
gen2.c and gen3.c.
What you're submitting here is csid-780 so the file should be named
csid-780.
When we add 680 or 880 then it makes sense to try to encapsulate a class
of generation into one file - potentially.
I'd guess that was the intent behind gen2.c.
TL;DR please name your file csid-xxx.c
> .../media/platform/qcom/camss/camss-csid.c | 46 ++-
> .../media/platform/qcom/camss/camss-csid.h | 10 +
> drivers/media/platform/qcom/camss/camss.c | 91 +++++
> drivers/media/platform/qcom/camss/camss.h | 2 +
> 7 files changed, 503 insertions(+), 12 deletions(-)
> 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..d96bc126f0a9
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> @@ -0,0 +1,339 @@
> +// 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_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_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_REG_UPDATE_CMD 0x18
> +
> +#define CSID_BUF_DONE_IRQ_STATUS 0x8C
> +#define BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 1 : 14)
> +#define CSID_BUF_DONE_IRQ_MASK 0x90
> +#define CSID_BUF_DONE_IRQ_CLEAR 0x94
> +#define CSID_BUF_DONE_IRQ_SET 0x98
> +
> +#define CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
> +
> +#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_PHY_NUM_SEL 20
> +
> +#define CSID_CSI2_RX_CFG1 0x204
> +#define CSI2_RX_CFG1_ECC_CORRECTION_EN 0
> +#define CSI2_RX_CFG1_VC_MODE 2
> +
> +#define CSID_RDI_CFG0(rdi) (0x500 + 0x100 * (rdi))
> +#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_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_PIX_STORE 10
> +#define RDI_CFG1_PACKING_FORMAT 15
> +
> +#define CSID_RDI_CTRL(rdi) (0x504 + 0x100 * (rdi))
> +#define RDI_CTRL_START_CMD 0
> +
> +#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi) (0x548 + 0x100 * (rdi))
> +#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi) (0x54C + 0x100 * (rdi))
> +
> +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(val, csid->base + CSID_CSI2_RX_CFG0);
> +
> + val = 1 << CSI2_RX_CFG1_ECC_CORRECTION_EN;
> + if (vc > 3)
> + val |= 1 << CSI2_RX_CFG1_VC_MODE;
So again these are needless bit-shifts.
#define CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN BIT(0)
val = CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
> + writel(val, csid->base + CSID_CSI2_RX_CFG1);
> +}
> +
> +static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8 rdi)
> +{
> + int val = 0;
> +
> + if (enable)
> + val = 1 << RDI_CTRL_START_CMD;
and again, please apply BIT() as thorougly as possible in your submission.
> + writel(val, csid->base + CSID_RDI_CTRL(rdi));
> +}
> +
> +static void __csid_configure_top(struct csid_device *csid)
> +{
> + u32 val;
> +
> + /* csid lite doesn't need to configure top register */
> + if (csid->res->is_lite)
> + return;
> +
> + /* CSID top is a new function in Titan780.
> + * CSID can connect to VFE & SFE(Sensor Front End).
> + * This connection is controlled by CSID top.
> + * Only enable VFE path in current driver.
> + */
> + val = OUTPUT_IFE_EN | INTERNAL_CSID;
> + writel(val, csid->camss->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 |= 1 << 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(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;
> +
> + writel(val, csid->base + CSID_RDI_CFG1(vc));
> +
> + val = 0;
> + writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
> +
> + val = 1;
> + writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
> +
> + val = 0;
> + writel(val, csid->base + CSID_RDI_CTRL(vc));
> +
> + val = readl(csid->base + CSID_RDI_CFG0(vc));
> +
> + if (enable)
> + val |= 1 << RDI_CFG0_EN;
> + writel(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)) {
> + __csid_configure_top(csid);
> + __csid_configure_rdi_stream(csid, enable, i);
> + __csid_configure_rx(csid, &csid->phy, i);
> + __csid_ctrl_rdi(csid, enable, i);
> + }
> +}
> +
> +/*
> + * 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(csid->base + CSID_TOP_IRQ_STATUS);
> + writel(val, csid->base + CSID_TOP_IRQ_CLEAR);
> + reset_done = val & BIT(TOP_IRQ_STATUS_RESET_DONE);
> +
> + val = readl(csid->base + CSID_CSI2_RX_IRQ_STATUS);
> + writel(val, csid->base + CSID_CSI2_RX_IRQ_CLEAR);
> +
> + buf_done_val = readl(csid->base + CSID_BUF_DONE_IRQ_STATUS);
> + writel(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(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
> + writel(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
> +
> + if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i)) {
> + /* For Titan 780, Buf Done IRQ® has been moved to CSID from VFE.
> + * Once CSID received Buf Done, need notify this event to VFE.
> + * Trigger VFE to handle Buf Done process.
> + */
> + camss_buf_done(csid->camss, csid->id, i);
> + }
> + }
> +
> + val = 1 << IRQ_CMD_CLEAR;
> + writel(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(1, csid->base + CSID_TOP_IRQ_CLEAR);
> + writel(1, csid->base + CSID_IRQ_CMD);
> + writel(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(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> + csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> + writel(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
> + writel(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(val, csid->base + CSID_RST_CFG);
> +
> + val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
> + writel(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 void csid_subdev_reg_update(struct csid_device *csid, int port_id, bool is_clear)
> +{
> + if (is_clear) {
> + csid->reg_update &= ~REG_UPDATE_RDI(csid, port_id);
> + } else {
> + csid->reg_update |= REG_UPDATE_RDI(csid, port_id);
> + writel(csid->reg_update, csid->base + CSID_REG_UPDATE_CMD);
> + }
> +}
Right so this function should
1. Write the register
2. Wait on a completion
See camss-vfe-480.c::vfe_isr_reg_update()
3. Have that completion fire in the CSID ISR
4. Or timeout
5. Returning either 0 for success or -ETIMEDOUT
to the calling function so that we can be sure the RUP interrupt has
fired and completed - or we have appropriately timed out and captured
the failure.
Also - in camss-vfe-480.c the ISR clears the RUP which one assumes is
still the required logical flow with the RUP now residing in CSID.
> +
> +static void csid_subdev_init(struct csid_device *csid)
> +{
> + /* nop */
> +}
> +
> +const struct csid_hw_ops csid_ops_gen3 = {
> + /* No testgen pattern hw in csid gen3 HW */
> + .configure_testgen_pattern = NULL,
> + .configure_stream = csid_configure_stream,
> + .hw_version = csid_hw_version,
> + .isr = csid_isr,
> + .reset = csid_reset,
> + .src_pad_code = csid_src_pad_code,
> + .subdev_init = csid_subdev_init,
> + .reg_update = csid_subdev_reg_update,
> +};
> 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..43aa0d8d89b9
> --- /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 3
> + *
> + * 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.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 5806df7e7a7c..8ce08929d91f 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -838,7 +838,7 @@ static void csid_try_format(struct csid_device *csid,
> break;
>
> case MSM_CSID_PAD_SRC:
> - if (csid->testgen_mode->cur.val == 0) {
> + if (!csid->testgen_mode || csid->testgen_mode->cur.val == 0) {
See my comments on adding new guards to core functionality.
Is this sm8550 specific or generic ?
> /* Test generator is disabled, */
> /* keep pad formats in sync */
> u32 code = fmt->code;
> @@ -1042,6 +1042,7 @@ static int csid_init_formats(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> static int csid_set_test_pattern(struct csid_device *csid, s32 value)
> {
> struct csid_testgen_config *tg = &csid->testgen;
> + const struct csid_hw_ops *hw_ops = csid->res->hw_ops;
>
> /* If CSID is linked to CSIPHY, do not allow to enable test generator */
> if (value && media_pad_remote_pad_first(&csid->pads[MSM_CSID_PAD_SINK]))
> @@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct csid_device *csid, s32 value)
>
> tg->enabled = !!value;
>
> - return csid->res->hw_ops->configure_testgen_pattern(csid, value);
> + if (hw_ops->configure_testgen_pattern)
> + return -EOPNOTSUPP;
> + else
> + return hw_ops->configure_testgen_pattern(csid, value);
If you just add a dummy configure_testgen_pattern we can get rid of this
branching stuff.
> }
>
> /*
> @@ -1121,6 +1125,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
> csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
> if (IS_ERR(csid->base))
> return PTR_ERR(csid->base);
> +
> + /* CSID "top" is a new function in new version HW,
> + * CSID can connect to VFE & SFE(Sensor Front End).
> + * this connection is controlled by CSID "top" registers.
> + * There is only one CSID "top" region for all CSIDs.
> + */
> + if (!csid_is_lite(csid) && res->reg[1] && !camss->csid_top_base) {
> + camss->csid_top_base =
> + devm_platform_ioremap_resource_byname(pdev, res->reg[1]);
That's a complex clause.
Let me send you a patch to do it a different way.
> +
> + if (IS_ERR(camss->csid_top_base))
> + return PTR_ERR(camss->csid_top_base);
> + }
> }
>
> /* Interrupt */
> @@ -1267,7 +1284,7 @@ static int csid_link_setup(struct media_entity *entity,
>
> /* If test generator is enabled */
> /* do not allow a link from CSIPHY to CSID */
> - if (csid->testgen_mode->cur.val != 0)
> + if (csid->testgen_mode && csid->testgen_mode->cur.val != 0)
OK, so stuff like this isn't really about enabling 8550.
If you want to add an additional guard here, it _needs_ to be a
standalone patch which fixes a particular issue.
I'm not sure if this new guard is necessary - and the right place to
tease that out, is in a specific patch for that purpose, not in a new
silicon enabling patch.
That's a blanket comment for the series.
> return -EBUSY;
>
> sd = media_entity_to_v4l2_subdev(remote->entity);
> @@ -1366,15 +1383,20 @@ int msm_csid_register_entity(struct csid_device *csid,
> return ret;
> }
>
> - csid->testgen_mode = v4l2_ctrl_new_std_menu_items(&csid->ctrls,
> - &csid_ctrl_ops, V4L2_CID_TEST_PATTERN,
> - csid->testgen.nmodes, 0, 0,
> - csid->testgen.modes);
> -
> - if (csid->ctrls.error) {
> - dev_err(dev, "Failed to init ctrl: %d\n", csid->ctrls.error);
> - ret = csid->ctrls.error;
> - goto free_ctrl;
> + if (csid->res->hw_ops->configure_testgen_pattern) {
> + csid->testgen_mode =
> + v4l2_ctrl_new_std_menu_items(&csid->ctrls,
> + &csid_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + csid->testgen.nmodes, 0,
> + 0, csid->testgen.modes);
Here again - what's going on here and how is it 8550/csid-680 specific ?
Don't just add new guards or new debug statements unless its under a
heading of "add a new check for reason X".
The reason for that is we want granular patches in particular we don't
want to "backdoor" in fixes into a new SoC which can't be cherry-picked
back to a stable kernel.
> +
> + if (csid->ctrls.error) {
> + dev_err(dev, "Failed to init ctrl: %d\n",
> + csid->ctrls.error);
> + ret = csid->ctrls.error;
> + goto free_ctrl;
> + }
> }
>
> csid->subdev.ctrl_handler = &csid->ctrls;
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index f52209b96583..2715707dcdb4 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -152,6 +152,14 @@ struct csid_hw_ops {
> * @csid: CSID device
> */
> void (*subdev_init)(struct csid_device *csid);
> +
> + /*
> + * reg_update - receive message from other sub device
> + * @csid: CSID device
> + * @port_id: Port id
> + * @is_clear: Indicate if it is clearing reg update or setting reg update
> + */
> + void (*reg_update)(struct csid_device *csid, int port_id, bool is_clear);
> };
>
> struct csid_subdev_resources {
> @@ -168,6 +176,7 @@ struct csid_device {
> struct media_pad pads[MSM_CSID_PADS_NUM];
> void __iomem *base;
> u32 irq;
> + u32 reg_update;
> char irq_name[30];
> struct camss_clock *clock;
> int nclocks;
> @@ -228,6 +237,7 @@ extern const struct csid_formats csid_formats_gen2;
> extern const struct csid_hw_ops csid_ops_4_1;
> extern const struct csid_hw_ops csid_ops_4_7;
> extern const struct csid_hw_ops csid_ops_gen2;
> +extern const struct csid_hw_ops csid_ops_gen3;
>
> /*
> * csid_is_lite - Check if CSID is CSID lite.
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1cdd40f49c27..7ee102948dc4 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1588,6 +1588,84 @@ static const struct camss_subdev_resources csiphy_res_8550[] = {
> }
> };
>
> +static const struct camss_subdev_resources csid_res_8550[] = {
> + /* CSID0 */
> + {
> + .regulators = { "vdda-phy", "vdda-pll" },
> + .clock = { "csid", "csiphy_rx" },
> + .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
> + { 400000000, 480000000, 480000000, 480000000, 480000000 } },
I think you should be setting at least _one_ CAMNOC clock in the CSID.
> + .reg = { "csid0", "csid_top" },
> + .interrupt = { "csid0" },
> + .csid = {
> + .is_lite = false,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .hw_ops = &csid_ops_gen3,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID1 */
> + {
> + .regulators = { "vdda-phy", "vdda-pll" },
> + .clock = { "csid", "csiphy_rx" },
> + .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
> + { 400000000, 480000000, 480000000, 480000000, 480000000 } },
> + .reg = { "csid1", "csid_top" },
> + .interrupt = { "csid1" },
> + .csid = {
> + .is_lite = false,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .hw_ops = &csid_ops_gen3,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID2 */
> + {
> + .regulators = { "vdda-phy", "vdda-pll" },
> + .clock = { "csid", "csiphy_rx" },
> + .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
> + { 400000000, 480000000, 480000000, 480000000, 480000000 } },
> + .reg = { "csid2", "csid_top" },
> + .interrupt = { "csid2" },
> + .csid = {
> + .is_lite = false,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .hw_ops = &csid_ops_gen3,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID3 */
> + {
> + .regulators = { "vdda-phy", "vdda-pll" },
> + .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx" },
> + .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
> + { 400000000, 480000000, 480000000, 480000000, 480000000 } },
> + .reg = { "csid_lite0" },
> + .interrupt = { "csid_lite0" },
> + .csid = {
> + .is_lite = true,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .hw_ops = &csid_ops_gen3,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID4 */
> + {
> + .regulators = { "vdda-phy", "vdda-pll" },
> + .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx" },
> + .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
> + { 400000000, 480000000, 480000000, 480000000, 480000000 } },
> + .reg = { "csid_lite1" },
> + .interrupt = { "csid_lite1" },
> + .csid = {
> + .is_lite = true,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .hw_ops = &csid_ops_gen3,
> + .formats = &csid_formats_gen2
> + }
> + }
> +};
> +
> static const struct resources_icc icc_res_sm8550[] = {
> {
> .name = "ahb",
> @@ -1768,6 +1846,17 @@ void camss_pm_domain_off(struct camss *camss, int id)
> }
> }
>
> +void camss_buf_done(struct camss *camss, int hw_id, int port_id)
> +{
> + struct vfe_device *vfe;
> +
> + if (hw_id < camss->res->vfe_num) {
> + vfe = &(camss->vfe[hw_id]);
> +
> + vfe->res->hw_ops->vfe_buf_done(vfe, port_id);
> + }
> +}
> +
> static int vfe_parent_dev_ops_get(struct camss *camss, int id)
> {
> int ret = -EINVAL;
> @@ -2578,9 +2667,11 @@ 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
> };
>
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 5568ab32d5d7..d6b6558a82b9 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -117,6 +117,7 @@ struct camss {
> struct device_link *genpd_link;
> struct icc_path *icc_path[ICC_SM8250_COUNT];
> const struct camss_resources *res;
> + void __iomem *csid_top_base;
> };
>
> struct camss_camera_interface {
> @@ -155,5 +156,6 @@ void camss_pm_domain_off(struct camss *camss, int id);
> 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);
>
> #endif /* QC_MSM_CAMSS_H */
Hi Bryan,
>> ---
>> drivers/media/platform/qcom/camss/Makefile | 1 +
>> .../platform/qcom/camss/camss-csid-gen3.c | 339 ++++++++++++++++++
>> .../platform/qcom/camss/camss-csid-gen3.h | 26 ++
>
>
> So this "gen2" and "gen3" stuff would make sense if we had a number of
> SoCs based on gen2 and gen3 which were controlled from the upper-level
> gen2.c and gen3.c.
>
> What you're submitting here is csid-780 so the file should be named
> csid-780.
>
> When we add 680 or 880 then it makes sense to try to encapsulate a class
> of generation into one file - potentially.
>
> I'd guess that was the intent behind gen2.c.
>
> TL;DR please name your file csid-xxx.c
Sure, I will use csid-780.c
>> +
>> + writel(val, csid->base + CSID_CSI2_RX_CFG0);
>> +
>> + val = 1 << CSI2_RX_CFG1_ECC_CORRECTION_EN;
>> + if (vc > 3)
>> + val |= 1 << CSI2_RX_CFG1_VC_MODE;
>
> So again these are needless bit-shifts.
>
> #define CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN BIT(0)
>
> val = CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
>
You posted same comments in v3 series, I also replied it.
https://lore.kernel.org/all/eeaf4f4e-5200-4b13-b38f-3f3385fc2a2b@quicinc.com/
Some of register bits which just need to be configured to 0 or 1, then
can use BIT(X), but some register bits need to configure a specific
value, e.g., CSID_RDI_CFG0 bits[22:26] need to configure a vc vaule,
bits[16:21] need to configure a dt value, then we can't use BIT(x) to
handle this.
>> +
>> +static void csid_subdev_reg_update(struct csid_device *csid, int
>> port_id, bool is_clear)
>> +{
>> + if (is_clear) {
>> + csid->reg_update &= ~REG_UPDATE_RDI(csid, port_id);
>> + } else {
>> + csid->reg_update |= REG_UPDATE_RDI(csid, port_id);
>> + writel(csid->reg_update, csid->base + CSID_REG_UPDATE_CMD);
>> + }
>> +}
>
> Right so this function should
>
> 1. Write the register
> 2. Wait on a completion
> See camss-vfe-480.c::vfe_isr_reg_update()
> 3. Have that completion fire in the CSID ISR
> 4. Or timeout
> 5. Returning either 0 for success or -ETIMEDOUT
>
> to the calling function so that we can be sure the RUP interrupt has
> fired and completed - or we have appropriately timed out and captured
> the failure.
>
> Also - in camss-vfe-480.c the ISR clears the RUP which one assumes is
> still the required logical flow with the RUP now residing in CSID.
>
Sure, I forget to add this, will add them in next series.
>> case MSM_CSID_PAD_SRC:
>> - if (csid->testgen_mode->cur.val == 0) {
>> + if (!csid->testgen_mode || csid->testgen_mode->cur.val == 0) {
>
> See my comments on adding new guards to core functionality.
>
> Is this sm8550 specific or generic ?
>
It is sm8550 specific, since we don't have testgen mode in sm8550 csid,
so need to add some guards, the guards are added for similar reason.
>> /* Test generator is disabled, */
>> /* keep pad formats in sync */
>> u32 code = fmt->code;
>> @@ -1042,6 +1042,7 @@ static int csid_init_formats(struct v4l2_subdev
>> *sd, struct v4l2_subdev_fh *fh)
>> static int csid_set_test_pattern(struct csid_device *csid, s32 value)
>> {
>> struct csid_testgen_config *tg = &csid->testgen;
>> + const struct csid_hw_ops *hw_ops = csid->res->hw_ops;
>> /* If CSID is linked to CSIPHY, do not allow to enable test
>> generator */
>> if (value && media_pad_remote_pad_first(&csid-
>> >pads[MSM_CSID_PAD_SINK]))
>> @@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct
>> csid_device *csid, s32 value)
>> tg->enabled = !!value;
>> - return csid->res->hw_ops->configure_testgen_pattern(csid, value);
>> + if (hw_ops->configure_testgen_pattern)
>> + return -EOPNOTSUPP;
>> + else
>> + return hw_ops->configure_testgen_pattern(csid, value);
>
> If you just add a dummy configure_testgen_pattern we can get rid of this
> branching stuff.
>
Do you mean add dummy function in csid-780/gen3.c? How about the other
ops in vfe_ops_780, add dummy function or use NULL? We need to guards if
we set it as NULL.
static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
{
return 0;
}
>> }
>> /*
>> @@ -1121,6 +1125,19 @@ int msm_csid_subdev_init(struct camss *camss,
>> struct csid_device *csid,
>> csid->base = devm_platform_ioremap_resource_byname(pdev,
>> res->reg[0]);
>> if (IS_ERR(csid->base))
>> return PTR_ERR(csid->base);
>> +
>> + /* CSID "top" is a new function in new version HW,
>> + * CSID can connect to VFE & SFE(Sensor Front End).
>> + * this connection is controlled by CSID "top" registers.
>> + * There is only one CSID "top" region for all CSIDs.
>> + */
>> + if (!csid_is_lite(csid) && res->reg[1] && !camss-
>> >csid_top_base) {
>> + camss->csid_top_base =
>> + devm_platform_ioremap_resource_byname(pdev, res-
>> >reg[1]);
>
> That's a complex clause.
>
> Let me send you a patch to do it a different way.
>
I was also thinking to addd it in camss level, then I thought it is in
csid block, so I moved it to csid, but it is also fine to add it in
camss. Can I add your patch into this series? Just like the csiphy patches.
Thanks,
Depeng
On 15/08/2024 16:14, Depeng Shao wrote:
> Hi Bryan,
>
>
>>> ---
>>> drivers/media/platform/qcom/camss/Makefile | 1 +
>>> .../platform/qcom/camss/camss-csid-gen3.c | 339 ++++++++++++++++++
>>> .../platform/qcom/camss/camss-csid-gen3.h | 26 ++
>>
>>
>> So this "gen2" and "gen3" stuff would make sense if we had a number of
>> SoCs based on gen2 and gen3 which were controlled from the upper-level
>> gen2.c and gen3.c.
>>
>> What you're submitting here is csid-780 so the file should be named
>> csid-780.
>>
>> When we add 680 or 880 then it makes sense to try to encapsulate a
>> class of generation into one file - potentially.
>>
>> I'd guess that was the intent behind gen2.c.
>>
>> TL;DR please name your file csid-xxx.c
>
> Sure, I will use csid-780.c
>
>>> +
>>> + writel(val, csid->base + CSID_CSI2_RX_CFG0);
>>> +
>>> + val = 1 << CSI2_RX_CFG1_ECC_CORRECTION_EN;
>>> + if (vc > 3)
>>> + val |= 1 << CSI2_RX_CFG1_VC_MODE;
>>
>> So again these are needless bit-shifts.
>>
>> #define CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN BIT(0)
>>
>> val = CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
>>
>
> You posted same comments in v3 series, I also replied it.
> https://lore.kernel.org/all/eeaf4f4e-5200-4b13-b38f-3f3385fc2a2b@quicinc.com/
>
> Some of register bits which just need to be configured to 0 or 1, then
> can use BIT(X), but some register bits need to configure a specific
> value, e.g., CSID_RDI_CFG0 bits[22:26] need to configure a vc vaule,
> bits[16:21] need to configure a dt value, then we can't use BIT(x) to
> handle this.
Yes please use macros() to bury any _necessary_ bit shifts to populate a
_bit_field_ away but as an example CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN
is not a bit-field.
>
>
>>> case MSM_CSID_PAD_SRC:
>>> - if (csid->testgen_mode->cur.val == 0) {
>>> + if (!csid->testgen_mode || csid->testgen_mode->cur.val == 0) {
>>
>> See my comments on adding new guards to core functionality.
>>
>> Is this sm8550 specific or generic ?
>>
>
> It is sm8550 specific, since we don't have testgen mode in sm8550 csid,
> so need to add some guards, the guards are added for similar reason.
Hmm, I see in my tree I just assigned testgen_mode to some dummy data.
You're right, retain this, when we enable testgen as a standalone entity
outside of CSID we can address this again.
>
>>> /* Test generator is disabled, */
>>> /* keep pad formats in sync */
>>> u32 code = fmt->code;
>>> @@ -1042,6 +1042,7 @@ static int csid_init_formats(struct v4l2_subdev
>>> *sd, struct v4l2_subdev_fh *fh)
>>> static int csid_set_test_pattern(struct csid_device *csid, s32 value)
>>> {
>>> struct csid_testgen_config *tg = &csid->testgen;
>>> + const struct csid_hw_ops *hw_ops = csid->res->hw_ops;
>>> /* If CSID is linked to CSIPHY, do not allow to enable test
>>> generator */
>>> if (value && media_pad_remote_pad_first(&csid-
>>> >pads[MSM_CSID_PAD_SINK]))
>>> @@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct
>>> csid_device *csid, s32 value)
>>> tg->enabled = !!value;
>>> - return csid->res->hw_ops->configure_testgen_pattern(csid, value);
>>> + if (hw_ops->configure_testgen_pattern)
>>> + return -EOPNOTSUPP;
>>> + else
>>> + return hw_ops->configure_testgen_pattern(csid, value);
>>
>> If you just add a dummy configure_testgen_pattern we can get rid of
>> this branching stuff.
>>
>
> Do you mean add dummy function in csid-780/gen3.c? How about the other
> ops in vfe_ops_780, add dummy function or use NULL? We need to guards if
> we set it as NULL.
See above, you're right what you have is fine.
>
> static int csid_configure_testgen_pattern(struct csid_device *csid, s32
> val)
> {
> return 0;
> }
>
>>> }
>>> /*
>>> @@ -1121,6 +1125,19 @@ int msm_csid_subdev_init(struct camss *camss,
>>> struct csid_device *csid,
>>> csid->base = devm_platform_ioremap_resource_byname(pdev,
>>> res->reg[0]);
>>> if (IS_ERR(csid->base))
>>> return PTR_ERR(csid->base);
>>> +
>>> + /* CSID "top" is a new function in new version HW,
>>> + * CSID can connect to VFE & SFE(Sensor Front End).
>>> + * this connection is controlled by CSID "top" registers.
>>> + * There is only one CSID "top" region for all CSIDs.
>>> + */
>>> + if (!csid_is_lite(csid) && res->reg[1] && !camss-
>>> >csid_top_base) {
>>> + camss->csid_top_base =
>>> + devm_platform_ioremap_resource_byname(pdev, res-
>>> >reg[1]);
>>
>> That's a complex clause.
>>
>> Let me send you a patch to do it a different way.
>>
>
> I was also thinking to addd it in camss level, then I thought it is in
> csid block, so I moved it to csid, but it is also fine to add it in
> camss. Can I add your patch into this series? Just like the csiphy patches.
static const struct resources_wrapper csid_wrapper_res_sm8550 = {
.reg = "csid_wrapper",
};
Yes go ahead, all you should need to do then is add
"&csid_wrapper_res_sm8550" to your resources.
static const struct camss_resources sm8550_resources = {
.version = CAMSS_SM8550,
.pd_name = "top",
.csiphy_res = csiphy_res_sm8550,
.csid_res = csid_res_sm8550,
.ispif_res = NULL,
.vfe_res = vfe_res_sm8550,
.csid_wrapper_res = &csid_wrapper_res_sm8550,
...
};
---
bod
On 12/08/2024 15:41, Depeng Shao wrote:
> +
> +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)) {
> + __csid_configure_top(csid);
> + __csid_configure_rdi_stream(csid, enable, i);
> + __csid_configure_rx(csid, &csid->phy, i);
> + __csid_ctrl_rdi(csid, enable, i);
> + }
> +}
Just noticed this too.
You're configuring the CSID routing here for each enabled VC but, you
should only do that once @ the top level
->
__csid_configure_top(csid);
/* 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)) {
__csid_configure_rdi_stream(csid, enable, i);
__csid_configure_rx(csid, &csid->phy, i);
__csid_ctrl_rdi(csid, enable, i);
}
---
bod
Hi Bryan,
On 8/16/2024 7:34 PM, Bryan O'Donoghue wrote:
>
> Just noticed this too.
>
> You're configuring the CSID routing here for each enabled VC but, you
> should only do that once @ the top level
>
> ->
>
>
> __csid_configure_top(csid);
>
> /* 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)) {
> __csid_configure_rdi_stream(csid, enable, i);
> __csid_configure_rx(csid, &csid->phy, i);
> __csid_ctrl_rdi(csid, enable, i);
> }
>
Yes, you are right, will move configure_top out of the for loop.
Thanks,
Depeng
On 12/08/2024 15:41, Depeng Shao wrote:
> +#define CSID_RDI_CFG1(rdi) (0x510 + 0x100 * (rdi))
> +#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_PIX_STORE 10
Hmm - is bit 10 valid ? I'm looking at a register set derived from 8550
and don't see it
> +#define RDI_CFG1_PACKING_FORMAT 15
Bit 15 selects either BIT(15) = 0 PACKING_FORMAT_PLAIN or BIT(15) = 1
PACKING_FORMAT_MIPI
Please give this bit a more descriptive name =>
#define RDI_CFG1_PACKING_FORMAT_MIPI 15
---
bod
On 12/08/2024 15:41, Depeng Shao wrote:
> + for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
> + if (csid->phy.en_vc & BIT(i)) {
> + writel(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> + csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> + writel(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
IRQ_CMD_CLEAR is for the CSID block not per RDI.
I think you need to move the write outside of this loop too.
> + writel(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> + csid->base + CSID_BUF_DONE_IRQ_MASK);
> + }
---
bod
On 12/08/2024 15:41, Depeng Shao wrote:
> + writel(1, csid->base + CSID_TOP_IRQ_CLEAR);
> + writel(1, csid->base + CSID_IRQ_CMD);
CSID_IRQ_CMD bit(0) = CMD_CLEAR
> + writel(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(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> + csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> + writel(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
CSID_IRQ_CMD bit(0) = CMD_CLEAR
and again here.
> + writel(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
> + csid->base + CSID_BUF_DONE_IRQ_MASK);
> + }
re: previous comments
1. Please define bits so that'd be
#define CSID_IRQ_CMD_CLEAR BIT(0)
writel(CSID_IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
There's no value in circumscribing the meaning of bitfields in upstream
code, we just make our own lives easier by having self-documenting code.
TL;DR please name your bits - a blanket statement for the series.
2. And as mentioned above, you don't need to execute that clear n times
in a loop. Just do it once at the top of the routine.
---
bod
Hi Bryan,
On 8/16/2024 10:45 PM, Bryan O'Donoghue wrote:
> On 12/08/2024 15:41, Depeng Shao wrote:
>> + for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>> + if (csid->phy.en_vc & BIT(i)) {
>> + writel(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>> + csid->base + CSID_BUF_DONE_IRQ_CLEAR);
>> + writel(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
>
> IRQ_CMD_CLEAR is for the CSID block not per RDI.
>
Sure, I will move IRQ_CMD_CLEAR outside of this loop.
> I think you need to move the write outside of this loop too.
>
>> + writel(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
>> + csid->base + CSID_BUF_DONE_IRQ_MASK);
>> + }
>
Yes, this also can be moved to outside of the loop. I will update them.
Thanks,
Depeng
Hi Bryan,
On 8/16/2024 10:21 PM, Bryan O'Donoghue wrote:
> On 12/08/2024 15:41, Depeng Shao wrote:
>> +#define CSID_RDI_CFG1(rdi) (0x510 + 0x100 * (rdi))
>> +#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_PIX_STORE 10
>
> Hmm - is bit 10 valid ? I'm looking at a register set derived from 8550
> and don't see it
>
The bit10 is valid in sm8550, but it isn't there in sm8750.
>> +#define RDI_CFG1_PACKING_FORMAT 15
>
> Bit 15 selects either BIT(15) = 0 PACKING_FORMAT_PLAIN or BIT(15) = 1
> PACKING_FORMAT_MIPI
>
> Please give this bit a more descriptive name =>
>
> #define RDI_CFG1_PACKING_FORMAT_MIPI 15
>
Sure. I will update it.
Thanks,
Depeng
On 8/12/24 17:41, 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.
>
> The sm8550 also has a new block is named as CSID top, CSID can
> connect to VFE or SFE(Sensor Front End), the connection is controlled
> by CSID top.
>
> 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 | 339 ++++++++++++++++++
> .../platform/qcom/camss/camss-csid-gen3.h | 26 ++
> .../media/platform/qcom/camss/camss-csid.c | 46 ++-
> .../media/platform/qcom/camss/camss-csid.h | 10 +
> drivers/media/platform/qcom/camss/camss.c | 91 +++++
> drivers/media/platform/qcom/camss/camss.h | 2 +
> 7 files changed, 503 insertions(+), 12 deletions(-)
> 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..d96bc126f0a9
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> @@ -0,0 +1,339 @@
> +// 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_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_CLEAR(rdi) (0xF4 + 0x10 * (rdi))
> +#define CSID_CSI2_RDIN_IRQ_SET(rdi) (0xF8 + 0x10 * (rdi))
> +
> +#define CSID_TOP_IRQ_STATUS 0x7C
The list of macros shall be sorted by register offset value.
> +#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_REG_UPDATE_CMD 0x18
> +
> +#define CSID_BUF_DONE_IRQ_STATUS 0x8C
> +#define BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 1 : 14)
> +#define CSID_BUF_DONE_IRQ_MASK 0x90
> +#define CSID_BUF_DONE_IRQ_CLEAR 0x94
> +#define CSID_BUF_DONE_IRQ_SET 0x98
> +
> +#define CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
> +
> +#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_PHY_NUM_SEL 20
> +
> +#define CSID_CSI2_RX_CFG1 0x204
> +#define CSI2_RX_CFG1_ECC_CORRECTION_EN 0
> +#define CSI2_RX_CFG1_VC_MODE 2
> +
> +#define CSID_RDI_CFG0(rdi) (0x500 + 0x100 * (rdi))
> +#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_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_PIX_STORE 10
> +#define RDI_CFG1_PACKING_FORMAT 15
> +
> +#define CSID_RDI_CTRL(rdi) (0x504 + 0x100 * (rdi))
Sorted by register offset please.
> +#define RDI_CTRL_START_CMD 0
> +
> +#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi) (0x548 + 0x100 * (rdi))
> +#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi) (0x54C + 0x100 * (rdi))
> +
> +static inline int reg_update_rdi(struct csid_device *csid, int n)
> +{
> + return BIT(n + 4) + BIT(20 + n);
Taking as unshifted bit BIT(4) is RUP and BIT(20) is AUP, add
corresponding macros for them, then
return (... | ...) << n;
> +}
> +#define REG_UPDATE_RDI reg_update_rdi
> +
--
Best wishes,
Vladimir
Hi Depeng.
On 8/12/24 17:41, 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.
>
> The sm8550 also has a new block is named as CSID top, CSID can
> connect to VFE or SFE(Sensor Front End), the connection is controlled
> by CSID top.
>
> 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>
> @@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct csid_device *csid, s32 value)
>
> tg->enabled = !!value;
>
> - return csid->res->hw_ops->configure_testgen_pattern(csid, value);
> + if (hw_ops->configure_testgen_pattern)
> + return -EOPNOTSUPP;
> + else
> + return hw_ops->configure_testgen_pattern(csid, value);
> }
>
> /*
Here you accedentally break the TPG on all platforms and introduce a NULL
pointer dereference, please fix it.
Any generic/non-sm8550 support related changes like the part of this
one shall be split into a stand-alone generic change aside of the added
SM8550 platform support, it will simplify patch reviews and hunting bugs
like the one above.
--
Best wishes,
Vladimir
Hi Vladimir,
On 9/30/2024 5:23 PM, Vladimir Zapolskiy wrote:
>> @@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct
>> csid_device *csid, s32 value)
>> tg->enabled = !!value;
>> - return csid->res->hw_ops->configure_testgen_pattern(csid, value);
>> + if (hw_ops->configure_testgen_pattern)
>> + return -EOPNOTSUPP;
>> + else
>> + return hw_ops->configure_testgen_pattern(csid, value);
>> }
>> /*
>
> Here you accedentally break the TPG on all platforms and introduce a NULL
> pointer dereference, please fix it.
>
> Any generic/non-sm8550 support related changes like the part of this
> one shall be split into a stand-alone generic change aside of the added
> SM8550 platform support, it will simplify patch reviews and hunting bugs
> like the one above.
>
Thanks for catching this. This is indeed a bug which is introduced by
this patch. And I will follow your suggestion to use a stand-alone
generic change for the TPG part.
Thanks,
Depeng
@@ -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 \
new file mode 100644
@@ -0,0 +1,339 @@
+// 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_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_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_REG_UPDATE_CMD 0x18
+
+#define CSID_BUF_DONE_IRQ_STATUS 0x8C
+#define BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 1 : 14)
+#define CSID_BUF_DONE_IRQ_MASK 0x90
+#define CSID_BUF_DONE_IRQ_CLEAR 0x94
+#define CSID_BUF_DONE_IRQ_SET 0x98
+
+#define CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
+
+#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_PHY_NUM_SEL 20
+
+#define CSID_CSI2_RX_CFG1 0x204
+#define CSI2_RX_CFG1_ECC_CORRECTION_EN 0
+#define CSI2_RX_CFG1_VC_MODE 2
+
+#define CSID_RDI_CFG0(rdi) (0x500 + 0x100 * (rdi))
+#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_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_PIX_STORE 10
+#define RDI_CFG1_PACKING_FORMAT 15
+
+#define CSID_RDI_CTRL(rdi) (0x504 + 0x100 * (rdi))
+#define RDI_CTRL_START_CMD 0
+
+#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi) (0x548 + 0x100 * (rdi))
+#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi) (0x54C + 0x100 * (rdi))
+
+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(val, csid->base + CSID_CSI2_RX_CFG0);
+
+ val = 1 << CSI2_RX_CFG1_ECC_CORRECTION_EN;
+ if (vc > 3)
+ val |= 1 << CSI2_RX_CFG1_VC_MODE;
+ writel(val, csid->base + CSID_CSI2_RX_CFG1);
+}
+
+static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8 rdi)
+{
+ int val = 0;
+
+ if (enable)
+ val = 1 << RDI_CTRL_START_CMD;
+
+ writel(val, csid->base + CSID_RDI_CTRL(rdi));
+}
+
+static void __csid_configure_top(struct csid_device *csid)
+{
+ u32 val;
+
+ /* csid lite doesn't need to configure top register */
+ if (csid->res->is_lite)
+ return;
+
+ /* CSID top is a new function in Titan780.
+ * CSID can connect to VFE & SFE(Sensor Front End).
+ * This connection is controlled by CSID top.
+ * Only enable VFE path in current driver.
+ */
+ val = OUTPUT_IFE_EN | INTERNAL_CSID;
+ writel(val, csid->camss->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 |= 1 << 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(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;
+
+ writel(val, csid->base + CSID_RDI_CFG1(vc));
+
+ val = 0;
+ writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
+
+ val = 1;
+ writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
+
+ val = 0;
+ writel(val, csid->base + CSID_RDI_CTRL(vc));
+
+ val = readl(csid->base + CSID_RDI_CFG0(vc));
+
+ if (enable)
+ val |= 1 << RDI_CFG0_EN;
+ writel(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)) {
+ __csid_configure_top(csid);
+ __csid_configure_rdi_stream(csid, enable, i);
+ __csid_configure_rx(csid, &csid->phy, i);
+ __csid_ctrl_rdi(csid, enable, i);
+ }
+}
+
+/*
+ * 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(csid->base + CSID_TOP_IRQ_STATUS);
+ writel(val, csid->base + CSID_TOP_IRQ_CLEAR);
+ reset_done = val & BIT(TOP_IRQ_STATUS_RESET_DONE);
+
+ val = readl(csid->base + CSID_CSI2_RX_IRQ_STATUS);
+ writel(val, csid->base + CSID_CSI2_RX_IRQ_CLEAR);
+
+ buf_done_val = readl(csid->base + CSID_BUF_DONE_IRQ_STATUS);
+ writel(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(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
+ writel(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
+
+ if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i)) {
+ /* For Titan 780, Buf Done IRQ® has been moved to CSID from VFE.
+ * Once CSID received Buf Done, need notify this event to VFE.
+ * Trigger VFE to handle Buf Done process.
+ */
+ camss_buf_done(csid->camss, csid->id, i);
+ }
+ }
+
+ val = 1 << IRQ_CMD_CLEAR;
+ writel(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(1, csid->base + CSID_TOP_IRQ_CLEAR);
+ writel(1, csid->base + CSID_IRQ_CMD);
+ writel(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(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+ csid->base + CSID_BUF_DONE_IRQ_CLEAR);
+ writel(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
+ writel(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(val, csid->base + CSID_RST_CFG);
+
+ val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
+ writel(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 void csid_subdev_reg_update(struct csid_device *csid, int port_id, bool is_clear)
+{
+ if (is_clear) {
+ csid->reg_update &= ~REG_UPDATE_RDI(csid, port_id);
+ } else {
+ csid->reg_update |= REG_UPDATE_RDI(csid, port_id);
+ writel(csid->reg_update, csid->base + CSID_REG_UPDATE_CMD);
+ }
+}
+
+static void csid_subdev_init(struct csid_device *csid)
+{
+ /* nop */
+}
+
+const struct csid_hw_ops csid_ops_gen3 = {
+ /* No testgen pattern hw in csid gen3 HW */
+ .configure_testgen_pattern = NULL,
+ .configure_stream = csid_configure_stream,
+ .hw_version = csid_hw_version,
+ .isr = csid_isr,
+ .reset = csid_reset,
+ .src_pad_code = csid_src_pad_code,
+ .subdev_init = csid_subdev_init,
+ .reg_update = csid_subdev_reg_update,
+};
new file mode 100644
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * camss-csid-gen3.h
+ *
+ * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module Generation 3
+ *
+ * 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 */
@@ -838,7 +838,7 @@ static void csid_try_format(struct csid_device *csid,
break;
case MSM_CSID_PAD_SRC:
- if (csid->testgen_mode->cur.val == 0) {
+ if (!csid->testgen_mode || csid->testgen_mode->cur.val == 0) {
/* Test generator is disabled, */
/* keep pad formats in sync */
u32 code = fmt->code;
@@ -1042,6 +1042,7 @@ static int csid_init_formats(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
static int csid_set_test_pattern(struct csid_device *csid, s32 value)
{
struct csid_testgen_config *tg = &csid->testgen;
+ const struct csid_hw_ops *hw_ops = csid->res->hw_ops;
/* If CSID is linked to CSIPHY, do not allow to enable test generator */
if (value && media_pad_remote_pad_first(&csid->pads[MSM_CSID_PAD_SINK]))
@@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct csid_device *csid, s32 value)
tg->enabled = !!value;
- return csid->res->hw_ops->configure_testgen_pattern(csid, value);
+ if (hw_ops->configure_testgen_pattern)
+ return -EOPNOTSUPP;
+ else
+ return hw_ops->configure_testgen_pattern(csid, value);
}
/*
@@ -1121,6 +1125,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
if (IS_ERR(csid->base))
return PTR_ERR(csid->base);
+
+ /* CSID "top" is a new function in new version HW,
+ * CSID can connect to VFE & SFE(Sensor Front End).
+ * this connection is controlled by CSID "top" registers.
+ * There is only one CSID "top" region for all CSIDs.
+ */
+ if (!csid_is_lite(csid) && res->reg[1] && !camss->csid_top_base) {
+ camss->csid_top_base =
+ devm_platform_ioremap_resource_byname(pdev, res->reg[1]);
+
+ if (IS_ERR(camss->csid_top_base))
+ return PTR_ERR(camss->csid_top_base);
+ }
}
/* Interrupt */
@@ -1267,7 +1284,7 @@ static int csid_link_setup(struct media_entity *entity,
/* If test generator is enabled */
/* do not allow a link from CSIPHY to CSID */
- if (csid->testgen_mode->cur.val != 0)
+ if (csid->testgen_mode && csid->testgen_mode->cur.val != 0)
return -EBUSY;
sd = media_entity_to_v4l2_subdev(remote->entity);
@@ -1366,15 +1383,20 @@ int msm_csid_register_entity(struct csid_device *csid,
return ret;
}
- csid->testgen_mode = v4l2_ctrl_new_std_menu_items(&csid->ctrls,
- &csid_ctrl_ops, V4L2_CID_TEST_PATTERN,
- csid->testgen.nmodes, 0, 0,
- csid->testgen.modes);
-
- if (csid->ctrls.error) {
- dev_err(dev, "Failed to init ctrl: %d\n", csid->ctrls.error);
- ret = csid->ctrls.error;
- goto free_ctrl;
+ if (csid->res->hw_ops->configure_testgen_pattern) {
+ csid->testgen_mode =
+ v4l2_ctrl_new_std_menu_items(&csid->ctrls,
+ &csid_ctrl_ops,
+ V4L2_CID_TEST_PATTERN,
+ csid->testgen.nmodes, 0,
+ 0, csid->testgen.modes);
+
+ if (csid->ctrls.error) {
+ dev_err(dev, "Failed to init ctrl: %d\n",
+ csid->ctrls.error);
+ ret = csid->ctrls.error;
+ goto free_ctrl;
+ }
}
csid->subdev.ctrl_handler = &csid->ctrls;
@@ -152,6 +152,14 @@ struct csid_hw_ops {
* @csid: CSID device
*/
void (*subdev_init)(struct csid_device *csid);
+
+ /*
+ * reg_update - receive message from other sub device
+ * @csid: CSID device
+ * @port_id: Port id
+ * @is_clear: Indicate if it is clearing reg update or setting reg update
+ */
+ void (*reg_update)(struct csid_device *csid, int port_id, bool is_clear);
};
struct csid_subdev_resources {
@@ -168,6 +176,7 @@ struct csid_device {
struct media_pad pads[MSM_CSID_PADS_NUM];
void __iomem *base;
u32 irq;
+ u32 reg_update;
char irq_name[30];
struct camss_clock *clock;
int nclocks;
@@ -228,6 +237,7 @@ extern const struct csid_formats csid_formats_gen2;
extern const struct csid_hw_ops csid_ops_4_1;
extern const struct csid_hw_ops csid_ops_4_7;
extern const struct csid_hw_ops csid_ops_gen2;
+extern const struct csid_hw_ops csid_ops_gen3;
/*
* csid_is_lite - Check if CSID is CSID lite.
@@ -1588,6 +1588,84 @@ static const struct camss_subdev_resources csiphy_res_8550[] = {
}
};
+static const struct camss_subdev_resources csid_res_8550[] = {
+ /* CSID0 */
+ {
+ .regulators = { "vdda-phy", "vdda-pll" },
+ .clock = { "csid", "csiphy_rx" },
+ .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
+ { 400000000, 480000000, 480000000, 480000000, 480000000 } },
+ .reg = { "csid0", "csid_top" },
+ .interrupt = { "csid0" },
+ .csid = {
+ .is_lite = false,
+ .parent_dev_ops = &vfe_parent_dev_ops,
+ .hw_ops = &csid_ops_gen3,
+ .formats = &csid_formats_gen2
+ }
+ },
+ /* CSID1 */
+ {
+ .regulators = { "vdda-phy", "vdda-pll" },
+ .clock = { "csid", "csiphy_rx" },
+ .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
+ { 400000000, 480000000, 480000000, 480000000, 480000000 } },
+ .reg = { "csid1", "csid_top" },
+ .interrupt = { "csid1" },
+ .csid = {
+ .is_lite = false,
+ .parent_dev_ops = &vfe_parent_dev_ops,
+ .hw_ops = &csid_ops_gen3,
+ .formats = &csid_formats_gen2
+ }
+ },
+ /* CSID2 */
+ {
+ .regulators = { "vdda-phy", "vdda-pll" },
+ .clock = { "csid", "csiphy_rx" },
+ .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
+ { 400000000, 480000000, 480000000, 480000000, 480000000 } },
+ .reg = { "csid2", "csid_top" },
+ .interrupt = { "csid2" },
+ .csid = {
+ .is_lite = false,
+ .parent_dev_ops = &vfe_parent_dev_ops,
+ .hw_ops = &csid_ops_gen3,
+ .formats = &csid_formats_gen2
+ }
+ },
+ /* CSID3 */
+ {
+ .regulators = { "vdda-phy", "vdda-pll" },
+ .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx" },
+ .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
+ { 400000000, 480000000, 480000000, 480000000, 480000000 } },
+ .reg = { "csid_lite0" },
+ .interrupt = { "csid_lite0" },
+ .csid = {
+ .is_lite = true,
+ .parent_dev_ops = &vfe_parent_dev_ops,
+ .hw_ops = &csid_ops_gen3,
+ .formats = &csid_formats_gen2
+ }
+ },
+ /* CSID4 */
+ {
+ .regulators = { "vdda-phy", "vdda-pll" },
+ .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx" },
+ .clock_rate = { { 400000000, 480000000, 480000000, 480000000, 480000000 },
+ { 400000000, 480000000, 480000000, 480000000, 480000000 } },
+ .reg = { "csid_lite1" },
+ .interrupt = { "csid_lite1" },
+ .csid = {
+ .is_lite = true,
+ .parent_dev_ops = &vfe_parent_dev_ops,
+ .hw_ops = &csid_ops_gen3,
+ .formats = &csid_formats_gen2
+ }
+ }
+};
+
static const struct resources_icc icc_res_sm8550[] = {
{
.name = "ahb",
@@ -1768,6 +1846,17 @@ void camss_pm_domain_off(struct camss *camss, int id)
}
}
+void camss_buf_done(struct camss *camss, int hw_id, int port_id)
+{
+ struct vfe_device *vfe;
+
+ if (hw_id < camss->res->vfe_num) {
+ vfe = &(camss->vfe[hw_id]);
+
+ vfe->res->hw_ops->vfe_buf_done(vfe, port_id);
+ }
+}
+
static int vfe_parent_dev_ops_get(struct camss *camss, int id)
{
int ret = -EINVAL;
@@ -2578,9 +2667,11 @@ 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
};
@@ -117,6 +117,7 @@ struct camss {
struct device_link *genpd_link;
struct icc_path *icc_path[ICC_SM8250_COUNT];
const struct camss_resources *res;
+ void __iomem *csid_top_base;
};
struct camss_camera_interface {
@@ -155,5 +156,6 @@ void camss_pm_domain_off(struct camss *camss, int id);
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);
#endif /* QC_MSM_CAMSS_H */