Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 14 Feb 2022 10:39:54 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Stefan Wahren <stefan.wahren@i2se.com>
Cc:     Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,
        dave.stevenson@raspberrypi.com, devicetree@vger.kernel.org,
        kernel-list@raspberrypi.com, laurent.pinchart@ideasonboard.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
        lukasz@jany.st, mchehab@kernel.org, naush@raspberrypi.com,
        robh@kernel.org, tomi.valkeinen@ideasonboard.com,
        bcm-kernel-feedback-list@broadcom.com,
        Florian Fainelli <f.fainelli@gmail.com>
Subject: Re: [PATCH v5 03/11] dt-bindings: media: Add bindings for
 bcm2835-unicam
Message-ID: <20220214093954.5y4jbqcddmwhgxr5@houat>
References: <20220208155027.891055-1-jeanmichel.hautbois@ideasonboard.com>
 <20220208155027.891055-4-jeanmichel.hautbois@ideasonboard.com>
 <f58bf6a9-c63f-19ab-36c8-a9a7b9182859@i2se.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="t3f56jmwvuijnmiw"
Content-Disposition: inline
In-Reply-To: <f58bf6a9-c63f-19ab-36c8-a9a7b9182859@i2se.com>
Precedence: bulk


--t3f56jmwvuijnmiw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Sun, Feb 13, 2022 at 04:48:45PM +0100, Stefan Wahren wrote:
> as someone with a little more insight to the clocks, i like to know your
> opinion about the bcm2835-unicam binding.
>=20
> Am 08.02.22 um 16:50 schrieb Jean-Michel Hautbois:
> > Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> > camera interface.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.c=
om>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > ---
> > v4:
> > - make MAINTAINERS its own patch
> > - describe the reg and clocks correctly
> > - use a vendor entry for the number of data lanes
> > ---
> >  .../bindings/media/brcm,bcm2835-unicam.yaml   | 117 ++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm283=
5-unicam.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/brcm,bcm2835-unica=
m.yaml b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > new file mode 100644
> > index 000000000000..1938ace23b3d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > @@ -0,0 +1,117 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/brcm,bcm2835-unicam.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Broadcom BCM283x Camera Interface (Unicam)
> > +
> > +maintainers:
> > +  - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> > +
> > +description: |-
> > +  The Unicam block on BCM283x SoCs is the receiver for either
> > +  CSI-2 or CCP2 data from image sensors or similar devices.
> > +
> > +  The main platform using this SoC is the Raspberry Pi family of board=
s.  On
> > +  the Pi the VideoCore firmware can also control this hardware block, =
and
> > +  driving it from two different processors will cause issues.  To avoi=
d this,
> > +  the firmware checks the device tree configuration during boot. If it=
 finds
> > +  device tree nodes whose name starts with 'csi' then it will stop the=
 firmware
> > +  accessing the block, and it can then safely be used via the device t=
ree
> > +  binding.
> > +
> > +properties:
> > +  compatible:
> > +    const: brcm,bcm2835-unicam
> > +
> > +  reg:
> > +    items:
> > +      - description: Unicam block.
> > +      - description: Clock Manager Image (CMI) block.
> > +
> > +  reg-names:
> > +    items:
> > +      - const: unicam
> > +      - const: cmi
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Clock to drive the LP state machine of Unicam.
> > +      - description: Clock for the VPU (core clock).
> > +
> > +  clock-names:
> > +    items:
> > +      - const: lp
> > +      - const: vpu
> > +
>=20
> according to this patch [1], the unicam driver only needs the VPU clock
> reference just to enforce a minimum of 250 MHz. The firmware clock
> binding and its driver is specific to the bcm2711, but the Unicam IP
> exists since bcm2835.
>=20
> So do you think the clock part is correct or should be the VPU clock
> optional?

I think we should keep it mandatory. Indeed, that clock is shared with
the HVS that will change its rate on a regular basis, so even just
enforcing that 250MHz while it's on without a clock handle will be
fairly hard.

Also, those are the constraints we have now, but having the clock handle
all the time will allow us to add any constraint we might need in the
future.

And BCM2711 or not, the clock has probably always been there.

Maxime

--t3f56jmwvuijnmiw
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYgojagAKCRDj7w1vZxhR
xRwhAP4nTOU8IjZGJK9OngHxyKECh2qYDcYc37EHQpvvUHsodQD/eDaW99MTlwsB
G1jvqQ86KEqFJZSiz5OxGOzUBgGtSg4=
=jJIB
-----END PGP SIGNATURE-----

--t3f56jmwvuijnmiw--