[PATCH v3 7/9] media: qcom: camss: Add support for CSID for sa8775p
Bryan O'Donoghue
bryan.odonoghue at linaro.org
Mon Jul 28 07:02:11 PDT 2025
On 03/07/2025 18:19, Vikram Sharma wrote:
> The CSID in sa8775p is version 690, This csid is different from
> csid 780 w.r.t few bit-fields.
>
> Co-developed-by: Suresh Vankadara <quic_svankada at quicinc.com>
> Signed-off-by: Suresh Vankadara <quic_svankada at quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa at quicinc.com>
> ---
> .../platform/qcom/camss/camss-csid-gen3.c | 31 +++-
> drivers/media/platform/qcom/camss/camss.c | 151 ++++++++++++++++++
> 2 files changed, 175 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> index 0941152ec301..f62084fb8287 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> @@ -47,8 +47,12 @@
> #define CSID_CSI2_RX_IRQ_CLEAR 0xA4
> #define CSID_CSI2_RX_IRQ_SET 0xA8
>
> +#define IS_CSID_690(csid) (csid->camss->res->version ==\
> + CAMSS_8775P ? true : false)
You don't need to make this into a bool.
version == CAMSS_8775P will do exactly the same logical thing.
> #define CSID_BUF_DONE_IRQ_STATUS 0x8C
> -#define BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 1 : 14)
> +#define BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ?\
> + 1 : (IS_CSID_690(csid) ?\
> + 13 : 14))
> #define CSID_BUF_DONE_IRQ_MASK 0x90
> #define CSID_BUF_DONE_IRQ_CLEAR 0x94
> #define CSID_BUF_DONE_IRQ_SET 0x98
> @@ -61,6 +65,7 @@
>
> #define CSID_CSI2_RX_CFG0 0x200
> #define CSI2_RX_CFG0_NUM_ACTIVE_LANES 0
> +#define CSI2_RX_CFG0_VC_MODE 3
> #define CSI2_RX_CFG0_DL0_INPUT_SEL 4
> #define CSI2_RX_CFG0_PHY_NUM_SEL 20
>
> @@ -68,7 +73,9 @@
> #define CSI2_RX_CFG1_ECC_CORRECTION_EN BIT(0)
> #define CSI2_RX_CFG1_VC_MODE BIT(2)
>
> -#define CSID_RDI_CFG0(rdi) (0x500 + 0x100 * (rdi))
> +#define CSID_RDI_CFG0(rdi) (csid_is_lite(csid) && IS_CSID_690(csid) ?\
> + (0x300 + 0x100 * (rdi)) :\
> + (0x500 + 0x100 * (rdi)))
> #define RDI_CFG0_TIMESTAMP_EN BIT(6)
> #define RDI_CFG0_TIMESTAMP_STB_SEL BIT(8)
> #define RDI_CFG0_DECODE_FORMAT 12
> @@ -77,10 +84,14 @@
> #define RDI_CFG0_DT_ID 27
> #define RDI_CFG0_EN BIT(31)
>
> -#define CSID_RDI_CTRL(rdi) (0x504 + 0x100 * (rdi))
> +#define CSID_RDI_CTRL(rdi) (csid_is_lite(csid) && IS_CSID_690(csid) ?\
> + (0x304 + 0x100 * (rdi)) :\
> + (0x504 + 0x100 * (rdi)))
> #define RDI_CTRL_START_CMD BIT(0)
>
> -#define CSID_RDI_CFG1(rdi) (0x510 + 0x100 * (rdi))
> +#define CSID_RDI_CFG1(rdi) (csid_is_lite(csid) && IS_CSID_690(csid) ?\
> + (0x310 + 0x100 * (rdi)) :\
> + (0x510 + 0x100 * (rdi)))
> #define RDI_CFG1_DROP_H_EN BIT(5)
> #define RDI_CFG1_DROP_V_EN BIT(6)
> #define RDI_CFG1_CROP_H_EN BIT(7)
> @@ -88,9 +99,12 @@
> #define RDI_CFG1_PIX_STORE BIT(10)
> #define RDI_CFG1_PACKING_FORMAT_MIPI BIT(15)
>
> -#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi) (0x548 + 0x100 * (rdi))
> -#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi) (0x54C + 0x100 * (rdi))
> -
> +#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi) (csid_is_lite(csid) && IS_CSID_690(csid) ?\
> + (0x348 + 0x100 * (rdi)) :\
> + (0x548 + 0x100 * (rdi)))
> +#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi) (csid_is_lite(csid) && IS_CSID_690(csid) ?\
> + (0x34C + 0x100 * (rdi)) :\
> + (0x54C + 0x100 * (rdi)))
> #define CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
>
> static void __csid_configure_rx(struct csid_device *csid,
> @@ -102,6 +116,9 @@ static void __csid_configure_rx(struct csid_device *csid,
> 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;
>
> + if (IS_CSID_690(csid) && (vc > 3))
> + val |= 1 << CSI2_RX_CFG0_VC_MODE;
> +
I remeber that the > 3 has a purpose but, I don't remember of off the
top of my head what that is.
Please add a comment to state why "> 3" matters.
BTW should this be "> 3" or ">= 3" please double check.
> writel(val, csid->base + CSID_CSI2_RX_CFG0);
>
> val = CSI2_RX_CFG1_ECC_CORRECTION_EN;
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index ebc3b296bb50..b2398196b9ff 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2269,6 +2269,10 @@ static const struct camss_subdev_resources csiphy_res_8550[] = {
> }
> };
>
> +static const struct resources_wrapper csid_wrapper_res_sa8775p = {
> + .reg = "csid_wrapper",
> +};
> +
> static const struct resources_wrapper csid_wrapper_res_sm8550 = {
> .reg = "csid_wrapper",
> };
Redefining the same string in separate structures over and over again
seems wasteful.
Please take the opporunity of this new addition to rationalise down the
string declarations and .. if possible the number of struct
resources_wrappers we are doing here.
> @@ -2558,6 +2562,153 @@ static const struct camss_subdev_resources csiphy_res_8775p[] = {
> },
> };
>
> +static const struct camss_subdev_resources csid_res_8775p[] = {
> + /* CSID0 */
> + {
> + .regulators = {},
> +
Zap the newlines.
> + .clock = { "csid", "csiphy_rx"},
> + .clock_rate = {
> + { 400000000, 400000000},
> + { 400000000, 400000000}
> + },
> +
> + .reg = { "csid0" },
> + .interrupt = { "csid0" },
> + .csid = {
> + .is_lite = false,
> + .hw_ops = &csid_ops_gen3,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID1 */
> + {
> + .regulators = {},
> +
> + .clock = { "csid", "csiphy_rx"},
> + .clock_rate = {
> + { 400000000, 400000000},
> + { 400000000, 400000000}
> + },
> +
> + .reg = { "csid1" },
> + .interrupt = { "csid1" },
> + .csid = {
> + .is_lite = false,
> + .hw_ops = &csid_ops_gen3,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .formats = &csid_formats_gen2
> + }
> + },
> +
> + /* CSID2 (lite) */
> + {
> + .regulators = {},
> +
> + .clock = { "cpas_vfe_lite", "vfe_lite_ahb",
> + "vfe_lite_csid", "vfe_lite_cphy_rx",
> + "vfe_lite"},
> + .clock_rate = {
> + { 0, 0, 400000000, 400000000, 0},
> + { 0, 0, 400000000, 480000000, 0}
> + },
> +
> + .reg = { "csid_lite0" },
> + .interrupt = { "csid_lite0" },
> + .csid = {
> + .is_lite = true,
> + .hw_ops = &csid_ops_gen3,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID3 (lite) */
> + {
> + .regulators = {},
> +
> + .clock = { "cpas_vfe_lite", "vfe_lite_ahb",
> + "vfe_lite_csid", "vfe_lite_cphy_rx",
> + "vfe_lite"},
> + .clock_rate = {
> + { 0, 0, 400000000, 400000000, 0},
> + { 0, 0, 400000000, 480000000, 0}
> + },
> +
> + .reg = { "csid_lite1" },
> + .interrupt = { "csid_lite1" },
> + .csid = {
> + .is_lite = true,
> + .hw_ops = &csid_ops_gen3,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID4 (lite) */
> + {
> + .regulators = {},
> +
> + .clock = { "cpas_vfe_lite", "vfe_lite_ahb",
> + "vfe_lite_csid", "vfe_lite_cphy_rx",
> + "vfe_lite"},
> + .clock_rate = {
> + { 0, 0, 400000000, 400000000, 0},
> + { 0, 0, 400000000, 480000000, 0}
> + },
> +
> + .reg = { "csid_lite2" },
> + .interrupt = { "csid_lite2" },
> + .csid = {
> + .is_lite = true,
> + .hw_ops = &csid_ops_gen3,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID5 (lite) */
> + {
> + .regulators = {},
> +
> + .clock = { "cpas_vfe_lite", "vfe_lite_ahb",
> + "vfe_lite_csid", "vfe_lite_cphy_rx",
> + "vfe_lite"},
> + .clock_rate = {
> + { 0, 0, 400000000, 400000000, 0},
> + { 0, 0, 400000000, 480000000, 0}
> + },
> +
> + .reg = { "csid_lite3" },
> + .interrupt = { "csid_lite3" },
> + .csid = {
> + .is_lite = true,
> + .hw_ops = &csid_ops_gen3,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .formats = &csid_formats_gen2
> + }
> + },
> + /* CSID6 (lite) */
> + {
> + .regulators = {},
> +
> + .clock = { "cpas_vfe_lite", "vfe_lite_ahb",
> + "vfe_lite_csid", "vfe_lite_cphy_rx",
> + "vfe_lite"},
> + .clock_rate = {
> + { 0, 0, 400000000, 400000000, 0},
> + { 0, 0, 400000000, 480000000, 0}
> + },
> +
> + .reg = { "csid_lite4" },
> + .interrupt = { "csid_lite4" },
> + .csid = {
> + .is_lite = true,
> + .hw_ops = &csid_ops_gen3,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> + .formats = &csid_formats_gen2
> + }
> + },
> +};
> +
> static const struct resources_icc icc_res_sa8775p[] = {
> {
> .name = "ahb",
---
bod
More information about the linux-arm-kernel
mailing list