| Message ID | 20190723203723.11730-4-luca@lucaceresoli.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | TI camera serdes and I2C address translation | expand |
On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote: > Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer > with I2C Address Translator and remote GPIOs. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > --- > > Changes RFCv1 -> RFCv2: > > - add explicit aliases for the FPD-link RX ports (optional) > - add proper remote GPIO description > --- > .../bindings/media/i2c/ti,ds90ub954-q1.txt | 194 ++++++++++++++++++ > 1 file changed, 194 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt > > diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt > new file mode 100644 > index 000000000000..73ce21ecc3b6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt > @@ -0,0 +1,194 @@ > +Texas Instruments DS90UB954-Q1 dual video deserializer > +====================================================== > + > +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video > +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It > +also allows access to remote I2C and GPIO. > + > +Required properties: > + > + - compatible: must be "ti,ds90ub954-q1" > + > + - reg: main I2C slave address; optionally aliases for RX port registers > + and remote serializers. The main address is mandatory and must be the > + first, others are optional and fall back to defaults if not > + specified. See "reg-names". > + > + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip) > + - i2c-alias-pool: list of I2C addresses that are known to be available on the > + "local" (SoC-to-deser) I2C bus; they will be picked at > + runtime and used as aliases to reach remove I2C chips s/remove/remote/ Needs a vendor prefix. > + - gpio-controller > + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags We're pretty standardized on 2 cells for GPIO. Perhaps combine the port and gpio number to 1 cell. > + > +Optional properties: > + > + - reg-names: names of I2C address used to communicate with the chip, must > + match the "reg" values; mandatory if there are 2 or more > + addresses > + - "main": the main I2C address, used to access shared registers > + - "rxport0", "rxport1": I2C alias to access FPD-link RX port specific > + registers; must not be used by other slaves on the same bus > + - "ser0", "ser1": I2C alias to access the remote serializer connected > + on each FPD-link RX port; must not be used by other slaves on the > + same bus > + - interrupts: interrupt pin from the chip > + > +Required subnodes: > + > + - ports: A ports node with one port child node per device input and output > + port, in accordance with the video interface bindings defined in > + Documentation/devicetree/bindings/media/video-interfaces.txt. The > + port nodes are numbered as follows: > + > + Port Description > + ------------------------------------ > + 0 Input from FPD-Link 3 RX port 0 > + 1 Input from FPD-Link 3 RX port 1 > + 2 CSI-2 output > + > + Each port must have a "remote-chip" subnode that defines the remote > + chip (serializer) with at least a "compatible" property We don't allow other nodes within graph nodes. I'm not really clear what you are trying to do here. > + > + - i2c-atr: contains one child per RX port, each describes the I2C bus on > + the remote side > + > + Required properties: > + - #address-cells = <1>; > + - #size-cells = <0>; > + > + Subnodes: one per each FPD-link RX port, each having: > + > + Required properties for "i2c-atr" child bus nodes: > + - reg: The number of the port where the remove chip is connected s/remove/remote/ > + - #address-cells = <1>; > + - #size-cells = <0>; > + > + Optional properties for "i2c-atr" child bus nodes: > + - Other properties specific to the remote hardware Such as? > + - Child nodes conforming to i2c bus binding > + > + > +Device node example > +------------------- > + > +&i2c0 { > + deser: deser@3d { > + compatible = "ti,ds90ub954-q1"; > + reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; > + reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; > + clocks = <&clk_25M>; > + interrupt-parent = <&gic>; > + interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; > + reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; > + > + i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; > + > + gpio-controller; > + #gpio-cells = <3>; /* rxport, remote gpio num, flags */ > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + ds90ub954_fpd3_in0: endpoint { > + remote-endpoint = <&sensor_0_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + ds90ub954_fpd3_in1: endpoint { > + remote-endpoint = <&sensor_1_out>; > + }; > + }; > + > + port@2 { > + reg = <2>; > + ds90ub954_mipi_out0: endpoint { > + data-lanes = <1 2 3 4>; > + /* Actually a REFCLK multiplier */ > + data-rate = <1600000000>; > + remote-endpoint = <&csirx_0_in>; > + }; > + }; > + }; > + > + i2c-atr { > + #address-cells = <1>; > + #size-cells = <0>; > + > + remote_i2c0: i2c@0 { > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; Presumably, there are child I2C devices here. Please show that in the example. > + }; > + > + remote_i2c1: i2c@1 { > + reg = <1>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + }; > + }; > +}; > + > +&ds90ub954_fpd3_in0 { > + remote-chip { > + compatible = "ti,ds90ub953-q1"; > + gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE Not documented. > + DS90_GPIO_FUNC_UNUSED > + DS90_GPIO_FUNC_UNUSED > + DS90_GPIO_FUNC_UNUSED>; > + }; > +}; > + > +&ds90ub954_fpd3_in1 { > + remote-chip { > + compatible = "ti,ds90ub953-q1"; > + gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE > + DS90_GPIO_FUNC_UNUSED > + DS90_GPIO_FUNC_UNUSED > + DS90_GPIO_FUNC_UNUSED>; > + }; > +}; > + > +&remote_i2c0 { > + sensor_0@3c { > + compatible = "sony,imx274"; > + reg = <0x3c>; > + > + reset-gpios = <&deser 0 0 GPIO_ACTIVE_LOW>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + sensor_0_out: endpoint { > + remote-endpoint = <&ds90ub954_fpd3_in0>; > + }; > + }; > + }; > +}; > + > +&remote_i2c1 { > + sensor_0@3c { > + compatible = "sony,imx274"; > + reg = <0x3c>; > + > + reset-gpios = <&deser 1 0 GPIO_ACTIVE_LOW>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + sensor_1_out: endpoint { > + remote-endpoint = <&ds90ub954_fpd3_in1>; > + }; > + }; > + }; > +}; > -- > 2.17.1 >
Hi Rob, thanks for your review. On 13/08/19 17:44, Rob Herring wrote: > On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote: >> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer >> with I2C Address Translator and remote GPIOs. >> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> >> --- >> >> Changes RFCv1 -> RFCv2: >> >> - add explicit aliases for the FPD-link RX ports (optional) >> - add proper remote GPIO description >> --- >> .../bindings/media/i2c/ti,ds90ub954-q1.txt | 194 ++++++++++++++++++ >> 1 file changed, 194 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt >> new file mode 100644 >> index 000000000000..73ce21ecc3b6 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt >> @@ -0,0 +1,194 @@ >> +Texas Instruments DS90UB954-Q1 dual video deserializer >> +====================================================== >> + >> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video >> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It >> +also allows access to remote I2C and GPIO. >> + >> +Required properties: >> + >> + - compatible: must be "ti,ds90ub954-q1" >> + >> + - reg: main I2C slave address; optionally aliases for RX port registers >> + and remote serializers. The main address is mandatory and must be the >> + first, others are optional and fall back to defaults if not >> + specified. See "reg-names". >> + >> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip) >> + - i2c-alias-pool: list of I2C addresses that are known to be available on the >> + "local" (SoC-to-deser) I2C bus; they will be picked at >> + runtime and used as aliases to reach remove I2C chips > > s/remove/remote/ Will fix. > Needs a vendor prefix. The ultimate goal here is to define a standard property that all chips able to to I2C forwarding (video serdes or, potentially, other chips) can use. That's why the GMSL (Maxim deser) developers are in Cc: they are also facing a similar need. However I'm OK to change this to "ti,i2c-alias-pool" just in case there are reasons to not use a common name [yet]. However, following this argument, shouldn't a prefix be needed also for other nonstandard strings, such as "i2c-atr" below? >> + - gpio-controller >> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags > > We're pretty standardized on 2 cells for GPIO. Perhaps combine the port > and gpio number to 1 cell. Oh dear. I dislike implementing software that does not model the physical reality. I know it will bite me back sooner or later. Here we _really_ have N physically separate GPIO controllers, and the number of GPIOs they have depends on the model of the chip that is connected remotely. This is how things look in the case of 2 ports: <-- base board --> <------- remote camera module 1 -----> .---------------------. .-----. .------. | SER 1 | | CPU |----|port 1|========|----------. | `-----' | | FPD | GPIO ctl | | | | Link 3 `---------------------' | | cable |||| | DES | remote GPIO pins | | | | <------- remote camera module 2 -----> | | .---------------------. |port 2| | SER 2 | | |========|----------. | `------' FPD | GPIO ctl | | Link 3 `---------------------' cable |||||||| remote GPIO pins Perhaps we should have N separate gpiochips, one per port? Under which node? Not under "ports" ("We don't allow other nodes within graph nodes"), and "i2c-atr" does not seem like a suitable name. Under a new "remote-gpiochips" node? >> +Required subnodes: >> + >> + - ports: A ports node with one port child node per device input and output >> + port, in accordance with the video interface bindings defined in >> + Documentation/devicetree/bindings/media/video-interfaces.txt. The >> + port nodes are numbered as follows: >> + >> + Port Description >> + ------------------------------------ >> + 0 Input from FPD-Link 3 RX port 0 >> + 1 Input from FPD-Link 3 RX port 1 >> + 2 CSI-2 output >> + >> + Each port must have a "remote-chip" subnode that defines the remote >> + chip (serializer) with at least a "compatible" property > > We don't allow other nodes within graph nodes. I'm not really clear what > you are trying to do here. Each of the deser ports (2 ports in this chip) creates a physical point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL" in the Maxim chips. One "remote chip" serializer can be connected to each bus. The remote chip has, at least, a model (e.g. DS90UB953) and some properties. So I need a place where model and properties can be described. The port node looked like a good place, but as you point out it is not. Adding to the above discussion about 3 gpio-cells, I can think of a different, tentative DT layout: deser: deser@3d { compatible = "ti,ds90ub954-q1"; reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; clocks = <&clk_25M>; interrupt-parent = <&gic>; interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; endpoint { remote-endpoint = <&sensor_0_out>; }; }; port@1 { reg = <1>; endpoint { remote-endpoint = <&sensor_1_out>; }; }; port@2 { reg = <2>; endpoint { data-lanes = <1 2 3 4>; /* Actually a REFCLK multiplier */ data-rate = <1600000000>; remote-endpoint = <&csirx_0_in>; }; }; }; remote-chips { #address-cells = <1>; #size-cells = <0>; remote-chip@0 { reg = <0>; chip { compatible = "ti,ds90ub953-q1"; gpio-functions = <...>; }; remote_i2c0: i2c@0 { reg = <0>; #address-cells = <1>; #size-cells = <0>; }; gpio-controller; /* 2 cells for _each_ gpiochip */ #gpio-cells = <2>; /* remote gpio num, flags */ }; remote-chip@1 { reg = <1>; /* similar to remote chip 0 */ }; }; }; Here there are two subnodes: - "remote-chips" with a subnode per remote port, each describing the remote chip model and properties, remote i2c bus, gpio controller and others (other chips have I2S, SPI, UART...) - "ports" with only the standard v4l2 port properties, no subnodes under the endpoint nodes Does this look better from the device tree point of view? >> + - i2c-atr: contains one child per RX port, each describes the I2C bus on >> + the remote side >> + >> + Required properties: >> + - #address-cells = <1>; >> + - #size-cells = <0>; >> + >> + Subnodes: one per each FPD-link RX port, each having: >> + >> + Required properties for "i2c-atr" child bus nodes: >> + - reg: The number of the port where the remove chip is connected > > s/remove/remote/ Ok. >> + - #address-cells = <1>; >> + - #size-cells = <0>; >> + >> + Optional properties for "i2c-atr" child bus nodes: >> + - Other properties specific to the remote hardware > > Such as? "clock-frequency" at least. The remote chip is an I2C master, thus any property that applies to an I2C master might apply as well. "clock-frequency" is only half-implemented in these RFC patches: * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK value of the deser to the remote chip (serializer): this value is the physical reference clock the remote chip receives, all of its timings are based on it * the remote chip, given refclk, computes the register values that best approximate "clock-frequency" [not implemented in this version, would be in patch 6] I plan to implement and document the whole clock-frequency feature for the next patch iteration. But I'd love to receive comments about how reflck is passed from the deser to the serializer via platform_data. >> +Device node example >> +------------------- >> + >> +&i2c0 { >> + deser: deser@3d { >> + compatible = "ti,ds90ub954-q1"; >> + reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; >> + reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; >> + clocks = <&clk_25M>; >> + interrupt-parent = <&gic>; >> + interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; >> + reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; >> + >> + i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; >> + >> + gpio-controller; >> + #gpio-cells = <3>; /* rxport, remote gpio num, flags */ >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + ds90ub954_fpd3_in0: endpoint { >> + remote-endpoint = <&sensor_0_out>; >> + }; >> + }; >> + >> + port@1 { >> + reg = <1>; >> + ds90ub954_fpd3_in1: endpoint { >> + remote-endpoint = <&sensor_1_out>; >> + }; >> + }; >> + >> + port@2 { >> + reg = <2>; >> + ds90ub954_mipi_out0: endpoint { >> + data-lanes = <1 2 3 4>; >> + /* Actually a REFCLK multiplier */ >> + data-rate = <1600000000>; >> + remote-endpoint = <&csirx_0_in>; >> + }; >> + }; >> + }; >> + >> + i2c-atr { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + remote_i2c0: i2c@0 { >> + reg = <0>; >> + #address-cells = <1>; >> + #size-cells = <0>; > > Presumably, there are child I2C devices here. Please show that in the > example. They are shown below to avoid excessive nesting, look for "&remote_i2c0" (1). >> +&ds90ub954_fpd3_in0 { >> + remote-chip { >> + compatible = "ti,ds90ub953-q1"; >> + gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE > > Not documented. That's because this node describes the remote chip. The remote chip is a ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to showing some child I2C devices under the I2C master, as you suggested just above, except it's not an I2C bus but an FPD-Link 3 "bus". Is it OK if I replace the whole gpio-functions node with the comment "/* properties specific to the ti,ds90ub953-q1 chip */"? >> +&remote_i2c0 { >> + sensor_0@3c { >> + compatible = "sony,imx274"; >> + reg = <0x3c>; (1) here are the child I2C device mentioned above
On Mon, Aug 19, 2019 at 5:41 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Rob, > > thanks for your review. > > On 13/08/19 17:44, Rob Herring wrote: > > On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote: > >> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer > >> with I2C Address Translator and remote GPIOs. > >> > >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > >> > >> --- > >> > >> Changes RFCv1 -> RFCv2: > >> > >> - add explicit aliases for the FPD-link RX ports (optional) > >> - add proper remote GPIO description > >> --- > >> .../bindings/media/i2c/ti,ds90ub954-q1.txt | 194 ++++++++++++++++++ > >> 1 file changed, 194 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt > >> > >> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt > >> new file mode 100644 > >> index 000000000000..73ce21ecc3b6 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt > >> @@ -0,0 +1,194 @@ > >> +Texas Instruments DS90UB954-Q1 dual video deserializer > >> +====================================================== > >> + > >> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video > >> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It > >> +also allows access to remote I2C and GPIO. > >> + > >> +Required properties: > >> + > >> + - compatible: must be "ti,ds90ub954-q1" > >> + > >> + - reg: main I2C slave address; optionally aliases for RX port registers > >> + and remote serializers. The main address is mandatory and must be the > >> + first, others are optional and fall back to defaults if not > >> + specified. See "reg-names". > >> + > >> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip) > >> + - i2c-alias-pool: list of I2C addresses that are known to be available on the > >> + "local" (SoC-to-deser) I2C bus; they will be picked at > >> + runtime and used as aliases to reach remove I2C chips > > > > s/remove/remote/ > > Will fix. > > > Needs a vendor prefix. > > The ultimate goal here is to define a standard property that all chips > able to to I2C forwarding (video serdes or, potentially, other chips) > can use. That's why the GMSL (Maxim deser) developers are in Cc: they > are also facing a similar need. > > However I'm OK to change this to "ti,i2c-alias-pool" just in case there > are reasons to not use a common name [yet]. However, following this > argument, shouldn't a prefix be needed also for other nonstandard > strings, such as "i2c-atr" below? Okay, no vendor prefix is fine. 'i2c-atr' is a node name, not a property so no vendor prefix. > > >> + - gpio-controller > >> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags > > > > We're pretty standardized on 2 cells for GPIO. Perhaps combine the port > > and gpio number to 1 cell. > > Oh dear. I dislike implementing software that does not model the > physical reality. I know it will bite me back sooner or later. Here we > _really_ have N physically separate GPIO controllers, and the number of > GPIOs they have depends on the model of the chip that is connected remotely. > > This is how things look in the case of 2 ports: > > <-- base board --> <------- remote camera module 1 -----> > .---------------------. > .-----. .------. | SER 1 | > | CPU |----|port 1|========|----------. | > `-----' | | FPD | GPIO ctl | | > | | Link 3 `---------------------' > | | cable |||| > | DES | remote GPIO pins > | | > | | <------- remote camera module 2 -----> > | | .---------------------. > |port 2| | SER 2 | > | |========|----------. | > `------' FPD | GPIO ctl | | > Link 3 `---------------------' > cable |||||||| > remote GPIO pins > > Perhaps we should have N separate gpiochips, one per port? Yes, seems like it. > Under which > node? Not under "ports" ("We don't allow other nodes within graph > nodes"), and "i2c-atr" does not seem like a suitable name. Under a new > "remote-gpiochips" node? > > >> +Required subnodes: > >> + > >> + - ports: A ports node with one port child node per device input and output > >> + port, in accordance with the video interface bindings defined in > >> + Documentation/devicetree/bindings/media/video-interfaces.txt. The > >> + port nodes are numbered as follows: > >> + > >> + Port Description > >> + ------------------------------------ > >> + 0 Input from FPD-Link 3 RX port 0 > >> + 1 Input from FPD-Link 3 RX port 1 > >> + 2 CSI-2 output > >> + > >> + Each port must have a "remote-chip" subnode that defines the remote > >> + chip (serializer) with at least a "compatible" property > > > > We don't allow other nodes within graph nodes. I'm not really clear what > > you are trying to do here. > > Each of the deser ports (2 ports in this chip) creates a physical > point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL" > in the Maxim chips. One "remote chip" serializer can be connected to > each bus. The remote chip has, at least, a model (e.g. DS90UB953) and > some properties. So I need a place where model and properties can be > described. The port node looked like a good place, but as you point out > it is not. > > Adding to the above discussion about 3 gpio-cells, I can think of a > different, tentative DT layout: > > deser: deser@3d { > compatible = "ti,ds90ub954-q1"; > reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; > reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; > clocks = <&clk_25M>; > interrupt-parent = <&gic>; > interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; > reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; > > i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > endpoint { > remote-endpoint = <&sensor_0_out>; > }; > }; > > port@1 { > reg = <1>; > endpoint { > remote-endpoint = <&sensor_1_out>; > }; > }; > > port@2 { > reg = <2>; > endpoint { > data-lanes = <1 2 3 4>; > /* Actually a REFCLK multiplier */ > data-rate = <1600000000>; > remote-endpoint = <&csirx_0_in>; > }; > }; > }; > > remote-chips { I don't have a better suggestion for the location of this. > #address-cells = <1>; > #size-cells = <0>; > > remote-chip@0 { > reg = <0>; > chip { > compatible = "ti,ds90ub953-q1"; Seems like this should be in the parent? It's the ds90ub953 implementing the I2C bus, GPIO controller, etc.? > gpio-functions = <...>; > }; > > remote_i2c0: i2c@0 { What does '0' mean here? At a given level, you can only have 1 number space of addresses. > reg = <0>; > #address-cells = <1>; > #size-cells = <0>; > }; > > gpio-controller; > /* 2 cells for _each_ gpiochip */ > #gpio-cells = <2>; /* remote gpio num, flags */ > }; > > remote-chip@1 { > reg = <1>; > /* similar to remote chip 0 */ > }; > }; > }; > > Here there are two subnodes: > > - "remote-chips" with a subnode per remote port, each describing > the remote chip model and properties, remote i2c bus, gpio controller > and others (other chips have I2S, SPI, UART...) > > - "ports" with only the standard v4l2 port properties, no subnodes > under the endpoint nodes > > Does this look better from the device tree point of view? Yes. > >> + - #address-cells = <1>; > >> + - #size-cells = <0>; > >> + > >> + Optional properties for "i2c-atr" child bus nodes: > >> + - Other properties specific to the remote hardware > > > > Such as? > > "clock-frequency" at least. The remote chip is an I2C master, thus any > property that applies to an I2C master might apply as well. > > "clock-frequency" is only half-implemented in these RFC patches: > > * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK > value of the deser to the remote chip (serializer): this value is > the physical reference clock the remote chip receives, all of its > timings are based on it > > * the remote chip, given refclk, computes the register values that best > approximate "clock-frequency" [not implemented in this version, would > be in patch 6] > > I plan to implement and document the whole clock-frequency feature for > the next patch iteration. But I'd love to receive comments about how > reflck is passed from the deser to the serializer via platform_data. Okay. Please try to make bindings as complete as possible even if there's not yet driver support. > >> + i2c-atr { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + remote_i2c0: i2c@0 { > >> + reg = <0>; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > > > > Presumably, there are child I2C devices here. Please show that in the > > example. > > They are shown below to avoid excessive nesting, look for "&remote_i2c0" > (1). I'd rather have longish lines than have things split up. > > >> +&ds90ub954_fpd3_in0 { > >> + remote-chip { > >> + compatible = "ti,ds90ub953-q1"; > >> + gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE > > > > Not documented. > > That's because this node describes the remote chip. The remote chip is a > ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to > showing some child I2C devices under the I2C master, as you suggested > just above, except it's not an I2C bus but an FPD-Link 3 "bus". I think I realized this after sending this... > > Is it OK if I replace the whole gpio-functions node with the comment > "/* properties specific to the ti,ds90ub953-q1 chip */"? Not necessary. gpio-functions does need a vendor prefix though. Rob
Hi Rob, On 20/08/19 17:44, Rob Herring wrote: >>>> + - i2c-alias-pool: list of I2C addresses that are known to be available on the >>>> + "local" (SoC-to-deser) I2C bus; they will be picked at >>>> + runtime and used as aliases to reach remove I2C chips >>> >>> s/remove/remote/ >> >> Will fix. >> >>> Needs a vendor prefix. >> >> The ultimate goal here is to define a standard property that all chips >> able to to I2C forwarding (video serdes or, potentially, other chips) >> can use. That's why the GMSL (Maxim deser) developers are in Cc: they >> are also facing a similar need. >> >> However I'm OK to change this to "ti,i2c-alias-pool" just in case there >> are reasons to not use a common name [yet]. However, following this >> argument, shouldn't a prefix be needed also for other nonstandard >> strings, such as "i2c-atr" below? > > Okay, no vendor prefix is fine. > > 'i2c-atr' is a node name, not a property so no vendor prefix. I see, thanks. >>>> + - gpio-controller >>>> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags >>> >>> We're pretty standardized on 2 cells for GPIO. Perhaps combine the port >>> and gpio number to 1 cell. >> >> Oh dear. I dislike implementing software that does not model the >> physical reality. I know it will bite me back sooner or later. Here we >> _really_ have N physically separate GPIO controllers, and the number of >> GPIOs they have depends on the model of the chip that is connected remotely. >> >> This is how things look in the case of 2 ports: >> >> <-- base board --> <------- remote camera module 1 -----> >> .---------------------. >> .-----. .------. | SER 1 | >> | CPU |----|port 1|========|----------. | >> `-----' | | FPD | GPIO ctl | | >> | | Link 3 `---------------------' >> | | cable |||| >> | DES | remote GPIO pins >> | | >> | | <------- remote camera module 2 -----> >> | | .---------------------. >> |port 2| | SER 2 | >> | |========|----------. | >> `------' FPD | GPIO ctl | | >> Link 3 `---------------------' >> cable |||||||| >> remote GPIO pins >> >> Perhaps we should have N separate gpiochips, one per port? > > Yes, seems like it. I'm more and more convinced of that. >>>> +Required subnodes: >>>> + >>>> + - ports: A ports node with one port child node per device input and output >>>> + port, in accordance with the video interface bindings defined in >>>> + Documentation/devicetree/bindings/media/video-interfaces.txt. The >>>> + port nodes are numbered as follows: >>>> + >>>> + Port Description >>>> + ------------------------------------ >>>> + 0 Input from FPD-Link 3 RX port 0 >>>> + 1 Input from FPD-Link 3 RX port 1 >>>> + 2 CSI-2 output >>>> + >>>> + Each port must have a "remote-chip" subnode that defines the remote >>>> + chip (serializer) with at least a "compatible" property >>> >>> We don't allow other nodes within graph nodes. I'm not really clear what >>> you are trying to do here. >> >> Each of the deser ports (2 ports in this chip) creates a physical >> point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL" >> in the Maxim chips. One "remote chip" serializer can be connected to >> each bus. The remote chip has, at least, a model (e.g. DS90UB953) and >> some properties. So I need a place where model and properties can be >> described. The port node looked like a good place, but as you point out >> it is not. >> >> Adding to the above discussion about 3 gpio-cells, I can think of a >> different, tentative DT layout: >> >> deser: deser@3d { >> compatible = "ti,ds90ub954-q1"; >> reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; >> reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; >> clocks = <&clk_25M>; >> interrupt-parent = <&gic>; >> interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; >> reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; >> >> i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; >> >> ports { >> #address-cells = <1>; >> #size-cells = <0>; >> >> port@0 { >> reg = <0>; >> endpoint { >> remote-endpoint = <&sensor_0_out>; >> }; >> }; >> >> port@1 { >> reg = <1>; >> endpoint { >> remote-endpoint = <&sensor_1_out>; >> }; >> }; >> >> port@2 { >> reg = <2>; >> endpoint { >> data-lanes = <1 2 3 4>; >> /* Actually a REFCLK multiplier */ >> data-rate = <1600000000>; >> remote-endpoint = <&csirx_0_in>; >> }; >> }; >> }; >> >> remote-chips { > > I don't have a better suggestion for the location of this. > >> #address-cells = <1>; >> #size-cells = <0>; >> >> remote-chip@0 { >> reg = <0>; >> chip { >> compatible = "ti,ds90ub953-q1"; > > Seems like this should be in the parent? It's the ds90ub953 > implementing the I2C bus, GPIO controller, etc.? You're right: it's the ds90ub953 implementing i2c, gpio etc. The (undoubtedly weird) DT structure originates from the hotplugging issue that I explained in the cover letter. The compromise I found to handle it is to describe the remote I2C adapters and GPIO controllers as if they were part of the "local" domain (the ds90ub954). This is why they are represented in DT outside the ds90ub953 node: the "chip" node would be added by an overlay, the i2c and gpio nodes are always there. But this corresponds to how I implemented it in the code. Let's see if at least DT can have a better layout: deser: deser@3d { compatible = "ti,ds90ub954-q1"; reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; clocks = <&clk_25M>; interrupt-parent = <&gic>; interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; endpoint { remote-endpoint = <&sensor_0_out>; }; }; port@1 { reg = <1>; endpoint { remote-endpoint = <&sensor_1_out>; }; }; port@2 { reg = <2>; endpoint { data-lanes = <1 2 3 4>; /* Actually a REFCLK multiplier */ data-rate = <1600000000>; remote-endpoint = <&csirx_0_in>; }; }; }; remote-chips { #address-cells = <1>; #size-cells = <0>; remote-chip@0 { reg = <0>; /* dynamic */ compatible = "ti,ds90ub953-q1"; gpio-functions = <...>; remote_i2c0: i2c { reg = <0>; #address-cells = <1>; #size-cells = <0>; }; gpio-controller; /* 2 cells for _each_ gpiochip: */ /* remote gpio num, flags */ #gpio-cells = <2>; }; remote-chip@1 { reg = <1>; /* similar to remote chip 0 */ }; }; }; Now things _look_ correct: I just removed the "chip" subnode and hoisted its properties up, where the i2c and gpio nodes are. However this would not correspond a currently doable implementation. The gpio- and i2c-related nodes under remote-chip@<N> would be always instantiated in the base board DT, while the "compatible" property would be added/removed by overlays on hotplug. This would require the deser chip driver to probe the remote serializer chip not when the device node appears but when the compatible property appears inside it. This would look weird on the device driver side, even though I understand very well that a DT needs to be future-proof much more than a driver. I'll have a more detailed look into this. Also, in case this were not clear, no hotplugging is actually implemented currently, mostly due to the lack of a complete implementation of runtime DT overlay insertion and removal. I think work is in progress on that side, but it doesn't look like it is done yet. However I'm trying to be ready for it when it will be available. >> remote_i2c0: i2c@0 { > > What does '0' mean here? At a given level, you can only have 1 number > space of addresses. Copy-paste leftover, will fix. >>>> + - #address-cells = <1>; >>>> + - #size-cells = <0>; >>>> + >>>> + Optional properties for "i2c-atr" child bus nodes: >>>> + - Other properties specific to the remote hardware >>> >>> Such as? >> >> "clock-frequency" at least. The remote chip is an I2C master, thus any >> property that applies to an I2C master might apply as well. >> >> "clock-frequency" is only half-implemented in these RFC patches: >> >> * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK >> value of the deser to the remote chip (serializer): this value is >> the physical reference clock the remote chip receives, all of its >> timings are based on it >> >> * the remote chip, given refclk, computes the register values that best >> approximate "clock-frequency" [not implemented in this version, would >> be in patch 6] >> >> I plan to implement and document the whole clock-frequency feature for >> the next patch iteration. But I'd love to receive comments about how >> reflck is passed from the deser to the serializer via platform_data. > > Okay. Please try to make bindings as complete as possible even if > there's not yet driver support. > >>>> + i2c-atr { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + remote_i2c0: i2c@0 { >>>> + reg = <0>; >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>> >>> Presumably, there are child I2C devices here. Please show that in the >>> example. >> >> They are shown below to avoid excessive nesting, look for "&remote_i2c0" >> (1). > > I'd rather have longish lines than have things split up. Ok, but do we have 80-chars limit in the bindings too? >>>> +&ds90ub954_fpd3_in0 { >>>> + remote-chip { >>>> + compatible = "ti,ds90ub953-q1"; >>>> + gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE >>> >>> Not documented. >> >> That's because this node describes the remote chip. The remote chip is a >> ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to >> showing some child I2C devices under the I2C master, as you suggested >> just above, except it's not an I2C bus but an FPD-Link 3 "bus". > > I think I realized this after sending this... > >> >> Is it OK if I replace the whole gpio-functions node with the comment >> "/* properties specific to the ti,ds90ub953-q1 chip */"? > > Not necessary. gpio-functions does need a vendor prefix though. Fair enough. I think other models/brands of serdes chips will need something similar, but they could accept different values, wo I'll add a "ti," prefix.
> + - i2c-alias-pool: list of I2C addresses that are known to be available on the > + "local" (SoC-to-deser) I2C bus; they will be picked at > + runtime and used as aliases to reach remove I2C chips After some internal discussion, I have been kinda convinced that it may be better to assume all non-described addresses are free to use and enter the pool. The problem with the binding above is that you may run out of aliases depending on how many aliases one to-be-attached module needs or how many modules will be attached. And another add-on module with non-repogrammable devices may occupy addresses from the defined pool above. I am not perfectly happy with the assumption that all undescribed addresses are automatically free. That also might need DTS updates to describe all clients properly. But this change only needs to be done once, and it will improve the description of the hardware. So, I could agree this is the better option of the two.
Hi Wolfram, On 02/09/19 22:48, Wolfram Sang wrote: > >> + - i2c-alias-pool: list of I2C addresses that are known to be available on the >> + "local" (SoC-to-deser) I2C bus; they will be picked at >> + runtime and used as aliases to reach remove I2C chips > > After some internal discussion, I have been kinda convinced that it may > be better to assume all non-described addresses are free to use and > enter the pool. > > The problem with the binding above is that you may run out of aliases > depending on how many aliases one to-be-attached module needs or how > many modules will be attached. Not if you define enough addresses in the pool. E.g. the DS90UB954 hardware can have 8 aliases per port, so if you have (n_ports * 8) addresses in the pool the problem is solved. > And another add-on module with > non-repogrammable devices may occupy addresses from the defined pool > above. You mean a new device on the local (SoC-to-ATR) bus? Well, it could as well occupy a non-described address that the ATR has already picked as an alias. > I am not perfectly happy with the assumption that all undescribed > addresses are automatically free. That also might need DTS updates to > describe all clients properly. But this change only needs to be done > once, and it will improve the description of the hardware. Right, but I still suspect some users won't do their homework and discover address conflicts at runtime, maybe months later, in a painful way. Also a chip might be undocumented on a given board, so they could do their homework and still have problems. Despite my comments, I'm not strongly against your proposal. To me it doesn't seem to solve any problem, while it does introduce some degree of risk. Could you elaborate more on but what benefit it introduces? Thanks,
> Not if you define enough addresses in the pool. E.g. the DS90UB954 > hardware can have 8 aliases per port, so if you have (n_ports * 8) > addresses in the pool the problem is solved. And then you plug-in somewhere another board with another need for ATR and you are out of addresses. > > And another add-on module with > > non-repogrammable devices may occupy addresses from the defined pool > > above. > > You mean a new device on the local (SoC-to-ATR) bus? Well, it could as > well occupy a non-described address that the ATR has already picked as > an alias. Nope, I mean a seperate add-on which has a hardcoded I2C address on the bus of the ATR parent. Then this hardcoded address needs to be removed from the pool if it is in the wrong range. > > I am not perfectly happy with the assumption that all undescribed > > addresses are automatically free. That also might need DTS updates to > > describe all clients properly. But this change only needs to be done > > once, and it will improve the description of the hardware. > > Right, but I still suspect some users won't do their homework and > discover address conflicts at runtime, maybe months later, in a painful > way. Also a chip might be undocumented on a given board, so they could > do their homework and still have problems. Yes, we probably need a binding to mark an address as used even though we don't know the device or don't have a driver for it. Don't get me wrong, I know what you mean. One of my boards has a client soldered in a way so that it is still in debug mode. That means it listens to addresses 0x03-0x07 to provide debug information. Took me a while to find out what is happening there. But still, 'i2cdetect' showed all of these. > Despite my comments, I'm not strongly against your proposal. To me it > doesn't seem to solve any problem, while it does introduce some degree > of risk. Could you elaborate more on but what benefit it introduces? I'd think the risk of running out of defined addresses is somewhere equal to running into (after a while) an unexpectedly used address. I like the fix for the latter better because describing what is on the bus is more helpful and generic than updating the pool-property every time you need it. Plus, as mentioned above, other add-on hardware may disturb your pool allocation. I expect this topic to be one of the discussion points of the BoF.
Hi Wolfram, On 03/09/19 11:34, Wolfram Sang wrote: > >> Not if you define enough addresses in the pool. E.g. the DS90UB954 >> hardware can have 8 aliases per port, so if you have (n_ports * 8) >> addresses in the pool the problem is solved. > > And then you plug-in somewhere another board with another need for ATR > and you are out of addresses. Now I got what you mean: the aliases in a pool are reserved to an ATR and not available to another ATR. What about hoisting the pool one level up in DT, to the adapter where the ATRs are connected? >>> And another add-on module with >>> non-repogrammable devices may occupy addresses from the defined pool >>> above. >> >> You mean a new device on the local (SoC-to-ATR) bus? Well, it could as >> well occupy a non-described address that the ATR has already picked as >> an alias. > > Nope, I mean a seperate add-on which has a hardcoded I2C address on the > bus of the ATR parent. Then this hardcoded address needs to be removed > from the pool if it is in the wrong range. As I understand it, we are referring to the same use case: .---------------. .-----. .-------------------. | adapter (SoC) |---+---| ATR |---| remote slave 0x10 | '---------------' | '-----' '-------------------' | .----------. | device X | '----------' Here device X with hard-coded address 0x20 is plugged in at runtime. As you say it, if 0x20 is in the ATR pool we have a problem. But even without an explicit pool, initially 0x20 is unused and the ATR can use it for remote slave 0x10. Then at some point device X is connected, and suddenly 0x20 conflicts. To a limited extend the explicit pool could help if the list of possible addons is known: one can put in the pool only address that will never appear in addons. Hey, wait, the no-pool solution could have a way to handle this conflict. When device X is connected, the adapter can look for a new alias (0x21), call the i2c_atr_deconfigure_hw() and i2c_atr_setup_hw() callbacks to stop using 0x20 and start using 0x21. Doesn't look very lovely, but may be worth considering. >>> I am not perfectly happy with the assumption that all undescribed >>> addresses are automatically free. That also might need DTS updates to >>> describe all clients properly. But this change only needs to be done >>> once, and it will improve the description of the hardware. >> >> Right, but I still suspect some users won't do their homework and >> discover address conflicts at runtime, maybe months later, in a painful >> way. Also a chip might be undocumented on a given board, so they could >> do their homework and still have problems. > > Yes, we probably need a binding to mark an address as used even though > we don't know the device or don't have a driver for it. > > Don't get me wrong, I know what you mean. One of my boards has a client > soldered in a way so that it is still in debug mode. That means it > listens to addresses 0x03-0x07 to provide debug information. Took me a > while to find out what is happening there. > > But still, 'i2cdetect' showed all of these. > >> Despite my comments, I'm not strongly against your proposal. To me it >> doesn't seem to solve any problem, while it does introduce some degree >> of risk. Could you elaborate more on but what benefit it introduces? > > I'd think the risk of running out of defined addresses is somewhere > equal to running into (after a while) an unexpectedly used address. > I like the fix for the latter better because describing what is on the > bus is more helpful and generic than updating the pool-property every > time you need it. Plus, as mentioned above, other add-on hardware may > disturb your pool allocation. > > I expect this topic to be one of the discussion points of the BoF. Definitely. And having a list of use cases would help a lot IMO. I had never considered the use case described above, for example. Thanks,
Hi Luca, > Now I got what you mean: the aliases in a pool are reserved to an ATR > and not available to another ATR. What about hoisting the pool one level > up in DT, to the adapter where the ATRs are connected? Frankly, it sounds error prone to keep them in sync. > As I understand it, we are referring to the same use case: > > .---------------. .-----. .-------------------. > | adapter (SoC) |---+---| ATR |---| remote slave 0x10 | > '---------------' | '-----' '-------------------' > | > .----------. > | device X | > '----------' > > Here device X with hard-coded address 0x20 is plugged in at runtime. Yes. For the generic case, it doesn't even need to be plugged at runtime. A baseboard can have various additional modules with various requirements. Hot-plugging (not a favourite topic for I2C) adds to it. > As you say it, if 0x20 is in the ATR pool we have a problem. Right. > But even without an explicit pool, initially 0x20 is unused and the ATR > can use it for remote slave 0x10. Then at some point device X is > connected, and suddenly 0x20 conflicts. I haven't considered hot-plugging so far, but you are right here. > To a limited extend the explicit pool could help if the list of possible > addons is known: one can put in the pool only address that will never > appear in addons. And this is why I kinda accepted the all-unoccupied-addresses-are-free approach. For a generic solution, the list of possible add-ons is unknown and we should expect "the worst". > Hey, wait, the no-pool solution could have a way to handle this > conflict. When device X is connected, the adapter can look for a new > alias (0x21), call the i2c_atr_deconfigure_hw() and i2c_atr_setup_hw() > callbacks to stop using 0x20 and start using 0x21. Doesn't look very > lovely, but may be worth considering. Thanks for also thinking about the other approach! Everything combining I2C and hot-plugging is likely to not be lovely, so this may be OK given it really works in practice. Another note: I was hoping we could keep the number of callbacks to one. E.g. if i2c_atr_setup_hw() has the same values for 'address' and 'alias' (or one of them is zero), then we unregister an alias, otherwise we register. Not sure if this will work out in the end, but only one callback is what I'd like to have. (It could be put into the i2c_algorithm struct maybe) > Definitely. And having a list of use cases would help a lot IMO. I had > never considered the use case described above, for example. Yes. I hope to see some new faces in the room giving us their cases. Kind regards, Wolfram
Hi Luca, On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote: ... > +Device node example > +------------------- > + > +&i2c0 { > + deser: deser@3d { > + compatible = "ti,ds90ub954-q1"; > + reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; > + reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; > + clocks = <&clk_25M>; > + interrupt-parent = <&gic>; > + interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; > + reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; > + > + i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; > + > + gpio-controller; > + #gpio-cells = <3>; /* rxport, remote gpio num, flags */ > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + ds90ub954_fpd3_in0: endpoint { > + remote-endpoint = <&sensor_0_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + ds90ub954_fpd3_in1: endpoint { > + remote-endpoint = <&sensor_1_out>; > + }; > + }; > + > + port@2 { > + reg = <2>; > + ds90ub954_mipi_out0: endpoint { > + data-lanes = <1 2 3 4>; > + /* Actually a REFCLK multiplier */ > + data-rate = <1600000000>; What is data-rate used for? Is it documented somewhere? Could you use link-frequencies property instead? It's defined in video-interfaces.txt.
Hi Sakari, On 10/09/19 10:43, Sakari Ailus wrote: > Hi Luca, > > On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote: > > ... > >> +Device node example >> +------------------- >> + >> +&i2c0 { >> + deser: deser@3d { >> + compatible = "ti,ds90ub954-q1"; >> + reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; >> + reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; >> + clocks = <&clk_25M>; >> + interrupt-parent = <&gic>; >> + interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; >> + reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; >> + >> + i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; >> + >> + gpio-controller; >> + #gpio-cells = <3>; /* rxport, remote gpio num, flags */ >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + ds90ub954_fpd3_in0: endpoint { >> + remote-endpoint = <&sensor_0_out>; >> + }; >> + }; >> + >> + port@1 { >> + reg = <1>; >> + ds90ub954_fpd3_in1: endpoint { >> + remote-endpoint = <&sensor_1_out>; >> + }; >> + }; >> + >> + port@2 { >> + reg = <2>; >> + ds90ub954_mipi_out0: endpoint { >> + data-lanes = <1 2 3 4>; >> + /* Actually a REFCLK multiplier */ >> + data-rate = <1600000000>; > > What is data-rate used for? Is it documented somewhere? Could you use > link-frequencies property instead? It's defined in video-interfaces.txt. Right, it should be link-frequencies. Thanks for pointing out.
diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt new file mode 100644 index 000000000000..73ce21ecc3b6 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt @@ -0,0 +1,194 @@ +Texas Instruments DS90UB954-Q1 dual video deserializer +====================================================== + +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It +also allows access to remote I2C and GPIO. + +Required properties: + + - compatible: must be "ti,ds90ub954-q1" + + - reg: main I2C slave address; optionally aliases for RX port registers + and remote serializers. The main address is mandatory and must be the + first, others are optional and fall back to defaults if not + specified. See "reg-names". + + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip) + - i2c-alias-pool: list of I2C addresses that are known to be available on the + "local" (SoC-to-deser) I2C bus; they will be picked at + runtime and used as aliases to reach remove I2C chips + - gpio-controller + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags + +Optional properties: + + - reg-names: names of I2C address used to communicate with the chip, must + match the "reg" values; mandatory if there are 2 or more + addresses + - "main": the main I2C address, used to access shared registers + - "rxport0", "rxport1": I2C alias to access FPD-link RX port specific + registers; must not be used by other slaves on the same bus + - "ser0", "ser1": I2C alias to access the remote serializer connected + on each FPD-link RX port; must not be used by other slaves on the + same bus + - interrupts: interrupt pin from the chip + +Required subnodes: + + - ports: A ports node with one port child node per device input and output + port, in accordance with the video interface bindings defined in + Documentation/devicetree/bindings/media/video-interfaces.txt. The + port nodes are numbered as follows: + + Port Description + ------------------------------------ + 0 Input from FPD-Link 3 RX port 0 + 1 Input from FPD-Link 3 RX port 1 + 2 CSI-2 output + + Each port must have a "remote-chip" subnode that defines the remote + chip (serializer) with at least a "compatible" property + + - i2c-atr: contains one child per RX port, each describes the I2C bus on + the remote side + + Required properties: + - #address-cells = <1>; + - #size-cells = <0>; + + Subnodes: one per each FPD-link RX port, each having: + + Required properties for "i2c-atr" child bus nodes: + - reg: The number of the port where the remove chip is connected + - #address-cells = <1>; + - #size-cells = <0>; + + Optional properties for "i2c-atr" child bus nodes: + - Other properties specific to the remote hardware + - Child nodes conforming to i2c bus binding + + +Device node example +------------------- + +&i2c0 { + deser: deser@3d { + compatible = "ti,ds90ub954-q1"; + reg-names = "main", "rxport0", "rxport1", "ser0", "ser1"; + reg = <0x3d>, <0x40>, <0x41>, <0x44>, <0x45>; + clocks = <&clk_25M>; + interrupt-parent = <&gic>; + interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>; + reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>; + + i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>; + + gpio-controller; + #gpio-cells = <3>; /* rxport, remote gpio num, flags */ + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + ds90ub954_fpd3_in0: endpoint { + remote-endpoint = <&sensor_0_out>; + }; + }; + + port@1 { + reg = <1>; + ds90ub954_fpd3_in1: endpoint { + remote-endpoint = <&sensor_1_out>; + }; + }; + + port@2 { + reg = <2>; + ds90ub954_mipi_out0: endpoint { + data-lanes = <1 2 3 4>; + /* Actually a REFCLK multiplier */ + data-rate = <1600000000>; + remote-endpoint = <&csirx_0_in>; + }; + }; + }; + + i2c-atr { + #address-cells = <1>; + #size-cells = <0>; + + remote_i2c0: i2c@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + }; + + remote_i2c1: i2c@1 { + reg = <1>; + #address-cells = <1>; + #size-cells = <0>; + }; + }; + }; +}; + +&ds90ub954_fpd3_in0 { + remote-chip { + compatible = "ti,ds90ub953-q1"; + gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE + DS90_GPIO_FUNC_UNUSED + DS90_GPIO_FUNC_UNUSED + DS90_GPIO_FUNC_UNUSED>; + }; +}; + +&ds90ub954_fpd3_in1 { + remote-chip { + compatible = "ti,ds90ub953-q1"; + gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE + DS90_GPIO_FUNC_UNUSED + DS90_GPIO_FUNC_UNUSED + DS90_GPIO_FUNC_UNUSED>; + }; +}; + +&remote_i2c0 { + sensor_0@3c { + compatible = "sony,imx274"; + reg = <0x3c>; + + reset-gpios = <&deser 0 0 GPIO_ACTIVE_LOW>; + + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + sensor_0_out: endpoint { + remote-endpoint = <&ds90ub954_fpd3_in0>; + }; + }; + }; +}; + +&remote_i2c1 { + sensor_0@3c { + compatible = "sony,imx274"; + reg = <0x3c>; + + reset-gpios = <&deser 1 0 GPIO_ACTIVE_LOW>; + + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + sensor_1_out: endpoint { + remote-endpoint = <&ds90ub954_fpd3_in1>; + }; + }; + }; +};
Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer with I2C Address Translator and remote GPIOs. Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- Changes RFCv1 -> RFCv2: - add explicit aliases for the FPD-link RX ports (optional) - add proper remote GPIO description --- .../bindings/media/i2c/ti,ds90ub954-q1.txt | 194 ++++++++++++++++++ 1 file changed, 194 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt