Re: [PATCH v2 net-next 1/3] ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.

[Jakub Kicinski]

On Sat, 13 Feb 2021 00:06:04 -0500 vincent.cheng.xh@xxxxxxxxxxx wrote:
> From: Vincent Cheng <vincent.cheng.xh@xxxxxxxxxxx>
> 
> Part of the device initialization aligns the rising edge of the output
> clock to the internal 1 PPS clock. If the system APLL and DPLL is not
> locked, then the alignment will fail and there will be a fixed offset
> between the internal 1 PPS clock and the output clock.
> 
> After loading the device firmware, poll the system APLL and DPLL for
> locked state prior to initialization, timing out after 2 seconds.
> 
> Signed-off-by: Vincent Cheng <vincent.cheng.xh@xxxxxxxxxxx>
> Acked-by: Richard Cochran <richardcochran@xxxxxxxxx>

> diff --git a/drivers/ptp/idt8a340_reg.h b/drivers/ptp/idt8a340_reg.h
> index a664dfe..ac524cf 100644
> --- a/drivers/ptp/idt8a340_reg.h
> +++ b/drivers/ptp/idt8a340_reg.h
> @@ -122,6 +122,8 @@
>  #define OTP_SCSR_CONFIG_SELECT            0x0022
>  
>  #define STATUS                            0xc03c
> +#define DPLL_SYS_STATUS                   0x0020
> +#define DPLL_SYS_APLL_STATUS              0x0021
>  #define USER_GPIO0_TO_7_STATUS            0x008a
>  #define USER_GPIO8_TO_15_STATUS           0x008b
>  
> @@ -707,4 +709,12 @@
>  /* Bit definitions for the DPLL_CTRL_COMBO_MASTER_CFG register */
>  #define COMBO_MASTER_HOLD                 BIT(0)
>  
> +/* Bit definitions for DPLL_SYS_STATUS register */
> +#define DPLL_SYS_STATE_MASK               (0xf)
> +
> +/* Bit definitions for SYS_APLL_STATUS register */
> +#define SYS_APLL_LOSS_LOCK_LIVE_MASK       BIT(0)
> +#define SYS_APLL_LOSS_LOCK_LIVE_LOCKED     0
> +#define SYS_APLL_LOSS_LOCK_LIVE_UNLOCKED   1
> +
>  #endif
> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> index 051511f..3de8411 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c
> @@ -335,6 +335,79 @@ static int wait_for_boot_status_ready(struct idtcm *idtcm)
>  	return -EBUSY;
>  }
>  
> +static int read_sys_apll_status(struct idtcm *idtcm, u8 *status)
> +{
> +	int err;
> +
> +	err = idtcm_read(idtcm, STATUS, DPLL_SYS_APLL_STATUS, status,
> +			 sizeof(u8));
> +	return err;

Please remove the unnecessary 'err' variable:

	return idtcm_read(..

There are bots scanning the tree for such code simplifications, 
better to get this right from the start than deal with flood of 
simplifications patches.

> +}
> +
> +static int read_sys_dpll_status(struct idtcm *idtcm, u8 *status)
> +{
> +	int err;
> +
> +	err = idtcm_read(idtcm, STATUS, DPLL_SYS_STATUS, status, sizeof(u8));
> +
> +	return err;

same here

> +}
> +
> +static int wait_for_sys_apll_dpll_lock(struct idtcm *idtcm)
> +{
> +	const char *fmt = "%d ms SYS lock timeout: APLL Loss Lock %d  DPLL state %d";
> +	u8 i = LOCK_TIMEOUT_MS / LOCK_POLL_INTERVAL_MS;

Using msleep() and loops is quite inaccurate. I'd recommend you switch
to:

	unsigned long timeout = jiffies + msecs_to_jiffies(LOCK_TIMEOUT_MS);

And then use:

	while (time_is_after_jiffies(timeout))

For the condition.

> +	u8 apll = 0;
> +	u8 dpll = 0;
> +
> +	int err;

No empty lines between variables, please.

> +
> +	do {
> +		err = read_sys_apll_status(idtcm, &apll);
> +

No empty lines between call and the if, please.

> +		if (err)
> +			return err;
> +
> +		err = read_sys_dpll_status(idtcm, &dpll);
> +
> +		if (err)
> +			return err;
> +
> +		apll &= SYS_APLL_LOSS_LOCK_LIVE_MASK;
> +		dpll &= DPLL_SYS_STATE_MASK;
> +
> +		if ((apll == SYS_APLL_LOSS_LOCK_LIVE_LOCKED)

parenthesis around a == b are unnecessary.

> +		    && (dpll == DPLL_STATE_LOCKED)) {
> +			return 0;
> +		} else if ((dpll == DPLL_STATE_FREERUN) ||
> +			   (dpll == DPLL_STATE_HOLDOVER) ||
> +			   (dpll == DPLL_STATE_OPEN_LOOP)) {

same here.

> +			dev_warn(&idtcm->client->dev,
> +				"No wait state: DPLL_SYS_STATE %d", dpll);

It looks like other prints in this function use \n at the end of the
lines, should we keep it consistent?

> +			return -EPERM;
> +		}
> +
> +		msleep(LOCK_POLL_INTERVAL_MS);
> +		i--;
> +

unnecessary empty line

> +	} while (i);
> +
> +	dev_warn(&idtcm->client->dev, fmt, LOCK_TIMEOUT_MS, apll, dpll);

I'd recommend leaving the format in place, that way static code
checkers can validate the arguments.

> +	return -ETIME;

> +}
> +
> +static void wait_for_chip_ready(struct idtcm *idtcm)
> +{
> +	if (wait_for_boot_status_ready(idtcm))
> +		dev_warn(&idtcm->client->dev, "BOOT_STATUS != 0xA0");

no new line?

> +
> +	if (wait_for_sys_apll_dpll_lock(idtcm))
> +		dev_warn(&idtcm->client->dev,
> +			 "Continuing while SYS APLL/DPLL is not locked");

And here.

> +}
> +
>  static int _idtcm_gettime(struct idtcm_channel *channel,
>  			  struct timespec64 *ts)
>  {
> @@ -2235,8 +2308,7 @@ static int idtcm_probe(struct i2c_client *client,
>  		dev_warn(&idtcm->client->dev,
>  			 "loading firmware failed with %d\n", err);
>  
> -	if (wait_for_boot_status_ready(idtcm))
> -		dev_warn(&idtcm->client->dev, "BOOT_STATUS != 0xA0\n");
> +	wait_for_chip_ready(idtcm);
>  
>  	if (idtcm->tod_mask) {
>  		for (i = 0; i < MAX_TOD; i++) {
> diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
> index 645de2c..0233236 100644
> --- a/drivers/ptp/ptp_clockmatrix.h
> +++ b/drivers/ptp/ptp_clockmatrix.h
> @@ -51,6 +51,9 @@
>  #define TOD_WRITE_OVERHEAD_COUNT_MAX		(2)
>  #define TOD_BYTE_COUNT				(11)
>  
> +#define LOCK_TIMEOUT_MS			(2000)
> +#define LOCK_POLL_INTERVAL_MS		(10)
> +
>  #define PEROUT_ENABLE_OUTPUT_MASK	(0xdeadbeef)
>  
>  #define IDTCM_MAX_WRITE_COUNT		(512)
> @@ -105,6 +108,18 @@ enum scsr_tod_write_type_sel {
>  	SCSR_TOD_WR_TYPE_SEL_MAX = SCSR_TOD_WR_TYPE_SEL_DELTA_MINUS,
>  };
>  
> +/* Values STATUS.DPLL_SYS_STATUS.DPLL_SYS_STATE */
> +enum dpll_state {
> +	DPLL_STATE_MIN = 0,
> +	DPLL_STATE_FREERUN = DPLL_STATE_MIN,
> +	DPLL_STATE_LOCKACQ = 1,
> +	DPLL_STATE_LOCKREC = 2,
> +	DPLL_STATE_LOCKED = 3,
> +	DPLL_STATE_HOLDOVER = 4,
> +	DPLL_STATE_OPEN_LOOP = 5,
> +	DPLL_STATE_MAX = DPLL_STATE_OPEN_LOOP,
> +};
> +
>  struct idtcm;
>  
>  struct idtcm_channel {