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@xxxxxxxxxxxxxxxx> > >>>>> --- > >>>>> .../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@xxxxxxxxxxxxxxxx> > >>>>> + > >>>>> +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. -- Regards, Laurent Pinchart
