Re: [PATCH 09/13] media: qcom: camss: Add CSID Gen3 support for SM8550
From: Depeng Shao
Date: Thu Jul 11 2024 - 11:34:23 EST
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@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
+
+#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