| Message ID | 20250315152708.328036-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rcar-isp: Prepare for ISP core support | expand |
On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > diff --git a/Documentation/devicetree/bindings/media/renesas,isp.yaml b/Documentation/devicetree/bindings/media/renesas,isp.yaml > index c4de4555b753..de9bc739e084 100644 > --- a/Documentation/devicetree/bindings/media/renesas,isp.yaml > +++ b/Documentation/devicetree/bindings/media/renesas,isp.yaml > @@ -25,19 +25,54 @@ properties: > - renesas,r8a779h0-isp # V4M > - const: renesas,rcar-gen4-isp # Generic R-Car Gen4 > reg: > - maxItems: 1 > + minItems: 1 > + maxItems: 2 > + > + reg-names: > + minItems: 1 > + items: > + - const: cs > + - const: core All of this and further must be restricted per compatible. Otherwise commit msg should explain why one SoC can have it different on different boards. > > interrupts: > - maxItems: 1 > + minItems: 1 > + maxItems: 2 > + > + interrupt-names: > + minItems: 1 > + items: > + - const: cs > + - const: core > > clocks: > - maxItems: 1 > + minItems: 1 > + maxItems: 2 > + > + clock-names: > + minItems: 1 > + items: > + - const: cs > + - const: core > > power-domains: > maxItems: 1 > > resets: > - maxItems: 1 > + minItems: 1 > + maxItems: 2 > + > + reset-names: > + minItems: 1 > + items: > + - const: cs > + - const: core > + > + renesas,vspx: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + A phandle to the companion VSPX responsible for the Streaming Bridge But what does this device needs it for? > + functionality. This property is not mandatory and not all ISP devices > + have one attached. Drop last sentence, redundant. Instead disallow it (renesas,vspx: false) for all the variants not having VSPX. > > ports: > $ref: /schemas/graph.yaml#/properties/ports > @@ -103,10 +138,14 @@ properties: > required: > - compatible > - reg > + - reg-names > - interrupts > + - interrupt-names > - clocks > + - clock-names > - power-domains > - resets > + - reset-names > - ports > > additionalProperties: false > @@ -119,11 +158,16 @@ examples: > > isp1: isp@fed20000 { > compatible = "renesas,r8a779a0-isp", "renesas,rcar-gen4-isp"; > - reg = <0xfed20000 0x10000>; > - interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0xfed20000 0x10000>, <0xfee00000 0x10000>; > + reg-names = "cs", "core"; > + interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "cs", "core"; > clocks = <&cpg CPG_MOD 613>; > + clock-names = "cs"; Why no core? The names feel inconsistent. If your block has "core" reg for the "ISP core" sublock, why there is no "ISP core" clock for that subblock? Best regards, Krzysztof
On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > ports: > $ref: /schemas/graph.yaml#/properties/ports > @@ -103,10 +138,14 @@ properties: > required: > - compatible > - reg > + - reg-names > - interrupts > + - interrupt-names > - clocks > + - clock-names > - power-domains > - resets > + - reset-names Another point, this will spawn bunch of warnings for no real reason. Just drop all the xxx-names from properties and from here. Best regards, Krzysztof
Hi Krzysztof, Thanks for your feedback. On 2025-03-17 12:31:51 +0100, Krzysztof Kozlowski wrote: > On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > > diff --git a/Documentation/devicetree/bindings/media/renesas,isp.yaml b/Documentation/devicetree/bindings/media/renesas,isp.yaml > > index c4de4555b753..de9bc739e084 100644 > > --- a/Documentation/devicetree/bindings/media/renesas,isp.yaml > > +++ b/Documentation/devicetree/bindings/media/renesas,isp.yaml > > @@ -25,19 +25,54 @@ properties: > > - renesas,r8a779h0-isp # V4M > > - const: renesas,rcar-gen4-isp # Generic R-Car Gen4 > > reg: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 2 > > + > > + reg-names: > > + minItems: 1 > > + items: > > + - const: cs > > + - const: core > > All of this and further must be restricted per compatible. Otherwise > commit msg should explain why one SoC can have it different on different > boards. I will expand the commit message. In short this can't be restricted per compatible, different instances of the IP on the same board can and can not have a core part. > > > > > interrupts: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 2 > > + > > + interrupt-names: > > + minItems: 1 > > + items: > > + - const: cs > > + - const: core > > > > clocks: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 2 > > + > > + clock-names: > > + minItems: 1 > > + items: > > + - const: cs > > + - const: core > > > > power-domains: > > maxItems: 1 > > > > resets: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 2 > > + > > + reset-names: > > + minItems: 1 > > + items: > > + - const: cs > > + - const: core > > + > > + renesas,vspx: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + A phandle to the companion VSPX responsible for the Streaming Bridge > > But what does this device needs it for? It's the external IP that controls the DMA of data to the ISP. I will expand this description. > > > + functionality. This property is not mandatory and not all ISP devices > > + have one attached. > > Drop last sentence, redundant. Instead disallow it (renesas,vspx: false) > for all the variants not having VSPX. I can't do that as all variants can possibly have one attached to it. All instances of the ISP that have a core part have a VSPX. And on the same SoC different instances of the IP can have and can not have a core part. > > > > > ports: > > $ref: /schemas/graph.yaml#/properties/ports > > @@ -103,10 +138,14 @@ properties: > > required: > > - compatible > > - reg > > + - reg-names > > - interrupts > > + - interrupt-names > > - clocks > > + - clock-names > > - power-domains > > - resets > > + - reset-names > > - ports > > > > additionalProperties: false > > @@ -119,11 +158,16 @@ examples: > > > > isp1: isp@fed20000 { > > compatible = "renesas,r8a779a0-isp", "renesas,rcar-gen4-isp"; > > - reg = <0xfed20000 0x10000>; > > - interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>; > > + reg = <0xfed20000 0x10000>, <0xfee00000 0x10000>; > > + reg-names = "cs", "core"; > > + interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "cs", "core"; > > clocks = <&cpg CPG_MOD 613>; > > + clock-names = "cs"; > > Why no core? The names feel inconsistent. If your block has "core" reg > for the "ISP core" sublock, why there is no "ISP core" clock for that > subblock? Indeed this is wrong, there should be a core clock added here too, thanks for catching this. > > Best regards, > Krzysztof >
On 2025-03-17 12:33:07 +0100, Krzysztof Kozlowski wrote: > On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > > ports: > > $ref: /schemas/graph.yaml#/properties/ports > > @@ -103,10 +138,14 @@ properties: > > required: > > - compatible > > - reg > > + - reg-names > > - interrupts > > + - interrupt-names > > - clocks > > + - clock-names > > - power-domains > > - resets > > + - reset-names > > Another point, this will spawn bunch of warnings for no real reason. > Just drop all the xxx-names from properties and from here. I'm sorry maybe I'm missing something, but if I drop them from properties how can I add checks to makesure the names are either "cs" or "core"? > > Best regards, > Krzysztof >
On 17/03/2025 12:50, Niklas Söderlund wrote: > On 2025-03-17 12:33:07 +0100, Krzysztof Kozlowski wrote: >> On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: >>> ports: >>> $ref: /schemas/graph.yaml#/properties/ports >>> @@ -103,10 +138,14 @@ properties: >>> required: >>> - compatible >>> - reg >>> + - reg-names >>> - interrupts >>> + - interrupt-names >>> - clocks >>> + - clock-names >>> - power-domains >>> - resets >>> + - reset-names >> >> Another point, this will spawn bunch of warnings for no real reason. >> Just drop all the xxx-names from properties and from here. > > I'm sorry maybe I'm missing something, but if I drop them from > properties how can I add checks to makesure the names are either "cs" or Why do you need to check for the names? There will be no names, so nothing to check for. > "core"? Best regards, Krzysztof
On 17/03/2025 12:49, Niklas Söderlund wrote: > Hi Krzysztof, > > Thanks for your feedback. > > On 2025-03-17 12:31:51 +0100, Krzysztof Kozlowski wrote: >> On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: >>> diff --git a/Documentation/devicetree/bindings/media/renesas,isp.yaml b/Documentation/devicetree/bindings/media/renesas,isp.yaml >>> index c4de4555b753..de9bc739e084 100644 >>> --- a/Documentation/devicetree/bindings/media/renesas,isp.yaml >>> +++ b/Documentation/devicetree/bindings/media/renesas,isp.yaml >>> @@ -25,19 +25,54 @@ properties: >>> - renesas,r8a779h0-isp # V4M >>> - const: renesas,rcar-gen4-isp # Generic R-Car Gen4 >>> reg: >>> - maxItems: 1 >>> + minItems: 1 >>> + maxItems: 2 >>> + >>> + reg-names: >>> + minItems: 1 >>> + items: >>> + - const: cs >>> + - const: core >> >> All of this and further must be restricted per compatible. Otherwise >> commit msg should explain why one SoC can have it different on different >> boards. > > I will expand the commit message. In short this can't be restricted per > compatible, different instances of the IP on the same board can and can > not have a core part. s/Same board/same SoC/? Or you really meant that same SoC on different boards will have or have not that ISP core? Both are odd, first even weirder. I wonder if some other difference is not the documented. E.g. same IP blocks are not exactly the same, but have different programming model. What is this ISP core responsible for inside Renesas ISP? How many ISPs are inside of SoC? And how would it work? You have two exactly the same IP blocks in the SoC, but one you program differently than other. How do you know which one? Best regards, Krzysztof
On 2025-03-17 16:02:34 +0100, Krzysztof Kozlowski wrote: > On 17/03/2025 12:49, Niklas Söderlund wrote: > > Hi Krzysztof, > > > > Thanks for your feedback. > > > > On 2025-03-17 12:31:51 +0100, Krzysztof Kozlowski wrote: > >> On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > >>> diff --git a/Documentation/devicetree/bindings/media/renesas,isp.yaml b/Documentation/devicetree/bindings/media/renesas,isp.yaml > >>> index c4de4555b753..de9bc739e084 100644 > >>> --- a/Documentation/devicetree/bindings/media/renesas,isp.yaml > >>> +++ b/Documentation/devicetree/bindings/media/renesas,isp.yaml > >>> @@ -25,19 +25,54 @@ properties: > >>> - renesas,r8a779h0-isp # V4M > >>> - const: renesas,rcar-gen4-isp # Generic R-Car Gen4 > >>> reg: > >>> - maxItems: 1 > >>> + minItems: 1 > >>> + maxItems: 2 > >>> + > >>> + reg-names: > >>> + minItems: 1 > >>> + items: > >>> + - const: cs > >>> + - const: core > >> > >> All of this and further must be restricted per compatible. Otherwise > >> commit msg should explain why one SoC can have it different on different > >> boards. > > > > I will expand the commit message. In short this can't be restricted per > > compatible, different instances of the IP on the same board can and can > > not have a core part. > > s/Same board/same SoC/? Or you really meant that same SoC on different > boards will have or have not that ISP core? > > Both are odd, first even weirder. > > I wonder if some other difference is not the documented. E.g. same IP > blocks are not exactly the same, but have different programming model. The IP block named "ISP" is in fact two different things on R-Car Gen4 SoCs. I know it's confusing and not logical but that's how they are made. - One part is the ISP Channel Selector, this is a function that sits on the CIS-2 receiver data bus. It is responsible for selecting which CSI-2 Virtual Channel is routed to which DMA capture engine. This part is what the rcar-isp.ko driver have always supported and every instance of the ISP that is described in tree deals with just this one function as this is the one we always had documentation for. This block is the one the reg-names and clock-names labels "cs". - One part that we now wish to add is the ISP Core. This is a traditional ISP that act on image data in different ways. This is what I try to model with the reg-name and clock-name labeled "core". This is new and we have not had documentation for this until recently. Unfortunately the "core" and "cs" functions is implemented in the same IP block. And to operate the "core" one needs to also deal with "cs". To make it more interesting all ISP Channel Selectors (cs) do not have a companion ISP Core, but most do. The lack of a ISP core is OK, it just means that video capture path can't manipulate the image as much as paths that do. It's not ideal but to support the ISP Core and ISP Channel Selecotr the rcar-isp.ko module needs both "core" and "cs" clocks and regs. And to support just the Channel Selector it only needs the "cs" resources. Sorry if I have been confusing. A good example of this is patch 4/7 in this series. It shows the V4M board that have 2 ISP instances, one that have both cs and core functions, and one that only have cs function. > > What is this ISP core responsible for inside Renesas ISP? How many ISPs > are inside of SoC? The ISP core is responsible for image manipulation. ISP Channel Selector for CSI-2 channel routing. The number of ISP varies between SoCs: V3U: Have 4 ISP instances, all 4 have cs and core. V4H: Have 2 ISP instances, both have cs and core. V4M: Have 2 ISP instances, one have cs and core, one have only cs. > > And how would it work? You have two exactly the same IP blocks in the > SoC, but one you program differently than other. How do you know which one? All cs blocks are the same on all SoCs. The core block is the same on V4H and V4M, and different on V3U. > > Best regards, > Krzysztof
On 2025-03-17 15:57:31 +0100, Krzysztof Kozlowski wrote: > On 17/03/2025 12:50, Niklas Söderlund wrote: > > On 2025-03-17 12:33:07 +0100, Krzysztof Kozlowski wrote: > >> On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > >>> ports: > >>> $ref: /schemas/graph.yaml#/properties/ports > >>> @@ -103,10 +138,14 @@ properties: > >>> required: > >>> - compatible > >>> - reg > >>> + - reg-names > >>> - interrupts > >>> + - interrupt-names > >>> - clocks > >>> + - clock-names > >>> - power-domains > >>> - resets > >>> + - reset-names > >> > >> Another point, this will spawn bunch of warnings for no real reason. > >> Just drop all the xxx-names from properties and from here. > > > > I'm sorry maybe I'm missing something, but if I drop them from > > properties how can I add checks to makesure the names are either "cs" or > > Why do you need to check for the names? There will be no names, so > nothing to check for. Ahh I see. But I would like to have names if possible. The driver is backward compatible with the old bindings, and going forward we have better bindings with names. All users are updated in the next commits in this series so the warnings will go way rather quickly. > > "core"? > > Best regards, > Krzysztof
Hi Niklas, On Mon, 17 Mar 2025 at 16:37, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > On 2025-03-17 15:57:31 +0100, Krzysztof Kozlowski wrote: > > On 17/03/2025 12:50, Niklas Söderlund wrote: > > > On 2025-03-17 12:33:07 +0100, Krzysztof Kozlowski wrote: > > >> On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > > >>> ports: > > >>> $ref: /schemas/graph.yaml#/properties/ports > > >>> @@ -103,10 +138,14 @@ properties: > > >>> required: > > >>> - compatible > > >>> - reg > > >>> + - reg-names > > >>> - interrupts > > >>> + - interrupt-names > > >>> - clocks > > >>> + - clock-names > > >>> - power-domains > > >>> - resets > > >>> + - reset-names > > >> > > >> Another point, this will spawn bunch of warnings for no real reason. > > >> Just drop all the xxx-names from properties and from here. > > > > > > I'm sorry maybe I'm missing something, but if I drop them from > > > properties how can I add checks to makesure the names are either "cs" or > > > > Why do you need to check for the names? There will be no names, so > > nothing to check for. > > Ahh I see. But I would like to have names if possible. > > The driver is backward compatible with the old bindings, and going > forward we have better bindings with names. All users are updated in the > next commits in this series so the warnings will go way rather quickly. Note that the driver does not _have_ to obtain the "cs" clock by name, as it will always be the first clock anyway ("make dtbs_check" will sort-of enforce that). So you can simplify the code by obtaining the first clock without specifying a name, and the second (optional) clock with a name. Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for your feedback. On 2025-03-17 20:21:14 +0100, Geert Uytterhoeven wrote: > Hi Niklas, > > On Mon, 17 Mar 2025 at 16:37, Niklas Söderlund > <niklas.soderlund+renesas@ragnatech.se> wrote: > > On 2025-03-17 15:57:31 +0100, Krzysztof Kozlowski wrote: > > > On 17/03/2025 12:50, Niklas Söderlund wrote: > > > > On 2025-03-17 12:33:07 +0100, Krzysztof Kozlowski wrote: > > > >> On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote: > > > >>> ports: > > > >>> $ref: /schemas/graph.yaml#/properties/ports > > > >>> @@ -103,10 +138,14 @@ properties: > > > >>> required: > > > >>> - compatible > > > >>> - reg > > > >>> + - reg-names > > > >>> - interrupts > > > >>> + - interrupt-names > > > >>> - clocks > > > >>> + - clock-names > > > >>> - power-domains > > > >>> - resets > > > >>> + - reset-names > > > >> > > > >> Another point, this will spawn bunch of warnings for no real reason. > > > >> Just drop all the xxx-names from properties and from here. > > > > > > > > I'm sorry maybe I'm missing something, but if I drop them from > > > > properties how can I add checks to makesure the names are either "cs" or > > > > > > Why do you need to check for the names? There will be no names, so > > > nothing to check for. > > > > Ahh I see. But I would like to have names if possible. > > > > The driver is backward compatible with the old bindings, and going > > forward we have better bindings with names. All users are updated in the > > next commits in this series so the warnings will go way rather quickly. > > Note that the driver does not _have_ to obtain the "cs" clock by name, > as it will always be the first clock anyway ("make dtbs_check" will > sort-of enforce that). So you can simplify the code by obtaining > the first clock without specifying a name, and the second (optional) > clock with a name. I understand that, and for this fix this would be acceptable. I'm just trying to think a head, something I should have done when first writing these bindings... I'm fearing a scenario where we will need to add a 3rd reg region or clock. I don't think we will need that for the compatible values we have here, but then I never though we get the documentation that now allows us to describe the second region... If you and Krzysztof are happy without names I can move forward with that too, I'm just trying to learn from my mistakes ;-) I will give it a few days and then head down this road without names.
diff --git a/Documentation/devicetree/bindings/media/renesas,isp.yaml b/Documentation/devicetree/bindings/media/renesas,isp.yaml index c4de4555b753..de9bc739e084 100644 --- a/Documentation/devicetree/bindings/media/renesas,isp.yaml +++ b/Documentation/devicetree/bindings/media/renesas,isp.yaml @@ -25,19 +25,54 @@ properties: - renesas,r8a779h0-isp # V4M - const: renesas,rcar-gen4-isp # Generic R-Car Gen4 reg: - maxItems: 1 + minItems: 1 + maxItems: 2 + + reg-names: + minItems: 1 + items: + - const: cs + - const: core interrupts: - maxItems: 1 + minItems: 1 + maxItems: 2 + + interrupt-names: + minItems: 1 + items: + - const: cs + - const: core clocks: - maxItems: 1 + minItems: 1 + maxItems: 2 + + clock-names: + minItems: 1 + items: + - const: cs + - const: core power-domains: maxItems: 1 resets: - maxItems: 1 + minItems: 1 + maxItems: 2 + + reset-names: + minItems: 1 + items: + - const: cs + - const: core + + renesas,vspx: + $ref: /schemas/types.yaml#/definitions/phandle + description: + A phandle to the companion VSPX responsible for the Streaming Bridge + functionality. This property is not mandatory and not all ISP devices + have one attached. ports: $ref: /schemas/graph.yaml#/properties/ports @@ -103,10 +138,14 @@ properties: required: - compatible - reg + - reg-names - interrupts + - interrupt-names - clocks + - clock-names - power-domains - resets + - reset-names - ports additionalProperties: false @@ -119,11 +158,16 @@ examples: isp1: isp@fed20000 { compatible = "renesas,r8a779a0-isp", "renesas,rcar-gen4-isp"; - reg = <0xfed20000 0x10000>; - interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>; + reg = <0xfed20000 0x10000>, <0xfee00000 0x10000>; + reg-names = "cs", "core"; + interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "cs", "core"; clocks = <&cpg CPG_MOD 613>; + clock-names = "cs"; power-domains = <&sysc R8A779A0_PD_A3ISP01>; resets = <&cpg 613>; + reset-names = "cs"; ports { #address-cells = <1>;
Some R-Car ISP instances have in addition to the channel selector (CS) an ISP core to perform operations on an image stream. The core function is mapped to a different memory region and have a separate interrupt that CS, extend the bindings to allow describing this. In order for the ISP core to function in memory-to-memory mode it needs to be feed input data from a Streaming Bridge interface. This interface is provided thru the VSP-X device. Add an optional new property "renesas,vspx" to provide a phandle to describe this relationship. While adding mandatory reg-names and interrupt-names breaks existing bindings the driver itself remains backward compatible and provides CS functionality if a single unnamed reg and interrupt property is present. Furthermore all existing users of the bindings are updated in following work to add these new mandatory properties. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- .../bindings/media/renesas,isp.yaml | 56 +++++++++++++++++-- 1 file changed, 50 insertions(+), 6 deletions(-)