Hi Stefan,
On 02/02/2022 19:33, Stefan Wahren wrote:
> Hi Jean-Michel,
>
> please drop the first "media:" before dt-bindings.
>
> Am 02.02.22 um 18:56 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 <[email protected]>
>> Signed-off-by: Naushir Patuck <[email protected]>
>> Signed-off-by: Jean-Michel Hautbois <[email protected]>
>> ---
>> Dave: I assumed you were the maintainer for this file, as I based it on the
>> bcm2835-unicam.txt file. Are you happy to be added directly as the
>> maintainer, or should this be specified as "Raspberry Pi Kernel
>> Maintenance <[email protected]>"
>> ---
>> .../bindings/media/brcm,bcm2835-unicam.yaml | 107 ++++++++++++++++++
>> MAINTAINERS | 7 ++
>> 2 files changed, 114 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..5bf41a8834fa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
>> @@ -0,0 +1,107 @@
>> +# 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:
>> + - Dave Stevenson <[email protected]>
>> +
>> +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:
>> + maxItems: 2
> I would be nice to have reg-names here similar to the clocks.
Sure, I just don't know what the names are ;-).
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + clocks:
>> + items:
>> + - description: Clock for the camera.
>> + - description: Clock for the vpu.
>> +
>> + clock-names:
>> + items:
>> + - const: lp
>> + - const: vpu
>> +
>> + power-domains:
>> + items:
>> + - description: Unicam power domain
>> +
>> + num-data-lanes:
>> + items:
>> + - enum: [ 2, 4 ]
>> +
>> + 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
>> + - 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>;
>> + num-data-lanes = <2>;
>> + port {
>> + csi1_ep: endpoint {
>> + remote-endpoint = <&imx219_0>;
>> + data-lanes = <1 2>;
>> + link-frequencies = /bits/ 64 <456000000>;
>> + };
>> + };
>> + };
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a0770a861ca4..29344ea86847 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3670,6 +3670,13 @@ N: bcm113*
>> N: bcm216*
>> N: kona
>>
>> +BROADCOM BCM2835 CAMERA DRIVER
>> +M: Raspberry Pi Kernel Maintenance <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
>> +F: arch/arm/boot/dts/bcm283x*
>> +
>
> I suggest to make the MAINTAINERS changes a single separate patch
> instead of small incremental changes.
I can make it a separate patch, indeed.
>
> Best regards
>
>> BROADCOM BCM47XX MIPS ARCHITECTURE
>> M: Hauke Mehrtens <[email protected]>
>> M: Rafał Miłecki <[email protected]>
>
Hi Jean-Michel,
On Wed, Feb 02, 2022 at 11:09:20PM +0100, Jean-Michel Hautbois wrote:
> On 02/02/2022 19:33, Stefan Wahren wrote:
> > Hi Jean-Michel,
> >
> > please drop the first "media:" before dt-bindings.
> >
> > Am 02.02.22 um 18:56 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 <[email protected]>
> >> Signed-off-by: Naushir Patuck <[email protected]>
> >> Signed-off-by: Jean-Michel Hautbois <[email protected]>
> >> ---
> >> Dave: I assumed you were the maintainer for this file, as I based it on the
> >> bcm2835-unicam.txt file. Are you happy to be added directly as the
> >> maintainer, or should this be specified as "Raspberry Pi Kernel
> >> Maintenance <[email protected]>"
> >> ---
> >> .../bindings/media/brcm,bcm2835-unicam.yaml | 107 ++++++++++++++++++
> >> MAINTAINERS | 7 ++
> >> 2 files changed, 114 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..5bf41a8834fa
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> >> @@ -0,0 +1,107 @@
> >> +# 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:
> >> + - Dave Stevenson <[email protected]>
> >> +
> >> +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:
> >> + maxItems: 2
> >
> > I would be nice to have reg-names here similar to the clocks.
>
> Sure, I just don't know what the names are ;-).
Please discuss this with the Rasperry Pi developers to figure out then.
> >> +
> >> + interrupts:
> >> + maxItems: 1
> >> +
> >> + clocks:
> >> + items:
> >> + - description: Clock for the camera.
This also seems weird, as far as I know the SoC doesn't output a clock
for the camera sensor (and it should be specified in the camera sensor
DT node if it did anyway).
> >> + - description: Clock for the vpu.
> >> +
> >> + clock-names:
> >> + items:
> >> + - const: lp
> >> + - const: vpu
> >> +
> >> + power-domains:
> >> + items:
> >> + - description: Unicam power domain
> >> +
> >> + num-data-lanes:
This is a vendor-specific property and thus requires a vendor prefix.
> >> + items:
> >> + - enum: [ 2, 4 ]
> >> +
> >> + 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
> >> + - 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>;
> >> + num-data-lanes = <2>;
> >> + port {
> >> + csi1_ep: endpoint {
> >> + remote-endpoint = <&imx219_0>;
> >> + data-lanes = <1 2>;
> >> + link-frequencies = /bits/ 64 <456000000>;
> >> + };
> >> + };
> >> + };
> >> +...
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index a0770a861ca4..29344ea86847 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -3670,6 +3670,13 @@ N: bcm113*
> >> N: bcm216*
> >> N: kona
> >>
> >> +BROADCOM BCM2835 CAMERA DRIVER
> >> +M: Raspberry Pi Kernel Maintenance <[email protected]>
> >> +L: [email protected]
> >> +S: Maintained
> >> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> >> +F: arch/arm/boot/dts/bcm283x*
> >> +
> >
> > I suggest to make the MAINTAINERS changes a single separate patch
> > instead of small incremental changes.
>
> I can make it a separate patch, indeed.
>
> >> BROADCOM BCM47XX MIPS ARCHITECTURE
> >> M: Hauke Mehrtens <[email protected]>
> >> M: Rafał Miłecki <[email protected]>
--
Regards,
Laurent Pinchart
Hi Jean-Michel and Laurent
n Wed, 2 Feb 2022 at 22:36, Laurent Pinchart
<[email protected]> wrote:
>
> Hi Jean-Michel,
>
> On Wed, Feb 02, 2022 at 11:09:20PM +0100, Jean-Michel Hautbois wrote:
> > On 02/02/2022 19:33, Stefan Wahren wrote:
> > > Hi Jean-Michel,
> > >
> > > please drop the first "media:" before dt-bindings.
> > >
> > > Am 02.02.22 um 18:56 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 <[email protected]>
> > >> Signed-off-by: Naushir Patuck <[email protected]>
> > >> Signed-off-by: Jean-Michel Hautbois <[email protected]>
> > >> ---
> > >> Dave: I assumed you were the maintainer for this file, as I based it on the
> > >> bcm2835-unicam.txt file. Are you happy to be added directly as the
> > >> maintainer, or should this be specified as "Raspberry Pi Kernel
> > >> Maintenance <[email protected]>"
Probably easiest to switch to "Raspberry Pi Kernel Maintenance
<[email protected]>".
That list didn't exist when I originally wrote the doc, and it just
makes life easier should I decide to move on (not planning it). Naush
is on that list too.
> > >> ---
> > >> .../bindings/media/brcm,bcm2835-unicam.yaml | 107 ++++++++++++++++++
> > >> MAINTAINERS | 7 ++
> > >> 2 files changed, 114 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..5bf41a8834fa
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > >> @@ -0,0 +1,107 @@
> > >> +# 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:
> > >> + - Dave Stevenson <[email protected]>
> > >> +
> > >> +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:
> > >> + maxItems: 2
> > >
> > > I would be nice to have reg-names here similar to the clocks.
> >
> > Sure, I just don't know what the names are ;-).
>
> Please discuss this with the Rasperry Pi developers to figure out then.
It's the "Unicam" and "Clock Manager Image" (CMI) blocks respectively.
CMI is only 4 registers. It provides high speed clock source selection
for the two Unicam blocks, a camera test block that has never been
used, and one of the USB controllers. Each peripheral is controlled by
a separate register.
It was discussed previously and viewed as not worthwhile creating a
full clock driver for it.
> > >> +
> > >> + interrupts:
> > >> + maxItems: 1
> > >> +
> > >> + clocks:
> > >> + items:
> > >> + - description: Clock for the camera.
>
> This also seems weird, as far as I know the SoC doesn't output a clock
> for the camera sensor (and it should be specified in the camera sensor
> DT node if it did anyway).
It's the clocks to Unicam, not to the camera / sensor.
The LP clock drives the LP state machine of Unicam for the relevant
DPHY state transitions.
The VPU or core clock is needed to ensure that the other bus systems
are running fast enough for the data generated.
Dave
> > >> + - description: Clock for the vpu.
> > >> +
> > >> + clock-names:
> > >> + items:
> > >> + - const: lp
> > >> + - const: vpu
> > >> +
> > >> + power-domains:
> > >> + items:
> > >> + - description: Unicam power domain
> > >> +
> > >> + num-data-lanes:
>
> This is a vendor-specific property and thus requires a vendor prefix.
>
> > >> + items:
> > >> + - enum: [ 2, 4 ]
> > >> +
> > >> + 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
> > >> + - 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>;
> > >> + num-data-lanes = <2>;
> > >> + port {
> > >> + csi1_ep: endpoint {
> > >> + remote-endpoint = <&imx219_0>;
> > >> + data-lanes = <1 2>;
> > >> + link-frequencies = /bits/ 64 <456000000>;
> > >> + };
> > >> + };
> > >> + };
> > >> +...
> > >> diff --git a/MAINTAINERS b/MAINTAINERS
> > >> index a0770a861ca4..29344ea86847 100644
> > >> --- a/MAINTAINERS
> > >> +++ b/MAINTAINERS
> > >> @@ -3670,6 +3670,13 @@ N: bcm113*
> > >> N: bcm216*
> > >> N: kona
> > >>
> > >> +BROADCOM BCM2835 CAMERA DRIVER
> > >> +M: Raspberry Pi Kernel Maintenance <[email protected]>
> > >> +L: [email protected]
> > >> +S: Maintained
> > >> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > >> +F: arch/arm/boot/dts/bcm283x*
> > >> +
> > >
> > > I suggest to make the MAINTAINERS changes a single separate patch
> > > instead of small incremental changes.
> >
> > I can make it a separate patch, indeed.
> >
> > >> BROADCOM BCM47XX MIPS ARCHITECTURE
> > >> M: Hauke Mehrtens <[email protected]>
> > >> M: Rafał Miłecki <[email protected]>
>
> --
> Regards,
>
> Laurent Pinchart