[RFC,v4,03/12] dt-bindings: media: Add bindings for bcm2835-unicam

Commit Message

Jean-Michel Hautbois Feb. 3, 2022, 5:50 p.m. UTC

Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
camera interface. Also add a MAINTAINERS entry for it.

Signed-off-by: Dave Stevenson <[e-mail]>
Signed-off-by: Naushir Patuck <[e-mail]>
Signed-off-by: Jean-Michel Hautbois <[e-mail]>

---
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   | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml

Comments

Stefan Wahren Feb. 3, 2022, 8:01 p.m. UTC | #1

Hi Jean-Michel,

Am 03.02.22 um 18:50 schrieb Jean-Michel Hautbois:
> Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> camera interface. Also add a MAINTAINERS entry for it.
>
> Signed-off-by: Dave Stevenson <[e-mail]>
> Signed-off-by: Naushir Patuck <[e-mail]>
> Signed-off-by: Jean-Michel Hautbois <[e-mail]>
>
> ---
> 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   | 110 ++++++++++++++++++
>  1 file changed, 110 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 000000000000..0725a0267c60
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> @@ -0,0 +1,110 @@
> +# 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 <[e-mail]>
> +
> +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 starting by 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: Unicam block.
> +      - description: Clock Manager Image (CMI) block.
thanks for describing the registers, but this is not what i meant with
adding "reg-names" in my last comment. Please look at the clocks below ...
> +
> +  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
> +
> +  
Best regards

Laurent Pinchart Feb. 4, 2022, 2:42 a.m. UTC | #2

Hi Jean-Michel,

Thank you for the patch.

On Thu, Feb 03, 2022 at 06:50:00PM +0100, Jean-Michel Hautbois wrote:
> Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> camera interface. Also add a MAINTAINERS entry for it.

You can drop the last sentence now that the MAINTAINERS entry has been
moved out.

> Signed-off-by: Dave Stevenson <[e-mail]>
> Signed-off-by: Naushir Patuck <[e-mail]>
> Signed-off-by: Jean-Michel Hautbois <[e-mail]>
> 
> ---
> 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   | 110 ++++++++++++++++++
>  1 file changed, 110 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 000000000000..0725a0267c60
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> @@ -0,0 +1,110 @@
> +# 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 <[e-mail]>
> +
> +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 starting by csi then

"[...] 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: Unicam block.
> +      - description: Clock Manager Image (CMI) block.

As Stefan pointed out, you need

  reg-names:
    items:
      - const: main
      - const: cmi

Alternatives for main could be core or unicam. Dave, do you have a
preference ?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Clock to drive the LP state machine of Unicam.
> +      - description: Clock for the vpu (core clock).

s/vpu/VPU/

> +
> +  clock-names:
> +    items:
> +      - const: lp
> +      - const: vpu
> +
> +  power-domains:
> +    items:
> +      - description: Unicam power domain
> +
> +  brcm,num-data-lanes:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 2, 4 ]
> +    description: Number of data lanes on the csi bus

I'd write

    description: |
      Number of CSI-2 data lanes supported by this Unicam instance. The number
      of data lanes actively used is specified with the data-lanes endpoint
      property.

> +
> +  port:
> +    additionalProperties: false

Shouldn't this be unevaluatedProperties ?

I would also put it after the $ref line.

> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes: true
> +          link-frequencies: true

link-frequencies is specified on the sensor side, not here. You can drop
it.

> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +    required:
> +      - endpoint
> +
> +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>
> +    csi1: csi@7e801000 {
> +        compatible = "brcm,bcm2835-unicam";
> +        reg = <0x7e801000 0x800>,
> +              <0x7e802004 0x4>;
> +        interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clocks BCM2835_CLOCK_CAM1>,
> +                 <&firmware_clocks 4>;
> +        clock-names = "lp", "vpu";
> +        power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>;
> +        brcm,num-data-lanes = <2>;
> +        port {
> +                csi1_ep: endpoint {

Wrong indentation.

> +                        remote-endpoint = <&imx219_0>;
> +                        data-lanes = <1 2>;
> +                        link-frequencies = /bits/ 64 <456000000>;

Drop link-frequencies here too.

With these small issues fixed,

Reviewed-by: Laurent Pinchart <[e-mail]>

> +                };
> +        };
> +    };
> +...

Alexander Stein Feb. 4, 2022, 8:50 a.m. UTC | #3

Am Donnerstag, 3. Februar 2022, 18:50:00 CET schrieb Jean-Michel Hautbois:
> Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> camera interface. Also add a MAINTAINERS entry for it.
> 
> Signed-off-by: Dave Stevenson <[e-mail]>
> Signed-off-by: Naushir Patuck <[e-mail]>
> Signed-off-by: Jean-Michel Hautbois <[e-mail]>
> 
> ---
> 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   | 110 ++++++++++++++++++
>  1 file changed, 110 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 000000000000..0725a0267c60
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> @@ -0,0 +1,110 @@
> +# 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 <[e-mail]>
> +
> +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 starting by 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: Unicam block.
> +      - description: Clock Manager Image (CMI) block.
> +
> +  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
> +
> +  power-domains:
> +    items:
> +      - description: Unicam power domain
> +
> +  brcm,num-data-lanes:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 2, 4 ]
> +    description: Number of data lanes on the csi bus

There is already data-lanes in Documentation/devicetree/bindings/media/video-
interfaces.yaml. AFAICS these two are identical. Can't the video-
interface.yaml be used for this? I'm no expert TBH.

Regards,
Alexander

Laurent Pinchart Feb. 5, 2022, 2:22 a.m. UTC | #4

Hi Alexander,

On Fri, Feb 04, 2022 at 09:50:06AM +0100, Alexander Stein wrote:
> Am Donnerstag, 3. Februar 2022, 18:50:00 CET schrieb Jean-Michel Hautbois:
> > Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> > camera interface. Also add a MAINTAINERS entry for it.
> > 
> > Signed-off-by: Dave Stevenson <[e-mail]>
> > Signed-off-by: Naushir Patuck <[e-mail]>
> > Signed-off-by: Jean-Michel Hautbois <[e-mail]>
> > 
> > ---
> > 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   | 110 ++++++++++++++++++
> >  1 file changed, 110 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 000000000000..0725a0267c60
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > @@ -0,0 +1,110 @@
> > +# 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 <[e-mail]>
> > +
> > +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 starting by 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: Unicam block.
> > +      - description: Clock Manager Image (CMI) block.
> > +
> > +  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
> > +
> > +  power-domains:
> > +    items:
> > +      - description: Unicam power domain
> > +
> > +  brcm,num-data-lanes:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 2, 4 ]
> > +    description: Number of data lanes on the csi bus
> 
> There is already data-lanes in Documentation/devicetree/bindings/media/video-
> interfaces.yaml. AFAICS these two are identical. Can't the video-
> interface.yaml be used for this? I'm no expert TBH.

This is the number of data lanes that the Unicam instance supports.
There are two Unicam instances, and they can have 2 or 4 data lanes
depending on the SoC. The data-lanes endpoint property indicates the
number of lanes used on a particular board.

Alexander Stein Feb. 7, 2022, 6:30 a.m. UTC | #5

Hi Laurent,

Am Samstag, 5. Februar 2022, 03:22:51 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Fri, Feb 04, 2022 at 09:50:06AM +0100, Alexander Stein wrote:
> > Am Donnerstag, 3. Februar 2022, 18:50:00 CET schrieb Jean-Michel Hautbois:
> > > Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> > > camera interface. Also add a MAINTAINERS entry for it.
> > > 
> > > Signed-off-by: Dave Stevenson <[e-mail]>
> > > Signed-off-by: Naushir Patuck <[e-mail]>
> > > Signed-off-by: Jean-Michel Hautbois
> > > <[e-mail]>
> > > 
> > > ---
> > > 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   | 110 ++++++++++++++++++
> > >  1 file changed, 110 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 000000000000..0725a0267c60
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > > @@ -0,0 +1,110 @@
> > > +# 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 <[e-mail]>
> > > +
> > > +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 starting by 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: Unicam block.
> > > +      - description: Clock Manager Image (CMI) block.
> > > +
> > > +  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
> > > +
> > > +  power-domains:
> > > +    items:
> > > +      - description: Unicam power domain
> > > +
> > > +  brcm,num-data-lanes:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [ 2, 4 ]
> > > +    description: Number of data lanes on the csi bus
> > 
> > There is already data-lanes in
> > Documentation/devicetree/bindings/media/video- interfaces.yaml. AFAICS
> > these two are identical. Can't the video-
> > interface.yaml be used for this? I'm no expert TBH.
> 
> This is the number of data lanes that the Unicam instance supports.
> There are two Unicam instances, and they can have 2 or 4 data lanes
> depending on the SoC. The data-lanes endpoint property indicates the
> number of lanes used on a particular board.

Thanks for the explanation. Isn't this something which could be derived from 
the compatible, e.g. having 2 different ones for 2 and 4 lanes respectively?
See [1] for a similar situation in the SPI subsystem.
I don't have a strong opinion, just want to share my feedback.

Regards,
Alexander

[1] https://patchwork.kernel.org/project/spi-devel-general/patch/
[e-mail]/#24641405

Laurent Pinchart Feb. 8, 2022, 1:36 a.m. UTC | #6

Hi Alexander,

On Mon, Feb 07, 2022 at 07:30:25AM +0100, Alexander Stein wrote:
> Am Samstag, 5. Februar 2022, 03:22:51 CET schrieb Laurent Pinchart:
> > On Fri, Feb 04, 2022 at 09:50:06AM +0100, Alexander Stein wrote:
> > > Am Donnerstag, 3. Februar 2022, 18:50:00 CET schrieb Jean-Michel Hautbois:
> > > > Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> > > > camera interface. Also add a MAINTAINERS entry for it.
> > > > 
> > > > Signed-off-by: Dave Stevenson <[e-mail]>
> > > > Signed-off-by: Naushir Patuck <[e-mail]>
> > > > Signed-off-by: Jean-Michel Hautbois <[e-mail]>
> > > > 
> > > > ---
> > > > 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   | 110 ++++++++++++++++++
> > > >  1 file changed, 110 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 000000000000..0725a0267c60
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > > > @@ -0,0 +1,110 @@
> > > > +# 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 <[e-mail]>
> > > > +
> > > > +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 starting by 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: Unicam block.
> > > > +      - description: Clock Manager Image (CMI) block.
> > > > +
> > > > +  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
> > > > +
> > > > +  power-domains:
> > > > +    items:
> > > > +      - description: Unicam power domain
> > > > +
> > > > +  brcm,num-data-lanes:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [ 2, 4 ]
> > > > +    description: Number of data lanes on the csi bus
> > > 
> > > There is already data-lanes in
> > > Documentation/devicetree/bindings/media/video- interfaces.yaml. AFAICS
> > > these two are identical. Can't the video-
> > > interface.yaml be used for this? I'm no expert TBH.
> > 
> > This is the number of data lanes that the Unicam instance supports.
> > There are two Unicam instances, and they can have 2 or 4 data lanes
> > depending on the SoC. The data-lanes endpoint property indicates the
> > number of lanes used on a particular board.
> 
> Thanks for the explanation. Isn't this something which could be derived from 
> the compatible, e.g. having 2 different ones for 2 and 4 lanes respectively?
> See [1] for a similar situation in the SPI subsystem.
> I don't have a strong opinion, just want to share my feedback.

Yes, it could also be done through compatible strings, but in this case
I think a vendor-specific property is better. The number of lanes routed
from the Unicam IP core to the external of the SoC depends on the exact
SoC model, so we would need to create different compatible strings for
essentially the same IP core.

> [1] https://patchwork.kernel.org/project/spi-devel-general/patch/[e-mail]/#24641405

Dave Stevenson Feb. 8, 2022, 11:35 a.m. UTC | #7

Hi Alexander and Laurent

On Tue, 8 Feb 2022 at 01:36, Laurent Pinchart
<[e-mail]> wrote:
>
> Hi Alexander,
>
> On Mon, Feb 07, 2022 at 07:30:25AM +0100, Alexander Stein wrote:
> > Am Samstag, 5. Februar 2022, 03:22:51 CET schrieb Laurent Pinchart:
> > > On Fri, Feb 04, 2022 at 09:50:06AM +0100, Alexander Stein wrote:
> > > > Am Donnerstag, 3. Februar 2022, 18:50:00 CET schrieb Jean-Michel Hautbois:
> > > > > Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> > > > > camera interface. Also add a MAINTAINERS entry for it.
> > > > >
> > > > > Signed-off-by: Dave Stevenson <[e-mail]>
> > > > > Signed-off-by: Naushir Patuck <[e-mail]>
> > > > > Signed-off-by: Jean-Michel Hautbois <[e-mail]>
> > > > >
> > > > > ---
> > > > > 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   | 110 ++++++++++++++++++
> > > > >  1 file changed, 110 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 000000000000..0725a0267c60
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > > > > @@ -0,0 +1,110 @@
> > > > > +# 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 <[e-mail]>
> > > > > +
> > > > > +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 starting by 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: Unicam block.
> > > > > +      - description: Clock Manager Image (CMI) block.
> > > > > +
> > > > > +  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
> > > > > +
> > > > > +  power-domains:
> > > > > +    items:
> > > > > +      - description: Unicam power domain
> > > > > +
> > > > > +  brcm,num-data-lanes:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    enum: [ 2, 4 ]
> > > > > +    description: Number of data lanes on the csi bus
> > > >
> > > > There is already data-lanes in
> > > > Documentation/devicetree/bindings/media/video- interfaces.yaml. AFAICS
> > > > these two are identical. Can't the video-
> > > > interface.yaml be used for this? I'm no expert TBH.
> > >
> > > This is the number of data lanes that the Unicam instance supports.
> > > There are two Unicam instances, and they can have 2 or 4 data lanes
> > > depending on the SoC. The data-lanes endpoint property indicates the
> > > number of lanes used on a particular board.
> >
> > Thanks for the explanation. Isn't this something which could be derived from
> > the compatible, e.g. having 2 different ones for 2 and 4 lanes respectively?
> > See [1] for a similar situation in the SPI subsystem.
> > I don't have a strong opinion, just want to share my feedback.
>
> Yes, it could also be done through compatible strings, but in this case
> I think a vendor-specific property is better. The number of lanes routed
> from the Unicam IP core to the external of the SoC depends on the exact
> SoC model, so we would need to create different compatible strings for
> essentially the same IP core.

Yes csi0 (only supporting 2 data lanes) and csi1 (supporting 4 lanes)
could have different compatibles. However lanes default to being
disabled, so there's no need to treat csi1 any differently to csi0 if
only using up to 2 lanes.

The issue is your second case as eg on Compute Module 4, all 4 data
lanes of csi1 on the BCM2711 are routed to the camera connector. On
the Pi4 which also uses BCM2711, only 2 data lanes of csi1 are routed
to the camera connector. It's the same SoC, so the same compatible
string would normally apply.
It's a board design decision over how many lanes to route, not a
difference in the silicon or IP core. With 3rd parties able to design
their own carrier boards for Compute Modules, there's no guarantee
that a Compute Module will always have 4 lanes routed either.

Take imx290 as a sensor driver which supports running on either 2 or 4
lanes based on DT. A user can take a dtoverlay to configure it for 4
lanes on a Pi4 and it can not work. If the driver can validate the
number of lanes requested then it can produce some form of error,
otherwise you get the support query.

If V4L2's CSI support did something similar to DRM's DSI support where
the API passed across the number of lanes to run with, then data-lanes
could be used for this validation (and lane reordering). However at
present we have to have the data-lanes property on all CSI2 devices in
a chain have to match.

This has been discussed previously [1], and I relented and dropped it
to only use data-lanes.
From a support side, having that validation avoids so many issues that
it will be in our vendor tree whether it is accepted to mainline or
not.

  Dave

[1] https://patchwork.linuxtv.org/project/linux-media/patch/888a28[e-mail]/

> > [1] https://patchwork.kernel.org/project/spi-devel-general/patch/[e-mail]/#24641405
>
> --
> Regards,
>
> Laurent Pinchart

Patch

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 000000000000..0725a0267c60
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
@@ -0,0 +1,110 @@ 
+# 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 <[e-mail]>
+
+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 starting by 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: Unicam block.
+      - description: Clock Manager Image (CMI) block.
+
+  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
+
+  power-domains:
+    items:
+      - description: Unicam power domain
+
+  brcm,num-data-lanes:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 2, 4 ]
+    description: Number of data lanes on the csi bus
+
+  port:
+    additionalProperties: false
+    $ref: /schemas/graph.yaml#/$defs/port-base
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes: true
+          link-frequencies: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+    required:
+      - endpoint
+
+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>
+    csi1: csi@7e801000 {
+        compatible = "brcm,bcm2835-unicam";
+        reg = <0x7e801000 0x800>,
+              <0x7e802004 0x4>;
+        interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clocks BCM2835_CLOCK_CAM1>,
+                 <&firmware_clocks 4>;
+        clock-names = "lp", "vpu";
+        power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>;
+        brcm,num-data-lanes = <2>;
+        port {
+                csi1_ep: endpoint {
+                        remote-endpoint = <&imx219_0>;
+                        data-lanes = <1 2>;
+                        link-frequencies = /bits/ 64 <456000000>;
+                };
+        };
+    };
+...