Hi Laurent and Jacopo, On Tue, 5 Mar 2024 at 15:25, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> 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@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 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
