[PATCH v2 net-next 0/3] ptp: ptp_clockmatrix: Fix output 1 PPS alignment.

[PATCH v2 net-next 0/3] ptp: ptp_clockmatrix: Fix output 1 PPS alignment.

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

This series fixes a race condition that may result in the output clock
not aligned to internal 1 PPS clock.

Part of device initialization is to align the rising edge of output
clocks to the internal rising edge of the 1 PPS clock.  If the system
APLL and DPLL are not locked when this alignment occurs, the alignment
fails and a fixed offset between the internal 1 PPS clock and the
output clock occurs.

If a clock is dynamically enabled after power-up, the output clock
also needs to be aligned to the internal 1 PPS clock.

v2:
Suggested by: Richard Cochran <richardcochran@gmail.com>
- Added const to "char * fmt"
- Break unrelated header change into separate patch

Vincent Cheng (3):
  ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.
  ptp: ptp_clockmatrix: Add alignment of 1 PPS to idtcm_perout_enable.
  ptp: ptp_clockmatrix: Remove unused header declarations.

 drivers/ptp/idt8a340_reg.h    | 10 +++++
 drivers/ptp/ptp_clockmatrix.c | 92 ++++++++++++++++++++++++++++++++++++++++---
 drivers/ptp/ptp_clockmatrix.h | 17 +++++++-
 3 files changed, 112 insertions(+), 7 deletions(-)

-- 
2.7.4

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

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

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@renesas.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/idt8a340_reg.h    | 10 ++++++
 drivers/ptp/ptp_clockmatrix.c | 76 +++++++++++++++++++++++++++++++++++++++++--
 drivers/ptp/ptp_clockmatrix.h | 15 +++++++++
 3 files changed, 99 insertions(+), 2 deletions(-)

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;
+}
+
+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;
+}
+
+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;
+	u8 apll = 0;
+	u8 dpll = 0;
+
+	int err;
+
+	do {
+		err = read_sys_apll_status(idtcm, &apll);
+
+		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)
+		    && (dpll == DPLL_STATE_LOCKED)) {
+			return 0;
+		} else if ((dpll == DPLL_STATE_FREERUN) ||
+			   (dpll == DPLL_STATE_HOLDOVER) ||
+			   (dpll == DPLL_STATE_OPEN_LOOP)) {
+			dev_warn(&idtcm->client->dev,
+				"No wait state: DPLL_SYS_STATE %d", dpll);
+			return -EPERM;
+		}
+
+		msleep(LOCK_POLL_INTERVAL_MS);
+		i--;
+
+	} while (i);
+
+	dev_warn(&idtcm->client->dev, fmt, LOCK_TIMEOUT_MS, apll, dpll);
+
+	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");
+
+	if (wait_for_sys_apll_dpll_lock(idtcm))
+		dev_warn(&idtcm->client->dev,
+			 "Continuing while SYS APLL/DPLL is not locked");
+}
+
 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 {
-- 
2.7.4

[PATCH v2 net-next 2/3] ptp: ptp_clockmatrix: Add alignment of 1 PPS to idtcm_perout_enable.

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

When enabling output using PTP_CLK_REQ_PEROUT, need to align the output
clock to the internal 1 PPS clock.

Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_clockmatrix.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index 3de8411..a83ba4b 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -1401,13 +1401,23 @@ static int idtcm_perout_enable(struct idtcm_channel *channel,
 			       bool enable,
 			       struct ptp_perout_request *perout)
 {
+	struct idtcm *idtcm = channel->idtcm;
 	unsigned int flags = perout->flags;
+	struct timespec64 ts = {0, 0};
+	int err;
 
 	if (flags == PEROUT_ENABLE_OUTPUT_MASK)
-		return idtcm_output_mask_enable(channel, enable);
+		err = idtcm_output_mask_enable(channel, enable);
+	else
+		err = idtcm_output_enable(channel, enable, perout->index);
+
+	if (err) {
+		dev_err(&idtcm->client->dev, "Unable to set output enable");
+		return err;
+	}
 
-	/* Enable/disable individual output instead */
-	return idtcm_output_enable(channel, enable, perout->index);
+	/* Align output to internal 1 PPS */
+	return _idtcm_settime(channel, &ts, SCSR_TOD_WR_TYPE_SEL_DELTA_PLUS);
 }
 
 static int idtcm_get_pll_mode(struct idtcm_channel *channel,
-- 
2.7.4

[PATCH v2 net-next 3/3] ptp: ptp_clockmatrix: Remove unused header declarations.

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

Removed unused header declarations.

Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
---
 drivers/ptp/ptp_clockmatrix.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
index 0233236..fb32327 100644
--- a/drivers/ptp/ptp_clockmatrix.h
+++ b/drivers/ptp/ptp_clockmatrix.h
@@ -15,7 +15,6 @@
 #define FW_FILENAME	"idtcm.bin"
 #define MAX_TOD		(4)
 #define MAX_PLL		(8)
-#define MAX_OUTPUT	(12)
 
 #define MAX_ABS_WRITE_PHASE_PICOSECONDS (107374182350LL)
 
@@ -138,7 +137,6 @@ struct idtcm_channel {
 	enum pll_mode		pll_mode;
 	u8			pll;
 	u16			output_mask;
-	u8			output_phase_adj[MAX_OUTPUT][4];
 };
 
 struct idtcm {
-- 
2.7.4

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@renesas.com wrote:
> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
> 
> 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@renesas.com>
> Acked-by: Richard Cochran <richardcochran@gmail.com>

> 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 {

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

[Vincent Cheng]

On Mon, Feb 15, 2021 at 02:48:22PM EST, Jakub Kicinski wrote:
>On Sat, 13 Feb 2021 00:06:04 -0500 vincent.cheng.xh@renesas.com wrote:

>> +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:

Yes, fixed in v3 patch.

>	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.

Totally, agree.  Thanks.

>> +{
>> +	int err;
>> +
>> +	err = idtcm_read(idtcm, STATUS, DPLL_SYS_STATUS, status, sizeof(u8));
>> +
>> +	return err;
>
>same here

Fixed in v3 patch.

>
>> +}
>> +
>> +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))
>

To clarify, the suggestion is to use jiffies for accuracy, but
the msleep(LOCK_POLL_INTERVAL_MS) remains to prevent the do-while
loop from becoming a busy-wait loop.

#define LOCK_TIMEOUT_MS			(2000)
#define LOCK_POLL_INTERVAL_MS		(10)

unsigned long timeout = jiffies + msecs_to_jiffies(LOCK_TIMEOUT_MS);

do {
	...
        /*refresh 'locked' variable */
	if (locked)
		return 0;
	
	msleep(LOCK_POLL_INTERVAL_MS);

} while (time_is_after_jiffies(timeout));


>For the condition.
>
>> +	u8 apll = 0;
>> +	u8 dpll = 0;
>> +
>> +	int err;
>
>No empty lines between variables, please.

Yes, will fix in v3 patch.

>> +
>> +	do {
>> +		err = read_sys_apll_status(idtcm, &apll);
>> +
>
>No empty lines between call and the if, please.

Okay, will fix in v3 patch.

>> +		dpll &= DPLL_SYS_STATE_MASK;
>> +
>> +		if ((apll == SYS_APLL_LOSS_LOCK_LIVE_LOCKED)
>
>parenthesis around a == b are unnecessary.

Yes, will fix in v3 patch.


>> +		} else if ((dpll == DPLL_STATE_FREERUN) ||
>> +			   (dpll == DPLL_STATE_HOLDOVER) ||
>> +			   (dpll == DPLL_STATE_OPEN_LOOP)) {
>
>same here.

Ditto.

>
>> +			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?

Looks like the \n is not needed for dev_warn.  Will remove \n 
of existing messages for consistency.

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

Yes, will fix in v3 patch.

>> +	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.

Good point.  The fmt was used to keep 80 column rule.
But now that 100 column rule is in place, the fmt
workaround is not needed anymore.  Will fix in v3 patch.

>> +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?

Nope.  Tried an experiment and \n is not neeeded.

	dev_warn(&idtcm->client->dev, "debug: has \\n at end\n");
	dev_warn(&idtcm->client->dev, "debug: does not have \\n at end");
	dev_warn(&idtcm->client->dev, "debug: has \\n\\n at end\n\n");
	dev_warn(&idtcm->client->dev, "debug: hello");
	dev_warn(&idtcm->client->dev, "debug: world");

[   99.069100] idtcm 15-005b: debug: has \n at end
[   99.073623] idtcm 15-005b: debug: does not have \n at end
[   99.079017] idtcm 15-005b: debug: has \n\n at end
[   99.079017]
[   99.085194] idtcm 15-005b: debug: hello
[   99.089025] idtcm 15-005b: debug: world

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

\n not needed.

Thank-you for the comments, helps make cleaner code.

Vincent

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

[Jakub Kicinski]

On Tue, 16 Feb 2021 13:12:29 -0500 Vincent Cheng wrote:
> >> +}
> >> +
> >> +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))
> >  
> 
> To clarify, the suggestion is to use jiffies for accuracy, but
> the msleep(LOCK_POLL_INTERVAL_MS) remains to prevent the do-while
> loop from becoming a busy-wait loop.
> 
> #define LOCK_TIMEOUT_MS			(2000)
> #define LOCK_POLL_INTERVAL_MS		(10)
> 
> unsigned long timeout = jiffies + msecs_to_jiffies(LOCK_TIMEOUT_MS);
> 
> do {
> 	...
>         /*refresh 'locked' variable */
> 	if (locked)
> 		return 0;
> 	
> 	msleep(LOCK_POLL_INTERVAL_MS);
> 
> } while (time_is_after_jiffies(timeout));

Yes, exactly, sorry for lack of clarity.

> >> +			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?  
> 
> Looks like the \n is not needed for dev_warn.  Will remove \n 
> of existing messages for consistency.
>
> >> +	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.  
> 
> Good point.  The fmt was used to keep 80 column rule.
> But now that 100 column rule is in place, the fmt
> workaround is not needed anymore.  Will fix in v3 patch.

Log strings / formats are a well known / long standing exception 
to the line length limit. No need to worry about that.

> >> +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?  
> 
> Nope.  Tried an experiment and \n is not neeeded.
> 
> 	dev_warn(&idtcm->client->dev, "debug: has \\n at end\n");
> 	dev_warn(&idtcm->client->dev, "debug: does not have \\n at end");
> 	dev_warn(&idtcm->client->dev, "debug: has \\n\\n at end\n\n");
> 	dev_warn(&idtcm->client->dev, "debug: hello");
> 	dev_warn(&idtcm->client->dev, "debug: world");
> 
> [   99.069100] idtcm 15-005b: debug: has \n at end
> [   99.073623] idtcm 15-005b: debug: does not have \n at end
> [   99.079017] idtcm 15-005b: debug: has \n\n at end
> [   99.079017]
> [   99.085194] idtcm 15-005b: debug: hello
> [   99.089025] idtcm 15-005b: debug: world
> 
> >> +
> >> +	if (wait_for_sys_apll_dpll_lock(idtcm))
> >> +		dev_warn(&idtcm->client->dev,
> >> +			 "Continuing while SYS APLL/DPLL is not locked");  
> >
> >And here.  
> 
> \n not needed.

Right, it's not needed I was just commenting that the new cases are not
consistent with existing code in the file, but removing \n everywhere
sounds fine as well.