| Message ID | 20201110174036.220883-3-jacopo@jmondi.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: staging: Add bcm2835-unicam driver | expand |
Hi Jacopo, On Tue, Nov 10, 2020 at 06:40:33PM +0100, Jacopo Mondi wrote: > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as > Unicam) on BCM283x SoCs. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../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@raspberrypi.com> > + - Raspberry Pi kernel list <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 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? > + > + 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? > + > + 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. > + > + 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. > + > + lane-polarities: > + description: > + Lane inversion is not supported. If the property is specified, it > + shall contain all 0's. Ditto. > + allOf: > + - $ref: "/schemas/types.yaml#/definitions/uint32-array" > + - maxItems: 1 > + items: > + minItems: 2 > + maxItems: 5 > + items: > + - const: 0 > + - const: 0 > + - const: 0 > + - const: 0 > + - const: 0 > + > + remote-endpoint: true > + > + required: > + - data-lanes > + - remote-endpoint > + > + additionalProperties: false > + > + required: > + - endpoint > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - power-domains > + - brcm,num-data-lanes > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/bcm2835.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/power/raspberrypi-power.h> > + > + csi@7e801000 { > + compatible = "brcm,bcm2835-unicam"; > + reg = <0x7e801000 0x800>, > + <0x7e802004 0x4>; > + interrupts = <2 7>; > + clocks = <&clocks BCM2835_CLOCK_CAM1>; > + clock-names = "lp"; > + power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>; > + brcm,num-data-lanes = <4>; > + > + port { > + csi1_ep: endpoint { > + remote-endpoint = <&imx219_0>; > + data-lanes = <1 2>; > + }; > + }; > + }; > +... -- Sakari Ailus
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@raspberrypi.com> > > > > Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as > > Unicam) on BCM283x SoCs. > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../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@raspberrypi.com> > > + - Raspberry Pi kernel list <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 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 ? > > + > > + 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@ideasonboard.com/#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. > > + > > + 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 > > + > > + lane-polarities: > > + description: > > + Lane inversion is not supported. If the property is specified, it > > + shall contain all 0's. > > Ditto. > Ack > > + allOf: > > + - $ref: "/schemas/types.yaml#/definitions/uint32-array" > > + - maxItems: 1 > > + items: > > + minItems: 2 > > + maxItems: 5 > > + items: > > + - const: 0 > > + - const: 0 > > + - const: 0 > > + - const: 0 > > + - const: 0 > > + > > + remote-endpoint: true > > + > > + required: > > + - data-lanes > > + - remote-endpoint > > + > > + additionalProperties: false > > + > > + required: > > + - endpoint > > + > > + additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - power-domains > > + - brcm,num-data-lanes > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/bcm2835.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/power/raspberrypi-power.h> > > + > > + csi@7e801000 { > > + compatible = "brcm,bcm2835-unicam"; > > + reg = <0x7e801000 0x800>, > > + <0x7e802004 0x4>; > > + interrupts = <2 7>; > > + clocks = <&clocks BCM2835_CLOCK_CAM1>; > > + clock-names = "lp"; > > + power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>; > > + brcm,num-data-lanes = <4>; > > + > > + port { > > + csi1_ep: endpoint { > > + remote-endpoint = <&imx219_0>; > > + data-lanes = <1 2>; > > + }; > > + }; > > + }; > > +... > > -- > Sakari Ailus
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@raspberrypi.com> > > > > > > Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as > > > Unicam) on BCM283x SoCs. > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > .../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@raspberrypi.com> > > > + - Raspberry Pi kernel list <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 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. > > > > + > > > + 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@ideasonboard.com/#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
Hi, On Fri, Dec 4, 2020 at 1:27 AM Sakari Ailus <sakari.ailus@iki.fi> 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@raspberrypi.com> > > > > > > > > Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as > > > > Unicam) on BCM283x SoCs. > > > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > .../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@raspberrypi.com> > > > > + - Raspberry Pi kernel list <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 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@ideasonboard.com/#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
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@raspberrypi.com> > > > > > > > > Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as > > > > Unicam) on BCM283x SoCs. > > > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > .../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@raspberrypi.com> > > > > + - Raspberry Pi kernel list <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 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@ideasonboard.com/#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 ? > 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 > > -- > Kind regards, > > Sakari Ailus
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@raspberrypi.com> > > > > > > > > > > Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as > > > > > Unicam) on BCM283x SoCs. > > > > > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > .../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@raspberrypi.com> > > > > > + - Raspberry Pi kernel list <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 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@ideasonboard.com/#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
Hi Sakari, On Thu, Dec 03, 2020 at 07:41:02PM +0200, Sakari Ailus wrote: [snip] > > > > 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, I meant the dt-validation :) > 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
On Thu, Dec 03, 2020 at 06:50:27PM +0100, Jacopo Mondi wrote: > Hi Sakari, > > On Thu, Dec 03, 2020 at 07:41:02PM +0200, Sakari Ailus wrote: > > [snip] > > > > > > > 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, > > I meant the dt-validation :) I think it'd be very nice to have that check there, too. :-) -- Sakari Ailus
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@raspberrypi.com> + - Raspberry Pi kernel list <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 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 + + 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] + + port: + type: object + description: + Input port node, as described in video-interfaces.txt. + + properties: + endpoint: + type: object + + properties: + clock-lanes: + items: + - const: 0 + + 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 + + lane-polarities: + description: + Lane inversion is not supported. If the property is specified, it + shall contain all 0's. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32-array" + - maxItems: 1 + items: + minItems: 2 + maxItems: 5 + items: + - const: 0 + - const: 0 + - const: 0 + - const: 0 + - const: 0 + + remote-endpoint: true + + required: + - data-lanes + - remote-endpoint + + additionalProperties: false + + required: + - endpoint + + additionalProperties: false + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - power-domains + - brcm,num-data-lanes + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/bcm2835.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/power/raspberrypi-power.h> + + csi@7e801000 { + compatible = "brcm,bcm2835-unicam"; + reg = <0x7e801000 0x800>, + <0x7e802004 0x4>; + interrupts = <2 7>; + clocks = <&clocks BCM2835_CLOCK_CAM1>; + clock-names = "lp"; + power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>; + brcm,num-data-lanes = <4>; + + port { + csi1_ep: endpoint { + remote-endpoint = <&imx219_0>; + data-lanes = <1 2>; + }; + }; + }; +...