Re: [PATCH v2 2/3] net: mvpp2: tai: add extts support

[Shmuel Hazan <shmuel.h@siklu.com>]

On Tue, 2023-04-18 at 11:37 +0200, Horatiu Vultur wrote:
> 
> 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?

Hi Horatiu,

Thanks for your review. 

As for the 95 value, I just borrowed it from a similar driver
(ptp_clockmatrix) that also employs a similar polling mechanism for
extts support. It seems to work pretty well in testing so I had no
reason to change it.

> 
> > <snip>
> --
> /Horatiu