diff mbox series

[v5,3/8] dt-bindings: media: add bindings for TI DS90UB913

Message ID 20221208104006.316606-4-tomi.valkeinen@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series i2c-atr and FPDLink | expand

Commit Message

Tomi Valkeinen Dec. 8, 2022, 10:40 a.m. UTC
Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml

Comments

Laurent Pinchart Dec. 11, 2022, 5:13 p.m. UTC | #1
Hi Tomi,

Thank you for the patch.

On Thu, Dec 08, 2022 at 12:40:01PM +0200, Tomi Valkeinen wrote:
> Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> new file mode 100644
> index 000000000000..3a5b34c6bb64
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments DS90UB913 FPD-Link 3 Serializer

I think TI consistently writes it "FPD-Link III". If you rename it,
please do so through the whole series.

> +
> +maintainers:
> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> +
> +description:
> +  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ds90ub913a-q1

Is the -q1 suffix needed, are there other variants ?

> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  clocks:
> +    maxItems: 1
> +    description:
> +      Reference clock connected to the CLKIN pin.
> +
> +  clock-names:
> +    items:
> +      - const: clkin
> +
> +  '#clock-cells':
> +    const: 0
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description: CSI-2 input port
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +        description: FPD-Link 3 output port

I'd add

    required:
      - port@0
      - port@1

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +  i2c:
> +    $ref: /schemas/i2c/i2c-controller.yaml#
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - '#gpio-cells'
> +  - gpio-controller
> +  - '#clock-cells'
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    serializer {
> +      compatible = "ti,ds90ub913a-q1";
> +
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +
> +      clocks = <&clk_cam_48M>;
> +      clock-names = "clkin";
> +
> +      #clock-cells = <0>;
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +          ub913_in: endpoint {
> +            remote-endpoint = <&sensor_out>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +          endpoint {
> +            remote-endpoint = <&deser_fpd_in>;
> +          };
> +        };
> +      };
> +
> +      i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        sensor@48 {
> +          compatible = "aptina,mt9v111";
> +          reg = <0x48>;
> +
> +          clocks = <&fixed_clock>;
> +
> +          port {
> +            sensor_out: endpoint {
> +              remote-endpoint = <&ub913_in>;
> +            };
> +          };
> +        };
> +      };
> +    };
> +...
Laurent Pinchart Dec. 11, 2022, 5:21 p.m. UTC | #2
I missed one issue.

On Sun, Dec 11, 2022 at 07:13:10PM +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Dec 08, 2022 at 12:40:01PM +0200, Tomi Valkeinen wrote:
> > Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> > new file mode 100644
> > index 000000000000..3a5b34c6bb64
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> > @@ -0,0 +1,121 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Texas Instruments DS90UB913 FPD-Link 3 Serializer
> 
> I think TI consistently writes it "FPD-Link III". If you rename it,
> please do so through the whole series.
> 
> > +
> > +maintainers:
> > +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > +
> > +description:
> > +  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ti,ds90ub913a-q1
> 
> Is the -q1 suffix needed, are there other variants ?
> 
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +  gpio-controller: true
> > +
> > +  clocks:
> > +    maxItems: 1
> > +    description:
> > +      Reference clock connected to the CLKIN pin.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: clkin
> > +
> > +  '#clock-cells':
> > +    const: 0
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > +        unevaluatedProperties: false
> > +        description: CSI-2 input port

This should be "Parallel input port".

> > +
> > +        properties:
> > +          endpoint:
> > +            $ref: /schemas/media/video-interfaces.yaml#
> > +            unevaluatedProperties: false

Should at least the bus-width property be mandatory, as the device
supports both 10- and 12-bit inputs ?

> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        unevaluatedProperties: false
> > +        description: FPD-Link 3 output port
> 
> I'd add
> 
>     required:
>       - port@0
>       - port@1
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> > +  i2c:
> > +    $ref: /schemas/i2c/i2c-controller.yaml#
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - '#gpio-cells'
> > +  - gpio-controller
> > +  - '#clock-cells'
> > +  - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    serializer {
> > +      compatible = "ti,ds90ub913a-q1";
> > +
> > +      gpio-controller;
> > +      #gpio-cells = <2>;
> > +
> > +      clocks = <&clk_cam_48M>;
> > +      clock-names = "clkin";
> > +
> > +      #clock-cells = <0>;
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +          ub913_in: endpoint {
> > +            remote-endpoint = <&sensor_out>;
> > +          };
> > +        };
> > +
> > +        port@1 {
> > +          reg = <1>;
> > +          endpoint {
> > +            remote-endpoint = <&deser_fpd_in>;
> > +          };
> > +        };
> > +      };
> > +
> > +      i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        sensor@48 {
> > +          compatible = "aptina,mt9v111";
> > +          reg = <0x48>;
> > +
> > +          clocks = <&fixed_clock>;
> > +
> > +          port {
> > +            sensor_out: endpoint {
> > +              remote-endpoint = <&ub913_in>;
> > +            };
> > +          };
> > +        };
> > +      };
> > +    };
> > +...
Tomi Valkeinen Dec. 13, 2022, 1:21 p.m. UTC | #3
On 11/12/2022 19:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Dec 08, 2022 at 12:40:01PM +0200, Tomi Valkeinen wrote:
>> Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
>>   1 file changed, 121 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>> new file mode 100644
>> index 000000000000..3a5b34c6bb64
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>> @@ -0,0 +1,121 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments DS90UB913 FPD-Link 3 Serializer
> 
> I think TI consistently writes it "FPD-Link III". If you rename it,
> please do so through the whole series.

Indeed, good point.

>> +
>> +maintainers:
>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> +
>> +description:
>> +  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,ds90ub913a-q1
> 
> Is the -q1 suffix needed, are there other variants ?

I'm not sure, but I couldn't see at a quick glance. I don't know what 
the -q1 means. Even if many of the FPD-Link devices have -q1, there are 
models without -q1, like DS90CF366. Oh, all those non-q1s seem to be 
from Catalog, and -q1 versions from Automotive. But the -q1 seems to be 
part of the model name, so... Maybe it's better to keep it.

https://www.ti.com/interface/high-speed-serdes/fpd-link-serdes/products.html

>> +
>> +  '#gpio-cells':
>> +    const: 2
>> +
>> +  gpio-controller: true
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description:
>> +      Reference clock connected to the CLKIN pin.
>> +
>> +  clock-names:
>> +    items:
>> +      - const: clkin
>> +
>> +  '#clock-cells':
>> +    const: 0
>> +
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>> +        description: CSI-2 input port
>> +
>> +        properties:
>> +          endpoint:
>> +            $ref: /schemas/media/video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        unevaluatedProperties: false
>> +        description: FPD-Link 3 output port
> 
> I'd add
> 
>      required:
>        - port@0
>        - port@1

Ok.

  Tomi
Tomi Valkeinen Dec. 13, 2022, 1:36 p.m. UTC | #4
On 11/12/2022 19:21, Laurent Pinchart wrote:
> I missed one issue.
> 
> On Sun, Dec 11, 2022 at 07:13:10PM +0200, Laurent Pinchart wrote:
>> Hi Tomi,
>>
>> Thank you for the patch.
>>
>> On Thu, Dec 08, 2022 at 12:40:01PM +0200, Tomi Valkeinen wrote:
>>> Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
>>>   1 file changed, 121 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>>> new file mode 100644
>>> index 000000000000..3a5b34c6bb64
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>>> @@ -0,0 +1,121 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Texas Instruments DS90UB913 FPD-Link 3 Serializer
>>
>> I think TI consistently writes it "FPD-Link III". If you rename it,
>> please do so through the whole series.
>>
>>> +
>>> +maintainers:
>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> +
>>> +description:
>>> +  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ti,ds90ub913a-q1
>>
>> Is the -q1 suffix needed, are there other variants ?
>>
>>> +
>>> +  '#gpio-cells':
>>> +    const: 2
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +    description:
>>> +      Reference clock connected to the CLKIN pin.
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: clkin
>>> +
>>> +  '#clock-cells':
>>> +    const: 0
>>> +
>>> +  ports:
>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>> +
>>> +    properties:
>>> +      port@0:
>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>> +        unevaluatedProperties: false
>>> +        description: CSI-2 input port
> 
> This should be "Parallel input port".

Oops...

>>> +
>>> +        properties:
>>> +          endpoint:
>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>> +            unevaluatedProperties: false
> 
> Should at least the bus-width property be mandatory, as the device
> supports both 10- and 12-bit inputs ?

Hmm... It supports 10-bit, 12-bit HF and 12-bit LF modes. If we need to 
configure the mode based on DT, we need one more property for the HF/LF. 
Then again, the HF/LF is separate from the input port, it's more about 
internal operation and the link to the deserializer.

However, this (the mode) should always be set in the HW via the MODE 
pins. And the driver can read the HW's MODE from the registers. Only in 
some very odd circumstances should the mode be configured by hand (and 
then carefully, as the link to the deserializer will drop).

So the bus-width is not something that the driver would normally use. If 
we would need to define the bus-width and HF/LF in the DT for some 
reason in the future, I think an "old" DT without those specified should 
continue working fine, as the mode can be read from a register.

That said, to complicate matters, the deserializer needs to know the 
serializer's mode before it can communicate with it (and thus, before we 
can read the mode). This is set with the deserializer's "ti,rx-mode" 
property, where you find RAW10, RAW12LF and RAW12HF modes (and for 
ub953, CSI-2 sync and non-sync modes).

So if we would define the bus-width and HF/LF in ub913's properties, the 
deserializer could go peeking the mode from there. But is that a good 
idea... I'm not so sure.

  Tomi
Laurent Pinchart Dec. 26, 2022, 4:46 p.m. UTC | #5
Hi Tomi,

On Tue, Dec 13, 2022 at 03:36:49PM +0200, Tomi Valkeinen wrote:
> On 11/12/2022 19:21, Laurent Pinchart wrote:
> > On Sun, Dec 11, 2022 at 07:13:10PM +0200, Laurent Pinchart wrote:
> >> On Thu, Dec 08, 2022 at 12:40:01PM +0200, Tomi Valkeinen wrote:
> >>> Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.
> >>>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>> ---
> >>>   .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
> >>>   1 file changed, 121 insertions(+)
> >>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> >>> new file mode 100644
> >>> index 000000000000..3a5b34c6bb64
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> >>> @@ -0,0 +1,121 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Texas Instruments DS90UB913 FPD-Link 3 Serializer
> >>
> >> I think TI consistently writes it "FPD-Link III". If you rename it,
> >> please do so through the whole series.
> >>
> >>> +
> >>> +maintainers:
> >>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>> +
> >>> +description:
> >>> +  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - ti,ds90ub913a-q1
> >>
> >> Is the -q1 suffix needed, are there other variants ?
> >>
> >>> +
> >>> +  '#gpio-cells':
> >>> +    const: 2
> >>> +
> >>> +  gpio-controller: true
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>> +    description:
> >>> +      Reference clock connected to the CLKIN pin.
> >>> +
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: clkin
> >>> +
> >>> +  '#clock-cells':
> >>> +    const: 0
> >>> +
> >>> +  ports:
> >>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>> +
> >>> +    properties:
> >>> +      port@0:
> >>> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >>> +        unevaluatedProperties: false
> >>> +        description: CSI-2 input port
> > 
> > This should be "Parallel input port".
> 
> Oops...
> 
> >>> +
> >>> +        properties:
> >>> +          endpoint:
> >>> +            $ref: /schemas/media/video-interfaces.yaml#
> >>> +            unevaluatedProperties: false
> > 
> > Should at least the bus-width property be mandatory, as the device
> > supports both 10- and 12-bit inputs ?
> 
> Hmm... It supports 10-bit, 12-bit HF and 12-bit LF modes. If we need to 
> configure the mode based on DT, we need one more property for the HF/LF. 
> Then again, the HF/LF is separate from the input port, it's more about 
> internal operation and the link to the deserializer.
>
> However, this (the mode) should always be set in the HW via the MODE 
> pins. And the driver can read the HW's MODE from the registers. Only in 
> some very odd circumstances should the mode be configured by hand (and 
> then carefully, as the link to the deserializer will drop).

Both the DS90UB913A and DS90UB913Q datasheets state that the MODE pin on
the serializer only selects between PCLK and external oscillator modes.

The DS90UB913A datasheet seems to hint in documentation of the mode
select register (0x05) that the mode is selected on the deserializer and
transmitted to the serializer through the back-channel, as the
MODE_OVERRIDE bit is documented as "Allows overriding mode select bits
coming from back-channel" and the MODE_UP_TO_DATE bit as "Status of mode
select from Deserializer is up-to- date". Bits 2 and 3 are however named
"Pin_MODE_12-bit High Frequency" and "Pin_MODE_10-bit mode", which hint
that their value could come from a mode pin, but I see no trace of that
anywhere.

The DS90UB913Q datasheet is similar, with a notable difference in that
it documents bits 1 and 0 as reserved, where the DS90UB913A datasheet
documents them as mode override selection. In the same document, the
DS90UB914Q MODE pin is documented as selecting the 10-bit, 12-bit LF or
12-bit HF operation mode. The datasheet also states that "The
deserializer automatically configures the serializer to correct mode
through the back-channel".

Th DS90UB953 datasheet also hints in the documentation of the
BC_MODE_SELECT register (0x04) that the mode is configured automatically
for backward-compatible DVP mode. For CSI-2 mode, I assume the mode is
strapped from the MODE pin and not configured through the back-channel.

The DS90UB960 datasheet documents how to configure the mode on the
deserializer side, but doesn't state whether the serializer is then
automatically configured through the back-channel (in RAW/DVP mode). I
assume it is, do you have any information about this ?

> So the bus-width is not something that the driver would normally use. If 
> we would need to define the bus-width and HF/LF in the DT for some 
> reason in the future, I think an "old" DT without those specified should 
> continue working fine, as the mode can be read from a register.
> 
> That said, to complicate matters, the deserializer needs to know the 
> serializer's mode before it can communicate with it (and thus, before we 
> can read the mode). This is set with the deserializer's "ti,rx-mode" 
> property, where you find RAW10, RAW12LF and RAW12HF modes (and for 
> ub953, CSI-2 sync and non-sync modes).
> 
> So if we would define the bus-width and HF/LF in ub913's properties, the 
> deserializer could go peeking the mode from there. But is that a good 
> idea... I'm not so sure.

Peeking into another device's DT node isn't great. It looks like the
best option for the DS90UB913 is to specify the mode on the
deserializer's side (either through the MODE strap or with a software
override through DT). In case the serializer mode would need to be
manually overridden in the future, we could add an optional DT property.
Tomi Valkeinen Jan. 4, 2023, 8:12 a.m. UTC | #6
On 26/12/2022 18:46, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tue, Dec 13, 2022 at 03:36:49PM +0200, Tomi Valkeinen wrote:
>> On 11/12/2022 19:21, Laurent Pinchart wrote:
>>> On Sun, Dec 11, 2022 at 07:13:10PM +0200, Laurent Pinchart wrote:
>>>> On Thu, Dec 08, 2022 at 12:40:01PM +0200, Tomi Valkeinen wrote:
>>>>> Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.
>>>>>
>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> ---
>>>>>    .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
>>>>>    1 file changed, 121 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..3a5b34c6bb64
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
>>>>> @@ -0,0 +1,121 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Texas Instruments DS90UB913 FPD-Link 3 Serializer
>>>>
>>>> I think TI consistently writes it "FPD-Link III". If you rename it,
>>>> please do so through the whole series.
>>>>
>>>>> +
>>>>> +maintainers:
>>>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> +
>>>>> +description:
>>>>> +  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - ti,ds90ub913a-q1
>>>>
>>>> Is the -q1 suffix needed, are there other variants ?
>>>>
>>>>> +
>>>>> +  '#gpio-cells':
>>>>> +    const: 2
>>>>> +
>>>>> +  gpio-controller: true
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 1
>>>>> +    description:
>>>>> +      Reference clock connected to the CLKIN pin.
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: clkin
>>>>> +
>>>>> +  '#clock-cells':
>>>>> +    const: 0
>>>>> +
>>>>> +  ports:
>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>> +
>>>>> +    properties:
>>>>> +      port@0:
>>>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>>>> +        unevaluatedProperties: false
>>>>> +        description: CSI-2 input port
>>>
>>> This should be "Parallel input port".
>>
>> Oops...
>>
>>>>> +
>>>>> +        properties:
>>>>> +          endpoint:
>>>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>>>> +            unevaluatedProperties: false
>>>
>>> Should at least the bus-width property be mandatory, as the device
>>> supports both 10- and 12-bit inputs ?
>>
>> Hmm... It supports 10-bit, 12-bit HF and 12-bit LF modes. If we need to
>> configure the mode based on DT, we need one more property for the HF/LF.
>> Then again, the HF/LF is separate from the input port, it's more about
>> internal operation and the link to the deserializer.
>>
>> However, this (the mode) should always be set in the HW via the MODE
>> pins. And the driver can read the HW's MODE from the registers. Only in
>> some very odd circumstances should the mode be configured by hand (and
>> then carefully, as the link to the deserializer will drop).
> 
> Both the DS90UB913A and DS90UB913Q datasheets state that the MODE pin on
> the serializer only selects between PCLK and external oscillator modes.
> 
> The DS90UB913A datasheet seems to hint in documentation of the mode
> select register (0x05) that the mode is selected on the deserializer and
> transmitted to the serializer through the back-channel, as the
> MODE_OVERRIDE bit is documented as "Allows overriding mode select bits
> coming from back-channel" and the MODE_UP_TO_DATE bit as "Status of mode
> select from Deserializer is up-to- date". Bits 2 and 3 are however named
> "Pin_MODE_12-bit High Frequency" and "Pin_MODE_10-bit mode", which hint
> that their value could come from a mode pin, but I see no trace of that
> anywhere.
> 
> The DS90UB913Q datasheet is similar, with a notable difference in that
> it documents bits 1 and 0 as reserved, where the DS90UB913A datasheet
> documents them as mode override selection. In the same document, the
> DS90UB914Q MODE pin is documented as selecting the 10-bit, 12-bit LF or
> 12-bit HF operation mode. The datasheet also states that "The
> deserializer automatically configures the serializer to correct mode
> through the back-channel".
> 
> Th DS90UB953 datasheet also hints in the documentation of the
> BC_MODE_SELECT register (0x04) that the mode is configured automatically
> for backward-compatible DVP mode. For CSI-2 mode, I assume the mode is
> strapped from the MODE pin and not configured through the back-channel.
> 
> The DS90UB960 datasheet documents how to configure the mode on the
> deserializer side, but doesn't state whether the serializer is then
> automatically configured through the back-channel (in RAW/DVP mode). I
> assume it is, do you have any information about this ?

I have to admit I had missed the mode management of the RAW mode while 
going through all this. I had mostly looked at the UB953's CSI mode.

I don't have more information, but your analysis looks correct to me. So 
the whole mode thing is an interesting mix of serializer & deserializer 
HW straps, deserializer sending the (RAW) mode to the serializer, and 
then the override registers on the serializer side.

As to the original question, should we have mandatory bus-width for 
ub913... I don't think it would be useful, even after the updated 
understanding about modes. Do you agree?

>> So the bus-width is not something that the driver would normally use. If
>> we would need to define the bus-width and HF/LF in the DT for some
>> reason in the future, I think an "old" DT without those specified should
>> continue working fine, as the mode can be read from a register.
>>
>> That said, to complicate matters, the deserializer needs to know the
>> serializer's mode before it can communicate with it (and thus, before we
>> can read the mode). This is set with the deserializer's "ti,rx-mode"
>> property, where you find RAW10, RAW12LF and RAW12HF modes (and for
>> ub953, CSI-2 sync and non-sync modes).
>>
>> So if we would define the bus-width and HF/LF in ub913's properties, the
>> deserializer could go peeking the mode from there. But is that a good
>> idea... I'm not so sure.
> 
> Peeking into another device's DT node isn't great. It looks like the
> best option for the DS90UB913 is to specify the mode on the
> deserializer's side (either through the MODE strap or with a software
> override through DT). In case the serializer mode would need to be
> manually overridden in the future, we could add an optional DT property.

I don't like the mode strap on UB960 side, as it's for all ports. It 
works in certain cases, of course, but if we anyway need the mode in DT 
to allow port-specific configuration, I think it's just easier to always 
require the DT mode and always use that, overriding the strap mode.

  Tomi
Laurent Pinchart Jan. 4, 2023, 12:52 p.m. UTC | #7
Hi Tomi,

On Wed, Jan 04, 2023 at 10:12:56AM +0200, Tomi Valkeinen wrote:
> On 26/12/2022 18:46, Laurent Pinchart wrote:
> > On Tue, Dec 13, 2022 at 03:36:49PM +0200, Tomi Valkeinen wrote:
> >> On 11/12/2022 19:21, Laurent Pinchart wrote:
> >>> On Sun, Dec 11, 2022 at 07:13:10PM +0200, Laurent Pinchart wrote:
> >>>> On Thu, Dec 08, 2022 at 12:40:01PM +0200, Tomi Valkeinen wrote:
> >>>>> Add DT bindings for TI DS90UB913 FPDLink-3 Serializer.
> >>>>>
> >>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>> ---
> >>>>>    .../bindings/media/i2c/ti,ds90ub913.yaml      | 121 ++++++++++++++++++
> >>>>>    1 file changed, 121 insertions(+)
> >>>>>    create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..3a5b34c6bb64
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
> >>>>> @@ -0,0 +1,121 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Texas Instruments DS90UB913 FPD-Link 3 Serializer
> >>>>
> >>>> I think TI consistently writes it "FPD-Link III". If you rename it,
> >>>> please do so through the whole series.
> >>>>
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>> +
> >>>>> +description:
> >>>>> +  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    enum:
> >>>>> +      - ti,ds90ub913a-q1
> >>>>
> >>>> Is the -q1 suffix needed, are there other variants ?
> >>>>
> >>>>> +
> >>>>> +  '#gpio-cells':
> >>>>> +    const: 2
> >>>>> +
> >>>>> +  gpio-controller: true
> >>>>> +
> >>>>> +  clocks:
> >>>>> +    maxItems: 1
> >>>>> +    description:
> >>>>> +      Reference clock connected to the CLKIN pin.
> >>>>> +
> >>>>> +  clock-names:
> >>>>> +    items:
> >>>>> +      - const: clkin
> >>>>> +
> >>>>> +  '#clock-cells':
> >>>>> +    const: 0
> >>>>> +
> >>>>> +  ports:
> >>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>> +
> >>>>> +    properties:
> >>>>> +      port@0:
> >>>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >>>>> +        unevaluatedProperties: false
> >>>>> +        description: CSI-2 input port
> >>>
> >>> This should be "Parallel input port".
> >>
> >> Oops...
> >>
> >>>>> +
> >>>>> +        properties:
> >>>>> +          endpoint:
> >>>>> +            $ref: /schemas/media/video-interfaces.yaml#
> >>>>> +            unevaluatedProperties: false
> >>>
> >>> Should at least the bus-width property be mandatory, as the device
> >>> supports both 10- and 12-bit inputs ?
> >>
> >> Hmm... It supports 10-bit, 12-bit HF and 12-bit LF modes. If we need to
> >> configure the mode based on DT, we need one more property for the HF/LF.
> >> Then again, the HF/LF is separate from the input port, it's more about
> >> internal operation and the link to the deserializer.
> >>
> >> However, this (the mode) should always be set in the HW via the MODE
> >> pins. And the driver can read the HW's MODE from the registers. Only in
> >> some very odd circumstances should the mode be configured by hand (and
> >> then carefully, as the link to the deserializer will drop).
> > 
> > Both the DS90UB913A and DS90UB913Q datasheets state that the MODE pin on
> > the serializer only selects between PCLK and external oscillator modes.
> > 
> > The DS90UB913A datasheet seems to hint in documentation of the mode
> > select register (0x05) that the mode is selected on the deserializer and
> > transmitted to the serializer through the back-channel, as the
> > MODE_OVERRIDE bit is documented as "Allows overriding mode select bits
> > coming from back-channel" and the MODE_UP_TO_DATE bit as "Status of mode
> > select from Deserializer is up-to- date". Bits 2 and 3 are however named
> > "Pin_MODE_12-bit High Frequency" and "Pin_MODE_10-bit mode", which hint
> > that their value could come from a mode pin, but I see no trace of that
> > anywhere.
> > 
> > The DS90UB913Q datasheet is similar, with a notable difference in that
> > it documents bits 1 and 0 as reserved, where the DS90UB913A datasheet
> > documents them as mode override selection. In the same document, the
> > DS90UB914Q MODE pin is documented as selecting the 10-bit, 12-bit LF or
> > 12-bit HF operation mode. The datasheet also states that "The
> > deserializer automatically configures the serializer to correct mode
> > through the back-channel".
> > 
> > Th DS90UB953 datasheet also hints in the documentation of the
> > BC_MODE_SELECT register (0x04) that the mode is configured automatically
> > for backward-compatible DVP mode. For CSI-2 mode, I assume the mode is
> > strapped from the MODE pin and not configured through the back-channel.
> > 
> > The DS90UB960 datasheet documents how to configure the mode on the
> > deserializer side, but doesn't state whether the serializer is then
> > automatically configured through the back-channel (in RAW/DVP mode). I
> > assume it is, do you have any information about this ?
> 
> I have to admit I had missed the mode management of the RAW mode while 
> going through all this. I had mostly looked at the UB953's CSI mode.
> 
> I don't have more information, but your analysis looks correct to me. So 
> the whole mode thing is an interesting mix of serializer & deserializer 
> HW straps, deserializer sending the (RAW) mode to the serializer, and 
> then the override registers on the serializer side.
> 
> As to the original question, should we have mandatory bus-width for 
> ub913... I don't think it would be useful, even after the updated 
> understanding about modes. Do you agree?

Yes, as far as I understand, the mode is configured from the
deserializer side for parallel serializers. While it can be overridden
on the serializer side through I2C, this would be an exception rather
than a rule, so I don't think there's a need to make bus-width mandatory
in DT.

> >> So the bus-width is not something that the driver would normally use. If
> >> we would need to define the bus-width and HF/LF in the DT for some
> >> reason in the future, I think an "old" DT without those specified should
> >> continue working fine, as the mode can be read from a register.
> >>
> >> That said, to complicate matters, the deserializer needs to know the
> >> serializer's mode before it can communicate with it (and thus, before we
> >> can read the mode). This is set with the deserializer's "ti,rx-mode"
> >> property, where you find RAW10, RAW12LF and RAW12HF modes (and for
> >> ub953, CSI-2 sync and non-sync modes).
> >>
> >> So if we would define the bus-width and HF/LF in ub913's properties, the
> >> deserializer could go peeking the mode from there. But is that a good
> >> idea... I'm not so sure.
> > 
> > Peeking into another device's DT node isn't great. It looks like the
> > best option for the DS90UB913 is to specify the mode on the
> > deserializer's side (either through the MODE strap or with a software
> > override through DT). In case the serializer mode would need to be
> > manually overridden in the future, we could add an optional DT property.
> 
> I don't like the mode strap on UB960 side, as it's for all ports. It 
> works in certain cases, of course, but if we anyway need the mode in DT 
> to allow port-specific configuration, I think it's just easier to always 
> require the DT mode and always use that, overriding the strap mode.

I wonder if there could be systems where the strap could be configurable
through a DIP switch or a jumper, depending on what cameras are
connected. It could then be convenient to not have to modify the device
tree. And just as I write this, I realize we have to specify the cameras
in DT anyway, so I suppose we could as well specify the mode too.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
new file mode 100644
index 000000000000..3a5b34c6bb64
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
@@ -0,0 +1,121 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub913.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments DS90UB913 FPD-Link 3 Serializer
+
+maintainers:
+  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
+
+description:
+  The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video.
+
+properties:
+  compatible:
+    enum:
+      - ti,ds90ub913a-q1
+
+  '#gpio-cells':
+    const: 2
+
+  gpio-controller: true
+
+  clocks:
+    maxItems: 1
+    description:
+      Reference clock connected to the CLKIN pin.
+
+  clock-names:
+    items:
+      - const: clkin
+
+  '#clock-cells':
+    const: 0
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description: CSI-2 input port
+
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        unevaluatedProperties: false
+        description: FPD-Link 3 output port
+
+  i2c:
+    $ref: /schemas/i2c/i2c-controller.yaml#
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - '#gpio-cells'
+  - gpio-controller
+  - '#clock-cells'
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    serializer {
+      compatible = "ti,ds90ub913a-q1";
+
+      gpio-controller;
+      #gpio-cells = <2>;
+
+      clocks = <&clk_cam_48M>;
+      clock-names = "clkin";
+
+      #clock-cells = <0>;
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+          ub913_in: endpoint {
+            remote-endpoint = <&sensor_out>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+          endpoint {
+            remote-endpoint = <&deser_fpd_in>;
+          };
+        };
+      };
+
+      i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@48 {
+          compatible = "aptina,mt9v111";
+          reg = <0x48>;
+
+          clocks = <&fixed_clock>;
+
+          port {
+            sensor_out: endpoint {
+              remote-endpoint = <&ub913_in>;
+            };
+          };
+        };
+      };
+    };
+...