| Message ID | 20250315152708.328036-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| 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.
On 17/03/2025 16:34, Niklas Söderlund wrote: > 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. Based on this I think the instances with ISP core are not the same hardware as instances without. You have there different (new) programming model for entirely new part of hardware not present in "old" instances. Different device means different compatible. And judging by the address: reg = <0 0xfed00000 0 0x10000>, <0 0xfec00000 0 0x100000>; 1. 0xfec0 < 0xfed0 2. Huge address range that's not "renesas,r8a779h0-isp" at all, but your old "ISP" device is actually a part of that 0xfec0_0000. Probably the channel selector should have never been called "ISP" because it does not process images. :/ > >> >> 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. Best regards, Krzysztof
On 17/03/2025 20:44, Niklas Söderlund wrote: >>> 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 Bindings should be complete, so add 3rd clock now. If you need to add it later, what's the problem? The position or order is strictly fixed, so at 3rd place you will always have new foo-clock. > here, but then I never though we get the documentation that now allows > us to describe the second region... Best regards, Krzysztof
Hi Niklas, On Mon, 17 Mar 2025 at 20:44, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > On 2025-03-17 20:21:14 +0100, Geert Uytterhoeven wrote: > > 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. I would still specify the names in the bindings, so full ISPs have all names. Gr{oetje,eeting}s, Geert
On 2025-03-18 08:29:17 +0100, Krzysztof Kozlowski wrote: > On 17/03/2025 20:44, Niklas Söderlund wrote: > >>> 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 > > Bindings should be complete, so add 3rd clock now. > > If you need to add it later, what's the problem? The position or order > is strictly fixed, so at 3rd place you will always have new foo-clock. I agree, bindings should be complete. But it's hard to create complete bindings from incomplete documentation. There is no 3rd clock or memory region that can be added now, at lest not one in the documentation I have access too. I was only trying to make the point that I do want to add *-names properties now and not only depend on argument position. Sorry if I have misunderstood you. > > > here, but then I never though we get the documentation that now allows > > us to describe the second region... > Best regards, > Krzysztof
Hi Krzysztof, On Tue, 18 Mar 2025 at 08:27, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 17/03/2025 16:34, Niklas Söderlund wrote: > > 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. > > Based on this I think the instances with ISP core are not the same > hardware as instances without. You have there different (new) > programming model for entirely new part of hardware not present in "old" > instances. > > Different device means different compatible. I think the intention has always been to represent the "full" ISP, but we started with limited bindings, due to the lack of documentation. Note that at the time the bindings were written, all SoCs we were aware of only had the "full" ISP. > And judging by the address: > reg = <0 0xfed00000 0 0x10000>, <0 0xfec00000 0 0x100000>; > 1. 0xfec0 < 0xfed0 Relative addresses don't mean anything. > 2. Huge address range > > that's not "renesas,r8a779h0-isp" at all, but your old "ISP" device is > actually a part of that 0xfec0_0000. > > Probably the channel selector should have never been called "ISP" > because it does not process images. :/ The documentation has just a single chapter for the combined Image Signal Processor with Channel Selector, and considers it a single block. From my point of view, the ISP processing core is just an optional feature, which is not that dissimilar from e.g. the optional stream buffer in Documentation/devicetree/bindings/net/renesas,etheravb.yaml. > >> 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. Gr{oetje,eeting}s, Geert
On 2025-03-18 08:27:36 +0100, Krzysztof Kozlowski wrote: > On 17/03/2025 16:34, Niklas Söderlund wrote: > > 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. > > Based on this I think the instances with ISP core are not the same > hardware as instances without. You have there different (new) > programming model for entirely new part of hardware not present in "old" > instances. > > Different device means different compatible. > > And judging by the address: > reg = <0 0xfed00000 0 0x10000>, <0 0xfec00000 0 0x100000>; > 1. 0xfec0 < 0xfed0 > 2. Huge address range > > that's not "renesas,r8a779h0-isp" at all, but your old "ISP" device is > actually a part of that 0xfec0_0000. > > Probably the channel selector should have never been called "ISP" > because it does not process images. :/ There are always room for improvement, but we can only try and create bindings that describe the hardware as it is documented. In the block diagrams the channel selector and the core isp are in the same block. And for better or worse to operate the ISP to process images the driver need to poke at the channel selector, even tho I fail to see why some of the core registers where put in the cs block. On Gen3 an equally interesting hardware design can be found, the CSI-2 channel selector was there placed together with the IP block for image capture DMA engines... Luckily that only created a mess in the driver and not in the bindings. Thanks again for your feedback, I really learn a lot! > > > > >> > >> 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. > > > Best regards, > Krzysztof
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(-)