[PATCH net-next 0/2] net: renesas: rswitch: update irq handling

[PATCH net-next 0/2] net: renesas: rswitch: update irq handling

[Nikita Yushchenko]

This series switches rswitch driver to per-port interrupt handlers and
does related cleanup.

Nikita Yushchenko (2):
  net: renesas: rswitch: use per-port irq handlers
  net: renesas: rswitch: request ts interrupt at port open

 drivers/net/ethernet/renesas/rswitch.c | 223 ++++++++++---------------
 drivers/net/ethernet/renesas/rswitch.h |  12 +-
 2 files changed, 99 insertions(+), 136 deletions(-)

-- 
2.39.5

[PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Nikita Yushchenko]

Instead of handling all possible data interrupts in the same handler,
switch to per-port handlers.

This significantly simplifies handling: when the same interrupt is used
for several ports, system calls all handlers, and each handler only has
to check interrupts for one port's tx and rx queues.

But it is not required to use the same interrupt for all ports - GWCA
provides 8 data interrupts and allows arbitrary per-queue assignment
of those. Support that by reading interrupt index for each port from
optional 'irq-index' device tree property.

With per-port interrupts it becomes possible to configure affinity such
that traffic coming from different ports is serviced simultaneously on
different CPUs.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 190 ++++++++++---------------
 drivers/net/ethernet/renesas/rswitch.h |  10 +-
 2 files changed, 82 insertions(+), 118 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 84d09a8973b7..eb9dea8b16f3 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -99,15 +99,6 @@ static void rswitch_coma_init(struct rswitch_private *priv)
 	iowrite32(CABPPFLC_INIT_VALUE, priv->addr + CABPPFLC0);
 }
 
-/* R-Switch-2 block (TOP) */
-static void rswitch_top_init(struct rswitch_private *priv)
-{
-	unsigned int i;
-
-	for (i = 0; i < RSWITCH_MAX_NUM_QUEUES; i++)
-		iowrite32((i / 16) << (GWCA_INDEX * 8), priv->addr + TPEMIMC7(i));
-}
-
 /* Forwarding engine block (MFWD) */
 static void rswitch_fwd_init(struct rswitch_private *priv)
 {
@@ -175,29 +166,6 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
 	return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
 }
 
-static bool rswitch_is_any_data_irq(struct rswitch_private *priv, u32 *dis, bool tx)
-{
-	u32 *mask = tx ? priv->gwca.tx_irq_bits : priv->gwca.rx_irq_bits;
-	unsigned int i;
-
-	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
-		if (dis[i] & mask[i])
-			return true;
-	}
-
-	return false;
-}
-
-static void rswitch_get_data_irq_status(struct rswitch_private *priv, u32 *dis)
-{
-	unsigned int i;
-
-	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
-		dis[i] = ioread32(priv->addr + GWDIS(i));
-		dis[i] &= ioread32(priv->addr + GWDIE(i));
-	}
-}
-
 static void rswitch_enadis_data_irq(struct rswitch_private *priv,
 				    unsigned int index, bool enable)
 {
@@ -206,12 +174,18 @@ static void rswitch_enadis_data_irq(struct rswitch_private *priv,
 	iowrite32(BIT(index % 32), priv->addr + offs);
 }
 
-static void rswitch_ack_data_irq(struct rswitch_private *priv,
-				 unsigned int index)
+static bool rswitch_check_ack_data_irq(struct rswitch_private *priv,
+				       unsigned int index)
 {
-	u32 offs = GWDIS(index / 32);
+	u32 reg = GWDIS(index / 32);
+	u32 bit = BIT(index % 32);
 
-	iowrite32(BIT(index % 32), priv->addr + offs);
+	if (ioread32(priv->addr + reg) & bit) {
+		iowrite32(bit, priv->addr + reg);
+		return true;
+	}
+
+	return false;
 }
 
 static unsigned int rswitch_next_queue_index(struct rswitch_gwca_queue *gq,
@@ -314,8 +288,6 @@ static int rswitch_gwca_queue_alloc(struct net_device *ndev,
 				    struct rswitch_gwca_queue *gq,
 				    bool dir_tx, unsigned int ring_size)
 {
-	unsigned int i, bit;
-
 	gq->dir_tx = dir_tx;
 	gq->ring_size = ring_size;
 	gq->ndev = ndev;
@@ -345,13 +317,6 @@ static int rswitch_gwca_queue_alloc(struct net_device *ndev,
 	if (!gq->rx_ring && !gq->tx_ring)
 		goto out;
 
-	i = gq->index / 32;
-	bit = BIT(gq->index % 32);
-	if (dir_tx)
-		priv->gwca.tx_irq_bits[i] |= bit;
-	else
-		priv->gwca.rx_irq_bits[i] |= bit;
-
 	return 0;
 
 out:
@@ -583,6 +548,15 @@ static void rswitch_gwca_put(struct rswitch_private *priv,
 	clear_bit(gq->index, priv->gwca.used);
 }
 
+static void rswitch_gwca_queue_assign_irq(struct rswitch_private *priv,
+					  struct rswitch_gwca_queue *gq,
+					  unsigned int irq_index)
+{
+	rswitch_modify(priv->addr, TPEMIMC7(gq->index),
+		       TPEMIMC7_GDICM(GWCA_INDEX),
+		       TPEMIMC7_GDICM_PREP(GWCA_INDEX, irq_index));
+}
+
 static int rswitch_txdmac_alloc(struct net_device *ndev)
 {
 	struct rswitch_device *rdev = netdev_priv(ndev);
@@ -614,6 +588,7 @@ static int rswitch_txdmac_init(struct rswitch_private *priv, unsigned int index)
 {
 	struct rswitch_device *rdev = priv->rdev[index];
 
+	rswitch_gwca_queue_assign_irq(priv, rdev->tx_queue, rdev->irq_index);
 	return rswitch_gwca_queue_format(rdev->ndev, priv, rdev->tx_queue);
 }
 
@@ -649,6 +624,7 @@ static int rswitch_rxdmac_init(struct rswitch_private *priv, unsigned int index)
 	struct rswitch_device *rdev = priv->rdev[index];
 	struct net_device *ndev = rdev->ndev;
 
+	rswitch_gwca_queue_assign_irq(priv, rdev->rx_queue, rdev->irq_index);
 	return rswitch_gwca_queue_ext_ts_format(ndev, priv, rdev->rx_queue);
 }
 
@@ -933,9 +909,13 @@ static int rswitch_poll(struct napi_struct *napi, int budget)
 	return 0;
 }
 
-static void rswitch_queue_interrupt(struct net_device *ndev)
+static irqreturn_t rswitch_gwca_data_irq(int irq, void *data)
 {
-	struct rswitch_device *rdev = netdev_priv(ndev);
+	struct rswitch_device *rdev = data;
+
+	if (!rswitch_check_ack_data_irq(rdev->priv, rdev->tx_queue->index) &&
+	    !rswitch_check_ack_data_irq(rdev->priv, rdev->rx_queue->index))
+		return IRQ_NONE;
 
 	if (napi_schedule_prep(&rdev->napi)) {
 		spin_lock(&rdev->priv->lock);
@@ -944,71 +924,10 @@ static void rswitch_queue_interrupt(struct net_device *ndev)
 		spin_unlock(&rdev->priv->lock);
 		__napi_schedule(&rdev->napi);
 	}
-}
-
-static irqreturn_t rswitch_data_irq(struct rswitch_private *priv, u32 *dis)
-{
-	struct rswitch_gwca_queue *gq;
-	unsigned int i, index, bit;
-
-	for (i = 0; i < priv->gwca.num_queues; i++) {
-		gq = &priv->gwca.queues[i];
-		index = gq->index / 32;
-		bit = BIT(gq->index % 32);
-		if (!(dis[index] & bit))
-			continue;
-
-		rswitch_ack_data_irq(priv, gq->index);
-		rswitch_queue_interrupt(gq->ndev);
-	}
 
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t rswitch_gwca_irq(int irq, void *dev_id)
-{
-	struct rswitch_private *priv = dev_id;
-	u32 dis[RSWITCH_NUM_IRQ_REGS];
-	irqreturn_t ret = IRQ_NONE;
-
-	rswitch_get_data_irq_status(priv, dis);
-
-	if (rswitch_is_any_data_irq(priv, dis, true) ||
-	    rswitch_is_any_data_irq(priv, dis, false))
-		ret = rswitch_data_irq(priv, dis);
-
-	return ret;
-}
-
-static int rswitch_gwca_request_irqs(struct rswitch_private *priv)
-{
-	char *resource_name, *irq_name;
-	int i, ret, irq;
-
-	for (i = 0; i < GWCA_NUM_IRQS; i++) {
-		resource_name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, i);
-		if (!resource_name)
-			return -ENOMEM;
-
-		irq = platform_get_irq_byname(priv->pdev, resource_name);
-		kfree(resource_name);
-		if (irq < 0)
-			return irq;
-
-		irq_name = devm_kasprintf(&priv->pdev->dev, GFP_KERNEL,
-					  GWCA_IRQ_NAME, i);
-		if (!irq_name)
-			return -ENOMEM;
-
-		ret = devm_request_irq(&priv->pdev->dev, irq, rswitch_gwca_irq,
-				       0, irq_name, priv);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
 static void rswitch_ts(struct rswitch_private *priv)
 {
 	struct rswitch_gwca_queue *gq = &priv->gwca.ts_queue;
@@ -1589,12 +1508,18 @@ static int rswitch_open(struct net_device *ndev)
 {
 	struct rswitch_device *rdev = netdev_priv(ndev);
 	unsigned long flags;
+	int ret;
 
 	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
 		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDIE);
 
 	napi_enable(&rdev->napi);
 
+	ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED,
+			  netdev_name(ndev), rdev);
+	if (ret < 0)
+		goto err_request_irq;
+
 	spin_lock_irqsave(&rdev->priv->lock, flags);
 	bitmap_set(rdev->priv->opened_ports, rdev->port, 1);
 	rswitch_enadis_data_irq(rdev->priv, rdev->tx_queue->index, true);
@@ -1606,6 +1531,14 @@ static int rswitch_open(struct net_device *ndev)
 	netif_start_queue(ndev);
 
 	return 0;
+
+err_request_irq:
+	napi_disable(&rdev->napi);
+
+	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
+		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDID);
+
+	return ret;
 };
 
 static int rswitch_stop(struct net_device *ndev)
@@ -1625,6 +1558,8 @@ static int rswitch_stop(struct net_device *ndev)
 	bitmap_clear(rdev->priv->opened_ports, rdev->port, 1);
 	spin_unlock_irqrestore(&rdev->priv->lock, flags);
 
+	free_irq(rdev->irq, rdev);
+
 	napi_disable(&rdev->napi);
 
 	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
@@ -1906,6 +1841,34 @@ static void rswitch_etha_init(struct rswitch_private *priv, unsigned int index)
 	etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
 }
 
+static int rswitch_port_get_irq(struct rswitch_device *rdev)
+{
+	unsigned int irq_index;
+	char *name;
+	int err;
+
+	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
+	if (err == 0) {
+		if (irq_index < GWCA_NUM_IRQS)
+			rdev->irq_index = irq_index;
+		else
+			dev_warn(&rdev->priv->pdev->dev,
+				 "%pOF: irq-index out of range\n",
+				 rdev->np_port);
+	}
+
+	name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);
+	if (!name)
+		return -ENOMEM;
+	err = platform_get_irq_byname(rdev->priv->pdev, name);
+	kfree(name);
+	if (err < 0)
+		return err;
+	rdev->irq = err;
+
+	return 0;
+}
+
 static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index)
 {
 	struct platform_device *pdev = priv->pdev;
@@ -1954,6 +1917,10 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
 	if (err < 0)
 		goto out_get_params;
 
+	err = rswitch_port_get_irq(rdev);
+	if (err < 0)
+		goto out_get_irq;
+
 	err = rswitch_rxdmac_alloc(ndev);
 	if (err < 0)
 		goto out_rxdmac;
@@ -1968,6 +1935,7 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
 	rswitch_rxdmac_free(ndev);
 
 out_rxdmac:
+out_get_irq:
 out_get_params:
 	of_node_put(rdev->np_port);
 	netif_napi_del(&rdev->napi);
@@ -2003,7 +1971,6 @@ static int rswitch_init(struct rswitch_private *priv)
 	rswitch_reset(priv);
 
 	rswitch_clock_enable(priv);
-	rswitch_top_init(priv);
 	err = rswitch_bpool_config(priv);
 	if (err < 0)
 		return err;
@@ -2034,10 +2001,6 @@ static int rswitch_init(struct rswitch_private *priv)
 	if (err < 0)
 		goto err_ptp_register;
 
-	err = rswitch_gwca_request_irqs(priv);
-	if (err < 0)
-		goto err_gwca_request_irq;
-
 	err = rswitch_gwca_ts_request_irqs(priv);
 	if (err < 0)
 		goto err_gwca_ts_request_irq;
@@ -2073,7 +2036,6 @@ static int rswitch_init(struct rswitch_private *priv)
 
 err_gwca_hw_init:
 err_gwca_ts_request_irq:
-err_gwca_request_irq:
 	rcar_gen4_ptp_unregister(priv->ptp_priv);
 
 err_ptp_register:
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 532192cbca4b..a1e62a6b3844 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -51,7 +51,6 @@
 
 /* TODO: hardcoded ETHA/GWCA settings for now */
 #define GWCA_IRQ_RESOURCE_NAME	"gwca0_rxtx%d"
-#define GWCA_IRQ_NAME		"rswitch: gwca0_rxtx%d"
 #define GWCA_NUM_IRQS		8
 #define GWCA_INDEX		0
 #define AGENT_INDEX_GWCA	3
@@ -828,6 +827,10 @@ enum rswitch_gwca_mode {
 
 /* TOP */
 #define TPEMIMC7(queue)		(TPEMIMC70 + (queue) * 4)
+#define TPEMIMC7_GDICM0			GENMASK(2, 0)
+#define TPEMIMC7_GDICM_SHIFT(i)		((i) * 8)
+#define TPEMIMC7_GDICM(i)		(TPEMIMC7_GDICM0 << TPEMIMC7_GDICM_SHIFT(i))
+#define TPEMIMC7_GDICM_PREP(i, val)	(((val) & TPEMIMC7_GDICM0) << TPEMIMC7_GDICM_SHIFT(i))
 
 /* Descriptors */
 enum RX_DS_CC_BIT {
@@ -967,7 +970,6 @@ struct rswitch_gwca_queue {
 	};
 };
 
-#define RSWITCH_NUM_IRQ_REGS	(RSWITCH_MAX_NUM_QUEUES / BITS_PER_TYPE(u32))
 struct rswitch_gwca {
 	unsigned int index;
 	struct rswitch_desc *linkfix_table;
@@ -977,8 +979,6 @@ struct rswitch_gwca {
 	int num_queues;
 	struct rswitch_gwca_queue ts_queue;
 	DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
-	u32 tx_irq_bits[RSWITCH_NUM_IRQ_REGS];
-	u32 rx_irq_bits[RSWITCH_NUM_IRQ_REGS];
 };
 
 #define NUM_QUEUES_PER_NDEV	2
@@ -990,6 +990,8 @@ struct rswitch_device {
 	void __iomem *addr;
 	struct rswitch_gwca_queue *tx_queue;
 	struct rswitch_gwca_queue *rx_queue;
+	unsigned int irq_index;
+	int irq;
 	struct sk_buff *ts_skb[TS_TAGS_PER_PORT];
 	DECLARE_BITMAP(ts_skb_used, TS_TAGS_PER_PORT);
 	bool disabled;
-- 
2.39.5

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Geert Uytterhoeven]

Hi Nikita,

CC devicetree

Thanks for your patch!

On Fri, Dec 20, 2024 at 5:17 AM Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
> Instead of handling all possible data interrupts in the same handler,
> switch to per-port handlers.
>
> This significantly simplifies handling: when the same interrupt is used
> for several ports, system calls all handlers, and each handler only has
> to check interrupts for one port's tx and rx queues.
>
> But it is not required to use the same interrupt for all ports - GWCA
> provides 8 data interrupts and allows arbitrary per-queue assignment
> of those. Support that by reading interrupt index for each port from
> optional 'irq-index' device tree property.

Sorry, but I can't find where this property is documented?

> With per-port interrupts it becomes possible to configure affinity such
> that traffic coming from different ports is serviced simultaneously on
> different CPUs.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>

> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c

> @@ -1906,6 +1841,34 @@ static void rswitch_etha_init(struct rswitch_private *priv, unsigned int index)
>         etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
>  }
>
> +static int rswitch_port_get_irq(struct rswitch_device *rdev)
> +{
> +       unsigned int irq_index;
> +       char *name;
> +       int err;
> +
> +       err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
> +       if (err == 0) {
> +               if (irq_index < GWCA_NUM_IRQS)
> +                       rdev->irq_index = irq_index;
> +               else
> +                       dev_warn(&rdev->priv->pdev->dev,
> +                                "%pOF: irq-index out of range\n",
> +                                rdev->np_port);
> +       }
> +
> +       name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);
> +       if (!name)
> +               return -ENOMEM;
> +       err = platform_get_irq_byname(rdev->priv->pdev, name);
> +       kfree(name);
> +       if (err < 0)
> +               return err;
> +       rdev->irq = err;
> +
> +       return 0;
> +}
> +
>  static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index)
>  {
>         struct platform_device *pdev = priv->pdev;

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Nikita Yushchenko]

>> But it is not required to use the same interrupt for all ports - GWCA
>> provides 8 data interrupts and allows arbitrary per-queue assignment
>> of those. Support that by reading interrupt index for each port from
>> optional 'irq-index' device tree property.
> 
> Sorry, but I can't find where this property is documented?

I will add this.

Nikita

RE: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Yoshihiro Shimoda]

Hello Nikita-san,

> From: Nikita Yushchenko, Sent: Friday, December 20, 2024 5:09 PM
> 
> >> But it is not required to use the same interrupt for all ports - GWCA
> >> provides 8 data interrupts and allows arbitrary per-queue assignment
> >> of those. Support that by reading interrupt index for each port from
> >> optional 'irq-index' device tree property.
> >
> > Sorry, but I can't find where this property is documented?
> 
> I will add this.

Device tree properties should be a hardware description. However,
about the "irq-index", it seems a software configuration. So, even if we would
like to submit such a patch to add the property, it will be rejected.

Also, even if we can add a new device tree property, we should keep backward compatible.
However, this patch seems to break a backward compatibility.

Unfortunately, I don't have alternative solutions how to configurate per-port irq though...
# Maybe configfs??

Best regards,
Yoshihiro Shimoda

> Nikita

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Nikita Yushchenko]

>>> Sorry, but I can't find where this property is documented?
>>
>> I will add this.
> 
> Device tree properties should be a hardware description. However,
> about the "irq-index", it seems a software configuration. So, even if we would
> like to submit such a patch to add the property, it will be rejected.

Hmm...

Indeed it is a software configuration.

I was not aware of such a rule.

I believe there shall be plenty of situations when a per-hardware-node software configuration is 
desired. What method do other use, if not device tree?

> Also, even if we can add a new device tree property, we should keep backward compatible.
> However, this patch seems to break a backward compatibility.

It does not.
If this new property is not defined, then it will default to 0, which will result exactly into previous 
behavior.

> Unfortunately, I don't have alternative solutions how to configurate per-port irq though...
> # Maybe configfs??

Looks like overengineering...

Perhaps can just hardcode irq-index N for port N for now. But then, flexibility will be lost.

In more complex situations that I target in future, some of 8 GWCA interrupts will be given to virtual 
machines (and/or Xen domains) to serve virtual port frontends, and some will be needed for virtual port 
backends.  So 8 will be not enough to have a per-consumer interrupt, and some configuration method is 
needed.

Nikita

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Andrew Lunn]

On Fri, Dec 20, 2024 at 02:23:31PM +0500, Nikita Yushchenko wrote:
> > > > Sorry, but I can't find where this property is documented?
> > > 
> > > I will add this.
> > 
> > Device tree properties should be a hardware description. However,
> > about the "irq-index", it seems a software configuration. So, even if we would
> > like to submit such a patch to add the property, it will be rejected.
> 
> Hmm...
> 
> Indeed it is a software configuration.
> 
> I was not aware of such a rule.
> 
> I believe there shall be plenty of situations when a per-hardware-node
> software configuration is desired. What method do other use, if not device
> tree?
> 
> > Also, even if we can add a new device tree property, we should keep backward compatible.
> > However, this patch seems to break a backward compatibility.
> 
> It does not.
> If this new property is not defined, then it will default to 0, which will
> result exactly into previous behavior.

This is where the DT binding patch would of been useful, because you
would of stated that in the binding...

Backwards compatibility is something reviewers always look for, so it
is good to make it obvious that it has been considered.

Even if it is backwards compatible, lets see if we can think of a way
to not require the property. Maybe you can explain the hardware in
more details, and what you are trying to achieve.

	Andrew

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Michal Swiatkowski]

On Fri, Dec 20, 2024 at 09:16:58AM +0500, Nikita Yushchenko wrote:
> Instead of handling all possible data interrupts in the same handler,
> switch to per-port handlers.
> 
> This significantly simplifies handling: when the same interrupt is used
> for several ports, system calls all handlers, and each handler only has
> to check interrupts for one port's tx and rx queues.
> 
> But it is not required to use the same interrupt for all ports - GWCA
> provides 8 data interrupts and allows arbitrary per-queue assignment
> of those. Support that by reading interrupt index for each port from
> optional 'irq-index' device tree property.
> 
> With per-port interrupts it becomes possible to configure affinity such
> that traffic coming from different ports is serviced simultaneously on
> different CPUs.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>  drivers/net/ethernet/renesas/rswitch.c | 190 ++++++++++---------------
>  drivers/net/ethernet/renesas/rswitch.h |  10 +-
>  2 files changed, 82 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index 84d09a8973b7..eb9dea8b16f3 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -99,15 +99,6 @@ static void rswitch_coma_init(struct rswitch_private *priv)
>  	iowrite32(CABPPFLC_INIT_VALUE, priv->addr + CABPPFLC0);
>  }
>  
> -/* R-Switch-2 block (TOP) */
> -static void rswitch_top_init(struct rswitch_private *priv)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < RSWITCH_MAX_NUM_QUEUES; i++)
> -		iowrite32((i / 16) << (GWCA_INDEX * 8), priv->addr + TPEMIMC7(i));
> -}
> -
>  /* Forwarding engine block (MFWD) */
>  static void rswitch_fwd_init(struct rswitch_private *priv)
>  {
> @@ -175,29 +166,6 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
>  	return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
>  }
>  
> -static bool rswitch_is_any_data_irq(struct rswitch_private *priv, u32 *dis, bool tx)
> -{
> -	u32 *mask = tx ? priv->gwca.tx_irq_bits : priv->gwca.rx_irq_bits;
> -	unsigned int i;
> -
> -	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
> -		if (dis[i] & mask[i])
> -			return true;
> -	}
> -
> -	return false;
> -}
> -
> -static void rswitch_get_data_irq_status(struct rswitch_private *priv, u32 *dis)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
> -		dis[i] = ioread32(priv->addr + GWDIS(i));
> -		dis[i] &= ioread32(priv->addr + GWDIE(i));
> -	}
> -}
> -
>  static void rswitch_enadis_data_irq(struct rswitch_private *priv,
>  				    unsigned int index, bool enable)
>  {
> @@ -206,12 +174,18 @@ static void rswitch_enadis_data_irq(struct rswitch_private *priv,
>  	iowrite32(BIT(index % 32), priv->addr + offs);
>  }
>  
> -static void rswitch_ack_data_irq(struct rswitch_private *priv,
> -				 unsigned int index)
> +static bool rswitch_check_ack_data_irq(struct rswitch_private *priv,
> +				       unsigned int index)
>  {
> -	u32 offs = GWDIS(index / 32);
> +	u32 reg = GWDIS(index / 32);
> +	u32 bit = BIT(index % 32);
>  
> -	iowrite32(BIT(index % 32), priv->addr + offs);
> +	if (ioread32(priv->addr + reg) & bit) {
> +		iowrite32(bit, priv->addr + reg);
> +		return true;
> +	}
> +
> +	return false;
>  }
>  
>  static unsigned int rswitch_next_queue_index(struct rswitch_gwca_queue *gq,
> @@ -314,8 +288,6 @@ static int rswitch_gwca_queue_alloc(struct net_device *ndev,
>  				    struct rswitch_gwca_queue *gq,
>  				    bool dir_tx, unsigned int ring_size)
>  {
> -	unsigned int i, bit;
> -
>  	gq->dir_tx = dir_tx;
>  	gq->ring_size = ring_size;
>  	gq->ndev = ndev;
> @@ -345,13 +317,6 @@ static int rswitch_gwca_queue_alloc(struct net_device *ndev,
>  	if (!gq->rx_ring && !gq->tx_ring)
>  		goto out;
>  
> -	i = gq->index / 32;
> -	bit = BIT(gq->index % 32);
> -	if (dir_tx)
> -		priv->gwca.tx_irq_bits[i] |= bit;
> -	else
> -		priv->gwca.rx_irq_bits[i] |= bit;
> -
>  	return 0;
>  
>  out:
> @@ -583,6 +548,15 @@ static void rswitch_gwca_put(struct rswitch_private *priv,
>  	clear_bit(gq->index, priv->gwca.used);
>  }
>  
> +static void rswitch_gwca_queue_assign_irq(struct rswitch_private *priv,
> +					  struct rswitch_gwca_queue *gq,
> +					  unsigned int irq_index)
> +{
> +	rswitch_modify(priv->addr, TPEMIMC7(gq->index),
> +		       TPEMIMC7_GDICM(GWCA_INDEX),
> +		       TPEMIMC7_GDICM_PREP(GWCA_INDEX, irq_index));
> +}
> +
>  static int rswitch_txdmac_alloc(struct net_device *ndev)
>  {
>  	struct rswitch_device *rdev = netdev_priv(ndev);
> @@ -614,6 +588,7 @@ static int rswitch_txdmac_init(struct rswitch_private *priv, unsigned int index)
>  {
>  	struct rswitch_device *rdev = priv->rdev[index];
>  
> +	rswitch_gwca_queue_assign_irq(priv, rdev->tx_queue, rdev->irq_index);
>  	return rswitch_gwca_queue_format(rdev->ndev, priv, rdev->tx_queue);
>  }
>  
> @@ -649,6 +624,7 @@ static int rswitch_rxdmac_init(struct rswitch_private *priv, unsigned int index)
>  	struct rswitch_device *rdev = priv->rdev[index];
>  	struct net_device *ndev = rdev->ndev;
>  
> +	rswitch_gwca_queue_assign_irq(priv, rdev->rx_queue, rdev->irq_index);
>  	return rswitch_gwca_queue_ext_ts_format(ndev, priv, rdev->rx_queue);
>  }
>  
> @@ -933,9 +909,13 @@ static int rswitch_poll(struct napi_struct *napi, int budget)
>  	return 0;
>  }
>  
> -static void rswitch_queue_interrupt(struct net_device *ndev)
> +static irqreturn_t rswitch_gwca_data_irq(int irq, void *data)
>  {
> -	struct rswitch_device *rdev = netdev_priv(ndev);
> +	struct rswitch_device *rdev = data;
> +
> +	if (!rswitch_check_ack_data_irq(rdev->priv, rdev->tx_queue->index) &&
> +	    !rswitch_check_ack_data_irq(rdev->priv, rdev->rx_queue->index))
> +		return IRQ_NONE;
>  
>  	if (napi_schedule_prep(&rdev->napi)) {
>  		spin_lock(&rdev->priv->lock);
> @@ -944,71 +924,10 @@ static void rswitch_queue_interrupt(struct net_device *ndev)
>  		spin_unlock(&rdev->priv->lock);
>  		__napi_schedule(&rdev->napi);
>  	}
> -}
> -
> -static irqreturn_t rswitch_data_irq(struct rswitch_private *priv, u32 *dis)
> -{
> -	struct rswitch_gwca_queue *gq;
> -	unsigned int i, index, bit;
> -
> -	for (i = 0; i < priv->gwca.num_queues; i++) {
> -		gq = &priv->gwca.queues[i];
> -		index = gq->index / 32;
> -		bit = BIT(gq->index % 32);
> -		if (!(dis[index] & bit))
> -			continue;
> -
> -		rswitch_ack_data_irq(priv, gq->index);
> -		rswitch_queue_interrupt(gq->ndev);
> -	}
>  
>  	return IRQ_HANDLED;
>  }
>  
> -static irqreturn_t rswitch_gwca_irq(int irq, void *dev_id)
> -{
> -	struct rswitch_private *priv = dev_id;
> -	u32 dis[RSWITCH_NUM_IRQ_REGS];
> -	irqreturn_t ret = IRQ_NONE;
> -
> -	rswitch_get_data_irq_status(priv, dis);
> -
> -	if (rswitch_is_any_data_irq(priv, dis, true) ||
> -	    rswitch_is_any_data_irq(priv, dis, false))
> -		ret = rswitch_data_irq(priv, dis);
> -
> -	return ret;
> -}
> -
> -static int rswitch_gwca_request_irqs(struct rswitch_private *priv)
> -{
> -	char *resource_name, *irq_name;
> -	int i, ret, irq;
> -
> -	for (i = 0; i < GWCA_NUM_IRQS; i++) {
> -		resource_name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, i);
> -		if (!resource_name)
> -			return -ENOMEM;
> -
> -		irq = platform_get_irq_byname(priv->pdev, resource_name);
> -		kfree(resource_name);
> -		if (irq < 0)
> -			return irq;
> -
> -		irq_name = devm_kasprintf(&priv->pdev->dev, GFP_KERNEL,
> -					  GWCA_IRQ_NAME, i);
> -		if (!irq_name)
> -			return -ENOMEM;
> -
> -		ret = devm_request_irq(&priv->pdev->dev, irq, rswitch_gwca_irq,
> -				       0, irq_name, priv);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static void rswitch_ts(struct rswitch_private *priv)
>  {
>  	struct rswitch_gwca_queue *gq = &priv->gwca.ts_queue;
> @@ -1589,12 +1508,18 @@ static int rswitch_open(struct net_device *ndev)
>  {
>  	struct rswitch_device *rdev = netdev_priv(ndev);
>  	unsigned long flags;
> +	int ret;
>  
>  	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
>  		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDIE);
>  
>  	napi_enable(&rdev->napi);
>  
> +	ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED,
It wasn't shared previously, maybe some notes in commit message about
that.

> +			  netdev_name(ndev), rdev);
> +	if (ret < 0)
> +		goto err_request_irq;
> +
>  	spin_lock_irqsave(&rdev->priv->lock, flags);
>  	bitmap_set(rdev->priv->opened_ports, rdev->port, 1);
>  	rswitch_enadis_data_irq(rdev->priv, rdev->tx_queue->index, true);
> @@ -1606,6 +1531,14 @@ static int rswitch_open(struct net_device *ndev)
>  	netif_start_queue(ndev);
>  
>  	return 0;
> +
> +err_request_irq:
> +	napi_disable(&rdev->napi);
> +
> +	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
> +		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDID);
> +
> +	return ret;
>  };
>  
>  static int rswitch_stop(struct net_device *ndev)
> @@ -1625,6 +1558,8 @@ static int rswitch_stop(struct net_device *ndev)
>  	bitmap_clear(rdev->priv->opened_ports, rdev->port, 1);
>  	spin_unlock_irqrestore(&rdev->priv->lock, flags);
>  
> +	free_irq(rdev->irq, rdev);
> +
>  	napi_disable(&rdev->napi);
>  
>  	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
> @@ -1906,6 +1841,34 @@ static void rswitch_etha_init(struct rswitch_private *priv, unsigned int index)
>  	etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
>  }
>  
> +static int rswitch_port_get_irq(struct rswitch_device *rdev)
> +{
> +	unsigned int irq_index;
> +	char *name;
> +	int err;
> +
> +	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
> +	if (err == 0) {
Usually if (!err) is used.

> +		if (irq_index < GWCA_NUM_IRQS)
> +			rdev->irq_index = irq_index;
> +		else
> +			dev_warn(&rdev->priv->pdev->dev,
> +				 "%pOF: irq-index out of range\n",
> +				 rdev->np_port);
Why not return here? It is a little counter intuitive, maybe:
if (err) {
	dev_warn();
	return -ERR;
}

if (irq_index < NUM_IRQS) {
	dev_warn();
	return -ERR;
}
> +	}
> +
> +	name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);

In case with not returning you are using invalid rdev_irq_index here
(probably 0, so may it be fine, I am only wondering).

> +	if (!name)
> +		return -ENOMEM;
> +	err = platform_get_irq_byname(rdev->priv->pdev, name);
> +	kfree(name);
> +	if (err < 0)
> +		return err;
> +	rdev->irq = err;

If you will be changing sth here consider:
rdev->irq = platform()
if (rdev->irq < 0)
	return rdev->irq;
> +
> +	return 0;
> +}
> +
>  static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index)
>  {
>  	struct platform_device *pdev = priv->pdev;
> @@ -1954,6 +1917,10 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
>  	if (err < 0)
>  		goto out_get_params;
>  
> +	err = rswitch_port_get_irq(rdev);
> +	if (err < 0)
You are returning 0 in case of success, the netdev code style is to
check it like that: if (!err)

> +		goto out_get_irq;
If you will use the label name according to what does happen under label
you will not have to add another one. Feel free to leave it as it is, as
you have the same scheme across driver with is completle fine. You can
check Przemek's answer according "came from" convention [1].

[1] https://lore.kernel.org/netdev/20241218150949.1037752-1-tariqt@nvidia.com/T/#m577436e76d3d1afce18676ad87be74e8f5b3cc02
> +
>  	err = rswitch_rxdmac_alloc(ndev);
>  	if (err < 0)
>  		goto out_rxdmac;
> @@ -1968,6 +1935,7 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
>  	rswitch_rxdmac_free(ndev);
>  
>  out_rxdmac:
> +out_get_irq:
>  out_get_params:
>  	of_node_put(rdev->np_port);
>  	netif_napi_del(&rdev->napi);
> @@ -2003,7 +1971,6 @@ static int rswitch_init(struct rswitch_private *priv)
>  	rswitch_reset(priv);
>  
>  	rswitch_clock_enable(priv);
> -	rswitch_top_init(priv);
>  	err = rswitch_bpool_config(priv);
>  	if (err < 0)
>  		return err;
> @@ -2034,10 +2001,6 @@ static int rswitch_init(struct rswitch_private *priv)
>  	if (err < 0)
>  		goto err_ptp_register;
>  
> -	err = rswitch_gwca_request_irqs(priv);
> -	if (err < 0)
> -		goto err_gwca_request_irq;
> -
>  	err = rswitch_gwca_ts_request_irqs(priv);
>  	if (err < 0)
>  		goto err_gwca_ts_request_irq;
> @@ -2073,7 +2036,6 @@ static int rswitch_init(struct rswitch_private *priv)
>  
>  err_gwca_hw_init:
>  err_gwca_ts_request_irq:
> -err_gwca_request_irq:
>  	rcar_gen4_ptp_unregister(priv->ptp_priv);
>  
>  err_ptp_register:
> diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
> index 532192cbca4b..a1e62a6b3844 100644
> --- a/drivers/net/ethernet/renesas/rswitch.h
> +++ b/drivers/net/ethernet/renesas/rswitch.h
> @@ -51,7 +51,6 @@
>  
>  /* TODO: hardcoded ETHA/GWCA settings for now */
>  #define GWCA_IRQ_RESOURCE_NAME	"gwca0_rxtx%d"
> -#define GWCA_IRQ_NAME		"rswitch: gwca0_rxtx%d"
>  #define GWCA_NUM_IRQS		8
>  #define GWCA_INDEX		0
>  #define AGENT_INDEX_GWCA	3
> @@ -828,6 +827,10 @@ enum rswitch_gwca_mode {
>  
>  /* TOP */
>  #define TPEMIMC7(queue)		(TPEMIMC70 + (queue) * 4)
> +#define TPEMIMC7_GDICM0			GENMASK(2, 0)
> +#define TPEMIMC7_GDICM_SHIFT(i)		((i) * 8)
> +#define TPEMIMC7_GDICM(i)		(TPEMIMC7_GDICM0 << TPEMIMC7_GDICM_SHIFT(i))
> +#define TPEMIMC7_GDICM_PREP(i, val)	(((val) & TPEMIMC7_GDICM0) << TPEMIMC7_GDICM_SHIFT(i))
>  
>  /* Descriptors */
>  enum RX_DS_CC_BIT {
> @@ -967,7 +970,6 @@ struct rswitch_gwca_queue {
>  	};
>  };
>  
> -#define RSWITCH_NUM_IRQ_REGS	(RSWITCH_MAX_NUM_QUEUES / BITS_PER_TYPE(u32))
>  struct rswitch_gwca {
>  	unsigned int index;
>  	struct rswitch_desc *linkfix_table;
> @@ -977,8 +979,6 @@ struct rswitch_gwca {
>  	int num_queues;
>  	struct rswitch_gwca_queue ts_queue;
>  	DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
> -	u32 tx_irq_bits[RSWITCH_NUM_IRQ_REGS];
> -	u32 rx_irq_bits[RSWITCH_NUM_IRQ_REGS];
>  };
>  
>  #define NUM_QUEUES_PER_NDEV	2
> @@ -990,6 +990,8 @@ struct rswitch_device {
>  	void __iomem *addr;
>  	struct rswitch_gwca_queue *tx_queue;
>  	struct rswitch_gwca_queue *rx_queue;
> +	unsigned int irq_index;
> +	int irq;
>  	struct sk_buff *ts_skb[TS_TAGS_PER_PORT];
>  	DECLARE_BITMAP(ts_skb_used, TS_TAGS_PER_PORT);
>  	bool disabled;
> -- 
> 2.39.5

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Nikita Yushchenko]

>> +	ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED,
> It wasn't shared previously, maybe some notes in commit message about
> that.

It can be shared between several ports.

I will try to rephrase the commit message to make this stated explicitly.

>> +	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
>> +	if (err == 0) {
> Usually if (!err) is used.

Ok, will fix it.

> 
>> +		if (irq_index < GWCA_NUM_IRQS)
>> +			rdev->irq_index = irq_index;
>> +		else
>> +			dev_warn(&rdev->priv->pdev->dev,
>> +				 "%pOF: irq-index out of range\n",
>> +				 rdev->np_port);
> Why not return here? It is a little counter intuitive, maybe:
> if (err) {
> 	dev_warn();
> 	return -ERR;
> }

It is meant to be optional, not having it defined shall not be an error

> if (irq_index < NUM_IRQS) {
> 	dev_warn();
> 	return -ERR;
> }

Ok - although if erroring out, I think it shall be dev_err.

>> +	}
>> +
>> +	name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);
> 
> In case with not returning you are using invalid rdev_irq_index here
> (probably 0, so may it be fine, I am only wondering).

Yes, the field is zero-initialized and that zero is a sane default.

> 
>> +	if (!name)
>> +		return -ENOMEM;
>> +	err = platform_get_irq_byname(rdev->priv->pdev, name);
>> +	kfree(name);
>> +	if (err < 0)
>> +		return err;
>> +	rdev->irq = err;
> 
> If you will be changing sth here consider:
> rdev->irq = platform()
> if (rdev->irq < 0)
> 	return rdev->irq;

Ok

>> +	err = rswitch_port_get_irq(rdev);
>> +	if (err < 0)
> You are returning 0 in case of success, the netdev code style is to
> check it like that: if (!err)

I tried to follow the style already existing in the driver.
Several checks just above and below are written this way.
Shall I add this one check written differently?

> 
>> +		goto out_get_irq;
> If you will use the label name according to what does happen under label
> you will not have to add another one. Feel free to leave it as it is, as
> you have the same scheme across driver with is completle fine. You can
> check Przemek's answer according "came from" convention [1].

Again, following existing style here.

My personal opinion is that "came from" labels are more reliable against future changes than other label 
styles. But if there is maintainer requirement here then definitely I will follow.

Nikita

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Michal Swiatkowski]

On Fri, Dec 20, 2024 at 02:11:26PM +0500, Nikita Yushchenko wrote:
> > > +	ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED,
> > It wasn't shared previously, maybe some notes in commit message about
> > that.
> 
> It can be shared between several ports.
> 
> I will try to rephrase the commit message to make this stated explicitly.
> 
> > > +	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
> > > +	if (err == 0) {
> > Usually if (!err) is used.
> 
> Ok, will fix it.
> 
> > 
> > > +		if (irq_index < GWCA_NUM_IRQS)
> > > +			rdev->irq_index = irq_index;
> > > +		else
> > > +			dev_warn(&rdev->priv->pdev->dev,
> > > +				 "%pOF: irq-index out of range\n",
> > > +				 rdev->np_port);
> > Why not return here? It is a little counter intuitive, maybe:
> > if (err) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> It is meant to be optional, not having it defined shall not be an error
> 
> > if (irq_index < NUM_IRQS) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> Ok - although if erroring out, I think it shall be dev_err.
> 
> > > +	}
> > > +
> > > +	name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);
> > 
> > In case with not returning you are using invalid rdev_irq_index here
> > (probably 0, so may it be fine, I am only wondering).
> 
> Yes, the field is zero-initialized and that zero is a sane default.
> 
> > 
> > > +	if (!name)
> > > +		return -ENOMEM;
> > > +	err = platform_get_irq_byname(rdev->priv->pdev, name);
> > > +	kfree(name);
> > > +	if (err < 0)
> > > +		return err;
> > > +	rdev->irq = err;
> > 
> > If you will be changing sth here consider:
> > rdev->irq = platform()
> > if (rdev->irq < 0)
> > 	return rdev->irq;
> 
> Ok
> 
> > > +	err = rswitch_port_get_irq(rdev);
> > > +	if (err < 0)
> > You are returning 0 in case of success, the netdev code style is to
> > check it like that: if (!err)
> 
> I tried to follow the style already existing in the driver.
> Several checks just above and below are written this way.
> Shall I add this one check written differently?
> 

I am fine with following exsisting style.

Thanks

> > 
> > > +		goto out_get_irq;
> > If you will use the label name according to what does happen under label
> > you will not have to add another one. Feel free to leave it as it is, as
> > you have the same scheme across driver with is completle fine. You can
> > check Przemek's answer according "came from" convention [1].
> 
> Again, following existing style here.
> 
> My personal opinion is that "came from" labels are more reliable against
> future changes than other label styles. But if there is maintainer
> requirement here then definitely I will follow.
> 
> Nikita

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Michal Swiatkowski]

On Fri, Dec 20, 2024 at 02:11:26PM +0500, Nikita Yushchenko wrote:
> > > +	ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED,
> > It wasn't shared previously, maybe some notes in commit message about
> > that.
> 
> It can be shared between several ports.
> 
> I will try to rephrase the commit message to make this stated explicitly.
> 
> > > +	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
> > > +	if (err == 0) {
> > Usually if (!err) is used.
> 
> Ok, will fix it.
> 
> > 
> > > +		if (irq_index < GWCA_NUM_IRQS)
> > > +			rdev->irq_index = irq_index;
> > > +		else
> > > +			dev_warn(&rdev->priv->pdev->dev,
> > > +				 "%pOF: irq-index out of range\n",
> > > +				 rdev->np_port);
> > Why not return here? It is a little counter intuitive, maybe:
> > if (err) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> It is meant to be optional, not having it defined shall not be an error
> 
> > if (irq_index < NUM_IRQS) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> Ok - although if erroring out, I think it shall be dev_err.
> 
> > > +	}
> > > +
> > > +	name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);
> > 
> > In case with not returning you are using invalid rdev_irq_index here
> > (probably 0, so may it be fine, I am only wondering).
> 
> Yes, the field is zero-initialized and that zero is a sane default.
> 
> > 
> > > +	if (!name)
> > > +		return -ENOMEM;
> > > +	err = platform_get_irq_byname(rdev->priv->pdev, name);
> > > +	kfree(name);
> > > +	if (err < 0)
> > > +		return err;
> > > +	rdev->irq = err;
> > 
> > If you will be changing sth here consider:
> > rdev->irq = platform()
> > if (rdev->irq < 0)
> > 	return rdev->irq;
> 
> Ok
> 
> > > +	err = rswitch_port_get_irq(rdev);
> > > +	if (err < 0)
> > You are returning 0 in case of success, the netdev code style is to
> > check it like that: if (!err)
> 
> I tried to follow the style already existing in the driver.
> Several checks just above and below are written this way.
> Shall I add this one check written differently?
> 

Just follow the style. (Sorry for late replay, I was OOO).

> > 
> > > +		goto out_get_irq;
> > If you will use the label name according to what does happen under label
> > you will not have to add another one. Feel free to leave it as it is, as
> > you have the same scheme across driver with is completle fine. You can
> > check Przemek's answer according "came from" convention [1].
> 
> Again, following existing style here.
> 
> My personal opinion is that "came from" labels are more reliable against
> future changes than other label styles. But if there is maintainer
> requirement here then definitely I will follow.
> 
> Nikita

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Andrew Lunn]

On Fri, Dec 20, 2024 at 09:16:58AM +0500, Nikita Yushchenko wrote:
> Instead of handling all possible data interrupts in the same handler,
> switch to per-port handlers.
> 
> This significantly simplifies handling: when the same interrupt is used
> for several ports, system calls all handlers, and each handler only has
> to check interrupts for one port's tx and rx queues.
> 
> But it is not required to use the same interrupt for all ports - GWCA
> provides 8 data interrupts and allows arbitrary per-queue assignment
> of those. Support that by reading interrupt index for each port from
> optional 'irq-index' device tree property.

It has been pointed out that adding this property breaks backwards
compatibility with older DT blobs.

I don't know this hardware...

How many ports are there? Less than 9? Can you just do a static
allocation, port 0 gets interrupt 0, port 1 interrupt 1...

	Andrew

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Nikita Yushchenko]

20.12.2024 14:19, Andrew Lunn wrote:
> On Fri, Dec 20, 2024 at 09:16:58AM +0500, Nikita Yushchenko wrote:
>> Instead of handling all possible data interrupts in the same handler,
>> switch to per-port handlers.
>>
>> This significantly simplifies handling: when the same interrupt is used
>> for several ports, system calls all handlers, and each handler only has
>> to check interrupts for one port's tx and rx queues.
>>
>> But it is not required to use the same interrupt for all ports - GWCA
>> provides 8 data interrupts and allows arbitrary per-queue assignment
>> of those. Support that by reading interrupt index for each port from
>> optional 'irq-index' device tree property.
> 
> It has been pointed out that adding this property breaks backwards
> compatibility with older DT blobs. 

It does not break backwards compatibility.

Current behavior is that everything is serviced by interrupt 0.
And in case of irq-index not defined, the fallback is exactly that.

(physically there is code that assigns interrupts per chain index, but in the current driver chains that 
get non-zero interrupts assigned are never used; anso currently multiple interrupts are just multiple 
entries to the exactly same handler that always services everything)

> I don't know this hardware...
> 
> How many ports are there? Less than 9? Can you just do a static
> allocation, port 0 gets interrupt 0, port 1 interrupt 1...

There are only 3 physical ports, however the use case I'm targeting is - virtual ports serving virtual 
machines (with offloading features making hardware directly L2-forward or L3-route most traffic between 
outside world and VM-owned virtual port frontends). In this setup, some of 8 GWCA irqs will be given to 
VMs and thus there are definitely not enough to per-consumer allocation.

Nikita

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Andrew Lunn]

> There are only 3 physical ports, however the use case I'm targeting is -
> virtual ports serving virtual machines (with offloading features making
> hardware directly L2-forward or L3-route most traffic between outside world
> and VM-owned virtual port frontends). In this setup, some of 8 GWCA irqs
> will be given to VMs and thus there are definitely not enough to
> per-consumer allocation.

And you are describing your VMs in DT as well? And if you change your
VM setup, you are going to modify your DT? This all sounds wrong.

It sounds like you need to be able to configure the interrupt mappings
from user space somehow. And you probably need to be able to do it on
the fly, or at least, when the interface is down.

I don't know if it will help, but ethtool mentions:

       -l --show-channels
              Queries the specified network device for the numbers of  channels
              it has.  A channel is an IRQ and the set of queues that can trig‐
              ger that IRQ.

       -L --set-channels
              Changes the numbers of channels of the specified network device.

           rx N   Changes the number of channels with only receive queues.

           tx N   Changes the number of channels with only transmit queues.

           other N
                  Changes  the  number of channels used only for other purposes
                  e.g. link interrupts or SR-IOV co-ordination.

           combined N
                  Changes the number of multi-purpose channels.


Maybe there is something you can use here? Or extend.

	Andrew

Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Nikita Yushchenko]

>> There are only 3 physical ports, however the use case I'm targeting is -
>> virtual ports serving virtual machines (with offloading features making
>> hardware directly L2-forward or L3-route most traffic between outside world
>> and VM-owned virtual port frontends). In this setup, some of 8 GWCA irqs
>> will be given to VMs and thus there are definitely not enough to
>> per-consumer allocation.
> 
> And you are describing your VMs in DT as well? And if you change your
> VM setup, you are going to modify your DT? This all sounds wrong.

Since this is for embedded, particular setups will likely be static... so defining driver configuration 
in device tree suits the needs quite well.

Still, if this is considered as device tree misuse, I will implement some other solution.

Maybe, add a sysfs_group to port netdev and have irq_index attribute there?
Then, target rootfs will be able to configure that via udev rules file.

> I don't know if it will help, but ethtool mentions:
> 
>         -l --show-channels
> ... 
>         -L --set-channels

I believe this is for multi-queue devices configuration, which is a somewhat different thing.

Nikita

[PATCH net-next 2/2] net: renesas: rswitch: request ts interrupt at port open

[Nikita Yushchenko]

Data interrupts are now requested at port open and freed at port close.

For symmetry, do the same for ts interrupt.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 35 +++++++++++++-------------
 drivers/net/ethernet/renesas/rswitch.h |  2 +-
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index eb9dea8b16f3..cc8f2a4e3d70 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -989,18 +989,6 @@ static irqreturn_t rswitch_gwca_ts_irq(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-static int rswitch_gwca_ts_request_irqs(struct rswitch_private *priv)
-{
-	int irq;
-
-	irq = platform_get_irq_byname(priv->pdev, GWCA_TS_IRQ_RESOURCE_NAME);
-	if (irq < 0)
-		return irq;
-
-	return devm_request_irq(&priv->pdev->dev, irq, rswitch_gwca_ts_irq,
-				0, GWCA_TS_IRQ_NAME, priv);
-}
-
 /* Ethernet TSN Agent block (ETHA) and Ethernet MAC IP block (RMAC) */
 static int rswitch_etha_change_mode(struct rswitch_etha *etha,
 				    enum rswitch_etha_mode mode)
@@ -1510,8 +1498,14 @@ static int rswitch_open(struct net_device *ndev)
 	unsigned long flags;
 	int ret;
 
-	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
+	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS)) {
+		ret = request_irq(rdev->priv->gwca.ts_irq, rswitch_gwca_ts_irq,
+				  0, "rswitch_ts", rdev->priv);
+		if (ret < 0)
+			return ret;
+
 		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDIE);
+	}
 
 	napi_enable(&rdev->napi);
 
@@ -1535,8 +1529,10 @@ static int rswitch_open(struct net_device *ndev)
 err_request_irq:
 	napi_disable(&rdev->napi);
 
-	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
+	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS)) {
 		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDID);
+		free_irq(rdev->priv->gwca.ts_irq, rdev->priv);
+	}
 
 	return ret;
 };
@@ -1562,8 +1558,10 @@ static int rswitch_stop(struct net_device *ndev)
 
 	napi_disable(&rdev->napi);
 
-	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
+	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS)) {
 		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDID);
+		free_irq(rdev->priv->gwca.ts_irq, rdev->priv);
+	}
 
 	for (tag = find_first_bit(rdev->ts_skb_used, TS_TAGS_PER_PORT);
 	     tag < TS_TAGS_PER_PORT;
@@ -2001,9 +1999,10 @@ static int rswitch_init(struct rswitch_private *priv)
 	if (err < 0)
 		goto err_ptp_register;
 
-	err = rswitch_gwca_ts_request_irqs(priv);
+	err = platform_get_irq_byname(priv->pdev, GWCA_TS_IRQ_RESOURCE_NAME);
 	if (err < 0)
-		goto err_gwca_ts_request_irq;
+		goto err_gwca_ts_irq;
+	priv->gwca.ts_irq = err;
 
 	err = rswitch_gwca_hw_init(priv);
 	if (err < 0)
@@ -2035,7 +2034,7 @@ static int rswitch_init(struct rswitch_private *priv)
 	rswitch_gwca_hw_deinit(priv);
 
 err_gwca_hw_init:
-err_gwca_ts_request_irq:
+err_gwca_ts_irq:
 	rcar_gen4_ptp_unregister(priv->ptp_priv);
 
 err_ptp_register:
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index a1e62a6b3844..54b9f059707a 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -58,7 +58,6 @@
 #define GWRO			RSWITCH_GWCA0_OFFSET
 
 #define GWCA_TS_IRQ_RESOURCE_NAME	"gwca0_rxts0"
-#define GWCA_TS_IRQ_NAME		"rswitch: gwca0_rxts0"
 #define GWCA_TS_IRQ_BIT			BIT(0)
 
 #define FWRO	0
@@ -978,6 +977,7 @@ struct rswitch_gwca {
 	struct rswitch_gwca_queue *queues;
 	int num_queues;
 	struct rswitch_gwca_queue ts_queue;
+	int ts_irq;
 	DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
 };
 
-- 
2.39.5

Re: [PATCH net-next 2/2] net: renesas: rswitch: request ts interrupt at port open

[Michal Swiatkowski]

On Fri, Dec 20, 2024 at 09:16:59AM +0500, Nikita Yushchenko wrote:
> Data interrupts are now requested at port open and freed at port close.
> 
> For symmetry, do the same for ts interrupt.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>  drivers/net/ethernet/renesas/rswitch.c | 35 +++++++++++++-------------
>  drivers/net/ethernet/renesas/rswitch.h |  2 +-
>  2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index eb9dea8b16f3..cc8f2a4e3d70 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -989,18 +989,6 @@ static irqreturn_t rswitch_gwca_ts_irq(int irq, void *dev_id)
>  	return IRQ_NONE;
>  }
>  
> -static int rswitch_gwca_ts_request_irqs(struct rswitch_private *priv)
> -{
> -	int irq;
> -
> -	irq = platform_get_irq_byname(priv->pdev, GWCA_TS_IRQ_RESOURCE_NAME);
> -	if (irq < 0)
> -		return irq;
> -
> -	return devm_request_irq(&priv->pdev->dev, irq, rswitch_gwca_ts_irq,
> -				0, GWCA_TS_IRQ_NAME, priv);
> -}
> -
>  /* Ethernet TSN Agent block (ETHA) and Ethernet MAC IP block (RMAC) */
>  static int rswitch_etha_change_mode(struct rswitch_etha *etha,
>  				    enum rswitch_etha_mode mode)
> @@ -1510,8 +1498,14 @@ static int rswitch_open(struct net_device *ndev)
>  	unsigned long flags;
>  	int ret;
>  
> -	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
> +	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS)) {
> +		ret = request_irq(rdev->priv->gwca.ts_irq, rswitch_gwca_ts_irq,
> +				  0, "rswitch_ts", rdev->priv);
> +		if (ret < 0)
> +			return ret;
> +
>  		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDIE);
> +	}
>  
>  	napi_enable(&rdev->napi);
>  
> @@ -1535,8 +1529,10 @@ static int rswitch_open(struct net_device *ndev)
>  err_request_irq:
>  	napi_disable(&rdev->napi);
>  
> -	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
> +	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS)) {
>  		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDID);
> +		free_irq(rdev->priv->gwca.ts_irq, rdev->priv);
> +	}
>  
>  	return ret;
>  };
> @@ -1562,8 +1558,10 @@ static int rswitch_stop(struct net_device *ndev)
>  
>  	napi_disable(&rdev->napi);
>  
> -	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS))
> +	if (bitmap_empty(rdev->priv->opened_ports, RSWITCH_NUM_PORTS)) {
>  		iowrite32(GWCA_TS_IRQ_BIT, rdev->priv->addr + GWTSDID);
> +		free_irq(rdev->priv->gwca.ts_irq, rdev->priv);
> +	}
>  
>  	for (tag = find_first_bit(rdev->ts_skb_used, TS_TAGS_PER_PORT);
>  	     tag < TS_TAGS_PER_PORT;
> @@ -2001,9 +1999,10 @@ static int rswitch_init(struct rswitch_private *priv)
>  	if (err < 0)
>  		goto err_ptp_register;
>  
> -	err = rswitch_gwca_ts_request_irqs(priv);
> +	err = platform_get_irq_byname(priv->pdev, GWCA_TS_IRQ_RESOURCE_NAME);
>  	if (err < 0)
> -		goto err_gwca_ts_request_irq;
> +		goto err_gwca_ts_irq;
> +	priv->gwca.ts_irq = err;
>  
>  	err = rswitch_gwca_hw_init(priv);
>  	if (err < 0)
> @@ -2035,7 +2034,7 @@ static int rswitch_init(struct rswitch_private *priv)
>  	rswitch_gwca_hw_deinit(priv);
>  
>  err_gwca_hw_init:
> -err_gwca_ts_request_irq:
> +err_gwca_ts_irq:
>  	rcar_gen4_ptp_unregister(priv->ptp_priv);
>  
>  err_ptp_register:
> diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
> index a1e62a6b3844..54b9f059707a 100644
> --- a/drivers/net/ethernet/renesas/rswitch.h
> +++ b/drivers/net/ethernet/renesas/rswitch.h
> @@ -58,7 +58,6 @@
>  #define GWRO			RSWITCH_GWCA0_OFFSET
>  
>  #define GWCA_TS_IRQ_RESOURCE_NAME	"gwca0_rxts0"
> -#define GWCA_TS_IRQ_NAME		"rswitch: gwca0_rxts0"
>  #define GWCA_TS_IRQ_BIT			BIT(0)
>  
>  #define FWRO	0
> @@ -978,6 +977,7 @@ struct rswitch_gwca {
>  	struct rswitch_gwca_queue *queues;
>  	int num_queues;
>  	struct rswitch_gwca_queue ts_queue;
> +	int ts_irq;
>  	DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
>  };

Wasn't previous implementation more obvious? This ts irq you have one
per device, no per port, so it better fit to one time initialization
instead of checking if it is first and last port.

Anyway, it is your descision, patch looks correct:
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

>  
> -- 
> 2.39.5

Re: [PATCH net-next 2/2] net: renesas: rswitch: request ts interrupt at port open

[Nikita Yushchenko]

> Wasn't previous implementation more obvious? This ts irq you have one
> per device, no per port, so it better fit to one time initialization
> instead of checking if it is first and last port.
> 
> Anyway, it is your descision, patch looks correct:
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

For data interrupts, after making them per-port, it is better to request only for opened device, to 
avoid unneeded calls to shared handlers when some ports are up and some are not.

And once data interrupts are requested at open, it looks cleaner for me to request ts interrupt at open 
as well. Although I agree that this is matter of taste.

Nikita