Hi, On Fri, Dec 4, 2020 at 1:27 AM Sakari Ailus <sakari.ailus@xxxxxx> 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. What we do is we use the first SoC that introduced the hardware block in the compatible string. For subsequent SoCs, if there is any doubt that the hardware might be different, then a SoC-specific compatible string is added in front. If the hardware ends up being the same, then the new compatible doesn't get used. If the hardware ends up different, then the new compatible string gets added to the driver to deal with the difference. If the hardware ends up being *incompatible*, then you have to remove the old compatible string, and only keep the SoC-specific one. So in your case you would have: brcm,bcm2835-unicam and brcm,bcm2711-unicam + brcm,bcm2835-unicam assuming the hardware is the same or at least backward-compatible. If you know a priori that the hardware is 100% the same, then you can choose whatever compatible you like, subject to approval from the DT binding maintainers I suppose. Regards ChenYu > > > > + > > > > + 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. > > 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. > > -- > Kind regards, > > Sakari Ailus
