Message-ID: <5db8987f-c074-4e13-b571-6d455d452f7f@ti.com>
Date: Mon, 2 Jun 2025 16:24:32 +0530
From: Yemike Abhilash Chandra <y-abhilashchandra@...com>
To: Tomi Valkeinen <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>, <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 Tomi,
Thanks for the review.

On 02/06/25 12:46, Tomi Valkeinen wrote:
> Hi,
> 
> On 28/05/2025 09:25, Yemike Abhilash Chandra wrote:
>> Hi Tomi,
>>
>> Thanks for the review.
>>
>> On 27/05/25 11:10, Tomi Valkeinen wrote:
>>> 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?
>>>
>>
>> It seems like it is showing 0's for these. I streamed around 100 frames.
>> But the frame counter and line counter returned is 0. Please find the
>> logs at [1].
> 
> If the registers are marked as reserved and don't function, we should
> not use them. Here it doesn't do any harm when running the code, but it
> does decrease the usefulness of log_status if the user is shown data
> that is wrong (and the user most likely doesn't know it's wrong).
> 

Yes, Understood. I will address this in the next revision.

>>>> 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).
>>>
>>
>> Got it. I will add enums in the next revision.
>>
>>>>    };
>>>>      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"?
>>>
>>
>> Apologies for the inaccurate comment description, My intention to
>> comment that the tx_port_select function does not make sense for
>> UB954, since we have only 1 CSI TX. May be I can have something
>> like below.
>>
>> /** UB954 has only 1 CSI TX. Hence, no need to select **/
>>
>>> 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.
>>>
>>
>> Noted, will do that in the next revision.
>>
>>>> +        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.
>>>
>>
>> I tried referring the app note and changed the strobe position values
>> accordingly, but it did not help.
>>
>> Since the app note also specifies the below at Table 2 Strobe Adaption
>> Modes
>>
>> "
>> AEQ Adaption Mode--> Strobe position is selected as part of AEQ. This is
>> the default mode.
>>
>> Manual Adaption Mode --> The strobe position is selected manually and
>> will remain at
>> the specified position until a new one is chosen. This mode is
>> recommended as an
>> evaluation and debugging mode "
>>
>> Since, under the default settings, the strobe position is selected as
>> part of the AEQ process.
>> Can we limit the ub960_rxport_set_strobe_pos function to only UB960 and
>> UB9702.
> 
> Ok. But it doesn't sound good if we just skip the
> ub960_rxport_set_strobe_pos(), but keep all the other EQ related writes.
> I.e. we do the EQ config partially, and leave out parts that, for
> unknown reasons, seem to cause problems...
> 
> So probably the check should be in ub960_rxport_config_eq(). With a
> FIXME comment, and a short note where it fails.
> 
> That said, if everyone (?) agrees that the HW should support this, it
> would be really nice if you can keep poking the FPD-Link people in TI
> and try to get clarification on what's going on (what's the diff between
> ub960 and ub954).
> 

Yes, I will do that.

> Btw, did you look at the other EQ related writes, and check if they're
> valid for ub954?
> 

Not until now, will check that.

Thanks and Regards
Abhilash Chandra


>   Tomi
> 
>>>>        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.
>>>
>>
>> okay, I will use the same code that is being used for UB960 and will add
>> a comment
>> about that.
>>
>> Thanks and Regards,
>> Abhilash Chandra
>>
>> [1]: https://gist.github.com/Yemike-Abhilash-Chandra/
>> c6b3da2a10586567a3a4179a2b20d21b
>>
>>>    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