| Message ID | 20240923072827.3772504-2-vladimir.zapolskiy@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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",
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(-)