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

[Depeng Shao <quic_depengs@quicinc.com>]

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