| Message ID | 20240923072827.3772504-1-vladimir.zapolskiy@linaro.org |
|---|---|
| Headers | show |
| Series | media: dt-bindings: media: camss: Fix interrupt types | expand |
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 >
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