Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Apr 2023 11:37:47 +0200
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Shmuel Hazan <shmuel.h@siklu.com>
CC:     Russell King <linux@armlinux.org.uk>,
        Marcin Wojtas <mw@semihalf.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Richard Cochran <richardcochran@gmail.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] net: mvpp2: tai: add extts support
Message-ID: <20230418093747.jxxvoizofd6cgk6v@soft-dev3-1>
References: <20230417170741.1714310-1-shmuel.h@siklu.com>
 <20230417170741.1714310-3-shmuel.h@siklu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20230417170741.1714310-3-shmuel.h@siklu.com>
Precedence: bulk

The 04/17/2023 20:07, Shmuel Hazan wrote:
> 
> This commit add support for capturing a timestamp in which the PTP_PULSE
> pin, received a signal.
> 
> This feature is needed in order to synchronize multiple clocks in the
> same board, using tools like ts2phc from the linuxptp project.
> 
> On the Armada 8040, this is the only way to do so as a result of
> multiple erattas with the PTP_PULSE_IN interface that was designed to
> synchronize the TAI on an external PPS signal (the errattas are
> FE-6856276, FE-7382160 from document MV-S501388-00).
> 
> This patch introduces a pinctrl configuration "extts" that will be
> selected once the user had enabled extts, and then will be returned back
> to the "default" pinctrl config once it has been disabled. Additionally
> these configurations will be also used in any case that the user asks us
> to perform any action that involves "triggerering" the TAI subsystem, in
> order to avoid a case where the external trigger would trigger with the
> wrong action.
> 
> This pinctrl mess is needed due to the fact that there is no way for us
> to distinguish betwee an external trigger (e.g. from the PTP_PULSE_IN
> pin) or an internal one, triggered by the registers.
> 
> This feature has been tested on an Aramda
> 8040 based board, with linuxptp 3.1.1's ts2phc.

It looks good to me, just one more questions bellow.
Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> 
> Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com>
> ---
>  .../net/ethernet/marvell/mvpp2/mvpp2_tai.c    | 304 ++++++++++++++++--
>  1 file changed, 273 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> index 2e3d43b1bac1..ff57075c6ebc 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
> @@ -3,8 +3,11 @@
>   * Marvell PP2.2 TAI support
>   *
>   * Note:
> - *   Do NOT use the event capture support.
> - *   Do Not even set the MPP muxes to allow PTP_EVENT_REQ to be used.
> + *   In order to use the event capture support, please see the example
> + *   in marvell,pp2.yaml.
> + *   Do not manually (e.g. without pinctrl-1, as described in
> + *   marvell,pp2.yaml) set the MPP muxes to allow PTP_EVENT_REQ to be
> + *   used.
>   *   It will disrupt the operation of this driver, and there is nothing
>   *   that this driver can do to prevent that.  Even using PTP_EVENT_REQ
>   *   as an output will be seen as a trigger input, which can't be masked.
> @@ -33,6 +36,8 @@
>   * Consequently, we support none of these.
>   */
>  #include <linux/io.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/ptp_clock.h>
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/slab.h>
> 
> @@ -53,6 +58,10 @@
>  #define TCSR_CAPTURE_1_VALID           BIT(1)
>  #define TCSR_CAPTURE_0_VALID           BIT(0)
> 
> +#define MVPP2_PINCTRL_EXTTS_STATE              "extts"
> +#define MAX_PINS 1
> +#define EXTTS_PERIOD_MS 95

How have you come with this 95 value?

> +
>  struct mvpp2_tai {
>         struct ptp_clock_info caps;
>         struct ptp_clock *ptp_clock;
> @@ -61,7 +70,12 @@ struct mvpp2_tai {
>         u64 period;             // nanosecond period in 32.32 fixed point
>         /* This timestamp is updated every two seconds */
>         struct timespec64 stamp;
> +       struct pinctrl *extts_pinctrl;
> +       struct pinctrl_state *default_pinctrl_state;
> +       struct pinctrl_state *extts_pinctrl_state;
> +       struct ptp_pin_desc pin_config[MAX_PINS];
>         u16 poll_worker_refcount;
> +       bool extts_enabled:1;
>  };
> 
>  static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
> @@ -73,6 +87,38 @@ static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set)
>         writel(val, reg);
>  }
> 
> +/* mvpp2_tai_{pause,resume}_external_trigger are used as guards
> + * to mask external triggers where it is undesirable. For example,
> + * in case that the action is "increment", we may want to perform it
> + * once; however, we may trigger it once internally and once from
> + * an external pulse, which will cause an issue.
> + * In order to work around this issue, we need perform the following sequence:
> + *  1. call mvpp2_tai_pause_external_trigger
> + *  2. save the current trigger operation.
> + *  3. update the trigger operation.
> + *  4. perform an internal trigger.
> + *  5. restore the previous trigger operation.
> + *  6. call mvpp2_tai_restore_external_trigger.
> + */
> +static int mvpp2_tai_pause_external_trigger(struct mvpp2_tai *tai)
> +{
> +       if (tai->extts_enabled && tai->extts_pinctrl &&
> +           tai->extts_pinctrl_state)
> +               return pinctrl_select_state(tai->extts_pinctrl,
> +                                           tai->default_pinctrl_state);
> +
> +       return 0;
> +}
> +
> +static int mvpp2_tai_resume_external_trigger(struct mvpp2_tai *tai)
> +{
> +       if (tai->extts_enabled && tai->extts_pinctrl &&
> +           tai->extts_pinctrl_state)
> +               return pinctrl_select_state(tai->extts_pinctrl,
> +                                           tai->extts_pinctrl_state);
> +       return 0;
> +}
> +
>  static void mvpp2_tai_write(u32 val, void __iomem *reg)
>  {
>         writel_relaxed(val & 0xffff, reg);
> @@ -102,6 +148,28 @@ static void mvpp22_tai_read_ts(struct timespec64 *ts, void __iomem *base)
>         readl_relaxed(base + 24);
>  }
> 
> +static int mvpp22_tai_try_read_ts(struct timespec64 *ts, void __iomem *base)
> +{
> +       long tcsr = readl(base + MVPP22_TAI_TCSR);
> +       /* If both captures are not valid, return EBUSY */
> +       int ret = -EBUSY;
> +
> +       if (tcsr & TCSR_CAPTURE_1_VALID) {
> +               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV1_SEC_HIGH);
> +               ret = 0;
> +       }
> +
> +       /* If both capture 1 and capture 0 are valid, use capture 0
> +        * but also read and clear capture 1.
> +        */
> +       if (tcsr & TCSR_CAPTURE_0_VALID) {
> +               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV0_SEC_HIGH);
> +               ret = 0;
> +       }
> +
> +       return ret;
> +}
> +
>  static void mvpp2_tai_write_tlv(const struct timespec64 *ts, u32 frac,
>                                 void __iomem *base)
>  {
> @@ -114,16 +182,30 @@ static void mvpp2_tai_write_tlv(const struct timespec64 *ts, u32 frac,
>         mvpp2_tai_write(frac, base + MVPP22_TAI_TLV_FRAC_LOW);
>  }
> 
> -static void mvpp2_tai_op(u32 op, void __iomem *base)
> +static int mvpp2_tai_op(u32 op, void __iomem *base, struct mvpp2_tai *tai)
>  {
> +       u32 reg_val;
> +       int ret;
> +
> +       reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
>         /* Trigger the operation. Note that an external unmaskable
>          * event on PTP_EVENT_REQ will also trigger this action.
> +        * therefore, pause possible (known) external triggers.
>          */
> +       ret = mvpp2_tai_pause_external_trigger(tai);
> +       if (ret)
> +               goto out;
> +
>         mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
>                          TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
>                          op | TCFCR0_TCF_TRIGGER);
> -       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> -                        TCFCR0_TCF_NOP);
> +       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> +                        TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER, reg_val);
> +
> +       mvpp2_tai_resume_external_trigger(tai);
> +
> +out:
> +       return ret;
>  }
> 
>  /* The adjustment has a range of +0.5ns to -0.5ns in 2^32 steps, so has units
> @@ -170,6 +252,7 @@ static int mvpp22_tai_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>         bool neg_adj;
>         s32 frac;
>         u64 val;
> +       int ret;
> 
>         neg_adj = scaled_ppm < 0;
>         if (neg_adj)
> @@ -197,10 +280,10 @@ static int mvpp22_tai_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>         spin_lock_irqsave(&tai->lock, flags);
>         mvpp2_tai_write(frac >> 16, base + MVPP22_TAI_TLV_FRAC_HIGH);
>         mvpp2_tai_write(frac, base + MVPP22_TAI_TLV_FRAC_LOW);
> -       mvpp2_tai_op(TCFCR0_TCF_FREQUPDATE, base);
> +       ret = mvpp2_tai_op(TCFCR0_TCF_FREQUPDATE, base, tai);
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
> -       return 0;
> +       return ret;
>  }
> 
>  static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
> @@ -210,6 +293,7 @@ static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
>         unsigned long flags;
>         void __iomem *base;
>         u32 tcf;
> +       int ret;
> 
>         /* We can't deal with S64_MIN */
>         if (delta == S64_MIN)
> @@ -227,10 +311,10 @@ static int mvpp22_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
>         base = tai->base;
>         spin_lock_irqsave(&tai->lock, flags);
>         mvpp2_tai_write_tlv(&ts, 0, base);
> -       mvpp2_tai_op(tcf, base);
> +       ret = mvpp2_tai_op(tcf, base, tai);
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
> -       return 0;
> +       return ret;
>  }
> 
>  static int mvpp22_tai_gettimex64(struct ptp_clock_info *ptp,
> @@ -240,35 +324,34 @@ static int mvpp22_tai_gettimex64(struct ptp_clock_info *ptp,
>         struct mvpp2_tai *tai = ptp_to_tai(ptp);
>         unsigned long flags;
>         void __iomem *base;
> -       u32 tcsr;
> +       u32 reg_val;
>         int ret;
> 
>         base = tai->base;
>         spin_lock_irqsave(&tai->lock, flags);
>         /* XXX: the only way to read the PTP time is for the CPU to trigger
>          * an event. However, there is no way to distinguish between the CPU
> -        * triggered event, and an external event on PTP_EVENT_REQ. So this
> -        * is incompatible with external use of PTP_EVENT_REQ.
> +        * triggered event, and an external event on PTP_EVENT_REQ. As a result
> +        * we are pausing here external triggers by switching the pinctrl state
> +        * to the default state (if applicable).
>          */
> +       ret = mvpp2_tai_pause_external_trigger(tai);
> +       if (ret)
> +               goto unlock_out;
> +
> +       reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
>         ptp_read_system_prets(sts);
>         mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
>                          TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
>                          TCFCR0_TCF_CAPTURE | TCFCR0_TCF_TRIGGER);
>         ptp_read_system_postts(sts);
> -       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> -                        TCFCR0_TCF_NOP);
> +       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> +                        TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER, reg_val);
> 
> -       tcsr = readl(base + MVPP22_TAI_TCSR);
> -       if (tcsr & TCSR_CAPTURE_1_VALID) {
> -               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV1_SEC_HIGH);
> -               ret = 0;
> -       } else if (tcsr & TCSR_CAPTURE_0_VALID) {
> -               mvpp22_tai_read_ts(ts, base + MVPP22_TAI_TCV0_SEC_HIGH);
> -               ret = 0;
> -       } else {
> -               /* We don't seem to have a reading... */
> -               ret = -EBUSY;
> -       }
> +       ret = mvpp22_tai_try_read_ts(ts, base);
> +       mvpp2_tai_resume_external_trigger(tai);
> +
> +unlock_out:
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
>         return ret;
> @@ -280,32 +363,71 @@ static int mvpp22_tai_settime64(struct ptp_clock_info *ptp,
>         struct mvpp2_tai *tai = ptp_to_tai(ptp);
>         unsigned long flags;
>         void __iomem *base;
> +       u32 reg_val;
> +       int ret;
> 
>         base = tai->base;
>         spin_lock_irqsave(&tai->lock, flags);
>         mvpp2_tai_write_tlv(ts, 0, base);
> 
> +       ret = mvpp2_tai_pause_external_trigger(tai);
> +       if (ret)
> +               goto unlock_out;
> +
>         /* Trigger an update to load the value from the TLV registers
>          * into the TOD counter. Note that an external unmaskable event on
>          * PTP_EVENT_REQ will also trigger this action.
>          */
> +       reg_val = mvpp2_tai_read(base + MVPP22_TAI_TCFCR0);
>         mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> -                        TCFCR0_PHASE_UPDATE_ENABLE |
> -                        TCFCR0_TCF_MASK | TCFCR0_TCF_TRIGGER,
> +                        TCFCR0_PHASE_UPDATE_ENABLE | TCFCR0_TCF_MASK |
> +                                TCFCR0_TCF_TRIGGER,
>                          TCFCR0_TCF_UPDATE | TCFCR0_TCF_TRIGGER);
> -       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> -                        TCFCR0_TCF_NOP);
> +       mvpp2_tai_modify(base + MVPP22_TAI_TCFCR0,
> +                        TCFCR0_PHASE_UPDATE_ENABLE | TCFCR0_TCF_MASK |
> +                                TCFCR0_TCF_TRIGGER,
> +                        reg_val);
> +
> +       mvpp2_tai_resume_external_trigger(tai);
> +
> +unlock_out:
>         spin_unlock_irqrestore(&tai->lock, flags);
> 
> -       return 0;
> +       return ret;
> +}
> +
> +static void do_aux_work_extts(struct mvpp2_tai *tai)
> +{
> +       unsigned long flags;
> +       int ret;
> +
> +       spin_lock_irqsave(&tai->lock, flags);
> +
> +       ret = mvpp22_tai_try_read_ts(&tai->stamp, tai->base);
> +       /* We are not managed to read a TS, try again later */
> +       if (!ret) {
> +               struct ptp_clock_event event;
> +
> +               /* Triggered - save timestamp */
> +               event.type = PTP_CLOCK_EXTTS;
> +               event.index = 0; /* We only have one channel */
> +               event.timestamp = timespec64_to_ns(&tai->stamp);
> +               ptp_clock_event(tai->ptp_clock, &event);
> +       }
> +
> +       spin_unlock_irqrestore(&tai->lock, flags);
>  }
> 
>  static long mvpp22_tai_aux_work(struct ptp_clock_info *ptp)
>  {
>         struct mvpp2_tai *tai = ptp_to_tai(ptp);
> 
> -       mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL);
> +       if (tai->extts_enabled) {
> +               do_aux_work_extts(tai);
> +               return msecs_to_jiffies(EXTTS_PERIOD_MS);
> +       }
> 
> +       mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL);
>         return msecs_to_jiffies(2000);
>  }
> 
> @@ -404,6 +526,94 @@ void mvpp22_tai_stop(struct mvpp2_tai *tai)
>         spin_unlock_irqrestore(&tai->lock, flags);
>  }
> 
> +static void mvpp22_tai_capture_enable(struct mvpp2_tai *tai, bool enable)
> +{
> +       mvpp2_tai_modify(tai->base + MVPP22_TAI_TCFCR0, TCFCR0_TCF_MASK,
> +                        enable ? TCFCR0_TCF_CAPTURE : TCFCR0_TCF_NOP);
> +}
> +
> +static int mvpp22_tai_req_extts_enable(struct mvpp2_tai *tai,
> +                                      struct ptp_clock_request *rq, int on)
> +{
> +       u8 index = rq->extts.index;
> +       int ret = 0;
> +
> +       if (!tai->extts_pinctrl)
> +               return -EINVAL;
> +
> +       /* Reject requests with unsupported flags */
> +       if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE |
> +                               PTP_FALLING_EDGE | PTP_STRICT_FLAGS))
> +               return -EOPNOTSUPP;
> +
> +       /* Reject requests to enable time stamping on falling edge */
> +       if ((rq->extts.flags & PTP_ENABLE_FEATURE) &&
> +           (rq->extts.flags & PTP_FALLING_EDGE))
> +               return -EOPNOTSUPP;
> +
> +       if (index >= MAX_PINS)
> +               return -EINVAL;
> +
> +       if (on)
> +               ret = pinctrl_select_state(tai->extts_pinctrl,
> +                                          tai->extts_pinctrl_state);
> +       else
> +               ret = pinctrl_select_state(tai->extts_pinctrl,
> +                                          tai->default_pinctrl_state);
> +       if (ret)
> +               goto out;
> +
> +       tai->extts_enabled = on != 0;
> +       mvpp22_tai_capture_enable(tai, tai->extts_enabled);
> +
> +       /* We need to enable the poll worker in order for events to be polled */
> +       if (on)
> +               mvpp22_tai_start_unlocked(tai);
> +       else
> +               mvpp22_tai_stop_unlocked(tai);
> +
> +out:
> +       return ret;
> +}
> +
> +static int mvpp22_tai_enable(struct ptp_clock_info *ptp,
> +                            struct ptp_clock_request *rq, int on)
> +{
> +       struct mvpp2_tai *tai = ptp_to_tai(ptp);
> +       int err = -EOPNOTSUPP;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&tai->lock, flags);
> +
> +       switch (rq->type) {
> +       case PTP_CLK_REQ_EXTTS:
> +               err = mvpp22_tai_req_extts_enable(tai, rq, on);
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       spin_unlock_irqrestore(&tai->lock, flags);
> +       return err;
> +}
> +
> +static int mvpp22_tai_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
> +                                enum ptp_pin_function func, unsigned int chan)
> +{
> +       if (chan != 0)
> +               return -1;
> +
> +       switch (func) {
> +       case PTP_PF_NONE:
> +       case PTP_PF_EXTTS:
> +               break;
> +       case PTP_PF_PEROUT:
> +       case PTP_PF_PHYSYNC:
> +               return -1;
> +       }
> +       return 0;
> +}
> +
>  static void mvpp22_tai_remove(void *priv)
>  {
>         struct mvpp2_tai *tai = priv;
> @@ -423,6 +633,24 @@ int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv)
> 
>         spin_lock_init(&tai->lock);
> 
> +       tai->extts_pinctrl = devm_pinctrl_get_select_default(dev);
> +       if (!IS_ERR(tai->extts_pinctrl)) {
> +               tai->default_pinctrl_state = pinctrl_lookup_state
> +                       (tai->extts_pinctrl, PINCTRL_STATE_DEFAULT);
> +               tai->extts_pinctrl_state = pinctrl_lookup_state
> +                       (tai->extts_pinctrl, MVPP2_PINCTRL_EXTTS_STATE);
> +
> +               if (IS_ERR(tai->default_pinctrl_state) ||
> +                   IS_ERR(tai->extts_pinctrl_state)) {
> +                       pinctrl_put(tai->extts_pinctrl);
> +                       tai->extts_pinctrl = NULL;
> +                       tai->default_pinctrl_state = NULL;
> +                       tai->extts_pinctrl_state = NULL;
> +               }
> +       } else {
> +               tai->extts_pinctrl = NULL;
> +       }
> +
>         tai->base = priv->iface_base;
> 
>         /* The step size consists of three registers - a 16-bit nanosecond step
> @@ -458,12 +686,26 @@ int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv)
> 
>         tai->caps.owner = THIS_MODULE;
>         strscpy(tai->caps.name, "Marvell PP2.2", sizeof(tai->caps.name));
> +       tai->caps.n_ext_ts = MAX_PINS;
> +       tai->caps.n_pins = MAX_PINS;
>         tai->caps.max_adj = mvpp22_calc_max_adj(tai);
>         tai->caps.adjfine = mvpp22_tai_adjfine;
>         tai->caps.adjtime = mvpp22_tai_adjtime;
>         tai->caps.gettimex64 = mvpp22_tai_gettimex64;
>         tai->caps.settime64 = mvpp22_tai_settime64;
>         tai->caps.do_aux_work = mvpp22_tai_aux_work;
> +       tai->caps.enable = mvpp22_tai_enable;
> +       tai->caps.verify = mvpp22_tai_verify_pin;
> +       tai->caps.pin_config = tai->pin_config;
> +
> +       for (int i = 0; i < tai->caps.n_pins; ++i) {
> +               struct ptp_pin_desc *ppd = &tai->caps.pin_config[i];
> +
> +               snprintf(ppd->name, sizeof(ppd->name), "PTP_PULSE_IN%d", i);
> +               ppd->index = i;
> +               ppd->func = PTP_PF_NONE;
> +               ppd->chan = 0;
> +       }
> 
>         ret = devm_add_action(dev, mvpp22_tai_remove, tai);
>         if (ret)
> --
> 2.40.0
> 

-- 
/Horatiu