Re: [Patch net-next v3 10/13] net: dsa: microchip: ptp: add periodic output signal

From: Vladimir Oltean
Date: Fri Dec 09 2022 - 10:16:50 EST

Next message: David Hildenbrand: "Re: [PATCH v2 08/10] mm/hugetlb: Make walk_hugetlb_range() safe to pmd unshare"
Previous message: Sergei Shtepa: "[PATCH v2 19/21] block, blksnap: Kconfig and Makefile"
In reply to: Arun Ramadoss: "[Patch net-next v3 10/13] net: dsa: microchip: ptp: add periodic output signal"
Next in thread: Arun Ramadoss: "[Patch net-next v3 11/13] net: dsa: microchip: ptp: add support for perout programmable pins"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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
>

Next message: David Hildenbrand: "Re: [PATCH v2 08/10] mm/hugetlb: Make walk_hugetlb_range() safe to pmd unshare"
Previous message: Sergei Shtepa: "[PATCH v2 19/21] block, blksnap: Kconfig and Makefile"
In reply to: Arun Ramadoss: "[Patch net-next v3 10/13] net: dsa: microchip: ptp: add periodic output signal"
Next in thread: Arun Ramadoss: "[Patch net-next v3 11/13] net: dsa: microchip: ptp: add support for perout programmable pins"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]