mbox series

[0/6] media: qcom: camss: Add sc7280 support

Message ID 20240629-camss_first_post_linux_next-v1-0-bc798edabc3a@quicinc.com
Headers show
Series media: qcom: camss: Add sc7280 support | expand

Message

Vikram Sharma June 28, 2024, 6:32 p.m. UTC 
SC7280 is a Qualcomm SoC. This series adds support to
bring up the CSIPHY, CSID, VFE/RDI interfaces in SC7280.

SC7280 provides

- 3 x VFE, 3 RDI per VFE
- 2 x VFE Lite, 4 RDI per VFE
- 3 x CSID
- 2 x CSID Lite
- 5 x CSI PHY

This series is rebased based on：
https://lore.kernel.org/linux-arm-msm/20240522154659.510-1-quic_grosikop@quicinc.com/

The changes are verified on SC7280 qcs6490-rb3gen2 board, the base dts for qcs6490-rb3gen2
is:
https://lore.kernel.org/all/20231103184655.23555-1-quic_kbajaj@quicinc.com/

Suresh Vankadara (2):
media: qcom: camss: support for camss driver on sc7280
arm64: dts: qcom: sc7280: Add support for camss

Trishansh Bhardwaj (2):
media: qcom: camss: support for camss driver on sc7280
arm64: dts: qcom: sc7280: Add support for camss

Vikram Sharma (1):
media: dt-bindings: media: camss: Add qcom,sc7280-camss binding

Hariram Purshotam (3):
i2c: Enable IMX577 camera sensor for qcm6490
arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
arm64: dts: qcom: sc7280: Add IMX577 camera sensor

Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
Suresh Vankadara (1):
      media: qcom: camss: support for camss driver for sc7280

Vikram Sharma (5):
      media: dt-bindings: media: camss: Add qcom,sc7280-camss binding
      arm64: dts: qcom: sc7280: Add support for camss
      arm64: dts: qcom: sc7280: Add IMX577 camera sensor
      arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
      i2c: Enable IMX577 camera sensor for qcm6490

 .../bindings/media/qcom,sc7280-camss.yaml          | 477 +++++++++++++++++++++
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |  67 +++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 215 ++++++++++
 drivers/i2c/busses/i2c-qcom-cci.c                  |   1 +
 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 +
 9 files changed, 1119 insertions(+), 3 deletions(-)
---
base-commit: 18eeb2d92baca167809cd5d48eb60e0a5c036d51
change-id: 20240627-camss_first_post_linux_next-f4163c90093c

Best regards,

Comments

Luca Weiss June 29, 2024, 8:22 a.m. UTC | #1 
On Freitag, 28. Juni 2024 20:32:39 MESZ Vikram Sharma wrote:
> This change enables IMX577 sensor driver for qcm6490.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 414882c57d7f..10e6df566ae3 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -817,6 +817,7 @@ static const struct of_device_id cci_dt_match[] = {
>  	 * Do not add any new ones unless they introduce a new config
>  	 */
>  	{ .compatible = "qcom,msm8916-cci", .data = &cci_v1_data},
> +	{ .compatible = "qcom,sc7280-cci", .data = &cci_v2_data},

Please read the comment above qcom,msm8916-cci.

And sc7280.dtsi already uses

  compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";

So qcom,msm8996-cci with the same match data (cci_v2_data) gets used, so
just drop this patch.

Regards
Luca

>  	{ .compatible = "qcom,sdm845-cci", .data = &cci_v2_data},
>  	{ .compatible = "qcom,sm8250-cci", .data = &cci_v2_data},
>  	{ .compatible = "qcom,sm8450-cci", .data = &cci_v2_data},
> 
>
Bryan O'Donoghue June 29, 2024, 11:07 a.m. UTC | #2 
On 28/06/2024 19:32, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>   				bias-pull-down;
>   			};
>   
> +			cam2_default: cam2-default {
> +				rst {
> +					pins = "gpio78"; /*cam3*/

I don't think the /* cam3 */ adds much here TBH.

> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-disable;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>; /*RB5 was 16 and i changed to 2 here*/

You can drop that comment too, actually more saliently, what are you 
changing from 16 to 2 since its being mentioned ?

---
bod
Bryan O'Donoghue June 29, 2024, 11:22 a.m. UTC | #3 
On 28/06/2024 19:32, Vikram Sharma wrote:
> Enable IMX577 camera sensor for qcs6490-rb3gen2.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 67 ++++++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index c4cde4328e3d..237231600dca 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts

I believe the rb3gen2 can be sold with and without the camera mezzanine 
[1], so the approach we have taken for a similar situation on rb5 was to 
have a separate mezzanine dts which includes the baseboard and extends it.


arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
arch/arm64/boot/dts/qcom/qrb5165-rb5-vision-mezzanine.dts

Please replicate that model here.

[1] https://www.thundercomm.com/product/qualcomm-rb3-gen-2/#versions

> @@ -513,6 +513,73 @@ vreg_bob_3p296: bob {
>   	};
>   };
>   
> +&camcc {
> +	status = "okay";
> +};
> +
> +&camss {
> +	status = "disabled";

once you move this into its own dts the camera should be enabled by 
default for that .dtb

> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* The port index denotes CSIPHY id i.e. csiphy2 */
> +		port@3 {
> +			reg = <3>;

copy/past splat from the rb5

port@3 means csiphy3

> +			csiphy3_ep: endpoint {
> +				clock-lanes = <7>;
> +				data-lanes = <0 1 2 3>;
> +				remote-endpoint = <&imx412_ep>;
> +			};
> +		};
> +	};
> +};
> +
> +&cci0 {
> +	status = "okay";
> +};

You're enabling cci0 here but not the sensor for it - presumably a 
monochrome sensor attached to one of the other CSIPHYs.

Zap cci0 here until you add that other sensor.

> +
> +&cci1 {
> +	status = "okay";
> +};
> +
> +&cci1_i2c1 {
> +	camera@1a {
> +		/*
> +		 * rb3gen2 ships with an imx577. qcom treats imx412
> +		 * and imx577 the same way. Absent better data do the same here.
> +		 */
> +		compatible = "sony,imx412";
> +		reg = <0x1a>;

The commit log says imx577 but the comapt string says imx412.

Choose which one and maintain the namespace. Its an imx577 right ? 
Upstream kernel has the relevant compat string

Commit: 1251663220d9 ("media: i2c: imx412: Add new compatible strings")

You can just cherry-pick that commit to your kernel and then the 
upstream dts and downstream dts will be compatible.


> +
> +		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default", "suspend";
> +		pinctrl-0 = <&cam2_default>;
> +		pinctrl-1 = <&cam2_suspend>;
> +
> +		clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;

This looks funny - why do you have MCLK2 ?

One final thing, you appear to be missing some power rails here no ?

e.g. rb5

         vdda-phy-supply = <&vreg_l5a_0p88>;
         vdda-pll-supply = <&vreg_l9a_1p2>;

---
bod
Bryan O'Donoghue June 29, 2024, 11:45 a.m. UTC | #4 
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
Bryan O'Donoghue June 29, 2024, 11:53 a.m. UTC | #5 
On 28/06/2024 19:32, Vikram Sharma wrote:
> +			reg = <0x0 0x0acaf000 0x0 0x5200>,
> +			      <0x0 0x0acb6000 0x0 0x5200>,
> +			      <0x0 0x0acbd000 0x0 0x5200>,
> +			      <0x0 0x0acc4000 0x0 0x5000>,
> +			      <0x0 0x0accb000 0x0 0x5000>,
> +			      <0x0 0x0ace0000 0x0 0x2000>,
> +			      <0x0 0x0ace2000 0x0 0x2000>,
> +			      <0x0 0x0ace4000 0x0 0x2000>,
> +			      <0x0 0x0ace6000 0x0 0x2000>,
> +			      <0x0 0x0ace8000 0x0 0x2000>;
> +
> +			reg-names = "vfe0",
> +				    "vfe1",
> +				    "vfe2",
> +				    "vfe_lite0",
> +				    "vfe_lite1",
> +				    "csiphy0",
> +				    "csiphy1",
> +				    "csiphy2",
> +				    "csiphy3",
> +				    "csiphy4";

Per my comment in the last patch for this series.

In V2 we should see csid and csid_lite registers defined in the dts and 
yaml, with the offset enabling code being dropped as a result.

---
bod
Konrad Dybcio June 29, 2024, 1:10 p.m. UTC | #6 
On 28.06.2024 8:32 PM, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>  				bias-pull-down;
>  			};
>  
> +			cam2_default: cam2-default {
> +				rst {
> +					pins = "gpio78"; /*cam3*/

You can drop these comments.. the node name and label suggest this is
cam*2* anyway

> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-disable;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>; /*RB5 was 16 and i changed to 2 here*/

/* why? */

Konrad
Krzysztof Kozlowski July 1, 2024, 8:59 a.m. UTC | #7 
On 28/06/2024 20:32, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>  				bias-pull-down;
>  			};
>  
> +			cam2_default: cam2-default {

There are tools. Use them, instead of us. Read you internal guideline -
it asks you to do that.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).



Best regards,
Krzysztof
Krzysztof Kozlowski July 1, 2024, 9 a.m. UTC | #8 
On 28/06/2024 20:32, Vikram Sharma wrote:
> Enable IMX577 camera sensor for qcs6490-rb3gen2.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 67 ++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index c4cde4328e3d..237231600dca 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -513,6 +513,73 @@ vreg_bob_3p296: bob {
>  	};
>  };
>  
> +&camcc {
> +	status = "okay";
> +};
> +
> +&camss {
> +	status = "disabled";
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* The port index denotes CSIPHY id i.e. csiphy2 */
> +		port@3 {
> +			reg = <3>;
> +			csiphy3_ep: endpoint {
> +				clock-lanes = <7>;
> +				data-lanes = <0 1 2 3>;
> +				remote-endpoint = <&imx412_ep>;
> +			};
> +		};
> +	};
> +};
> +
> +&cci0 {
> +	status = "okay";
> +};
> +
> +&cci1 {
> +	status = "okay";
> +};
> +
> +&cci1_i2c1 {
> +	camera@1a {
> +		/*
> +		 * rb3gen2 ships with an imx577. qcom treats imx412
> +		 * and imx577 the same way. Absent better data do the same here.

NAK, how qcom treats incorrect code, is not an correct argument.

Use correct compatibles and correct bindings.

> +		 */
> +		compatible = "sony,imx412";
> +		reg = <0x1a>;
> +
> +		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default", "suspend";
> +		pinctrl-0 = <&cam2_default>;
> +		pinctrl-1 = <&cam2_suspend>;
> +
> +		clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clock-rates = <24000000>,
> +							   <24000000>;
> +
> +		dovdd-supply  = <&vreg_l18b_1p8>;
> +		/* avdd-supply = <&vdc_5v>;
> +		 * dvdd-supply = <&vdc_5v>;

What's this?



Best regards,
Krzysztof
Krzysztof Kozlowski July 1, 2024, 9:01 a.m. UTC | #9 
On 28/06/2024 20:32, Vikram Sharma wrote:
> This change enables IMX577 sensor driver for qcm6490.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 414882c57d7f..10e6df566ae3 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -817,6 +817,7 @@ static const struct of_device_id cci_dt_match[] = {
>  	 * Do not add any new ones unless they introduce a new config
>  	 */
>  	{ .compatible = "qcom,msm8916-cci", .data = &cci_v1_data},
> +	{ .compatible = "qcom,sc7280-cci", .data = &cci_v2_data},

NAK, ridiculous.

Best regards,
Krzysztof
Krzysztof Kozlowski July 1, 2024, 9:01 a.m. UTC | #10 
On 29/06/2024 10:22, Luca Weiss wrote:
> On Freitag, 28. Juni 2024 20:32:39 MESZ Vikram Sharma wrote:
>> This change enables IMX577 sensor driver for qcm6490.
>>
>> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
>> ---
>>  drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
>> index 414882c57d7f..10e6df566ae3 100644
>> --- a/drivers/i2c/busses/i2c-qcom-cci.c
>> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
>> @@ -817,6 +817,7 @@ static const struct of_device_id cci_dt_match[] = {
>>  	 * Do not add any new ones unless they introduce a new config
>>  	 */
>>  	{ .compatible = "qcom,msm8916-cci", .data = &cci_v1_data},
>> +	{ .compatible = "qcom,sc7280-cci", .data = &cci_v2_data},
> 
> Please read the comment above qcom,msm8916-cci.
> 
> And sc7280.dtsi already uses
> 
>   compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
> 
> So qcom,msm8996-cci with the same match data (cci_v2_data) gets used, so
> just drop this patch.
> 

I think we put quite obvious comment, yet it is ignored.

Any ideas how to change the comment so people will read it?

Best regards,
Krzysztof
Dmitry Baryshkov July 1, 2024, 6:38 p.m. UTC | #11 
On Sat, Jun 29, 2024 at 12:02:37AM GMT, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.

Note, the subject and commit description do not match patch contents.

> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>  				bias-pull-down;
>  			};
>  
> +			cam2_default: cam2-default {
> +				rst {
> +					pins = "gpio78"; /*cam3*/
> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-disable;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>; /*RB5 was 16 and i changed to 2 here*/
> +					bias-disable;
> +				};
> +			};
> +
> +			cam2_suspend: cam2-suspend {
> +				rst {
> +					pins = "gpio78"; /*cam3*/
> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-pull-down;
> +					output-low;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>;
> +					bias-pull-down;
> +				};
> +			};
> +
>  			dp_hot_plug_det: dp-hot-plug-det-state {
>  				pins = "gpio47";
>  				function = "dp_hot";
> 
> -- 
> 2.25.1
>
Christophe JAILLET July 1, 2024, 7:22 p.m. UTC | #12 
Le 28/06/2024 à 20:32, Vikram Sharma a écrit :
> 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>
> ---

Hi,

...

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

Maybe, as said for other places by Bryan, keep it ordered (CAMSS_7280, 
then CAMSS_8250)?

> +		/* 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;
> +		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:
>   		ret = true;
>   		break;
>   	}
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 83c5a36d071f..757e872b8eb8 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -338,6 +338,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_7280:

Here as well.

>   		switch (sink_code) {
>   		case MEDIA_BUS_FMT_YUYV8_1X16:
>   		{
> @@ -1695,6 +1696,7 @@ static int vfe_bpl_align(struct vfe_device *vfe)
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_7280:

Here as well.

>   		ret = 16;
>   		break;
>   	default:

...

> +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);

Should there be some error handling path here and below to free the 
allocated resources?

.link_entities seems to be new and I can't find it in my -next-20240627, 
so I can't check myself if already handle in a way or another by the 
framework.

CJ


> +			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;
> +}

...
Luca Weiss July 4, 2024, 3:01 p.m. UTC | #13 
On Fri Jun 28, 2024 at 8:32 PM CEST, Vikram Sharma wrote:
> SC7280 is a Qualcomm SoC. This series adds support to
> bring up the CSIPHY, CSID, VFE/RDI interfaces in SC7280.
>
> SC7280 provides
>
> - 3 x VFE, 3 RDI per VFE
> - 2 x VFE Lite, 4 RDI per VFE
> - 3 x CSID
> - 2 x CSID Lite
> - 5 x CSI PHY
>
> This series is rebased based on：
> https://lore.kernel.org/linux-arm-msm/20240522154659.510-1-quic_grosikop@quicinc.com/
>
> The changes are verified on SC7280 qcs6490-rb3gen2 board, the base dts for qcs6490-rb3gen2
> is:
> https://lore.kernel.org/all/20231103184655.23555-1-quic_kbajaj@quicinc.com/

Hi Vikram,

Thanks for sending this patch!

I just tried this on QCM6490 Fairphone 5 smartphone but unfortunately
during probe something is not quite right.

[   99.531855] qcom-camss acaf000.camss: Adding to iommu group 11
[   99.533180] ------------[ cut here ]------------
[   99.533187] qcom-camss acaf000.camss: Error: CSID depends on VFE/IFE device ops!
[   99.533219] WARNING: CPU: 2 PID: 6902 at drivers/media/platform/qcom/camss/camss-csid.c:1024 msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533248] Modules linked in: qcom_camss(+) videobuf2_dma_sg videobuf2_memops videobuf2_v4l2 videobuf2_common
[   99.533266] CPU: 2 PID: 6902 Comm: modprobe Not tainted 6.10.0-rc5-00087-g1dd25cd60c69 #138
[   99.533272] Hardware name: Fairphone 5 (DT)
[   99.533276] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   99.533281] pc : msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533301] lr : msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533321] sp : ffff80008575b760
[   99.533324] x29: ffff80008575b760 x28: ffffae85fd7685d8 x27: ffff80008575bca8
[   99.533334] x26: 0000000000002c28 x25: 0000000000000000 x24: ffff307e4c48a080
[   99.533343] x23: ffff307e46876080 x22: 0000000000000000 x21: ffff307e40bdac10
[   99.533352] x20: ffff307e46876080 x19: 0000000000000000 x18: fffffffffffed520
[   99.533361] x17: 2065636976656420 x16: 4546492f45465620 x15: 6e6f2073646e6570
[   99.533369] x14: ffffae865b46dad0 x13: 2173706f20656369 x12: 766564204546492f
[   99.533377] x11: ffffae865b46dad0 x10: 00000000000002d3 x9 : ffffae865b4c5ad0
[   99.533386] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffffae865b4c5ad0
[   99.533394] x5 : ffff307fb6f83848 x4 : 0000000000000000 x3 : ffff81f95bd4d000
[   99.533402] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff307e4dfe9000
[   99.533412] Call trace:
[   99.533415]  msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533435]  camss_probe+0x310/0x998 [qcom_camss]
[   99.533454]  platform_probe+0x68/0xe0
[   99.533465]  really_probe+0xbc/0x2c0
[   99.533471]  __driver_probe_device+0x78/0x120
[   99.533479]  driver_probe_device+0x3c/0x160
[   99.533485]  __driver_attach+0x90/0x1a0
[   99.533490]  bus_for_each_dev+0x7c/0xf0
[   99.533497]  driver_attach+0x24/0x30
[   99.533503]  bus_add_driver+0xe4/0x208
[   99.533509]  driver_register+0x68/0x124
[   99.533514]  __platform_driver_register+0x28/0x40
[   99.533521]  qcom_camss_driver_init+0x20/0x1000 [qcom_camss]
[   99.533540]  do_one_initcall+0x60/0x1d4
[   99.533547]  do_init_module+0x5c/0x21c
[   99.533555]  load_module+0x18b0/0x1e40
[   99.533562]  init_module_from_file+0x88/0xcc
[   99.533568]  __arm64_sys_finit_module+0x174/0x340
[   99.533575]  invoke_syscall+0x48/0x10c
[   99.533582]  el0_svc_common.constprop.0+0x40/0xe0
[   99.533588]  do_el0_svc+0x1c/0x34
[   99.533594]  el0_svc+0x34/0xe0
[   99.533602]  el0t_64_sync_handler+0x120/0x12c
[   99.533608]  el0t_64_sync+0x190/0x194
[   99.533613] ---[ end trace 0000000000000000 ]---
[   99.533619] qcom-camss acaf000.camss: Failed to init csid0 sub-device: -22
[   99.533828] qcom-camss acaf000.camss: probe with driver qcom-camss failed with error -22

My tree is based on 6.10-rc5 plus v4 of:
https://lore.kernel.org/linux-arm-msm/20240522154659.510-1-quic_grosikop@quicinc.com/
plus your v1 series.

And then some extra patches for my device but nothing touching camss
driver.

Am I missing something?

Regards
Luca

>
> Suresh Vankadara (2):
> media: qcom: camss: support for camss driver on sc7280
> arm64: dts: qcom: sc7280: Add support for camss
>
> Trishansh Bhardwaj (2):
> media: qcom: camss: support for camss driver on sc7280
> arm64: dts: qcom: sc7280: Add support for camss
>
> Vikram Sharma (1):
> media: dt-bindings: media: camss: Add qcom,sc7280-camss binding
>
> Hariram Purshotam (3):
> i2c: Enable IMX577 camera sensor for qcm6490
> arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
> arm64: dts: qcom: sc7280: Add IMX577 camera sensor
>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
> Suresh Vankadara (1):
>       media: qcom: camss: support for camss driver for sc7280
>
> Vikram Sharma (5):
>       media: dt-bindings: media: camss: Add qcom,sc7280-camss binding
>       arm64: dts: qcom: sc7280: Add support for camss
>       arm64: dts: qcom: sc7280: Add IMX577 camera sensor
>       arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
>       i2c: Enable IMX577 camera sensor for qcm6490
>
>  .../bindings/media/qcom,sc7280-camss.yaml          | 477 +++++++++++++++++++++
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |  67 +++
>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 215 ++++++++++
>  drivers/i2c/busses/i2c-qcom-cci.c                  |   1 +
>  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 +
>  9 files changed, 1119 insertions(+), 3 deletions(-)
> ---
> base-commit: 18eeb2d92baca167809cd5d48eb60e0a5c036d51
> change-id: 20240627-camss_first_post_linux_next-f4163c90093c
>
> Best regards,