Re: [PATCH 09/13] media: qcom: camss: Add CSID Gen3 support for SM8550
From: Krzysztof Kozlowski
Date: Wed Jul 10 2024 - 07:21:30 EST
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@xxxxxxxxxxx>
> Signed-off-by: Yongsheng Li <quic_yon@xxxxxxxxxxx>
> Signed-off-by: Depeng Shao <quic_depengs@xxxxxxxxxxx>
> ---
> 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