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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 18 Apr 2023 10:09:15 +0000
From:   Shmuel Hazan <shmuel.h@...lu.com>
To:     "horatiu.vultur@...rochip.com" <horatiu.vultur@...rochip.com>
CC:     "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "mw@...ihalf.com" <mw@...ihalf.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>
Subject: Re: [PATCH v2 2/3] net: mvpp2: tai: add extts support

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@...rochip.com>
> 
> > 
> > Signed-off-by: Shmuel Hazan <shmuel.h@...lu.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