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@xxxxxxxxxx/
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
