[PATCH v2 0/6] media: dt-bindings: media: camss: Fix interrupt types

[PATCH v2 0/6] media: dt-bindings: media: camss: Fix interrupt types

[Vladimir Zapolskiy]

It was discovered that on a few Qualcomm platforms types of interrupts
do not match both downstream code and a type requested by the CAMSS driver.

The mismatched interrupt type between firmware and the correspondent CAMSS
driver leads to known problems, which were discussed previously:

  https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/

Here the situation is right the same, namely a repeated bind of camss device
is not possible due to a wrongly specified interrupt type, and it may lead
to an issue in runtime manifested like this:

  irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!

Changes from v1 to v2:
* added gained Acked-by, Tested-by and Reviewed-by tags,
* per patch review requests from Krzysztof deduplicated "media:" from subjects.

Link to v1 of the changeset:

  https://lore.kernel.org/all/20240905164142.3475873-1-vladimir.zapolskiy@linaro.org/

Vladimir Zapolskiy (6):
  dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types
  dt-bindings: media: qcom,sdm845-camss: Fix interrupt types
  dt-bindings: media: qcom,sm8250-camss: Fix interrupt types
  arm64: dts: qcom: sc8280xp: Fix interrupt type of camss interrupts
  arm64: dts: qcom: sdm845: Fix interrupt types of camss interrupts
  arm64: dts: qcom: sm8250: Fix interrupt types of camss interrupts

 .../bindings/media/qcom,sc8280xp-camss.yaml   | 40 +++++++++----------
 .../bindings/media/qcom,sdm845-camss.yaml     | 20 +++++-----
 .../bindings/media/qcom,sm8250-camss.yaml     | 28 ++++++-------
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi        | 40 +++++++++----------
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 20 +++++-----
 arch/arm64/boot/dts/qcom/sm8250.dtsi          | 28 ++++++-------
 6 files changed, 88 insertions(+), 88 deletions(-)

-- 
2.45.2

[PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

The expected type of all CAMSS interrupts is edge rising, fix it in
the documented example from CAMSS device tree bindings for sc8280xp.

Fixes: bc5191e5799e ("media: dt-bindings: media: camss: Add qcom,sc8280xp-camss binding")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/media/qcom,sc8280xp-camss.yaml   | 40 +++++++++----------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
index c0bc31709873..9936f0132417 100644
--- a/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
@@ -328,26 +328,26 @@ examples:
             vdda-phy-supply = <&vreg_l6d>;
             vdda-pll-supply = <&vreg_l4d>;
 
-            interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 758 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 759 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 760 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 762 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
+            interrupts = <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 758 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 759 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 760 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 761 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 762 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 764 IRQ_TYPE_EDGE_RISING>;
 
             interrupt-names = "csid1_lite",
                               "vfe_lite1",
-- 
2.45.2

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Bjorn Andersson]

On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
> The expected type of all CAMSS interrupts is edge rising, fix it in
> the documented example from CAMSS device tree bindings for sc8280xp.
> 

Who/what expects them to be RISING?

Regards,
Bjorn

> Fixes: bc5191e5799e ("media: dt-bindings: media: camss: Add qcom,sc8280xp-camss binding")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../bindings/media/qcom,sc8280xp-camss.yaml   | 40 +++++++++----------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
> index c0bc31709873..9936f0132417 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
> @@ -328,26 +328,26 @@ examples:
>              vdda-phy-supply = <&vreg_l6d>;
>              vdda-pll-supply = <&vreg_l4d>;
>  
> -            interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 758 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 759 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 760 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 762 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupts = <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 758 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 759 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 760 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 761 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 762 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 764 IRQ_TYPE_EDGE_RISING>;
>  
>              interrupt-names = "csid1_lite",
>                                "vfe_lite1",
> -- 
> 2.45.2
> 

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

Hi Bjorn,

On 10/6/24 05:36, Bjorn Andersson wrote:
> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>> The expected type of all CAMSS interrupts is edge rising, fix it in
>> the documented example from CAMSS device tree bindings for sc8280xp.
>>
> 
> Who/what expects them to be RISING?

I've checked CAMSS device tree bindings in a number of downstream kernels,
all of them describe interrupt types as edge rising.

$ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);

 From runtime point of view it's more important to get re-probed camss
driver, see an absolutely similar and previously discussed case (in the
cover letter):

https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/

Now in runtime I get this error, it's easy to check by unbinding/binding any
camss device:

irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!

Basically camss devices can not be bound on the second time on the
number of platforms touched by this changeset.

>> Fixes: bc5191e5799e ("media: dt-bindings: media: camss: Add qcom,sc8280xp-camss binding")
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>   .../bindings/media/qcom,sc8280xp-camss.yaml   | 40 +++++++++----------
>>   1 file changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
>> index c0bc31709873..9936f0132417 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
>> @@ -328,26 +328,26 @@ examples:
>>               vdda-phy-supply = <&vreg_l6d>;
>>               vdda-pll-supply = <&vreg_l4d>;
>>   
>> -            interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 758 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 759 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 760 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 762 IRQ_TYPE_LEVEL_HIGH>,
>> -                         <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
>> +            interrupts = <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 758 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 759 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 760 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 761 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 762 IRQ_TYPE_EDGE_RISING>,
>> +                         <GIC_SPI 764 IRQ_TYPE_EDGE_RISING>;
>>   
>>               interrupt-names = "csid1_lite",
>>                                 "vfe_lite1",
>> -- 
>> 2.45.2
>>

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Krzysztof Kozlowski]

On 08/10/2024 12:02, Vladimir Zapolskiy wrote:
> Hi Bjorn,
> 
> On 10/6/24 05:36, Bjorn Andersson wrote:
>> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>>> The expected type of all CAMSS interrupts is edge rising, fix it in
>>> the documented example from CAMSS device tree bindings for sc8280xp.
>>>
>>
>> Who/what expects them to be RISING?
> 
> I've checked CAMSS device tree bindings in a number of downstream kernels,
> all of them describe interrupt types as edge rising.
> 
> $ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
> drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
> drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
> drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
> drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
> drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);

Downstream has a lot of bad code, so I am not sure how good argument
this is.

I acked the patch because I assumed you *checked in hardware*.

> 
>  From runtime point of view it's more important to get re-probed camss
> driver, see an absolutely similar and previously discussed case (in the
> cover letter):
> 
> https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
> 
> Now in runtime I get this error, it's easy to check by unbinding/binding any
> camss device:
> 
> irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
> 
> Basically camss devices can not be bound on the second time on the
> number of platforms touched by this changeset.

This is solveable different way and I do not understand this rationale.
The driver should not request trigger type but use what DTS is
providing, unless of course only one valid trigger is possible. But so
far you did not provide any arguments for this. Downstream crappy code?
Nope. Existing driver? Same.

Was anything here actually checked with datasheets/hardware?

Best regards,
Krzysztof

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

Hi Krzysztof.

On 10/8/24 14:15, Krzysztof Kozlowski wrote:
> On 08/10/2024 12:02, Vladimir Zapolskiy wrote:
>> Hi Bjorn,
>>
>> On 10/6/24 05:36, Bjorn Andersson wrote:
>>> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>>>> The expected type of all CAMSS interrupts is edge rising, fix it in
>>>> the documented example from CAMSS device tree bindings for sc8280xp.
>>>>
>>>
>>> Who/what expects them to be RISING?
>>
>> I've checked CAMSS device tree bindings in a number of downstream kernels,
>> all of them describe interrupt types as edge rising.
>>
>> $ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
>> drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>> drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>> drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>> drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>> drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
> 
> Downstream has a lot of bad code, so I am not sure how good argument
> this is.
> 
> I acked the patch because I assumed you *checked in hardware*.
> 
>>
>>   From runtime point of view it's more important to get re-probed camss
>> driver, see an absolutely similar and previously discussed case (in the
>> cover letter):
>>
>> https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
>>
>> Now in runtime I get this error, it's easy to check by unbinding/binding any
>> camss device:
>>
>> irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
>>
>> Basically camss devices can not be bound on the second time on the
>> number of platforms touched by this changeset.
> 
> This is solveable different way and I do not understand this rationale.
> The driver should not request trigger type but use what DTS is
> providing, unless of course only one valid trigger is possible.

Right at the moment the driver uses rising edge type of interrupts, and
it works properly.

> But so
> far you did not provide any arguments for this. Downstream crappy code?

Downstream code works, that's the argument to support the change.

> Nope. Existing driver? Same.

The existing driver works, that's the argument to support the change.

> Was anything here actually checked with datasheets/hardware?

The initially open question is unanswered, why sc8280xp CAMSS does
specify interrupts as level high type, was it actually checked with
datasheets/hardware, as you say it? It has never been tested by anyone
and anywhere, downstream or upstream wise, only rising edge interrupts
were tested, and they do work.

I don't have access to datasheets or hardware of sc8280xp powered board,
someone may either verify, if CAMSS level high type interrupts are
supported/working at all or not (obviously its current presence in dts is
insufficient), or check the SoC datasheet.

To sum up, the intention of this change:
1) fix the unpleasant runtime issue with no regressions (it's been tested),
2) align CAMSS device description in firmware with known to work well
IP hardware configuration.

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Krzysztof Kozlowski]

On 08/10/2024 13:37, Vladimir Zapolskiy wrote:
> Hi Krzysztof.
> 
> On 10/8/24 14:15, Krzysztof Kozlowski wrote:
>> On 08/10/2024 12:02, Vladimir Zapolskiy wrote:
>>> Hi Bjorn,
>>>
>>> On 10/6/24 05:36, Bjorn Andersson wrote:
>>>> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>>>>> The expected type of all CAMSS interrupts is edge rising, fix it in
>>>>> the documented example from CAMSS device tree bindings for sc8280xp.
>>>>>
>>>>
>>>> Who/what expects them to be RISING?
>>>
>>> I've checked CAMSS device tree bindings in a number of downstream kernels,
>>> all of them describe interrupt types as edge rising.
>>>
>>> $ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
>>> drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>> drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>> drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>> drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>> drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
>>
>> Downstream has a lot of bad code, so I am not sure how good argument
>> this is.
>>
>> I acked the patch because I assumed you *checked in hardware*.
>>
>>>
>>>   From runtime point of view it's more important to get re-probed camss
>>> driver, see an absolutely similar and previously discussed case (in the
>>> cover letter):
>>>
>>> https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
>>>
>>> Now in runtime I get this error, it's easy to check by unbinding/binding any
>>> camss device:
>>>
>>> irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
>>>
>>> Basically camss devices can not be bound on the second time on the
>>> number of platforms touched by this changeset.
>>
>> This is solveable different way and I do not understand this rationale.
>> The driver should not request trigger type but use what DTS is
>> providing, unless of course only one valid trigger is possible.
> 
> Right at the moment the driver uses rising edge type of interrupts, and
> it works properly.
> 
>> But so
>> far you did not provide any arguments for this. Downstream crappy code?
> 
> Downstream code works, that's the argument to support the change.

That is not acceptable argument. If downstream has a bug, but somehow
works, you will implement the same bug upstream?

Downstream is well known of shortcuts, incomplete solutions and crappy
code, which passes some tests but might not be really correct.

I understand that downstream can be a supportive case, but not for level
of interrupts! People, not only downstream but it's even worse there, do
not see the difference between level and edge, between GPIO ACTIVE_HIGH
and ACTIVE_LOW.

> 
>> Nope. Existing driver? Same.
> 
> The existing driver works, that's the argument to support the change.

We are not going to get into such discussions. Code might be incorrect,
but mostly works because race issues are very tricky to spot, yet you
use that code as argument to say hardware is like that.

No. Hardware is not because driver is written that way.


> 
>> Was anything here actually checked with datasheets/hardware?
> 
> The initially open question is unanswered, why sc8280xp CAMSS does

This is about all CAMSS, not only sc8280xp.

> specify interrupts as level high type, was it actually checked with
> datasheets/hardware, as you say it? It has never been tested by anyone
> and anywhere, downstream or upstream wise, only rising edge interrupts
> were tested, and they do work.

I did not ask about testing. I ask how the manual, hardware engineers
designed it.

> 
> I don't have access to datasheets or hardware of sc8280xp powered board,
> someone may either verify, if CAMSS level high type interrupts are> supported/working at all or not (obviously its current presence in dts is
> insufficient), or check the SoC datasheet.
> 
> To sum up, the intention of this change:
> 1) fix the unpleasant runtime issue with no regressions (it's been tested),

Did you test races and all the tricky issues arising when you use
incorrectly edged interrupts? Or you just checked that "interrupt works"?

> 2) align CAMSS device description in firmware with known to work well
> IP hardware configuration.

Where is this description in firmware? Where is this IP hardware
configuration? You just said it is purely on downstream driver.

Best regards,
Krzysztof

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

On 10/8/24 14:45, Krzysztof Kozlowski wrote:
> On 08/10/2024 13:37, Vladimir Zapolskiy wrote:
>> Hi Krzysztof.
>>
>> On 10/8/24 14:15, Krzysztof Kozlowski wrote:
>>> On 08/10/2024 12:02, Vladimir Zapolskiy wrote:
>>>> Hi Bjorn,
>>>>
>>>> On 10/6/24 05:36, Bjorn Andersson wrote:
>>>>> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>>>>>> The expected type of all CAMSS interrupts is edge rising, fix it in
>>>>>> the documented example from CAMSS device tree bindings for sc8280xp.
>>>>>>
>>>>>
>>>>> Who/what expects them to be RISING?
>>>>
>>>> I've checked CAMSS device tree bindings in a number of downstream kernels,
>>>> all of them describe interrupt types as edge rising.
>>>>
>>>> $ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
>>>> drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>>> drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>>> drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>>> drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>>> drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
>>>
>>> Downstream has a lot of bad code, so I am not sure how good argument
>>> this is.
>>>
>>> I acked the patch because I assumed you *checked in hardware*.
>>>
>>>>
>>>>    From runtime point of view it's more important to get re-probed camss
>>>> driver, see an absolutely similar and previously discussed case (in the
>>>> cover letter):
>>>>
>>>> https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
>>>>
>>>> Now in runtime I get this error, it's easy to check by unbinding/binding any
>>>> camss device:
>>>>
>>>> irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
>>>>
>>>> Basically camss devices can not be bound on the second time on the
>>>> number of platforms touched by this changeset.
>>>
>>> This is solveable different way and I do not understand this rationale.
>>> The driver should not request trigger type but use what DTS is
>>> providing, unless of course only one valid trigger is possible.
>>
>> Right at the moment the driver uses rising edge type of interrupts, and
>> it works properly.
>>
>>> But so
>>> far you did not provide any arguments for this. Downstream crappy code?
>>
>> Downstream code works, that's the argument to support the change.
> 
> That is not acceptable argument. If downstream has a bug, but somehow
> works, you will implement the same bug upstream?
> 
> Downstream is well known of shortcuts, incomplete solutions and crappy
> code, which passes some tests but might not be really correct.
> 
> I understand that downstream can be a supportive case, but not for level
> of interrupts! People, not only downstream but it's even worse there, do
> not see the difference between level and edge, between GPIO ACTIVE_HIGH
> and ACTIVE_LOW.
> 
>>
>>> Nope. Existing driver? Same.
>>
>> The existing driver works, that's the argument to support the change.
> 
> We are not going to get into such discussions. Code might be incorrect,
> but mostly works because race issues are very tricky to spot, yet you
> use that code as argument to say hardware is like that.
> 
> No. Hardware is not because driver is written that way.
> 
> 
>>
>>> Was anything here actually checked with datasheets/hardware?
>>
>> The initially open question is unanswered, why sc8280xp CAMSS does
> 
> This is about all CAMSS, not only sc8280xp.
> 
>> specify interrupts as level high type, was it actually checked with
>> datasheets/hardware, as you say it? It has never been tested by anyone
>> and anywhere, downstream or upstream wise, only rising edge interrupts
>> were tested, and they do work.
> 
> I did not ask about testing. I ask how the manual, hardware engineers
> designed it.
> 
>>
>> I don't have access to datasheets or hardware of sc8280xp powered board,
>> someone may either verify, if CAMSS level high type interrupts are> supported/working at all or not (obviously its current presence in dts is
>> insufficient), or check the SoC datasheet.
>>
>> To sum up, the intention of this change:
>> 1) fix the unpleasant runtime issue with no regressions (it's been tested),
> 
> Did you test races and all the tricky issues arising when you use
> incorrectly edged interrupts? Or you just checked that "interrupt works"?

Right from the beginning and any other day CAMSS interrupts are tested as
rising edge interrupts. So, I don't undestand your point here, please
elaborate.

>> 2) align CAMSS device description in firmware with known to work well
>> IP hardware configuration.
> 
> Where is this description in firmware? Where is this IP hardware
> configuration? You just said it is purely on downstream driver.

CAMSS IP configuration, in particular interrupt type, is done by the
upstream driver, note that the fixes in this changeset is also sent
against the upstream driver, tested on the upstream driver etc.

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Krzysztof Kozlowski]

On 08/10/2024 14:03, Vladimir Zapolskiy wrote:
> 
> 
> On 10/8/24 14:45, Krzysztof Kozlowski wrote:
>> On 08/10/2024 13:37, Vladimir Zapolskiy wrote:
>>> Hi Krzysztof.
>>>
>>> On 10/8/24 14:15, Krzysztof Kozlowski wrote:
>>>> On 08/10/2024 12:02, Vladimir Zapolskiy wrote:
>>>>> Hi Bjorn,
>>>>>
>>>>> On 10/6/24 05:36, Bjorn Andersson wrote:
>>>>>> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>>>>>>> The expected type of all CAMSS interrupts is edge rising, fix it in
>>>>>>> the documented example from CAMSS device tree bindings for sc8280xp.
>>>>>>>
>>>>>>
>>>>>> Who/what expects them to be RISING?
>>>>>
>>>>> I've checked CAMSS device tree bindings in a number of downstream kernels,
>>>>> all of them describe interrupt types as edge rising.
>>>>>
>>>>> $ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
>>>>> drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>>>> drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>>>> drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>>>> drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>>>> drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
>>>>
>>>> Downstream has a lot of bad code, so I am not sure how good argument
>>>> this is.
>>>>
>>>> I acked the patch because I assumed you *checked in hardware*.
>>>>
>>>>>
>>>>>    From runtime point of view it's more important to get re-probed camss
>>>>> driver, see an absolutely similar and previously discussed case (in the
>>>>> cover letter):
>>>>>
>>>>> https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
>>>>>
>>>>> Now in runtime I get this error, it's easy to check by unbinding/binding any
>>>>> camss device:
>>>>>
>>>>> irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
>>>>>
>>>>> Basically camss devices can not be bound on the second time on the
>>>>> number of platforms touched by this changeset.
>>>>
>>>> This is solveable different way and I do not understand this rationale.
>>>> The driver should not request trigger type but use what DTS is
>>>> providing, unless of course only one valid trigger is possible.
>>>
>>> Right at the moment the driver uses rising edge type of interrupts, and
>>> it works properly.
>>>
>>>> But so
>>>> far you did not provide any arguments for this. Downstream crappy code?
>>>
>>> Downstream code works, that's the argument to support the change.
>>
>> That is not acceptable argument. If downstream has a bug, but somehow
>> works, you will implement the same bug upstream?
>>
>> Downstream is well known of shortcuts, incomplete solutions and crappy
>> code, which passes some tests but might not be really correct.
>>
>> I understand that downstream can be a supportive case, but not for level
>> of interrupts! People, not only downstream but it's even worse there, do
>> not see the difference between level and edge, between GPIO ACTIVE_HIGH
>> and ACTIVE_LOW.
>>
>>>
>>>> Nope. Existing driver? Same.
>>>
>>> The existing driver works, that's the argument to support the change.
>>
>> We are not going to get into such discussions. Code might be incorrect,
>> but mostly works because race issues are very tricky to spot, yet you
>> use that code as argument to say hardware is like that.
>>
>> No. Hardware is not because driver is written that way.
>>
>>
>>>
>>>> Was anything here actually checked with datasheets/hardware?
>>>
>>> The initially open question is unanswered, why sc8280xp CAMSS does
>>
>> This is about all CAMSS, not only sc8280xp.
>>
>>> specify interrupts as level high type, was it actually checked with
>>> datasheets/hardware, as you say it? It has never been tested by anyone
>>> and anywhere, downstream or upstream wise, only rising edge interrupts
>>> were tested, and they do work.
>>
>> I did not ask about testing. I ask how the manual, hardware engineers
>> designed it.
>>
>>>
>>> I don't have access to datasheets or hardware of sc8280xp powered board,
>>> someone may either verify, if CAMSS level high type interrupts are> supported/working at all or not (obviously its current presence in dts is
>>> insufficient), or check the SoC datasheet.
>>>
>>> To sum up, the intention of this change:
>>> 1) fix the unpleasant runtime issue with no regressions (it's been tested),
>>
>> Did you test races and all the tricky issues arising when you use
>> incorrectly edged interrupts? Or you just checked that "interrupt works"?
> 
> Right from the beginning and any other day CAMSS interrupts are tested as
> rising edge interrupts. So, I don't undestand your point here, please
> elaborate.

So you did not test whether these are correct interrupt types. What to
elaborate more? You have very tricky race condition, for example, so you
test that it is not possible.

> 
>>> 2) align CAMSS device description in firmware with known to work well
>>> IP hardware configuration.
>>
>> Where is this description in firmware? Where is this IP hardware
>> configuration? You just said it is purely on downstream driver.
> 
> CAMSS IP configuration, in particular interrupt type, is done by the
> upstream driver, note that the fixes in this changeset is also sent
> against the upstream driver, tested on the upstream driver etc.

What does it even mean? You said "device description in firmware" and
"IP hardware configuration", but now speak about drivers.

Best regards,
Krzysztof

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

On 10/8/24 15:06, Krzysztof Kozlowski wrote:
> On 08/10/2024 14:03, Vladimir Zapolskiy wrote:
>>
>>
>> On 10/8/24 14:45, Krzysztof Kozlowski wrote:
>>> On 08/10/2024 13:37, Vladimir Zapolskiy wrote:
>>>> Hi Krzysztof.
>>>>
>>>> On 10/8/24 14:15, Krzysztof Kozlowski wrote:
>>>>> On 08/10/2024 12:02, Vladimir Zapolskiy wrote:
>>>>>> Hi Bjorn,
>>>>>>
>>>>>> On 10/6/24 05:36, Bjorn Andersson wrote:
>>>>>>> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>>>>>>>> The expected type of all CAMSS interrupts is edge rising, fix it in
>>>>>>>> the documented example from CAMSS device tree bindings for sc8280xp.
>>>>>>>>
>>>>>>>
>>>>>>> Who/what expects them to be RISING?
>>>>>>
>>>>>> I've checked CAMSS device tree bindings in a number of downstream kernels,
>>>>>> all of them describe interrupt types as edge rising.
>>>>>>
>>>>>> $ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
>>>>>> drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>>>>> drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>>>>>> drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>>>>> drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>>>>>> drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
>>>>>
>>>>> Downstream has a lot of bad code, so I am not sure how good argument
>>>>> this is.
>>>>>
>>>>> I acked the patch because I assumed you *checked in hardware*.
>>>>>
>>>>>>
>>>>>>     From runtime point of view it's more important to get re-probed camss
>>>>>> driver, see an absolutely similar and previously discussed case (in the
>>>>>> cover letter):
>>>>>>
>>>>>> https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
>>>>>>
>>>>>> Now in runtime I get this error, it's easy to check by unbinding/binding any
>>>>>> camss device:
>>>>>>
>>>>>> irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
>>>>>>
>>>>>> Basically camss devices can not be bound on the second time on the
>>>>>> number of platforms touched by this changeset.
>>>>>
>>>>> This is solveable different way and I do not understand this rationale.
>>>>> The driver should not request trigger type but use what DTS is
>>>>> providing, unless of course only one valid trigger is possible.
>>>>
>>>> Right at the moment the driver uses rising edge type of interrupts, and
>>>> it works properly.
>>>>
>>>>> But so
>>>>> far you did not provide any arguments for this. Downstream crappy code?
>>>>
>>>> Downstream code works, that's the argument to support the change.
>>>
>>> That is not acceptable argument. If downstream has a bug, but somehow
>>> works, you will implement the same bug upstream?
>>>
>>> Downstream is well known of shortcuts, incomplete solutions and crappy
>>> code, which passes some tests but might not be really correct.
>>>
>>> I understand that downstream can be a supportive case, but not for level
>>> of interrupts! People, not only downstream but it's even worse there, do
>>> not see the difference between level and edge, between GPIO ACTIVE_HIGH
>>> and ACTIVE_LOW.
>>>
>>>>
>>>>> Nope. Existing driver? Same.
>>>>
>>>> The existing driver works, that's the argument to support the change.
>>>
>>> We are not going to get into such discussions. Code might be incorrect,
>>> but mostly works because race issues are very tricky to spot, yet you
>>> use that code as argument to say hardware is like that.
>>>
>>> No. Hardware is not because driver is written that way.
>>>
>>>
>>>>
>>>>> Was anything here actually checked with datasheets/hardware?
>>>>
>>>> The initially open question is unanswered, why sc8280xp CAMSS does
>>>
>>> This is about all CAMSS, not only sc8280xp.
>>>
>>>> specify interrupts as level high type, was it actually checked with
>>>> datasheets/hardware, as you say it? It has never been tested by anyone
>>>> and anywhere, downstream or upstream wise, only rising edge interrupts
>>>> were tested, and they do work.
>>>
>>> I did not ask about testing. I ask how the manual, hardware engineers
>>> designed it.
>>>
>>>>
>>>> I don't have access to datasheets or hardware of sc8280xp powered board,
>>>> someone may either verify, if CAMSS level high type interrupts are> supported/working at all or not (obviously its current presence in dts is
>>>> insufficient), or check the SoC datasheet.
>>>>
>>>> To sum up, the intention of this change:
>>>> 1) fix the unpleasant runtime issue with no regressions (it's been tested),
>>>
>>> Did you test races and all the tricky issues arising when you use
>>> incorrectly edged interrupts? Or you just checked that "interrupt works"?
>>
>> Right from the beginning and any other day CAMSS interrupts are tested as
>> rising edge interrupts. So, I don't undestand your point here, please
>> elaborate.
> 
> So you did not test whether these are correct interrupt types. What to
> elaborate more? You have very tricky race condition, for example, so you
> test that it is not possible.

Krzysztof, we are going rounds...

Every single user of CAMSS test only rising edge type of interrupts of
the IP. What are the races you are talking about? I kindly ask to read
the cover letter, it describes the problem fixed by the changeset.

>>>> 2) align CAMSS device description in firmware with known to work well
>>>> IP hardware configuration.
>>>
>>> Where is this description in firmware? Where is this IP hardware
>>> configuration? You just said it is purely on downstream driver.
>>
>> CAMSS IP configuration, in particular interrupt type, is done by the
>> upstream driver, note that the fixes in this changeset is also sent
>> against the upstream driver, tested on the upstream driver etc.
> 
> What does it even mean? You said "device description in firmware" and

"Device description in firmware" is DTB.

> "IP hardware configuration", but now speak about drivers.
> 

"IP hardware configuration" is done by the driver, this terminology does
not cause any surprises or ambiguity, I hope.

It's been always that "IP hardware configuration" of CAMSS interrupt types
completely ignores "device description in firmware" of CAMSS interrupt types.

By design due to endless problems with firmware like the one under discussion
interrupt types derived from firmware are ignored, and their correction in DTS
is problematic for whatever reason.

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Bryan O'Donoghue]

On 08/10/2024 12:37, Vladimir Zapolskiy wrote:
> 
> I don't have access to datasheets or hardware of sc8280xp powered board,
> someone may either verify, if CAMSS level high type interrupts are
> supported/working at all or not (obviously its current presence in dts is
> insufficient), or check the SoC datasheet.

I've tested both as was submitted and your change.

I _always_ test my patches. I'm not sure there's a datasheet which 
spells this out to be honest.

Rising or High can both be justified, its really down to how your 
interrupt controller latches the state change. However I personally am 
fine with the change you've provided because I trust it fixes an error 
for you.

I didn't try loading and unloading that module but, since you did I'm 
happy to Ack the change and trust your work.

---
bod

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

Hi Bryan,

On 10/8/24 14:50, Bryan O'Donoghue wrote:
> On 08/10/2024 12:37, Vladimir Zapolskiy wrote:
>>
>> I don't have access to datasheets or hardware of sc8280xp powered board,
>> someone may either verify, if CAMSS level high type interrupts are
>> supported/working at all or not (obviously its current presence in dts is
>> insufficient), or check the SoC datasheet.
> 
> I've tested both as was submitted and your change.

let me give you a correction, whatever is found in the CAMSS device tree
node is ignored - unless you meet the problem fixed by this changeset.

All what you see and on any variant of CAMSS device tree node is
rising edge interrupt, this is the type requested by the driver, and
I believe you've tested the driver.

> I _always_ test my patches. I'm not sure there's a datasheet which
> spells this out to be honest.
> 
> Rising or High can both be justified, its really down to how your
> interrupt controller latches the state change. However I personally am
> fine with the change you've provided because I trust it fixes an error
> for you.

Please share the change to the driver, which you've used to test
high level type of interrupts, shall it be send for upstream inclusion?

Such a change has never been a subject of discussion.

> I didn't try loading and unloading that module but, since you did I'm
> happy to Ack the change and trust your work.
> 

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Bryan O'Donoghue]

On 08/10/2024 13:00, Vladimir Zapolskiy wrote:
>> Rising or High can both be justified, its really down to how your
>> interrupt controller latches the state change. However I personally am
>> fine with the change you've provided because I trust it fixes an error
>> for you.
> 
> Please share the change to the driver, which you've used to test
> high level type of interrupts, shall it be send for upstream inclusion?
> 
> Such a change has never been a subject of discussion.

I tried running libcamera "cam" application to capture a data stream 
before and after your change - from memory at least on the sc8280xp and 
I think on 8250 too.

What I haven't tested is unloading and reloading the kernel module. My 
understanding of your bug report is your change fixes an error on reload.

?

---
bod

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

Hi Bryan,

On 10/8/24 16:24, Bryan O'Donoghue wrote:
> On 08/10/2024 13:00, Vladimir Zapolskiy wrote:
>>> Rising or High can both be justified, its really down to how your
>>> interrupt controller latches the state change. However I personally am
>>> fine with the change you've provided because I trust it fixes an error
>>> for you.
>>
>> Please share the change to the driver, which you've used to test
>> high level type of interrupts, shall it be send for upstream inclusion?
>>
>> Such a change has never been a subject of discussion.
> 
> I tried running libcamera "cam" application to capture a data stream
> before and after your change - from memory at least on the sc8280xp and
> I think on 8250 too.

it does not test high level type of CAMSS interrupts, I hope this is closed.

Nobody has ever tested high level type of CAMSS interrupts, and there is no
reason why they are specified in the platform dtsi file, but I repeat myself.

> What I haven't tested is unloading and reloading the kernel module. My
> understanding of your bug report is your change fixes an error on reload.
> 

Since you have access to the hardware, you are always welcome to make a simple
test, which was given in the recent past, but I do accept that quite many
things has to be repeated a few times in a row before people, me included,
grasp them:

https://lore.kernel.org/all/8d3e4ad1-82e3-42ad-80c2-dacd863e8e3e@linaro.org/

% echo -n ac5a000.camss > /sys/bus/platform/drivers/qcom-camss/unbind
% echo -n ac5a000.camss > /sys/bus/platform/drivers/qcom-camss/bind

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Bryan O'Donoghue]

On 08/10/2024 16:38, Vladimir Zapolskiy wrote:
> 
> % echo -n ac5a000.camss > /sys/bus/platform/drivers/qcom-camss/unbind
> % echo -n ac5a000.camss > /sys/bus/platform/drivers/qcom-camss/bind

Yes understood.

Lets go through the process of checking the qcom docs to make sure we 
are making the right change per Bjorn and Krzysztof's queries.

I'll do that, I have the necessary network credentials.

---
bod

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Depeng Shao]

Hi Bryan, Vladimir,

On 10/8/2024 11:51 PM, Bryan O'Donoghue wrote:
> On 08/10/2024 16:38, Vladimir Zapolskiy wrote:
>>
>> % echo -n ac5a000.camss > /sys/bus/platform/drivers/qcom-camss/unbind
>> % echo -n ac5a000.camss > /sys/bus/platform/drivers/qcom-camss/bind
> 
> Yes understood.
> 
> Lets go through the process of checking the qcom docs to make sure we 
> are making the right change per Bjorn and Krzysztof's queries.
> 
> I'll do that, I have the necessary network credentials.
> 

I have checked this, the trigger type of camera interrupt is _Edge_ what 
is listed in Qualcomm ipcatalog website.

I also verified IRQ_TYPE_EDGE_RISING on SM8550 platform, it works good.

Thanks,
Depeng

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Bryan O'Donoghue]

On 08/10/2024 17:24, Depeng Shao wrote:
> I have checked this, the trigger type of camera interrupt is _Edge_ what 
> is listed in Qualcomm ipcatalog website.

Nice thanks for confirming.

---
bod

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Krzysztof Kozlowski]

On 08/10/2024 13:50, Bryan O'Donoghue wrote:
> On 08/10/2024 12:37, Vladimir Zapolskiy wrote:
>>
>> I don't have access to datasheets or hardware of sc8280xp powered board,
>> someone may either verify, if CAMSS level high type interrupts are
>> supported/working at all or not (obviously its current presence in dts is
>> insufficient), or check the SoC datasheet.
> 
> I've tested both as was submitted and your change.
> 
> I _always_ test my patches. I'm not sure there's a datasheet which 
> spells this out to be honest.

Datasheet, HPG, interrupt list in the IP catalog. They all might provide
some hints, e.g. recommendation.

> 
> Rising or High can both be justified, its really down to how your 
> interrupt controller latches the state change. However I personally am 
> fine with the change you've provided because I trust it fixes an error 
> for you.

That's a GIC, right? So most of the GIC interrupts are level high.

I can easily imagine that 10 years ago one engineer made mistake and
wrote camss downstream DTS with edge and this kept going, because
"99.999% it works" and no one will ever hit that 0.001%. And if it is
hit, we blame something else because debugging is very difficult.

If this entire patchset is based on downstream driver code, not
datasheets, then it should be clearly explained in commit msg, not just
"The expected type is...".

Why? Because "the expected type" means datasheet or some hardware
engineer says it, not driver.

Best regards,
Krzysztof

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy]

On 10/8/24 15:01, Krzysztof Kozlowski wrote:
> On 08/10/2024 13:50, Bryan O'Donoghue wrote:
>> On 08/10/2024 12:37, Vladimir Zapolskiy wrote:
>>>
>>> I don't have access to datasheets or hardware of sc8280xp powered board,
>>> someone may either verify, if CAMSS level high type interrupts are
>>> supported/working at all or not (obviously its current presence in dts is
>>> insufficient), or check the SoC datasheet.
>>
>> I've tested both as was submitted and your change.
>>
>> I _always_ test my patches. I'm not sure there's a datasheet which
>> spells this out to be honest.
> 
> Datasheet, HPG, interrupt list in the IP catalog. They all might provide
> some hints, e.g. recommendation.
> 
>>
>> Rising or High can both be justified, its really down to how your
>> interrupt controller latches the state change. However I personally am
>> fine with the change you've provided because I trust it fixes an error
>> for you.
> 
> That's a GIC, right? So most of the GIC interrupts are level high.
> 
> I can easily imagine that 10 years ago one engineer made mistake and
> wrote camss downstream DTS with edge and this kept going, because
> "99.999% it works" and no one will ever hit that 0.001%. And if it is
> hit, we blame something else because debugging is very difficult.

Debugging of what? Again, nobody ever tested high level type of interrupts
of CAMSS IP. Why some irrelevant imaginary "races" are into the discussion,
have you or any other CAMSS user ever seen them? If no, this argument shall
be excluded.

Apparently nobody followerd the link in the cover letter to comprehend
the problem...

> If this entire patchset is based on downstream driver code, not
> datasheets, then it should be clearly explained in commit msg, not just
> "The expected type is...".
> 
> Why? Because "the expected type" means datasheet or some hardware
> engineer says it, not driver.
> 

The driver and only the driver dictates what's been tested so far in
this respect.

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Bryan O'Donoghue]

On 08/10/2024 13:01, Krzysztof Kozlowski wrote:
> On 08/10/2024 13:50, Bryan O'Donoghue wrote:
>> On 08/10/2024 12:37, Vladimir Zapolskiy wrote:
>>>
>>> I don't have access to datasheets or hardware of sc8280xp powered board,
>>> someone may either verify, if CAMSS level high type interrupts are
>>> supported/working at all or not (obviously its current presence in dts is
>>> insufficient), or check the SoC datasheet.
>>
>> I've tested both as was submitted and your change.
>>
>> I _always_ test my patches. I'm not sure there's a datasheet which
>> spells this out to be honest.
> 
> Datasheet, HPG, interrupt list in the IP catalog. They all might provide
> some hints, e.g. recommendation.
> 
>>
>> Rising or High can both be justified, its really down to how your
>> interrupt controller latches the state change. However I personally am
>> fine with the change you've provided because I trust it fixes an error
>> for you.
> 
> That's a GIC, right? So most of the GIC interrupts are level high.
> 
> I can easily imagine that 10 years ago one engineer made mistake and
> wrote camss downstream DTS with edge and this kept going, because
> "99.999% it works" and no one will ever hit that 0.001%. And if it is
> hit, we blame something else because debugging is very difficult.
> 
> If this entire patchset is based on downstream driver code, not
> datasheets, then it should be clearly explained in commit msg, not just
> "The expected type is...".
> 
> Why? Because "the expected type" means datasheet or some hardware
> engineer says it, not driver.
> 
> Best regards,
> Krzysztof
> 

Yes, true its entirely possible - likely even that copy/paste is the 
dejure method.

Lets have a poke around the qcom documentation and see if we can come up 
with a definitive answer rooted in the spec.

+1

---
bod

Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Krzysztof Kozlowski]

On 23/09/2024 09:28, Vladimir Zapolskiy wrote:
> The expected type of all CAMSS interrupts is edge rising, fix it in

Please re-phrase the commit msg to explain who expects it ("expected
type"). Further discussion lead to feeling that you based it on drivers,
which is obviously not enough.

> the documented example from CAMSS device tree bindings for sc8280xp


Best regards,
Krzysztof

[PATCH v2 2/6] dt-bindings: media: qcom,sdm845-camss: Fix interrupt types

[Vladimir Zapolskiy]

The expected type of all CAMSS interrupts is edge rising, fix it in
the documented example from CAMSS device tree bindings for sdm845.

Fixes: d1d5ce260165 ("media: dt-bindings: media: camss: Add qcom,sdm845-camss binding")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/media/qcom,sdm845-camss.yaml     | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
index ec4380a0a03f..d32daaef1b50 100644
--- a/Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
@@ -296,16 +296,16 @@ examples:
           "vfe_lite_cphy_rx",
           "vfe_lite_src";
 
-        interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
-          <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>;
+        interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+          <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>;
 
         interrupt-names = "csid0",
           "csid1",
-- 
2.45.2

[PATCH v2 3/6] dt-bindings: media: qcom,sm8250-camss: Fix interrupt types

[Vladimir Zapolskiy]

The expected type of all CAMSS interrupts is edge rising, fix it in
the documented example from CAMSS device tree bindings for SM8250.

Fixes: 46f8ac8497c5 ("media: dt-bindings: media: camss: Add qcom,sm8250-camss binding")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/media/qcom,sm8250-camss.yaml     | 28 +++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
index fa5073c0fd1e..06db2c1e6079 100644
--- a/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
@@ -329,20 +329,20 @@ examples:
             vdda-phy-supply = <&vreg_l5a_0p88>;
             vdda-pll-supply = <&vreg_l9a_1p2>;
 
-            interrupts = <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>;
+            interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 86 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 89 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>;
             interrupt-names = "csiphy0",
                               "csiphy1",
                               "csiphy2",
-- 
2.45.2

[PATCH v2 4/6] arm64: dts: qcom: sc8280xp: Fix interrupt type of camss interrupts

[Vladimir Zapolskiy]

The expected type of all CAMSS interrupts is edge rising, fix it in
the CAMSS device tree node for sc8280xp platform.

Fixes: 5994dd60753e ("arm64: dts: qcom: sc8280xp: camss: Add CAMSS block definition")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 40 +++++++++++++-------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 80a57aa22839..aa2678eb3bcd 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3882,26 +3882,26 @@ camss: camss@ac5a000 {
 				    "vfe3",
 				    "csid3";
 
-			interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 758 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 759 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 760 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 762 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 758 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 759 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 760 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 761 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 762 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 764 IRQ_TYPE_EDGE_RISING>;
 			interrupt-names = "csid1_lite",
 					  "vfe_lite1",
 					  "csiphy3",
-- 
2.45.2

Re: [PATCH v2 4/6] arm64: dts: qcom: sc8280xp: Fix interrupt type of camss interrupts

[Bjorn Andersson]

On Mon, Sep 23, 2024 at 10:28:25AM GMT, Vladimir Zapolskiy wrote:
> The expected type of all CAMSS interrupts is edge rising, fix it in
> the CAMSS device tree node for sc8280xp platform.
> 

Why did we get all these interrupts wrong? Why should they be RISING and
not LEVEL_HIGH?

Please document the reason why this is changed, so that the next person
adding a camss node can find your explanation and understand why it
should look this way.

Regards,
Bjorn

> Fixes: 5994dd60753e ("arm64: dts: qcom: sc8280xp: camss: Add CAMSS block definition")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 40 +++++++++++++-------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 80a57aa22839..aa2678eb3bcd 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3882,26 +3882,26 @@ camss: camss@ac5a000 {
>  				    "vfe3",
>  				    "csid3";
>  
> -			interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 758 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 759 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 760 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 762 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 764 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupts = <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 758 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 759 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 760 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 761 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 762 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 764 IRQ_TYPE_EDGE_RISING>;
>  			interrupt-names = "csid1_lite",
>  					  "vfe_lite1",
>  					  "csiphy3",
> -- 
> 2.45.2
> 

[PATCH v2 5/6] arm64: dts: qcom: sdm845: Fix interrupt types of camss interrupts

[Vladimir Zapolskiy]

The expected type of all CAMSS interrupts is edge rising, fix it in
the CAMSS device tree node for sdm845 platform.

Fixes: d48a6698a6b7 ("arm64: dts: qcom: sdm845: Add CAMSS ISP node")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 54077549b9da..0a0cef9dfcc4 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -4326,16 +4326,16 @@ camss: camss@acb3000 {
 				"vfe1",
 				"vfe_lite";
 
-			interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
-				<GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 469 IRQ_TYPE_EDGE_RISING>;
 			interrupt-names = "csid0",
 				"csid1",
 				"csid2",
-- 
2.45.2

[PATCH v2 6/6] arm64: dts: qcom: sm8250: Fix interrupt types of camss interrupts

[Vladimir Zapolskiy]

The expected type of all CAMSS interrupts is edge rising, fix it in
the CAMSS device tree node for sm8250 platform.

Fixes: 30325603b910 ("arm64: dts: qcom: sm8250: camss: Add CAMSS block definition")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 630f4eff20bf..f4b00cac5fcd 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -4481,20 +4481,20 @@ camss: camss@ac6a000 {
 				    "vfe_lite0",
 				    "vfe_lite1";
 
-			interrupts = <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 86 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 89 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>;
 			interrupt-names = "csiphy0",
 					  "csiphy1",
 					  "csiphy2",
-- 
2.45.2

Re: [PATCH v2 0/6] media: dt-bindings: media: camss: Fix interrupt types

[Bryan O'Donoghue]

On 23/09/2024 08:28, Vladimir Zapolskiy wrote:
> It was discovered that on a few Qualcomm platforms types of interrupts
> do not match both downstream code and a type requested by the CAMSS driver.
> 
> The mismatched interrupt type between firmware and the correspondent CAMSS
> driver leads to known problems, which were discussed previously:
> 
>    https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
> 
> Here the situation is right the same, namely a repeated bind of camss device
> is not possible due to a wrongly specified interrupt type, and it may lead
> to an issue in runtime manifested like this:
> 
>    irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
> 
> Changes from v1 to v2:
> * added gained Acked-by, Tested-by and Reviewed-by tags,
> * per patch review requests from Krzysztof deduplicated "media:" from subjects.
> 
> Link to v1 of the changeset:
> 
>    https://lore.kernel.org/all/20240905164142.3475873-1-vladimir.zapolskiy@linaro.org/
> 
> Vladimir Zapolskiy (6):
>    dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types
>    dt-bindings: media: qcom,sdm845-camss: Fix interrupt types
>    dt-bindings: media: qcom,sm8250-camss: Fix interrupt types
>    arm64: dts: qcom: sc8280xp: Fix interrupt type of camss interrupts
>    arm64: dts: qcom: sdm845: Fix interrupt types of camss interrupts
>    arm64: dts: qcom: sm8250: Fix interrupt types of camss interrupts
> 
>   .../bindings/media/qcom,sc8280xp-camss.yaml   | 40 +++++++++----------
>   .../bindings/media/qcom,sdm845-camss.yaml     | 20 +++++-----
>   .../bindings/media/qcom,sm8250-camss.yaml     | 28 ++++++-------
>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi        | 40 +++++++++----------
>   arch/arm64/boot/dts/qcom/sdm845.dtsi          | 20 +++++-----
>   arch/arm64/boot/dts/qcom/sm8250.dtsi          | 28 ++++++-------
>   6 files changed, 88 insertions(+), 88 deletions(-)
> 

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

For the series.