| Message ID | 20250217130013.2802293-2-y-abhilashchandra@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable support for error detection in CSI2RX | expand |
Hi Abhilash, On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote: > The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. > Enabling these interrupts will provide additional information about a CSI > packet or an individual frame. So, add support for optional interrupts > and interrupt-names properties. > > [0]: http://www.ti.com/lit/pdf/spruil1 > > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> > --- > > Changes in v2: > - Address Krzysztof's review comment to remove flexibility while adding > interrupts. > > .../devicetree/bindings/media/cdns,csi2rx.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > index 2008a47c0580..f335429cbde9 100644 > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > @@ -24,6 +24,16 @@ properties: > reg: > maxItems: 1 > > + interrupts: > + minItems: 3 > + maxItems: 3 > + > + interrupt-names: > + items: > + - const: info > + - const: error > + - const: monitor > + How many interrupt lines are actually exposed by the Cadence IP? It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF Hardware Requests" it looks like there are three separate interrupt lines CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC. And four lines for the ASF submodule (?) that are not routed to GIC. This does not match with the error, info and monitor line names mentioned here. In my understanding which interrupt lines are actually integrated will vary from SoC to SoC, so please check what are the actual interrupt line names exposed by the Cadence IP. Maybe Maxime knows more. But I don't think it is correct to make all 3 mandatory together, as some vendors may only integrate the error interrupt ignoring the rest. CC: Changhuang Liang <changhuang.liang@starfivetech.com> > clocks: > items: > - description: CSI2Rx system clock > -- > 2.34.1 > Thanks, Jai
On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote: > The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. > Enabling these interrupts will provide additional information about a CSI > packet or an individual frame. So, add support for optional interrupts > and interrupt-names properties. > > [0]: http://www.ti.com/lit/pdf/spruil1 > > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> > --- > > Changes in v2: > - Address Krzysztof's review comment to remove flexibility while adding > interrupts. > > .../devicetree/bindings/media/cdns,csi2rx.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > index 2008a47c0580..f335429cbde9 100644 > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > @@ -24,6 +24,16 @@ properties: > reg: > maxItems: 1 > > + interrupts: > + minItems: 3 Drop, see other bindings. Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi Jai, Abhilash > Hi Abhilash, > > On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote: > > The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. > > Enabling these interrupts will provide additional information about a > > CSI packet or an individual frame. So, add support for optional > > interrupts and interrupt-names properties. > > > > [0]: http://www.ti.com/lit/pdf/spruil1 > > > > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> > > --- > > > > Changes in v2: > > - Address Krzysztof's review comment to remove flexibility while adding > > interrupts. > > > > .../devicetree/bindings/media/cdns,csi2rx.yaml | 10 > ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > index 2008a47c0580..f335429cbde9 100644 > > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > @@ -24,6 +24,16 @@ properties: > > reg: > > maxItems: 1 > > > > + interrupts: > > + minItems: 3 > > + maxItems: 3 > > + > > + interrupt-names: > > + items: > > + - const: info > > + - const: error > > + - const: monitor > > + > > How many interrupt lines are actually exposed by the Cadence IP? I only see that the Cadence IP exposes two interrupt lines: irq and err_irq. If there are any mistakes, please help correct them. > It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF > Hardware Requests" it looks like there are three separate interrupt lines > CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC. > And four lines for the ASF submodule (?) that are not routed to GIC. > > This does not match with the error, info and monitor line names mentioned > here. > > In my understanding which interrupt lines are actually integrated will vary > from SoC to SoC, so please check what are the actual interrupt line names > exposed by the Cadence IP. Maybe Maxime knows more. > > But I don't think it is correct to make all 3 mandatory together, as some > vendors may only integrate the error interrupt ignoring the rest. Agreed. Best Regards, Changhuang
On Tue, Feb 18, 2025 at 09:35:50AM +0000, Changhuang Liang wrote: > Hi Jai, Abhilash > > > Hi Abhilash, > > > > On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote: > > > The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. > > > Enabling these interrupts will provide additional information about a > > > CSI packet or an individual frame. So, add support for optional > > > interrupts and interrupt-names properties. > > > > > > [0]: http://www.ti.com/lit/pdf/spruil1 > > > > > > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> > > > --- > > > > > > Changes in v2: > > > - Address Krzysztof's review comment to remove flexibility while adding > > > interrupts. > > > > > > .../devicetree/bindings/media/cdns,csi2rx.yaml | 10 > > ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > > b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > > index 2008a47c0580..f335429cbde9 100644 > > > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > > > @@ -24,6 +24,16 @@ properties: > > > reg: > > > maxItems: 1 > > > > > > + interrupts: > > > + minItems: 3 > > > + maxItems: 3 > > > + > > > + interrupt-names: > > > + items: > > > + - const: info > > > + - const: error > > > + - const: monitor > > > + > > > > How many interrupt lines are actually exposed by the Cadence IP? > > I only see that the Cadence IP exposes two interrupt lines: irq and err_irq. If there are any mistakes, > please help correct them. > Thank you Changhuang for the quick confirmation. This seems to match CSI_ERR_IRQ and CSI_IRQ in TI's integration. > > It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF > > Hardware Requests" it looks like there are three separate interrupt lines > > CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC. > > And four lines for the ASF submodule (?) that are not routed to GIC. > > Abhilash, What is unclear to me is where the third CSI_LEVEL interrupt line is coming from? The TRM description of the CSI_LEVEL interrupt says it is for PSI_L overflow or VP0/VP1 frame/line mismatch, both of which are outside the Cadence CSI2RX, instead belonging to the TI wrapper hardware, which has its own driver. > > This does not match with the error, info and monitor line names mentioned > > here. > > > > In my understanding which interrupt lines are actually integrated will vary > > from SoC to SoC, so please check what are the actual interrupt line names > > exposed by the Cadence IP. Maybe Maxime knows more. > > > > But I don't think it is correct to make all 3 mandatory together, as some > > vendors may only integrate the error interrupt ignoring the rest. > > Agreed. > I think by default there should only be two optional interrupt lines for cdns-csi2rx, "irq" and "err_irq" which is common across vendors. If this third TI-specific CSI_LEVEL interrupt *has* to be handled by cdns-csi2rx driver, it would be better to create conditional bindings and match data in the driver such that the irq is only requested if "ti,j721e-csi2rx" compatible is present. > Best Regards, > Changhuang Thanks, Jai
Hi Jai,Changhuang, Thanks for the reviews. On 18/02/25 21:07, Jai Luthra wrote: > On Tue, Feb 18, 2025 at 09:35:50AM +0000, Changhuang Liang wrote: >> Hi Jai, Abhilash >> >>> Hi Abhilash, >>> >>> On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote: >>>> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. >>>> Enabling these interrupts will provide additional information about a >>>> CSI packet or an individual frame. So, add support for optional >>>> interrupts and interrupt-names properties. >>>> >>>> [0]: http://www.ti.com/lit/pdf/spruil1 >>>> >>>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> >>>> --- >>>> >>>> Changes in v2: >>>> - Address Krzysztof's review comment to remove flexibility while adding >>>> interrupts. >>>> >>>> .../devicetree/bindings/media/cdns,csi2rx.yaml | 10 >>> ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml >>>> b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml >>>> index 2008a47c0580..f335429cbde9 100644 >>>> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml >>>> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml >>>> @@ -24,6 +24,16 @@ properties: >>>> reg: >>>> maxItems: 1 >>>> >>>> + interrupts: >>>> + minItems: 3 >>>> + maxItems: 3 >>>> + >>>> + interrupt-names: >>>> + items: >>>> + - const: info >>>> + - const: error >>>> + - const: monitor >>>> + >>> >>> How many interrupt lines are actually exposed by the Cadence IP? >> >> I only see that the Cadence IP exposes two interrupt lines: irq and err_irq. If there are any mistakes, >> please help correct them. >> > > Thank you Changhuang for the quick confirmation. > This seems to match CSI_ERR_IRQ and CSI_IRQ in TI's integration. > >>> It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF >>> Hardware Requests" it looks like there are three separate interrupt lines >>> CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC. >>> And four lines for the ASF submodule (?) that are not routed to GIC. >>> > > Abhilash, > > What is unclear to me is where the third CSI_LEVEL interrupt line is coming > from? > > The TRM description of the CSI_LEVEL interrupt says it is for PSI_L overflow > or VP0/VP1 frame/line mismatch, both of which are outside the Cadence CSI2RX, > instead belonging to the TI wrapper hardware, which has its own driver. Yes, we will update the device tree bindings accordingly. We are planning to handle the TI-specific interrupt line separately in another series. > >>> This does not match with the error, info and monitor line names mentioned >>> here. >>> >>> In my understanding which interrupt lines are actually integrated will vary >>> from SoC to SoC, so please check what are the actual interrupt line names >>> exposed by the Cadence IP. Maybe Maxime knows more. >>> >>> But I don't think it is correct to make all 3 mandatory together, as some >>> vendors may only integrate the error interrupt ignoring the rest. >> >> Agreed. >> > > I think by default there should only be two optional interrupt lines for > cdns-csi2rx, "irq" and "err_irq" which is common across vendors. > > If this third TI-specific CSI_LEVEL interrupt *has* to be handled by > cdns-csi2rx driver, it would be better to create conditional bindings and > match data in the driver such that the irq is only requested if > "ti,j721e-csi2rx" compatible is present. I will correct this in v3 by introducing bindings that allow vendors the flexibility to use either two or three interrupt lines. Thanks and Regards, Yemike Abhilash Chandra > >> Best Regards, >> Changhuang > > Thanks, > Jai
diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml index 2008a47c0580..f335429cbde9 100644 --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml @@ -24,6 +24,16 @@ properties: reg: maxItems: 1 + interrupts: + minItems: 3 + maxItems: 3 + + interrupt-names: + items: + - const: info + - const: error + - const: monitor + clocks: items: - description: CSI2Rx system clock
The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem. Enabling these interrupts will provide additional information about a CSI packet or an individual frame. So, add support for optional interrupts and interrupt-names properties. [0]: http://www.ti.com/lit/pdf/spruil1 Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> --- Changes in v2: - Address Krzysztof's review comment to remove flexibility while adding interrupts. .../devicetree/bindings/media/cdns,csi2rx.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)