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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxxxxx>
> > +  - Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > +
> > +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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux