[PATCH v2 0/8] media: qcom: camss: Add sm8550 support

[PATCH v2 0/8] media: qcom: camss: Add sm8550 support

[Depeng Shao]

V2:
- Update some commit messages
Link to V1: https://lore.kernel.org/all/20240320134227.16587-1-quic_depengs@quicinc.com/

V1:
SM8550 is a Qualcomm flagship SoC. This series adds support to
bring up the CSIPHY, CSID, VFE/RDI interfaces in SM8550.

SM8550 provides

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

This series is rebased based on：
https://patchew.org/linux/20240222-b4-camss-sc8280xp-v6-0-0e0e6a2f8962@linaro.org/

The changes are verified on SM8550 AIM300 board, the base dts for AIM300 is:
https://patchew.org/linux/20231117101817.4401-1-quic._5Ftengfan@quicinc.com/

Depeng Shao (3):
  media: qcom: camss: Add new params for csid_device
  media: qcom: camss: Add CSID gen3 driver
  media: qcom: camss: Add sm8550 support

Yongsheng Li (5):
  media: qcom: camss: Add CAMSS_8550 enum
  media: qcom: camss: Add subdev notify support
  media: qcom: camss: Add new csiphy driver 2-1-2
  media: qcom: camss: Add new VFE driver for SM8550
  media: qcom: camss: Add sm8550 resources

 drivers/media/platform/qcom/camss/Makefile    |   3 +
 .../platform/qcom/camss/camss-csid-gen3.c     | 639 ++++++++++++++++++
 .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
 .../media/platform/qcom/camss/camss-csid.c    |  19 +
 .../media/platform/qcom/camss/camss-csid.h    |  10 +
 .../platform/qcom/camss/camss-csiphy-2-1-2.c  | 343 ++++++++++
 .../media/platform/qcom/camss/camss-csiphy.c  |   1 +
 .../media/platform/qcom/camss/camss-csiphy.h  |   3 +
 .../media/platform/qcom/camss/camss-vfe-780.c | 455 +++++++++++++
 drivers/media/platform/qcom/camss/camss-vfe.c |   7 +
 drivers/media/platform/qcom/camss/camss-vfe.h |   3 +
 .../media/platform/qcom/camss/camss-video.c   |   1 +
 drivers/media/platform/qcom/camss/camss.c     | 395 +++++++++++
 drivers/media/platform/qcom/camss/camss.h     |   8 +
 14 files changed, 1913 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
 create mode 100644 drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
 create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c

-- 
2.17.1

[PATCH v2 1/8] media: qcom: camss: Add CAMSS_8550 enum

[Depeng Shao]

From: Yongsheng Li <quic_yon@quicinc.com>

Adds a CAMSS SoC identifier for the sm8550.

Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
---
 drivers/media/platform/qcom/camss/camss.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index ac15fe23a702..2f63206a8463 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -78,6 +78,7 @@ enum camss_version {
 	CAMSS_845,
 	CAMSS_8250,
 	CAMSS_8280XP,
+	CAMSS_8550,
 };
 
 enum icc_count {
-- 
2.17.1

Re: [PATCH v2 1/8] media: qcom: camss: Add CAMSS_8550 enum

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> From: Yongsheng Li <quic_yon@quicinc.com>
> 
> Adds a CAMSS SoC identifier for the sm8550.
> 
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/camss.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index ac15fe23a702..2f63206a8463 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -78,6 +78,7 @@ enum camss_version {
>   	CAMSS_845,
>   	CAMSS_8250,
>   	CAMSS_8280XP,
> +	CAMSS_8550,
>   };
>   
>   enum icc_count {

Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v2 1/8] media: qcom: camss: Add CAMSS_8550 enum

[Krzysztof Kozlowski]

On 20/03/2024 15:11, Depeng Shao wrote:
> From: Yongsheng Li <quic_yon@quicinc.com>
> 
> Adds a CAMSS SoC identifier for the sm8550.

Why?

> 
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> ---
>  drivers/media/platform/qcom/camss/camss.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index ac15fe23a702..2f63206a8463 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -78,6 +78,7 @@ enum camss_version {
>  	CAMSS_845,
>  	CAMSS_8250,
>  	CAMSS_8280XP,
> +	CAMSS_8550,
>  };

What is the point of this patch alone? What does it change? Why adding
enum? In the next patch, are you going to add one line to some
kerneldoc? Another patch, one function?

Best regards,
Krzysztof

Re: [PATCH v2 1/8] media: qcom: camss: Add CAMSS_8550 enum

[Bryan O'Donoghue]

On 20/03/2024 15:51, Krzysztof Kozlowski wrote:
> On 20/03/2024 15:11, Depeng Shao wrote:
>> From: Yongsheng Li <quic_yon@quicinc.com>
>>
>> Adds a CAMSS SoC identifier for the sm8550.
> 
> Why?
> 
>>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/camss/camss.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
>> index ac15fe23a702..2f63206a8463 100644
>> --- a/drivers/media/platform/qcom/camss/camss.h
>> +++ b/drivers/media/platform/qcom/camss/camss.h
>> @@ -78,6 +78,7 @@ enum camss_version {
>>   	CAMSS_845,
>>   	CAMSS_8250,
>>   	CAMSS_8280XP,
>> +	CAMSS_8550,
>>   };
> 
> What is the point of this patch alone? What does it change? Why adding
> enum? In the next patch, are you going to add one line to some
> kerneldoc? Another patch, one function?
> 
> Best regards,
> Krzysztof
> 

Yeah true enough, you could also add this enum where you use it..

---
bod

Re: [PATCH v2 1/8] media: qcom: camss: Add CAMSS_8550 enum

[Krzysztof Kozlowski]

On 20/03/2024 16:53, Bryan O'Donoghue wrote:
> On 20/03/2024 15:51, Krzysztof Kozlowski wrote:
>> On 20/03/2024 15:11, Depeng Shao wrote:
>>> From: Yongsheng Li <quic_yon@quicinc.com>
>>>
>>> Adds a CAMSS SoC identifier for the sm8550.
>>
>> Why?
>>
>>>
>>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>>> ---
>>>   drivers/media/platform/qcom/camss/camss.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
>>> index ac15fe23a702..2f63206a8463 100644
>>> --- a/drivers/media/platform/qcom/camss/camss.h
>>> +++ b/drivers/media/platform/qcom/camss/camss.h
>>> @@ -78,6 +78,7 @@ enum camss_version {
>>>   	CAMSS_845,
>>>   	CAMSS_8250,
>>>   	CAMSS_8280XP,
>>> +	CAMSS_8550,
>>>   };
>>
>> What is the point of this patch alone? What does it change? Why adding
>> enum? In the next patch, are you going to add one line to some
>> kerneldoc? Another patch, one function?
>>
>> Best regards,
>> Krzysztof
>>
> 
> Yeah true enough, you could also add this enum where you use it..

Just to clarify my comment: this does not mean that entire patchset
should be squashed into one patch, because that would be unmanageable.
But your work should be organized in logical chunks, where some added
code is being used. There is at least one more such change without
immediate users in this patchset...

Best regards,
Krzysztof

[PATCH v2 2/8] media: qcom: camss: Add subdev notify support

[Depeng Shao]

From: Yongsheng Li <quic_yon@quicinc.com>

The buf done irq and register update register are moved
to CSID in SM8550, so but the write master configuration
in VFE, in case adapt existing code logic. So add buf
done and register related subdev event, and use the notify
interface in the v4l2_device structure to communicate
between CSID and VFE driver.

Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
---
 .../media/platform/qcom/camss/camss-csid.h    |  7 +++
 .../media/platform/qcom/camss/camss-csiphy.h  |  2 +
 drivers/media/platform/qcom/camss/camss-vfe.h |  2 +
 drivers/media/platform/qcom/camss/camss.c     | 50 +++++++++++++++++++
 drivers/media/platform/qcom/camss/camss.h     |  7 +++
 5 files changed, 68 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index fddccb69da13..4a9e5a2d1f92 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -147,6 +147,13 @@ struct csid_hw_ops {
 	 * @csid: CSID device
 	 */
 	void (*subdev_init)(struct csid_device *csid);
+
+	/*
+	 * event - receive event from parent v4l2 device
+	 * @csid: CSID device
+	 */
+	void (*event)(struct csid_device *csid,
+			unsigned int evt_type, void *arg);
 };
 
 struct csid_device {
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
index c9b7fe82b1f0..ffe1b95eea98 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.h
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
@@ -61,6 +61,8 @@ struct csiphy_hw_ops {
 	void (*lanes_disable)(struct csiphy_device *csiphy,
 			      struct csiphy_config *cfg);
 	irqreturn_t (*isr)(int irq, void *dev);
+	void (*event)(struct csiphy_device *csiphy,
+			unsigned int evt_type, void *arg);
 };
 
 struct csiphy_device {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 0572c9b08e11..9919fe0ff101 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -115,6 +115,8 @@ struct vfe_hw_ops {
 	int (*vfe_halt)(struct vfe_device *vfe);
 	void (*violation_read)(struct vfe_device *vfe);
 	void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm);
+	void (*event)(struct vfe_device *vfe,
+			unsigned int evt_type, void *arg);
 };
 
 struct vfe_isr_ops {
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1923615f0eea..b57cd25bf6c7 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1904,6 +1904,55 @@ static void camss_genpd_cleanup(struct camss *camss)
 	dev_pm_domain_detach(camss->genpd, true);
 }
 
+static void camss_v4l2_subdev_notify(struct v4l2_subdev *sd,
+			unsigned int cmd, void *arg)
+{
+	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
+	struct camss *camss = to_camss(v4l2_dev);
+	struct vfe_device *vfe;
+	struct vfe_line *vfe_line;
+	struct csid_device *csid;
+	int evt_data = *(int *)arg;
+
+	if (camss->res->version != CAMSS_8550)
+		return;
+
+	switch (cmd) {
+	case NOTIFY_BUF_DONE:
+		csid = v4l2_get_subdevdata(sd);
+		vfe = &(camss->vfe[csid->id]);
+		if (vfe->ops->event)
+			vfe->ops->event(vfe,
+				NOTIFY_BUF_DONE, (void *)&evt_data);
+		break;
+
+	case NOTIFY_RUP:
+		vfe_line = v4l2_get_subdevdata(sd);
+		vfe = to_vfe(vfe_line);
+		csid = &(camss->csid[vfe->id]);
+
+		if (csid->ops->event)
+			csid->ops->event(csid,
+				NOTIFY_RUP, (void *)&evt_data);
+		break;
+
+	case NOTIFY_RUP_CLEAR:
+		vfe_line = v4l2_get_subdevdata(sd);
+		vfe = to_vfe(vfe_line);
+		csid = &(camss->csid[vfe->id]);
+
+		if (csid->ops->event)
+			csid->ops->event(csid,
+				NOTIFY_RUP_CLEAR, (void *)&evt_data);
+
+		break;
+
+	default:
+		dev_err(camss->dev, "Not supported evt type\n");
+		break;
+	}
+}
+
 /*
  * camss_probe - Probe CAMSS platform device
  * @pdev: Pointer to CAMSS platform device
@@ -1974,6 +2023,7 @@ static int camss_probe(struct platform_device *pdev)
 	media_device_init(&camss->media_dev);
 
 	camss->v4l2_dev.mdev = &camss->media_dev;
+	camss->v4l2_dev.notify = camss_v4l2_subdev_notify;
 	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index 2f63206a8463..f1fe68dedd9e 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -86,6 +86,13 @@ enum icc_count {
 	ICC_SM8250_COUNT = 4,
 };
 
+enum subdev_notify_evt {
+	NOTIFY_BUF_DONE = 0,
+	NOTIFY_RUP,
+	NOTIFY_RUP_CLEAR,
+	NOTIFY_MAX,
+};
+
 struct camss_resources {
 	enum camss_version version;
 	const char *pd_name;
-- 
2.17.1

Re: [PATCH v2 2/8] media: qcom: camss: Add subdev notify support

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> From: Yongsheng Li <quic_yon@quicinc.com>
> 
> The buf done irq and register update register are moved
> to CSID in SM8550, so but the write master configuration
> in VFE, in case adapt existing code logic. So add buf
> done and register related subdev event, and use the notify
> interface in the v4l2_device structure to communicate
> between CSID and VFE driver.


Shouldn't it be possible to just have a function to write internally ?

You know the indexes of the CSID -> VFE connection.

The subdev notify is I think not the right fit for this purpose within 
our driver.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index fddccb69da13..4a9e5a2d1f92 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -147,6 +147,13 @@ struct csid_hw_ops {
>   	 * @csid: CSID device
>   	 */
>   	void (*subdev_init)(struct csid_device *csid);
> +
> +	/*
> +	 * event - receive event from parent v4l2 device
> +	 * @csid: CSID device
> +	 */
> +	void (*event)(struct csid_device *csid,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct csid_device {
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index c9b7fe82b1f0..ffe1b95eea98 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -61,6 +61,8 @@ struct csiphy_hw_ops {
>   	void (*lanes_disable)(struct csiphy_device *csiphy,
>   			      struct csiphy_config *cfg);
>   	irqreturn_t (*isr)(int irq, void *dev);
> +	void (*event)(struct csiphy_device *csiphy,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct csiphy_device {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index 0572c9b08e11..9919fe0ff101 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -115,6 +115,8 @@ struct vfe_hw_ops {
>   	int (*vfe_halt)(struct vfe_device *vfe);
>   	void (*violation_read)(struct vfe_device *vfe);
>   	void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm);
> +	void (*event)(struct vfe_device *vfe,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct vfe_isr_ops {
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1923615f0eea..b57cd25bf6c7 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1904,6 +1904,55 @@ static void camss_genpd_cleanup(struct camss *camss)
>   	dev_pm_domain_detach(camss->genpd, true);
>   }
>   
> +static void camss_v4l2_subdev_notify(struct v4l2_subdev *sd,
> +			unsigned int cmd, void *arg)
> +{
> +	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
> +	struct camss *camss = to_camss(v4l2_dev);
> +	struct vfe_device *vfe;
> +	struct vfe_line *vfe_line;
> +	struct csid_device *csid;
> +	int evt_data = *(int *)arg;
> +
> +	if (camss->res->version != CAMSS_8550)
> +		return;
> +
> +	switch (cmd) {
> +	case NOTIFY_BUF_DONE:
> +		csid = v4l2_get_subdevdata(sd);
> +		vfe = &(camss->vfe[csid->id]);
> +		if (vfe->ops->event)
> +			vfe->ops->event(vfe,
> +				NOTIFY_BUF_DONE, (void *)&evt_data);
> +		break;
> +
> +	case NOTIFY_RUP:
> +		vfe_line = v4l2_get_subdevdata(sd);
> +		vfe = to_vfe(vfe_line);
> +		csid = &(camss->csid[vfe->id]);
> +
> +		if (csid->ops->event)
> +			csid->ops->event(csid,
> +				NOTIFY_RUP, (void *)&evt_data);
> +		break;
> +
> +	case NOTIFY_RUP_CLEAR:
> +		vfe_line = v4l2_get_subdevdata(sd);
> +		vfe = to_vfe(vfe_line);
> +		csid = &(camss->csid[vfe->id]);
> +
> +		if (csid->ops->event)
> +			csid->ops->event(csid,
> +				NOTIFY_RUP_CLEAR, (void *)&evt_data);
> +
> +		break;
> +
> +	default:
> +		dev_err(camss->dev, "Not supported evt type\n");
> +		break;
> +	}
> +}

I'm really not sure I see a good reason for this.

Why can't we just define calls between vfe and csid similar to

drivers/media/platform/qcom/camss/camss-csid.c:		ret = vfe_get(vfe);

---
bod

Re: [PATCH v2 2/8] media: qcom: camss: Add subdev notify support

[Gjorgji Rosikopulos (Consultant)]

Hi Bryan,

On 3/20/2024 6:08 PM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
>> From: Yongsheng Li <quic_yon@quicinc.com>
>>
>> The buf done irq and register update register are moved
>> to CSID in SM8550, so but the write master configuration
>> in VFE, in case adapt existing code logic. So add buf
>> done and register related subdev event, and use the notify
>> interface in the v4l2_device structure to communicate
>> between CSID and VFE driver.
> 
> 
> Shouldn't it be possible to just have a function to write internally ?
> 
> You know the indexes of the CSID -> VFE connection.
> 
> The subdev notify is I think not the right fit for this purpose within
> our driver.
> 

<snip>

> 
> I'm really not sure I see a good reason for this.
> 
> Why can't we just define calls between vfe and csid similar to
> 
> drivers/media/platform/qcom/camss/camss-csid.c:        ret = vfe_get(vfe);


Maybe we need to rethink and redesign this part of the driver.

In the initial version when this driver was introduced the CSID was
independent device from hw perspective,
and represented as separate sub-device.

With the newer architectures CSID was part of IFE hw (handled by the VFE
sub-device in this driver)
and vfe_get was introduced, but i believe it was not an issue because
the CISD still was kind of independent.

In the patch series:
"https://lore.kernel.org/lkml/20240319173935.481-4-quic_grosikop@quicinc.com/T/"

We try to decouple CSID from VFE and remove direct dependency
introducing parent_dev_ops,
where depending of the topology and parent device (VFE in this case or
other parent in future chipsets which contain CSID)
can reuse the code.

I am not sure if introducing parent_dev_ops is right way to go but we
can discuss further and see how to extend the driver
in proper way.

Just to add i am not saying that adding direct calls to VFE is not
proper but just close the door for other sub-device contain CSID to use
the code. :-)


Regards,
~Gjorgji

> 
> ---
> bod
> 

[PATCH v2 3/8] media: qcom: camss: Add new csiphy driver 2-1-2

[Depeng Shao]

From: Yongsheng Li <quic_yon@quicinc.com>

The CSIPHY register offset of SM8550 is different with
SM8250, so add a new driver for the SM8550 platform.

Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
Co-developed-by: Depeng Shao <depengs@qti.qualcomm.com>
Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
 drivers/media/platform/qcom/camss/Makefile    |   1 +
 .../platform/qcom/camss/camss-csiphy-2-1-2.c  | 343 ++++++++++++++++++
 .../media/platform/qcom/camss/camss-csiphy.h  |   1 +
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c

diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
index 0d4389ab312d..28eba4bf3e38 100644
--- a/drivers/media/platform/qcom/camss/Makefile
+++ b/drivers/media/platform/qcom/camss/Makefile
@@ -9,6 +9,7 @@ qcom-camss-objs += \
 		camss-csid-gen2.o \
 		camss-csiphy-2ph-1-0.o \
 		camss-csiphy-3ph-1-0.o \
+		camss-csiphy-2-1-2.o \
 		camss-csiphy.o \
 		camss-ispif.o \
 		camss-vfe-4-1.o \
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c b/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
new file mode 100644
index 000000000000..df7860d7a4f4
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * camss-csiphy-2-1-2.c
+ *
+ * Qualcomm MSM Camera Subsystem - CSIPHY Module 3phase v2.0
+ *
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "camss.h"
+#include "camss-csiphy.h"
+
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+
+
+#define CSIPHY_CMN_CSI_COMMON_CTRLn(n)	(0x1000 + 0x4 * (n))
+#define CSIPHY_CMN_CSI_COMMON_CTRL5_CLK_ENABLE	BIT(7)
+#define CSIPHY_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B	BIT(0)
+#define CSIPHY_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID	BIT(1)
+#define CSIPHY_CMN_CSI_COMMON_CTRL7_CONTROL	0x7A
+#define CSIPHY_CMN_CSI_COMMON_CTRL0_PHY_SW_RESET	BIT(0)
+#define CSIPHY_CMN_CSI_COMMON_CTRL0_SWI_ENABLE_LANESYNC	BIT(1)
+#define CSIPHY_CMN_CSI_COMMON_STATUSn(n)	(0x10B0 + 0x4 * (n))
+
+#define CSIPHY_DEFAULT_PARAMS            0
+#define CSIPHY_LANE_ENABLE               1
+#define CSIPHY_SETTLE_CNT_LOWER_BYTE     2
+#define CSIPHY_SETTLE_CNT_HIGHER_BYTE    3
+#define CSIPHY_DNP_PARAMS                4
+#define CSIPHY_2PH_REGS                  5
+#define CSIPHY_3PH_REGS                  6
+
+struct csiphy_reg_t {
+	s32 reg_addr;
+	s32 reg_data;
+	s32 delay;
+	u32 csiphy_param_type;
+};
+
+/* 2.1.2 2PH */
+static const struct
+csiphy_reg_t lane_regs_sm8550[5][20] = {
+	{
+		{0x0E90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+		{0x00A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0090, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0098, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0094, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+		{0x0494, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x04A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0490, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0498, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0494, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+		{0x0894, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x08A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0890, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0898, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0894, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+		{0x0C94, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0CA0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+	},
+	{
+		{0x0C98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+		{0x0E30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E28, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E00, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E0C, 0xFF, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E38, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0E08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+		{0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0000, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+	},
+	{
+		{0x0034, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x001C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0020, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+		{0x0010, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0400, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0434, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x041C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0420, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+		{0x0410, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+	},
+	{
+		{0x0830, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0800, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0838, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x082C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0834, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x081C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0814, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x083C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0804, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0820, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0808, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+		{0x0810, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C00, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C38, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+	},
+	{
+		{0x0C04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+		{0x0C10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0094, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x005C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0060, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0064, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0494, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x045C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0460, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0464, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0894, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x085C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0860, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0864, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C94, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C5C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C60, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
+		{0x0C64, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
+	}
+};
+
+static void csiphy_hw_version_read(struct csiphy_device *csiphy,
+				   struct device *dev)
+{
+	u32 hw_version;
+
+	writel(CSIPHY_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID,
+	       csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(6));
+
+	hw_version = readl_relaxed(csiphy->base +
+				   CSIPHY_CMN_CSI_COMMON_STATUSn(12));
+	hw_version |= readl_relaxed(csiphy->base +
+				   CSIPHY_CMN_CSI_COMMON_STATUSn(13)) << 8;
+	hw_version |= readl_relaxed(csiphy->base +
+				   CSIPHY_CMN_CSI_COMMON_STATUSn(14)) << 16;
+	hw_version |= readl_relaxed(csiphy->base +
+				   CSIPHY_CMN_CSI_COMMON_STATUSn(15)) << 24;
+
+	dev_info(dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);
+}
+
+/*
+ * csiphy_reset - Perform software reset on CSIPHY module
+ * @csiphy: CSIPHY device
+ */
+static void csiphy_reset(struct csiphy_device *csiphy)
+{
+	writel_relaxed(0x1, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(0));
+	usleep_range(5000, 8000);
+	writel_relaxed(0x0, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(0));
+}
+
+static irqreturn_t csiphy_isr(int irq, void *dev)
+{
+	struct csiphy_device *csiphy = dev;
+	int i;
+
+	for (i = 0; i < 11; i++) {
+		int c = i + 22;
+		u8 val = readl_relaxed(csiphy->base +
+				       CSIPHY_CMN_CSI_COMMON_STATUSn(i));
+
+		writel_relaxed(val, csiphy->base +
+				    CSIPHY_CMN_CSI_COMMON_CTRLn(c));
+	}
+
+	writel_relaxed(0x1, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(10));
+	writel_relaxed(0x0, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(10));
+
+	for (i = 22; i < 33; i++)
+		writel_relaxed(0x0, csiphy->base +
+				    CSIPHY_CMN_CSI_COMMON_CTRLn(i));
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * csiphy_settle_cnt_calc - Calculate settle count value
+ *
+ * Helper function to calculate settle count value. This is
+ * based on the CSI2 T_hs_settle parameter which in turn
+ * is calculated based on the CSI2 transmitter link frequency.
+ *
+ * Return settle count value or 0 if the CSI2 link frequency
+ * is not available
+ */
+static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
+{
+	u32 ui; /* ps */
+	u32 timer_period; /* ps */
+	u32 t_hs_prepare_max; /* ps */
+	u32 t_hs_settle; /* ps */
+	u8 settle_cnt;
+
+	if (link_freq <= 0)
+		return 0;
+
+	ui = div_u64(1000000000000LL, link_freq);
+	ui /= 2;
+	t_hs_prepare_max = 85000 + 6 * ui;
+	t_hs_settle = t_hs_prepare_max;
+
+	timer_period = div_u64(1000000000000LL, timer_clk_rate);
+	settle_cnt = t_hs_settle / timer_period - 6;
+
+	return settle_cnt;
+}
+
+static void csiphy_config_lanes(struct csiphy_device *csiphy,
+				     u8 settle_cnt)
+{
+	const struct csiphy_reg_t *r;
+	int i, l, array_size;
+	u32 val;
+
+	switch (csiphy->camss->res->version) {
+	case CAMSS_8550:
+		r = &lane_regs_sm8550[0][0];
+		array_size = ARRAY_SIZE(lane_regs_sm8550[0]);
+		break;
+	default:
+		WARN(1, "unknown csi version\n");
+		return;
+	}
+
+	for (l = 0; l < 5; l++) {
+		for (i = 0; i < array_size; i++, r++) {
+			switch (r->csiphy_param_type) {
+			case CSIPHY_SETTLE_CNT_LOWER_BYTE:
+				val = settle_cnt & 0xff;
+				break;
+			case CSIPHY_DNP_PARAMS:
+				continue;
+			default:
+				val = r->reg_data;
+				break;
+			}
+			writel_relaxed(val, csiphy->base + r->reg_addr);
+
+			if (r->delay > 0)
+				udelay(r->delay);
+		}
+	}
+}
+
+static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
+{
+	u8 lane_mask;
+	int i;
+
+	for (i = 0; i < lane_cfg->num_data; i++)
+		lane_mask |= 1 << lane_cfg->data[i].pos;
+
+	return lane_mask;
+}
+
+static void csiphy_lanes_enable(struct csiphy_device *csiphy,
+				struct csiphy_config *cfg,
+				s64 link_freq, u8 lane_mask)
+{
+	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
+	u8 settle_cnt;
+	u8 val = 0;
+	int i;
+
+	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
+
+	val = CSIPHY_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
+	for (i = 0; i < c->num_data; i++)
+		val |= BIT(c->data[i].pos * 2);
+
+	writel_relaxed(val, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(5));
+
+	val = CSIPHY_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B;
+	writel_relaxed(val, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(6));
+
+	val = 0x2;
+	writel_relaxed(val, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(7));
+
+	val = 0x0;
+	writel_relaxed(val, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(0));
+
+	csiphy_config_lanes(csiphy, settle_cnt);
+
+	/* IRQ_MASK registers - disable all interrupts */
+	for (i = 11; i < 22; i++)
+		writel_relaxed(0, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(i));
+}
+
+static void csiphy_lanes_disable(struct csiphy_device *csiphy,
+				 struct csiphy_config *cfg)
+{
+	writel_relaxed(0, csiphy->base +
+		CSIPHY_CMN_CSI_COMMON_CTRLn(5));
+
+	writel_relaxed(0, csiphy->base +
+		CSIPHY_CMN_CSI_COMMON_CTRLn(6));
+}
+
+const struct csiphy_hw_ops csiphy_ops_2_1_2 = {
+	.get_lane_mask = csiphy_get_lane_mask,
+	.hw_version_read = csiphy_hw_version_read,
+	.reset = csiphy_reset,
+	.lanes_enable = csiphy_lanes_enable,
+	.lanes_disable = csiphy_lanes_disable,
+	.isr = csiphy_isr,
+	.event = NULL,
+};
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
index ffe1b95eea98..286fdb9f4fdf 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.h
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
@@ -98,5 +98,6 @@ void msm_csiphy_unregister_entity(struct csiphy_device *csiphy);
 
 extern const struct csiphy_hw_ops csiphy_ops_2ph_1_0;
 extern const struct csiphy_hw_ops csiphy_ops_3ph_1_0;
+extern const struct csiphy_hw_ops csiphy_ops_2_1_2;
 
 #endif /* QC_MSM_CAMSS_CSIPHY_H */
-- 
2.17.1

Re: [PATCH v2 3/8] media: qcom: camss: Add new csiphy driver 2-1-2

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> From: Yongsheng Li <quic_yon@quicinc.com>
> 
> The CSIPHY register offset of SM8550 is different with
> SM8250, so add a new driver for the SM8550 platform.
> 
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Co-developed-by: Depeng Shao <depengs@qti.qualcomm.com>

You're listing yourself ? Twice with a different CD and SOB.
Shouldn't this be
Co-developed-by: Depeng Shao <quic_depengs@quicinc.com>

?

> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>   .../platform/qcom/camss/camss-csiphy-2-1-2.c  | 343 ++++++++++++++++++
>   .../media/platform/qcom/camss/camss-csiphy.h  |   1 +
>   3 files changed, 345 insertions(+)
>   create mode 100644 drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
> 
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index 0d4389ab312d..28eba4bf3e38 100644
> --- a/drivers/media/platform/qcom/camss/Makefile
> +++ b/drivers/media/platform/qcom/camss/Makefile
> @@ -9,6 +9,7 @@ qcom-camss-objs += \
>   		camss-csid-gen2.o \
>   		camss-csiphy-2ph-1-0.o \
>   		camss-csiphy-3ph-1-0.o \
> +		camss-csiphy-2-1-2.o \

File name here is incorrect camss-csiphy-3ph-1-2.o this is a 3 phase 
capable PHY right ? So both the filename and the commit title need to 
reflect that.

I thought we had discussed offline using an offset instead of a new file ?

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/6dc8f49ef7c6ca69783aa02bdca81c77e66ecc0d

Then just set the offset

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/022a837257d9e1fa8070f0dfa2f683e903111aa0


>   		camss-csiphy.o \
>   		camss-ispif.o \
>   		camss-vfe-4-1.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c b/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
> new file mode 100644
> index 000000000000..df7860d7a4f4
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-csiphy-2-1-2.c
> + *
> + * Qualcomm MSM Camera Subsystem - CSIPHY Module 3phase v2.0
> + *
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include "camss.h"
> +#include "camss-csiphy.h"
> +
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +
> +
> +#define CSIPHY_CMN_CSI_COMMON_CTRLn(n)	(0x1000 + 0x4 * (n))
> +#define CSIPHY_CMN_CSI_COMMON_CTRL5_CLK_ENABLE	BIT(7)
> +#define CSIPHY_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B	BIT(0)
> +#define CSIPHY_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID	BIT(1)
> +#define CSIPHY_CMN_CSI_COMMON_CTRL7_CONTROL	0x7A
> +#define CSIPHY_CMN_CSI_COMMON_CTRL0_PHY_SW_RESET	BIT(0)
> +#define CSIPHY_CMN_CSI_COMMON_CTRL0_SWI_ENABLE_LANESYNC	BIT(1)
> +#define CSIPHY_CMN_CSI_COMMON_STATUSn(n)	(0x10B0 + 0x4 * (n))
> +
> +#define CSIPHY_DEFAULT_PARAMS            0
> +#define CSIPHY_LANE_ENABLE               1
> +#define CSIPHY_SETTLE_CNT_LOWER_BYTE     2
> +#define CSIPHY_SETTLE_CNT_HIGHER_BYTE    3
> +#define CSIPHY_DNP_PARAMS                4
> +#define CSIPHY_2PH_REGS                  5
> +#define CSIPHY_3PH_REGS                  6
> +
> +struct csiphy_reg_t {
> +	s32 reg_addr;
> +	s32 reg_data;
> +	s32 delay;
> +	u32 csiphy_param_type;
> +};
> +
> +/* 2.1.2 2PH */
> +static const struct
> +csiphy_reg_t lane_regs_sm8550[5][20] = {

So per the tree I shared with you..

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/fa666c241eb530a08aa6b391e15b018296e93f66

We don't need to break this up into a multi-dimensional array

its just a straight write of values
> +	{
> +		{0x0E90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
> +		{0x00A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0090, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0098, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0094, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
> +		{0x0494, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x04A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0490, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0498, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0494, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
> +		{0x0894, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x08A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0890, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0898, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0894, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C94, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0CA0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0C98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E28, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E00, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E0C, 0xFF, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E38, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0E08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0000, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0034, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x001C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0020, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0010, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0400, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0434, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x041C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0420, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0410, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0830, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0800, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0838, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x082C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0834, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x081C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0814, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x083C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0804, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0820, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0808, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0810, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C00, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C38, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0C04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0C10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0094, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x005C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0060, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0064, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0494, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x045C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0460, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0464, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0894, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x085C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0860, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0864, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C94, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C5C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C60, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C64, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	}
> +};
> +
> +static void csiphy_hw_version_read(struct csiphy_device *csiphy,
> +				   struct device *dev)
> +{
> +	u32 hw_version;
> +
> +	writel(CSIPHY_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID,
> +	       csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(6));
> +
> +	hw_version = readl_relaxed(csiphy->base +
> +				   CSIPHY_CMN_CSI_COMMON_STATUSn(12));
> +	hw_version |= readl_relaxed(csiphy->base +
> +				   CSIPHY_CMN_CSI_COMMON_STATUSn(13)) << 8;
> +	hw_version |= readl_relaxed(csiphy->base +
> +				   CSIPHY_CMN_CSI_COMMON_STATUSn(14)) << 16;
> +	hw_version |= readl_relaxed(csiphy->base +
> +				   CSIPHY_CMN_CSI_COMMON_STATUSn(15)) << 24;
> +
> +	dev_info(dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);
> +}
> +
> +/*
> + * csiphy_reset - Perform software reset on CSIPHY module
> + * @csiphy: CSIPHY device
> + */
> +static void csiphy_reset(struct csiphy_device *csiphy)
> +{
> +	writel_relaxed(0x1, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(0));
> +	usleep_range(5000, 8000);
> +	writel_relaxed(0x0, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(0));
> +}
> +
> +static irqreturn_t csiphy_isr(int irq, void *dev)
> +{
> +	struct csiphy_device *csiphy = dev;
> +	int i;
> +
> +	for (i = 0; i < 11; i++) {
> +		int c = i + 22;
> +		u8 val = readl_relaxed(csiphy->base +
> +				       CSIPHY_CMN_CSI_COMMON_STATUSn(i));
> +
> +		writel_relaxed(val, csiphy->base +
> +				    CSIPHY_CMN_CSI_COMMON_CTRLn(c));
> +	}
> +
> +	writel_relaxed(0x1, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(10));
> +	writel_relaxed(0x0, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(10));
> +
> +	for (i = 22; i < 33; i++)
> +		writel_relaxed(0x0, csiphy->base +
> +				    CSIPHY_CMN_CSI_COMMON_CTRLn(i));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * csiphy_settle_cnt_calc - Calculate settle count value
> + *
> + * Helper function to calculate settle count value. This is
> + * based on the CSI2 T_hs_settle parameter which in turn
> + * is calculated based on the CSI2 transmitter link frequency.
> + *
> + * Return settle count value or 0 if the CSI2 link frequency
> + * is not available
> + */
> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
> +{
> +	u32 ui; /* ps */
> +	u32 timer_period; /* ps */
> +	u32 t_hs_prepare_max; /* ps */
> +	u32 t_hs_settle; /* ps */
> +	u8 settle_cnt;
> +
> +	if (link_freq <= 0)
> +		return 0;
> +
> +	ui = div_u64(1000000000000LL, link_freq);
> +	ui /= 2;
> +	t_hs_prepare_max = 85000 + 6 * ui;
> +	t_hs_settle = t_hs_prepare_max;
> +
> +	timer_period = div_u64(1000000000000LL, timer_clk_rate);
> +	settle_cnt = t_hs_settle / timer_period - 6;
> +
> +	return settle_cnt;
> +}

Yep this is all literal copy/paste of existing code.

Please try taking

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/aa27ac8e1ffedd48c5ef6ac0f75f1f15716fe296

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/fa666c241eb530a08aa6b391e15b018296e93f66

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/e9e99dee4e723755274e6430bb43adfbf77d2a1a

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/5dc53c39b96eb22bdfe47554f047e6b63ddb25c0

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/36655b353fa71269812cff0e695341ff12042546

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/6dc8f49ef7c6ca69783aa02bdca81c77e66ecc0d

and

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/042ec64867d43e014c2b369db3b92b4f5432497f

into your tree

it works for me on sm8250 and is WIP for x1e80100

Point being we need to make an effort to unify/reuse/fix init sequences 
upstream not to copy/paste from one file to another.

> +
> +static void csiphy_config_lanes(struct csiphy_device *csiphy,
> +				     u8 settle_cnt)
> +{
> +	const struct csiphy_reg_t *r;
> +	int i, l, array_size;
> +	u32 val;
> +
> +	switch (csiphy->camss->res->version) {
> +	case CAMSS_8550:
> +		r = &lane_regs_sm8550[0][0];
> +		array_size = ARRAY_SIZE(lane_regs_sm8550[0]);
> +		break;
> +	default:
> +		WARN(1, "unknown csi version\n");
> +		return;
> +	}
> +
> +	for (l = 0; l < 5; l++) {

These hard-coded values in control loops must be done away with upstream.

---
bod

Re: [PATCH v2 3/8] media: qcom: camss: Add new csiphy driver 2-1-2

[Depeng Shao]

Hi Bryan,


On 3/20/2024 11:21 PM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
>> From: Yongsheng Li <quic_yon@quicinc.com>
>>
>> The CSIPHY register offset of SM8550 is different with
>> SM8250, so add a new driver for the SM8550 platform.
>>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> Co-developed-by: Depeng Shao <depengs@qti.qualcomm.com>
>
> You're listing yourself ? Twice with a different CD and SOB.
> Shouldn't this be
> Co-developed-by: Depeng Shao <quic_depengs@quicinc.com>
>
> ?
Sorry for mistake, will update it in new patch.
>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>>   .../platform/qcom/camss/camss-csiphy-2-1-2.c  | 343 ++++++++++++++++++
>>   .../media/platform/qcom/camss/camss-csiphy.h  |   1 +
>>   3 files changed, 345 insertions(+)
>>   create mode 100644 
>> drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
>>
>> diff --git a/drivers/media/platform/qcom/camss/Makefile 
>> b/drivers/media/platform/qcom/camss/Makefile
>> index 0d4389ab312d..28eba4bf3e38 100644
>> --- a/drivers/media/platform/qcom/camss/Makefile
>> +++ b/drivers/media/platform/qcom/camss/Makefile
>> @@ -9,6 +9,7 @@ qcom-camss-objs += \
>>           camss-csid-gen2.o \
>>           camss-csiphy-2ph-1-0.o \
>>           camss-csiphy-3ph-1-0.o \
>> +        camss-csiphy-2-1-2.o \
>
> File name here is incorrect camss-csiphy-3ph-1-2.o this is a 3 phase 
> capable PHY right ? So both the filename and the commit title need to 
> reflect that.
>
> I thought we had discussed offline using an offset instead of a new 
> file ?
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/6dc8f49ef7c6ca69783aa02bdca81c77e66ecc0d 
>
>
> Then just set the offset
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/022a837257d9e1fa8070f0dfa2f683e903111aa0 
>
>
Yes, this is a good idea, but looks like the code in your x1e80100 
branch hasn't ready yet, could we have a separate patch for this 
optimization, then I can rebase my change based on it.


>
>>           camss-csiphy.o \
>>           camss-ispif.o \
>>           camss-vfe-4-1.o \
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c 
>> b/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
>> new file mode 100644
>> index 000000000000..df7860d7a4f4
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2-1-2.c
>> @@ -0,0 +1,343 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * camss-csiphy-2-1-2.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - CSIPHY Module 3phase v2.0
>> + *
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#include "camss.h"
>> +#include "camss-csiphy.h"
>> +
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +
>> +
>> +#define CSIPHY_CMN_CSI_COMMON_CTRLn(n)    (0x1000 + 0x4 * (n))
>> +#define CSIPHY_CMN_CSI_COMMON_CTRL5_CLK_ENABLE    BIT(7)
>> +#define CSIPHY_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B    BIT(0)
>> +#define CSIPHY_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID    BIT(1)
>> +#define CSIPHY_CMN_CSI_COMMON_CTRL7_CONTROL    0x7A
>> +#define CSIPHY_CMN_CSI_COMMON_CTRL0_PHY_SW_RESET    BIT(0)
>> +#define CSIPHY_CMN_CSI_COMMON_CTRL0_SWI_ENABLE_LANESYNC BIT(1)
>> +#define CSIPHY_CMN_CSI_COMMON_STATUSn(n)    (0x10B0 + 0x4 * (n))
>> +
>> +#define CSIPHY_DEFAULT_PARAMS            0
>> +#define CSIPHY_LANE_ENABLE               1
>> +#define CSIPHY_SETTLE_CNT_LOWER_BYTE     2
>> +#define CSIPHY_SETTLE_CNT_HIGHER_BYTE    3
>> +#define CSIPHY_DNP_PARAMS                4
>> +#define CSIPHY_2PH_REGS                  5
>> +#define CSIPHY_3PH_REGS                  6
>> +
>> +struct csiphy_reg_t {
>> +    s32 reg_addr;
>> +    s32 reg_data;
>> +    s32 delay;
>> +    u32 csiphy_param_type;
>> +};
>> +
>> +/* 2.1.2 2PH */
>> +static const struct
>> +csiphy_reg_t lane_regs_sm8550[5][20] = {
>
> So per the tree I shared with you..
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/fa666c241eb530a08aa6b391e15b018296e93f66 
>
>
> We don't need to break this up into a multi-dimensional array
>
> its just a straight write of values


Yeah, agree, I also don't want to do like this, I just follow the old 
driver's format


>> +    {
>> +        {0x0E90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>> +        {0x00A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0090, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0098, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0094, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0494, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x04A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0490, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0498, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0494, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0894, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x08A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0890, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0898, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0894, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C94, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0CA0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +    },
>> +    {
>> +        {0x0C98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E28, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E00, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E0C, 0xFF, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E38, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0E08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>> +        {0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0000, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +    },
>> +    {
>> +        {0x0034, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x001C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0020, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>> +        {0x0010, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0400, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0434, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x041C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0420, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>> +        {0x0410, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +    },
>> +    {
>> +        {0x0830, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0800, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0838, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x082C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0834, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x081C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0814, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x083C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0804, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0820, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0808, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>> +        {0x0810, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C00, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C38, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +    },
>> +    {
>> +        {0x0C04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>> +        {0x0C10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0094, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x005C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0060, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0064, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0494, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x045C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0460, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0464, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0894, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x085C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0860, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0864, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C94, 0xD7, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C5C, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C60, 0xBD, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +        {0x0C64, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
>> +    }
>> +};
>> +
>> +static void csiphy_hw_version_read(struct csiphy_device *csiphy,
>> +                   struct device *dev)
>> +{
>> +    u32 hw_version;
>> +
>> +    writel(CSIPHY_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID,
>> +           csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(6));
>> +
>> +    hw_version = readl_relaxed(csiphy->base +
>> +                   CSIPHY_CMN_CSI_COMMON_STATUSn(12));
>> +    hw_version |= readl_relaxed(csiphy->base +
>> +                   CSIPHY_CMN_CSI_COMMON_STATUSn(13)) << 8;
>> +    hw_version |= readl_relaxed(csiphy->base +
>> +                   CSIPHY_CMN_CSI_COMMON_STATUSn(14)) << 16;
>> +    hw_version |= readl_relaxed(csiphy->base +
>> +                   CSIPHY_CMN_CSI_COMMON_STATUSn(15)) << 24;
>> +
>> +    dev_info(dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);
>> +}
>> +
>> +/*
>> + * csiphy_reset - Perform software reset on CSIPHY module
>> + * @csiphy: CSIPHY device
>> + */
>> +static void csiphy_reset(struct csiphy_device *csiphy)
>> +{
>> +    writel_relaxed(0x1, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(0));
>> +    usleep_range(5000, 8000);
>> +    writel_relaxed(0x0, csiphy->base + CSIPHY_CMN_CSI_COMMON_CTRLn(0));
>> +}
>> +
>> +static irqreturn_t csiphy_isr(int irq, void *dev)
>> +{
>> +    struct csiphy_device *csiphy = dev;
>> +    int i;
>> +
>> +    for (i = 0; i < 11; i++) {
>> +        int c = i + 22;
>> +        u8 val = readl_relaxed(csiphy->base +
>> +                       CSIPHY_CMN_CSI_COMMON_STATUSn(i));
>> +
>> +        writel_relaxed(val, csiphy->base +
>> +                    CSIPHY_CMN_CSI_COMMON_CTRLn(c));
>> +    }
>> +
>> +    writel_relaxed(0x1, csiphy->base + 
>> CSIPHY_CMN_CSI_COMMON_CTRLn(10));
>> +    writel_relaxed(0x0, csiphy->base + 
>> CSIPHY_CMN_CSI_COMMON_CTRLn(10));
>> +
>> +    for (i = 22; i < 33; i++)
>> +        writel_relaxed(0x0, csiphy->base +
>> +                    CSIPHY_CMN_CSI_COMMON_CTRLn(i));
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +/*
>> + * csiphy_settle_cnt_calc - Calculate settle count value
>> + *
>> + * Helper function to calculate settle count value. This is
>> + * based on the CSI2 T_hs_settle parameter which in turn
>> + * is calculated based on the CSI2 transmitter link frequency.
>> + *
>> + * Return settle count value or 0 if the CSI2 link frequency
>> + * is not available
>> + */
>> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>> +{
>> +    u32 ui; /* ps */
>> +    u32 timer_period; /* ps */
>> +    u32 t_hs_prepare_max; /* ps */
>> +    u32 t_hs_settle; /* ps */
>> +    u8 settle_cnt;
>> +
>> +    if (link_freq <= 0)
>> +        return 0;
>> +
>> +    ui = div_u64(1000000000000LL, link_freq);
>> +    ui /= 2;
>> +    t_hs_prepare_max = 85000 + 6 * ui;
>> +    t_hs_settle = t_hs_prepare_max;
>> +
>> +    timer_period = div_u64(1000000000000LL, timer_clk_rate);
>> +    settle_cnt = t_hs_settle / timer_period - 6;
>> +
>> +    return settle_cnt;
>> +}
>
> Yep this is all literal copy/paste of existing code.
>
> Please try taking
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/aa27ac8e1ffedd48c5ef6ac0f75f1f15716fe296 
>
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/fa666c241eb530a08aa6b391e15b018296e93f66 
>
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/e9e99dee4e723755274e6430bb43adfbf77d2a1a 
>
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/5dc53c39b96eb22bdfe47554f047e6b63ddb25c0 
>
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/36655b353fa71269812cff0e695341ff12042546 
>
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/6dc8f49ef7c6ca69783aa02bdca81c77e66ecc0d 
>
>
> and
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/042ec64867d43e014c2b369db3b92b4f5432497f 
>
>
> into your tree
>
> it works for me on sm8250 and is WIP for x1e80100
>
> Point being we need to make an effort to unify/reuse/fix init 
> sequences upstream not to copy/paste from one file to another.
>
I asked can we have a separate change for the csiphy optimization in 
above, looks like this the separate series.

Sorry, I just want to make sure the merge order since there are many 
ongoing changes, they will conflict with each other.

>> +
>> +static void csiphy_config_lanes(struct csiphy_device *csiphy,
>> +                     u8 settle_cnt)
>> +{
>> +    const struct csiphy_reg_t *r;
>> +    int i, l, array_size;
>> +    u32 val;
>> +
>> +    switch (csiphy->camss->res->version) {
>> +    case CAMSS_8550:
>> +        r = &lane_regs_sm8550[0][0];
>> +        array_size = ARRAY_SIZE(lane_regs_sm8550[0]);
>> +        break;
>> +    default:
>> +        WARN(1, "unknown csi version\n");
>> +        return;
>> +    }
>> +
>> +    for (l = 0; l < 5; l++) {
>
> These hard-coded values in control loops must be done away with upstream.
>
Won't have these code if I rebase the change based on yours.
> ---
> bod


Thanks,

Depeng

[PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

[Depeng Shao]

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 SFE or original VFE in CSID gen3.
The register update is moved to CSID from VFE in CSID
gen3.
So, adding top_base and reg_update variables in csid
device structure for CSID gen3.

Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
 drivers/media/platform/qcom/camss/camss-csid.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index 4a9e5a2d1f92..ca654b007441 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -162,7 +162,9 @@ struct csid_device {
 	struct v4l2_subdev subdev;
 	struct media_pad pads[MSM_CSID_PADS_NUM];
 	void __iomem *base;
+	void __iomem *top_base;
 	u32 irq;
+	u32 reg_update;
 	char irq_name[30];
 	struct camss_clock *clock;
 	int nclocks;
-- 
2.17.1

Re: [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> 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 SFE or original VFE in CSID gen3.
> The register update is moved to CSID from VFE in CSID
> gen3.
> So, adding top_base and reg_update variables in csid
> device structure for CSID gen3.

You need to define three letter acronyms (TLAs) - within reason - the 
first time you use them.

There is no concept of an "SFE" upstream - please define what the SFE is 
in the commit log "to the Sensor Front End (SFE)" then you can refer 
back to the acronym liberally.

---
bod

Re: [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

[Krzysztof Kozlowski]

On 20/03/2024 15:11, Depeng Shao wrote:
> 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 SFE or original VFE in CSID gen3.
> The register update is moved to CSID from VFE in CSID
> gen3.
> So, adding top_base and reg_update variables in csid
> device structure for CSID gen3.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---
>  drivers/media/platform/qcom/camss/camss-csid.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index 4a9e5a2d1f92..ca654b007441 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -162,7 +162,9 @@ struct csid_device {
>  	struct v4l2_subdev subdev;
>  	struct media_pad pads[MSM_CSID_PADS_NUM];
>  	void __iomem *base;
> +	void __iomem *top_base;
>  	u32 irq;
> +	u32 reg_update;
>  	char irq_name[30];

This is pointless. The are no users of this!

Sorry, don't add random code here or there without concept.

Best regards,
Krzysztof

Re: [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device

[Depeng Shao]

Hi Krzysztof,

On 3/20/2024 11:53 PM, Krzysztof Kozlowski wrote:
> On 20/03/2024 15:11, Depeng Shao wrote:
>> 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 SFE or original VFE in CSID gen3.
>> The register update is moved to CSID from VFE in CSID
>> gen3.
>> So, adding top_base and reg_update variables in csid
>> device structure for CSID gen3.
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> 

Thanks, will update it.

>>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/camss/camss-csid.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
>> index 4a9e5a2d1f92..ca654b007441 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.h
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
>> @@ -162,7 +162,9 @@ struct csid_device {
>>   	struct v4l2_subdev subdev;
>>   	struct media_pad pads[MSM_CSID_PADS_NUM];
>>   	void __iomem *base;
>> +	void __iomem *top_base;
>>   	u32 irq;
>> +	u32 reg_update;
>>   	char irq_name[30];
> 
> This is pointless. The are no users of this!
> 
> Sorry, don't add random code here or there without concept.
> 

For the old code, they are new concept, since it is new block in the 
hardware, I just want to highlight them.

But thanks for the comment, will update the code based on your comment.

> Best regards,
> Krzysztof
> 

Thanks,
Depeng

[PATCH v2 5/8] media: qcom: camss: Add CSID gen3 driver

[Depeng Shao]

The CSID in SM8550 is gen3, it has new register
offset and new functionality, the buf done irq,
register update and reset is moved to CSID gen3.

Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
---
 drivers/media/platform/qcom/camss/Makefile    |   1 +
 .../platform/qcom/camss/camss-csid-gen3.c     | 639 ++++++++++++++++++
 .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
 .../media/platform/qcom/camss/camss-csid.h    |   1 +
 4 files changed, 667 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

diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
index 28eba4bf3e38..c5fcd6eec0f2 100644
--- a/drivers/media/platform/qcom/camss/Makefile
+++ b/drivers/media/platform/qcom/camss/Makefile
@@ -7,6 +7,7 @@ qcom-camss-objs += \
 		camss-csid-4-1.o \
 		camss-csid-4-7.o \
 		camss-csid-gen2.o \
+		camss-csid-gen3.o \
 		camss-csiphy-2ph-1-0.o \
 		camss-csiphy-3ph-1-0.o \
 		camss-csiphy-2-1-2.o \
diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
new file mode 100644
index 000000000000..b97005f7065d
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
@@ -0,0 +1,639 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * camss-csid-gen3.c
+ *
+ * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module
+ *
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+
+
+#include "camss-csid.h"
+#include "camss-csid-gen3.h"
+#include "camss.h"
+
+
+#define CSID_TOP_IO_PATH_CFG0(csid)	(0x4 * (csid))
+#define		OUTPUT_IFE_EN 0x100
+#define		INTERNAL_CSID 1
+
+#define CSID_HW_VERSION		0x0
+#define		HW_VERSION_STEPPING	0
+#define		HW_VERSION_REVISION	16
+#define		HW_VERSION_GENERATION	28
+
+#define CSID_RST_CFG	0xC
+#define		RST_MODE		0
+#define		RST_LOCATION	4
+
+#define CSID_RST_CMD	0x10
+#define		SELECT_HW_RST	0
+#define		SELECT_SW_RST	1
+#define		SELECT_IRQ_RST	2
+
+#define CSID_CSI2_RX_IRQ_STATUS	0x9C
+#define CSID_CSI2_RX_IRQ_MASK	0xA0
+#define CSID_CSI2_RX_IRQ_CLEAR	0xA4
+#define CSID_CSI2_RX_IRQ_SET	0xA8
+
+#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)		(0xEC + 0x10 * (rdi))
+#define CSID_CSI2_RDIN_IRQ_MASK(rdi)		(0xF0 + 0x10 * (rdi))
+#define		CSID_CSI2_RDIN_INFO_FIFO_FULL 2
+#define		CSID_CSI2_RDIN_INFO_CAMIF_EOF 3
+#define		CSID_CSI2_RDIN_INFO_CAMIF_SOF 4
+#define		CSID_CSI2_RDIN_INFO_INPUT_EOF 9
+#define		CSID_CSI2_RDIN_INFO_INPUT_SOF 12
+#define		CSID_CSI2_RDIN_ERROR_REC_FRAME_DROP 18
+#define		CSID_CSI2_RDIN_ERROR_REC_OVERFLOW_IRQ 19
+#define		CSID_CSI2_RDIN_ERROR_REC_CCIF_VIOLATION 20
+#define		CSID_CSI2_RDIN_EPOCH0 21
+#define		CSID_CSI2_RDIN_EPOCH1 22
+#define		CSID_CSI2_RDIN_RUP_DONE 23
+#define		CSID_CSI2_RDIN_CCIF_VIOLATION 29
+
+#define CSID_CSI2_RDIN_IRQ_CLEAR(rdi)		(0xF4 + 0x10 * (rdi))
+#define CSID_CSI2_RDIN_IRQ_SET(rdi)			(0xF8 + 0x10 * (rdi))
+
+#define CSID_TOP_IRQ_STATUS	0x7C
+#define		TOP_IRQ_STATUS_RESET_DONE 0
+#define CSID_TOP_IRQ_MASK	0x80
+#define CSID_TOP_IRQ_CLEAR	0x84
+#define CSID_TOP_IRQ_SET	0x88
+#define CSID_IRQ_CMD		0x14
+#define		IRQ_CMD_CLEAR	0
+#define		IRQ_CMD_SET	4
+
+#define CSID_BUF_DONE_IRQ_STATUS	0x8C
+#define		BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 1 : 14)
+#define CSID_BUF_DONE_IRQ_MASK	0x90
+#define CSID_BUF_DONE_IRQ_CLEAR	0x94
+#define CSID_BUF_DONE_IRQ_SET	0x98
+
+#define CSID_CSI2_RX_CFG0	0x200
+#define		CSI2_RX_CFG0_NUM_ACTIVE_LANES	0
+#define		CSI2_RX_CFG0_DL0_INPUT_SEL	4
+#define		CSI2_RX_CFG0_DL1_INPUT_SEL	8
+#define		CSI2_RX_CFG0_DL2_INPUT_SEL	12
+#define		CSI2_RX_CFG0_DL3_INPUT_SEL	16
+#define		CSI2_RX_CFG0_PHY_NUM_SEL	20
+#define			CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
+#define		CSI2_RX_CFG0_PHY_TYPE_SEL	24
+#define		CSI2_RX_CFG0_TPG_MUX_EN		27
+#define		CSI2_RX_CFG0_TPG_NUM_SEL	28
+
+#define CSID_CSI2_RX_CFG1	0x204
+#define		CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN		0
+#define		CSI2_RX_CFG1_DE_SCRAMBLE_EN			1
+#define		CSI2_RX_CFG1_VC_MODE				2
+#define		CSI2_RX_CFG1_COMPLETE_STREAM_EN			4
+#define		CSI2_RX_CFG1_COMPLETE_STREAM_FRAME_TIMING	5
+#define		CSI2_RX_CFG1_MISR_EN				6
+#define		CSI2_RX_CFG1_PHY_BIST_EN			7
+#define		CSI2_RX_CFG1_EPD_MODE				8
+#define		CSI2_RX_CFG1_EOTP_EN				9
+#define		CSI2_RX_CFG1_DYN_SWITCH_EN			10
+#define		CSI2_RX_CFG1_RUP_AUP_LATCH_DIS		11
+
+#define CSID_CSI2_RX_CAPTURE_CTRL	0x208
+#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_EN	0
+#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_EN	1
+#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_EN	3
+#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_DT		4
+#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_VC		10
+#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_VC	15
+#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_DT		20
+#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_VC		26
+
+#define CSID_RDI_CFG0(rdi)			(0x500 + 0x100 * (rdi))
+#define		RDI_CFG0_VFR_EN				0
+#define		RDI_CFG0_FRAME_ID_DEC_EN	1
+#define		RDI_CFG0_RETIME_DIS			5
+#define		RDI_CFG0_TIMESTAMP_EN		6
+#define		RDI_CFG0_TIMESTAMP_STB_SEL	8
+#define		RDI_CFG0_DECODE_FORMAT		12
+#define		RDI_CFG0_DT					16
+#define		RDI_CFG0_VC					22
+#define		RDI_CFG0_DT_ID				27
+#define		RDI_CFG0_EN					31
+
+#define CSID_RDI_CFG1(rdi)			(0x510 + 0x100 * (rdi))
+#define		RDI_CFG1_BYTE_CNTR_EN	2
+#define		RDI_CFG1_DROP_H_EN	5
+#define		RDI_CFG1_DROP_V_EN	6
+#define		RDI_CFG1_CROP_H_EN	7
+#define		RDI_CFG1_CROP_V_EN	8
+#define		RDI_CFG1_MISR_EN	9
+#define		RDI_CFG1_PIX_STORE  10
+#define		RDI_CFG1_PLAIN_ALIGNMENT	11
+#define		RDI_CFG1_PLAIN_FORMAT	12
+#define		RDI_CFG1_EARLY_EOF_EN	14
+#define		RDI_CFG1_PACKING_FORMAT	15
+
+#define CSID_RDI_CTRL(rdi)			(0x504 + 0x100 * (rdi))
+#define		RDI_CTRL_START_CMD		0
+#define		RDI_CTRL_START_MODE		2
+
+#define CSID_RDI_EPOCH_IRQ_CFG(rdi) (0x52C + 0x100 * (rdi))
+
+#define CSID_RDI_FRM_DROP_PATTERN(rdi)			(0x540 + 0x100 * (rdi))
+#define CSID_RDI_FRM_DROP_PERIOD(rdi)			(0x544 + 0x100 * (rdi))
+#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi)		(0x548 + 0x100 * (rdi))
+#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi)		(0x54C + 0x100 * (rdi))
+#define CSID_RDI_RPP_PIX_DROP_PATTERN(rdi)		(0x558 + 0x100 * (rdi))
+#define CSID_RDI_RPP_PIX_DROP_PERIOD(rdi)		(0x55C + 0x100 * (rdi))
+#define CSID_RDI_RPP_LINE_DROP_PATTERN(rdi)		(0x560 + 0x100 * (rdi))
+#define CSID_RDI_RPP_LINE_DROP_PERIOD(rdi)		(0x564 + 0x100 * (rdi))
+
+#define CSID_RDI_RPP_HCROP(rdi)                 (0x550 + 0x100 * (rdi))
+#define CSID_RDI_RPP_VCROP(rdi)                 (0x554 + 0x100 * (rdi))
+
+#define CSID_RDI_ERROR_RECOVERY_CFG0(rdi)       (0x514 + 0x100 * (rdi))
+
+#define CSID_DISCARD_FRAMES 4
+
+#define CSID_REG_UPDATE_CMD		0x18
+static inline int reg_update_rdi(struct csid_device *csid, int n)
+{
+	return BIT(n + 4) + BIT(20 + n);
+}
+
+#define	    REG_UPDATE_RDI		reg_update_rdi
+
+static const struct csid_format csid_formats[] = {
+	{
+		MEDIA_BUS_FMT_UYVY8_1X16,
+		DATA_TYPE_YUV422_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		2,
+	},
+	{
+		MEDIA_BUS_FMT_VYUY8_1X16,
+		DATA_TYPE_YUV422_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		2,
+	},
+	{
+		MEDIA_BUS_FMT_YUYV8_1X16,
+		DATA_TYPE_YUV422_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		2,
+	},
+	{
+		MEDIA_BUS_FMT_YVYU8_1X16,
+		DATA_TYPE_YUV422_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		2,
+	},
+	{
+		MEDIA_BUS_FMT_SBGGR8_1X8,
+		DATA_TYPE_RAW_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGBRG8_1X8,
+		DATA_TYPE_RAW_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGRBG8_1X8,
+		DATA_TYPE_RAW_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SRGGB8_1X8,
+		DATA_TYPE_RAW_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SBGGR10_1X10,
+		DATA_TYPE_RAW_10BIT,
+		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
+		10,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGBRG10_1X10,
+		DATA_TYPE_RAW_10BIT,
+		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
+		10,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGRBG10_1X10,
+		DATA_TYPE_RAW_10BIT,
+		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
+		10,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SRGGB10_1X10,
+		DATA_TYPE_RAW_10BIT,
+		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
+		10,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_Y8_1X8,
+		DATA_TYPE_RAW_8BIT,
+		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
+		8,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_Y10_1X10,
+		DATA_TYPE_RAW_10BIT,
+		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
+		10,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SBGGR12_1X12,
+		DATA_TYPE_RAW_12BIT,
+		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
+		12,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGBRG12_1X12,
+		DATA_TYPE_RAW_12BIT,
+		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
+		12,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGRBG12_1X12,
+		DATA_TYPE_RAW_12BIT,
+		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
+		12,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SRGGB12_1X12,
+		DATA_TYPE_RAW_12BIT,
+		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
+		12,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SBGGR14_1X14,
+		DATA_TYPE_RAW_14BIT,
+		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
+		14,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGBRG14_1X14,
+		DATA_TYPE_RAW_14BIT,
+		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
+		14,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SGRBG14_1X14,
+		DATA_TYPE_RAW_14BIT,
+		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
+		14,
+		1,
+	},
+	{
+		MEDIA_BUS_FMT_SRGGB14_1X14,
+		DATA_TYPE_RAW_14BIT,
+		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
+		14,
+		1,
+	},
+};
+
+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;
+	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;
+	writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
+}
+
+static void __csid_configure_top(struct csid_device *csid)
+{
+	u32 val;
+
+	if (csid->top_base) {
+		val = OUTPUT_IFE_EN | INTERNAL_CSID;
+		writel_relaxed(val, csid->top_base + CSID_TOP_IO_PATH_CFG0(csid->id));
+	}
+}
+
+static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 vc)
+{
+	u32 val;
+	u8 lane_cnt = csid->phy.lane_cnt;
+	/* Source pads matching RDI channels on hardware. Pad 1 -> RDI0, Pad 2 -> RDI1, etc. */
+	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + vc];
+	const struct csid_format *format = csid_get_fmt_entry(csid->formats, csid->nformats,
+							      input_format->code);
+
+	if (!lane_cnt)
+		lane_cnt = 4;
+
+	val = 0;
+	writel_relaxed(val, csid->base + CSID_RDI_FRM_DROP_PERIOD(vc));
+
+	/*
+	 * DT_ID is a two bit bitfield that is concatenated with
+	 * the four least significant bits of the five bit VC
+	 * bitfield to generate an internal CID value.
+	 *
+	 * CSID_RDI_CFG0(vc)
+	 * DT_ID : 28:27
+	 * VC    : 26:22
+	 * DT    : 21:16
+	 *
+	 * CID   : VC 3:0 << 2 | DT_ID 1:0
+	 */
+	u8 dt_id = vc & 0x03;
+
+	val = 1 << RDI_CFG0_TIMESTAMP_EN;
+	val |= 2 << RDI_CFG0_TIMESTAMP_STB_SEL;
+	/* note: for non-RDI path, this should be format->decode_format */
+	val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
+	val |= vc << RDI_CFG0_VC;
+	val |= format->data_type << RDI_CFG0_DT;
+	val |= dt_id << RDI_CFG0_DT_ID;
+
+	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
+
+	val = 1 << RDI_CFG1_PACKING_FORMAT;
+	val |= 1 << RDI_CFG1_PIX_STORE;
+	val |= 1 << RDI_CFG1_DROP_H_EN;
+	val |= 1 << RDI_CFG1_DROP_V_EN;
+	val |= 1 << RDI_CFG1_CROP_H_EN;
+	val |= 1 << RDI_CFG1_CROP_V_EN;
+	val |= RDI_CFG1_EARLY_EOF_EN;
+
+	writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
+
+	val = 0;
+	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
+
+	val = 1;
+	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
+
+	val = 0;
+	writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
+
+	val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
+	val |=  enable << RDI_CFG0_EN;
+	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
+}
+
+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)) {
+			__csid_configure_top(csid);
+			__csid_configure_rdi_stream(csid, enable, i);
+			__csid_configure_rx(csid, &csid->phy, i);
+			__csid_ctrl_rdi(csid, enable, i);
+		}
+}
+
+static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
+{
+	if (val > 0 && val <= csid->testgen.nmodes)
+		csid->testgen.mode = val;
+
+	return 0;
+}
+
+/*
+ * csid_hw_version - CSID hardware version query
+ * @csid: CSID device
+ *
+ * Return HW version or error
+ */
+static u32 csid_hw_version(struct csid_device *csid)
+{
+	u32 hw_version;
+	u32 hw_gen;
+	u32 hw_rev;
+	u32 hw_step;
+
+	hw_version = readl_relaxed(csid->base + CSID_HW_VERSION);
+	hw_gen = (hw_version >> HW_VERSION_GENERATION) & 0xF;
+	hw_rev = (hw_version >> HW_VERSION_REVISION) & 0xFFF;
+	hw_step = (hw_version >> HW_VERSION_STEPPING) & 0xFFFF;
+	dev_info(csid->camss->dev, "CSID HW Version = %u.%u.%u\n",
+		hw_gen, hw_rev, hw_step);
+
+	return hw_version;
+}
+
+static void csid_reg_update(struct csid_device *csid, int port_id)
+{
+	csid->reg_update |= REG_UPDATE_RDI(csid, port_id);
+	writel_relaxed(csid->reg_update, csid->base + CSID_REG_UPDATE_CMD);
+}
+
+static inline void csid_reg_update_clear(struct csid_device *csid,
+					int port_id)
+{
+	csid->reg_update &= ~REG_UPDATE_RDI(csid, port_id);
+}
+
+
+/*
+ * csid_isr - CSID module interrupt service routine
+ * @irq: Interrupt line
+ * @dev: CSID device
+ *
+ * Return IRQ_HANDLED on success
+ */
+static irqreturn_t csid_isr(int irq, void *dev)
+{
+	struct csid_device *csid = dev;
+	u32 val, buf_done_val;
+	u8 reset_done;
+	int i;
+
+	val = readl_relaxed(csid->base + CSID_TOP_IRQ_STATUS);
+	writel_relaxed(val, csid->base + CSID_TOP_IRQ_CLEAR);
+	reset_done = val & BIT(TOP_IRQ_STATUS_RESET_DONE);
+
+	val = readl_relaxed(csid->base + CSID_CSI2_RX_IRQ_STATUS);
+	writel_relaxed(val, csid->base + CSID_CSI2_RX_IRQ_CLEAR);
+
+	buf_done_val = readl_relaxed(csid->base + CSID_BUF_DONE_IRQ_STATUS);
+	writel_relaxed(buf_done_val, csid->base + CSID_BUF_DONE_IRQ_CLEAR);
+
+	/* Read and clear IRQ status for each enabled RDI channel */
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i)) {
+			val = readl_relaxed(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
+			writel_relaxed(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
+
+			if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i)) {
+				dev_dbg(csid->camss->dev, "RDI%d BUF DONE", i);
+				v4l2_subdev_notify(&csid->subdev, NOTIFY_BUF_DONE, (void *)&i);
+			}
+		}
+
+	val = 1 << IRQ_CMD_CLEAR;
+	writel_relaxed(val, csid->base + CSID_IRQ_CMD);
+
+	if (reset_done)
+		complete(&csid->reset_complete);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * csid_reset - Trigger reset on CSID module and wait to complete
+ * @csid: CSID device
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int csid_reset(struct csid_device *csid)
+{
+	unsigned long time;
+	u32 val;
+	int i;
+
+	reinit_completion(&csid->reset_complete);
+
+	writel_relaxed(1, csid->base + CSID_TOP_IRQ_CLEAR);
+	writel_relaxed(1, csid->base + CSID_IRQ_CMD);
+	writel_relaxed(1, csid->base + CSID_TOP_IRQ_MASK);
+
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i)) {
+			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
+			writel_relaxed(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
+			writel_relaxed(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+						csid->base + CSID_BUF_DONE_IRQ_MASK);
+		}
+
+	/* preserve registers */
+	val = (0x1 << RST_LOCATION) | (0x1 << RST_MODE);
+	writel_relaxed(val, csid->base + CSID_RST_CFG);
+
+	val = (0x1 << SELECT_HW_RST) | (0x1 << SELECT_IRQ_RST);
+	writel_relaxed(val, csid->base + CSID_RST_CMD);
+
+	time = wait_for_completion_timeout(&csid->reset_complete,
+					   msecs_to_jiffies(CSID_RESET_TIMEOUT_MS));
+	if (!time) {
+		dev_err(csid->camss->dev, "CSID reset timeout\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+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;
+	}
+}
+
+static void csid_subdev_init(struct csid_device *csid)
+{
+	csid->formats = csid_formats;
+	csid->nformats = ARRAY_SIZE(csid_formats);
+	csid->testgen.modes = csid_testgen_modes;
+	csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
+}
+
+static void csid_subdev_event(struct csid_device *csid, unsigned int evt_type, void *arg)
+{
+	int evt_data = *(int *)arg;
+
+	switch (evt_type) {
+	case NOTIFY_RUP:
+		csid_reg_update(csid, evt_data);
+		break;
+	case NOTIFY_RUP_CLEAR:
+		csid_reg_update_clear(csid, evt_data);
+		break;
+	default:
+		dev_err(csid->camss->dev, "NOT Supported EVT Type\n");
+		break;
+	}
+}
+
+const struct csid_hw_ops csid_ops_gen3 = {
+	.configure_stream = csid_configure_stream,
+	.configure_testgen_pattern = csid_configure_testgen_pattern,
+	.hw_version = csid_hw_version,
+	.isr = csid_isr,
+	.reset = csid_reset,
+	.src_pad_code = csid_src_pad_code,
+	.subdev_init = csid_subdev_init,
+	.event = csid_subdev_event,
+};
diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.h b/drivers/media/platform/qcom/camss/camss-csid-gen3.h
new file mode 100644
index 000000000000..aa5bd5f2fec5
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * camss-csid-gen3.h
+ *
+ * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module Generation 1
+ *
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#ifndef QC_MSM_CAMSS_CSID_GEN3_H
+#define QC_MSM_CAMSS_CSID_GEN3_H
+
+#define DECODE_FORMAT_UNCOMPRESSED_8_BIT	0x1
+#define DECODE_FORMAT_UNCOMPRESSED_10_BIT	0x2
+#define DECODE_FORMAT_UNCOMPRESSED_12_BIT	0x3
+#define DECODE_FORMAT_UNCOMPRESSED_14_BIT	0x4
+#define DECODE_FORMAT_UNCOMPRESSED_16_BIT	0x5
+#define DECODE_FORMAT_UNCOMPRESSED_20_BIT	0x6
+#define DECODE_FORMAT_UNCOMPRESSED_24_BIT	0x7
+#define DECODE_FORMAT_PAYLOAD_ONLY		0xf
+
+
+#define PLAIN_FORMAT_PLAIN8	0x0 /* supports DPCM, UNCOMPRESSED_6/8_BIT */
+#define PLAIN_FORMAT_PLAIN16	0x1 /* supports DPCM, UNCOMPRESSED_10/16_BIT */
+#define PLAIN_FORMAT_PLAIN32	0x2 /* supports UNCOMPRESSED_20_BIT */
+
+#endif /* QC_MSM_CAMSS_CSID_GEN3_H */
diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index ca654b007441..c1d061f91b78 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -223,6 +223,7 @@ extern const char * const csid_testgen_modes[];
 extern const struct csid_hw_ops csid_ops_4_1;
 extern const struct csid_hw_ops csid_ops_4_7;
 extern const struct csid_hw_ops csid_ops_gen2;
+extern const struct csid_hw_ops csid_ops_gen3;
 
 /*
  * csid_is_lite - Check if CSID is CSID lite.
-- 
2.17.1

Re: [PATCH v2 5/8] media: qcom: camss: Add CSID gen3 driver

[Bryan O'Donoghue]

On 20/03/2024 14:11, 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 is moved to CSID gen3.
> 
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>   .../platform/qcom/camss/camss-csid-gen3.c     | 639 ++++++++++++++++++
>   .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
>   .../media/platform/qcom/camss/camss-csid.h    |   1 +
>   4 files changed, 667 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
> 
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index 28eba4bf3e38..c5fcd6eec0f2 100644
> --- a/drivers/media/platform/qcom/camss/Makefile
> +++ b/drivers/media/platform/qcom/camss/Makefile
> @@ -7,6 +7,7 @@ qcom-camss-objs += \
>   		camss-csid-4-1.o \
>   		camss-csid-4-7.o \
>   		camss-csid-gen2.o \
> +		camss-csid-gen3.o \
>   		camss-csiphy-2ph-1-0.o \
>   		camss-csiphy-3ph-1-0.o \
>   		camss-csiphy-2-1-2.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> new file mode 100644
> index 000000000000..b97005f7065d
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> @@ -0,0 +1,639 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-csid-gen3.c
> + *
> + * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module
> + *
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/delay.h>

Please alphanumerically sort your include list.

> +
> +
> +#include "camss-csid.h"
> +#include "camss-csid-gen3.h"
> +#include "camss.h"

and here.

> +
> +#define CSID_TOP_IO_PATH_CFG0(csid)	(0x4 * (csid))
> +#define		OUTPUT_IFE_EN 0x100
> +#define		INTERNAL_CSID 1
> +
> +#define CSID_HW_VERSION		0x0
> +#define		HW_VERSION_STEPPING	0
> +#define		HW_VERSION_REVISION	16
> +#define		HW_VERSION_GENERATION	28
> +
> +#define CSID_RST_CFG	0xC
> +#define		RST_MODE		0
> +#define		RST_LOCATION	4
> +
> +#define CSID_RST_CMD	0x10
> +#define		SELECT_HW_RST	0
> +#define		SELECT_SW_RST	1
> +#define		SELECT_IRQ_RST	2
> +
> +#define CSID_CSI2_RX_IRQ_STATUS	0x9C
> +#define CSID_CSI2_RX_IRQ_MASK	0xA0
> +#define CSID_CSI2_RX_IRQ_CLEAR	0xA4
> +#define CSID_CSI2_RX_IRQ_SET	0xA8
> +
> +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)		(0xEC + 0x10 * (rdi))
> +#define CSID_CSI2_RDIN_IRQ_MASK(rdi)		(0xF0 + 0x10 * (rdi))
> +#define		CSID_CSI2_RDIN_INFO_FIFO_FULL 2
> +#define		CSID_CSI2_RDIN_INFO_CAMIF_EOF 3
> +#define		CSID_CSI2_RDIN_INFO_CAMIF_SOF 4
> +#define		CSID_CSI2_RDIN_INFO_INPUT_EOF 9
> +#define		CSID_CSI2_RDIN_INFO_INPUT_SOF 12
> +#define		CSID_CSI2_RDIN_ERROR_REC_FRAME_DROP 18
> +#define		CSID_CSI2_RDIN_ERROR_REC_OVERFLOW_IRQ 19
> +#define		CSID_CSI2_RDIN_ERROR_REC_CCIF_VIOLATION 20
> +#define		CSID_CSI2_RDIN_EPOCH0 21
> +#define		CSID_CSI2_RDIN_EPOCH1 22
> +#define		CSID_CSI2_RDIN_RUP_DONE 23
> +#define		CSID_CSI2_RDIN_CCIF_VIOLATION 29
> +
> +#define CSID_CSI2_RDIN_IRQ_CLEAR(rdi)		(0xF4 + 0x10 * (rdi))
> +#define CSID_CSI2_RDIN_IRQ_SET(rdi)			(0xF8 + 0x10 * (rdi))
> +
> +#define CSID_TOP_IRQ_STATUS	0x7C
> +#define		TOP_IRQ_STATUS_RESET_DONE 0
> +#define CSID_TOP_IRQ_MASK	0x80
> +#define CSID_TOP_IRQ_CLEAR	0x84
> +#define CSID_TOP_IRQ_SET	0x88
> +#define CSID_IRQ_CMD		0x14
> +#define		IRQ_CMD_CLEAR	0
> +#define		IRQ_CMD_SET	4
> +
> +#define CSID_BUF_DONE_IRQ_STATUS	0x8C
> +#define		BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 1 : 14)

Stick with hex please 0x01 : 0x0e

> +#define CSID_BUF_DONE_IRQ_MASK	0x90
> +#define CSID_BUF_DONE_IRQ_CLEAR	0x94
> +#define CSID_BUF_DONE_IRQ_SET	0x98
> +
> +#define CSID_CSI2_RX_CFG0	0x200
> +#define		CSI2_RX_CFG0_NUM_ACTIVE_LANES	0
> +#define		CSI2_RX_CFG0_DL0_INPUT_SEL	4
> +#define		CSI2_RX_CFG0_DL1_INPUT_SEL	8
> +#define		CSI2_RX_CFG0_DL2_INPUT_SEL	12
> +#define		CSI2_RX_CFG0_DL3_INPUT_SEL	16
> +#define		CSI2_RX_CFG0_PHY_NUM_SEL	20
> +#define			CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
> +#define		CSI2_RX_CFG0_PHY_TYPE_SEL	24
> +#define		CSI2_RX_CFG0_TPG_MUX_EN		27
> +#define		CSI2_RX_CFG0_TPG_NUM_SEL	28
> +
> +#define CSID_CSI2_RX_CFG1	0x204
> +#define		CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN		0
> +#define		CSI2_RX_CFG1_DE_SCRAMBLE_EN			1
> +#define		CSI2_RX_CFG1_VC_MODE				2
> +#define		CSI2_RX_CFG1_COMPLETE_STREAM_EN			4
> +#define		CSI2_RX_CFG1_COMPLETE_STREAM_FRAME_TIMING	5
> +#define		CSI2_RX_CFG1_MISR_EN				6
> +#define		CSI2_RX_CFG1_PHY_BIST_EN			7
> +#define		CSI2_RX_CFG1_EPD_MODE				8
> +#define		CSI2_RX_CFG1_EOTP_EN				9
> +#define		CSI2_RX_CFG1_DYN_SWITCH_EN			10
> +#define		CSI2_RX_CFG1_RUP_AUP_LATCH_DIS		11
> +
> +#define CSID_CSI2_RX_CAPTURE_CTRL	0x208
> +#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_EN	0
> +#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_EN	1
> +#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_EN	3
> +#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_DT		4
> +#define		CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_VC		10
> +#define		CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_VC	15
> +#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_DT		20
> +#define		CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_VC		26
> +
> +#define CSID_RDI_CFG0(rdi)			(0x500 + 0x100 * (rdi))
> +#define		RDI_CFG0_VFR_EN				0
> +#define		RDI_CFG0_FRAME_ID_DEC_EN	1
> +#define		RDI_CFG0_RETIME_DIS			5
> +#define		RDI_CFG0_TIMESTAMP_EN		6
> +#define		RDI_CFG0_TIMESTAMP_STB_SEL	8
> +#define		RDI_CFG0_DECODE_FORMAT		12
> +#define		RDI_CFG0_DT					16
> +#define		RDI_CFG0_VC					22
> +#define		RDI_CFG0_DT_ID				27
> +#define		RDI_CFG0_EN					31
> +
> +#define CSID_RDI_CFG1(rdi)			(0x510 + 0x100 * (rdi))
> +#define		RDI_CFG1_BYTE_CNTR_EN	2
> +#define		RDI_CFG1_DROP_H_EN	5
> +#define		RDI_CFG1_DROP_V_EN	6
> +#define		RDI_CFG1_CROP_H_EN	7
> +#define		RDI_CFG1_CROP_V_EN	8
> +#define		RDI_CFG1_MISR_EN	9
> +#define		RDI_CFG1_PIX_STORE  10
> +#define		RDI_CFG1_PLAIN_ALIGNMENT	11
> +#define		RDI_CFG1_PLAIN_FORMAT	12
> +#define		RDI_CFG1_EARLY_EOF_EN	14
> +#define		RDI_CFG1_PACKING_FORMAT	15
> +
> +#define CSID_RDI_CTRL(rdi)			(0x504 + 0x100 * (rdi))
> +#define		RDI_CTRL_START_CMD		0
> +#define		RDI_CTRL_START_MODE		2
> +
> +#define CSID_RDI_EPOCH_IRQ_CFG(rdi) (0x52C + 0x100 * (rdi))
> +
> +#define CSID_RDI_FRM_DROP_PATTERN(rdi)			(0x540 + 0x100 * (rdi))
> +#define CSID_RDI_FRM_DROP_PERIOD(rdi)			(0x544 + 0x100 * (rdi))
> +#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi)		(0x548 + 0x100 * (rdi))
> +#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi)		(0x54C + 0x100 * (rdi))
> +#define CSID_RDI_RPP_PIX_DROP_PATTERN(rdi)		(0x558 + 0x100 * (rdi))
> +#define CSID_RDI_RPP_PIX_DROP_PERIOD(rdi)		(0x55C + 0x100 * (rdi))
> +#define CSID_RDI_RPP_LINE_DROP_PATTERN(rdi)		(0x560 + 0x100 * (rdi))
> +#define CSID_RDI_RPP_LINE_DROP_PERIOD(rdi)		(0x564 + 0x100 * (rdi))
> +
> +#define CSID_RDI_RPP_HCROP(rdi)                 (0x550 + 0x100 * (rdi))
> +#define CSID_RDI_RPP_VCROP(rdi)                 (0x554 + 0x100 * (rdi))
> +
> +#define CSID_RDI_ERROR_RECOVERY_CFG0(rdi)       (0x514 + 0x100 * (rdi))
> +
> +#define CSID_DISCARD_FRAMES 4
> +
> +#define CSID_REG_UPDATE_CMD		0x18
> +static inline int reg_update_rdi(struct csid_device *csid, int n)
> +{
> +	return BIT(n + 4) + BIT(20 + n);
> +}
> +
> +#define	    REG_UPDATE_RDI		reg_update_rdi
> +
> +static const struct csid_format csid_formats[] = {
> +	{
> +		MEDIA_BUS_FMT_UYVY8_1X16,
> +		DATA_TYPE_YUV422_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		2,
> +	},
> +	{
> +		MEDIA_BUS_FMT_VYUY8_1X16,
> +		DATA_TYPE_YUV422_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		2,
> +	},
> +	{
> +		MEDIA_BUS_FMT_YUYV8_1X16,
> +		DATA_TYPE_YUV422_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		2,
> +	},
> +	{
> +		MEDIA_BUS_FMT_YVYU8_1X16,
> +		DATA_TYPE_YUV422_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		2,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SBGGR8_1X8,
> +		DATA_TYPE_RAW_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGBRG8_1X8,
> +		DATA_TYPE_RAW_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGRBG8_1X8,
> +		DATA_TYPE_RAW_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SRGGB8_1X8,
> +		DATA_TYPE_RAW_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SBGGR10_1X10,
> +		DATA_TYPE_RAW_10BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +		10,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGBRG10_1X10,
> +		DATA_TYPE_RAW_10BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +		10,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGRBG10_1X10,
> +		DATA_TYPE_RAW_10BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +		10,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SRGGB10_1X10,
> +		DATA_TYPE_RAW_10BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +		10,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_Y8_1X8,
> +		DATA_TYPE_RAW_8BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +		8,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_Y10_1X10,
> +		DATA_TYPE_RAW_10BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +		10,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SBGGR12_1X12,
> +		DATA_TYPE_RAW_12BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +		12,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGBRG12_1X12,
> +		DATA_TYPE_RAW_12BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +		12,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGRBG12_1X12,
> +		DATA_TYPE_RAW_12BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +		12,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SRGGB12_1X12,
> +		DATA_TYPE_RAW_12BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +		12,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SBGGR14_1X14,
> +		DATA_TYPE_RAW_14BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +		14,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGBRG14_1X14,
> +		DATA_TYPE_RAW_14BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +		14,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SGRBG14_1X14,
> +		DATA_TYPE_RAW_14BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +		14,
> +		1,
> +	},
> +	{
> +		MEDIA_BUS_FMT_SRGGB14_1X14,
> +		DATA_TYPE_RAW_14BIT,
> +		DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +		14,
> +		1,
> +	},
> +};
> +
> +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;
> +	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;
> +	writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
> +}
> +
> +static void __csid_configure_top(struct csid_device *csid)
> +{
> +	u32 val;
> +
> +	if (csid->top_base) {
> +		val = OUTPUT_IFE_EN | INTERNAL_CSID;
> +		writel_relaxed(val, csid->top_base + CSID_TOP_IO_PATH_CFG0(csid->id));
> +	}
> +}
> +
> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 vc)
> +{
> +	u32 val;
> +	u8 lane_cnt = csid->phy.lane_cnt;
> +	/* Source pads matching RDI channels on hardware. Pad 1 -> RDI0, Pad 2 -> RDI1, etc. */
> +	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + vc];
> +	const struct csid_format *format = csid_get_fmt_entry(csid->formats, csid->nformats,
> +							      input_format->code);
> +
> +	if (!lane_cnt)
> +		lane_cnt = 4;
> +
> +	val = 0;
> +	writel_relaxed(val, csid->base + CSID_RDI_FRM_DROP_PERIOD(vc));
> +
> +	/*
> +	 * DT_ID is a two bit bitfield that is concatenated with
> +	 * the four least significant bits of the five bit VC
> +	 * bitfield to generate an internal CID value.
> +	 *
> +	 * CSID_RDI_CFG0(vc)
> +	 * DT_ID : 28:27
> +	 * VC    : 26:22
> +	 * DT    : 21:16
> +	 *
> +	 * CID   : VC 3:0 << 2 | DT_ID 1:0
> +	 */
> +	u8 dt_id = vc & 0x03;
> +
> +	val = 1 << RDI_CFG0_TIMESTAMP_EN;
> +	val |= 2 << RDI_CFG0_TIMESTAMP_STB_SEL;
> +	/* note: for non-RDI path, this should be format->decode_format */
> +	val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
> +	val |= vc << RDI_CFG0_VC;
> +	val |= format->data_type << RDI_CFG0_DT;
> +	val |= dt_id << RDI_CFG0_DT_ID;
> +
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
> +
> +	val = 1 << RDI_CFG1_PACKING_FORMAT;
> +	val |= 1 << RDI_CFG1_PIX_STORE;
> +	val |= 1 << RDI_CFG1_DROP_H_EN;
> +	val |= 1 << RDI_CFG1_DROP_V_EN;
> +	val |= 1 << RDI_CFG1_CROP_H_EN;
> +	val |= 1 << RDI_CFG1_CROP_V_EN;
> +	val |= RDI_CFG1_EARLY_EOF_EN;
> +
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
> +
> +	val = 0;
> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
> +
> +	val = 1;
> +	writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
> +
> +	val = 0;
> +	writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
> +
> +	val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
> +	val |=  enable << RDI_CFG0_EN;
> +	writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
> +}

Hmm.

So I think 90% of the code here can be moved into a shared file - 
ideally instantiated in gen2.c and then reused here rather than 
copy/paste from one file to the other.

Lets sync up and try to get a unified tree for x1e80100 and sm8550 for 
that purpose.

I think the code you have here is slightly further along that the 
CSID/VFE stuff I've been working with.

But still - reductio ad absurdum - we need to functionally decompose and 
not replicate code.

> +
> +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)) {
> +			__csid_configure_top(csid);
> +			__csid_configure_rdi_stream(csid, enable, i);
> +			__csid_configure_rx(csid, &csid->phy, i);
> +			__csid_ctrl_rdi(csid, enable, i);
> +		}
> +}

Another example, straight copy/paste - we need to zap all of that.

---
bod

Re: [PATCH v2 5/8] media: qcom: camss: Add CSID gen3 driver

[Bryan O'Donoghue]

On 20/03/2024 15:40, Bryan O'Donoghue wrote:
> +static const struct csid_format csid_formats[] = {
> +    {
> +        MEDIA_BUS_FMT_UYVY8_1X16,
> +        DATA_TYPE_YUV422_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        2,
> +    },
> +    {
> +        MEDIA_BUS_FMT_VYUY8_1X16,
> +        DATA_TYPE_YUV422_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        2,
> +    },
> +    {
> +        MEDIA_BUS_FMT_YUYV8_1X16,
> +        DATA_TYPE_YUV422_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        2,
> +    },
> +    {
> +        MEDIA_BUS_FMT_YVYU8_1X16,
> +        DATA_TYPE_YUV422_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        2,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SBGGR8_1X8,
> +        DATA_TYPE_RAW_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGBRG8_1X8,
> +        DATA_TYPE_RAW_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGRBG8_1X8,
> +        DATA_TYPE_RAW_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SRGGB8_1X8,
> +        DATA_TYPE_RAW_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SBGGR10_1X10,
> +        DATA_TYPE_RAW_10BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +        10,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGBRG10_1X10,
> +        DATA_TYPE_RAW_10BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +        10,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGRBG10_1X10,
> +        DATA_TYPE_RAW_10BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +        10,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SRGGB10_1X10,
> +        DATA_TYPE_RAW_10BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +        10,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_Y8_1X8,
> +        DATA_TYPE_RAW_8BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +        8,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_Y10_1X10,
> +        DATA_TYPE_RAW_10BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
> +        10,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SBGGR12_1X12,
> +        DATA_TYPE_RAW_12BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +        12,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGBRG12_1X12,
> +        DATA_TYPE_RAW_12BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +        12,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGRBG12_1X12,
> +        DATA_TYPE_RAW_12BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +        12,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SRGGB12_1X12,
> +        DATA_TYPE_RAW_12BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
> +        12,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SBGGR14_1X14,
> +        DATA_TYPE_RAW_14BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +        14,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGBRG14_1X14,
> +        DATA_TYPE_RAW_14BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +        14,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SGRBG14_1X14,
> +        DATA_TYPE_RAW_14BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +        14,
> +        1,
> +    },
> +    {
> +        MEDIA_BUS_FMT_SRGGB14_1X14,
> +        DATA_TYPE_RAW_14BIT,
> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
> +        14,
> +        1,
> +    },
> +};

Also please consider including/reviewing Gjorgji's patchset which 
reworks the declaration of resources.

https://lore.kernel.org/lkml/20240319173935.481-4-quic_grosikop@quicinc.com/T/

---
bod

Re: [PATCH v2 5/8] media: qcom: camss: Add CSID gen3 driver

[Depeng Shao]

Hi Bryan,

On 3/20/2024 11:40 PM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, 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 is moved to CSID gen3.
>>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>>   .../platform/qcom/camss/camss-csid-gen3.c     | 639 ++++++++++++++++++
>>   .../platform/qcom/camss/camss-csid-gen3.h     |  26 +
>>   .../media/platform/qcom/camss/camss-csid.h    |   1 +
>>   4 files changed, 667 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
>>
>> diff --git a/drivers/media/platform/qcom/camss/Makefile 
>> b/drivers/media/platform/qcom/camss/Makefile
>> index 28eba4bf3e38..c5fcd6eec0f2 100644
>> --- a/drivers/media/platform/qcom/camss/Makefile
>> +++ b/drivers/media/platform/qcom/camss/Makefile
>> @@ -7,6 +7,7 @@ qcom-camss-objs += \
>>           camss-csid-4-1.o \
>>           camss-csid-4-7.o \
>>           camss-csid-gen2.o \
>> +        camss-csid-gen3.o \
>>           camss-csiphy-2ph-1-0.o \
>>           camss-csiphy-3ph-1-0.o \
>>           camss-csiphy-2-1-2.o \
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c 
>> b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>> new file mode 100644
>> index 000000000000..b97005f7065d
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>> @@ -0,0 +1,639 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * camss-csid-gen3.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - CSID (CSI Decoder) Module
>> + *
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +#include <linux/completion.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of.h>
>> +#include <linux/delay.h>
> 
> Please alphanumerically sort your include list.
> 
>> +
>> +
>> +#include "camss-csid.h"
>> +#include "camss-csid-gen3.h"
>> +#include "camss.h"
> 
> and here.
> 
>> +
>> +#define CSID_TOP_IO_PATH_CFG0(csid)    (0x4 * (csid))
>> +#define        OUTPUT_IFE_EN 0x100
>> +#define        INTERNAL_CSID 1
>> +
>> +#define CSID_HW_VERSION        0x0
>> +#define        HW_VERSION_STEPPING    0
>> +#define        HW_VERSION_REVISION    16
>> +#define        HW_VERSION_GENERATION    28
>> +
>> +#define CSID_RST_CFG    0xC
>> +#define        RST_MODE        0
>> +#define        RST_LOCATION    4
>> +
>> +#define CSID_RST_CMD    0x10
>> +#define        SELECT_HW_RST    0
>> +#define        SELECT_SW_RST    1
>> +#define        SELECT_IRQ_RST    2
>> +
>> +#define CSID_CSI2_RX_IRQ_STATUS    0x9C
>> +#define CSID_CSI2_RX_IRQ_MASK    0xA0
>> +#define CSID_CSI2_RX_IRQ_CLEAR    0xA4
>> +#define CSID_CSI2_RX_IRQ_SET    0xA8
>> +
>> +#define CSID_CSI2_RDIN_IRQ_STATUS(rdi)        (0xEC + 0x10 * (rdi))
>> +#define CSID_CSI2_RDIN_IRQ_MASK(rdi)        (0xF0 + 0x10 * (rdi))
>> +#define        CSID_CSI2_RDIN_INFO_FIFO_FULL 2
>> +#define        CSID_CSI2_RDIN_INFO_CAMIF_EOF 3
>> +#define        CSID_CSI2_RDIN_INFO_CAMIF_SOF 4
>> +#define        CSID_CSI2_RDIN_INFO_INPUT_EOF 9
>> +#define        CSID_CSI2_RDIN_INFO_INPUT_SOF 12
>> +#define        CSID_CSI2_RDIN_ERROR_REC_FRAME_DROP 18
>> +#define        CSID_CSI2_RDIN_ERROR_REC_OVERFLOW_IRQ 19
>> +#define        CSID_CSI2_RDIN_ERROR_REC_CCIF_VIOLATION 20
>> +#define        CSID_CSI2_RDIN_EPOCH0 21
>> +#define        CSID_CSI2_RDIN_EPOCH1 22
>> +#define        CSID_CSI2_RDIN_RUP_DONE 23
>> +#define        CSID_CSI2_RDIN_CCIF_VIOLATION 29
>> +
>> +#define CSID_CSI2_RDIN_IRQ_CLEAR(rdi)        (0xF4 + 0x10 * (rdi))
>> +#define CSID_CSI2_RDIN_IRQ_SET(rdi)            (0xF8 + 0x10 * (rdi))
>> +
>> +#define CSID_TOP_IRQ_STATUS    0x7C
>> +#define        TOP_IRQ_STATUS_RESET_DONE 0
>> +#define CSID_TOP_IRQ_MASK    0x80
>> +#define CSID_TOP_IRQ_CLEAR    0x84
>> +#define CSID_TOP_IRQ_SET    0x88
>> +#define CSID_IRQ_CMD        0x14
>> +#define        IRQ_CMD_CLEAR    0
>> +#define        IRQ_CMD_SET    4
>> +
>> +#define CSID_BUF_DONE_IRQ_STATUS    0x8C
>> +#define        BUF_DONE_IRQ_STATUS_RDI_OFFSET (csid_is_lite(csid) ? 1 
>> : 14)
> 
> Stick with hex please 0x01 : 0x0e
> 
>> +#define CSID_BUF_DONE_IRQ_MASK    0x90
>> +#define CSID_BUF_DONE_IRQ_CLEAR    0x94
>> +#define CSID_BUF_DONE_IRQ_SET    0x98
>> +
>> +#define CSID_CSI2_RX_CFG0    0x200
>> +#define        CSI2_RX_CFG0_NUM_ACTIVE_LANES    0
>> +#define        CSI2_RX_CFG0_DL0_INPUT_SEL    4
>> +#define        CSI2_RX_CFG0_DL1_INPUT_SEL    8
>> +#define        CSI2_RX_CFG0_DL2_INPUT_SEL    12
>> +#define        CSI2_RX_CFG0_DL3_INPUT_SEL    16
>> +#define        CSI2_RX_CFG0_PHY_NUM_SEL    20
>> +#define            CSI2_RX_CFG0_PHY_SEL_BASE_IDX 1
>> +#define        CSI2_RX_CFG0_PHY_TYPE_SEL    24
>> +#define        CSI2_RX_CFG0_TPG_MUX_EN        27
>> +#define        CSI2_RX_CFG0_TPG_NUM_SEL    28
>> +
>> +#define CSID_CSI2_RX_CFG1    0x204
>> +#define        CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN        0
>> +#define        CSI2_RX_CFG1_DE_SCRAMBLE_EN            1
>> +#define        CSI2_RX_CFG1_VC_MODE                2
>> +#define        CSI2_RX_CFG1_COMPLETE_STREAM_EN            4
>> +#define        CSI2_RX_CFG1_COMPLETE_STREAM_FRAME_TIMING    5
>> +#define        CSI2_RX_CFG1_MISR_EN                6
>> +#define        CSI2_RX_CFG1_PHY_BIST_EN            7
>> +#define        CSI2_RX_CFG1_EPD_MODE                8
>> +#define        CSI2_RX_CFG1_EOTP_EN                9
>> +#define        CSI2_RX_CFG1_DYN_SWITCH_EN            10
>> +#define        CSI2_RX_CFG1_RUP_AUP_LATCH_DIS        11
>> +
>> +#define CSID_CSI2_RX_CAPTURE_CTRL    0x208
>> +#define        CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_EN    0
>> +#define        CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_EN    1
>> +#define        CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_EN    3
>> +#define        CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_DT        4
>> +#define        CSI2_RX_CAPTURE_CTRL_LONG_PKT_CAPTURE_VC        10
>> +#define        CSI2_RX_CAPTURE_CTRL_SHORT_PKT_CAPTURE_VC    15
>> +#define        CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_DT        20
>> +#define        CSI2_RX_CAPTURE_CTRL_CPHY_PKT_CAPTURE_VC        26
>> +
>> +#define CSID_RDI_CFG0(rdi)            (0x500 + 0x100 * (rdi))
>> +#define        RDI_CFG0_VFR_EN                0
>> +#define        RDI_CFG0_FRAME_ID_DEC_EN    1
>> +#define        RDI_CFG0_RETIME_DIS            5
>> +#define        RDI_CFG0_TIMESTAMP_EN        6
>> +#define        RDI_CFG0_TIMESTAMP_STB_SEL    8
>> +#define        RDI_CFG0_DECODE_FORMAT        12
>> +#define        RDI_CFG0_DT                    16
>> +#define        RDI_CFG0_VC                    22
>> +#define        RDI_CFG0_DT_ID                27
>> +#define        RDI_CFG0_EN                    31
>> +
>> +#define CSID_RDI_CFG1(rdi)            (0x510 + 0x100 * (rdi))
>> +#define        RDI_CFG1_BYTE_CNTR_EN    2
>> +#define        RDI_CFG1_DROP_H_EN    5
>> +#define        RDI_CFG1_DROP_V_EN    6
>> +#define        RDI_CFG1_CROP_H_EN    7
>> +#define        RDI_CFG1_CROP_V_EN    8
>> +#define        RDI_CFG1_MISR_EN    9
>> +#define        RDI_CFG1_PIX_STORE  10
>> +#define        RDI_CFG1_PLAIN_ALIGNMENT    11
>> +#define        RDI_CFG1_PLAIN_FORMAT    12
>> +#define        RDI_CFG1_EARLY_EOF_EN    14
>> +#define        RDI_CFG1_PACKING_FORMAT    15
>> +
>> +#define CSID_RDI_CTRL(rdi)            (0x504 + 0x100 * (rdi))
>> +#define        RDI_CTRL_START_CMD        0
>> +#define        RDI_CTRL_START_MODE        2
>> +
>> +#define CSID_RDI_EPOCH_IRQ_CFG(rdi) (0x52C + 0x100 * (rdi))
>> +
>> +#define CSID_RDI_FRM_DROP_PATTERN(rdi)            (0x540 + 0x100 * 
>> (rdi))
>> +#define CSID_RDI_FRM_DROP_PERIOD(rdi)            (0x544 + 0x100 * (rdi))
>> +#define CSID_RDI_IRQ_SUBSAMPLE_PATTERN(rdi)        (0x548 + 0x100 * 
>> (rdi))
>> +#define CSID_RDI_IRQ_SUBSAMPLE_PERIOD(rdi)        (0x54C + 0x100 * 
>> (rdi))
>> +#define CSID_RDI_RPP_PIX_DROP_PATTERN(rdi)        (0x558 + 0x100 * 
>> (rdi))
>> +#define CSID_RDI_RPP_PIX_DROP_PERIOD(rdi)        (0x55C + 0x100 * (rdi))
>> +#define CSID_RDI_RPP_LINE_DROP_PATTERN(rdi)        (0x560 + 0x100 * 
>> (rdi))
>> +#define CSID_RDI_RPP_LINE_DROP_PERIOD(rdi)        (0x564 + 0x100 * 
>> (rdi))
>> +
>> +#define CSID_RDI_RPP_HCROP(rdi)                 (0x550 + 0x100 * (rdi))
>> +#define CSID_RDI_RPP_VCROP(rdi)                 (0x554 + 0x100 * (rdi))
>> +
>> +#define CSID_RDI_ERROR_RECOVERY_CFG0(rdi)       (0x514 + 0x100 * (rdi))
>> +
>> +#define CSID_DISCARD_FRAMES 4
>> +
>> +#define CSID_REG_UPDATE_CMD        0x18
>> +static inline int reg_update_rdi(struct csid_device *csid, int n)
>> +{
>> +    return BIT(n + 4) + BIT(20 + n);
>> +}
>> +
>> +#define        REG_UPDATE_RDI        reg_update_rdi
>> +
>> +static const struct csid_format csid_formats[] = {
>> +    {
>> +        MEDIA_BUS_FMT_UYVY8_1X16,
>> +        DATA_TYPE_YUV422_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        2,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_VYUY8_1X16,
>> +        DATA_TYPE_YUV422_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        2,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_YUYV8_1X16,
>> +        DATA_TYPE_YUV422_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        2,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_YVYU8_1X16,
>> +        DATA_TYPE_YUV422_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        2,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SBGGR8_1X8,
>> +        DATA_TYPE_RAW_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGBRG8_1X8,
>> +        DATA_TYPE_RAW_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGRBG8_1X8,
>> +        DATA_TYPE_RAW_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SRGGB8_1X8,
>> +        DATA_TYPE_RAW_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SBGGR10_1X10,
>> +        DATA_TYPE_RAW_10BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
>> +        10,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGBRG10_1X10,
>> +        DATA_TYPE_RAW_10BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
>> +        10,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGRBG10_1X10,
>> +        DATA_TYPE_RAW_10BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
>> +        10,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SRGGB10_1X10,
>> +        DATA_TYPE_RAW_10BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
>> +        10,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_Y8_1X8,
>> +        DATA_TYPE_RAW_8BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_8_BIT,
>> +        8,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_Y10_1X10,
>> +        DATA_TYPE_RAW_10BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_10_BIT,
>> +        10,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SBGGR12_1X12,
>> +        DATA_TYPE_RAW_12BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
>> +        12,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGBRG12_1X12,
>> +        DATA_TYPE_RAW_12BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
>> +        12,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGRBG12_1X12,
>> +        DATA_TYPE_RAW_12BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
>> +        12,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SRGGB12_1X12,
>> +        DATA_TYPE_RAW_12BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_12_BIT,
>> +        12,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SBGGR14_1X14,
>> +        DATA_TYPE_RAW_14BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
>> +        14,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGBRG14_1X14,
>> +        DATA_TYPE_RAW_14BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
>> +        14,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SGRBG14_1X14,
>> +        DATA_TYPE_RAW_14BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
>> +        14,
>> +        1,
>> +    },
>> +    {
>> +        MEDIA_BUS_FMT_SRGGB14_1X14,
>> +        DATA_TYPE_RAW_14BIT,
>> +        DECODE_FORMAT_UNCOMPRESSED_14_BIT,
>> +        14,
>> +        1,
>> +    },
>> +};
>> +
>> +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;
>> +    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;
>> +    writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
>> +}
>> +
>> +static void __csid_configure_top(struct csid_device *csid)
>> +{
>> +    u32 val;
>> +
>> +    if (csid->top_base) {
>> +        val = OUTPUT_IFE_EN | INTERNAL_CSID;
>> +        writel_relaxed(val, csid->top_base + 
>> CSID_TOP_IO_PATH_CFG0(csid->id));
>> +    }
>> +}
>> +
>> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 
>> enable, u8 vc)
>> +{
>> +    u32 val;
>> +    u8 lane_cnt = csid->phy.lane_cnt;
>> +    /* Source pads matching RDI channels on hardware. Pad 1 -> RDI0, 
>> Pad 2 -> RDI1, etc. */
>> +    struct v4l2_mbus_framefmt *input_format = 
>> &csid->fmt[MSM_CSID_PAD_FIRST_SRC + vc];
>> +    const struct csid_format *format = 
>> csid_get_fmt_entry(csid->formats, csid->nformats,
>> +                                  input_format->code);
>> +
>> +    if (!lane_cnt)
>> +        lane_cnt = 4;
>> +
>> +    val = 0;
>> +    writel_relaxed(val, csid->base + CSID_RDI_FRM_DROP_PERIOD(vc));
>> +
>> +    /*
>> +     * DT_ID is a two bit bitfield that is concatenated with
>> +     * the four least significant bits of the five bit VC
>> +     * bitfield to generate an internal CID value.
>> +     *
>> +     * CSID_RDI_CFG0(vc)
>> +     * DT_ID : 28:27
>> +     * VC    : 26:22
>> +     * DT    : 21:16
>> +     *
>> +     * CID   : VC 3:0 << 2 | DT_ID 1:0
>> +     */
>> +    u8 dt_id = vc & 0x03;
>> +
>> +    val = 1 << RDI_CFG0_TIMESTAMP_EN;
>> +    val |= 2 << RDI_CFG0_TIMESTAMP_STB_SEL;
>> +    /* note: for non-RDI path, this should be format->decode_format */
>> +    val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
>> +    val |= vc << RDI_CFG0_VC;
>> +    val |= format->data_type << RDI_CFG0_DT;
>> +    val |= dt_id << RDI_CFG0_DT_ID;
>> +
>> +    writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
>> +
>> +    val = 1 << RDI_CFG1_PACKING_FORMAT;
>> +    val |= 1 << RDI_CFG1_PIX_STORE;
>> +    val |= 1 << RDI_CFG1_DROP_H_EN;
>> +    val |= 1 << RDI_CFG1_DROP_V_EN;
>> +    val |= 1 << RDI_CFG1_CROP_H_EN;
>> +    val |= 1 << RDI_CFG1_CROP_V_EN;
>> +    val |= RDI_CFG1_EARLY_EOF_EN;
>> +
>> +    writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
>> +
>> +    val = 0;
>> +    writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
>> +
>> +    val = 1;
>> +    writel_relaxed(val, csid->base + 
>> CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
>> +
>> +    val = 0;
>> +    writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
>> +
>> +    val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
>> +    val |=  enable << RDI_CFG0_EN;
>> +    writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
>> +}
> 
> Hmm.
> 
> So I think 90% of the code here can be moved into a shared file - 
> ideally instantiated in gen2.c and then reused here rather than 
> copy/paste from one file to the other.
> 
> Lets sync up and try to get a unified tree for x1e80100 and sm8550 for 
> that purpose.
> 
> I think the code you have here is slightly further along that the 
> CSID/VFE stuff I've been working with.
> 
> But still - reductio ad absurdum - we need to functionally decompose and 
> not replicate code.
> 

Yes, agree, most of the code are same, but the difficult part is most of 
the code is configuring the register which are defined as macro 
definition, and they are different between different platform.

>> +
>> +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)) {
>> +            __csid_configure_top(csid);
>> +            __csid_configure_rdi_stream(csid, enable, i);
>> +            __csid_configure_rx(csid, &csid->phy, i);
>> +            __csid_ctrl_rdi(csid, enable, i);
>> +        }
>> +}
> 
> Another example, straight copy/paste - we need to zap all of that.
> 
> ---
> bod

Thanks,
Depeng

[PATCH v2 6/8] media: qcom: camss: Add new VFE driver for SM8550

[Depeng Shao]

From: Yongsheng Li <quic_yon@quicinc.com>

Add IFE driver for SM8550, the main difference with
old HW is register offset is different, register
update, reset and buf done is moved to CSID. And
the image address support 36 bits, so need to right
shift the image address when configuring the write
master.

Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
Co-developed-by: Depeng Shao <quic_depengs@quicinc.com>
Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
 drivers/media/platform/qcom/camss/Makefile    |   1 +
 .../media/platform/qcom/camss/camss-vfe-780.c | 455 ++++++++++++++++++
 drivers/media/platform/qcom/camss/camss-vfe.h |   1 +
 3 files changed, 457 insertions(+)
 create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c

diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
index c5fcd6eec0f2..ac40bbab18a3 100644
--- a/drivers/media/platform/qcom/camss/Makefile
+++ b/drivers/media/platform/qcom/camss/Makefile
@@ -18,6 +18,7 @@ qcom-camss-objs += \
 		camss-vfe-4-8.o \
 		camss-vfe-17x.o \
 		camss-vfe-480.o \
+		camss-vfe-780.o \
 		camss-vfe-gen1.o \
 		camss-vfe.o \
 		camss-video.o \
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
new file mode 100644
index 000000000000..a78647f23c8c
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * camss-vfe-780.c
+ *
+ * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module v780 (SM8550)
+ *
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+
+#include "camss.h"
+#include "camss-vfe.h"
+
+#define VFE_HW_VERSION			(vfe_is_lite(vfe) ? 0x1000 : 0x0)
+
+#define VFE_IRQ_CMD			(vfe_is_lite(vfe) ? 0x1038 : 0x30)
+#define     IRQ_CMD_GLOBAL_CLEAR	BIT(0)
+
+#define VFE_IRQ_MASK(n)			((vfe_is_lite(vfe) ? 0x1024 : 0x34) + (n) * 4)
+#define	    IRQ_MASK_0_BUS_TOP_IRQ	(vfe_is_lite(vfe) ? BIT(0) | BIT(1) | BIT(2) : \
+						BIT(0) | BIT(4) | BIT(18))
+#define	    IRQ_MASK_1_BUS_TOP_IRQ(n)	(vfe_is_lite(vfe) ? BIT(2 * n + 2) | BIT(2 * n + 3) : \
+						BIT(2 * n + 8) | BIT(2 * n + 9))
+#define VFE_IRQ_CLEAR(n)		((vfe_is_lite(vfe) ? 0x102C : 0x3C) + (n) * 4)
+#define VFE_IRQ_STATUS(n)		((vfe_is_lite(vfe) ? 0x101C : 0x44) + (n) * 4)
+
+#define BUS_REG_BASE			(vfe_is_lite(vfe) ? 0x1200 : 0xC00)
+
+#define VFE_BUS_WM_CGC_OVERRIDE		(BUS_REG_BASE + 0x08)
+#define		WM_CGC_OVERRIDE_ALL	(0x7FFFFFF)
+
+#define VFE_BUS_WM_TEST_BUS_CTRL	(BUS_REG_BASE + 0xdc)
+
+#define VFE_BUS_WM_CFG(n)		(BUS_REG_BASE + 0x200 + (n) * 0x100)
+#define		WM_CFG_EN			(0)
+#define		WM_VIR_FRM_EN		(1)
+#define		WM_CFG_MODE			(16)
+#define			MODE_QCOM_PLAIN	(0)
+#define			MODE_MIPI_RAW	(1)
+#define VFE_BUS_WM_IMAGE_ADDR(n)	(BUS_REG_BASE + 0x204 + (n) * 0x100)
+#define VFE_BUS_WM_FRAME_INCR(n)	(BUS_REG_BASE + 0x208 + (n) * 0x100)
+#define VFE_BUS_WM_IMAGE_CFG_0(n)	(BUS_REG_BASE + 0x20c + (n) * 0x100)
+#define                WM_IMAGE_CFG_0_DEFAULT_WIDTH    (0xFFFF)
+#define VFE_BUS_WM_IMAGE_CFG_1(n)	(BUS_REG_BASE + 0x210 + (n) * 0x100)
+#define VFE_BUS_WM_IMAGE_CFG_2(n)	(BUS_REG_BASE + 0x214 + (n) * 0x100)
+#define                WM_IMAGE_CFG_2_DEFAULT_STRIDE    (0xFFFF)
+#define VFE_BUS_WM_PACKER_CFG(n)	(BUS_REG_BASE + 0x218 + (n) * 0x100)
+#define VFE_BUS_WM_HEADER_ADDR(n)	(BUS_REG_BASE + 0x220 + (n) * 0x100)
+#define VFE_BUS_WM_HEADER_INCR(n)	(BUS_REG_BASE + 0x224 + (n) * 0x100)
+#define VFE_BUS_WM_HEADER_CFG(n)	(BUS_REG_BASE + 0x228 + (n) * 0x100)
+
+#define VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(n)	(BUS_REG_BASE + 0x230 + (n) * 0x100)
+#define VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(n)	(BUS_REG_BASE + 0x234 + (n) * 0x100)
+#define VFE_BUS_WM_FRAMEDROP_PERIOD(n)		(BUS_REG_BASE + 0x238 + (n) * 0x100)
+#define VFE_BUS_WM_FRAMEDROP_PATTERN(n)		(BUS_REG_BASE + 0x23c + (n) * 0x100)
+
+#define VFE_BUS_WM_MMU_PREFETCH_CFG(n)	(BUS_REG_BASE + 0x260 + (n) * 0x100)
+#define VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(n)	(BUS_REG_BASE + 0x264 + (n) * 0x100)
+#define VFE_BUS_WM_SYSTEM_CACHE_CFG(n)	(BUS_REG_BASE + 0x268 + (n) * 0x100)
+
+
+/* for titan 780, each bus client is hardcoded to a specific path */
+#define RDI_WM(n)			((vfe_is_lite(vfe) ? 0 : 23) + (n))
+
+#define MAX_VFE_OUTPUT_LINES	4
+#define MAX_VFE_ACT_BUF	1
+
+static u32 vfe_hw_version(struct vfe_device *vfe)
+{
+	u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
+
+	u32 gen = (hw_version >> 28) & 0xF;
+	u32 rev = (hw_version >> 16) & 0xFFF;
+	u32 step = hw_version & 0xFFFF;
+
+	dev_info(vfe->camss->dev, "VFE HW Version = %u.%u.%u\n", gen, rev, step);
+
+	return hw_version;
+}
+
+static void vfe_global_reset(struct vfe_device *vfe)
+{
+}
+
+static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
+{
+	struct v4l2_pix_format_mplane *pix =
+		&line->video_out.active_fmt.fmt.pix_mp;
+
+	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
+
+	/* no clock gating at bus input */
+	writel_relaxed(0, vfe->base + VFE_BUS_WM_CGC_OVERRIDE);
+
+	writel_relaxed(0x0, vfe->base + VFE_BUS_WM_TEST_BUS_CTRL);
+
+	writel_relaxed(ALIGN(pix->plane_fmt[0].bytesperline, 16) * pix->height >> 8,
+		       vfe->base + VFE_BUS_WM_FRAME_INCR(wm));
+	writel_relaxed((WM_IMAGE_CFG_0_DEFAULT_WIDTH & 0xFFFF),
+		       vfe->base + VFE_BUS_WM_IMAGE_CFG_0(wm));
+	writel_relaxed(WM_IMAGE_CFG_2_DEFAULT_STRIDE,
+		       vfe->base + VFE_BUS_WM_IMAGE_CFG_2(wm));
+	writel_relaxed(0, vfe->base + VFE_BUS_WM_PACKER_CFG(wm));
+
+	/* no dropped frames, one irq per frame */
+	writel_relaxed(0, vfe->base + VFE_BUS_WM_FRAMEDROP_PERIOD(wm));
+	writel_relaxed(1, vfe->base + VFE_BUS_WM_FRAMEDROP_PATTERN(wm));
+	writel_relaxed(0, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(wm));
+	writel_relaxed(1, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(wm));
+
+	writel_relaxed(1, vfe->base + VFE_BUS_WM_MMU_PREFETCH_CFG(wm));
+	writel_relaxed(0xFFFFFFFF, vfe->base + VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(wm));
+
+	writel_relaxed(1 << WM_CFG_EN | MODE_MIPI_RAW << WM_CFG_MODE,
+		       vfe->base + VFE_BUS_WM_CFG(wm));
+}
+
+static void vfe_wm_stop(struct vfe_device *vfe, u8 wm)
+{
+	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
+	writel_relaxed(0, vfe->base + VFE_BUS_WM_CFG(wm));
+}
+
+static void vfe_wm_update(struct vfe_device *vfe, u8 wm, u64 addr,
+			  struct vfe_line *line)
+{
+	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
+	writel_relaxed((addr >> 8) & 0xFFFFFFFF, vfe->base + VFE_BUS_WM_IMAGE_ADDR(wm));
+
+	dev_dbg(vfe->camss->dev, "wm:%d, image buf addr:0x%llx\n",
+		wm, addr);
+}
+
+static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
+{
+	int port_id = line_id;
+
+	v4l2_subdev_notify(&vfe->line[line_id].subdev, NOTIFY_RUP, (void *)&port_id);
+}
+
+static inline void vfe_reg_update_clear(struct vfe_device *vfe,
+					enum vfe_line_id line_id)
+{
+	int port_id = line_id;
+
+	v4l2_subdev_notify(&vfe->line[line_id].subdev, NOTIFY_RUP_CLEAR, (void *)&port_id);
+}
+
+static void vfe_enable_irq_common(struct vfe_device *vfe, enum vfe_line_id line_id)
+{
+	int port_id = line_id;
+
+	/* enable top BUS status IRQ */
+	writel_relaxed(IRQ_MASK_0_BUS_TOP_IRQ,
+				vfe->base + VFE_IRQ_MASK(0));
+
+	writel_relaxed(IRQ_MASK_1_BUS_TOP_IRQ(port_id),
+				vfe->base + VFE_IRQ_MASK(1));
+}
+
+/*
+ * vfe_isr - VFE module interrupt handler
+ * @irq: Interrupt line
+ * @dev: VFE device
+ *
+ * Return IRQ_HANDLED on success
+ */
+static irqreturn_t vfe_isr(int irq, void *dev)
+{
+	struct vfe_device *vfe = dev;
+	u32 status;
+
+	status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(0));
+	writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(0));
+	writel_relaxed(IRQ_CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);
+
+	if (status)
+		dev_dbg(vfe->camss->dev, "Top Status_0:0x%x\n", status);
+
+	status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(1));
+	writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(1));
+	writel_relaxed(IRQ_CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);
+
+	if (status)
+		dev_dbg(vfe->camss->dev, "Top Status_1:0x%x\n", status);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * vfe_halt - Trigger halt on VFE module and wait to complete
+ * @vfe: VFE device
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int vfe_halt(struct vfe_device *vfe)
+{
+	/* rely on vfe_disable_output() to stop the VFE */
+	return 0;
+}
+
+static int vfe_get_output(struct vfe_line *line)
+{
+	struct vfe_device *vfe = to_vfe(line);
+	struct vfe_output *output;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vfe->output_lock, flags);
+
+	output = &line->output;
+	if (output->state > VFE_OUTPUT_RESERVED) {
+		dev_err(vfe->camss->dev, "Output is running\n");
+		goto error;
+	}
+
+	output->wm_num = 1;
+
+	/* Correspondence between VFE line number and WM number.
+	 * line 0 -> RDI 0, line 1 -> RDI1, line 2 -> RDI2, line 3 -> PIX/RDI3
+	 * Note this 1:1 mapping will not work for PIX streams.
+	 */
+	output->wm_idx[0] = line->id;
+	vfe->wm_output_map[line->id] = line->id;
+
+	output->drop_update_idx = 0;
+
+	spin_unlock_irqrestore(&vfe->output_lock, flags);
+
+	return 0;
+
+error:
+	spin_unlock_irqrestore(&vfe->output_lock, flags);
+	output->state = VFE_OUTPUT_OFF;
+
+	return -EINVAL;
+}
+
+static int vfe_enable_output(struct vfe_line *line)
+{
+	struct vfe_device *vfe = to_vfe(line);
+	struct vfe_output *output = &line->output;
+	unsigned long flags;
+	unsigned int i;
+
+	spin_lock_irqsave(&vfe->output_lock, flags);
+
+	vfe_reg_update_clear(vfe, line->id);
+
+	if (output->state > VFE_OUTPUT_RESERVED) {
+		dev_err(vfe->camss->dev, "Output is not in reserved state %d\n",
+			output->state);
+		spin_unlock_irqrestore(&vfe->output_lock, flags);
+		return -EINVAL;
+	}
+
+	WARN_ON(output->gen2.active_num);
+
+	output->state = VFE_OUTPUT_ON;
+
+	output->sequence = 0;
+
+	vfe_wm_start(vfe, output->wm_idx[0], line);
+
+	for (i = 0; i < MAX_VFE_ACT_BUF; i++) {
+		output->buf[i] = vfe_buf_get_pending(output);
+		if (!output->buf[i])
+			break;
+		output->gen2.active_num++;
+		vfe_wm_update(vfe, output->wm_idx[0], output->buf[i]->addr[0], line);
+
+		vfe_reg_update(vfe, line->id);
+	}
+
+	spin_unlock_irqrestore(&vfe->output_lock, flags);
+
+	return 0;
+}
+
+/*
+ * vfe_enable - Enable streaming on VFE line
+ * @line: VFE line
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int vfe_enable(struct vfe_line *line)
+{
+	struct vfe_device *vfe = to_vfe(line);
+	int ret;
+
+	mutex_lock(&vfe->stream_lock);
+
+	if (!vfe->stream_count)
+		vfe_enable_irq_common(vfe, line->id);
+
+	vfe->stream_count++;
+
+	mutex_unlock(&vfe->stream_lock);
+
+	ret = vfe_get_output(line);
+	if (ret < 0)
+		goto error_get_output;
+
+	ret = vfe_enable_output(line);
+	if (ret < 0)
+		goto error_enable_output;
+
+	vfe->was_streaming = 1;
+
+	return 0;
+
+error_enable_output:
+	vfe_put_output(line);
+
+error_get_output:
+	mutex_lock(&vfe->stream_lock);
+
+	vfe->stream_count--;
+
+	mutex_unlock(&vfe->stream_lock);
+
+	return ret;
+}
+
+/*
+ * vfe_buf_done - Process write master done interrupt
+ * @vfe: VFE Device
+ * @wm: Write master id
+ */
+static void vfe_buf_done(struct vfe_device *vfe, int wm)
+{
+	struct vfe_line *line = &vfe->line[vfe->wm_output_map[wm]];
+	struct camss_buffer *ready_buf;
+	struct vfe_output *output;
+	unsigned long flags;
+	u32 index;
+	u64 ts = ktime_get_ns();
+
+	spin_lock_irqsave(&vfe->output_lock, flags);
+
+	if (vfe->wm_output_map[wm] == VFE_LINE_NONE) {
+		dev_err_ratelimited(vfe->camss->dev,
+			"Received wm done for unmapped index\n");
+		goto out_unlock;
+	}
+	output = &vfe->line[vfe->wm_output_map[wm]].output;
+
+	ready_buf = output->buf[0];
+	if (!ready_buf) {
+		dev_err_ratelimited(vfe->camss->dev,
+			"Missing ready buf %d!\n", output->state);
+		goto out_unlock;
+	}
+
+	ready_buf->vb.vb2_buf.timestamp = ts;
+	ready_buf->vb.sequence = output->sequence++;
+
+	index = 0;
+	output->buf[0] = output->buf[1];
+	if (output->buf[0])
+		index = 1;
+
+	output->buf[index] = vfe_buf_get_pending(output);
+
+	if (output->buf[index]) {
+		vfe_wm_update(vfe, output->wm_idx[0], output->buf[index]->addr[0], line);
+		vfe_reg_update(vfe, line->id);
+	} else
+		output->gen2.active_num--;
+
+	spin_unlock_irqrestore(&vfe->output_lock, flags);
+
+	vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+
+	return;
+
+out_unlock:
+	spin_unlock_irqrestore(&vfe->output_lock, flags);
+}
+
+/*
+ * vfe_queue_buffer - Add empty buffer
+ * @vid: Video device structure
+ * @buf: Buffer to be enqueued
+ *
+ * Add an empty buffer - depending on the current number of buffers it will be
+ * put in pending buffer queue or directly given to the hardware to be filled.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int vfe_queue_buffer(struct camss_video *vid,
+			    struct camss_buffer *buf)
+{
+	struct vfe_line *line = container_of(vid, struct vfe_line, video_out);
+	struct vfe_device *vfe = to_vfe(line);
+	struct vfe_output *output;
+	unsigned long flags;
+
+	output = &line->output;
+
+	spin_lock_irqsave(&vfe->output_lock, flags);
+
+	if (output->state == VFE_OUTPUT_ON &&
+		output->gen2.active_num < MAX_VFE_ACT_BUF) {
+		output->buf[output->gen2.active_num++] = buf;
+		vfe_wm_update(vfe, output->wm_idx[0], buf->addr[0], line);
+		vfe_reg_update(vfe, line->id);
+	} else {
+		vfe_buf_add_pending(output, buf);
+	}
+
+	spin_unlock_irqrestore(&vfe->output_lock, flags);
+
+	return 0;
+}
+
+static const struct camss_video_ops vfe_video_ops_780 = {
+	.queue_buffer = vfe_queue_buffer,
+	.flush_buffers = vfe_flush_buffers,
+};
+
+static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
+{
+	vfe->video_ops = vfe_video_ops_780;
+}
+
+static void vfe_subdev_event(struct vfe_device *vfe, unsigned int evt_type, void *arg)
+{
+	int port_id = *(int *)arg;
+
+	switch (evt_type) {
+	case NOTIFY_BUF_DONE:
+		vfe_buf_done(vfe, port_id);
+		break;
+
+	default:
+		break;
+	}
+}
+
+const struct vfe_hw_ops vfe_ops_780 = {
+	.global_reset = vfe_global_reset,
+	.hw_version = vfe_hw_version,
+	.isr = vfe_isr,
+	.pm_domain_off = vfe_pm_domain_off,
+	.pm_domain_on = vfe_pm_domain_on,
+	.subdev_init = vfe_subdev_init,
+	.vfe_disable = vfe_disable,
+	.vfe_enable = vfe_enable,
+	.vfe_halt = vfe_halt,
+	.vfe_wm_stop = vfe_wm_stop,
+	.event = vfe_subdev_event,
+};
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 9919fe0ff101..150ea0b31fb1 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -224,6 +224,7 @@ extern const struct vfe_hw_ops vfe_ops_4_7;
 extern const struct vfe_hw_ops vfe_ops_4_8;
 extern const struct vfe_hw_ops vfe_ops_170;
 extern const struct vfe_hw_ops vfe_ops_480;
+extern const struct vfe_hw_ops vfe_ops_780;
 
 int vfe_get(struct vfe_device *vfe);
 void vfe_put(struct vfe_device *vfe);
-- 
2.17.1

Re: [PATCH v2 6/8] media: qcom: camss: Add new VFE driver for SM8550

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> From: Yongsheng Li <quic_yon@quicinc.com>
> 
> Add IFE driver for SM8550, the main difference with
> old HW is register offset is different, register
> update, reset and buf done is moved to CSID. And
> the image address support 36 bits, so need to right
> shift the image address when configuring the write
> master.
> 
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Co-developed-by: Depeng Shao <quic_depengs@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>

Same comment with your co-developed and SOB

> ---
>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>   .../media/platform/qcom/camss/camss-vfe-780.c | 455 ++++++++++++++++++
>   drivers/media/platform/qcom/camss/camss-vfe.h |   1 +
>   3 files changed, 457 insertions(+)
>   create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c
> 
> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> index c5fcd6eec0f2..ac40bbab18a3 100644
> --- a/drivers/media/platform/qcom/camss/Makefile
> +++ b/drivers/media/platform/qcom/camss/Makefile
> @@ -18,6 +18,7 @@ qcom-camss-objs += \
>   		camss-vfe-4-8.o \
>   		camss-vfe-17x.o \
>   		camss-vfe-480.o \
> +		camss-vfe-780.o \
>   		camss-vfe-gen1.o \
>   		camss-vfe.o \
>   		camss-video.o \
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> new file mode 100644
> index 000000000000..a78647f23c8c
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-vfe-780.c
> + *
> + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module v780 (SM8550)
> + *
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +
> +#include "camss.h"
> +#include "camss-vfe.h"
> +
> +#define VFE_HW_VERSION			(vfe_is_lite(vfe) ? 0x1000 : 0x0)
> +
> +#define VFE_IRQ_CMD			(vfe_is_lite(vfe) ? 0x1038 : 0x30)
> +#define     IRQ_CMD_GLOBAL_CLEAR	BIT(0)
> +
> +#define VFE_IRQ_MASK(n)			((vfe_is_lite(vfe) ? 0x1024 : 0x34) + (n) * 4)
> +#define	    IRQ_MASK_0_BUS_TOP_IRQ	(vfe_is_lite(vfe) ? BIT(0) | BIT(1) | BIT(2) : \
> +						BIT(0) | BIT(4) | BIT(18))
> +#define	    IRQ_MASK_1_BUS_TOP_IRQ(n)	(vfe_is_lite(vfe) ? BIT(2 * n + 2) | BIT(2 * n + 3) : \
> +						BIT(2 * n + 8) | BIT(2 * n + 9))
> +#define VFE_IRQ_CLEAR(n)		((vfe_is_lite(vfe) ? 0x102C : 0x3C) + (n) * 4)
> +#define VFE_IRQ_STATUS(n)		((vfe_is_lite(vfe) ? 0x101C : 0x44) + (n) * 4)
> +
> +#define BUS_REG_BASE			(vfe_is_lite(vfe) ? 0x1200 : 0xC00)
> +
> +#define VFE_BUS_WM_CGC_OVERRIDE		(BUS_REG_BASE + 0x08)
> +#define		WM_CGC_OVERRIDE_ALL	(0x7FFFFFF)
> +
> +#define VFE_BUS_WM_TEST_BUS_CTRL	(BUS_REG_BASE + 0xdc)
> +
> +#define VFE_BUS_WM_CFG(n)		(BUS_REG_BASE + 0x200 + (n) * 0x100)
> +#define		WM_CFG_EN			(0)
> +#define		WM_VIR_FRM_EN		(1)
> +#define		WM_CFG_MODE			(16)
> +#define			MODE_QCOM_PLAIN	(0)
> +#define			MODE_MIPI_RAW	(1)
> +#define VFE_BUS_WM_IMAGE_ADDR(n)	(BUS_REG_BASE + 0x204 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAME_INCR(n)	(BUS_REG_BASE + 0x208 + (n) * 0x100)
> +#define VFE_BUS_WM_IMAGE_CFG_0(n)	(BUS_REG_BASE + 0x20c + (n) * 0x100)
> +#define                WM_IMAGE_CFG_0_DEFAULT_WIDTH    (0xFFFF)
> +#define VFE_BUS_WM_IMAGE_CFG_1(n)	(BUS_REG_BASE + 0x210 + (n) * 0x100)
> +#define VFE_BUS_WM_IMAGE_CFG_2(n)	(BUS_REG_BASE + 0x214 + (n) * 0x100)
> +#define                WM_IMAGE_CFG_2_DEFAULT_STRIDE    (0xFFFF)
> +#define VFE_BUS_WM_PACKER_CFG(n)	(BUS_REG_BASE + 0x218 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_ADDR(n)	(BUS_REG_BASE + 0x220 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_INCR(n)	(BUS_REG_BASE + 0x224 + (n) * 0x100)
> +#define VFE_BUS_WM_HEADER_CFG(n)	(BUS_REG_BASE + 0x228 + (n) * 0x100)
> +
> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(n)	(BUS_REG_BASE + 0x230 + (n) * 0x100)
> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(n)	(BUS_REG_BASE + 0x234 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAMEDROP_PERIOD(n)		(BUS_REG_BASE + 0x238 + (n) * 0x100)
> +#define VFE_BUS_WM_FRAMEDROP_PATTERN(n)		(BUS_REG_BASE + 0x23c + (n) * 0x100)
> +
> +#define VFE_BUS_WM_MMU_PREFETCH_CFG(n)	(BUS_REG_BASE + 0x260 + (n) * 0x100)
> +#define VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(n)	(BUS_REG_BASE + 0x264 + (n) * 0x100)
> +#define VFE_BUS_WM_SYSTEM_CACHE_CFG(n)	(BUS_REG_BASE + 0x268 + (n) * 0x100)
> +
> +
> +/* for titan 780, each bus client is hardcoded to a specific path */
> +#define RDI_WM(n)			((vfe_is_lite(vfe) ? 0 : 23) + (n))

No admixture of hex and decimal please.

> +
> +#define MAX_VFE_OUTPUT_LINES	4
> +#define MAX_VFE_ACT_BUF	1
> +
> +static u32 vfe_hw_version(struct vfe_device *vfe)
> +{
> +	u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
> +
> +	u32 gen = (hw_version >> 28) & 0xF;
> +	u32 rev = (hw_version >> 16) & 0xFFF;
> +	u32 step = hw_version & 0xFFFF;
> +
> +	dev_info(vfe->camss->dev, "VFE HW Version = %u.%u.%u\n", gen, rev, step);
> +
> +	return hw_version;
> +}

Same comment as with CSID, its time to rationalise all of this 
replicated code down to one place, instead of proliferating it further.

> +static void vfe_global_reset(struct vfe_device *vfe)
> +{
> +}
> +
> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
> +{
> +	struct v4l2_pix_format_mplane *pix =
> +		&line->video_out.active_fmt.fmt.pix_mp;
> +
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> +
> +	/* no clock gating at bus input */
> +	writel_relaxed(0, vfe->base + VFE_BUS_WM_CGC_OVERRIDE);
> +
> +	writel_relaxed(0x0, vfe->base + VFE_BUS_WM_TEST_BUS_CTRL);
> +
> +	writel_relaxed(ALIGN(pix->plane_fmt[0].bytesperline, 16) * pix->height >> 8,
> +		       vfe->base + VFE_BUS_WM_FRAME_INCR(wm));
> +	writel_relaxed((WM_IMAGE_CFG_0_DEFAULT_WIDTH & 0xFFFF),
> +		       vfe->base + VFE_BUS_WM_IMAGE_CFG_0(wm));
> +	writel_relaxed(WM_IMAGE_CFG_2_DEFAULT_STRIDE,
> +		       vfe->base + VFE_BUS_WM_IMAGE_CFG_2(wm));
> +	writel_relaxed(0, vfe->base + VFE_BUS_WM_PACKER_CFG(wm));
> +
> +	/* no dropped frames, one irq per frame */
> +	writel_relaxed(0, vfe->base + VFE_BUS_WM_FRAMEDROP_PERIOD(wm));
> +	writel_relaxed(1, vfe->base + VFE_BUS_WM_FRAMEDROP_PATTERN(wm));
> +	writel_relaxed(0, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(wm));
> +	writel_relaxed(1, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(wm));
> +
> +	writel_relaxed(1, vfe->base + VFE_BUS_WM_MMU_PREFETCH_CFG(wm));
> +	writel_relaxed(0xFFFFFFFF, vfe->base + VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(wm));
> +
> +	writel_relaxed(1 << WM_CFG_EN | MODE_MIPI_RAW << WM_CFG_MODE,
> +		       vfe->base + VFE_BUS_WM_CFG(wm));
> +}
> +
> +static void vfe_wm_stop(struct vfe_device *vfe, u8 wm)
> +{
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> +	writel_relaxed(0, vfe->base + VFE_BUS_WM_CFG(wm));
> +}

vfe_wm_stop() as an example can/should live in a shared file.

As proof of concept code its fine to copy/paste between files but, to 
merge we need to get rid of any replicated code - introducing 
indirection/offsets as necessary.

> +static void vfe_wm_update(struct vfe_device *vfe, u8 wm, u64 addr,
> +			  struct vfe_line *line)
> +{
> +	wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> +	writel_relaxed((addr >> 8) & 0xFFFFFFFF, vfe->base + VFE_BUS_WM_IMAGE_ADDR(wm));
> +
> +	dev_dbg(vfe->camss->dev, "wm:%d, image buf addr:0x%llx\n",
> +		wm, addr);
> +}
> +
> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	v4l2_subdev_notify(&vfe->line[line_id].subdev, NOTIFY_RUP, (void *)&port_id);
> +}
> +
> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
> +					enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	v4l2_subdev_notify(&vfe->line[line_id].subdev, NOTIFY_RUP_CLEAR, (void *)&port_id);
> +}

I'm not sure I quite understand why we need to use this API to clear 
registers inside of the address space of the same driver.

Is there not a much more direct way to write our _internal_ registers ?


> +static void vfe_enable_irq_common(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	/* enable top BUS status IRQ */
> +	writel_relaxed(IRQ_MASK_0_BUS_TOP_IRQ,
> +				vfe->base + VFE_IRQ_MASK(0));
> +
> +	writel_relaxed(IRQ_MASK_1_BUS_TOP_IRQ(port_id),
> +				vfe->base + VFE_IRQ_MASK(1));
> +}
> +
> +/*
> + * vfe_isr - VFE module interrupt handler
> + * @irq: Interrupt line
> + * @dev: VFE device
> + *
> + * Return IRQ_HANDLED on success
> + */
> +static irqreturn_t vfe_isr(int irq, void *dev)
> +{
> +	struct vfe_device *vfe = dev;
> +	u32 status;
> +
> +	status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(0));
> +	writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(0));
> +	writel_relaxed(IRQ_CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);
> +
> +	if (status)
> +		dev_dbg(vfe->camss->dev, "Top Status_0:0x%x\n", status);
> +
> +	status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(1));
> +	writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(1));
> +	writel_relaxed(IRQ_CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);
> +
> +	if (status)
> +		dev_dbg(vfe->camss->dev, "Top Status_1:0x%x\n", status);
> +
> +	return IRQ_HANDLED;
> +}

Why is this ISR required ? What does it do ?

Does it actually run on your reference hardware ?

If the purpose of the ISR is just to clear the status registers, why 
even enable it ?

> +
> +/*
> + * vfe_halt - Trigger halt on VFE module and wait to complete
> + * @vfe: VFE device
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +static int vfe_halt(struct vfe_device *vfe)
> +{
> +	/* rely on vfe_disable_output() to stop the VFE */
> +	return 0;
> +}
> +
> +static int vfe_get_output(struct vfe_line *line)
> +{
> +	struct vfe_device *vfe = to_vfe(line);
> +	struct vfe_output *output;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vfe->output_lock, flags);
> +
> +	output = &line->output;
> +	if (output->state > VFE_OUTPUT_RESERVED) {
> +		dev_err(vfe->camss->dev, "Output is running\n");
> +		goto error;
> +	}
> +
> +	output->wm_num = 1;
> +
> +	/* Correspondence between VFE line number and WM number.
> +	 * line 0 -> RDI 0, line 1 -> RDI1, line 2 -> RDI2, line 3 -> PIX/RDI3
> +	 * Note this 1:1 mapping will not work for PIX streams.
> +	 */
> +	output->wm_idx[0] = line->id;
> +	vfe->wm_output_map[line->id] = line->id;
> +
> +	output->drop_update_idx = 0;
> +
> +	spin_unlock_irqrestore(&vfe->output_lock, flags);
> +
> +	return 0;
> +
> +error:
> +	spin_unlock_irqrestore(&vfe->output_lock, flags);
> +	output->state = VFE_OUTPUT_OFF;
> +
> +	return -EINVAL;
> +}
> +
> +static int vfe_enable_output(struct vfe_line *line)
> +{
> +	struct vfe_device *vfe = to_vfe(line);
> +	struct vfe_output *output = &line->output;
> +	unsigned long flags;
> +	unsigned int i;
> +
> +	spin_lock_irqsave(&vfe->output_lock, flags);
> +
> +	vfe_reg_update_clear(vfe, line->id);
> +
> +	if (output->state > VFE_OUTPUT_RESERVED) {
> +		dev_err(vfe->camss->dev, "Output is not in reserved state %d\n",
> +			output->state);
> +		spin_unlock_irqrestore(&vfe->output_lock, flags);
> +		return -EINVAL;
> +	}
> +
> +	WARN_ON(output->gen2.active_num);
> +
> +	output->state = VFE_OUTPUT_ON;
> +
> +	output->sequence = 0;
> +
> +	vfe_wm_start(vfe, output->wm_idx[0], line);
> +
> +	for (i = 0; i < MAX_VFE_ACT_BUF; i++) {
> +		output->buf[i] = vfe_buf_get_pending(output);
> +		if (!output->buf[i])
> +			break;
> +		output->gen2.active_num++;
> +		vfe_wm_update(vfe, output->wm_idx[0], output->buf[i]->addr[0], line);
> +
> +		vfe_reg_update(vfe, line->id);
> +	}
> +
> +	spin_unlock_irqrestore(&vfe->output_lock, flags);
> +
> +	return 0;
> +}

So you'll see with the WIP code for x1e80100 which is I think the same 
VFE - no one generation less 680 - that this code is copy/pasted.

But there's again no good reason for that. Common code needs to be 
squashed down into one place.

---
bod

Re: [PATCH v2 6/8] media: qcom: camss: Add new VFE driver for SM8550

[Depeng Shao]

Hi Bryan,

On 3/20/2024 11:57 PM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
>> From: Yongsheng Li <quic_yon@quicinc.com>
>>
>> Add IFE driver for SM8550, the main difference with
>> old HW is register offset is different, register
>> update, reset and buf done is moved to CSID. And
>> the image address support 36 bits, so need to right
>> shift the image address when configuring the write
>> master.
>>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> Co-developed-by: Depeng Shao <quic_depengs@quicinc.com>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> 
> Same comment with your co-developed and SOB
> 
>> ---
>>   drivers/media/platform/qcom/camss/Makefile    |   1 +
>>   .../media/platform/qcom/camss/camss-vfe-780.c | 455 ++++++++++++++++++
>>   drivers/media/platform/qcom/camss/camss-vfe.h |   1 +
>>   3 files changed, 457 insertions(+)
>>   create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c
>>
>> diff --git a/drivers/media/platform/qcom/camss/Makefile 
>> b/drivers/media/platform/qcom/camss/Makefile
>> index c5fcd6eec0f2..ac40bbab18a3 100644
>> --- a/drivers/media/platform/qcom/camss/Makefile
>> +++ b/drivers/media/platform/qcom/camss/Makefile
>> @@ -18,6 +18,7 @@ qcom-camss-objs += \
>>           camss-vfe-4-8.o \
>>           camss-vfe-17x.o \
>>           camss-vfe-480.o \
>> +        camss-vfe-780.o \
>>           camss-vfe-gen1.o \
>>           camss-vfe.o \
>>           camss-video.o \
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c 
>> b/drivers/media/platform/qcom/camss/camss-vfe-780.c
>> new file mode 100644
>> index 000000000000..a78647f23c8c
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
>> @@ -0,0 +1,455 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * camss-vfe-780.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module v780 
>> (SM8550)
>> + *
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +
>> +#include "camss.h"
>> +#include "camss-vfe.h"
>> +
>> +#define VFE_HW_VERSION            (vfe_is_lite(vfe) ? 0x1000 : 0x0)
>> +
>> +#define VFE_IRQ_CMD            (vfe_is_lite(vfe) ? 0x1038 : 0x30)
>> +#define     IRQ_CMD_GLOBAL_CLEAR    BIT(0)
>> +
>> +#define VFE_IRQ_MASK(n)            ((vfe_is_lite(vfe) ? 0x1024 : 
>> 0x34) + (n) * 4)
>> +#define        IRQ_MASK_0_BUS_TOP_IRQ    (vfe_is_lite(vfe) ? BIT(0) | 
>> BIT(1) | BIT(2) : \
>> +                        BIT(0) | BIT(4) | BIT(18))
>> +#define        IRQ_MASK_1_BUS_TOP_IRQ(n)    (vfe_is_lite(vfe) ? BIT(2 
>> * n + 2) | BIT(2 * n + 3) : \
>> +                        BIT(2 * n + 8) | BIT(2 * n + 9))
>> +#define VFE_IRQ_CLEAR(n)        ((vfe_is_lite(vfe) ? 0x102C : 0x3C) + 
>> (n) * 4)
>> +#define VFE_IRQ_STATUS(n)        ((vfe_is_lite(vfe) ? 0x101C : 0x44) 
>> + (n) * 4)
>> +
>> +#define BUS_REG_BASE            (vfe_is_lite(vfe) ? 0x1200 : 0xC00)
>> +
>> +#define VFE_BUS_WM_CGC_OVERRIDE        (BUS_REG_BASE + 0x08)
>> +#define        WM_CGC_OVERRIDE_ALL    (0x7FFFFFF)
>> +
>> +#define VFE_BUS_WM_TEST_BUS_CTRL    (BUS_REG_BASE + 0xdc)
>> +
>> +#define VFE_BUS_WM_CFG(n)        (BUS_REG_BASE + 0x200 + (n) * 0x100)
>> +#define        WM_CFG_EN            (0)
>> +#define        WM_VIR_FRM_EN        (1)
>> +#define        WM_CFG_MODE            (16)
>> +#define            MODE_QCOM_PLAIN    (0)
>> +#define            MODE_MIPI_RAW    (1)
>> +#define VFE_BUS_WM_IMAGE_ADDR(n)    (BUS_REG_BASE + 0x204 + (n) * 0x100)
>> +#define VFE_BUS_WM_FRAME_INCR(n)    (BUS_REG_BASE + 0x208 + (n) * 0x100)
>> +#define VFE_BUS_WM_IMAGE_CFG_0(n)    (BUS_REG_BASE + 0x20c + (n) * 
>> 0x100)
>> +#define                WM_IMAGE_CFG_0_DEFAULT_WIDTH    (0xFFFF)
>> +#define VFE_BUS_WM_IMAGE_CFG_1(n)    (BUS_REG_BASE + 0x210 + (n) * 
>> 0x100)
>> +#define VFE_BUS_WM_IMAGE_CFG_2(n)    (BUS_REG_BASE + 0x214 + (n) * 
>> 0x100)
>> +#define                WM_IMAGE_CFG_2_DEFAULT_STRIDE    (0xFFFF)
>> +#define VFE_BUS_WM_PACKER_CFG(n)    (BUS_REG_BASE + 0x218 + (n) * 0x100)
>> +#define VFE_BUS_WM_HEADER_ADDR(n)    (BUS_REG_BASE + 0x220 + (n) * 
>> 0x100)
>> +#define VFE_BUS_WM_HEADER_INCR(n)    (BUS_REG_BASE + 0x224 + (n) * 
>> 0x100)
>> +#define VFE_BUS_WM_HEADER_CFG(n)    (BUS_REG_BASE + 0x228 + (n) * 0x100)
>> +
>> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(n)    (BUS_REG_BASE + 0x230 + 
>> (n) * 0x100)
>> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(n)    (BUS_REG_BASE + 0x234 
>> + (n) * 0x100)
>> +#define VFE_BUS_WM_FRAMEDROP_PERIOD(n)        (BUS_REG_BASE + 0x238 + 
>> (n) * 0x100)
>> +#define VFE_BUS_WM_FRAMEDROP_PATTERN(n)        (BUS_REG_BASE + 0x23c 
>> + (n) * 0x100)
>> +
>> +#define VFE_BUS_WM_MMU_PREFETCH_CFG(n)    (BUS_REG_BASE + 0x260 + (n) 
>> * 0x100)
>> +#define VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(n)    (BUS_REG_BASE + 
>> 0x264 + (n) * 0x100)
>> +#define VFE_BUS_WM_SYSTEM_CACHE_CFG(n)    (BUS_REG_BASE + 0x268 + (n) 
>> * 0x100)
>> +
>> +
>> +/* for titan 780, each bus client is hardcoded to a specific path */
>> +#define RDI_WM(n)            ((vfe_is_lite(vfe) ? 0 : 23) + (n))
> 
> No admixture of hex and decimal please.
> 
>> +
>> +#define MAX_VFE_OUTPUT_LINES    4
>> +#define MAX_VFE_ACT_BUF    1
>> +
>> +static u32 vfe_hw_version(struct vfe_device *vfe)
>> +{
>> +    u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
>> +
>> +    u32 gen = (hw_version >> 28) & 0xF;
>> +    u32 rev = (hw_version >> 16) & 0xFFF;
>> +    u32 step = hw_version & 0xFFFF;
>> +
>> +    dev_info(vfe->camss->dev, "VFE HW Version = %u.%u.%u\n", gen, 
>> rev, step);
>> +
>> +    return hw_version;
>> +}
> 
> Same comment as with CSID, its time to rationalise all of this 
> replicated code down to one place, instead of proliferating it further.
> 

I understand, but the original driver and current driver are using macro 
definition to define the register offset, then we aren't able to add the 
common code to one place.

We need to refactor the code to not use macro definition for the 
register offset first, then we can reuse the common code.

>> +static void vfe_global_reset(struct vfe_device *vfe)
>> +{
>> +}
>> +
>> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct 
>> vfe_line *line)
>> +{
>> +    struct v4l2_pix_format_mplane *pix =
>> +        &line->video_out.active_fmt.fmt.pix_mp;
>> +
>> +    wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
>> +
>> +    /* no clock gating at bus input */
>> +    writel_relaxed(0, vfe->base + VFE_BUS_WM_CGC_OVERRIDE);
>> +
>> +    writel_relaxed(0x0, vfe->base + VFE_BUS_WM_TEST_BUS_CTRL);
>> +
>> +    writel_relaxed(ALIGN(pix->plane_fmt[0].bytesperline, 16) * 
>> pix->height >> 8,
>> +               vfe->base + VFE_BUS_WM_FRAME_INCR(wm));
>> +    writel_relaxed((WM_IMAGE_CFG_0_DEFAULT_WIDTH & 0xFFFF),
>> +               vfe->base + VFE_BUS_WM_IMAGE_CFG_0(wm));
>> +    writel_relaxed(WM_IMAGE_CFG_2_DEFAULT_STRIDE,
>> +               vfe->base + VFE_BUS_WM_IMAGE_CFG_2(wm));
>> +    writel_relaxed(0, vfe->base + VFE_BUS_WM_PACKER_CFG(wm));
>> +
>> +    /* no dropped frames, one irq per frame */
>> +    writel_relaxed(0, vfe->base + VFE_BUS_WM_FRAMEDROP_PERIOD(wm));
>> +    writel_relaxed(1, vfe->base + VFE_BUS_WM_FRAMEDROP_PATTERN(wm));
>> +    writel_relaxed(0, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(wm));
>> +    writel_relaxed(1, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(wm));
>> +
>> +    writel_relaxed(1, vfe->base + VFE_BUS_WM_MMU_PREFETCH_CFG(wm));
>> +    writel_relaxed(0xFFFFFFFF, vfe->base + 
>> VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(wm));
>> +
>> +    writel_relaxed(1 << WM_CFG_EN | MODE_MIPI_RAW << WM_CFG_MODE,
>> +               vfe->base + VFE_BUS_WM_CFG(wm));
>> +}
>> +
>> +static void vfe_wm_stop(struct vfe_device *vfe, u8 wm)
>> +{
>> +    wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
>> +    writel_relaxed(0, vfe->base + VFE_BUS_WM_CFG(wm));
>> +}
> 
> vfe_wm_stop() as an example can/should live in a shared file.
> 
> As proof of concept code its fine to copy/paste between files but, to 
> merge we need to get rid of any replicated code - introducing 
> indirection/offsets as necessary.
> 

Same with above, we'd better to refactor the code to not use macro 
definition for the register offset first.

>> +static void vfe_wm_update(struct vfe_device *vfe, u8 wm, u64 addr,
>> +              struct vfe_line *line)
>> +{
>> +    wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
>> +    writel_relaxed((addr >> 8) & 0xFFFFFFFF, vfe->base + 
>> VFE_BUS_WM_IMAGE_ADDR(wm));
>> +
>> +    dev_dbg(vfe->camss->dev, "wm:%d, image buf addr:0x%llx\n",
>> +        wm, addr);
>> +}
>> +
>> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id 
>> line_id)
>> +{
>> +    int port_id = line_id;
>> +
>> +    v4l2_subdev_notify(&vfe->line[line_id].subdev, NOTIFY_RUP, (void 
>> *)&port_id);
>> +}
>> +
>> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
>> +                    enum vfe_line_id line_id)
>> +{
>> +    int port_id = line_id;
>> +
>> +    v4l2_subdev_notify(&vfe->line[line_id].subdev, NOTIFY_RUP_CLEAR, 
>> (void *)&port_id);
>> +}
> 
> I'm not sure I quite understand why we need to use this API to clear 
> registers inside of the address space of the same driver.
> 
> Is there not a much more direct way to write our _internal_ registers ?
> 

Since the register update(RUP) is moved to CSID, so it isn't _internal_ 
registers in this hardware.

> 
>> +static void vfe_enable_irq_common(struct vfe_device *vfe, enum 
>> vfe_line_id line_id)
>> +{
>> +    int port_id = line_id;
>> +
>> +    /* enable top BUS status IRQ */
>> +    writel_relaxed(IRQ_MASK_0_BUS_TOP_IRQ,
>> +                vfe->base + VFE_IRQ_MASK(0));
>> +
>> +    writel_relaxed(IRQ_MASK_1_BUS_TOP_IRQ(port_id),
>> +                vfe->base + VFE_IRQ_MASK(1));
>> +}
>> +
>> +/*
>> + * vfe_isr - VFE module interrupt handler
>> + * @irq: Interrupt line
>> + * @dev: VFE device
>> + *
>> + * Return IRQ_HANDLED on success
>> + */
>> +static irqreturn_t vfe_isr(int irq, void *dev)
>> +{
>> +    struct vfe_device *vfe = dev;
>> +    u32 status;
>> +
>> +    status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(0));
>> +    writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(0));
>> +    writel_relaxed(IRQ_CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);
>> +
>> +    if (status)
>> +        dev_dbg(vfe->camss->dev, "Top Status_0:0x%x\n", status);
>> +
>> +    status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(1));
>> +    writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(1));
>> +    writel_relaxed(IRQ_CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);
>> +
>> +    if (status)
>> +        dev_dbg(vfe->camss->dev, "Top Status_1:0x%x\n", status);
>> +
>> +    return IRQ_HANDLED;
>> +}
> 
> Why is this ISR required ? What does it do ?
> 
> Does it actually run on your reference hardware ?
> 
> If the purpose of the ISR is just to clear the status registers, why 
> even enable it ?
> 

Yeah, we can remove it, it was added for some debug info and verify the 
hardware, but it doesn't affect the control in this hardware, so we can 
remove it.

>> +
>> +/*
>> + * vfe_halt - Trigger halt on VFE module and wait to complete
>> + * @vfe: VFE device
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static int vfe_halt(struct vfe_device *vfe)
>> +{
>> +    /* rely on vfe_disable_output() to stop the VFE */
>> +    return 0;
>> +}
>> +
>> +static int vfe_get_output(struct vfe_line *line)
>> +{
>> +    struct vfe_device *vfe = to_vfe(line);
>> +    struct vfe_output *output;
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&vfe->output_lock, flags);
>> +
>> +    output = &line->output;
>> +    if (output->state > VFE_OUTPUT_RESERVED) {
>> +        dev_err(vfe->camss->dev, "Output is running\n");
>> +        goto error;
>> +    }
>> +
>> +    output->wm_num = 1;
>> +
>> +    /* Correspondence between VFE line number and WM number.
>> +     * line 0 -> RDI 0, line 1 -> RDI1, line 2 -> RDI2, line 3 -> 
>> PIX/RDI3
>> +     * Note this 1:1 mapping will not work for PIX streams.
>> +     */
>> +    output->wm_idx[0] = line->id;
>> +    vfe->wm_output_map[line->id] = line->id;
>> +
>> +    output->drop_update_idx = 0;
>> +
>> +    spin_unlock_irqrestore(&vfe->output_lock, flags);
>> +
>> +    return 0;
>> +
>> +error:
>> +    spin_unlock_irqrestore(&vfe->output_lock, flags);
>> +    output->state = VFE_OUTPUT_OFF;
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int vfe_enable_output(struct vfe_line *line)
>> +{
>> +    struct vfe_device *vfe = to_vfe(line);
>> +    struct vfe_output *output = &line->output;
>> +    unsigned long flags;
>> +    unsigned int i;
>> +
>> +    spin_lock_irqsave(&vfe->output_lock, flags);
>> +
>> +    vfe_reg_update_clear(vfe, line->id);
>> +
>> +    if (output->state > VFE_OUTPUT_RESERVED) {
>> +        dev_err(vfe->camss->dev, "Output is not in reserved state %d\n",
>> +            output->state);
>> +        spin_unlock_irqrestore(&vfe->output_lock, flags);
>> +        return -EINVAL;
>> +    }
>> +
>> +    WARN_ON(output->gen2.active_num);
>> +
>> +    output->state = VFE_OUTPUT_ON;
>> +
>> +    output->sequence = 0;
>> +
>> +    vfe_wm_start(vfe, output->wm_idx[0], line);
>> +
>> +    for (i = 0; i < MAX_VFE_ACT_BUF; i++) {
>> +        output->buf[i] = vfe_buf_get_pending(output);
>> +        if (!output->buf[i])
>> +            break;
>> +        output->gen2.active_num++;
>> +        vfe_wm_update(vfe, output->wm_idx[0], 
>> output->buf[i]->addr[0], line);
>> +
>> +        vfe_reg_update(vfe, line->id);
>> +    }
>> +
>> +    spin_unlock_irqrestore(&vfe->output_lock, flags);
>> +
>> +    return 0;
>> +}
> 
> So you'll see with the WIP code for x1e80100 which is I think the same 
> VFE - no one generation less 680 - that this code is copy/pasted.
> 
> But there's again no good reason for that. Common code needs to be 
> squashed down into one place.
> 

Agree, this part can be moved to camss-vfe.c, since they don't have 
register configuration, just some common code.

> ---
> bod

Thanks,
Depeng

Re: [PATCH v2 6/8] media: qcom: camss: Add new VFE driver for SM8550

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> +#define VFE_IRQ_STATUS(n)		((vfe_is_lite(vfe) ? 0x101C : 0x44) + (n) * 4)

These defines do the right thing for the RDI however

1. Please use 'rdi' not 'n' as the parameter here and
2. Pass 'vfe' explicitly not implicitly

Right now you need a variable named 'vfe' in the scope of the macro but 
that ought to be an explicit parameter passed to the macro.

---
bod

Re: [PATCH v2 6/8] media: qcom: camss: Add new VFE driver for SM8550

[Bryan O'Donoghue]

On 09/04/2024 14:39, Bryan O'Donoghue wrote:
> 1. Please use 'rdi' not 'n' as the parameter here and

Ignore this comment 'n' is the right value here - you've already used 
'rdi' in CSID which is the appropriate place 'n' is correct here.

---
bod

[PATCH v2 7/8] media: qcom: camss: Add sm8550 resources

[Depeng Shao]

From: Yongsheng Li <quic_yon@quicinc.com>

Add sm8550 resources
- 3 x VFE, 3 RDI per VFE
- 2 x VFE Lite, 4 RDI per VFE
- 3 x CSID
- 2 x CSID Lite
- 8 x CSI PHY

Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
---
 drivers/media/platform/qcom/camss/camss.c | 345 ++++++++++++++++++++++
 1 file changed, 345 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index b57cd25bf6c7..f43c957de49e 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1233,6 +1233,337 @@ static const struct resources_icc icc_res_sc8280xp[] = {
 	},
 };
 
+static const struct camss_subdev_resources csiphy_res_8550[] = {
+	/* CSIPHY0 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy0",
+			"csiphy0_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy0" },
+		.interrupt = { "csiphy0" },
+		.ops = &csiphy_ops_2_1_2,
+	},
+	/* CSIPHY1 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy1",
+			"csiphy1_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy1" },
+		.interrupt = { "csiphy1" },
+		.ops = &csiphy_ops_2_1_2,
+	},
+	/* CSIPHY2 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy2",
+			"csiphy2_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy2" },
+		.interrupt = { "csiphy2" },
+		.ops = &csiphy_ops_2_1_2,
+	},
+	/* CSIPHY3 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy3",
+			"csiphy3_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy3" },
+		.interrupt = { "csiphy3" },
+		.ops = &csiphy_ops_2_1_2,
+	},
+	/* CSIPHY4 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy4",
+			"csiphy4_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy4" },
+		.interrupt = { "csiphy4" },
+		.ops = &csiphy_ops_2_1_2,
+	},
+	/* CSIPHY5 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy5",
+			"csiphy5_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy5" },
+		.interrupt = { "csiphy5" },
+		.ops = &csiphy_ops_2_1_2,
+	},
+	/* CSIPHY6 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy6",
+			"csiphy6_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy6" },
+		.interrupt = { "csiphy6" },
+		.ops = &csiphy_ops_2_1_2,
+	},
+	/* CSIPHY7 */
+	{
+		.regulators = {},
+		.clock = {
+			"csiphy7",
+			"csiphy7_timer" },
+		.clock_rate = {
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 400000000, 400000000, 400000000, 400000000 } },
+		.reg = { "csiphy7" },
+		.interrupt = { "csiphy7" },
+		.ops = &csiphy_ops_2_1_2,
+	}
+};
+
+static const struct camss_subdev_resources csid_res_8550[] = {
+	/* CSID0 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = {
+			"csid",
+			"csiphy_rx" },
+		.clock_rate = {
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 } },
+		.reg = { "csid0", "csid_top" },
+		.interrupt = { "csid0" },
+		.ops = &csid_ops_gen3,
+	},
+	/* CSID1 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "csid", "csiphy_rx" },
+		.clock_rate = {
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 } },
+		.reg = { "csid1", "csid_top" },
+		.interrupt = { "csid1" },
+		.ops = &csid_ops_gen3,
+	},
+	/* CSID2 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "csid", "csiphy_rx" },
+		.clock_rate = {
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 } },
+		.reg = { "csid2", "csid_top" },
+		.interrupt = { "csid2" },
+		.ops = &csid_ops_gen3,
+	},
+	/* CSID3 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "vfe_lite_csid",
+			"vfe_lite_cphy_rx" },
+		.clock_rate = {
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 } },
+		.reg = { "csid_lite0" },
+		.interrupt = { "csid_lite0" },
+		.ops = &csid_ops_gen3,
+	},
+	/* CSID4 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "vfe_lite_csid",
+			"vfe_lite_cphy_rx" },
+		.clock_rate = {
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 } },
+		.reg = { "csid_lite1" },
+		.interrupt = { "csid_lite1" },
+		.ops = &csid_ops_gen3,
+	}
+};
+
+static const struct camss_subdev_resources vfe_res_8550[] = {
+	/* VFE0 */
+	{
+		.regulators = {},
+		.clock = {
+			"cam_hf_axi",
+			"cpas_ahb",
+			"cpas_fast_ahb_clk",
+			"vfe0_fast_ahb",
+			"vfe0",
+			"cpas_vfe0",
+			"camnoc_axi"},
+		.clock_rate = {
+				{ 0, 0, 0, 0, 0 },
+				{ 0, 0, 0, 0, 80000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 }
+				},
+		.reg = { "vfe0" },
+		.interrupt = { "vfe0" },
+		.line_num = 3,
+		.is_lite = false,
+		.has_pd = true,
+		.pd_name = "ife0",
+		.ops = &vfe_ops_780,
+	},
+	/* VFE1 */
+	{
+		.regulators = {},
+		.clock = {
+			"cam_hf_axi",
+			"cpas_ahb",
+			"cpas_fast_ahb_clk",
+			"vfe1_fast_ahb",
+			"vfe1",
+			"cpas_vfe1",
+			"camnoc_axi"},
+		.clock_rate = {
+				{ 0, 0, 0, 0, 0 },
+				{ 0, 0, 0, 0, 80000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 }
+				},
+		.reg = { "vfe1" },
+		.interrupt = { "vfe1" },
+		.line_num = 3,
+		.is_lite = false,
+		.has_pd = true,
+		.pd_name = "ife1",
+		.ops = &vfe_ops_780,
+	},
+	/* VFE2 */
+	{
+		.regulators = {},
+		.clock = {
+			"cam_hf_axi",
+			"cpas_ahb",
+			"cpas_fast_ahb_clk",
+			"vfe2_fast_ahb",
+			"vfe2",
+			"cpas_vfe2",
+			"camnoc_axi"},
+		.clock_rate = {
+				{ 0, 0, 0, 0, 0 },
+				{ 0, 0, 0, 0, 80000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 466000000, 594000000, 675000000, 785000000, 785000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 }
+				},
+		.reg = { "vfe2" },
+		.interrupt = { "vfe2" },
+		.line_num = 3,
+		.is_lite = false,
+		.has_pd = true,
+		.pd_name = "ife2",
+		.ops = &vfe_ops_780,
+	},
+	/* VFE3 (lite) */
+	{
+		.regulators = {},
+		.clock = {
+			"cam_hf_axi",
+			"cpas_ahb",
+			"cpas_fast_ahb_clk",
+			"vfe_lite_ahb",
+			"vfe_lite",
+			"cpas_ife_lite",
+			"camnoc_axi"},
+		.clock_rate = {
+				{ 0, 0, 0, 0, 0 },
+				{ 0, 0, 0, 0, 80000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 }
+				},
+		.reg = { "vfe_lite0" },
+		.interrupt = { "vfe_lite0" },
+		.line_num = 4,
+		.is_lite = true,
+		.ops = &vfe_ops_780,
+	},
+	/* VFE4 (lite) */
+	{
+		.regulators = {},
+		.clock = {
+			"cam_hf_axi",
+			"cpas_ahb",
+			"cpas_fast_ahb_clk",
+			"vfe_lite_ahb",
+			"vfe_lite",
+			"cpas_ife_lite",
+			"camnoc_axi"},
+		.clock_rate = {
+				{ 0, 0, 0, 0, 0 },
+				{ 0, 0, 0, 0, 80000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 400000000, 480000000, 480000000, 480000000, 480000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 },
+				{ 300000000, 300000000, 400000000, 400000000, 400000000 }
+				},
+		.reg = { "vfe_lite1" },
+		.interrupt = { "vfe_lite1" },
+		.line_num = 4,
+		.is_lite = true,
+		.ops = &vfe_ops_780,
+	},
+};
+
+static const struct resources_icc icc_res_sm8550[] = {
+	{
+		.name = "cam_ahb",
+		.icc_bw_tbl.avg = 2097152,
+		.icc_bw_tbl.peak = 2097152,
+	},
+	{
+		.name = "cam_hf_0_mnoc",
+		.icc_bw_tbl.avg = 2097152,
+		.icc_bw_tbl.peak = 2097152,
+	},
+	{
+		.name = "cam_sf_0_mnoc",
+		.icc_bw_tbl.avg = 2097152,
+		.icc_bw_tbl.peak = 2097152,
+	},
+	{
+		.name = "cam_sf_icp_mnoc",
+		.icc_bw_tbl.avg = 2097152,
+		.icc_bw_tbl.peak = 2097152,
+	},
+};
+
 /*
  * camss_add_clock_margin - Add margin to clock frequency rate
  * @rate: Clock frequency rate
@@ -2182,6 +2513,19 @@ static const struct camss_resources sc8280xp_resources = {
 	.vfe_num = ARRAY_SIZE(vfe_res_sc8280xp),
 };
 
+static const struct camss_resources sm8550_resources = {
+	.version = CAMSS_8550,
+	.pd_name = "top",
+	.csiphy_res = csiphy_res_8550,
+	.csid_res = csid_res_8550,
+	.vfe_res = vfe_res_8550,
+	.icc_res = icc_res_sm8550,
+	.icc_path_num = ARRAY_SIZE(icc_res_sm8550),
+	.csiphy_num = ARRAY_SIZE(csiphy_res_8550),
+	.csid_num = ARRAY_SIZE(csid_res_8550),
+	.vfe_num = ARRAY_SIZE(vfe_res_8550),
+};
+
 static const struct of_device_id camss_dt_match[] = {
 	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
 	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
@@ -2189,6 +2533,7 @@ static const struct of_device_id camss_dt_match[] = {
 	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
 	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
 	{ .compatible = "qcom,sc8280xp-camss", .data = &sc8280xp_resources },
+	{ .compatible = "qcom,sm8550-camss", .data = &sm8550_resources },
 	{ }
 };
 
-- 
2.17.1

Re: [PATCH v2 7/8] media: qcom: camss: Add sm8550 resources

[Krzysztof Kozlowski]

On 20/03/2024 15:11, Depeng Shao wrote:
>  
> +static const struct camss_resources sm8550_resources = {
> +	.version = CAMSS_8550,
> +	.pd_name = "top",
> +	.csiphy_res = csiphy_res_8550,
> +	.csid_res = csid_res_8550,
> +	.vfe_res = vfe_res_8550,
> +	.icc_res = icc_res_sm8550,
> +	.icc_path_num = ARRAY_SIZE(icc_res_sm8550),
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_8550),
> +	.csid_num = ARRAY_SIZE(csid_res_8550),
> +	.vfe_num = ARRAY_SIZE(vfe_res_8550),
> +};
> +
>  static const struct of_device_id camss_dt_match[] = {
>  	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
>  	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
> @@ -2189,6 +2533,7 @@ static const struct of_device_id camss_dt_match[] = {
>  	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
>  	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
>  	{ .compatible = "qcom,sc8280xp-camss", .data = &sc8280xp_resources },
> +	{ .compatible = "qcom,sm8550-camss", .data = &sm8550_resources },

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Do not ignore checkpatch warnings.

Also, because you did not post bindings, I am afraid of sneaking here
stuff which is not accepted upstream. Therefore please post DTS in
separate patchset, for public review.
>  	{ }
>  };
>  

Best regards,
Krzysztof

[PATCH v2 8/8] media: qcom: camss: Add sm8550 support

[Depeng Shao]

Add in functional logic throughout the code to support the SM8550.

Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
 .../media/platform/qcom/camss/camss-csid.c    | 19 +++++++++++++++++++
 .../media/platform/qcom/camss/camss-csiphy.c  |  1 +
 drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++++++
 .../media/platform/qcom/camss/camss-video.c   |  1 +
 4 files changed, 28 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index eb27d69e89a1..e9203dc15798 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -590,6 +590,25 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 			csid->base = camss->vfe[id].base + VFE_480_LITE_CSID_OFFSET;
 		else
 			csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
+	} else if (camss->res->version == CAMSS_8550) {
+		/* for titan 780, CSID lite registers are inside the VFE lite region,
+		 * between the VFE "top" and "bus" registers. this requires
+		 * VFE to be initialized before CSID
+		 */
+		if (id >= 2)
+			csid->base = camss->vfe[id].base;
+		else {
+			csid->base =
+				devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
+			if (id != 0)
+				csid->top_base = camss->csid[0].top_base;
+			else
+				csid->top_base =
+					devm_platform_ioremap_resource_byname(pdev, res->reg[1]);
+		}
+
+		if (IS_ERR(csid->base))
+			return PTR_ERR(csid->base);
 	} else {
 		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
 		if (IS_ERR(csid->base))
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index 45b3a8e5dea4..f35af0dd2147 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -579,6 +579,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
 	case CAMSS_845:
 	case CAMSS_8250:
 	case CAMSS_8280XP:
+	case CAMSS_8550:
 		csiphy->formats = csiphy_formats_sdm845;
 		csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
 		break;
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index d875237cf244..ff115c5521c6 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -226,6 +226,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_8550:
 		switch (sink_code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
 		{
@@ -296,6 +297,10 @@ int vfe_reset(struct vfe_device *vfe)
 
 	reinit_completion(&vfe->reset_complete);
 
+	// The reset has been moved to csid in 8550
+	if (vfe->camss->res->version == CAMSS_8550)
+		return 0;
+
 	vfe->ops->global_reset(vfe);
 
 	time = wait_for_completion_timeout(&vfe->reset_complete,
@@ -1520,6 +1525,7 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 		case CAMSS_845:
 		case CAMSS_8250:
 		case CAMSS_8280XP:
+		case CAMSS_8550:
 			l->formats = formats_rdi_845;
 			l->nformats = ARRAY_SIZE(formats_rdi_845);
 			break;
@@ -1605,6 +1611,7 @@ static int vfe_bpl_align(struct vfe_device *vfe)
 	case CAMSS_845:
 	case CAMSS_8250:
 	case CAMSS_8280XP:
+	case CAMSS_8550:
 		ret = 16;
 		break;
 	default:
diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 54cd82f74115..78a746be952c 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -1029,6 +1029,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 	case CAMSS_845:
 	case CAMSS_8250:
 	case CAMSS_8280XP:
+	case CAMSS_8550:
 		video->formats = formats_rdi_845;
 		video->nformats = ARRAY_SIZE(formats_rdi_845);
 		break;
-- 
2.17.1

Re: [PATCH v2 8/8] media: qcom: camss: Add sm8550 support

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> Add in functional logic throughout the code to support the SM8550.
> 
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---
>   .../media/platform/qcom/camss/camss-csid.c    | 19 +++++++++++++++++++
>   .../media/platform/qcom/camss/camss-csiphy.c  |  1 +
>   drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++++++
>   .../media/platform/qcom/camss/camss-video.c   |  1 +
>   4 files changed, 28 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index eb27d69e89a1..e9203dc15798 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -590,6 +590,25 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   			csid->base = camss->vfe[id].base + VFE_480_LITE_CSID_OFFSET;
>   		else
>   			csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
> +	} else if (camss->res->version == CAMSS_8550) {
> +		/* for titan 780, CSID lite registers are inside the VFE lite region,
> +		 * between the VFE "top" and "bus" registers. this requires
> +		 * VFE to be initialized before CSID
> +		 */
> +		if (id >= 2)
> +			csid->base = camss->vfe[id].base;

Hard-coded magic numbers are definitely out.

If you need to differentiate - please include something in the struct 
resources so that the flag is always available and we don't have to 
start doing funky magic index/magic number gymnastics.

> +		else {
> +			csid->base =
> +				devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
> +			if (id != 0)
> +				csid->top_base = camss->csid[0].top_base;
> +			else
> +				csid->top_base =
> +					devm_platform_ioremap_resource_byname(pdev, res->reg[1]);
> +		}

What is the point of hooking the TOP base just to clear our the status 
registers ?

We take no meaningful action in the ISR that I can see.


> +
> +		if (IS_ERR(csid->base))
> +			return PTR_ERR(csid->base);
>   	} else {
>   		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
>   		if (IS_ERR(csid->base))
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 45b3a8e5dea4..f35af0dd2147 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -579,6 +579,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_8550:
>   		csiphy->formats = csiphy_formats_sdm845;
>   		csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
>   		break;
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index d875237cf244..ff115c5521c6 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -226,6 +226,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_8550:
>   		switch (sink_code) {
>   		case MEDIA_BUS_FMT_YUYV8_1X16:
>   		{
> @@ -296,6 +297,10 @@ int vfe_reset(struct vfe_device *vfe)
>   
>   	reinit_completion(&vfe->reset_complete);
>   
> +	// The reset has been moved to csid in 8550

Please run checkpatch.pl on your code before submission C++ are not allowed.

---
bod

Re: [PATCH v2 8/8] media: qcom: camss: Add sm8550 support

[Depeng Shao]

Hi Bryan,

On 3/21/2024 12:01 AM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
>> Add in functional logic throughout the code to support the SM8550.
>>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>> ---
>>   .../media/platform/qcom/camss/camss-csid.c    | 19 +++++++++++++++++++
>>   .../media/platform/qcom/camss/camss-csiphy.c  |  1 +
>>   drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++++++
>>   .../media/platform/qcom/camss/camss-video.c   |  1 +
>>   4 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c 
>> b/drivers/media/platform/qcom/camss/camss-csid.c
>> index eb27d69e89a1..e9203dc15798 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -590,6 +590,25 @@ int msm_csid_subdev_init(struct camss *camss, 
>> struct csid_device *csid,
>>               csid->base = camss->vfe[id].base + 
>> VFE_480_LITE_CSID_OFFSET;
>>           else
>>               csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
>> +    } else if (camss->res->version == CAMSS_8550) {
>> +        /* for titan 780, CSID lite registers are inside the VFE lite 
>> region,
>> +         * between the VFE "top" and "bus" registers. this requires
>> +         * VFE to be initialized before CSID
>> +         */
>> +        if (id >= 2)
>> +            csid->base = camss->vfe[id].base;
> 
> Hard-coded magic numbers are definitely out.
> 
> If you need to differentiate - please include something in the struct 
> resources so that the flag is always available and we don't have to 
> start doing funky magic index/magic number gymnastics.
> 

Sure, will update this part.

>> +        else {
>> +            csid->base =
>> +                devm_platform_ioremap_resource_byname(pdev, 
>> res->reg[0]);
>> +            if (id != 0)
>> +                csid->top_base = camss->csid[0].top_base;
>> +            else
>> +                csid->top_base =
>> +                    devm_platform_ioremap_resource_byname(pdev, 
>> res->reg[1]);
>> +        }
> 
> What is the point of hooking the TOP base just to clear our the status 
> registers ?
> 
> We take no meaningful action in the ISR that I can see.
>

The csid top is a new small hardware block, it controls the output of 
CSID, since the CSID can connect to Sensor Front End(SFE) or original 
VFE in CSID gen3.

> 
>> +
>> +        if (IS_ERR(csid->base))
>> +            return PTR_ERR(csid->base);
>>       } else {
>>           csid->base = devm_platform_ioremap_resource_byname(pdev, 
>> res->reg[0]);
>>           if (IS_ERR(csid->base))
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c 
>> b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> index 45b3a8e5dea4..f35af0dd2147 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> @@ -579,6 +579,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
>>       case CAMSS_845:
>>       case CAMSS_8250:
>>       case CAMSS_8280XP:
>> +    case CAMSS_8550:
>>           csiphy->formats = csiphy_formats_sdm845;
>>           csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
>>           break;
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c 
>> b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index d875237cf244..ff115c5521c6 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -226,6 +226,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_8550:
>>           switch (sink_code) {
>>           case MEDIA_BUS_FMT_YUYV8_1X16:
>>           {
>> @@ -296,6 +297,10 @@ int vfe_reset(struct vfe_device *vfe)
>>       reinit_completion(&vfe->reset_complete);
>> +    // The reset has been moved to csid in 8550
> 
> Please run checkpatch.pl on your code before submission C++ are not 
> allowed.
> 
> ---
> bod

Thanks,
Depeng

Re: [PATCH v2 0/8] media: qcom: camss: Add sm8550 support

[Bryan O'Donoghue]

On 20/03/2024 14:11, Depeng Shao wrote:
> V2:
> - Update some commit messages
> Link to V1: https://lore.kernel.org/all/20240320134227.16587-1-quic_depengs@quicinc.com/

Here's your first problem, doesn't apply.

Please rebase on - at a minimum linux-next or on top of this branch

https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/b4/camss-sc8280xp-v6?ref_type=heads

and indicate which branch you've done the work against.

deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git fetch 
linux-next
remote: Enumerating objects: 1907, done.
remote: Counting objects: 100% (1860/1860), done.
remote: Compressing objects: 100% (491/491), done.
remote: Total 1907 (delta 1432), reused 1759 (delta 1367), pack-reused 47
Receiving objects: 100% (1907/1907), 1.16 MiB | 8.24 MiB/s, done.
Resolving deltas: 100% (1461/1461), completed with 263 local objects.
 From git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next
  + 226d3c72fcde1...72fb52fb0ac44 master        -> linux-next/master 
(forced update)
  + 00fe0619c6bef...ad58b0c356a01 pending-fixes -> 
linux-next/pending-fixes  (forced update)
    bf3a69c6861ff..78c3925c048c7  stable        -> linux-next/stable
  * [new tag]                     next-20240320 -> next-20240320

deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git checkout -b 
linux-next-24-03-20-sm8550 linux-next/master

Updating files: 100% (18508/18508), done.
branch 'linux-next-24-03-20-sm8550' set up to track 'linux-next/master'.
Switched to a new branch 'linux-next-24-03-20-sm8550'
deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ b4 shazam 
20240320141136.26827-1-quic_depengs@quicinc.com
Grabbing thread from 
lore.kernel.org/all/20240320141136.26827-1-quic_depengs@quicinc.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 10 messages in the thread
Checking attestation on all messages, may take a moment...
---
   [PATCH v2 1/8] media: qcom: camss: Add CAMSS_8550 enum
     + Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
   [PATCH v2 2/8] media: qcom: camss: Add subdev notify support
   [PATCH v2 3/8] media: qcom: camss: Add new csiphy driver 2-1-2
   [PATCH v2 4/8] media: qcom: camss: Add new params for csid_device
   [PATCH v2 5/8] media: qcom: camss: Add CSID gen3 driver
   [PATCH v2 6/8] media: qcom: camss: Add new VFE driver for SM8550
   [PATCH v2 7/8] media: qcom: camss: Add sm8550 resources
   [PATCH v2 8/8] media: qcom: camss: Add sm8550 support
   ---
   NOTE: install dkimpy for DKIM signature verification
---
Total patches: 8
---
Applying: media: qcom: camss: Add CAMSS_8550 enum
Patch failed at 0001 media: qcom: camss: Add CAMSS_8550 enum
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
error: patch failed: drivers/media/platform/qcom/camss/camss.h:78
error: drivers/media/platform/qcom/camss/camss.h: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch

---
bod