2024-03-20 14:14:45

by Depeng Shao

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

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

Signed-off-by: Depeng Shao <[email protected]>
---
.../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

2024-03-20 16:02:01

by Bryan O'Donoghue

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

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 <[email protected]>
> ---
> .../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

2024-03-25 15:41:22

by Depeng Shao

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

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 <[email protected]>
>> ---
>>   .../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