| 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: <88e656e4-c9b6-43a3-8dd5-ac9ab65a0ff5@ti.com>
Date: Thu, 18 Dec 2025 16:09:37 +0530
From: Yemike Abhilash Chandra <y-abhilashchandra@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
CC: <hansg@...nel.org>, <mehdi.djait@...ux.intel.com>, <ribalda@...omium.org>,
<git@...tzsch.eu>, <vladimir.zapolskiy@...aro.org>,
<benjamin.mugnier@...s.st.com>, <dongcheng.yan@...el.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>, <hverkuil@...all.nl>, <sakari.ailus@...ux.intel.com>,
<laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH V2 4/4] media: i2c: ds90ub960: Add support for
DS90UB954-Q1
Hi Tomi,
Thanks for the review.
On 05/12/25 16:40, Tomi Valkeinen wrote:
> Hi,
>
> On 02/12/2025 12:22, 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.
>>
>> A couple of differences are between the status registers and the
>> strobe setting registers. Hence accommodate these differences in
>> the UB960 driver so that we can reuse a large part of the existing code.
>>
>> Link: https://www.ti.com/lit/gpn/ds90ub954-q1
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@...com>
>> ---
>> Refer table 5.2.1 STROBE_SET Register in [1] for DS90UB954 strobe
>> setting register.
>>
>> [1]: https://www.ti.com/lit/an/snla301/snla301.pdf
>>
>> drivers/media/i2c/Kconfig | 4 +-
>> drivers/media/i2c/ds90ub960.c | 165 +++++++++++++++++++++++++---------
>> 2 files changed, 125 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 745819c625d6..52104f76e371 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -1703,8 +1703,8 @@ config VIDEO_DS90UB960
>> select V4L2_FWNODE
>> select VIDEO_V4L2_SUBDEV_API
>> help
>> - Device driver for the Texas Instruments DS90UB960
>> - FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer.
>> + Device driver for the Texas Instruments DS90UB954, DS90UB960
>> + FPD-Link III Deserializers and DS90UB9702 FPD-Link IV Deserializer.
>>
>> config VIDEO_MAX96714
>> tristate "Maxim MAX96714 GMSL2 deserializer"
>> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
>> index 45494fcaf095..7d3e5a87bb17 100644
>> --- a/drivers/media/i2c/ds90ub960.c
>> +++ b/drivers/media/i2c/ds90ub960.c
>> @@ -396,6 +396,12 @@
>> #define UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY BIT(3)
>> #define UB960_IR_RX_ANA_STROBE_SET_DATA_DELAY_MASK GENMASK(2, 0)
>>
>> +#define UB954_IR_RX_ANA_STROBE_SET_CLK_DATA 0x08
>> +#define UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY BIT(3)
>> +#define UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY BIT(7)
>> +#define UB954_IR_RX_ANA_STROBE_SET_CLK_DELAY_MASK GENMASK(2, 0)
>> +#define UB954_IR_RX_ANA_STROBE_SET_DATA_DELAY_MASK GENMASK(4, 6)
>> +
>> /* UB9702 Registers */
>>
>> #define UB9702_SR_CSI_EXCLUSIVE_FWD2 0x3c
>> @@ -455,6 +461,7 @@
>> #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1)
>>
>> enum chip_type {
>> + UB954,
>> UB960,
>> UB9702,
>> };
>> @@ -1000,6 +1007,10 @@ static int ub960_txport_select(struct ub960_data *priv, u8 nport)
>>
>> lockdep_assert_held(&priv->reg_lock);
>>
>> + /* UB954 has only 1 CSI TX. Hence, no need to select */
>> + if (priv->hw_data->chip_type == UB954)
>> + return 0;
>> +
>> if (priv->reg_current.txport == nport)
>> return 0;
>>
>> @@ -1424,10 +1435,11 @@ static int ub960_parse_dt_txport(struct ub960_data *priv,
>> priv->tx_link_freq[0] = vep.link_frequencies[0];
>> priv->tx_data_rate = priv->tx_link_freq[0] * 2;
>>
>> - if (priv->tx_data_rate != MHZ(1600) &&
>> - priv->tx_data_rate != MHZ(1200) &&
>> - priv->tx_data_rate != MHZ(800) &&
>> - priv->tx_data_rate != MHZ(400)) {
>> + if ((priv->tx_data_rate != MHZ(1600) &&
>> + priv->tx_data_rate != MHZ(1200) &&
>> + priv->tx_data_rate != MHZ(800) &&
>> + priv->tx_data_rate != MHZ(400)) ||
>> + (priv->hw_data->chip_type == UB954 && priv->tx_data_rate == MHZ(1200))) {
>> dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport);
>> ret = -EINVAL;
>> goto err_free_vep;
>> @@ -1551,22 +1563,44 @@ static int ub960_rxport_get_strobe_pos(struct ub960_data *priv,
>> u8 clk_delay, data_delay;
>> int ret;
>>
>> - ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> - UB960_IR_RX_ANA_STROBE_SET_CLK, &v, NULL);
>> - if (ret)
>> - return ret;
>> + /*
>> + * DS90UB960 has two separate registers for clk and data delay whereas
>> + * DS90UB954 has a single combined register. Hence read accordingly
>> + */
>
> Why do you read the single register twice? In any case, I don't think
> the comment is needed, as it's quite clear from the code. Unless there's
> some extra complication with the registers.
>
I will fix this in v3.
>> + if (priv->hw_data->chip_type == UB954) {
>> + ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB954_IR_RX_ANA_STROBE_SET_CLK_DATA, &v, NULL);
>> + if (ret)
>> + return ret;
>>
>> - clk_delay = (v & UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY) ?
>> - 0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
>> + clk_delay = (v & UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY) ?
>> + 0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
>>
>> - ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> - UB960_IR_RX_ANA_STROBE_SET_DATA, &v, NULL);
>> - if (ret)
>> - return ret;
>> + ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB954_IR_RX_ANA_STROBE_SET_CLK_DATA, &v, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + data_delay = (v & UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY) ?
>> + 0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
>> + } else {
>> + ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB960_IR_RX_ANA_STROBE_SET_CLK, &v, NULL);
>> + if (ret)
>> + return ret;
>>
>> - data_delay = (v & UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY) ?
>> + clk_delay = (v & UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY) ?
>> 0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
>>
>> + ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB960_IR_RX_ANA_STROBE_SET_DATA, &v, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + data_delay = (v & UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY) ?
>> + 0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
>> + }
>> +
>> ret = ub960_rxport_read(priv, nport, UB960_RR_SFILTER_STS_0, &v, NULL);
>> if (ret)
>> return ret;
>> @@ -1590,8 +1624,17 @@ static int ub960_rxport_set_strobe_pos(struct ub960_data *priv,
>> u8 clk_delay, data_delay;
>> int ret = 0;
>>
>> - clk_delay = UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
>> - data_delay = UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
>> + /*
>> + * DS90UB960 has two separate registers for clk and data delay whereas
>> + * DS90UB954 has a single combined register. Hence assign accordingly.
>> + */
>> + if (priv->hw_data->chip_type == UB954) {
>> + clk_delay = UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
>> + data_delay = UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
>> + } else {
>> + clk_delay = UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
>> + data_delay = UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
>> + }
>>
>> if (strobe_pos < UB960_MIN_AEQ_STROBE_POS)
>> clk_delay = abs(strobe_pos) - UB960_MANUAL_STROBE_EXTRA_DELAY;
>> @@ -1602,11 +1645,25 @@ static int ub960_rxport_set_strobe_pos(struct ub960_data *priv,
>> else if (strobe_pos > 0)
>> data_delay = strobe_pos | UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
>>
>> - ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> - UB960_IR_RX_ANA_STROBE_SET_CLK, clk_delay, &ret);
>> -
>> - ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> - UB960_IR_RX_ANA_STROBE_SET_DATA, data_delay, &ret);
>> + /*
>> + * DS90UB960 has two separate registers for clk and data delay whereas
>> + * DS90UB954 has a single combined register. Hence write the registers accordingly.
>> + */
>> + if (priv->hw_data->chip_type == UB954) {
>> + ub960_ind_update_bits(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB954_IR_RX_ANA_STROBE_SET_CLK_DATA,
>> + UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY,
>> + clk_delay, &ret);
>> + ub960_ind_update_bits(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB954_IR_RX_ANA_STROBE_SET_CLK_DATA,
>> + UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY,
>> + data_delay, &ret);
>
> Here, too. It's a single register, why write it twice?
>
> And I don't think this is correct at all... Did you validate this? The
> above only sets the EXTRA_DELAY bits, not the values at all. And the
> code that sets clk_delay and data_delay use UB960's bit positions, which
> are not the same on UB954.
>
Apologies, Yes, I did validate this. however, my testing did not
encounter the
condition where the data delay bits are set, which is the main
difference in registers.
I will correct this in v3.
>> + } else {
>> + ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB960_IR_RX_ANA_STROBE_SET_CLK, clk_delay, &ret);
>> + ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
>> + UB960_IR_RX_ANA_STROBE_SET_DATA, data_delay, &ret);
>> + }
>>
>> return ret;
>> }
>> @@ -4176,33 +4233,40 @@ static int ub960_log_status(struct v4l2_subdev *sd)
>> dev_info(dev, "\tsync %u, pass %u\n", v & (u8)BIT(1),
>> v & (u8)BIT(0));
>>
>> - ret = ub960_read16(priv, UB960_SR_CSI_FRAME_COUNT_HI(nport),
>> - &v16, NULL);
>> - if (ret)
>> - return ret;
>> + /*
>> + * Frame counter, frame error counter, line counter and line error counter
>> + * registers are marked as reserved in the UB954 datasheet. Hence restrict
>> + * the following register reads only for UB960 and UB9702.
>> + */
>> + if (priv->hw_data->chip_type != UB954) {
>
> It is better to check for the chips that have the registers, unless
> we're sure that this particular chip, ub954, is and will be the only
> outlier.
>
I will make this change in v3.
>> + ret = ub960_read16(priv, UB960_SR_CSI_FRAME_COUNT_HI(nport),
>> + &v16, NULL);
>> + if (ret)
>> + return ret;
>>
>> - dev_info(dev, "\tframe counter %u\n", v16);
>> + dev_info(dev, "\tframe counter %u\n", v16);
>>
>> - ret = ub960_read16(priv, UB960_SR_CSI_FRAME_ERR_COUNT_HI(nport),
>> - &v16, NULL);
>> - if (ret)
>> - return ret;
>> + ret = ub960_read16(priv, UB960_SR_CSI_FRAME_ERR_COUNT_HI(nport),
>> + &v16, NULL);
>> + if (ret)
>> + return ret;
>>
>> - dev_info(dev, "\tframe error counter %u\n", v16);
>> + dev_info(dev, "\tframe error counter %u\n", v16);
>>
>> - ret = ub960_read16(priv, UB960_SR_CSI_LINE_COUNT_HI(nport),
>> - &v16, NULL);
>> - if (ret)
>> - return ret;
>> + ret = ub960_read16(priv, UB960_SR_CSI_LINE_COUNT_HI(nport),
>> + &v16, NULL);
>> + if (ret)
>> + return ret;
>>
>> - dev_info(dev, "\tline counter %u\n", v16);
>> + dev_info(dev, "\tline counter %u\n", v16);
>>
>> - ret = ub960_read16(priv, UB960_SR_CSI_LINE_ERR_COUNT_HI(nport),
>> - &v16, NULL);
>> - if (ret)
>> - return ret;
>> + ret = ub960_read16(priv, UB960_SR_CSI_LINE_ERR_COUNT_HI(nport),
>> + &v16, NULL);
>> + if (ret)
>> + return ret;
>>
>> - dev_info(dev, "\tline error counter %u\n", v16);
>> + dev_info(dev, "\tline error counter %u\n", v16);
>> + }
>> }
>>
>> for_each_rxport(priv, it) {
>> @@ -5023,6 +5087,9 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
>> }
>>
>> switch (priv->hw_data->chip_type) {
>> + case UB954:
>> + model = "UB954";
>> + break;
>> case UB960:
>> model = "UB960";
>> break;
>> @@ -5039,6 +5106,11 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
>> if (ret)
>> goto err_pd_gpio;
>>
>> + /*
>> + * UB954 REFCLK_FREQ is not synchronized, so multiple reads are recommended
>> + * by the datasheet. However, we use the same logic as UB960 (single read),
>> + * as practical testing showed this is sufficient and stable for UB954 as well.
>> + */
>
> I think the important point is that the clk rate is only used for a
> debug print.
>
Yeah, I will add this detail to the comment.
Thanks and Regards
Yemike Abhilash Chandra
>> if (priv->hw_data->chip_type == UB9702)
>> ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq,
>> NULL);
>> @@ -5198,6 +5270,13 @@ static void ub960_remove(struct i2c_client *client)
>> mutex_destroy(&priv->reg_lock);
>> }
>>
>> +static const struct ub960_hw_data ds90ub954_hw = {
>> + .chip_type = UB954,
>> + .chip_family = FAMILY_FPD3,
>> + .num_rxports = 2,
>> + .num_txports = 1,
>> +};
>> +
>> static const struct ub960_hw_data ds90ub960_hw = {
>> .chip_type = UB960,
>> .chip_family = FAMILY_FPD3,
>> @@ -5213,6 +5292,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 },
>> {}
>> @@ -5220,6 +5300,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 },
>> {}
>
> Tomi
>
Powered by blists - more mailing lists