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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

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