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

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