| 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: <482afba8-c6fb-4420-83f1-597fb7088ce4@ti.com>
Date: Mon, 26 May 2025 11:54:32 +0530
From: Yemike Abhilash Chandra <y-abhilashchandra@...com>
To: Jai Luthra <jai.luthra@...asonboard.com>, <conor+dt@...nel.org>,
<krzk+dt@...nel.org>, <mchehab@...nel.org>, <robh@...nel.org>,
<tomi.valkeinen@...asonboard.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>
Subject: Re: [PATCH 2/2] media: i2c: ds90ub960: Add support for DS90UB954-Q1
Hi Jai,
Thanks for the review.
On 23/05/25 22:23, Jai Luthra wrote:
> Hi Abhilash,
>
> Thanks for the patch.
>
> Quoting Yemike Abhilash Chandra (2025-05-23 14:06:55)
>> 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
>> status registers. The datasheet also does not mention anything about
>
> So what happens when userspace calls LOG_STATUS and the driver tries to
> read these monitoring registers? Are these populated in the device but just
> marked as reserved in the datasheet?
>
> Whatever is the case, please make sure the driver doesn't crash, and update
> the commit message with the reality if the datasheet is wrong.
>
I don't see a crash while doing a log-status [1]. In the driver, we
check what
TX and RX ports are active from the HW data and the do a register read
accordingly. That should be fine I believe.
>> setting strobe position, and fails to lock the RX ports if we forcefully
>> set it, so disable it through the hw_data.
>>
>> 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.
>
> nit:
> Device driver for the Texas Instruments DS90UB954, DS90UB960
> FPD-Link III Deserializers 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;
>> };
>>
>> 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*/
>> + if (priv->hw_data->is_ub954)
>> + return 0;
>> +
>
> nit: This could be moved above the assertion
Will do that in next revision.
>
>> 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) {
>> + dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport);
>> + ret = -EINVAL;
>> + goto err_free_vep;
>> + }
>> +
>
> The error handling is exactly the same as the previous if {} block that
> checks the allowed data rates for UB960. IMO cleaner to move this condition
> in that block.
>
Noted, will try to do that in a cleaner way in next revision.
> Maybe even a separate table for allowed data-rates for each chip, but that
> is probably overkill.
>
>> 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;
>> +
>
> It would be good to understand if this is a hardware limitation or not.
> Tomi, do you have any idea?
>
>> 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
>
> * register should be 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));
>> + }
>
> Hmm.. in your testing did you find this actually requiring more than one
> read?
>
> I'm surprised because this is missing from UB960 which is an older device.
>
In my testing (around 20 reboots) , I had to do only 1 check i.e just 2
iterations. I am not sure on how to proceed but the data sheet at7.6.121
REFCLK_FREQ Register clearly specifies the below.
"REFCLK_FREQ measurement is not synchronized. Value in this
register should read twice and only considered valid if
REFCLK_FREQ is unchanged between reads."
Thanks and Regards,
Abhilash Chandra
[1]:
https://gist.github.com/Yemike-Abhilash-Chandra/dc07a6389d06648d9e80de23d8cae954
>> 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 },
>> {}
>> --
>> 2.34.1
>>
>>
>
> Thanks,
> Jai
Powered by blists - more mailing lists