On Sat, 13 Feb 2021 00:06:04 -0500 vincent.cheng...@renesas.com wrote: > From: Vincent Cheng <vincent.cheng...@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...@renesas.com> > Acked-by: Richard Cochran <richardcoch...@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 {