| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <07e4d87f-0893-40d6-8704-f37c743ff979@ideasonboard.com>
Date: Tue, 27 May 2025 08:40:11 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Yemike Abhilash Chandra <y-abhilashchandra@...com>
Cc: hverkuil@...all.nl, sakari.ailus@...ux.intel.com,
laurent.pinchart@...asonboard.com, vaishnav.a@...com, u-kumar1@...com,
jai.luthra@...ux.dev, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
mchehab@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Subject: Re: [PATCH 2/2] media: i2c: ds90ub960: Add support for DS90UB954-Q1
Hi,
On 23/05/2025 11:36, Yemike Abhilash Chandra wrote:
> DS90UB954-Q1 is an FPDLink-III deserializer that is mostly register
> compatible with DS90UB960-Q1. The main difference is that it supports
> half of the RX and TX ports, i.e. 2x FPDLink RX ports and 1x CSI TX
> port.
>
> Some other registers are marked as reserved in the datasheet as well,
> notably around CSI-TX frame and line-count monitoring and some other
Hmm what does that mean? That in log_status we show random data (or
maybe always 0) for these?
> status registers. The datasheet also does not mention anything about
> setting strobe position, and fails to lock the RX ports if we forcefully
> set it, so disable it through the hw_data.
This app-note has some details:
https://www.ti.com/lit/an/snla301/snla301.pdf
> Link: https://www.ti.com/lit/gpn/ds90ub954-q1
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@...com>
> ---
> drivers/media/i2c/Kconfig | 2 +-
> drivers/media/i2c/ds90ub960.c | 46 +++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index e68202954a8f..6e265e1cec20 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1662,7 +1662,7 @@ config VIDEO_DS90UB960
> select V4L2_FWNODE
> select VIDEO_V4L2_SUBDEV_API
> help
> - Device driver for the Texas Instruments DS90UB960
> + Device driver for the Texas Instruments DS90UB954/DS90UB960
> FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer.
>
> config VIDEO_MAX96714
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index ed2cf9d247d1..38e4f006d098 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -460,6 +460,7 @@ struct ub960_hw_data {
> u8 num_txports;
> bool is_ub9702;
> bool is_fpdlink4;
> + bool is_ub954;
No, let's not add any more of these. We should have enums for the device
model and the "family" (ub954/ub960 are clearly of the same family,
whereas ub9702 is of a newer one).
> };
>
> enum ub960_rxport_mode {
> @@ -982,6 +983,10 @@ static int ub960_txport_select(struct ub960_data *priv, u8 nport)
>
> lockdep_assert_held(&priv->reg_lock);
>
> + /* TX port registers are shared for UB954*/
Space missing at the end. What does the comment mean? "registers are
shared"?
I think it's good to have this after the lockdep assert. The lock rules
are in place, even if on ub954 we don't do anything here.
> + if (priv->hw_data->is_ub954)
> + return 0;
> +
> if (priv->reg_current.txport == nport)
> return 0;
>
> @@ -1415,6 +1420,13 @@ static int ub960_parse_dt_txport(struct ub960_data *priv,
> goto err_free_vep;
> }
>
> + /* UB954 does not support 1.2 Gbps */
> + if (priv->tx_data_rate == MHZ(1200) && priv->hw_data->is_ub954) {
Test for ub954 first, 1200 MHz second. It's more logical for the reader
that way.
> + dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport);
> + ret = -EINVAL;
> + goto err_free_vep;
> + }
> +
> v4l2_fwnode_endpoint_free(&vep);
>
> priv->txports[nport] = txport;
> @@ -1572,6 +1584,10 @@ static int ub960_rxport_set_strobe_pos(struct ub960_data *priv,
> u8 clk_delay, data_delay;
> int ret = 0;
>
> + /* FIXME: After writing to this area the UB954 chip no longer responds */
> + if (priv->hw_data->is_ub954)
> + return 0;
> +
Check the app note. It would be nice to have this working, as, afaik,
the HW functionality should be the same on ub954 and ub960.
> clk_delay = UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
> data_delay = UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
>
> @@ -5021,6 +5037,27 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
> if (priv->hw_data->is_ub9702)
> ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq,
> NULL);
> + else if (priv->hw_data->is_ub954) {
> + /* From DS90UB954-Q1 datasheet:
> + * "REFCLK_FREQ measurement is not synchronized. Value in this
> + * register should read twice and only considered valid if
> + * REFCLK_FREQ is unchanged between reads."
> + */
> + unsigned long timeout = jiffies + msecs_to_jiffies(100);
> +
> + do {
> + u8 refclk_new;
> +
> + ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_new,
> + NULL);
> + if (ret)
> + goto err_pd_gpio;
> +
> + if (refclk_new == refclk_freq)
> + break;
> + refclk_freq = refclk_new;
> + } while (time_before(jiffies, timeout));
> + }
This feels a bit too much for a not-that-important debug print... As the
tests show that a single read is (practically always?) enough, I think
we can just use the same code as for ub960. Maybe add a comment about
it, though.
Tomi
> else
> ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq,
> NULL);
> @@ -5177,6 +5214,13 @@ static void ub960_remove(struct i2c_client *client)
> mutex_destroy(&priv->reg_lock);
> }
>
> +static const struct ub960_hw_data ds90ub954_hw = {
> + .model = "ub954",
> + .num_rxports = 2,
> + .num_txports = 1,
> + .is_ub954 = true,
> +};
> +
> static const struct ub960_hw_data ds90ub960_hw = {
> .model = "ub960",
> .num_rxports = 4,
> @@ -5192,6 +5236,7 @@ static const struct ub960_hw_data ds90ub9702_hw = {
> };
>
> static const struct i2c_device_id ub960_id[] = {
> + { "ds90ub954-q1", (kernel_ulong_t)&ds90ub954_hw },
> { "ds90ub960-q1", (kernel_ulong_t)&ds90ub960_hw },
> { "ds90ub9702-q1", (kernel_ulong_t)&ds90ub9702_hw },
> {}
> @@ -5199,6 +5244,7 @@ static const struct i2c_device_id ub960_id[] = {
> MODULE_DEVICE_TABLE(i2c, ub960_id);
>
> static const struct of_device_id ub960_dt_ids[] = {
> + { .compatible = "ti,ds90ub954-q1", .data = &ds90ub954_hw },
> { .compatible = "ti,ds90ub960-q1", .data = &ds90ub960_hw },
> { .compatible = "ti,ds90ub9702-q1", .data = &ds90ub9702_hw },
> {}
Powered by blists - more mailing lists