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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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