Re: [PATCH v1 1/1] gpio: Support interrupts in gpio-mlxbf2.c

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Asmaa,

thanks for your patch!

Please send GPIO related patches to linux-gpio@xxxxxxxxxxxxxxx!

On Tue, Feb 23, 2021 at 11:51 PM Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx> wrote:
>
> From: Asmaa Mnebhi <asmaa@xxxxxxxxxx>
>
> There are 3 possible GPIO interrupts which can be
> supported on BlueField-2 boards. Some BlueField boards support:
> 1) PHY interrupt only
> 2) PHY interrupt and Reset interrupt
> 3) Low power interrupt only

Is this a property of the GPIO block or more of a property of the
chip?

> There is one hardware line shared among all GPIOs among other
> things. So the interrupt controller checks whether the
> hardware interrupt is from a GPIO first. Then it checks which
> GPIO block it is for. And within the GPIO block, it checks
> which GPIO pin it is for.

This is normal for cascaded GPIO interrupts.

> The "reset interrupt" and "low power interrupt" trigger a
> user space program.

Then they should be doing so using drivers in the proper kernel
subsystems, such as
drivers/power/reset/gpio-poweroff.c
drivers/power/reset/gpio-restart.c

Userspace has no business trying to intercept and take control
over such low level tasks as machine reset.

> The PHY interrupt is mapped to a linux
> IRQ and passed down to a PHY driver.

Then the phy driver should obtain its IRQ just like any other IRQ
in the system, the fact that it happens to be on a GPIO line
does not matter.

> The GPIO pin responsible for these interrupts may also change
> across boards.

That's fine, the hardware description model (I guess in your case
ACPI) should take care of that.

> The ACPI table contains a property which is assigned a valid
> GPIO pin number if the interrupt is supported on a particular
> BlueField-2 board. The bootloader will change that property
> based on the board id.

OK and then the kernel uses ACPI.

We have some ACPI experts on GPIO, and you must have noticed
that we have a special ACPI layer for gpiolib.

This is what should provide IRQ resources to your other drivers so they
can look them up with a simple
struct gpio_desc *gpiod = devm_gpiod_get(dev, ...)

NOTE: you are using GPIOLIB_IRQCHIP so you need to
add

select GPIOLIB_IRQCHIP

to Kconfig for this driver.

> -// SPDX-License-Identifier: GPL-2.0
> +// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
> +
> +/*
> + *  Copyright (c) 2020-2021 NVIDIA Corporation.
> + */

Send this as a separate patch as it is only administrativa.
It's fine I think, you mostly wrote this driver.

> +#include <linux/gpio/consumer.h>

This looks weird but let's see!

> +#define DRV_VERSION "1.2"

We usually don't make this kind of stuff, skip this.
It's not like the API changes.

> +#define YU_CAUSE_GPIO_ADDR             0x2801530

Shouldn't this address come from ACPI?

> +#define YU_CAUSE_GPIO_ADDR_SIZE                0x4

Does this mean it is four bytes? That should be implied I think.

>  struct mlxbf2_gpio_context {
>         struct gpio_chip gc;
> +       struct irq_chip irq_chip;
>
>         /* YU GPIO blocks address */
>         void __iomem *gpio_io;
>
> +       /* YU cause gpio arm coalesce0 address */
> +       void __iomem *cause_gpio_arm_coalesce0_io;
> +
> +       /* YU GPIO pin responsible for low power mode */
> +       unsigned long low_pwr_pin;

I think this should be handled by a driver outside of GPIO
like those I pointed out. We need strict separation of concerns
between GPIO and other drivers.

> +       /* YU GPIO pin responsible for soft reset */
> +       unsigned long rst_pin;

Dito.

> +       /* YU GPIO pin connected to PHY INT_N signal */
> +       unsigned long phy_int_pin;

Dito. Should be handled by the PHY driver.

> +static void mlxbf2_gpio_send_work(struct work_struct *work)
> +{
> +       struct mlxbf2_gpio_context *gs;
> +
> +       gs = container_of(work, struct mlxbf2_gpio_context, send_work);
> +
> +       acpi_bus_generate_netlink_event("button/power.*", "Power Button",
> +                                       0x80, 1);
> +}

Uh oh that's interesting. I need someone from ACPI camp to look at this.

To me it seems more like something that either ACPI should handle internally
or, if that is impossible, something that could be added to
drivers/power/reset/gpio-poweroff.c

> +static bool mlxbf2_gpio_is_acpi_event(u32 gpio_block, unsigned long gpio_pin,
> +                         struct mlxbf2_gpio_context *gs)
> +{
> +       if (gpio_block & BIT(GPIO_BLOCK0)) {
> +               if (gpio_pin & BIT(gs->rst_pin))
> +                       return true;
> +       }
> +       if (gpio_block & BIT(GPIO_BLOCK16)) {
> +               if (gpio_pin & BIT(gs->low_pwr_pin))
> +                       return true;
> +       }
> +
> +       return false;
> +}
(...)
> +       if (mlxbf2_gpio_is_acpi_event(gpio_block, gpio_pin, gs))
> +               schedule_work(&gs->send_work);

So you determine that some line is an "ACPI event" using some hardware
registers? I don't know, this looks fragile.

> +static int mlxbf2_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
> +{
> +       struct mlxbf2_gpio_context *gs;
> +
> +       gs = gpiochip_get_data(chip);
> +
> +       return irq_create_mapping(gs->gc.irq.domain, gpio);
> +}

I would rather you use the built-in gpiolib irqchip IRQ domain
for this. I think you can just delete this. The core should provide it.

> +       spin_lock_init(&gs->gc.bgpio_lock);

Why? This should be initialized by the core as an effect
of bgpio_init().

> +       if (ret) {
> +               dev_err(dev, "bgpio_init failed\n");
> +               return ret;
> +       }

Maybe a separate patch adding some debug print? Not the biggest thing but...

>         gc->direction_input = mlxbf2_gpio_direction_input;
>         gc->direction_output = mlxbf2_gpio_direction_output;
>         gc->ngpio = npins;
>         gc->owner = THIS_MODULE;
> +       gc->to_irq = mlxbf2_gpio_to_irq;

Drop this.

> +       /*
> +        * PHY interrupt
> +        */
> +       ret = device_property_read_u32(dev, "phy-int-pin", &phy_int_pin);
> +       if (ret < 0)
> +               phy_int_pin = MLXBF2_GPIO_MAX_PINS_PER_BLOCK;
> +
> +       /*
> +        * OCP3.0 supports the low power mode interrupt.
> +        */
> +       ret = device_property_read_u32(dev, "low-pwr-pin", &low_pwr_pin);
> +       if (ret < 0)
> +               low_pwr_pin = MLXBF2_GPIO_MAX_PINS_PER_BLOCK;
> +
> +       /*
> +        * BlueSphere and the PRIS boards support the reset interrupt.
> +        */
> +       ret = device_property_read_u32(dev, "rst-pin", &rst_pin);
> +       if (ret < 0)
> +               rst_pin = MLXBF2_GPIO_MAX_PINS_PER_BLOCK;
> +
> +       gs->phy_int_pin = phy_int_pin;
> +       gs->low_pwr_pin = low_pwr_pin;
> +       gs->rst_pin = rst_pin;

I see what you are doing but why on earth are these resources tied to
this interrupt controller? They should be resources for something else
in my mind, however I don't know much about ACPI.

> +               irq = platform_get_irq(pdev, 0);
> +               ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
> +                                      IRQF_ONESHOT | IRQF_SHARED, name, gs);

Why is it oneshot? That is usually just useful for threaded IRQs.

> +       if (phy_int_pin != MLXBF2_GPIO_MAX_PINS_PER_BLOCK) {
> +               /* Create phy irq mapping */
> +               mlxbf2_gpio_to_irq(&gs->gc, phy_int_pin);
> +               /* Enable sharing the irq domain with the PHY driver */
> +               irq_set_default_host(gs->gc.irq.domain);
> +       }

Ugh this is messy, we need to provide the IRQs out of the driver in
a clean way.

Yours,
Linus Walleij