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

On 8/5/26 12:03 PM, Vadim Fedorenko wrote:
> 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

OK, will update in v5.

Thanks,
Ivan