Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Prabhakar,

On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> allocation of IRQ resources in DT core code, this causes an issue
> when using hierarchical interrupt domains using "interrupts" property
> in the node as this bypasses the hierarchical setup and messes up the
> irq chaining.

Thanks for your patch!

> In preparation for removal of static setup of IRQ resource from DT core
> code use platform_get_irq_optional() for DT users only.

Why only for DT users?
Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
(non-DT) SoCs already uses platform_get_irq_optional(), so I expect
that to work for both.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
>
>  static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
>  {
> -       struct resource *res;
> -       resource_size_t n;
> +       struct device_node *np = dev_of_node(&dev->dev);
>         int k = 0, ret;
>
> -       while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> -               for (n = res->start; n <= res->end; n++) {
> -                       ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> -                                         0, dev_name(&dev->dev), pd);
> +       if (!np) {
> +               struct resource *res;
> +               resource_size_t n;
> +
> +               while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> +                       for (n = res->start; n <= res->end; n++) {
> +                               ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> +                                                      0, dev_name(&dev->dev), pd);
> +                               if (ret) {
> +                                       dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> +                                       return ret;
> +                               }
> +                       }
> +                       k++;
> +               }
> +       } else {
> +               int irq;
> +
> +               do {
> +                       irq = platform_get_irq_optional(dev, k);

Check for irq == -ENXIO first, to simplify the checks below?

> +                       if (irq <= 0 && irq != -ENXIO)
> +                               return irq ? irq : -ENXIO;

Can irq == 0 really happen?

All SuperH users of the "i2c-sh_mobile" platform device use an
evt2irq() value that is non-zero.

I might have missed something, but it seems the only user of IRQ 0 on
SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
These should have been seeing the "0 is an invalid IRQ number"
warning splat since it was introduced in commit a85a6c86c25be2d2
("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
the rare users may not have upgraded their kernels beyond v5.8 yet...

> +                       if (irq == -ENXIO)
> +                               break;
> +                       ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> +                                              0, dev_name(&dev->dev), pd);
>                         if (ret) {
> -                               dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> +                               dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
>                                 return ret;
>                         }
> -               }
> -               k++;
> +                       k++;
> +               } while (irq);
>         }
>
>         return k > 0 ? 0 : -ENOENT;

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 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 20.12.2021 13:17, Geert Uytterhoeven wrote:

[...]
>> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
>> allocation of IRQ resources in DT core code, this causes an issue
>> when using hierarchical interrupt domains using "interrupts" property
>> in the node as this bypasses the hierarchical setup and messes up the
>> irq chaining.
> 
> Thanks for your patch!
> 
>> In preparation for removal of static setup of IRQ resource from DT core
>> code use platform_get_irq_optional() for DT users only.
> 
> Why only for DT users?
> Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> that to work for both.
> 
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
>> @@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
>>
>>   static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
>>   {
>> -       struct resource *res;
>> -       resource_size_t n;
>> +       struct device_node *np = dev_of_node(&dev->dev);
>>          int k = 0, ret;
>>
>> -       while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
>> -               for (n = res->start; n <= res->end; n++) {
>> -                       ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
>> -                                         0, dev_name(&dev->dev), pd);
>> +       if (!np) {
>> +               struct resource *res;
>> +               resource_size_t n;
>> +
>> +               while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
>> +                       for (n = res->start; n <= res->end; n++) {
>> +                               ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
>> +                                                      0, dev_name(&dev->dev), pd);
>> +                               if (ret) {
>> +                                       dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
>> +                                       return ret;
>> +                               }
>> +                       }
>> +                       k++;
>> +               }
>> +       } else {
>> +               int irq;
>> +
>> +               do {
>> +                       irq = platform_get_irq_optional(dev, k);
> 
> Check for irq == -ENXIO first, to simplify the checks below?
> 
>> +                       if (irq <= 0 && irq != -ENXIO)
>> +                               return irq ? irq : -ENXIO;
> 
> Can irq == 0 really happen?

    Doesn't matter much in this case -- devm_request_irq() happily takes IRQ0. :-)

> All SuperH users of the "i2c-sh_mobile" platform device use an
> evt2irq() value that is non-zero.
> 
> I might have missed something, but it seems the only user of IRQ 0 on
> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> These should have been seeing the "0 is an invalid IRQ number"
> warning splat since it was introduced in commit a85a6c86c25be2d2
> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:

    Warning or no warning, 0 is still returned. :-/
    My attempt to put an end to this has stuck waiting a review from the IRQ 
people...

> the rare users may not have upgraded their kernels beyond v5.8 yet...
> 
>> +                       if (irq == -ENXIO)
>> +                               break;
>> +                       ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
>> +                                              0, dev_name(&dev->dev), pd);
>>                          if (ret) {
>> -                               dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
>> +                               dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
>>                                  return ret;
>>                          }
>> -               }
>> -               k++;
>> +                       k++;
>> +               } while (irq);
>>          }
>>
>>          return k > 0 ? 0 : -ENOENT;
> 
> 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 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > allocation of IRQ resources in DT core code, this causes an issue
> > when using hierarchical interrupt domains using "interrupts" property
> > in the node as this bypasses the hierarchical setup and messes up the
> > irq chaining.
>
> Thanks for your patch!
>
> > In preparation for removal of static setup of IRQ resource from DT core
> > code use platform_get_irq_optional() for DT users only.
>
> Why only for DT users?
> Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> that to work for both.
>
For the non DT users the IRQ resource is passed as a range [0] and not
a single interrupt so I went with this approach. Is there a way I'm
missing where we could still use platform_get_irq_xyz() variants for
such cases?

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
> >
> >  static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
> >  {
> > -       struct resource *res;
> > -       resource_size_t n;
> > +       struct device_node *np = dev_of_node(&dev->dev);
> >         int k = 0, ret;
> >
> > -       while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > -               for (n = res->start; n <= res->end; n++) {
> > -                       ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > -                                         0, dev_name(&dev->dev), pd);
> > +       if (!np) {
> > +               struct resource *res;
> > +               resource_size_t n;
> > +
> > +               while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > +                       for (n = res->start; n <= res->end; n++) {
> > +                               ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > +                                                      0, dev_name(&dev->dev), pd);
> > +                               if (ret) {
> > +                                       dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                                       return ret;
> > +                               }
> > +                       }
> > +                       k++;
> > +               }
> > +       } else {
> > +               int irq;
> > +
> > +               do {
> > +                       irq = platform_get_irq_optional(dev, k);
>
> Check for irq == -ENXIO first, to simplify the checks below?
>
OK.

> > +                       if (irq <= 0 && irq != -ENXIO)
> > +                               return irq ? irq : -ENXIO;
>
> Can irq == 0 really happen?
>
> All SuperH users of the "i2c-sh_mobile" platform device use an
> evt2irq() value that is non-zero.
>
> I might have missed something, but it seems the only user of IRQ 0 on
> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
>
I'll keep that in mind if the Ethernet driver falls in the convection
patch changes.

> These should have been seeing the "0 is an invalid IRQ number"
> warning splat since it was introduced in commit a85a6c86c25be2d2
> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
> the rare users may not have upgraded their kernels beyond v5.8 yet...
>
Might be users have not updated their kernels.

[0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454

Cheers,
Prabhakar
> > +                       if (irq == -ENXIO)
> > +                               break;
> > +                       ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> > +                                              0, dev_name(&dev->dev), pd);
> >                         if (ret) {
> > -                               dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                               dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
> >                                 return ret;
> >                         }
> > -               }
> > -               k++;
> > +                       k++;
> > +               } while (irq);
> >         }
> >
> >         return k > 0 ? 0 : -ENOENT;
>
> 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 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Dec 20, 2021 at 12:56 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > > allocation of IRQ resources in DT core code, this causes an issue
> > > when using hierarchical interrupt domains using "interrupts" property
> > > in the node as this bypasses the hierarchical setup and messes up the
> > > irq chaining.
> >
> > Thanks for your patch!
> >
> > > In preparation for removal of static setup of IRQ resource from DT core
> > > code use platform_get_irq_optional() for DT users only.
> >
> > Why only for DT users?
> > Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> > (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> > that to work for both.
> >
> For the non DT users the IRQ resource is passed as a range [0] and not
> a single interrupt so I went with this approach. Is there a way I'm
> missing where we could still use platform_get_irq_xyz() variants for
> such cases?

Oh, I didn't realize it used a single resource with a range.
Is this common, i.e. would it make sense to add support for this to
platform_get_irq_optional()?

> > > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > +++ b/drivers/i2c/busses/i2c-sh_mobile.c

> > > +                       if (irq <= 0 && irq != -ENXIO)
> > > +                               return irq ? irq : -ENXIO;
> >
> > Can irq == 0 really happen?
> >
> > All SuperH users of the "i2c-sh_mobile" platform device use an
> > evt2irq() value that is non-zero.
> >
> > I might have missed something, but it seems the only user of IRQ 0 on
> > SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> > arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> >
> I'll keep that in mind if the Ethernet driver falls in the convection
> patch changes.

The Ethernet driver was converted 6 years ago, cfr. commit
965b2aa78fbcb831 ("net/smsc911x: fix irq resource allocation failure").

> [0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454

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 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Geert,

On Mon, Dec 20, 2021 at 12:54 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Dec 20, 2021 at 12:56 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > > > allocation of IRQ resources in DT core code, this causes an issue
> > > > when using hierarchical interrupt domains using "interrupts" property
> > > > in the node as this bypasses the hierarchical setup and messes up the
> > > > irq chaining.
> > >
> > > Thanks for your patch!
> > >
> > > > In preparation for removal of static setup of IRQ resource from DT core
> > > > code use platform_get_irq_optional() for DT users only.
> > >
> > > Why only for DT users?
> > > Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> > > (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> > > that to work for both.
> > >
> > For the non DT users the IRQ resource is passed as a range [0] and not
> > a single interrupt so I went with this approach. Is there a way I'm
> > missing where we could still use platform_get_irq_xyz() variants for
> > such cases?
>
> Oh, I didn't realize it used a single resource with a range.
> Is this common, i.e. would it make sense to add support for this to
> platform_get_irq_optional()?
>
No this isn't common even non dt users should ideally be passing a
single IRQ resource. There are very few such platforms which do this
so I don't see any point in adding this support to
platform_get_irq_optional() unless the IRQ maintainers think otherwise.

> > > > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
>
> > > > +                       if (irq <= 0 && irq != -ENXIO)
> > > > +                               return irq ? irq : -ENXIO;
> > >
> > > Can irq == 0 really happen?
> > >
> > > All SuperH users of the "i2c-sh_mobile" platform device use an
> > > evt2irq() value that is non-zero.
> > >
> > > I might have missed something, but it seems the only user of IRQ 0 on
> > > SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> > > arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> > >
> > I'll keep that in mind if the Ethernet driver falls in the convection
> > patch changes.
>
> The Ethernet driver was converted 6 years ago, cfr. commit
> 965b2aa78fbcb831 ("net/smsc911x: fix irq resource allocation failure").
>
Thanks for the pointer.

Cheers,
Prabhakar

> > [0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454
>
> 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 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Mon, Dec 20, 2021 at 12:53 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 20.12.2021 13:17, Geert Uytterhoeven wrote:
>
> > I might have missed something, but it seems the only user of IRQ 0 on
> > SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> > arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> > These should have been seeing the "0 is an invalid IRQ number"
> > warning splat since it was introduced in commit a85a6c86c25be2d2
> > ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
>
>     Warning or no warning, 0 is still returned. :-/
>     My attempt to put an end to this has stuck waiting a review from the IRQ
> people...

I had another look at this after you asked about it on IRC. I don't
know much SH assembly, but I suspect IRQ 0 has not been delivered
since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
related note, CONFIG_INTC_BALANCING was broken in 2be6bb0c79c7
("sh: intc: Split up the INTC code.") by inadvertently removing the Kconfig
symbol.

        Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/8/22 3:31 PM, Arnd Bergmann wrote:

[...]
>>> I might have missed something, but it seems the only user of IRQ 0 on
>>> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
>>> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
>>> These should have been seeing the "0 is an invalid IRQ number"
>>> warning splat since it was introduced in commit a85a6c86c25be2d2
>>> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
>>
>>     Warning or no warning, 0 is still returned. :-/
>>     My attempt to put an end to this has stuck waiting a review from the IRQ
>> people...
> 
> I had another look at this after you asked about it on IRC. I don't
> know much SH assembly, but I suspect IRQ 0 has not been delivered
> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a

   Mhm... this commit changes the SH3 code while SH778x are SH4A, no?

[...]

> 
>         Arnd

MBR, Sergey

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 4:11 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
>
> On 2/8/22 3:31 PM, Arnd Bergmann wrote:
>
> [...]
> >>> I might have missed something, but it seems the only user of IRQ 0 on
> >>> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> >>> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
> >>> These should have been seeing the "0 is an invalid IRQ number"
> >>> warning splat since it was introduced in commit a85a6c86c25be2d2
> >>> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
> >>
> >>     Warning or no warning, 0 is still returned. :-/
> >>     My attempt to put an end to this has stuck waiting a review from the IRQ
> >> people...
> >
> > I had another look at this after you asked about it on IRC. I don't
> > know much SH assembly, but I suspect IRQ 0 has not been delivered
> > since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
>
>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?

This code is shared between both:

arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
../sh3/, entry.o ex.o)

       Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/9/22 6:18 PM, Arnd Bergmann wrote:
> On Wed, Feb 9, 2022 at 4:11 PM Sergei Shtylyov
> <sergei.shtylyov@gmail.com> wrote:
>>
>> On 2/8/22 3:31 PM, Arnd Bergmann wrote:
>>
>> [...]
>>>>> I might have missed something, but it seems the only user of IRQ 0 on
>>>>> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
>>>>> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
>>>>> These should have been seeing the "0 is an invalid IRQ number"
>>>>> warning splat since it was introduced in commit a85a6c86c25be2d2
>>>>> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
>>>>
>>>>     Warning or no warning, 0 is still returned. :-/
>>>>     My attempt to put an end to this has stuck waiting a review from the IRQ
>>>> people...
>>>
>>> I had another look at this after you asked about it on IRC. I don't
>>> know much SH assembly, but I suspect IRQ 0 has not been delivered

   Neither do I, sigh...
   I do know the instuctions are 16-bit and so there are no immediate
opperands... :-)

>>> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
>>
>>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?
> 
> This code is shared between both:
> 
> arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
> ../sh3/, entry.o ex.o)

   Ah, quite convoluted! :-)
   So you mean thet broke the delivery of EVT 0x200 when mucking with NMI?

>        Arnd

MBR, Sergey

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 4:48 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 2/9/22 6:18 PM, Arnd Bergmann wrote:
> >>> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
> >>
> >>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?
> >
> > This code is shared between both:
> >
> > arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
> > ../sh3/, entry.o ex.o)
>
>    Ah, quite convoluted! :-)
>    So you mean thet broke the delivery of EVT 0x200 when mucking with NMI?

Yes, exactly: If I read this right, the added code:

+       shlr2   r4
+       shlr    r4
+       mov     r4, r0          ! save vector->jmp table offset for later
+
+       shlr2   r4              ! vector to IRQ# conversion
+       add     #-0x10, r4
+
+       cmp/pz  r4              ! is it a valid IRQ?
+       bt      10f

gets the vector (0x200 for this device), shifts it five bits to 0x10,
and subtracts 0x10,
then branches to do_IRQ if the interrupt number is non-zero, otherwise it goes
through the exception_handling_table.

         Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/9/22 7:02 PM, Arnd Bergmann wrote:

>>>>> since 2009 after 1e1030dccb10 ("sh: nmi_debug support."). On a
>>>>
>>>>    Mhm... this commit changes the SH3 code while SH778x are SH4A, no?
>>>
>>> This code is shared between both:
>>>
>>> arch/sh/kernel/cpu/sh4/Makefile:common-y        += $(addprefix
>>> ../sh3/, entry.o ex.o)
>>
>>    Ah, quite convoluted! :-)
>>    So you mean thet broke the delivery of EVT 0x200 when mucking with NMI?
> 
> Yes, exactly: If I read this right, the added code:
> 
> +       shlr2   r4
> +       shlr    r4
> +       mov     r4, r0          ! save vector->jmp table offset for later
> +
> +       shlr2   r4              ! vector to IRQ# conversion
> +       add     #-0x10, r4
> +
> +       cmp/pz  r4              ! is it a valid IRQ?
> +       bt      10f
> 
> gets the vector (0x200 for this device), shifts it five bits to 0x10,
> and subtracts 0x10,
> then branches to do_IRQ if the interrupt number is non-zero, otherwise it goes
> through the exception_handling_table.

   The SH4 manual I found on my disk (have it from MontaVista times) tells me cmp/pz
sets T if Rn is >= 0, then bt branches if T = 1. So I do think the code is correct.
   One more thing: the board code for those boards was added in 2011, we can assume
it was working back then, right? :-_

>          Arnd

MBR, Sergey

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 5:08 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 2/9/22 7:02 PM, Arnd Bergmann wrote:
> >
> > +       shlr2   r4
> > +       shlr    r4
> > +       mov     r4, r0          ! save vector->jmp table offset for later
> > +
> > +       shlr2   r4              ! vector to IRQ# conversion
> > +       add     #-0x10, r4
> > +
> > +       cmp/pz  r4              ! is it a valid IRQ?
> > +       bt      10f
> >
> > gets the vector (0x200 for this device), shifts it five bits to 0x10,
> > and subtracts 0x10,
> > then branches to do_IRQ if the interrupt number is non-zero, otherwise it goes
> > through the exception_handling_table.
>
>    The SH4 manual I found on my disk (have it from MontaVista times) tells me cmp/pz
> sets T if Rn is >= 0, then bt branches if T = 1. So I do think the code is correct.
>    One more thing: the board code for those boards was added in 2011, we can assume
> it was working back then, right? :-_

Indeed, this does make more sense, I had not realized that the numbers could get
negative here.

         Arnd

Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Sergei,

On Wed, Feb 9, 2022 at 5:08 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
>    One more thing: the board code for those boards was added in 2011, we can assume
> it was working back then, right? :-_

This assumption may not be true: there is plenty of driver/board
support that was only upstreamed partially.

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 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Geert Uytterhoeven]

Hi Sergei,

On Wed, Feb 9, 2022 at 4:48 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 2/9/22 6:18 PM, Arnd Bergmann wrote:
> >>> I had another look at this after you asked about it on IRC. I don't
> >>> know much SH assembly, but I suspect IRQ 0 has not been delivered
>
>    Neither do I, sigh...
>    I do know the instuctions are 16-bit and so there are no immediate
> opperands... :-)

There is byte immediate data (TIL).

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 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Sergei Shtylyov]

On 2/10/22 12:32 PM, Geert Uytterhoeven wrote:

[...]

>>>>> I had another look at this after you asked about it on IRC. I don't
>>>>> know much SH assembly, but I suspect IRQ 0 has not been delivered
>>
>>    Neither do I, sigh...
>>    I do know the instuctions are 16-bit and so there are no immediate
>> opperands... :-)
> 
> There is byte immediate data (TIL).

    Yeah, I figured. :-)

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergey