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]

 



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@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?

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




[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