Hi Jacopo, On Thu, Dec 03, 2020 at 06:37:33PM +0100, Jacopo Mondi wrote: > Hi Sakari, > > On Thu, Dec 03, 2020 at 07:25:19PM +0200, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Thu, Dec 03, 2020 at 05:44:17PM +0100, Jacopo Mondi wrote: > > > Hi Sakari, > > > > > > On Wed, Dec 02, 2020 at 11:20:31PM +0200, Sakari Ailus wrote: > > > > Hi Jacopo, > > > > > > > > On Tue, Nov 10, 2020 at 06:40:33PM +0100, Jacopo Mondi wrote: > > > > > From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > > > > > > > > Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as > > > > > Unicam) on BCM283x SoCs. > > > > > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > > > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > > > > > --- > > > > > .../bindings/media/brcm,bcm2835-unicam.yaml | 155 ++++++++++++++++++ > > > > > 1 file changed, 155 insertions(+) > > > > > create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > > > > > new file mode 100644 > > > > > index 0000000000000..6ffc900e8ae8f > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > > > > > @@ -0,0 +1,155 @@ > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > +# Copyright (C) 2020 Raspberry Pi (Trading) Ltd. > > > > > +%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: > > > > > + - Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > > > + - Raspberry Pi kernel list <kernel-list@xxxxxxxxxxxxxxx> > > > > > + > > > > > +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 boards. On the > > > > > + Pi the VideoCore firmware can also control this hardware block, and driving > > > > > + it from two different processors will cause issues. To avoid 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 tree > > > > > + binding. > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + const: brcm,bcm2835-unicam > > > > > > > > The compatible string doesn't quite match with the title. Which SoCs are > > > > supported? > > > > > > > > > > Afaict bcm2835 and bcm2711. There might be more I'm not aware of. > > > There should be no distinction between then, but Dave knows more. > > > > > > Would a per-soc compatible work better or is a single brcm,unicam > > > good enough to start with and we can add per-soc compatibles if > > > there's any per-SoC difference that will need to be handled in future ? > > > > If there are any differences between them, that's the way to take them into > > account (assuming they can't told apart at runtime). In that case you could > > just do the check in the driver. I think some Allwinner CSI-2 / parallel > > receivers use that. > > > > There might be difference, I don't know, but as of now the driver does > not need to tell them apart. I agree if we ever find a SoC-specific > different then supporting old DTBS with a single compatible might get > tricky. > > I'll defer this to Dave, he knows how many SoC uses this IP and if > they will ever have to be told apart. > > > > > > > > > + > > > > > + reg: > > > > > + items: > > > > > + - description: Main registers block > > > > > + - description: Clock registers block > > > > > + > > > > > + interrupts: > > > > > + maxItems: 1 > > > > > + > > > > > + clocks: > > > > > + minItems: 1 > > > > > + maxItems: 2 > > > > > + > > > > > + clock-names: > > > > > + minItems: 1 > > > > > + items: > > > > > + - const: lp > > > > > + - const: core > > > > > + > > > > > + power-domains: > > > > > + maxItems: 1 > > > > > + > > > > > + brcm,num-data-lanes: > > > > > + description: > > > > > + The number of data lanes supported by this Unicam instance. It may be > > > > > + larger than the number of data lanes routed on the board, as described by > > > > > + the data-lanes property of the endpoint. > > > > > + allOf: > > > > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > > > > + - enum: [1, 2, 4] > > > > > > > > Do you need this information besides how many lanes are connected? > > > > > > > > Does the number of lanes change even within the same model of SoC? Could > > > > you use the compatible string to differentiate between them? > > > > > > > > > > I had a similar question: > > > https://patchwork.linuxtv.org/project/linux-media/patch/20200504092611.9798-5-laurent.pinchart@xxxxxxxxxxxxxxxx/#121079 > > > > > > I think the discussion ended with the fact that not all the available > > > lanes in the IP might be routed to the connector, and that depends on > > > the board. > > > > Right, and this is what this property describes. > > > > Ack to keep it then ? I mean that the data-lanes property describes the board. > > > Going back to my earlier question, doesn't the number of lanes available in > > the SoC depend on the SoC where this block is integrated, which could be > > indicated by a compatible string? > > > > I don't really have a strong opinion on either, but knowing the exact piece > > of hardware might be helpful for other reasons, too (I'm not commenting on > > Unicam in particular here). > > > > > > > > > > + > > > > > + port: > > > > > + type: object > > > > > + description: > > > > > + Input port node, as described in video-interfaces.txt. > > > > > + > > > > > + properties: > > > > > + endpoint: > > > > > + type: object > > > > > + > > > > > + properties: > > > > > + clock-lanes: > > > > > + items: > > > > > + - const: 0 > > > > > > > > Please drop. There's nothing to tell software here. > > > > > > > > > > Ack > > > > > > > > + > > > > > + data-lanes: > > > > > + description: > > > > > + Lane reordering is not supported, items shall be in order, > > > > > + starting at 1. > > > > > + allOf: > > > > > + - $ref: "/schemas/types.yaml#/definitions/uint32-array" > > > > > + - maxItems: 1 > > > > > + items: > > > > > + minItems: 1 > > > > > + maxItems: 4 > > > > > + items: > > > > > + - const: 1 > > > > > + - const: 2 > > > > > + - const: 3 > > > > > + - const: 4 > > > > > > > > No need to specify items. > > > > > > > > > > Actually here we might need to support [1, 2] and [1, 2, 3, 4]. > > > I'll rebase on the converted video-interfaces to see it that helps > > > > If you do not support lane reordering, then just the number of values > > matters. Ideally, indeed, it'd be monotonically incrementing integers from > > 1. > > > > See e.g. Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml for an > > example. It got merged earlier today. > > Ah! maximum: 8. I was about to reply to Rob's video-interfaces > conversion and this might need to be pointed out. > I was also about to ask it it's an issue if the bindings validation > does not catch arrays as: [1, 3] as the constraint of being > monotonically increasing values is expressed in words only. I guess > it's fine, the alternative syntax is awful: > > anyOf: > - items: > - const: 1 > - const: 2 > - items: > - const: 1 > - const: 2 > - const: 3 > - const: 4 The v4l2-fwnode framework doesn't do that, what it does check for, though, is that the same lane isn't used in more than a single position. After that it's up to the driver to make use of the configuration provided. If the driver doesn't support lane reordering, then it just ignores that information (well, devices where lane allocation to PHYs isn't static need some more than this). -- Regards, Sakari Ailus
