Re: [PATCH 6/6] media: qcom: camss: support for camss driver for sc7280

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 28/06/2024 19:32, Vikram Sharma wrote:
> From: Suresh Vankadara <quic_svankada@quicinc.com>
> 
> This change adds support for camss driver for sc7280 soc.
> 
> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/camss-csid.c     |  16 +-
>   .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     |   2 +
>   drivers/media/platform/qcom/camss/camss-vfe.c      |   2 +
>   drivers/media/platform/qcom/camss/camss.c          | 340 +++++++++++++++++++++
>   drivers/media/platform/qcom/camss/camss.h          |   2 +
>   5 files changed, 359 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 858db5d4ca75..2c622233da6f 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -28,6 +28,7 @@
>   /* offset of CSID registers in VFE region for VFE 480 */
>   #define VFE_480_CSID_OFFSET 0x1200
>   #define VFE_480_LITE_CSID_OFFSET 0x200
> +#define VFE_165_CSID_OFFSET 0x4000
>   
>   #define MSM_CSID_NAME "msm_csid"
>   
> @@ -1028,8 +1029,8 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   	csid->res->hw_ops->subdev_init(csid);
>   
>   	/* Memory */
> -
> -	if (camss->res->version == CAMSS_8250) {
> +	switch (camss->res->version) {
> +	case CAMSS_8250:
>   		/* for titan 480, CSID registers are inside the VFE region,
>   		 * between the VFE "top" and "bus" registers. this requires
>   		 * VFE to be initialized before CSID
> @@ -1040,10 +1041,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   		else
>   			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
>   				 + VFE_480_CSID_OFFSET;
> -	} else {
> +		break;
> +	case CAMSS_7280:
> +		/* for titan 165, CSID registers are inside the VFE region,
> +		 * between the VFE "top" and "bus" registers. this requires
> +		 * VFE to be initialized before CSID
> +		 */
> +		csid->base = camss->vfe[id].base + VFE_165_CSID_OFFSET;


Right but you can just define "csid" registers in your yaml and dts per 
standard definitions.

Looking at what we did for 8250 here there's absolutely no good reason 
to have C code derive offsets like this which can be described in dts.

I'll send a patch to that effect - along with named power-domains for 8250.

Please just define your CSID registers in the yaml/dts - there's no need 
to add executable code to the driver to find an offset.

> +		break;
> +	default:
>   		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
>   		if (IS_ERR(csid->base))
>   			return PTR_ERR(csid->base);
> +		break;
>   	}
>   
>   	/* Interrupt */
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index df7e93a5a4f6..c7e507420732 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -510,6 +510,7 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>   		array_size = ARRAY_SIZE(lane_regs_sdm845[0]);
>   		break;
>   	case CAMSS_8250:
> +	case CAMSS_7280:
>   		r = &lane_regs_sm8250[0][0];
>   		array_size = ARRAY_SIZE(lane_regs_sm8250[0]);
>   		break;
> @@ -560,6 +561,7 @@ static bool csiphy_is_gen2(u32 version)
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_7280:

Sort alphanumerically please.


> +	/* CSIPHY0 */
> +	{
> +		.regulators = {},
> +		.clock = { "csiphy0", "csiphy0_timer", "csiphy0_timer_src"},
> +		.clock_rate = { { 300000000 },
> +				{ 300000000 },
> +				{ 300000000 }},

I'll reiterate, I don't believe the _src clocks are required.


> +
> +static const struct resources_icc icc_res_sc7280[] = {
> +	{
> +		.name = "cam_ahb",
> +		.icc_bw_tbl.avg = 38400,
> +		.icc_bw_tbl.peak = 76800,
> +	},
> +	{
> +		.name = "cam_hf_0",
> +		.icc_bw_tbl.avg = 2097152,
> +		.icc_bw_tbl.peak = 2097152,
> +	},
> +};

Good to see this.

> +
>   /*
>    * camss_add_clock_margin - Add margin to clock frequency rate
>    * @rate: Clock frequency rate
> @@ -1824,6 +2099,57 @@ static int camss_init_subdevices(struct camss *camss)
>   	return 0;
>   }
>   
> +/*
> + * camss_link_entities_v2 - Register subdev nodes and create links
> + * @camss: CAMSS device
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int camss_link_entities_v2(struct camss *camss)
> +{
> +	int i, j;
> +	int ret;
> +
> +	for (i = 0; i < camss->res->csiphy_num; i++) {
> +		for (j = 0; j < camss->res->csid_num; j++) {
> +			ret = media_create_pad_link(&camss->csiphy[i].subdev.entity,
> +						    MSM_CSIPHY_PAD_SRC,
> +						    &camss->csid[j].subdev.entity,
> +						    MSM_CSID_PAD_SINK,
> +						    0);
> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					camss->csiphy[i].subdev.entity.name,
> +					camss->csid[j].subdev.entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	for (i = 0; i < camss->res->csid_num; i++)
> +		for (j = 0; j < camss->vfe[i].res->line_num; j++) {
> +			struct v4l2_subdev *csid = &camss->csid[i].subdev;
> +			struct v4l2_subdev *vfe = &camss->vfe[i].line[j].subdev;
> +
> +			ret = media_create_pad_link(&csid->entity,
> +						    MSM_CSID_PAD_FIRST_SRC + j,
> +						    &vfe->entity,
> +						    MSM_VFE_PAD_SINK,
> +						    0);
> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					csid->entity.name,
> +					vfe->entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	return 0;
> +}

So I see what you're doing here and agree but, I think it should be made 
into its own standalone patch.

We can break up the link_entities function into something for ispif the 
v1 and something for everybody else @ v2, not just 7280.

Either way such a change deserves its own standalone patch.

> +
>   /*
>    * camss_link_entities - Register subdev nodes and create links
>    * @camss: CAMSS device
> @@ -2440,12 +2766,26 @@ static const struct camss_resources sc8280xp_resources = {
>   	.link_entities = camss_link_entities
>   };
>   
> +static const struct camss_resources sc7280_resources = {
> +	.version = CAMSS_7280,
> +	.csiphy_res = csiphy_res_7280,
> +	.csid_res = csid_res_7280,
> +	.vfe_res = vfe_res_7280,
> +	.icc_res = icc_res_sc7280,
> +	.icc_path_num = ARRAY_SIZE(icc_res_sc7280),
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_7280),
> +	.csid_num = ARRAY_SIZE(csid_res_7280),
> +	.vfe_num = 3,
> +	.link_entities = camss_link_entities_v2
> +};
> +
>   static const struct of_device_id camss_dt_match[] = {
>   	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
>   	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
>   	{ .compatible = "qcom,sdm660-camss", .data = &sdm660_resources },
>   	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
>   	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
> +	{ .compatible = "qcom,sc7280-camss", .data = &sc7280_resources },
>   	{ .compatible = "qcom,sc8280xp-camss", .data = &sc8280xp_resources },

Its just occured to me, this list ought to be sorted alpanumerically too.

I'd be obliged if you could add a patch to this series to sort this list 
prior to adding in your new string - in the appropriate order.

>   	{ }
>   };
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 73c47c07fc30..29dbf93ce9c5 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -79,11 +79,13 @@ enum camss_version {
>   	CAMSS_845,
>   	CAMSS_8250,
>   	CAMSS_8280XP,
> +	CAMSS_7280,
>   };
>   
>   enum icc_count {
>   	ICC_DEFAULT_COUNT = 0,
>   	ICC_SM8250_COUNT = 4,
> +	ICC_SM7280_COUNT = 4,
>   };

Do you even use the SM7280 specific enum ? I didn't see it, SoC name is 
SC7280 anyway.

I think you can drop that.

---
bod