[RFC,1/2,media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

  Document the DT bindings for the CSI2/CCP2 receiver peripheral
(known as Unicam) on BCM283x SoCs.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
  

Hi Dave,

Am 14.06.2017 um 17:15 schrieb Dave Stevenson:
> Document the DT bindings for the CSI2/CCP2 receiver peripheral
> (known as Unicam) on BCM283x SoCs.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>

please add the devicetree guys in CC for the binding.

> ---
>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>
> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> new file mode 100644
> index 0000000..cc5a451
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> @@ -0,0 +1,76 @@
> +Broadcom BCM283x Camera Interface (Unicam)
> +------------------------------------------
> +
> +The Unicam block on BCM283x SoCs is the receiver for either
> +CSI-2 or CCP2 data from image sensors or similar devices.

It would be nice to add some of your explanations to Hans in this
document or into the driver.

> +
> +Required properties:
> +===================
> +- compatible	: must be "brcm,bcm2835-unicam".
> +- reg		: physical base address and length of the register sets for the
> +		  device.
> +- interrupts	: should contain the IRQ line for this Unicam instance.
> +- clocks	: list of clock specifiers, corresponding to entries in
> +		  clock-names property.
> +- clock-names	: must contain an "lp_clock" entry, matching entries
> +		  in the clocks property.
> +
> +Optional properties
> +===================
> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
> +		  This value is the maximum number supported by this instance.
> +		  Known values of 2 or 4. Default is 2.

AFAIK, this isn't a common property yet. So possibly a vendor prefix
must be added.

> +
> +
> +Unicam supports a single port node. It should contain one 'port' child node
> +with child 'endpoint' node. Please refer to the bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +	csi1: csi@7e801000 {
> +		compatible = "brcm,bcm2835-unicam";
> +		reg = <0x7e801000 0x800>,
> +		      <0x7e802004 0x4>;
> +		interrupts = <2 7>;
> +		clocks = <&clocks BCM2835_CLOCK_CAM1>;
> +		clock-names = "lp_clock";
> +
> +		port {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			endpoint {
> +				remote-endpoint = <&tc358743_0>;
> +
> +			};
> +		};
> +	};
> +
> +	i2c0: i2c@7e205000 {
> +
> +		tc358743: tc358743@0f {

Usually the node name should describe the function of the node for example:

tc358743: csi-hdmi-bridge@0f

Best regards
Stefan

> +			compatible = "toshiba,tc358743";
> +			reg = <0x0f>;
> +			status = "okay";
> +
> +			clocks = <&tc358743_clk>;
> +			clock-names = "refclk";
> +
> +			tc358743_clk: bridge-clk {
> +				compatible = "fixed-clock";
> +				#clock-cells = <0>;
> +				clock-frequency = <27000000>;
> +			};
> +
> +			port {
> +				tc358743_0: endpoint {
> +					remote-endpoint = <&csi1>;
> +					clock-lanes = <0>;
> +					data-lanes = <1 2 3 4>;
> +					clock-noncontinuous;
> +					link-frequencies =
> +						/bits/ 64 <297000000>;
> +				};
> +			};
> +		};
> +	};
  

Hi Stefan.
Thanks for taking the time to review this.

On 15 June 2017 at 07:34, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi Dave,
>
> Am 14.06.2017 um 17:15 schrieb Dave Stevenson:
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>
> please add the devicetree guys in CC for the binding.

Will do for V2.

>> ---
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 0000000..cc5a451
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,76 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +------------------------------------------
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>
> It would be nice to add some of your explanations to Hans in this
> document or into the driver.

Will do.

>> +
>> +Required properties:
>> +===================
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg                : physical base address and length of the register sets for the
>> +               device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks     : list of clock specifiers, corresponding to entries in
>> +               clock-names property.
>> +- clock-names        : must contain an "lp_clock" entry, matching entries
>> +               in the clocks property.
>> +
>> +Optional properties
>> +===================
>> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
>> +               This value is the maximum number supported by this instance.
>> +               Known values of 2 or 4. Default is 2.
>
> AFAIK, this isn't a common property yet. So possibly a vendor prefix
> must be added.

I think yoiu are correct that it isn't a common property. I was
wanting to cover the situation on the majority of the Pi boards where
Unicam1 has been used which can handle 4 lanes, but only 2 have been
broken out to the camera connector.
Happy to add a vendor prefix in V2.

>> +
>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +     csi1: csi@7e801000 {
>> +             compatible = "brcm,bcm2835-unicam";
>> +             reg = <0x7e801000 0x800>,
>> +                   <0x7e802004 0x4>;
>> +             interrupts = <2 7>;
>> +             clocks = <&clocks BCM2835_CLOCK_CAM1>;
>> +             clock-names = "lp_clock";
>> +
>> +             port {
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     endpoint {
>> +                             remote-endpoint = <&tc358743_0>;
>> +
>> +                     };
>> +             };
>> +     };
>> +
>> +     i2c0: i2c@7e205000 {
>> +
>> +             tc358743: tc358743@0f {
>
> Usually the node name should describe the function of the node for example:
>
> tc358743: csi-hdmi-bridge@0f

Will do.

> Best regards
> Stefan
>
>> +                     compatible = "toshiba,tc358743";
>> +                     reg = <0x0f>;
>> +                     status = "okay";
>> +
>> +                     clocks = <&tc358743_clk>;
>> +                     clock-names = "refclk";
>> +
>> +                     tc358743_clk: bridge-clk {
>> +                             compatible = "fixed-clock";
>> +                             #clock-cells = <0>;
>> +                             clock-frequency = <27000000>;
>> +                     };
>> +
>> +                     port {
>> +                             tc358743_0: endpoint {
>> +                                     remote-endpoint = <&csi1>;
>> +                                     clock-lanes = <0>;
>> +                                     data-lanes = <1 2 3 4>;
>> +                                     clock-noncontinuous;
>> +                                     link-frequencies =
>> +                                             /bits/ 64 <297000000>;
>> +                             };
>> +                     };
>> +             };
>> +     };

Thanks
  Dave
  

Hi Dave,

Thanks for the set!

On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
> Document the DT bindings for the CSI2/CCP2 receiver peripheral
> (known as Unicam) on BCM283x SoCs.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> ---
>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> new file mode 100644
> index 0000000..cc5a451
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> @@ -0,0 +1,76 @@
> +Broadcom BCM283x Camera Interface (Unicam)
> +------------------------------------------
> +
> +The Unicam block on BCM283x SoCs is the receiver for either
> +CSI-2 or CCP2 data from image sensors or similar devices.
> +
> +Required properties:
> +===================
> +- compatible	: must be "brcm,bcm2835-unicam".
> +- reg		: physical base address and length of the register sets for the
> +		  device.
> +- interrupts	: should contain the IRQ line for this Unicam instance.
> +- clocks	: list of clock specifiers, corresponding to entries in
> +		  clock-names property.
> +- clock-names	: must contain an "lp_clock" entry, matching entries
> +		  in the clocks property.
> +
> +Optional properties
> +===================
> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
> +		  This value is the maximum number supported by this instance.
> +		  Known values of 2 or 4. Default is 2.

Please use "data-lanes" endpoint property instead. This is the number of
connected physical lanes and specific to the hardware.

Could you also document which endpoint properties are mandatory and which
ones optional?

> +
> +
> +Unicam supports a single port node. It should contain one 'port' child node
> +with child 'endpoint' node. Please refer to the bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +	csi1: csi@7e801000 {
> +		compatible = "brcm,bcm2835-unicam";
> +		reg = <0x7e801000 0x800>,
> +		      <0x7e802004 0x4>;
> +		interrupts = <2 7>;
> +		clocks = <&clocks BCM2835_CLOCK_CAM1>;
> +		clock-names = "lp_clock";
> +
> +		port {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			endpoint {
> +				remote-endpoint = <&tc358743_0>;
> +

Extra newline. Don't you need any other properties here?

> +			};
> +		};
> +	};
> +
> +	i2c0: i2c@7e205000 {
> +
> +		tc358743: tc358743@0f {
> +			compatible = "toshiba,tc358743";
> +			reg = <0x0f>;
> +			status = "okay";
> +
> +			clocks = <&tc358743_clk>;
> +			clock-names = "refclk";
> +
> +			tc358743_clk: bridge-clk {
> +				compatible = "fixed-clock";
> +				#clock-cells = <0>;
> +				clock-frequency = <27000000>;
> +			};
> +
> +			port {
> +				tc358743_0: endpoint {
> +					remote-endpoint = <&csi1>;

This one needs to refer to the endpoint, just as the one in the CSI-2
receiver does.

> +					clock-lanes = <0>;
> +					data-lanes = <1 2 3 4>;
> +					clock-noncontinuous;
> +					link-frequencies =
> +						/bits/ 64 <297000000>;
> +				};
> +			};
> +		};
> +	};
  

Hi Sakari.

Thanks for the review.

On 15 June 2017 at 13:59, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Dave,
>
> Thanks for the set!
>
> On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>> ---
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 0000000..cc5a451
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,76 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +------------------------------------------
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>> +
>> +Required properties:
>> +===================
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg                : physical base address and length of the register sets for the
>> +               device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks     : list of clock specifiers, corresponding to entries in
>> +               clock-names property.
>> +- clock-names        : must contain an "lp_clock" entry, matching entries
>> +               in the clocks property.
>> +
>> +Optional properties
>> +===================
>> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
>> +               This value is the maximum number supported by this instance.
>> +               Known values of 2 or 4. Default is 2.
>
> Please use "data-lanes" endpoint property instead. This is the number of
> connected physical lanes and specific to the hardware.

I'll rethink/test, but to explain what I was intending to achieve:

Registers UNICAM_DAT2 and UNICAM_DAT3 are not valid for instances of
the hardware that only have two lanes instantiated in silicon.
In the case of the whole BCM283x family, Unicam0 ony has 2 lanes
instantiated, whilst Unicam1 has the maximum 4 lanes. (Lower
resolution front cameras would connect to Unicam0, whilst the higher
resolution back cameras would go to Unicam1).

To further confuse matters, on the Pi platforms (other than the
Compute Module), it is Unicam1 that is brought out to the camera
connector but with only 2 lanes wired up.

I was intending to make it possible for the driver to avoid writing to
invalid registers, and also describe the platform limitations to allow
sanity checking.
I haven't tested against Unicam0 as yet to see what actually happens
if we try to write UNICAM_DAT2 or UNICAM_DAT3. If the hardware
silently swallows it then that requirement is null and void. I'll do
some testing tomorrow.
The second bit comes down to how friendly an error you want should
someone write an invalid DT with 'data-lanes' greater than can be
supported by the platform.

> Could you also document which endpoint properties are mandatory and which
> ones optional?

Will do, although I'm not sure there are any required properties.

>> +
>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +     csi1: csi@7e801000 {
>> +             compatible = "brcm,bcm2835-unicam";
>> +             reg = <0x7e801000 0x800>,
>> +                   <0x7e802004 0x4>;
>> +             interrupts = <2 7>;
>> +             clocks = <&clocks BCM2835_CLOCK_CAM1>;
>> +             clock-names = "lp_clock";
>> +
>> +             port {
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     endpoint {
>> +                             remote-endpoint = <&tc358743_0>;
>> +
>
> Extra newline. Don't you need any other properties here?

Newline done.
As above, I don't believe there are any other properties required, but
will double check. What extras were you expecting to see there?

>> +                     };
>> +             };
>> +     };
>> +
>> +     i2c0: i2c@7e205000 {
>> +
>> +             tc358743: tc358743@0f {
>> +                     compatible = "toshiba,tc358743";
>> +                     reg = <0x0f>;
>> +                     status = "okay";
>> +
>> +                     clocks = <&tc358743_clk>;
>> +                     clock-names = "refclk";
>> +
>> +                     tc358743_clk: bridge-clk {
>> +                             compatible = "fixed-clock";
>> +                             #clock-cells = <0>;
>> +                             clock-frequency = <27000000>;
>> +                     };
>> +
>> +                     port {
>> +                             tc358743_0: endpoint {
>> +                                     remote-endpoint = <&csi1>;
>
> This one needs to refer to the endpoint, just as the one in the CSI-2
> receiver does.

OK.
(I'm suspecting very few drivers actually follow that link, but I'll correct).

>> +                                     clock-lanes = <0>;
>> +                                     data-lanes = <1 2 3 4>;

Oops, DT author beware! That should only have 2 lanes defined (not
that the TC358743 driver looks at it beyond checking it is non-zero).

>> +                                     clock-noncontinuous;
>> +                                     link-frequencies =
>> +                                             /bits/ 64 <297000000>;
>> +                             };
>> +                     };
>> +             };
>> +     };
>
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi     XMPP: sailus@retiisi.org.uk
  

Hi Dave,

On Thu, Jun 15, 2017 at 05:15:04PM +0100, Dave Stevenson wrote:
> Hi Sakari.
> 
> Thanks for the review.
> 
> On 15 June 2017 at 13:59, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > Hi Dave,
> >
> > Thanks for the set!
> >
> > On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
> >> Document the DT bindings for the CSI2/CCP2 receiver peripheral
> >> (known as Unicam) on BCM283x SoCs.
> >>
> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> >> ---
> >>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
> >>  1 file changed, 76 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> >> new file mode 100644
> >> index 0000000..cc5a451
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> >> @@ -0,0 +1,76 @@
> >> +Broadcom BCM283x Camera Interface (Unicam)
> >> +------------------------------------------
> >> +
> >> +The Unicam block on BCM283x SoCs is the receiver for either
> >> +CSI-2 or CCP2 data from image sensors or similar devices.
> >> +
> >> +Required properties:
> >> +===================
> >> +- compatible : must be "brcm,bcm2835-unicam".
> >> +- reg                : physical base address and length of the register sets for the
> >> +               device.
> >> +- interrupts : should contain the IRQ line for this Unicam instance.
> >> +- clocks     : list of clock specifiers, corresponding to entries in
> >> +               clock-names property.
> >> +- clock-names        : must contain an "lp_clock" entry, matching entries
> >> +               in the clocks property.
> >> +
> >> +Optional properties
> >> +===================
> >> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
> >> +               This value is the maximum number supported by this instance.
> >> +               Known values of 2 or 4. Default is 2.
> >
> > Please use "data-lanes" endpoint property instead. This is the number of
> > connected physical lanes and specific to the hardware.
> 
> I'll rethink/test, but to explain what I was intending to achieve:
> 
> Registers UNICAM_DAT2 and UNICAM_DAT3 are not valid for instances of
> the hardware that only have two lanes instantiated in silicon.
> In the case of the whole BCM283x family, Unicam0 ony has 2 lanes
> instantiated, whilst Unicam1 has the maximum 4 lanes. (Lower
> resolution front cameras would connect to Unicam0, whilst the higher
> resolution back cameras would go to Unicam1).
> 
> To further confuse matters, on the Pi platforms (other than the
> Compute Module), it is Unicam1 that is brought out to the camera
> connector but with only 2 lanes wired up.

This information should be present in the DT. I.e. the data-lanes property.

v4l2_fwnode_endpoint_alloc_parse() can obtain that from DT, among other
things (I haven't checked the second patch yet).

> 
> I was intending to make it possible for the driver to avoid writing to
> invalid registers, and also describe the platform limitations to allow
> sanity checking.
> I haven't tested against Unicam0 as yet to see what actually happens
> if we try to write UNICAM_DAT2 or UNICAM_DAT3. If the hardware
> silently swallows it then that requirement is null and void. I'll do
> some testing tomorrow.
> The second bit comes down to how friendly an error you want should
> someone write an invalid DT with 'data-lanes' greater than can be
> supported by the platform.

I'd expect things not to work if there's a grave error in DT. I guess you
could assume something but the fact is that you know you don't know.

> 
> > Could you also document which endpoint properties are mandatory and which
> > ones optional?
> 
> Will do, although I'm not sure there are any required properties.

data-lanes, among other things?
  

Hi Sakari

On 16 June 2017 at 10:57, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Dave,
>
> On Thu, Jun 15, 2017 at 05:15:04PM +0100, Dave Stevenson wrote:
>> Hi Sakari.
>>
>> Thanks for the review.
>>
>> On 15 June 2017 at 13:59, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> > Hi Dave,
>> >
>> > Thanks for the set!
>> >
>> > On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
>> >> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> >> (known as Unicam) on BCM283x SoCs.
>> >>
>> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>> >> ---
>> >>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
>> >>  1 file changed, 76 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >> new file mode 100644
>> >> index 0000000..cc5a451
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >> @@ -0,0 +1,76 @@
>> >> +Broadcom BCM283x Camera Interface (Unicam)
>> >> +------------------------------------------
>> >> +
>> >> +The Unicam block on BCM283x SoCs is the receiver for either
>> >> +CSI-2 or CCP2 data from image sensors or similar devices.
>> >> +
>> >> +Required properties:
>> >> +===================
>> >> +- compatible : must be "brcm,bcm2835-unicam".
>> >> +- reg                : physical base address and length of the register sets for the
>> >> +               device.
>> >> +- interrupts : should contain the IRQ line for this Unicam instance.
>> >> +- clocks     : list of clock specifiers, corresponding to entries in
>> >> +               clock-names property.
>> >> +- clock-names        : must contain an "lp_clock" entry, matching entries
>> >> +               in the clocks property.
>> >> +
>> >> +Optional properties
>> >> +===================
>> >> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
>> >> +               This value is the maximum number supported by this instance.
>> >> +               Known values of 2 or 4. Default is 2.
>> >
>> > Please use "data-lanes" endpoint property instead. This is the number of
>> > connected physical lanes and specific to the hardware.
>>
>> I'll rethink/test, but to explain what I was intending to achieve:
>>
>> Registers UNICAM_DAT2 and UNICAM_DAT3 are not valid for instances of
>> the hardware that only have two lanes instantiated in silicon.
>> In the case of the whole BCM283x family, Unicam0 ony has 2 lanes
>> instantiated, whilst Unicam1 has the maximum 4 lanes. (Lower
>> resolution front cameras would connect to Unicam0, whilst the higher
>> resolution back cameras would go to Unicam1).
>>
>> To further confuse matters, on the Pi platforms (other than the
>> Compute Module), it is Unicam1 that is brought out to the camera
>> connector but with only 2 lanes wired up.
>
> This information should be present in the DT. I.e. the data-lanes property.
>
> v4l2_fwnode_endpoint_alloc_parse() can obtain that from DT, among other
> things (I haven't checked the second patch yet).

I was interpreting data-lanes as being a function of the CSI-2 source
only, not the CSI-2 sink.
I haven't seen any examples where it has been it has been set on a
sink, but if that is valid then it sounds like a sensible thing to do.
...
OK, I'd missed it in video_interfaces.txt where it has been set for
the sh-mobile-csi2 endpoint (not that there appears to be a driver
advertising sh-mobile-csi2 as a compatible string anymore - it was
removed in 4.9).

It sounds like adopting a sink endpoint property of "data-lanes"  is
reasonable. Make it optional with the driver adopting a default "<1
2>" and you'll cover 99% of the cases.
On Compute Modules it can be overridden to "<1 2 3 4>" for Unicam1.
Selecting more lanes on Unicam0 is just an error that the DT author
has to sort out.

>>
>> I was intending to make it possible for the driver to avoid writing to
>> invalid registers, and also describe the platform limitations to allow
>> sanity checking.
>> I haven't tested against Unicam0 as yet to see what actually happens
>> if we try to write UNICAM_DAT2 or UNICAM_DAT3. If the hardware
>> silently swallows it then that requirement is null and void. I'll do
>> some testing tomorrow.
>> The second bit comes down to how friendly an error you want should
>> someone write an invalid DT with 'data-lanes' greater than can be
>> supported by the platform.
>
> I'd expect things not to work if there's a grave error in DT. I guess you
> could assume something but the fact is that you know you don't know.
>>
>> > Could you also document which endpoint properties are mandatory and which
>> > ones optional?
>>
>> Will do, although I'm not sure there are any required properties.
>
> data-lanes, among other things?

Looking through struct v4l2_fwnode_bus_mipi_csi2 for the parsed result
of v4l2_fwnode_endpoint_parse_csi_bus:
- V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK  /
V4L2_MBUS_CSI2_CONTINUOUS_CLOCK are the only flags set. The clock is
generated by the CSI2 source, so not relevant to configure it on the
sink.
- data_lane has been discussed above. Lane reordering is not supported
on the hardware so only the number of entries is of any importance,
although consecutive lanes can be checked. If a default is adopted by
the driver, then it is not mandatory.
- the clock lane is on a dedicated pair of pins, so it will always be
<0>. Little point in checking it.
- num_data_lanes comes out of data-lane. Let the driver adopt a default.
- lane polarity inversion is not supported by the hardware, so it will
always be an array of 0's. Little point in checking.

"data-lanes" can be mandatory if you think that is of any benefit, but
personally I don't see it. The other properties give no benefit, so I
think I am correct with only remote-endpoint being present.

Thanks,
  Dave
  

Hi Dave,

(Cc the devicetree list, I missed that earlier. It's DT binding
documentation so that list should be cc'd.)

On Fri, Jun 16, 2017 at 03:40:02PM +0100, Dave Stevenson wrote:
> Hi Sakari
> 
> On 16 June 2017 at 10:57, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > Hi Dave,
> >
> > On Thu, Jun 15, 2017 at 05:15:04PM +0100, Dave Stevenson wrote:
> >> Hi Sakari.
> >>
> >> Thanks for the review.
> >>
> >> On 15 June 2017 at 13:59, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >> > Hi Dave,
> >> >
> >> > Thanks for the set!
> >> >
> >> > On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
> >> >> Document the DT bindings for the CSI2/CCP2 receiver peripheral
> >> >> (known as Unicam) on BCM283x SoCs.
> >> >>
> >> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> >> >> ---
> >> >>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++++++++++++++++++++++
> >> >>  1 file changed, 76 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> >> >> new file mode 100644
> >> >> index 0000000..cc5a451
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> >> >> @@ -0,0 +1,76 @@
> >> >> +Broadcom BCM283x Camera Interface (Unicam)
> >> >> +------------------------------------------
> >> >> +
> >> >> +The Unicam block on BCM283x SoCs is the receiver for either
> >> >> +CSI-2 or CCP2 data from image sensors or similar devices.
> >> >> +
> >> >> +Required properties:
> >> >> +===================
> >> >> +- compatible : must be "brcm,bcm2835-unicam".
> >> >> +- reg                : physical base address and length of the register sets for the
> >> >> +               device.
> >> >> +- interrupts : should contain the IRQ line for this Unicam instance.
> >> >> +- clocks     : list of clock specifiers, corresponding to entries in
> >> >> +               clock-names property.
> >> >> +- clock-names        : must contain an "lp_clock" entry, matching entries
> >> >> +               in the clocks property.
> >> >> +
> >> >> +Optional properties
> >> >> +===================
> >> >> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
> >> >> +               This value is the maximum number supported by this instance.
> >> >> +               Known values of 2 or 4. Default is 2.
> >> >
> >> > Please use "data-lanes" endpoint property instead. This is the number of
> >> > connected physical lanes and specific to the hardware.
> >>
> >> I'll rethink/test, but to explain what I was intending to achieve:
> >>
> >> Registers UNICAM_DAT2 and UNICAM_DAT3 are not valid for instances of
> >> the hardware that only have two lanes instantiated in silicon.
> >> In the case of the whole BCM283x family, Unicam0 ony has 2 lanes
> >> instantiated, whilst Unicam1 has the maximum 4 lanes. (Lower
> >> resolution front cameras would connect to Unicam0, whilst the higher
> >> resolution back cameras would go to Unicam1).
> >>
> >> To further confuse matters, on the Pi platforms (other than the
> >> Compute Module), it is Unicam1 that is brought out to the camera
> >> connector but with only 2 lanes wired up.
> >
> > This information should be present in the DT. I.e. the data-lanes property.
> >
> > v4l2_fwnode_endpoint_alloc_parse() can obtain that from DT, among other
> > things (I haven't checked the second patch yet).
> 
> I was interpreting data-lanes as being a function of the CSI-2 source
> only, not the CSI-2 sink.
> I haven't seen any examples where it has been it has been set on a
> sink, but if that is valid then it sounds like a sensible thing to do.
> ...
> OK, I'd missed it in video_interfaces.txt where it has been set for
> the sh-mobile-csi2 endpoint (not that there appears to be a driver
> advertising sh-mobile-csi2 as a compatible string anymore - it was
> removed in 4.9).
> 
> It sounds like adopting a sink endpoint property of "data-lanes"  is
> reasonable. Make it optional with the driver adopting a default "<1
> 2>" and you'll cover 99% of the cases.
> On Compute Modules it can be overridden to "<1 2 3 4>" for Unicam1.
> Selecting more lanes on Unicam0 is just an error that the DT author
> has to sort out.

Sounds good. There are relevant examples in e.g. here:

Documentation/devicetree/bindings/media/ti,omap3isp.txt
Documentation/devicetree/bindings/media/i2c/nokia,smia.txt


> >>
> >> I was intending to make it possible for the driver to avoid writing to
> >> invalid registers, and also describe the platform limitations to allow
> >> sanity checking.
> >> I haven't tested against Unicam0 as yet to see what actually happens
> >> if we try to write UNICAM_DAT2 or UNICAM_DAT3. If the hardware
> >> silently swallows it then that requirement is null and void. I'll do
> >> some testing tomorrow.
> >> The second bit comes down to how friendly an error you want should
> >> someone write an invalid DT with 'data-lanes' greater than can be
> >> supported by the platform.
> >
> > I'd expect things not to work if there's a grave error in DT. I guess you
> > could assume something but the fact is that you know you don't know.
> >>
> >> > Could you also document which endpoint properties are mandatory and which
> >> > ones optional?
> >>
> >> Will do, although I'm not sure there are any required properties.
> >
> > data-lanes, among other things?
> 
> Looking through struct v4l2_fwnode_bus_mipi_csi2 for the parsed result
> of v4l2_fwnode_endpoint_parse_csi_bus:
> - V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK  /
> V4L2_MBUS_CSI2_CONTINUOUS_CLOCK are the only flags set. The clock is
> generated by the CSI2 source, so not relevant to configure it on the
> sink.
> - data_lane has been discussed above. Lane reordering is not supported
> on the hardware so only the number of entries is of any importance,
> although consecutive lanes can be checked. If a default is adopted by
> the driver, then it is not mandatory.

I think it'd be good to make that explicit albeit not all drivers do. AFAIK
all of them are sensor drivers though, which in practice means there's only
a single configuration (all lanes in use).

> - the clock lane is on a dedicated pair of pins, so it will always be
> <0>. Little point in checking it.

Ack.

> - num_data_lanes comes out of data-lane. Let the driver adopt a default.
> - lane polarity inversion is not supported by the hardware, so it will
> always be an array of 0's. Little point in checking.

Ack.

> 
> "data-lanes" can be mandatory if you think that is of any benefit, but
> personally I don't see it. The other properties give no benefit, so I
> think I am correct with only remote-endpoint being present.

The DT should describe hardware. You can have defaults, but in case when the
parameter is a numerical value (such as the number of lanes), omitting the
property for one particular value is a bit odd IMO.

I wonder what others think.