Hi,
this is a second round of RFC patches to move forward on discussion about
proper kernel support for the TI DS90UB9xx serializer/deserializer chipsets
with I2C address translation.
RFCv2 is a major improvement over RFCv1, with several parts rewritten from
scratch. I2C address translationis now in its own file, there's decent
remote GPIO support, the deser driver is much closer to being complete, I
added a minimal serializer driver and, last but not least, forwarding one
video stream works.
My long-term goal is to be able to model different camera modules [or
display or other modules] similarly to beaglebone capes or rpi hats,
up to a model where:
1. there can be different camera modules being designed over time
2. there can be different base boards being designed over time
3. there is a standard interconnection between them (mechanical,
electrical, communication bus)
4. camera modules and base boards are designed and sold independently
(thanks to point 3)
5. a camera module can be removed, and a different model connected, at
runtime while the other modules are streaming
To allow the required flexibility I introduced the idea of an I2C alias
pool that must be defined in device tree. It is the responsibility of the
DT writer to fill the pool with addresses that are otherwise unused on the
local bus. The pool could not be filled automatically because there might
be conflicting chips on the local bus that are unknown to the software, or
that are just connected later.
Addresses from the pool are assigned to remote I2C slaves at runtime, when
they are added on the remote bus. The technical details of how address
translation works are documented in patch 2.
The big beast is hotplugging. Unfortunately this does not seem to be doable
"the right way" at the moment for at least two reasons. First, a v4l2 media
graph cannot be modified while the pipe is streaming, and AFAIK this will
not be possible soon. Second, because handling hotplug of devicetree-based
peripherals would require runtime DT overlay insertion and removal, which
is a slowly progressing work, but again not ready currently.
To overcome at least some of these limitations I found a compromise. The
model that I would consider "the correct one" looks like this:
<-- base board --> <------- remote camera module ------->
.---------------------.
.-----. .-----. | SER |
| CPU |----| DES |========|----------.----------|
`-----' `-----' FPD | GPIO ctl | I2C adap |----+----+----.
Link 3 `---------------------' | | |
cable |||| remote I2C slaves
remote GPIO pins
Here the deserializer (DES) is always present and connected, so it can be
probed vie DT during boot. The serializer (SER) is instantiated at runtime
when a link is established on the FPD-Link cable and the model
detected. An I2C adapter is created under the SER, and all the remote I2C
slaves are then instantiated under it.
But this would require to stop and modify the v4l2 pipe, including the
cameras still connected, just because one of them has been removed (or a
cable has gone faulty).
The comprimise I took looks like this:
.------------------. .-----.
.-----. | |========| SER |
| CPU |----| DES .----------| `-----'
`-----' | | I2C adap |----+----+----.
`------------------' | | |
remote I2C slaves
In this case the I2C adapter (representing the "remote" bus) is
instantiated under the DES, and is always present. This stil allows proper
hotplugging of the SER, and userspace can still add/remove remote I2C
slaves. But it makes it possible to instantiate a sensor and leave it
always instantiated, so that the v4l2 pipe is never modified and "believes"
the sensor is always there. Of course this opens other issues, and requires
non-standard wachanisms to start/stop the sensor and handle missing frames
when it is disconnected. My prototype design works thanks to the above
structure, a somewhat tweaked sensor driver and a bit of help from
userspace.
Finally, remote GPIOs.
.------------------. .-----.
.-----. | |========| SER |
| CPU |----| DES .----------| `-----'
`-----' | | GPIO ctl | ||||
`------------------' remote GPIO pins
Similar to the I2C adapter, I chose to instantiate on the DES the GPIO
controllers to control the remote GPIOs, even if it looks like it should be
under the serializers. This decision allows to have the remote GPIOs
described in DT, and always available, so that e.g. the always-instantiated
sensor driver DT node can say 'reset-gpios = <&deser 1 2 0>'.
That's all, see the patches for the details.
References:
[0] Vladimir Zapolskiy proposal on other TI chips:
https://www.spinics.net/lists/linux-gpio/msg33291.html
[1] Kieran Bingham's patches covering Maxim chips:
https://www.spinics.net/lists/linux-media/msg142367.html
[RFCv1] https://lore.kernel.org/linux-media/[email protected]/
Luca Ceresoli (6):
i2c: core: let adapters be notified of client attach/detach
i2c: add I2C Address Translator (ATR) support
media: dt-bindings: add DS90UB954-Q1 video deserializer
media: dt-bindings: add DS90UB953-Q1 video serializer
media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer
media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer
.../bindings/media/i2c/ti,ds90ub953-q1.txt | 42 +
.../bindings/media/i2c/ti,ds90ub954-q1.txt | 194 ++
drivers/i2c/Kconfig | 9 +
drivers/i2c/Makefile | 1 +
drivers/i2c/i2c-atr.c | 557 ++++++
drivers/i2c/i2c-core-base.c | 16 +
drivers/media/i2c/Kconfig | 24 +
drivers/media/i2c/Makefile | 3 +
drivers/media/i2c/ds90ub953.c | 354 ++++
drivers/media/i2c/ds90ub954.c | 1575 +++++++++++++++++
include/dt-bindings/media/ds90ub953.h | 16 +
include/linux/i2c-atr.h | 82 +
include/linux/i2c.h | 16 +
13 files changed, 2889 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.txt
create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
create mode 100644 drivers/i2c/i2c-atr.c
create mode 100644 drivers/media/i2c/ds90ub953.c
create mode 100644 drivers/media/i2c/ds90ub954.c
create mode 100644 include/dt-bindings/media/ds90ub953.h
create mode 100644 include/linux/i2c-atr.h
--
2.17.1
An adapter might need to know when a new device is about to be
added. This will soon bee needed to implement an "I2C address
translator" (ATR for short), a device that propagates I2C transactions
with a different slave address (an "alias" address). An ATR driver
needs to know when a slave is being added to find a suitable alias and
program the device translation map.
Add an attach/detach callback pair to allow adapter drivers to be
notified of clients being added and removed.
Signed-off-by: Luca Ceresoli <[email protected]>
---
Changes RFCv1 -> RFCv2:
- Document struct i2c_attach_operations
---
drivers/i2c/i2c-core-base.c | 16 ++++++++++++++++
include/linux/i2c.h | 16 ++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index f26ed495d384..c08ca4bca9c1 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -776,6 +776,11 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
}
}
+ if (adap->attach_ops &&
+ adap->attach_ops->attach_client &&
+ adap->attach_ops->attach_client(adap, info, client) != 0)
+ goto err_attach_client;
+
status = device_register(&client->dev);
if (status)
goto out_free_props;
@@ -786,6 +791,9 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
return client;
out_free_props:
+ if (adap->attach_ops && adap->attach_ops->detach_client)
+ adap->attach_ops->detach_client(adap, client);
+err_attach_client:
if (info->properties)
device_remove_properties(&client->dev);
out_err_put_of_node:
@@ -832,9 +840,17 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
*/
void i2c_unregister_device(struct i2c_client *client)
{
+ struct i2c_adapter *adap;
+
if (!client)
return;
+ adap = client->adapter;
+
+ if (adap->attach_ops &&
+ adap->attach_ops->detach_client)
+ adap->attach_ops->detach_client(adap, client);
+
if (client->dev.of_node) {
of_node_clear_flag(client->dev.of_node, OF_POPULATED);
of_node_put(client->dev.of_node);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fa5552c2307b..ebc372a0e537 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -567,6 +567,21 @@ struct i2c_lock_operations {
void (*unlock_bus)(struct i2c_adapter *adapter, unsigned int flags);
};
+/**
+ * struct i2c_attach_operations - callbacks to notify client attach/detach
+ * @attach_client: Notify of new client being attached
+ * @detach_client: Notify of new client being detached
+ *
+ * Both ops are optional.
+ */
+struct i2c_attach_operations {
+ int (*attach_client)(struct i2c_adapter *adapter,
+ const struct i2c_board_info *info,
+ const struct i2c_client *client);
+ void (*detach_client)(struct i2c_adapter *adapter,
+ const struct i2c_client *client);
+};
+
/**
* struct i2c_timings - I2C timing information
* @bus_freq_hz: the bus frequency in Hz
@@ -690,6 +705,7 @@ struct i2c_adapter {
/* data fields that are valid for all devices */
const struct i2c_lock_operations *lock_ops;
+ const struct i2c_attach_operations *attach_ops;
struct rt_mutex bus_lock;
struct rt_mutex mux_lock;
--
2.17.1
Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
with I2C Address Translator and remote GPIOs.
Signed-off-by: Luca Ceresoli <[email protected]>
---
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
+ - 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>;
+ };
+ };
+ };
+};
--
2.17.1
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 <[email protected]>
>
> ---
>
> 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 <[email protected]>
>>
>> ---
>>
>> 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
--
Luca
On Mon, Aug 19, 2019 at 5:41 PM Luca Ceresoli <[email protected]> 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 <[email protected]>
> >>
> >> ---
> >>
> >> 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.
--
Luca
> + - 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,
--
Luca
> 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,
--
Luca
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.
--
Sakari Ailus
[email protected]
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.
--
Luca