linux-kernel - Re: [PATCH 12/13] media: qcom: camss: Add sm8550 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4151a353-1cc0-436d-a86e-8d489311c6ef@quicinc.com>
Date: Thu, 11 Jul 2024 22:36:33 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
        <todor.too@...il.com>, <mchehab@...nel.org>, <robh@...nel.org>,
        <krzk+dt@...nel.org>, <conor+dt@...nel.org>
CC: <quic_eberman@...cinc.com>, <linux-media@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <kernel@...cinc.com>
Subject: Re: [PATCH 12/13] media: qcom: camss: Add sm8550 support

Hi Bryan,


On 7/10/2024 8:02 PM, Bryan O'Donoghue wrote:
> On 09/07/2024 17:06, Depeng Shao wrote:

>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c 
>> b/drivers/media/platform/qcom/camss/camss-csid.c
>> index 858db5d4ca75..90fba25db4c6 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -1013,6 +1013,7 @@ int msm_csid_subdev_init(struct camss *camss, 
>> struct csid_device *csid,
>>   {
>>       struct device *dev = camss->dev;
>>       struct platform_device *pdev = to_platform_device(dev);
>> +    struct resource *top_res;
>>       int i, j;
>>       int ret;
>> @@ -1040,6 +1041,26 @@ int msm_csid_subdev_init(struct camss *camss, 
>> struct csid_device *csid,
>>           else
>>               csid->base = 
>> csid->res->parent_dev_ops->get_base_address(camss, id)
>>                    + VFE_480_CSID_OFFSET;
>> +    } else 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 (csid_is_lite(csid))
>> +            csid->base =  
>> csid->res->parent_dev_ops->get_base_address(camss, id);
>> +        else {
>> +            csid->base = devm_platform_ioremap_resource_byname(pdev, 
>> res->reg[0]);
>> +            /* CSID "top" is a new function in Titan780.
>> +             * CSID can connect to VFE & SFE(Sensor Front End).
>> +             * This connection is ontrolled by CSID "top" registers.
>> +             * CSID "top" registers at only one region.
>> +             */
>> +            top_res = platform_get_resource_byname(pdev, 
>> IORESOURCE_MEM, res->reg[1]);
>> +            csid->top_base = ioremap(top_res->start, 
>> resource_size(top_res));
>> +        }
> 
> If we just specify the csid lite regs in the dts, we don't need custom 
> code to get a pointer to them.
> 

Yes, we can get the csid lite reg directly, will update this part.

> Similarly the csid->top_base = () should be generic not SoC specific, 
> i.e. we should be able to add in CSID 980 without adding any custom code 
> to camss-csid.c
> 

csid lite doesn't have top reg block, but you are right, this can be 
generic, we can get csid->top_base if res->reg[1] isn't NULL.


>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c 
>> b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index 83c5a36d071f..479474c1cd95 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -338,6 +338,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, 
>> u32 sink_code,
>>       case CAMSS_845:
>>       case CAMSS_8250:
>>       case CAMSS_8280XP:
>> +    case CAMSS_8550:
>>           switch (sink_code) {
>>           case MEDIA_BUS_FMT_YUYV8_1X16:
>>           {
>> @@ -408,6 +409,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;
>> +
> 
> Custom code for a specific SoC in camss.c camss-csid.c or camss-vfe.c 
> indicates to me we need to do more work elsewhere.
> 
> This would do the same job for you.
> 
> static void vfe_global_reset(struct vfe_device *vfe)
> {
>          /* VFE780 has no global reset, simply report a completion */
>          complete(&vfe->reset_complete);
> }
> 
> const struct vfe_hw_ops vfe_ops_780 = {
>          .global_reset = vfe_global_reset,
> 
>>       vfe->res->hw_ops->global_reset(vfe);
>>       time = wait_for_completion_timeout(&vfe->reset_complete

Sure, I will optimize this part.


Thanks,
Depeng