LinuxLists.cc - [PATCH 0/3] i2c/busses: Use platform_get_irq/

[permalink] [raw]

Subject: Re: [PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

Hi Florian,

Thank you for the review.

On Sat, Dec 18, 2021 at 9:17 PM Florian Fainelli <[email protected]> wrote:
>
>
>
> On 12/18/2021 8:52 AM, Lad Prabhakar 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.
> >
> > In preparation for removal of static setup of IRQ resource from DT core
> > code use platform_get_irq().
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Just one nit below:
> > ---
> > drivers/i2c/busses/i2c-bcm2835.c | 11 ++++-------
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> > index 37443edbf754..d63dec5f3cb1 100644
> > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > @@ -402,7 +402,7 @@ static const struct i2c_adapter_quirks bcm2835_i2c_quirks = {
> > static int bcm2835_i2c_probe(struct platform_device *pdev)
> > {
> > struct bcm2835_i2c_dev *i2c_dev;
> > - struct resource *mem, *irq;
> > + struct resource *mem;
> > int ret;
> > struct i2c_adapter *adap;
> > struct clk *mclk;
> > @@ -452,12 +452,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > - if (!irq) {
> > - dev_err(&pdev->dev, "No IRQ resource\n");
> > - return -ENODEV;
> > - }
> > - i2c_dev->irq = irq->start;
> > + i2c_dev->irq = platform_get_irq(pdev, 0);
> > + if (i2c_dev->irq <= 0)
> > + return i2c_dev->irq ? i2c_dev->irq : -ENXIO;
>
> Why not just check for a negative return code and propagate it as is?
>
platform_get_irq() may return 0 said that we do get a splat in this
case and further request_irq() will fail so instead check it here.

Cheers,
Prabhakar

2021-12-19 09:53:13

[permalink] [raw]

Subject: Re: [PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

Hi Florian,

On Sat, Dec 18, 2021 at 10:44 PM Lad, Prabhakar
<[email protected]> wrote:
>
> Hi Florian,
>
> Thank you for the review.
>
> On Sat, Dec 18, 2021 at 9:17 PM Florian Fainelli <[email protected]> wrote:
> >
> >
> >
> > On 12/18/2021 8:52 AM, Lad Prabhakar 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.
> > >
> > > In preparation for removal of static setup of IRQ resource from DT core
> > > code use platform_get_irq().
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > Just one nit below:
> > > ---
> > > drivers/i2c/busses/i2c-bcm2835.c | 11 ++++-------
> > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> > > index 37443edbf754..d63dec5f3cb1 100644
> > > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > > @@ -402,7 +402,7 @@ static const struct i2c_adapter_quirks bcm2835_i2c_quirks = {
> > > static int bcm2835_i2c_probe(struct platform_device *pdev)
> > > {
> > > struct bcm2835_i2c_dev *i2c_dev;
> > > - struct resource *mem, *irq;
> > > + struct resource *mem;
> > > int ret;
> > > struct i2c_adapter *adap;
> > > struct clk *mclk;
> > > @@ -452,12 +452,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
> > > return ret;
> > > }
> > >
> > > - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > > - if (!irq) {
> > > - dev_err(&pdev->dev, "No IRQ resource\n");
> > > - return -ENODEV;
> > > - }
> > > - i2c_dev->irq = irq->start;
> > > + i2c_dev->irq = platform_get_irq(pdev, 0);
> > > + if (i2c_dev->irq <= 0)
> > > + return i2c_dev->irq ? i2c_dev->irq : -ENXIO;
> >
> > Why not just check for a negative return code and propagate it as is?
> >
> platform_get_irq() may return 0 said that we do get a splat in this
> case and further request_irq() will fail so instead check it here.
>
My bad, just the negative check should suffice.

Cheers,
Prabhakar

2021-12-19 18:21:38

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH 1/3] i2c: bcm2835: Use platform_get_irq() to get the interrupt

On 12/19/2021 1:52 AM, Lad, Prabhakar wrote:
> Hi Florian,
>
> On Sat, Dec 18, 2021 at 10:44 PM Lad, Prabhakar
> <[email protected]> wrote:
>>
>> Hi Florian,
>>
>> Thank you for the review.
>>
>> On Sat, Dec 18, 2021 at 9:17 PM Florian Fainelli <[email protected]> wrote:
>>>
>>>
>>>
>>> On 12/18/2021 8:52 AM, Lad Prabhakar 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.
>>>>
>>>> In preparation for removal of static setup of IRQ resource from DT core
>>>> code use platform_get_irq().
>>>>
>>>> Signed-off-by: Lad Prabhakar <[email protected]>
>>>
>>> Just one nit below:
>>>> ---
>>>> drivers/i2c/busses/i2c-bcm2835.c | 11 ++++-------
>>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
>>>> index 37443edbf754..d63dec5f3cb1 100644
>>>> --- a/drivers/i2c/busses/i2c-bcm2835.c
>>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c
>>>> @@ -402,7 +402,7 @@ static const struct i2c_adapter_quirks bcm2835_i2c_quirks = {
>>>> static int bcm2835_i2c_probe(struct platform_device *pdev)
>>>> {
>>>> struct bcm2835_i2c_dev *i2c_dev;
>>>> - struct resource *mem, *irq;
>>>> + struct resource *mem;
>>>> int ret;
>>>> struct i2c_adapter *adap;
>>>> struct clk *mclk;
>>>> @@ -452,12 +452,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>>>> return ret;
>>>> }
>>>>
>>>> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>> - if (!irq) {
>>>> - dev_err(&pdev->dev, "No IRQ resource\n");
>>>> - return -ENODEV;
>>>> - }
>>>> - i2c_dev->irq = irq->start;
>>>> + i2c_dev->irq = platform_get_irq(pdev, 0);
>>>> + if (i2c_dev->irq <= 0)
>>>> + return i2c_dev->irq ? i2c_dev->irq : -ENXIO;
>>>
>>> Why not just check for a negative return code and propagate it as is?
>>>
>> platform_get_irq() may return 0 said that we do get a splat in this
>> case and further request_irq() will fail so instead check it here.
>>
> My bad, just the negative check should suffice.

Think so too, looking forward to v2, thanks!
--
Florian

2021-12-20 10:16:25

by Wolfram Sang

[permalink] [raw]

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

Hi Prabhakar,

> + if (!np) {

Very minor nit: Maybe 'if (np)' and switch the blocks? Positive logic is
a tad easier to read.

> + 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++;
> + }

Yeah, it is good to keep the legacy block as is.

> + do {
> + irq = platform_get_irq_optional(dev, k);
> + if (irq <= 0 && irq != -ENXIO)
> + return irq ? irq : -ENXIO;
> + 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);

In addition to the 'irq == 0' case from patch 1, I tried to shorten the
block for the np-case. I only came up with this. The assigntment and
comparison of the while-argument is not exactly pretty, but the block
itself is easier to read. I'll let you decide.

while (irq = platform_get_irq_optional(dev, k) != -ENXIO) {
if (irq < 0)
return irq;

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 %d\n", irq);
return ret;
}
k++;
}

Only brainstorming, not even build tested.

All the best,

Wolfram

Attachments:

(No filename) (1.67 kB)
signature.asc (833.00 B)
Download all attachments

2021-12-20 10:16:55

by Wolfram Sang

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: riic: Use platform_get_irq() to get the interrupt

> + if (ret <= 0)
> + return ret ? ret : -ENXIO;

Same comment as patch 1. Otherwise, thanks for doing this cleanup!

Attachments:

(No filename) (122.00 B)
signature.asc (833.00 B)
Download all attachments

2021-12-20 10:18:08

[permalink] [raw]

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

Hi Prabhakar,

On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
<[email protected]> 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 <[email protected]>

> --- 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 -- [email protected]

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

2021-12-20 10:20:48

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: riic: Use platform_get_irq() to get the interrupt

2021-12-20 11:53:39

[permalink] [raw]

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

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 <[email protected]>
>
>> --- 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 -- [email protected]
>
> 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

2021-12-20 11:56:03

[permalink] [raw]

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

Hi Geert,

Thank you for the review.

On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> <[email protected]> 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 <[email protected]>
>
> > --- 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 -- [email protected]
>
> 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

2021-12-20 11:59:21

[permalink] [raw]

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

Hi Wolfram,

Thank you for the review.

On Mon, Dec 20, 2021 at 10:16 AM Wolfram Sang
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> > + if (!np) {
>
> Very minor nit: Maybe 'if (np)' and switch the blocks? Positive logic is
> a tad easier to read.
>
OK will update it for v2.

> > + 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++;
> > + }
>
> Yeah, it is good to keep the legacy block as is.
>
> > + do {
> > + irq = platform_get_irq_optional(dev, k);
> > + if (irq <= 0 && irq != -ENXIO)
> > + return irq ? irq : -ENXIO;
> > + 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);
>
> In addition to the 'irq == 0' case from patch 1, I tried to shorten the
> block for the np-case. I only came up with this. The assigntment and
> comparison of the while-argument is not exactly pretty, but the block
> itself is easier to read. I'll let you decide.
>
> while (irq = platform_get_irq_optional(dev, k) != -ENXIO) {
> if (irq < 0)
> return irq;
>
> 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 %d\n", irq);
> return ret;
> }
> k++;
> }
>
> Only brainstorming, not even build tested.
>
LGTM, I'll give that a shot.

Cheers,
Prabhakar

2021-12-20 12:54:25

[permalink] [raw]

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

Hi Prabhakar,

On Mon, Dec 20, 2021 at 12:56 PM Lad, Prabhakar
<[email protected]> wrote:
> On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
> <[email protected]> wrote:
> > On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> > <[email protected]> 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 -- [email protected]

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

2021-12-20 13:01:04

[permalink] [raw]

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

Hi Geert,

On Mon, Dec 20, 2021 at 12:54 PM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Dec 20, 2021 at 12:56 PM Lad, Prabhakar
> <[email protected]> wrote:
> > On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
> > <[email protected]> wrote:
> > > On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> > > <[email protected]> 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 -- [email protected]
>
> 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

2022-02-09 11:43:02

[permalink] [raw]

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

On Mon, Dec 20, 2021 at 12:53 PM Sergei Shtylyov
<[email protected]> 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

2022-02-09 16:47:36

[permalink] [raw]

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

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

2022-02-09 17:39:17

[permalink] [raw]

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

On 2/9/22 6:18 PM, Arnd Bergmann wrote:
> On Wed, Feb 9, 2022 at 4:11 PM Sergei Shtylyov
> <[email protected]> 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

2022-02-09 18:13:59

[permalink] [raw]

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

On Wed, Feb 9, 2022 at 4:48 PM Sergei Shtylyov
<[email protected]> 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

2022-02-09 18:30:01

[permalink] [raw]

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

On Wed, Feb 9, 2022 at 4:11 PM Sergei Shtylyov
<[email protected]> 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

2022-02-09 18:38:49

[permalink] [raw]

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

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

2022-02-09 23:25:26

[permalink] [raw]

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

On Wed, Feb 9, 2022 at 5:08 PM Sergei Shtylyov
<[email protected]> 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

2022-02-10 09:49:05

[permalink] [raw]

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

Hi Sergei,

On Wed, Feb 9, 2022 at 5:08 PM Sergei Shtylyov
<[email protected]> 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 -- [email protected]

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

2022-02-10 10:56:46

[permalink] [raw]

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

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

2022-02-10 15:08:38