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

Ivan Vecera <[email protected]>
Newsgroups org.kernel.vger.netdev,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
Sashiko findings with replies...

 > zl3073x_poll_zero_u8() hard-codes the interval:
 >
 > #define ZL_POLL_SLEEP_US 10
 >
 > so the new timeouts [...] all wait at a 10 us cadence. [...] giving on
 > the order of 10^5 bus reads while waiting out a WR_NEXT_1HZ command
 > [...]
 > Would a millisecond-scale interval variant be preferable for these
 > three cases, so the shared bus and the driver's monitor kworker are not
 > starved for up to a second?

Good catch. I did some testing and found that even the existing
registers typically clear within low hundreds of iterations, so the
fixed 10 us sleep is already unnecessarily aggressive for all callers.

I will add a prep patch that replaces the fixed ZL_POLL_SLEEP_US with
timeout_us / 50 in zl3073x_poll_zero_u8(). This scales the sleep
interval proportionally to the timeout, targeting ~50 iterations
regardless of timeout magnitude:
   - existing 30-50 ms timeouts: 600-1000 us sleep
   - new 1000-3000 ms PTP timeouts: 20-60 ms sleep

 > Can the tick-crossed branch skip the 20 ms rollover guard and shift the
 > applied time by one second?
 >
 > The margin test is gated on !diff.tv_sec, so when the 1 Hz tick crossed
 > between the two zl3073x_chan_tod_read() calls only the seconds get 
fixed up
 > and the remaining margin is never compared against threshold_ns [...]

Good point. Although this requires ~1 s delay between two consecutive
register reads (practically unreachable), the fix is clean and costs
nothing. I will restructure the guard to separate the tick-crossing
fixup from the margin check so both apply independently:

     diff = timespec64_sub(ts_next, ts_cur);

     if (diff.tv_sec < 0)
         ts_next.tv_sec -= diff.tv_sec;

     if (diff.tv_sec <= 0 && diff.tv_nsec < threshold_ns) {
         fsleep((unsigned long)diff.tv_nsec / NSEC_PER_USEC + 1);
         ts_next.tv_sec++;
     }

Thanks,
Ivan
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.