Re: [PATCH net-next 1/1] ptp: clockmatrix: support 32-bit address space

[Rahul Rameshbabu]

On Wed, 08 Nov, 2023 13:28:30 -0500 Min Li <lnimi@xxxxxxxxxxx> wrote:
> From: Min Li <min.li.xe@xxxxxxxxxxx>
>
> We used to assume 0x2010xxxx address. Now that
> we need to access 0x2011xxxx address, we need
> to support read/write the whole 32-bit address space.
>
> Signed-off-by: Min Li <min.li.xe@xxxxxxxxxxx>
> ---
>  drivers/ptp/ptp_clockmatrix.c    |  72 ++--
>  drivers/ptp/ptp_clockmatrix.h    |  33 +-
>  include/linux/mfd/idt8a340_reg.h | 542 ++++++++++++++++---------------
>  3 files changed, 340 insertions(+), 307 deletions(-)
>
> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> index f6f9d4adce04..875841892842 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c

<snip>

> @@ -1705,10 +1720,14 @@ static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused)
>  }
>  
>  /*
> - * Internal function for implementing support for write phase offset
> + * Maximum absolute value for write phase offset in picoseconds

This logic should be part of getmaxphase rather than adjphase. Due to
the existing implementation of getmaxphase in this driver, the checks
added to the driver in this patch are no longer applicable or reachable
based on the value of MAX_ABS_WRITE_PHASE_PICOSECONDS.

  https://lore.kernel.org/netdev/20230612211500.309075-8-rrameshbabu@xxxxxxxxxx/

>   *
>   * @channel:  channel
>   * @delta_ns: delta in nanoseconds
> + *
> + * Destination signed register is 32-bit register in resolution of 50ps
> + *
> + * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350
>   */
>  static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns)
>  {
> @@ -1717,6 +1736,7 @@ static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns)
>  	u8 i;
>  	u8 buf[4] = {0};
>  	s32 phase_50ps;
> +	s64 offset_ps;
>  
>  	if (channel->mode != PTP_PLL_MODE_WRITE_PHASE) {
>  		err = channel->configure_write_phase(channel);
> @@ -1724,7 +1744,19 @@ static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns)
>  			return err;
>  	}
>  
> -	phase_50ps = div_s64((s64)delta_ns * 1000, 50);
> +	offset_ps = (s64)delta_ns * 1000;
> +
> +	/*
> +	 * Check for 32-bit signed max * 50:
> +	 *
> +	 * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350
> +	 */
> +	if (offset_ps > MAX_ABS_WRITE_PHASE_PICOSECONDS)
> +		offset_ps = MAX_ABS_WRITE_PHASE_PICOSECONDS;
> +	else if (offset_ps < -MAX_ABS_WRITE_PHASE_PICOSECONDS)
> +		offset_ps = -MAX_ABS_WRITE_PHASE_PICOSECONDS;
> +
> +	phase_50ps = div_s64(offset_ps, 50);

This logic is unreachable because of idtcm_getmaxphase. Please remove
this hunk. The point of getmaxphase is to standardize the behavior of
what happens when a user passes an adjustment value not supported by a
device rather than letting device drivers define different behaviors for
handling this case.

  https://lore.kernel.org/netdev/20230612211500.309075-6-rrameshbabu@xxxxxxxxxx/

>  
>  	for (i = 0; i < 4; i++) {
>  		buf[i] = phase_50ps & 0xff;
> diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
> index 7c17c4f7f573..a0aa88c8a4ab 100644
> --- a/drivers/ptp/ptp_clockmatrix.h
> +++ b/drivers/ptp/ptp_clockmatrix.h
> @@ -19,6 +19,7 @@
>  #define MAX_REF_CLK	(16)
>  
>  #define MAX_ABS_WRITE_PHASE_NANOSECONDS (107374182L)
> +#define MAX_ABS_WRITE_PHASE_PICOSECONDS (107374182350LL)

This macro is not needed due to reasons previously mentioned.

<snip>

--
Thanks,

Rahul Rameshbabu