| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4c8095dd-4f96-4b0e-9282-8bdfb5badbc3@quicinc.com>
Date: Thu, 11 Jul 2024 19:08:54 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, <rfoss@...nel.org>,
<todor.too@...il.com>, <bryan.odonoghue@...aro.org>,
<mchehab@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>
CC: <quic_eberman@...cinc.com>, <linux-media@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <kernel@...cinc.com>,
Yongsheng Li
<quic_yon@...cinc.com>
Subject: Re: [PATCH 09/13] media: qcom: camss: Add CSID Gen3 support for
SM8550
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@...cinc.com>
>> Signed-off-by: Yongsheng Li <quic_yon@...cinc.com>
>> Signed-off-by: Depeng Shao <quic_depengs@...cinc.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
Powered by blists - more mailing lists