Re: [PATCH net-next v4 1/2] dpll: zl3073x: add channel ToD, phase step and TIE operations

On 05/08/2026 10:48, Ivan Vecera wrote:
> On 8/5/26 1:07 AM, Vadim Fedorenko wrote:
>> On 03/08/2026 15:06, Ivan Vecera wrote:
>>> Add low-level DPLL channel operations for ToD read/write/adjust,
>>> output phase step, delta frequency offset write and TIE (Time
>>> Interval Error) write. These serve as building blocks for the PTP
>>> clock callbacks added in the next patch.
>>>
>>> ToD operations use a wait-before-write pattern to avoid blocking
>>> after each operation.
>>>
>>> The tod_ready_wait helper selects the poll timeout based on the
>>> current ToD command - write operations use a longer timeout (1000 ms)
>>> than reads (30 ms).
>>>
>>> The ToD read captures system timestamps (ptp_system_timestamp) around
>>> the HW command and completion poll to support cross-timestamping.
>>>
>>> The TIE write operation provides sub-picosecond resolution phase
>>> adjustment for modes where the DPLL is tracking a reference
>>> (AUTO and REFLOCK).
>>>
>>> Add output step-time mask to struct zl3073x_dev and
>>> zl3073x_dev_out_is_stepped() helper to check if an output
>>> participates in step-time operations.
>>>
>>> Reviewed-by: Petr Oros <[email protected]>
>>> Tested-by: Chris du Quesnay <[email protected]>
>>> Signed-off-by: Ivan Vecera <[email protected]>
>>> ---
>>>   drivers/dpll/zl3073x/chan.c | 310 +++++++++++++++++++++++++++++++++++-
>>>   drivers/dpll/zl3073x/chan.h |  32 ++++
>>>   drivers/dpll/zl3073x/core.c |  13 ++
>>>   drivers/dpll/zl3073x/core.h |  23 +++
>>>   drivers/dpll/zl3073x/regs.h |  52 ++++++
>>>   5 files changed, 428 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c
>>> index 4ec2cf53dad468..79874a9fdb4962 100644
>>> --- a/drivers/dpll/zl3073x/chan.c
>>> +++ b/drivers/dpll/zl3073x/chan.c
>>> @@ -3,6 +3,7 @@
>>>   #include <linux/cleanup.h>
>>>   #include <linux/delay.h>
>>>   #include <linux/dev_printk.h>
>>> +#include <linux/ptp_clock_kernel.h>
>>>   #include <linux/string.h>
>>>   #include <linux/types.h>
>>> @@ -162,8 +163,8 @@ int zl3073x_chan_nco_mode_set(struct zl3073x_dev 
>>> *zldev, u8 index)
>>>    * @zldev: pointer to zl3073x_dev structure
>>>    * @index: DPLL channel index to fetch state for
>>>    *
>>> - * Reads the mode_refsel register and reference priority registers for
>>> - * the given DPLL channel and stores the raw values for later use.
>>> + * Reads the mode_refsel, status and reference priority registers for
>>> + * the given DPLL channel and stores the values for later use.
>>>    *
>>>    * Return: 0 on success, <0 on error
>>>    */
>>> @@ -234,6 +235,311 @@ const struct zl3073x_chan 
>>> *zl3073x_chan_state_get(struct zl3073x_dev *zldev,
>>>       return &zldev->chan[index];
>>>   }
>>> +/**
>>> + * zl3073x_chan_tod_ready_wait - wait for ToD semaphore to clear
>>> + * @zldev: pointer to zl3073x device
>>> + * @ch: DPLL channel index
>>> + *
>>> + * Polls the ToD control register until the semaphore bit is cleared,
>>> + * indicating the device has completed the previous ToD operation.
>>> + *
>>> + * Return: 0 on success, -EBUSY if semaphore not cleared, <0 on error
>>> + */
>>> +int zl3073x_chan_tod_ready_wait(struct zl3073x_dev *zldev, u8 ch)
>>> +{
>>> +    unsigned int timeout;
>>> +    u8 tod_ctrl;
>>> +    int rc;
>>> +
>>> +    rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_TOD_CTRL(ch), &tod_ctrl);
>>> +    if (rc)
>>> +        return rc;
>>> +
>>> +    switch (FIELD_GET(ZL_DPLL_TOD_CTRL_CMD, tod_ctrl)) {
>>> +    case ZL_DPLL_TOD_CTRL_CMD_WR_NEXT_1HZ:
>>> +        timeout = ZL_POLL_TOD_WR_TIMEOUT_US;
>>> +        break;
>>> +    default:
>>> +        timeout = ZL_POLL_TOD_RD_TIMEOUT_US;
>>> +        break;
>>> +    }
>>
>> there are 3 cmds defined, but FIELD_GET(ZL_DPLL_TOD_CTRL_CMD) can return
>> up to 16 possible values. I would explicitly put defined commands in
>> cases and make default to ENOTSUPP..
> 
> Hi Vadim,
> the ZL_DPLL_TOD_CTRL_CMD bits are never filled by firmware (only the
> semaphore bit) so the driver knows what it writes. So such check is not
> necessary but if you want it I can add something like:
> 
> ...
>      case ZL_DPLL_TOD_CTRL_CMD_RD_CURRENT:
>      case ZL_DPLL_TOD_CTRL_CMD_RD_NEXT_1HZ:
>          timeout = ZL_POLL_TOD_RD_TIMEOUT_US;
>          break;
>      default:
>          WARN_ON(1); /* this is really unexpected */
>          return -ENOTSUPP;
> ...

This looks a bit more safe, especially now while the driver is in active
development. Let's add this part.

Thanks

> 
> Let me know.
> 
> Thanks,
> Ivan
>