Message-ID: <20221209151638.cy7iwnjbqgju3tj3@skbuf>
Date:   Fri, 9 Dec 2022 17:16:38 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com,
        andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, linux@...linux.org.uk,
        Tristram.Ha@...rochip.com, richardcochran@...il.com,
        ceggers@...i.de
Subject: Re: [Patch net-next v3 10/13] net: dsa: microchip: ptp: add periodic
 output signal

On Fri, Dec 09, 2022 at 12:54:34PM +0530, Arun Ramadoss wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index c9da2a735165..6d7edc81909e 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -31,6 +31,252 @@
>  
>  #define KSZ_PTP_INT_START 13
>  
> +static int _ksz_ptp_gettime(struct ksz_device *dev, struct timespec64 *ts);

Can you enforce a natural function ordering from the beginning such that
forward declarations are not needed?

> +
> +static int ksz_ptp_tou_reset(struct ksz_device *dev, u8 unit)
> +{
> +	u32 data;
> +	int ret;
> +
> +	/* Reset trigger unit (clears TRIGGER_EN, but not GPIOSTATx) */
> +	ret = ksz_rmw32(dev, REG_PTP_CTRL_STAT__4, TRIG_RESET, TRIG_RESET);
> +
> +	data = FIELD_PREP(TRIG_DONE_M, BIT(unit));
> +	ret = ksz_write32(dev, REG_PTP_TRIG_STATUS__4, data);
> +	if (ret)
> +		return ret;
> +
> +	data = FIELD_PREP(TRIG_INT_M, BIT(unit));
> +	ret = ksz_write32(dev, REG_PTP_INT_STATUS__4, data);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear reset and set GPIO direction */
> +	return ksz_rmw32(dev, REG_PTP_CTRL_STAT__4, (TRIG_RESET | TRIG_ENABLE),
> +			 0);
> +}
> +
> +static int ksz_ptp_tou_pulse_verify(u64 pulse_ns)
> +{
> +	u32 data;
> +
> +	if (pulse_ns & 0x3)
> +		return -EINVAL;
> +
> +	data = (pulse_ns / 8);
> +	if (!FIELD_FIT(TRIG_PULSE_WIDTH_M, data))
> +		return -ERANGE;
> +
> +	return 0;
> +}
> +
> +static int ksz_ptp_tou_target_time_set(struct ksz_device *dev,
> +				       struct timespec64 const *ts)
> +{
> +	int ret;
> +
> +	/* Hardware has only 32 bit */
> +	if ((ts->tv_sec & 0xffffffff) != ts->tv_sec)
> +		return -EINVAL;
> +
> +	ret = ksz_write32(dev, REG_TRIG_TARGET_NANOSEC, ts->tv_nsec);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_write32(dev, REG_TRIG_TARGET_SEC, ts->tv_sec);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ksz_ptp_tou_start(struct ksz_device *dev, u8 unit)
> +{
> +	u32 data;
> +	int ret;
> +
> +	ret = ksz_rmw32(dev, REG_PTP_CTRL_STAT__4, TRIG_ENABLE, TRIG_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	/* Check error flag:
> +	 * - the ACTIVE flag is NOT cleared an error!
> +	 */
> +	ret = ksz_read32(dev, REG_PTP_TRIG_STATUS__4, &data);
> +	if (ret)
> +		return ret;
> +
> +	if (FIELD_GET(TRIG_ERROR_M, data) & (1 << unit)) {
> +		dev_err(dev->dev, "%s: Trigger unit%d error!\n", __func__,
> +			unit);
> +		ret = -EIO;
> +		/* Unit will be reset on next access */
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ksz_ptp_configure_perout(struct ksz_device *dev,
> +				    u32 cycle_width_ns, u32 pulse_width_ns,
> +				    struct timespec64 const *target_time,
> +				    u8 index)
> +{
> +	u32 data;
> +	int ret;
> +
> +	data = FIELD_PREP(TRIG_NOTIFY, 1) |
> +		FIELD_PREP(TRIG_GPO_M, index) |
> +		FIELD_PREP(TRIG_PATTERN_M, TRIG_POS_PERIOD);
> +	ret = ksz_write32(dev, REG_TRIG_CTRL__4, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_write32(dev, REG_TRIG_CYCLE_WIDTH, cycle_width_ns);
> +	if (ret)
> +		return ret;
> +
> +	/* Set cycle count 0 - Infinite */
> +	ret = ksz_rmw32(dev, REG_TRIG_CYCLE_CNT, TRIG_CYCLE_CNT_M, 0);
> +	if (ret)
> +		return ret;
> +
> +	data = (pulse_width_ns / 8);
> +	ret = ksz_write32(dev, REG_TRIG_PULSE_WIDTH__4, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_ptp_tou_target_time_set(dev, target_time);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +#define KSZ_PEROUT_VALID_FLAGS ( \
> +				 PTP_PEROUT_DUTY_CYCLE \
> +				 )

Do you have plans for more perout flags? If not, it looks odd for this
to unravel on 3 lines.

> +
> +static int ksz_ptp_enable_perout(struct ksz_device *dev,
> +				 struct ptp_perout_request const *request,
> +				 int on)
> +{
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +	u64 cycle_width_ns;
> +	u64 pulse_width_ns;
> +	int pin = 0;
> +	u32 data32;
> +	int ret;
> +
> +	if (request->flags & ~KSZ_PEROUT_VALID_FLAGS)
> +		return -EINVAL;
> +
> +	if (ptp_data->tou_mode != KSZ_PTP_TOU_PEROUT &&
> +	    ptp_data->tou_mode != KSZ_PTP_TOU_IDLE)
> +		return -EBUSY;
> +
> +	data32 = FIELD_PREP(PTP_GPIO_INDEX, pin) |
> +		 FIELD_PREP(PTP_TOU_INDEX, request->index);
> +	ret = ksz_rmw32(dev, REG_PTP_UNIT_INDEX__4,
> +			PTP_GPIO_INDEX | PTP_TOU_INDEX, data32);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_ptp_tou_reset(dev, request->index);
> +	if (ret)
> +		return ret;
> +
> +	if (!on) {
> +		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
> +		return 0;
> +	}
> +
> +	ptp_data->perout_target_time_first.tv_sec  = request->start.sec;
> +	ptp_data->perout_target_time_first.tv_nsec = request->start.nsec;
> +
> +	ptp_data->perout_period.tv_sec = request->period.sec;
> +	ptp_data->perout_period.tv_nsec = request->period.nsec;
> +
> +	cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
> +	if ((cycle_width_ns & TRIG_CYCLE_WIDTH_M) != cycle_width_ns)
> +		return -EINVAL;
> +
> +	if (request->flags & PTP_PEROUT_DUTY_CYCLE) {
> +		pulse_width_ns = request->on.sec * NSEC_PER_SEC +
> +			request->on.nsec;
> +	} else {
> +		/* Use a duty cycle of 50%. Maximum pulse width supported by the
> +		 * hardware is a little bit more than 125 ms.
> +		 */
> +		pulse_width_ns = min_t(u64,
> +				       (request->period.sec * NSEC_PER_SEC
> +					+ request->period.nsec) / 2
> +				       / 8 * 8,

Coding style nitpick: operators aren't generally put at the beginning of
a new line, but at the end of the previous one.

> +				       125000000LL);

You may want some helper macros for all of these magic constants, and
maybe a helper that transforms the perout period to a pulse width?

> +	}
> +
> +	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_ptp_configure_perout(dev, cycle_width_ns, pulse_width_ns,
> +				       &ptp_data->perout_target_time_first,
> +				       pin);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_ptp_tou_start(dev, request->index);
> +	if (ret)
> +		return ret;
> +
> +	ptp_data->tou_mode = KSZ_PTP_TOU_PEROUT;
> +
> +	return 0;
> +}
> +
> +static int ksz_ptp_restart_perout(struct ksz_device *dev)
> +{
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +	s64 now_ns, first_ns, period_ns, next_ns;
> +	struct ptp_perout_request request;
> +	struct timespec64 next;
> +	struct timespec64 now;
> +	unsigned int count;
> +	int ret;
> +
> +	ret = _ksz_ptp_gettime(dev, &now);
> +	if (ret)
> +		return ret;
> +
> +	now_ns = timespec64_to_ns(&now);
> +	first_ns = timespec64_to_ns(&ptp_data->perout_target_time_first);
> +
> +	/* Calculate next perout event based on start time and period */
> +	period_ns = timespec64_to_ns(&ptp_data->perout_period);
> +
> +	if (first_ns < now_ns) {
> +		count = div_u64(now_ns - first_ns, period_ns);
> +		next_ns = first_ns + count * period_ns;
> +	} else {
> +		next_ns = first_ns;
> +	}
> +
> +	/* Ensure 100 ms guard time prior next event */
> +	while (next_ns < now_ns + 100000000)
> +		next_ns += period_ns;
> +
> +	/* Restart periodic output signal */
> +	next = ns_to_timespec64(next_ns);
> +	request.start.sec  = next.tv_sec;
> +	request.start.nsec = next.tv_nsec;
> +	request.period.sec  = ptp_data->perout_period.tv_sec;
> +	request.period.nsec = ptp_data->perout_period.tv_nsec;
> +	request.index = 0;
> +	request.flags = 0;
> +
> +	return ksz_ptp_enable_perout(dev, &request, 1);
> +}
> +
>  static int ksz_ptp_enable_mode(struct ksz_device *dev)
>  {
>  	struct ksz_tagger_data *tagger_data = ksz_tagger_data(dev->ds);
> @@ -396,6 +642,20 @@ static int ksz_ptp_settime(struct ptp_clock_info *ptp,
>  	if (ret)
>  		goto unlock;
>  
> +	switch (ptp_data->tou_mode) {
> +	case KSZ_PTP_TOU_IDLE:
> +		break;
> +
> +	case KSZ_PTP_TOU_PEROUT:
> +		dev_info(dev->dev, "Restarting periodic output signal\n");
> +
> +		ret = ksz_ptp_restart_perout(dev);
> +		if (ret)
> +			goto unlock;
> +
> +		break;
> +	}
> +
>  	spin_lock_bh(&ptp_data->clock_lock);
>  	ptp_data->clock_time = *ts;
>  	spin_unlock_bh(&ptp_data->clock_lock);
> @@ -489,6 +749,20 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  	if (ret)
>  		goto unlock;
>  
> +	switch (ptp_data->tou_mode) {
> +	case KSZ_PTP_TOU_IDLE:
> +		break;
> +
> +	case KSZ_PTP_TOU_PEROUT:
> +		dev_info(dev->dev, "Restarting periodic output signal\n");

How about absorbing the dev_info() into the ksz_ptp_restart_perout()
call, so you don't have to duplicate it?

> +
> +		ret = ksz_ptp_restart_perout(dev);
> +		if (ret)
> +			goto unlock;
> +
> +		break;
> +	}
> +
>  	spin_lock_bh(&ptp_data->clock_lock);
>  	ptp_data->clock_time = timespec64_add(ptp_data->clock_time, delta64);
>  	spin_unlock_bh(&ptp_data->clock_lock);
> @@ -498,6 +772,26 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  	return ret;
>  }
>  
> +static int ksz_ptp_enable(struct ptp_clock_info *ptp,
> +			  struct ptp_clock_request *req, int on)
> +{
> +	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> +	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> +	int ret;
> +
> +	switch (req->type) {
> +	case PTP_CLK_REQ_PEROUT:
> +		mutex_lock(&ptp_data->lock);
> +		ret = ksz_ptp_enable_perout(dev, &req->perout, on);
> +		mutex_unlock(&ptp_data->lock);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ret;
> +}
> +
>  /*  Function is pointer to the do_aux_work in the ptp_clock capability */
>  static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
>  {
> @@ -546,6 +840,8 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
>  	ptp_data->caps.adjfine		= ksz_ptp_adjfine;
>  	ptp_data->caps.adjtime		= ksz_ptp_adjtime;
>  	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
> +	ptp_data->caps.enable		= ksz_ptp_enable;
> +	ptp_data->caps.n_per_out	= 3;
>  
>  	ret = ksz_ptp_start_clock(dev);
>  	if (ret)
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
> index 0b14aed71ec2..9451e3a76375 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.h
> +++ b/drivers/net/dsa/microchip/ksz_ptp.h
> @@ -12,6 +12,11 @@
>  
>  #include <linux/ptp_clock_kernel.h>
>  
> +enum ksz_ptp_tou_mode {
> +	KSZ_PTP_TOU_IDLE,
> +	KSZ_PTP_TOU_PEROUT,
> +};
> +
>  struct ksz_ptp_data {
>  	struct ptp_clock_info caps;
>  	struct ptp_clock *clock;
> @@ -20,6 +25,9 @@ struct ksz_ptp_data {
>  	/* lock for accessing the clock_time */
>  	spinlock_t clock_lock;
>  	struct timespec64 clock_time;
> +	enum ksz_ptp_tou_mode tou_mode;
> +	struct timespec64 perout_target_time_first;  /* start of first pulse */
> +	struct timespec64 perout_period;
>  };
>  
>  int ksz_ptp_clock_register(struct dsa_switch *ds);
> diff --git a/drivers/net/dsa/microchip/ksz_ptp_reg.h b/drivers/net/dsa/microchip/ksz_ptp_reg.h
> index abe95bbefc12..dbccfedf89e4 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp_reg.h
> +++ b/drivers/net/dsa/microchip/ksz_ptp_reg.h
> @@ -49,6 +49,69 @@
>  #define PTP_MASTER			BIT(1)
>  #define PTP_1STEP			BIT(0)
>  
> +#define REG_PTP_UNIT_INDEX__4		0x0520
> +
> +#define PTP_GPIO_INDEX			GENMASK(19, 16)
> +#define PTP_TSI_INDEX			BIT(8)
> +#define PTP_TOU_INDEX			GENMASK(1, 0)
> +
> +#define REG_PTP_TRIG_STATUS__4		0x0524
> +
> +#define TRIG_ERROR_M			GENMASK(18, 16)
> +#define TRIG_DONE_M			GENMASK(2, 0)
> +
> +#define REG_PTP_INT_STATUS__4		0x0528
> +
> +#define TRIG_INT_M			GENMASK(18, 16)
> +#define TS_INT_M			GENMASK(1, 0)
> +
> +#define REG_PTP_CTRL_STAT__4           0x052C
> +
> +#define GPIO_IN                        BIT(7)
> +#define GPIO_OUT                       BIT(6)
> +#define TS_INT_ENABLE                  BIT(5)
> +#define TRIG_ACTIVE                    BIT(4)
> +#define TRIG_ENABLE                    BIT(3)
> +#define TRIG_RESET                     BIT(2)
> +#define TS_ENABLE                      BIT(1)
> +#define TS_RESET                       BIT(0)
> +
> +#define REG_TRIG_TARGET_NANOSEC        0x0530
> +#define REG_TRIG_TARGET_SEC            0x0534
> +
> +#define REG_TRIG_CTRL__4               0x0538
> +
> +#define TRIG_CASCADE_ENABLE            BIT(31)
> +#define TRIG_CASCADE_TAIL              BIT(30)
> +#define TRIG_CASCADE_UPS_M             GENMASK(29, 26)
> +#define TRIG_NOW                       BIT(25)
> +#define TRIG_NOTIFY                    BIT(24)
> +#define TRIG_EDGE                      BIT(23)
> +#define TRIG_PATTERN_M		       GENMASK(22, 20)

Nitpick: strange combination of tabs and spaces.

> +#define TRIG_NEG_EDGE                  0
> +#define TRIG_POS_EDGE                  1
> +#define TRIG_NEG_PULSE                 2
> +#define TRIG_POS_PULSE                 3
> +#define TRIG_NEG_PERIOD                4
> +#define TRIG_POS_PERIOD                5
> +#define TRIG_REG_OUTPUT                6
> +#define TRIG_GPO_M		       GENMASK(19, 16)
> +#define TRIG_CASCADE_ITERATE_CNT_M     GENMASK(15, 0)
> +
> +#define REG_TRIG_CYCLE_WIDTH           0x053C
> +#define TRIG_CYCLE_WIDTH_M	       GENMASK(31, 0)
> +
> +#define REG_TRIG_CYCLE_CNT             0x0540
> +
> +#define TRIG_CYCLE_CNT_M	       GENMASK(31, 16)
> +#define TRIG_BIT_PATTERN_M             GENMASK(15, 0)
> +
> +#define REG_TRIG_ITERATE_TIME          0x0544
> +
> +#define REG_TRIG_PULSE_WIDTH__4        0x0548
> +
> +#define TRIG_PULSE_WIDTH_M             GENMASK(23, 0)
> +
>  /* Port PTP Register */
>  #define REG_PTP_PORT_RX_DELAY__2	0x0C00
>  #define REG_PTP_PORT_TX_DELAY__2	0x0C02
> -- 
> 2.36.1
> 

Powered by blists - more mailing lists