| 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: <eeaf4f4e-5200-4b13-b38f-3f3385fc2a2b@quicinc.com>
Date: Thu, 11 Jul 2024 23:33:56 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
<todor.too@...il.com>, <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 Bryan,
On 7/10/2024 7:28 PM, Bryan O'Donoghue wrote:
> On 09/07/2024 17: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
>>
>> +
>> +#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_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
>> +
>> + val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
>> + if (vc > 3)
>> + val |= 1 << CSI2_RX_CFG1_VC_MODE;
>
> I realise downstream does these shifts but, I think in upstream we
> should just define a BIT(x)
>
> #define CSI2_RX_CFG1_VC_MODE BIT(2)
>
> val |= CSI2_RX_CFG1_VC_MODE;
>
Here CSI2_RX_CFG1_VC_MODE just means a register bit offset, not a
register configuration.
Some fixed configuration can do this, but some register bits value are
configured based on actual parameter.
e.g.; val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
If we want to use macro definition, maybe we can do like below.
#define CSI2_RX_CFG1_VC_MODE(n) ((n) << 2)
val |= CSI2_RX_CFG1_VC_MODE(1);
#define CSI2_RX_CFG0_DL0_INPUT_SEL(n) ((n) << 4)
val |= CSI2_RX_CFG0_DL0_INPUT_SEL(phy->lane_assign)
Could you please comment if we really need to do like this?
>> + writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
>> +}
>> +
>> +static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8
>> rdi)
>> +{
>> + int val;
>> +
>> + if (enable)
>> + val = 1 << RDI_CTRL_START_CMD;
>> + else
>> + val = 0 << RDI_CTRL_START_CMD;
>
> Here is an example of how the bit shifting is weird
>
> (0 << anything) is still zero
>
Understood, the value is same, but we can know the configuration clearly
on this register bit. If we do like above way, then it likes below.
#define RDI_CTRL_START_CMD(n) ((n) << 0) --> it is same with (n), but
we don't know the register bit offset clearly if we use (n).
if (enable)
val = RDI_CTRL_START_CMD(1);
else
val = RDI_CTRL_START_CMD(0);
>> + writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
>> +}
>> +
>> +static void __csid_configure_top(struct csid_device *csid)
>> +{
>> + u32 val;
>> +
>> + /* CSID "top" is a new function in Titan780.
>> + * CSID can connect to VFE & SFE(Sensor Front End).
>> + * This connection is ontrolled by CSID "top".
>> + * Only enable VFE path in current driver.
>> + */
>> + if (csid->top_base) {
>
> When is csid->top_base NULL ?
>
> Its required in your yaml.
>
csid->top_base is NULL when it is csid lite, I will add this info in yaml.
>> +
>> +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)) {
>> + /* Configure CSID "top" */
>> + __csid_configure_top(csid);
>> + __csid_configure_rdi_stream(csid, enable, i);
>> + __csid_configure_rx(csid, &csid->phy, i);
>> + __csid_ctrl_rdi(csid, enable, i);
>> + }
>> +}
>
> I really like this breakdown
Sorry, I don't get it, do you mean you like that configuring the
different block use different functions, and no other meaning?
>> +
>> +static int csid_configure_testgen_pattern(struct csid_device *csid,
>> s32 val)
>> +{
>> + if (val > 0 && val <= csid->testgen.nmodes)
>> + csid->testgen.mode = val;
>
> Surely you should just set the val parameter directly ?
>
> Also why is this a signed integer and how does it get assigned a
> negative value which we need to mitigate against here >
This was copied from csid-gen2 driver, they are same, so we can move to
common file.
And the val comes from ctrl->val, so I guess this is the reason why this
agrument is signed integer.
struct v4l2_ctrl {
...
s32 val;
...
};
>> +
>> +static u32 csid_src_pad_code(struct csid_device *csid, u32 sink_code,
>> + unsigned int match_format_idx, u32 match_code)
>> +{
>> + switch (sink_code) {
>> + case MEDIA_BUS_FMT_SBGGR10_1X10:
>> + {
>> + u32 src_code[] = {
>> + MEDIA_BUS_FMT_SBGGR10_1X10,
>> + MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE,
>> + };
>> +
>> + return csid_find_code(src_code, ARRAY_SIZE(src_code),
>> + match_format_idx, match_code);
>> + }
>> + case MEDIA_BUS_FMT_Y10_1X10:
>> + {
>> + u32 src_code[] = {
>> + MEDIA_BUS_FMT_Y10_1X10,
>> + MEDIA_BUS_FMT_Y10_2X8_PADHI_LE,
>> + };
>> +
>> + return csid_find_code(src_code, ARRAY_SIZE(src_code),
>> + match_format_idx, match_code);
>> + }
>> + default:
>> + if (match_format_idx > 0)
>> + return 0;
>> +
>> + return sink_code;
>> + }
>> +}
>
> Same code as in gen2.
>
> You could move the gen2 version of this into camss-csid.c and just reuse
> in both.
>
Sure, it is same with the comments in vfe driver, I will try to move
same code to camss-csid.c
Thanks,
Depeng
Powered by blists - more mailing lists