| Message ID | 20240318-rp1-cfe-v1-2-ac6d960ff22d@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | media: raspberrypi: Support RPi5's CFE | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Mon, 18 Mar 2024 17:49:57 +0200, Tomi Valkeinen wrote: > Add DT bindings for raspberrypi,rp1-cfe. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 +++++++++++++++++++++ > 1 file changed, 103 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.example.dts:24:18: fatal error: dt-bindings/clock/rp1.h: No such file or directory 24 | #include <dt-bindings/clock/rp1.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1428: dt_binding_check] Error 2 make: *** [Makefile:240: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240318-rp1-cfe-v1-2-ac6d960ff22d@ideasonboard.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 18/03/2024 16:49, Tomi Valkeinen wrote: > Add DT bindings for raspberrypi,rp1-cfe. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 +++++++++++++++++++++ > 1 file changed, 103 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > new file mode 100644 > index 000000000000..7b2beeaaab0e > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > @@ -0,0 +1,103 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Raspberry Pi PiSP Camera Front End > + > +maintainers: > + - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com> > + > +description: | > + The Raspberry Pi PiSP Camera Front End is a module in Raspberrypi 5's RP1 I/O > + controller, that contains: > + - MIPI D-PHY > + - MIPI CSI-2 receiver > + - Simple image processor (called PiSP Front End, or FE) > + > + The FE documentation is available at: > + https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf > + > + The PHY and CSI-2 receiver part have no public documentation. > + > +properties: > + compatible: > + const: raspberrypi,rpi5-rp1-cfe > + > + reg: > + items: > + - description: CSI-2 registers > + - description: D-PHY registers > + - description: MIPI CFG (a simple top-level mux) registers > + - description: FE registers > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + port: > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + description: CSI-2 RX Port Only one port, so there is nothing to output to? > + > + properties: > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + minItems: 1 > + maxItems: 4 > + > + clock-lanes: > + maxItems: 1 > + > + clock-noncontinuous: true Drop > + > + required: > + - clock-lanes > + - data-lanes > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rp1.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/mfd/rp1.h> > + > + rpi1 { soc > + #address-cells = <2>; > + #size-cells = <2>; > + > + csi@110000 { Fix the indentation. You switched back to 2 spaces here... > + compatible = "raspberrypi,rp1-cfe"; > + reg = <0xc0 0x40110000 0x0 0x100>, > + <0xc0 0x40114000 0x0 0x100>, Just one space before 0x0 > Best regards, Krzysztof
On 18/03/2024 16:49, Tomi Valkeinen wrote: > Add DT bindings for raspberrypi,rp1-cfe. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 +++++++++++++++++++++ > 1 file changed, 103 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > new file mode 100644 > index 000000000000..7b2beeaaab0e > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > @@ -0,0 +1,103 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# Use compatible as filename. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Raspberry Pi PiSP Camera Front End > + > +properties: > + compatible: > + const: raspberrypi,rpi5-rp1-cfe Best regards, Krzysztof
On 19/03/2024 08:09, Krzysztof Kozlowski wrote: > On 18/03/2024 16:49, Tomi Valkeinen wrote: >> Add DT bindings for raspberrypi,rp1-cfe. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 +++++++++++++++++++++ >> 1 file changed, 103 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >> new file mode 100644 >> index 000000000000..7b2beeaaab0e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >> @@ -0,0 +1,103 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Raspberry Pi PiSP Camera Front End >> + >> +maintainers: >> + - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> + - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com> >> + >> +description: | >> + The Raspberry Pi PiSP Camera Front End is a module in Raspberrypi 5's RP1 I/O >> + controller, that contains: >> + - MIPI D-PHY >> + - MIPI CSI-2 receiver >> + - Simple image processor (called PiSP Front End, or FE) >> + >> + The FE documentation is available at: >> + https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf >> + >> + The PHY and CSI-2 receiver part have no public documentation. >> + >> +properties: >> + compatible: >> + const: raspberrypi,rpi5-rp1-cfe >> + >> + reg: >> + items: >> + - description: CSI-2 registers >> + - description: D-PHY registers >> + - description: MIPI CFG (a simple top-level mux) registers >> + - description: FE registers >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + port: >> + $ref: /schemas/graph.yaml#/$defs/port-base >> + additionalProperties: false >> + description: CSI-2 RX Port > > Only one port, so there is nothing to output to? The CFE has DMA, so it writes to memory. But no other outputs. >> + >> + properties: >> + endpoint: >> + $ref: video-interfaces.yaml# >> + unevaluatedProperties: false >> + >> + properties: >> + data-lanes: >> + minItems: 1 >> + maxItems: 4 >> + >> + clock-lanes: >> + maxItems: 1 >> + >> + clock-noncontinuous: true > > Drop Hmm, I saw this used in multiple other bindings, and thought it means the property is allowed and copied it here. If that's not the case, does this mean all the properties from video-interfaces.yaml are allowed (even invalid ones, like pclk-sample)? >> + >> + required: >> + - clock-lanes >> + - data-lanes >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/rp1.h> >> + #include <dt-bindings/interrupt-controller/irq.h> >> + #include <dt-bindings/mfd/rp1.h> >> + >> + rpi1 { > > soc That should actually be "rp1", not "rpi1". rp1 is the co-processor on which the cfe is located, so it doesn't reside in the soc itself. But perhaps that's not relevant, and "soc" is just a generic container that should always be used? >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + csi@110000 { > > Fix the indentation. You switched back to 2 spaces here... Oops. >> + compatible = "raspberrypi,rp1-cfe"; >> + reg = <0xc0 0x40110000 0x0 0x100>, >> + <0xc0 0x40114000 0x0 0x100>, > > Just one space before 0x0 Ok. Tomi
On 19/03/2024 08:23, Krzysztof Kozlowski wrote: > On 18/03/2024 16:49, Tomi Valkeinen wrote: >> Add DT bindings for raspberrypi,rp1-cfe. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 +++++++++++++++++++++ >> 1 file changed, 103 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >> new file mode 100644 >> index 000000000000..7b2beeaaab0e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >> @@ -0,0 +1,103 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# > > Use compatible as filename. Ah, indeed. I changed the compatible quite late, adding the "rpi5" as versioning, and missed changing the file name. I'll rename. Tomi >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Raspberry Pi PiSP Camera Front End > > >> + >> +properties: >> + compatible: >> + const: raspberrypi,rpi5-rp1-cfe > > > > Best regards, > Krzysztof >
On 19/03/2024 08:48, Tomi Valkeinen wrote: > On 19/03/2024 08:23, Krzysztof Kozlowski wrote: >> On 18/03/2024 16:49, Tomi Valkeinen wrote: >>> Add DT bindings for raspberrypi,rp1-cfe. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 >>> +++++++++++++++++++++ >>> 1 file changed, 103 insertions(+) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>> b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>> new file mode 100644 >>> index 000000000000..7b2beeaaab0e >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>> @@ -0,0 +1,103 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# >> >> Use compatible as filename. > > Ah, indeed. I changed the compatible quite late, adding the "rpi5" as > versioning, and missed changing the file name. > > I'll rename. Actually, maybe it's better to have two compatibles, "raspberrypi,rp1-cfe" as the generic one, and "raspberrypi,rpi5-rp1-cfe" (or something similar) for RaspberryPi 5. And I'm not sure if the "rp1" part is relevant there, would "raspberrypi,cfe" be just as fine? Naush? Tomi
On 19/03/2024 07:46, Tomi Valkeinen wrote: >>> + reg: >>> + items: >>> + - description: CSI-2 registers >>> + - description: D-PHY registers >>> + - description: MIPI CFG (a simple top-level mux) registers >>> + - description: FE registers >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + port: >>> + $ref: /schemas/graph.yaml#/$defs/port-base >>> + additionalProperties: false >>> + description: CSI-2 RX Port >> >> Only one port, so there is nothing to output to? > > The CFE has DMA, so it writes to memory. But no other outputs. You still might have some sort of pipeline, e.g. some post processing block. But if this is end block, then I guess it's fine. > >>> + >>> + properties: >>> + endpoint: >>> + $ref: video-interfaces.yaml# >>> + unevaluatedProperties: false >>> + >>> + properties: >>> + data-lanes: >>> + minItems: 1 >>> + maxItems: 4 >>> + >>> + clock-lanes: >>> + maxItems: 1 >>> + >>> + clock-noncontinuous: true >> >> Drop > > Hmm, I saw this used in multiple other bindings, and thought it means > the property is allowed and copied it here. > > If that's not the case, does this mean all the properties from > video-interfaces.yaml are allowed (even invalid ones, like pclk-sample)? Yes, that's the meaning of unevaluatedProperties you have a bit above. > >>> + >>> + required: >>> + - clock-lanes >>> + - data-lanes >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - clocks >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/clock/rp1.h> >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + #include <dt-bindings/mfd/rp1.h> >>> + >>> + rpi1 { >> >> soc > > That should actually be "rp1", not "rpi1". rp1 is the co-processor on > which the cfe is located, so it doesn't reside in the soc itself. But Where is the co-processor located? > perhaps that's not relevant, and "soc" is just a generic container that > should always be used? Does not have to be soc, but rp1 is not generic. Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Best regards, Krzysztof
On 19/03/2024 08:00, Tomi Valkeinen wrote: > On 19/03/2024 08:48, Tomi Valkeinen wrote: >> On 19/03/2024 08:23, Krzysztof Kozlowski wrote: >>> On 18/03/2024 16:49, Tomi Valkeinen wrote: >>>> Add DT bindings for raspberrypi,rp1-cfe. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> --- >>>> .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 >>>> +++++++++++++++++++++ >>>> 1 file changed, 103 insertions(+) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>>> b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>>> new file mode 100644 >>>> index 000000000000..7b2beeaaab0e >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>>> @@ -0,0 +1,103 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# >>> >>> Use compatible as filename. >> >> Ah, indeed. I changed the compatible quite late, adding the "rpi5" as >> versioning, and missed changing the file name. >> >> I'll rename. > > Actually, maybe it's better to have two compatibles, > "raspberrypi,rp1-cfe" as the generic one, and "raspberrypi,rpi5-rp1-cfe" > (or something similar) for RaspberryPi 5. > > And I'm not sure if the "rp1" part is relevant there, would > "raspberrypi,cfe" be just as fine? Naush? See writing bindings. Compatibles should be SoC specific. In some cases generic fallbacks make sense, in some note. But don't just choose "generic fallback" because you want. Provide rationale. Best regards, Krzysztof
Hi, On Tue, 19 Mar 2024 at 09:32, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 19/03/2024 08:00, Tomi Valkeinen wrote: > > On 19/03/2024 08:48, Tomi Valkeinen wrote: > >> On 19/03/2024 08:23, Krzysztof Kozlowski wrote: > >>> On 18/03/2024 16:49, Tomi Valkeinen wrote: > >>>> Add DT bindings for raspberrypi,rp1-cfe. > >>>> > >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>>> --- > >>>> .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 > >>>> +++++++++++++++++++++ > >>>> 1 file changed, 103 insertions(+) > >>>> > >>>> diff --git > >>>> a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > >>>> b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > >>>> new file mode 100644 > >>>> index 000000000000..7b2beeaaab0e > >>>> --- /dev/null > >>>> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > >>>> @@ -0,0 +1,103 @@ > >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>> +%YAML 1.2 > >>>> +--- > >>>> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# > >>> > >>> Use compatible as filename. > >> > >> Ah, indeed. I changed the compatible quite late, adding the "rpi5" as > >> versioning, and missed changing the file name. > >> > >> I'll rename. > > > > Actually, maybe it's better to have two compatibles, > > "raspberrypi,rp1-cfe" as the generic one, and "raspberrypi,rpi5-rp1-cfe" > > (or something similar) for RaspberryPi 5. > > > > And I'm not sure if the "rp1" part is relevant there, would > > "raspberrypi,cfe" be just as fine? Naush? > > See writing bindings. Compatibles should be SoC specific. In some cases > generic fallbacks make sense, in some note. But don't just choose > "generic fallback" because you want. Provide rationale. If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" would be the correct string. Naush > > Best regards, > Krzysztof >
On 19/03/2024 13:06, Naushir Patuck wrote: > Hi, > > On Tue, 19 Mar 2024 at 09:32, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 19/03/2024 08:00, Tomi Valkeinen wrote: >>> On 19/03/2024 08:48, Tomi Valkeinen wrote: >>>> On 19/03/2024 08:23, Krzysztof Kozlowski wrote: >>>>> On 18/03/2024 16:49, Tomi Valkeinen wrote: >>>>>> Add DT bindings for raspberrypi,rp1-cfe. >>>>>> >>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>>>> --- >>>>>> .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 >>>>>> +++++++++++++++++++++ >>>>>> 1 file changed, 103 insertions(+) >>>>>> >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>>>>> b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>>>>> new file mode 100644 >>>>>> index 000000000000..7b2beeaaab0e >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml >>>>>> @@ -0,0 +1,103 @@ >>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>> +%YAML 1.2 >>>>>> +--- >>>>>> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# >>>>> >>>>> Use compatible as filename. >>>> >>>> Ah, indeed. I changed the compatible quite late, adding the "rpi5" as >>>> versioning, and missed changing the file name. >>>> >>>> I'll rename. >>> >>> Actually, maybe it's better to have two compatibles, >>> "raspberrypi,rp1-cfe" as the generic one, and "raspberrypi,rpi5-rp1-cfe" >>> (or something similar) for RaspberryPi 5. >>> >>> And I'm not sure if the "rp1" part is relevant there, would >>> "raspberrypi,cfe" be just as fine? Naush? >> >> See writing bindings. Compatibles should be SoC specific. In some cases >> generic fallbacks make sense, in some note. But don't just choose >> "generic fallback" because you want. Provide rationale. > > If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" > would be the correct string. Sure, but then please think what if rp1 is on Rpi6, called exactly the same (rp1), with some minor differences? Could it be? I don't know, you are upstreaming this stuff. Just be also sure you read writing bindings document. Best regards, Krzysztof
On Tue, 19 Mar 2024 at 12:21, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 19/03/2024 13:06, Naushir Patuck wrote: > > Hi, > > > > On Tue, 19 Mar 2024 at 09:32, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 19/03/2024 08:00, Tomi Valkeinen wrote: > >>> On 19/03/2024 08:48, Tomi Valkeinen wrote: > >>>> On 19/03/2024 08:23, Krzysztof Kozlowski wrote: > >>>>> On 18/03/2024 16:49, Tomi Valkeinen wrote: > >>>>>> Add DT bindings for raspberrypi,rp1-cfe. > >>>>>> > >>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>>>>> --- > >>>>>> .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 > >>>>>> +++++++++++++++++++++ > >>>>>> 1 file changed, 103 insertions(+) > >>>>>> > >>>>>> diff --git > >>>>>> a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > >>>>>> b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > >>>>>> new file mode 100644 > >>>>>> index 000000000000..7b2beeaaab0e > >>>>>> --- /dev/null > >>>>>> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml > >>>>>> @@ -0,0 +1,103 @@ > >>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>>>> +%YAML 1.2 > >>>>>> +--- > >>>>>> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# > >>>>> > >>>>> Use compatible as filename. > >>>> > >>>> Ah, indeed. I changed the compatible quite late, adding the "rpi5" as > >>>> versioning, and missed changing the file name. > >>>> > >>>> I'll rename. > >>> > >>> Actually, maybe it's better to have two compatibles, > >>> "raspberrypi,rp1-cfe" as the generic one, and "raspberrypi,rpi5-rp1-cfe" > >>> (or something similar) for RaspberryPi 5. > >>> > >>> And I'm not sure if the "rp1" part is relevant there, would > >>> "raspberrypi,cfe" be just as fine? Naush? > >> > >> See writing bindings. Compatibles should be SoC specific. In some cases > >> generic fallbacks make sense, in some note. But don't just choose > >> "generic fallback" because you want. Provide rationale. > > > > If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" > > would be the correct string. > > Sure, but then please think what if rp1 is on Rpi6, called exactly the > same (rp1), with some minor differences? Could it be? Yes, this is definitely possible. In such cases, I would expect the hardware to have a version register that would be queried by the driver to adjust for minor differences, and the compatible string remains the same. Does that seem reasonable?
On 19/03/2024 13:57, Naushir Patuck wrote: >>>> >>>> See writing bindings. Compatibles should be SoC specific. In some cases >>>> generic fallbacks make sense, in some note. But don't just choose >>>> "generic fallback" because you want. Provide rationale. >>> >>> If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" >>> would be the correct string. >> >> Sure, but then please think what if rp1 is on Rpi6, called exactly the >> same (rp1), with some minor differences? Could it be? > > Yes, this is definitely possible. In such cases, I would expect the > hardware to have a version register that would be queried by the > driver to adjust for minor differences, and the compatible string > remains the same. Does that seem reasonable? The "would expect" is concerning. The register(s) must be there already, with proper value. Best regards, Krzysztof
On Tue, 19 Mar 2024 at 13:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 19/03/2024 13:57, Naushir Patuck wrote: > >>>> > >>>> See writing bindings. Compatibles should be SoC specific. In some cases > >>>> generic fallbacks make sense, in some note. But don't just choose > >>>> "generic fallback" because you want. Provide rationale. > >>> > >>> If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" > >>> would be the correct string. > >> > >> Sure, but then please think what if rp1 is on Rpi6, called exactly the > >> same (rp1), with some minor differences? Could it be? > > > > Yes, this is definitely possible. In such cases, I would expect the > > hardware to have a version register that would be queried by the > > driver to adjust for minor differences, and the compatible string > > remains the same. Does that seem reasonable? > > The "would expect" is concerning. The register(s) must be there already, > with proper value. > A version register already exists in the current hardware, so we will update it to identify future hardware revisions. Regards, Naush
On 19/03/2024 11:31, Krzysztof Kozlowski wrote: > On 19/03/2024 07:46, Tomi Valkeinen wrote: >>>> + reg: >>>> + items: >>>> + - description: CSI-2 registers >>>> + - description: D-PHY registers >>>> + - description: MIPI CFG (a simple top-level mux) registers >>>> + - description: FE registers >>>> + >>>> + interrupts: >>>> + maxItems: 1 >>>> + >>>> + clocks: >>>> + maxItems: 1 >>>> + >>>> + port: >>>> + $ref: /schemas/graph.yaml#/$defs/port-base >>>> + additionalProperties: false >>>> + description: CSI-2 RX Port >>> >>> Only one port, so there is nothing to output to? >> >> The CFE has DMA, so it writes to memory. But no other outputs. > > You still might have some sort of pipeline, e.g. some post processing > block. But if this is end block, then I guess it's fine. There is a processing block, FE. But it's considered part of the same module. Whether that's exactly correct or not, I'm not sure. I don't have detailed hardware docs, but my understanding of the architecture is that we have the D-PHY, CSI-2 RX and FE blocks, and a "MIPI CFG" wrapper around these (with some CSI-2 & FE muxing and interrupt status flags). These are all considered to be part of the same CFE module, and thus we represent it here as a single node. In the patch 4 I explain a bit more about the HW blocks, but maybe it would've been beneficial to have some description here too. Here's the relevant part: > +The PiSP Camera Front End (CFE) is a module which combines a CSI-2 receiver with > +a simple ISP, called the Front End (FE). > + > +The CFE has four DMA engines and can write frames from four separate streams > +received from the CSI-2 to the memory. One of those streams can also be routed > +directly to the FE, which can do minimal image processing, write two versions > +(e.g. non-scaled and downscaled versions) of the received frames to memory and > +provide statistics of the received frames. >>>> + >>>> + properties: >>>> + endpoint: >>>> + $ref: video-interfaces.yaml# >>>> + unevaluatedProperties: false >>>> + >>>> + properties: >>>> + data-lanes: >>>> + minItems: 1 >>>> + maxItems: 4 >>>> + >>>> + clock-lanes: >>>> + maxItems: 1 >>>> + >>>> + clock-noncontinuous: true >>> >>> Drop >> >> Hmm, I saw this used in multiple other bindings, and thought it means >> the property is allowed and copied it here. >> >> If that's not the case, does this mean all the properties from >> video-interfaces.yaml are allowed (even invalid ones, like pclk-sample)? > > Yes, that's the meaning of unevaluatedProperties you have a bit above. > >> >>>> + >>>> + required: >>>> + - clock-lanes >>>> + - data-lanes >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - interrupts >>>> + - clocks >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + #include <dt-bindings/clock/rp1.h> >>>> + #include <dt-bindings/interrupt-controller/irq.h> >>>> + #include <dt-bindings/mfd/rp1.h> >>>> + >>>> + rpi1 { >>> >>> soc >> >> That should actually be "rp1", not "rpi1". rp1 is the co-processor on >> which the cfe is located, so it doesn't reside in the soc itself. But > > Where is the co-processor located? RP1 is a separate chip on the board, behind PCIe. It contains multiple blocks, dealing with I/O (like this CSI-2, but also USB, eth, ...). To the driver the CFE just shows up as a normal memory mapped IP. Afaics, in theory CFE could as well be in the main SoC itself, so, for an example, I don't see "soc" as a bad parent node. Tomi >> perhaps that's not relevant, and "soc" is just a generic container that >> should always be used? > > Does not have to be soc, but rp1 is not generic. > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > > > > Best regards, > Krzysztof >
On 19/03/2024 15:05, Naushir Patuck wrote: > On Tue, 19 Mar 2024 at 13:02, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 19/03/2024 13:57, Naushir Patuck wrote: >>>>>> >>>>>> See writing bindings. Compatibles should be SoC specific. In some cases >>>>>> generic fallbacks make sense, in some note. But don't just choose >>>>>> "generic fallback" because you want. Provide rationale. >>>>> >>>>> If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" >>>>> would be the correct string. >>>> >>>> Sure, but then please think what if rp1 is on Rpi6, called exactly the >>>> same (rp1), with some minor differences? Could it be? >>> >>> Yes, this is definitely possible. In such cases, I would expect the >>> hardware to have a version register that would be queried by the >>> driver to adjust for minor differences, and the compatible string >>> remains the same. Does that seem reasonable? >> >> The "would expect" is concerning. The register(s) must be there already, >> with proper value. >> > > A version register already exists in the current hardware, so we will > update it to identify future hardware revisions. But that's a version register for the FE block, not for the whole module, right? Are you suggesting that you'll make sure the FE version will be changed every time anything in the bigger CFE block is changed, and thus the FE version would also reflect the whole CFE version? Can there be versions without the FE block, with just the CSI-2 parts? Also, I'm still wondering about the RP1 part there in the compatible string. Is it necessary? The CFE is located in the RP1 co-processor, but is that relevant? Is there a versioning for the whole RP1 chip? Maybe it's going to the wrong direction if we use the board/SoC for this compatible name, as it's actually the RP1 where the CFE is located in, not the SoC. Tomi
On Tue, 19 Mar 2024 at 14:03, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > > On 19/03/2024 15:05, Naushir Patuck wrote: > > On Tue, 19 Mar 2024 at 13:02, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 19/03/2024 13:57, Naushir Patuck wrote: > >>>>>> > >>>>>> See writing bindings. Compatibles should be SoC specific. In some cases > >>>>>> generic fallbacks make sense, in some note. But don't just choose > >>>>>> "generic fallback" because you want. Provide rationale. > >>>>> > >>>>> If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" > >>>>> would be the correct string. > >>>> > >>>> Sure, but then please think what if rp1 is on Rpi6, called exactly the > >>>> same (rp1), with some minor differences? Could it be? > >>> > >>> Yes, this is definitely possible. In such cases, I would expect the > >>> hardware to have a version register that would be queried by the > >>> driver to adjust for minor differences, and the compatible string > >>> remains the same. Does that seem reasonable? > >> > >> The "would expect" is concerning. The register(s) must be there already, > >> with proper value. > >> > > > > A version register already exists in the current hardware, so we will > > update it to identify future hardware revisions. > > But that's a version register for the FE block, not for the whole > module, right? Are you suggesting that you'll make sure the FE version > will be changed every time anything in the bigger CFE block is changed, > and thus the FE version would also reflect the whole CFE version? Yes, we will update the FE versioning when either CSI2 / FE blocks are updated. > > Can there be versions without the FE block, with just the CSI-2 parts? There is no version register just in the CSI2 block in isolation, so this is not possible. > > Also, I'm still wondering about the RP1 part there in the compatible > string. Is it necessary? The CFE is located in the RP1 co-processor, but > is that relevant? > > Is there a versioning for the whole RP1 chip? Maybe it's going to the > wrong direction if we use the board/SoC for this compatible name, as > it's actually the RP1 where the CFE is located in, not the SoC. > I don't really know the conversion required to answer this one. Logically CFE is on RP1, so it makes sense to me to have "rp1" in the string, but I will follow the judgment of the maintainers. Regards, Naush
On 19/03/2024 17:32, Naushir Patuck wrote: > On Tue, 19 Mar 2024 at 14:03, Tomi Valkeinen > <tomi.valkeinen@ideasonboard.com> wrote: >> >> On 19/03/2024 15:05, Naushir Patuck wrote: >>> On Tue, 19 Mar 2024 at 13:02, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 19/03/2024 13:57, Naushir Patuck wrote: >>>>>>>> >>>>>>>> See writing bindings. Compatibles should be SoC specific. In some cases >>>>>>>> generic fallbacks make sense, in some note. But don't just choose >>>>>>>> "generic fallback" because you want. Provide rationale. >>>>>>> >>>>>>> If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" >>>>>>> would be the correct string. >>>>>> >>>>>> Sure, but then please think what if rp1 is on Rpi6, called exactly the >>>>>> same (rp1), with some minor differences? Could it be? >>>>> >>>>> Yes, this is definitely possible. In such cases, I would expect the >>>>> hardware to have a version register that would be queried by the >>>>> driver to adjust for minor differences, and the compatible string >>>>> remains the same. Does that seem reasonable? >>>> >>>> The "would expect" is concerning. The register(s) must be there already, >>>> with proper value. >>>> >>> >>> A version register already exists in the current hardware, so we will >>> update it to identify future hardware revisions. >> >> But that's a version register for the FE block, not for the whole >> module, right? Are you suggesting that you'll make sure the FE version >> will be changed every time anything in the bigger CFE block is changed, >> and thus the FE version would also reflect the whole CFE version? > > Yes, we will update the FE versioning when either CSI2 / FE blocks are updated. > >> >> Can there be versions without the FE block, with just the CSI-2 parts? > > There is no version register just in the CSI2 block in isolation, so > this is not possible. I meant could there be a future RPx with only the CSI-2 parts on it, no FE? In which case we would not have any register for the version. But then, that would be a rather big change, so a different compatible string would be in order. So, while it's not exactly a perfect version register, I think it will work fine, assuming the HW people will actually increase the version also for changes outside FE. >> >> Also, I'm still wondering about the RP1 part there in the compatible >> string. Is it necessary? The CFE is located in the RP1 co-processor, but >> is that relevant? >> >> Is there a versioning for the whole RP1 chip? Maybe it's going to the >> wrong direction if we use the board/SoC for this compatible name, as >> it's actually the RP1 where the CFE is located in, not the SoC. >> > > I don't really know the conversion required to answer this one. > Logically CFE is on RP1, so it makes sense to me to have "rp1" in the > string, but I will follow the judgment of the maintainers. Well, my thinking here was that if we have a register from which to read the version, and Raspberry Pi would create a new co-processor, RP2, with the same CFE. Would we then have "raspberrypi,rp1-cfe" and "raspberrypi,rp2-cfe", even if there are no changes? Or would a plain "raspberrypi,cfe" do for both? In other words, if we don't need the "rp1" for versioning purposes, should it then be dropped? On the other hand, maybe it is safer to just keep the "rp1" there anyway... Tomi
On Tue, 19 Mar 2024 at 17:05, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > > On 19/03/2024 17:32, Naushir Patuck wrote: > > On Tue, 19 Mar 2024 at 14:03, Tomi Valkeinen > > <tomi.valkeinen@ideasonboard.com> wrote: > >> > >> On 19/03/2024 15:05, Naushir Patuck wrote: > >>> On Tue, 19 Mar 2024 at 13:02, Krzysztof Kozlowski > >>> <krzysztof.kozlowski@linaro.org> wrote: > >>>> > >>>> On 19/03/2024 13:57, Naushir Patuck wrote: > >>>>>>>> > >>>>>>>> See writing bindings. Compatibles should be SoC specific. In some cases > >>>>>>>> generic fallbacks make sense, in some note. But don't just choose > >>>>>>>> "generic fallback" because you want. Provide rationale. > >>>>>>> > >>>>>>> If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe" > >>>>>>> would be the correct string. > >>>>>> > >>>>>> Sure, but then please think what if rp1 is on Rpi6, called exactly the > >>>>>> same (rp1), with some minor differences? Could it be? > >>>>> > >>>>> Yes, this is definitely possible. In such cases, I would expect the > >>>>> hardware to have a version register that would be queried by the > >>>>> driver to adjust for minor differences, and the compatible string > >>>>> remains the same. Does that seem reasonable? > >>>> > >>>> The "would expect" is concerning. The register(s) must be there already, > >>>> with proper value. > >>>> > >>> > >>> A version register already exists in the current hardware, so we will > >>> update it to identify future hardware revisions. > >> > >> But that's a version register for the FE block, not for the whole > >> module, right? Are you suggesting that you'll make sure the FE version > >> will be changed every time anything in the bigger CFE block is changed, > >> and thus the FE version would also reflect the whole CFE version? > > > > Yes, we will update the FE versioning when either CSI2 / FE blocks are updated. > > > >> > >> Can there be versions without the FE block, with just the CSI-2 parts? > > > > There is no version register just in the CSI2 block in isolation, so > > this is not possible. > > I meant could there be a future RPx with only the CSI-2 parts on it, no > FE? In which case we would not have any register for the version. But > then, that would be a rather big change, so a different compatible > string would be in order. > > So, while it's not exactly a perfect version register, I think it will > work fine, assuming the HW people will actually increase the version > also for changes outside FE. > > >> > >> Also, I'm still wondering about the RP1 part there in the compatible > >> string. Is it necessary? The CFE is located in the RP1 co-processor, but > >> is that relevant? > >> > >> Is there a versioning for the whole RP1 chip? Maybe it's going to the > >> wrong direction if we use the board/SoC for this compatible name, as > >> it's actually the RP1 where the CFE is located in, not the SoC. > >> > > > > I don't really know the conversion required to answer this one. > > Logically CFE is on RP1, so it makes sense to me to have "rp1" in the > > string, but I will follow the judgment of the maintainers. > > Well, my thinking here was that if we have a register from which to read > the version, and Raspberry Pi would create a new co-processor, RP2, with > the same CFE. Would we then have "raspberrypi,rp1-cfe" and > "raspberrypi,rp2-cfe", even if there are no changes? Or would a plain > "raspberrypi,cfe" do for both? > > In other words, if we don't need the "rp1" for versioning purposes, > should it then be dropped? I agree with the above, you've convinced me that "raspberrypi,cfe" might be the more appropriate string, or a convincing argument for that to be a fallback string. Naush > > On the other hand, maybe it is safer to just keep the "rp1" there anyway... > > Tomi >
On 20/03/2024 09:50, Naushir Patuck wrote: >>>> >>>> Also, I'm still wondering about the RP1 part there in the compatible >>>> string. Is it necessary? The CFE is located in the RP1 co-processor, but >>>> is that relevant? >>>> >>>> Is there a versioning for the whole RP1 chip? Maybe it's going to the >>>> wrong direction if we use the board/SoC for this compatible name, as >>>> it's actually the RP1 where the CFE is located in, not the SoC. >>>> >>> >>> I don't really know the conversion required to answer this one. >>> Logically CFE is on RP1, so it makes sense to me to have "rp1" in the >>> string, but I will follow the judgment of the maintainers. >> >> Well, my thinking here was that if we have a register from which to read >> the version, and Raspberry Pi would create a new co-processor, RP2, with >> the same CFE. Would we then have "raspberrypi,rp1-cfe" and >> "raspberrypi,rp2-cfe", even if there are no changes? That would follow guidelines as expressed in writing bindings. >>Or would a plain >> "raspberrypi,cfe" do for both? >> >> In other words, if we don't need the "rp1" for versioning purposes, >> should it then be dropped? > > I agree with the above, you've convinced me that "raspberrypi,cfe" > might be the more appropriate string, or a convincing argument for > that to be a fallback string. Just follow the guidelines. If you come up with generic compatible alone, you could run to the same problems everyone is running. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml new file mode 100644 index 000000000000..7b2beeaaab0e --- /dev/null +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml @@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Raspberry Pi PiSP Camera Front End + +maintainers: + - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> + - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com> + +description: | + The Raspberry Pi PiSP Camera Front End is a module in Raspberrypi 5's RP1 I/O + controller, that contains: + - MIPI D-PHY + - MIPI CSI-2 receiver + - Simple image processor (called PiSP Front End, or FE) + + The FE documentation is available at: + https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf + + The PHY and CSI-2 receiver part have no public documentation. + +properties: + compatible: + const: raspberrypi,rpi5-rp1-cfe + + reg: + items: + - description: CSI-2 registers + - description: D-PHY registers + - description: MIPI CFG (a simple top-level mux) registers + - description: FE registers + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + description: CSI-2 RX Port + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + minItems: 1 + maxItems: 4 + + clock-lanes: + maxItems: 1 + + clock-noncontinuous: true + + required: + - clock-lanes + - data-lanes + +required: + - compatible + - reg + - interrupts + - clocks + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/rp1.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/mfd/rp1.h> + + rpi1 { + #address-cells = <2>; + #size-cells = <2>; + + csi@110000 { + compatible = "raspberrypi,rp1-cfe"; + reg = <0xc0 0x40110000 0x0 0x100>, + <0xc0 0x40114000 0x0 0x100>, + <0xc0 0x40120000 0x0 0x100>, + <0xc0 0x40124000 0x0 0x1000>; + + interrupts = <RP1_INT_MIPI0 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&rp1_clocks RP1_CLK_MIPI0_CFG>; + + port { + csi_ep: endpoint { + remote-endpoint = <&cam_endpoint>; + clock-lanes = <0>; + data-lanes = <1 2>; + }; + }; + }; + };
Add DT bindings for raspberrypi,rp1-cfe. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- .../bindings/media/raspberrypi,rp1-cfe.yaml | 103 +++++++++++++++++++++ 1 file changed, 103 insertions(+)