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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.


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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux