[PATCH v2 7/9] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[PATCH v2 7/9] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Jacopo Mondi]

Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
signal processor.

Datasheet:
https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../bindings/media/raspberrypi,pispbe.yaml    | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml

diff --git a/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
new file mode 100644
index 000000000000..d7839f32eabf
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
+
+maintainers:
+  - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
+  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+
+description: |
+  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
+  processor that fetches images in Bayer or Grayscale format from DRAM memory
+  in tiles and produces images consumable by application.
+
+  The full ISP documentation is available at:
+  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - brcm,bcm2712-pispbe
+      - const: raspberrypi,pispbe
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  iommus:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        isp@880000  {
+             compatible = "brcm,bcm2712-pispbe", "raspberrypi,pispbe";
+             reg = <0x10 0x00880000  0x0 0x4000>;
+             interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+             clocks = <&firmware_clocks 7>;
+             iommus = <&iommu2>;
+        };
+    };
--
2.43.0

Re: [PATCH v2 7/9] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Rob Herring]

On Fri, 23 Feb 2024 17:30:09 +0100, Jacopo Mondi wrote:
> Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> signal processor.
> 
> Datasheet:
> https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  .../bindings/media/raspberrypi,pispbe.yaml    | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 7/9] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Feb 23, 2024 at 05:30:09PM +0100, Jacopo Mondi wrote:
> Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> signal processor.
> 
> Datasheet:
> https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  .../bindings/media/raspberrypi,pispbe.yaml    | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> new file mode 100644
> index 000000000000..d7839f32eabf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> +
> +maintainers:
> +  - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> +
> +description: |
> +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> +  in tiles and produces images consumable by application.

s/application/applications/

> +
> +  The full ISP documentation is available at:

s/:$//

> +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - brcm,bcm2712-pispbe
> +      - const: raspberrypi,pispbe
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1

As this is a SoC IP with only memory and register interfaces, I would
expect two clocks to be present, one for the register interface (AHB ?
AXI4-Lite ?) and one for the memory interfaces (AXI4 ?). While the
register interface clock is likely always enabled (in all cases that
matter in practice) in the BCM2712, I'm not sure this can be guaranteed
for future integration in different SoCs. Should we plan for this, and
either define two clocks already (with one of them being optional), or
name the single clock ?

I know v1 named this clock "isp_be", and the name was dropped upon
Krzysztof's request, but I think naming the single clock "axi" or "aclk"
(assuming that one of them would be the right name) would be fine for
the reason explained above.

> +
> +  iommus:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        isp@880000  {
> +             compatible = "brcm,bcm2712-pispbe", "raspberrypi,pispbe";
> +             reg = <0x10 0x00880000  0x0 0x4000>;

Double space, I don't know if that's on purpose.

> +             interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> +             clocks = <&firmware_clocks 7>;
> +             iommus = <&iommu2>;
> +        };
> +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 7/9] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Jacopo Mondi]

Hi Laurent

On Fri, Mar 01, 2024 at 11:58:57PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Feb 23, 2024 at 05:30:09PM +0100, Jacopo Mondi wrote:
> > Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> > signal processor.
> >
> > Datasheet:
> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  .../bindings/media/raspberrypi,pispbe.yaml    | 63 +++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > new file mode 100644
> > index 000000000000..d7839f32eabf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > @@ -0,0 +1,63 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> > +
> > +maintainers:
> > +  - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > +
> > +description: |
> > +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> > +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> > +  in tiles and produces images consumable by application.
>
> s/application/applications/
>
> > +
> > +  The full ISP documentation is available at:
>
> s/:$//
>
> > +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - brcm,bcm2712-pispbe
> > +      - const: raspberrypi,pispbe
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
>
> As this is a SoC IP with only memory and register interfaces, I would
> expect two clocks to be present, one for the register interface (AHB ?
> AXI4-Lite ?) and one for the memory interfaces (AXI4 ?). While the
> register interface clock is likely always enabled (in all cases that
> matter in practice) in the BCM2712, I'm not sure this can be guaranteed
> for future integration in different SoCs. Should we plan for this, and
> either define two clocks already (with one of them being optional), or
> name the single clock ?
>
> I know v1 named this clock "isp_be", and the name was dropped upon
> Krzysztof's request, but I think naming the single clock "axi" or "aclk"
> (assuming that one of them would be the right name) would be fine for
> the reason explained above.
>

The PiSP datasheet does not offer many information on the IP
integration, only a small graph with the memory interfacing, but no
clocks.

However your reasoning makes sense, and unless someone from RPi
suggests the contrary, I'll do so

> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        isp@880000  {
> > +             compatible = "brcm,bcm2712-pispbe", "raspberrypi,pispbe";
> > +             reg = <0x10 0x00880000  0x0 0x4000>;
>
> Double space, I don't know if that's on purpose.
>

Ofc it was not.

Thanks
   j

> > +             interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&firmware_clocks 7>;
> > +             iommus = <&iommu2>;
> > +        };
> > +    };
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 7/9] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Naushir Patuck]

Hi Laurent and Jacopo,

On Tue, 5 Mar 2024 at 15:25, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Laurent
>
> On Fri, Mar 01, 2024 at 11:58:57PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Fri, Feb 23, 2024 at 05:30:09PM +0100, Jacopo Mondi wrote:
> > > Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> > > signal processor.
> > >
> > > Datasheet:
> > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  .../bindings/media/raspberrypi,pispbe.yaml    | 63 +++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > > new file mode 100644
> > > index 000000000000..d7839f32eabf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > > @@ -0,0 +1,63 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> > > +
> > > +maintainers:
> > > +  - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> > > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > +
> > > +description: |
> > > +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> > > +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> > > +  in tiles and produces images consumable by application.
> >
> > s/application/applications/
> >
> > > +
> > > +  The full ISP documentation is available at:
> >
> > s/:$//
> >
> > > +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - brcm,bcm2712-pispbe
> > > +      - const: raspberrypi,pispbe
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> >
> > As this is a SoC IP with only memory and register interfaces, I would
> > expect two clocks to be present, one for the register interface (AHB ?
> > AXI4-Lite ?) and one for the memory interfaces (AXI4 ?). While the
> > register interface clock is likely always enabled (in all cases that
> > matter in practice) in the BCM2712, I'm not sure this can be guaranteed
> > for future integration in different SoCs. Should we plan for this, and
> > either define two clocks already (with one of them being optional), or
> > name the single clock ?
> >
> > I know v1 named this clock "isp_be", and the name was dropped upon
> > Krzysztof's request, but I think naming the single clock "axi" or "aclk"
> > (assuming that one of them would be the right name) would be fine for
> > the reason explained above.
> >
>
> The PiSP datasheet does not offer many information on the IP
> integration, only a small graph with the memory interfacing, but no
> clocks.
>
> However your reasoning makes sense, and unless someone from RPi
> suggests the contrary, I'll do so

There is only a single clock that clocks the whole BE block, so does
the clock need to be explicitly named?  If it does, perhaps we can
just use "clk" as this is not explicitly an AXI or APB clock?

Naush

>
> > > +
> > > +  iommus:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - clocks
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +
> > > +    soc {
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +
> > > +        isp@880000  {
> > > +             compatible = "brcm,bcm2712-pispbe", "raspberrypi,pispbe";
> > > +             reg = <0x10 0x00880000  0x0 0x4000>;
> >
> > Double space, I don't know if that's on purpose.
> >
>
> Ofc it was not.
>
> Thanks
>    j
>
> > > +             interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > > +             clocks = <&firmware_clocks 7>;
> > > +             iommus = <&iommu2>;
> > > +        };
> > > +    };
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Re: [PATCH v2 7/9] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Laurent Pinchart]

On Wed, Mar 06, 2024 at 11:42:51AM +0000, Naushir Patuck wrote:
> On Tue, 5 Mar 2024 at 15:25, Jacopo Mondi wrote:
> > On Fri, Mar 01, 2024 at 11:58:57PM +0200, Laurent Pinchart wrote:
> > > On Fri, Feb 23, 2024 at 05:30:09PM +0100, Jacopo Mondi wrote:
> > > > Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> > > > signal processor.
> > > >
> > > > Datasheet:
> > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > >  .../bindings/media/raspberrypi,pispbe.yaml    | 63 +++++++++++++++++++
> > > >  1 file changed, 63 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > > > new file mode 100644
> > > > index 000000000000..d7839f32eabf
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
> > > > @@ -0,0 +1,63 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> > > > +
> > > > +maintainers:
> > > > +  - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> > > > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > +
> > > > +description: |
> > > > +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> > > > +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> > > > +  in tiles and produces images consumable by application.
> > >
> > > s/application/applications/
> > >
> > > > +
> > > > +  The full ISP documentation is available at:
> > >
> > > s/:$//
> > >
> > > > +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - brcm,bcm2712-pispbe
> > > > +      - const: raspberrypi,pispbe
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    maxItems: 1
> > >
> > > As this is a SoC IP with only memory and register interfaces, I would
> > > expect two clocks to be present, one for the register interface (AHB ?
> > > AXI4-Lite ?) and one for the memory interfaces (AXI4 ?). While the
> > > register interface clock is likely always enabled (in all cases that
> > > matter in practice) in the BCM2712, I'm not sure this can be guaranteed
> > > for future integration in different SoCs. Should we plan for this, and
> > > either define two clocks already (with one of them being optional), or
> > > name the single clock ?
> > >
> > > I know v1 named this clock "isp_be", and the name was dropped upon
> > > Krzysztof's request, but I think naming the single clock "axi" or "aclk"
> > > (assuming that one of them would be the right name) would be fine for
> > > the reason explained above.
> >
> > The PiSP datasheet does not offer many information on the IP
> > integration, only a small graph with the memory interfacing, but no
> > clocks.
> >
> > However your reasoning makes sense, and unless someone from RPi
> > suggests the contrary, I'll do so
> 
> There is only a single clock that clocks the whole BE block, so does
> the clock need to be explicitly named?  If it does, perhaps we can
> just use "clk" as this is not explicitly an AXI or APB clock?

If there's really a single clock then I don't think it needs to be
named. I was expecting there would be a clock for the register
interface, separate from the processing clock.

> > > > +
> > > > +  iommus:
> > > > +    maxItems: 1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - interrupts
> > > > +  - clocks
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +
> > > > +    soc {
> > > > +        #address-cells = <2>;
> > > > +        #size-cells = <2>;
> > > > +
> > > > +        isp@880000  {
> > > > +             compatible = "brcm,bcm2712-pispbe", "raspberrypi,pispbe";
> > > > +             reg = <0x10 0x00880000  0x0 0x4000>;
> > >
> > > Double space, I don't know if that's on purpose.
> >
> > Ofc it was not.
> >
> > > > +             interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > > > +             clocks = <&firmware_clocks 7>;
> > > > +             iommus = <&iommu2>;
> > > > +        };
> > > > +    };

-- 
Regards,

Laurent Pinchart