| 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
| ||
|
Message-ID: <20230418093747.jxxvoizofd6cgk6v@soft-dev3-1>
Date: Tue, 18 Apr 2023 11:37:47 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Shmuel Hazan <shmuel.h@...lu.com>
CC: Russell King <linux@...linux.org.uk>,
Marcin Wojtas <mw@...ihalf.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Richard Cochran <richardcochran@...il.com>,
<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] net: mvpp2: tai: add extts support
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?
> +
> 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
Powered by blists - more mailing lists