Add PTP IRQ handler for lan969x. This is required, as the PTP registers
are placed in two different targets on Sparx5 and lan969x. The
implementation is otherwise the same as on Sparx5.
Also, expose sparx5_get_hwtimestamp() for use by lan969x.
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
drivers/net/ethernet/microchip/lan969x/lan969x.c | 90 ++++++++++++++++++++++
.../net/ethernet/microchip/sparx5/sparx5_main.h | 5 ++
drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c | 9 +--
3 files changed, 99 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan969x/lan969x.c b/drivers/net/ethernet/microchip/lan969x/lan969x.c
index 2c2b86f9144e..a3b40e09b947 100644
--- a/drivers/net/ethernet/microchip/lan969x/lan969x.c
+++ b/drivers/net/ethernet/microchip/lan969x/lan969x.c
@@ -201,6 +201,95 @@ static int lan969x_port_mux_set(struct sparx5 *sparx5, struct sparx5_port *port,
return 0;
}
+static irqreturn_t lan969x_ptp_irq_handler(int irq, void *args)
+{
+ int budget = SPARX5_MAX_PTP_ID;
+ struct sparx5 *sparx5 = args;
+
+ while (budget--) {
+ struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
+ struct skb_shared_hwtstamps shhwtstamps;
+ struct sparx5_port *port;
+ struct timespec64 ts;
+ unsigned long flags;
+ u32 val, id, txport;
+ u32 delay;
+
+ val = spx5_rd(sparx5, PTP_TWOSTEP_CTRL);
+
+ /* Check if a timestamp can be retrieved */
+ if (!(val & PTP_TWOSTEP_CTRL_PTP_VLD))
+ break;
+
+ WARN_ON(val & PTP_TWOSTEP_CTRL_PTP_OVFL);
+
+ if (!(val & PTP_TWOSTEP_CTRL_STAMP_TX))
+ continue;
+
+ /* Retrieve the ts Tx port */
+ txport = PTP_TWOSTEP_CTRL_STAMP_PORT_GET(val);
+
+ /* Retrieve its associated skb */
+ port = sparx5->ports[txport];
+
+ /* Retrieve the delay */
+ delay = spx5_rd(sparx5, PTP_TWOSTEP_STAMP_NSEC);
+ delay = PTP_TWOSTEP_STAMP_NSEC_NS_GET(delay);
+
+ /* Get next timestamp from fifo, which needs to be the
+ * rx timestamp which represents the id of the frame
+ */
+ spx5_rmw(PTP_TWOSTEP_CTRL_PTP_NXT_SET(1),
+ PTP_TWOSTEP_CTRL_PTP_NXT,
+ sparx5, PTP_TWOSTEP_CTRL);
+
+ val = spx5_rd(sparx5, PTP_TWOSTEP_CTRL);
+
+ /* Check if a timestamp can be retrieved */
+ if (!(val & PTP_TWOSTEP_CTRL_PTP_VLD))
+ break;
+
+ /* Read RX timestamping to get the ID */
+ id = spx5_rd(sparx5, PTP_TWOSTEP_STAMP_NSEC);
+ id <<= 8;
+ id |= spx5_rd(sparx5, PTP_TWOSTEP_STAMP_SUBNS);
+
+ spin_lock_irqsave(&port->tx_skbs.lock, flags);
+ skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
+ if (SPARX5_SKB_CB(skb)->ts_id != id)
+ continue;
+
+ __skb_unlink(skb, &port->tx_skbs);
+ skb_match = skb;
+ break;
+ }
+ spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
+
+ /* Next ts */
+ spx5_rmw(PTP_TWOSTEP_CTRL_PTP_NXT_SET(1),
+ PTP_TWOSTEP_CTRL_PTP_NXT,
+ sparx5, PTP_TWOSTEP_CTRL);
+
+ if (WARN_ON(!skb_match))
+ continue;
+
+ spin_lock(&sparx5->ptp_ts_id_lock);
+ sparx5->ptp_skbs--;
+ spin_unlock(&sparx5->ptp_ts_id_lock);
+
+ /* Get the h/w timestamp */
+ sparx5_get_hwtimestamp(sparx5, &ts, delay);
+
+ /* Set the timestamp in the skb */
+ shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
+ skb_tstamp_tx(skb_match, &shhwtstamps);
+
+ dev_kfree_skb_any(skb_match);
+ }
+
+ return IRQ_HANDLED;
+}
+
static const struct sparx5_regs lan969x_regs = {
.tsize = lan969x_tsize,
.gaddr = lan969x_gaddr,
@@ -242,6 +331,7 @@ static const struct sparx5_ops lan969x_ops = {
.get_hsch_max_group_rate = &lan969x_get_hsch_max_group_rate,
.get_sdlb_group = &lan969x_get_sdlb_group,
.set_port_mux = &lan969x_port_mux_set,
+ .ptp_irq_handler = &lan969x_ptp_irq_handler,
};
const struct sparx5_match_data lan969x_desc = {
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 15f5d38776c4..3f66045c57ef 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -114,6 +114,8 @@ enum sparx5_vlan_port_type {
#define SPX5_DSM_CAL_LEN 64
#define SPX5_DSM_CAL_MAX_DEVS_PER_TAXI 13
+#define SPARX5_MAX_PTP_ID 512
+
struct sparx5;
struct sparx5_calendar_data {
@@ -499,6 +501,9 @@ void sparx5_ptp_txtstamp_release(struct sparx5_port *port,
struct sk_buff *skb);
irqreturn_t sparx5_ptp_irq_handler(int irq, void *args);
int sparx5_ptp_gettime64(struct ptp_clock_info *ptp, struct timespec64 *ts);
+void sparx5_get_hwtimestamp(struct sparx5 *sparx5,
+ struct timespec64 *ts,
+ u32 nsec);
/* sparx5_vcap_impl.c */
int sparx5_vcap_init(struct sparx5 *sparx5);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c b/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
index a511f14312f1..1c2903700a9c 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
@@ -11,8 +11,6 @@
#include "sparx5_main_regs.h"
#include "sparx5_main.h"
-#define SPARX5_MAX_PTP_ID 512
-
#define TOD_ACC_PIN 0x4
enum {
@@ -275,9 +273,9 @@ void sparx5_ptp_txtstamp_release(struct sparx5_port *port,
spin_unlock_irqrestore(&sparx5->ptp_ts_id_lock, flags);
}
-static void sparx5_get_hwtimestamp(struct sparx5 *sparx5,
- struct timespec64 *ts,
- u32 nsec)
+void sparx5_get_hwtimestamp(struct sparx5 *sparx5,
+ struct timespec64 *ts,
+ u32 nsec)
{
/* Read current PTP time to get seconds */
const struct sparx5_consts *consts = sparx5->data->consts;
@@ -305,6 +303,7 @@ static void sparx5_get_hwtimestamp(struct sparx5 *sparx5,
spin_unlock_irqrestore(&sparx5->ptp_clock_lock, flags);
}
+EXPORT_SYMBOL_GPL(sparx5_get_hwtimestamp);
irqreturn_t sparx5_ptp_irq_handler(int irq, void *args)
{
--
2.34.1On Thu, 24 Oct 2024 00:01:29 +0200 Daniel Machon wrote:
> + spin_lock_irqsave(&port->tx_skbs.lock, flags);
> + skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> + if (SPARX5_SKB_CB(skb)->ts_id != id)
> + continue;
> +
> + __skb_unlink(skb, &port->tx_skbs);
> + skb_match = skb;
> + break;
> + }
> + spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
For a followup for both drivers -- you're mixing irqsave and bare
spin_lock() here. The _irqsave/_irqrestore is not necessary, let's
drop it.
> + spin_lock(&sparx5->ptp_ts_id_lock);
> On Thu, 24 Oct 2024 00:01:29 +0200 Daniel Machon wrote:
> > + spin_lock_irqsave(&port->tx_skbs.lock, flags);
> > + skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> > + if (SPARX5_SKB_CB(skb)->ts_id != id)
> > + continue;
> > +
> > + __skb_unlink(skb, &port->tx_skbs);
> > + skb_match = skb;
> > + break;
> > + }
> > + spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
>
> For a followup for both drivers -- you're mixing irqsave and bare
> spin_lock() here. The _irqsave/_irqrestore is not necessary, let's
> drop it.
>
> > + spin_lock(&sparx5->ptp_ts_id_lock);
Hi Jakub,
I agree it seems wrong to mix these.
I just talked to Horatiu, and he mentioned posting a similar fix for the
lan966x driver some time ago [1]. Only this fix added
_irqsave/_irqrestore to the ptp_ts_id_lock - so basically the opposite
of what you are suggesting. Why do you think that the
_irqsave/_irqrestore is not necessary?
[1] 3a70e0d4c9d7 ("net: lan966x: Fix possible deadlock inside PTP")
/Daniel
On Thu, 31 Oct 2024 09:36:28 +0000 Daniel Machon wrote: > > For a followup for both drivers -- you're mixing irqsave and bare > > spin_lock() here. The _irqsave/_irqrestore is not necessary, let's > > drop it. > > > > > + spin_lock(&sparx5->ptp_ts_id_lock); > > Hi Jakub, > > I agree it seems wrong to mix these. > > I just talked to Horatiu, and he mentioned posting a similar fix for the > lan966x driver some time ago [1]. Only this fix added > _irqsave/_irqrestore to the ptp_ts_id_lock - so basically the opposite > of what you are suggesting. Why do you think that the > _irqsave/_irqrestore is not necessary? Oh, I thought this is a real IRQ handler, not a threaded one. I haven't read the code to figure out whether ptp_ts_id_lock needs to be IRQ-safe, but in other places you lock if _irqsave so yes, let's irqsave here, too.
On 23/10/2024 23:01, Daniel Machon wrote:
> Add PTP IRQ handler for lan969x. This is required, as the PTP registers
> are placed in two different targets on Sparx5 and lan969x. The
> implementation is otherwise the same as on Sparx5.
>
> Also, expose sparx5_get_hwtimestamp() for use by lan969x.
>
> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
> drivers/net/ethernet/microchip/lan969x/lan969x.c | 90 ++++++++++++++++++++++
> .../net/ethernet/microchip/sparx5/sparx5_main.h | 5 ++
> drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c | 9 +--
> 3 files changed, 99 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan969x/lan969x.c b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> index 2c2b86f9144e..a3b40e09b947 100644
> --- a/drivers/net/ethernet/microchip/lan969x/lan969x.c
> +++ b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> @@ -201,6 +201,95 @@ static int lan969x_port_mux_set(struct sparx5 *sparx5, struct sparx5_port *port,
> return 0;
> }
>
> +static irqreturn_t lan969x_ptp_irq_handler(int irq, void *args)
> +{
> + int budget = SPARX5_MAX_PTP_ID;
> + struct sparx5 *sparx5 = args;
> +
> + while (budget--) {
> + struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
> + struct skb_shared_hwtstamps shhwtstamps;
> + struct sparx5_port *port;
> + struct timespec64 ts;
> + unsigned long flags;
> + u32 val, id, txport;
> + u32 delay;
> +
> + val = spx5_rd(sparx5, PTP_TWOSTEP_CTRL);
> +
> + /* Check if a timestamp can be retrieved */
> + if (!(val & PTP_TWOSTEP_CTRL_PTP_VLD))
> + break;
> +
> + WARN_ON(val & PTP_TWOSTEP_CTRL_PTP_OVFL);
> +
> + if (!(val & PTP_TWOSTEP_CTRL_STAMP_TX))
> + continue;
> +
> + /* Retrieve the ts Tx port */
> + txport = PTP_TWOSTEP_CTRL_STAMP_PORT_GET(val);
> +
> + /* Retrieve its associated skb */
> + port = sparx5->ports[txport];
> +
> + /* Retrieve the delay */
> + delay = spx5_rd(sparx5, PTP_TWOSTEP_STAMP_NSEC);
> + delay = PTP_TWOSTEP_STAMP_NSEC_NS_GET(delay);
> +
> + /* Get next timestamp from fifo, which needs to be the
> + * rx timestamp which represents the id of the frame
> + */
> + spx5_rmw(PTP_TWOSTEP_CTRL_PTP_NXT_SET(1),
> + PTP_TWOSTEP_CTRL_PTP_NXT,
> + sparx5, PTP_TWOSTEP_CTRL);
> +
> + val = spx5_rd(sparx5, PTP_TWOSTEP_CTRL);
> +
> + /* Check if a timestamp can be retrieved */
> + if (!(val & PTP_TWOSTEP_CTRL_PTP_VLD))
> + break;
> +
> + /* Read RX timestamping to get the ID */
> + id = spx5_rd(sparx5, PTP_TWOSTEP_STAMP_NSEC);
> + id <<= 8;
> + id |= spx5_rd(sparx5, PTP_TWOSTEP_STAMP_SUBNS);
> +
> + spin_lock_irqsave(&port->tx_skbs.lock, flags);
> + skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> + if (SPARX5_SKB_CB(skb)->ts_id != id)
> + continue;
> +
> + __skb_unlink(skb, &port->tx_skbs);
> + skb_match = skb;
> + break;
> + }
> + spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
> +
> + /* Next ts */
> + spx5_rmw(PTP_TWOSTEP_CTRL_PTP_NXT_SET(1),
> + PTP_TWOSTEP_CTRL_PTP_NXT,
> + sparx5, PTP_TWOSTEP_CTRL);
> +
> + if (WARN_ON(!skb_match))
> + continue;
> +
> + spin_lock(&sparx5->ptp_ts_id_lock);
> + sparx5->ptp_skbs--;
> + spin_unlock(&sparx5->ptp_ts_id_lock);
> +
> + /* Get the h/w timestamp */
> + sparx5_get_hwtimestamp(sparx5, &ts, delay);
> +
> + /* Set the timestamp in the skb */
> + shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> + skb_tstamp_tx(skb_match, &shhwtstamps);
> +
> + dev_kfree_skb_any(skb_match);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
This handler looks like an absolute copy of sparx5_ptp_irq_handler()
with the difference in registers only. Did you consider keep one
function but substitute ptp register sets?
> static const struct sparx5_regs lan969x_regs = {
> .tsize = lan969x_tsize,
> .gaddr = lan969x_gaddr,
> @@ -242,6 +331,7 @@ static const struct sparx5_ops lan969x_ops = {
> .get_hsch_max_group_rate = &lan969x_get_hsch_max_group_rate,
> .get_sdlb_group = &lan969x_get_sdlb_group,
> .set_port_mux = &lan969x_port_mux_set,
> + .ptp_irq_handler = &lan969x_ptp_irq_handler,
> };
>
Hi Vadim,
Thanks for reviewing.
> On 23/10/2024 23:01, Daniel Machon wrote:
> > Add PTP IRQ handler for lan969x. This is required, as the PTP registers
> > are placed in two different targets on Sparx5 and lan969x. The
> > implementation is otherwise the same as on Sparx5.
> >
> > Also, expose sparx5_get_hwtimestamp() for use by lan969x.
> >
> > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> > ---
> > drivers/net/ethernet/microchip/lan969x/lan969x.c | 90 ++++++++++++++++++++++
> > .../net/ethernet/microchip/sparx5/sparx5_main.h | 5 ++
> > drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c | 9 +--
> > 3 files changed, 99 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan969x/lan969x.c b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > index 2c2b86f9144e..a3b40e09b947 100644
> > --- a/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > +++ b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > @@ -201,6 +201,95 @@ static int lan969x_port_mux_set(struct sparx5 *sparx5, struct sparx5_port *port,
> > return 0;
> > }
> >
> > +static irqreturn_t lan969x_ptp_irq_handler(int irq, void *args)
> > +{
> > + int budget = SPARX5_MAX_PTP_ID;
> > + struct sparx5 *sparx5 = args;
> > +
> > + while (budget--) {
> > + struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
> > + struct skb_shared_hwtstamps shhwtstamps;
> > + struct sparx5_port *port;
> > + struct timespec64 ts;
> > + unsigned long flags;
> > + u32 val, id, txport;
> > + u32 delay;
> > +
> > + val = spx5_rd(sparx5, PTP_TWOSTEP_CTRL);
> > +
> > + /* Check if a timestamp can be retrieved */
> > + if (!(val & PTP_TWOSTEP_CTRL_PTP_VLD))
> > + break;
> > +
> > + WARN_ON(val & PTP_TWOSTEP_CTRL_PTP_OVFL);
> > +
> > + if (!(val & PTP_TWOSTEP_CTRL_STAMP_TX))
> > + continue;
> > +
> > + /* Retrieve the ts Tx port */
> > + txport = PTP_TWOSTEP_CTRL_STAMP_PORT_GET(val);
> > +
> > + /* Retrieve its associated skb */
> > + port = sparx5->ports[txport];
> > +
> > + /* Retrieve the delay */
> > + delay = spx5_rd(sparx5, PTP_TWOSTEP_STAMP_NSEC);
> > + delay = PTP_TWOSTEP_STAMP_NSEC_NS_GET(delay);
> > +
> > + /* Get next timestamp from fifo, which needs to be the
> > + * rx timestamp which represents the id of the frame
> > + */
> > + spx5_rmw(PTP_TWOSTEP_CTRL_PTP_NXT_SET(1),
> > + PTP_TWOSTEP_CTRL_PTP_NXT,
> > + sparx5, PTP_TWOSTEP_CTRL);
> > +
> > + val = spx5_rd(sparx5, PTP_TWOSTEP_CTRL);
> > +
> > + /* Check if a timestamp can be retrieved */
> > + if (!(val & PTP_TWOSTEP_CTRL_PTP_VLD))
> > + break;
> > +
> > + /* Read RX timestamping to get the ID */
> > + id = spx5_rd(sparx5, PTP_TWOSTEP_STAMP_NSEC);
> > + id <<= 8;
> > + id |= spx5_rd(sparx5, PTP_TWOSTEP_STAMP_SUBNS);
> > +
> > + spin_lock_irqsave(&port->tx_skbs.lock, flags);
> > + skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> > + if (SPARX5_SKB_CB(skb)->ts_id != id)
> > + continue;
> > +
> > + __skb_unlink(skb, &port->tx_skbs);
> > + skb_match = skb;
> > + break;
> > + }
> > + spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
> > +
> > + /* Next ts */
> > + spx5_rmw(PTP_TWOSTEP_CTRL_PTP_NXT_SET(1),
> > + PTP_TWOSTEP_CTRL_PTP_NXT,
> > + sparx5, PTP_TWOSTEP_CTRL);
> > +
> > + if (WARN_ON(!skb_match))
> > + continue;
> > +
> > + spin_lock(&sparx5->ptp_ts_id_lock);
> > + sparx5->ptp_skbs--;
> > + spin_unlock(&sparx5->ptp_ts_id_lock);
> > +
> > + /* Get the h/w timestamp */
> > + sparx5_get_hwtimestamp(sparx5, &ts, delay);
> > +
> > + /* Set the timestamp in the skb */
> > + shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> > + skb_tstamp_tx(skb_match, &shhwtstamps);
> > +
> > + dev_kfree_skb_any(skb_match);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
>
> This handler looks like an absolute copy of sparx5_ptp_irq_handler()
> with the difference in registers only. Did you consider keep one
> function but substitute ptp register sets?
>
Yes, I did consider that. But since this is the only case where a group
of registers are moved to a different register target in hw, I chose to
instead copy the function.
The indirection layer introduced in the previous series does not handle
differences in register targets - maybe something to be added later if we
have more cases (hopefully not).
/Daniel
> > static const struct sparx5_regs lan969x_regs = {
> > .tsize = lan969x_tsize,
> > .gaddr = lan969x_gaddr,
> > @@ -242,6 +331,7 @@ static const struct sparx5_ops lan969x_ops = {
> > .get_hsch_max_group_rate = &lan969x_get_hsch_max_group_rate,
> > .get_sdlb_group = &lan969x_get_sdlb_group,
> > .set_port_mux = &lan969x_port_mux_set,
> > + .ptp_irq_handler = &lan969x_ptp_irq_handler,
> > };
> >
© 2016 - 2024 Red Hat, Inc.