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

[Depeng Shao <quic_depengs@quicinc.com>]

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

>> +
>> +#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