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

[Paolo Abeni]

Hi,

I'm sorry for the late feedback, I have a few comments below...

On Fri, 2023-11-24 at 15:20 -0500, Min Li 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>
> ---
> - Drop MAX_ABS_WRITE_PHASE_PICOSECONDS advised by Rahul
> - Apply SCSR_ADDR to scrach register in idtcm_load_firmware advised by Simon
> - Apply u32 to base in idtcm_output_enable advised by Simon
> - Correct sync_ctrl0/1 parameter position for idtcm_write advised by Simon
> - Restore adjphase function suggested by Rahul
> 
>  drivers/ptp/ptp_clockmatrix.c    |  69 ++--
>  drivers/ptp/ptp_clockmatrix.h    |  32 +-
>  include/linux/mfd/idt8a340_reg.h | 542 ++++++++++++++++---------------
>  3 files changed, 329 insertions(+), 314 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> index f6f9d4adce04..8a48214adc26 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c
> @@ -41,7 +41,7 @@ module_param(firmware, charp, 0);
>  static int _idtcm_adjfine(struct idtcm_channel *channel, long scaled_ppm);
>  
>  static inline int idtcm_read(struct idtcm *idtcm,
> -			     u16 module,
> +			     u32 module,
>  			     u16 regaddr,

If you change 'regaddr' type to u32, that will allow reducing some
relevant 'noise' in the chunks below, avoid swapping the 'mudule' and
'regaddr' arguments from the callers.

Such mentioned chunks are IMHO quite confusing/counter-intuitive, it
would be better get rid of them regardless of the better diffstat.

>  			     u8 *buf,
>  			     u16 count)
> @@ -50,7 +50,7 @@ static inline int idtcm_read(struct idtcm *idtcm,
>  }
>  
>  static inline int idtcm_write(struct idtcm *idtcm,
> -			      u16 module,
> +			      u32 module,
>  			      u16 regaddr,

Same here.

>  			      u8 *buf,
>  			      u16 count)

> @@ -1395,6 +1396,20 @@ static int idtcm_set_pll_mode(struct idtcm_channel *channel,
>  	struct idtcm *idtcm = channel->idtcm;
>  	int err;
>  	u8 dpll_mode;
> +	u8 timeout = 0;
> +
> +	/* Setup WF/WP timer for phase pull-in to work correctly */
> +	err = idtcm_write(idtcm, channel->dpll_n, DPLL_WF_TIMER,
> +			  &timeout, sizeof(timeout));
> +	if (err)
> +		return err;
> +
> +	if (mode == PLL_MODE_WRITE_PHASE)
> +		timeout = 160;
> +	err = idtcm_write(idtcm, channel->dpll_n, DPLL_WP_TIMER,
> +			  &timeout, sizeof(timeout));
> +	if (err)
> +		return err;
>  
>  	err = idtcm_read(idtcm, channel->dpll_n,
>  			 IDTCM_FW_REG(idtcm->fw_ver, V520, DPLL_MODE),

I think this chunk could/should go in a different patch: a first one is
implementing the support for 32 bit address space, a 2nd is leveraging
it to configure the PLL correctly.

Cheers,

Paolo