how to request gpiochip line which is only valid as an interrupt?

how to request gpiochip line which is only valid as an interrupt?

[Marek Behún]

Hello Bartosz,

I would like to ask you if you could find some time to look at

  [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for
                  MCU provided TRNG

  https://lore.kernel.org/soc/20240605161851.13911-7-kabel@kernel.org/

Andy Shevchenko added you to that conversation asking you about how to
correctly do the following part:

  irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));

I am writing this to give some more light into the problem. What is
going on:
- the turris-omnia-mcu driver provides a gpio chip with interrupts
- some lines are gpio + irq, but some lines are interrupt only
- later, after the gpiochip is registered, another part of the
  turris-omnia-mcu driver wants to use one interrupt only line

To use the gpiod_to_irq() function, I need gpio descriptor for that
line. I can get that with gpiochip_get_desc(), since this is within the
driver, I have access to the gpiochip. But this is semantically a
little weird, because

  1. gpiochip_get_desc() is supposed to be used by gpio driver, not
     consumer (and the trng part of the turris-omnia-mcu code is a
     consumer of the gpio)

  2. reference counting?

Looking at gpiolib, maybe the better function to use would be
gpiochip_request_own_desc(). This also is defined in
include/gpio/driver.c instead of include/gpio/consumer.c, but at least
it's name suggests that it is used by code that also owns the
gpiochip...

One problem is that gpiochip_request_own_desc() won't work, because the
gpiochip initializes valid masks for both gpios and irqs, and the 
gpiochip_request_own_desc() function calls gpiod_request_commit(),
which executes the following code

  if (guard.gc->request) {
    offset = gpio_chip_hwgpio(desc);
    if (gpiochip_line_is_valid(guard.gc, offset))
      ret = guard.gc->request(guard.gc, offset);
    else
      ret = -EINVAL;
    ...
  }

So if a gpiochip line is not valid GPIO, only valid IRQchip line, then
the GPIO cannot be requested, even for interrupts.

What is the proper solution here?

Thank you

Marek

Re: how to request gpiochip line which is only valid as an interrupt?

[Marek Behún]

On Mon, 10 Jun 2024 16:01:55 +0200
Marek Behún <kabel@kernel.org> wrote:

> Hello Bartosz,
> 
> I would like to ask you if you could find some time to look at
> 
>   [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for
>                   MCU provided TRNG
> 
>   https://lore.kernel.org/soc/20240605161851.13911-7-kabel@kernel.org/
> 
> Andy Shevchenko added you to that conversation asking you about how to
> correctly do the following part:
> 
>   irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> 
> I am writing this to give some more light into the problem. What is
> going on:
> - the turris-omnia-mcu driver provides a gpio chip with interrupts
> - some lines are gpio + irq, but some lines are interrupt only
> - later, after the gpiochip is registered, another part of the
>   turris-omnia-mcu driver wants to use one interrupt only line
> 
> To use the gpiod_to_irq() function, I need gpio descriptor for that
> line. I can get that with gpiochip_get_desc(), since this is within the
> driver, I have access to the gpiochip. But this is semantically a
> little weird, because
> 
>   1. gpiochip_get_desc() is supposed to be used by gpio driver, not
>      consumer (and the trng part of the turris-omnia-mcu code is a
>      consumer of the gpio)
> 
>   2. reference counting?
> 
> Looking at gpiolib, maybe the better function to use would be
> gpiochip_request_own_desc(). This also is defined in
> include/gpio/driver.c instead of include/gpio/consumer.c, but at least
> it's name suggests that it is used by code that also owns the
> gpiochip...
> 
> One problem is that gpiochip_request_own_desc() won't work, because the
> gpiochip initializes valid masks for both gpios and irqs, and the 
> gpiochip_request_own_desc() function calls gpiod_request_commit(),
> which executes the following code
> 
>   if (guard.gc->request) {
>     offset = gpio_chip_hwgpio(desc);
>     if (gpiochip_line_is_valid(guard.gc, offset))
>       ret = guard.gc->request(guard.gc, offset);
>     else
>       ret = -EINVAL;
>     ...
>   }
> 
> So if a gpiochip line is not valid GPIO, only valid IRQchip line, then
> the GPIO cannot be requested, even for interrupts.
> 
> What is the proper solution here?
> 
> Thank you
> 
> Marek

Bart, Andy,

it seems that if I write the mcu DT node interrupt property which
refers to self, i.e.:

  mcu: system-controller@2a {
    ...

    interrupts-extended = <&gpio1 11 IRQ_TYPE_NONE>,
                          <&mcu 13 IRQ_TYPE_NONE>;
    interrupt-names = "irq", "trng";

    ...
  };

it seems to work and I can use

  irq = fwnode_irq_get_byname(dev_fwnode(dev), "trng");

even if this is called from the mcu probe method.

Do you think this is a proper solution?

I find it a little bit weird that the mcu DT node refers to itself in
it's interrupt properties.

Marek

Re: how to request gpiochip line which is only valid as an interrupt?

[Bartosz Golaszewski]

On Tue, Jun 11, 2024 at 11:03 AM Marek Behún <kabel@kernel.org> wrote:
>
> On Mon, 10 Jun 2024 16:01:55 +0200
> Marek Behún <kabel@kernel.org> wrote:
>
> > Hello Bartosz,
> >
> > I would like to ask you if you could find some time to look at
> >
> >   [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for
> >                   MCU provided TRNG
> >
> >   https://lore.kernel.org/soc/20240605161851.13911-7-kabel@kernel.org/
> >
> > Andy Shevchenko added you to that conversation asking you about how to
> > correctly do the following part:
> >
> >   irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> >
> > I am writing this to give some more light into the problem. What is
> > going on:
> > - the turris-omnia-mcu driver provides a gpio chip with interrupts
> > - some lines are gpio + irq, but some lines are interrupt only
> > - later, after the gpiochip is registered, another part of the
> >   turris-omnia-mcu driver wants to use one interrupt only line
> >
> > To use the gpiod_to_irq() function, I need gpio descriptor for that
> > line. I can get that with gpiochip_get_desc(), since this is within the
> > driver, I have access to the gpiochip. But this is semantically a
> > little weird, because
> >
> >   1. gpiochip_get_desc() is supposed to be used by gpio driver, not
> >      consumer (and the trng part of the turris-omnia-mcu code is a
> >      consumer of the gpio)
> >
> >   2. reference counting?
> >
> > Looking at gpiolib, maybe the better function to use would be
> > gpiochip_request_own_desc(). This also is defined in
> > include/gpio/driver.c instead of include/gpio/consumer.c, but at least
> > it's name suggests that it is used by code that also owns the
> > gpiochip...
> >
> > One problem is that gpiochip_request_own_desc() won't work, because the
> > gpiochip initializes valid masks for both gpios and irqs, and the
> > gpiochip_request_own_desc() function calls gpiod_request_commit(),
> > which executes the following code
> >
> >   if (guard.gc->request) {
> >     offset = gpio_chip_hwgpio(desc);
> >     if (gpiochip_line_is_valid(guard.gc, offset))
> >       ret = guard.gc->request(guard.gc, offset);
> >     else
> >       ret = -EINVAL;
> >     ...
> >   }
> >
> > So if a gpiochip line is not valid GPIO, only valid IRQchip line, then
> > the GPIO cannot be requested, even for interrupts.
> >
> > What is the proper solution here?
> >
> > Thank you
> >
> > Marek
>
> Bart, Andy,
>
> it seems that if I write the mcu DT node interrupt property which
> refers to self, i.e.:
>
>   mcu: system-controller@2a {
>     ...
>
>     interrupts-extended = <&gpio1 11 IRQ_TYPE_NONE>,
>                           <&mcu 13 IRQ_TYPE_NONE>;
>     interrupt-names = "irq", "trng";
>
>     ...
>   };
>
> it seems to work and I can use
>
>   irq = fwnode_irq_get_byname(dev_fwnode(dev), "trng");
>
> even if this is called from the mcu probe method.
>
> Do you think this is a proper solution?
>
> I find it a little bit weird that the mcu DT node refers to itself in
> it's interrupt properties.
>
> Marek

Do I understand correctly that this is an I2C device visible under a
single address (and represented by a single device-tree node) that
registers with several kernel subsystems (among others: GPIO and RNG)?

If so then the interrupts should not be visible as a device property.
If you have access to the GPIO chip, can you simply call
gpiochip_lock_as_irq() and then request the interrupt? Users can still
read the value of this pin but won't be able to set direction to
output.

Bart

Re: how to request gpiochip line which is only valid as an interrupt?

[Marek Behún]

On Tue, 11 Jun 2024 21:22:38 +0200
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Tue, Jun 11, 2024 at 11:03 AM Marek Behún <kabel@kernel.org> wrote:
> >
> > On Mon, 10 Jun 2024 16:01:55 +0200
> > Marek Behún <kabel@kernel.org> wrote:
> >  
> > > Hello Bartosz,
> > >
> > > I would like to ask you if you could find some time to look at
> > >
> > >   [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for
> > >                   MCU provided TRNG
> > >
> > >   https://lore.kernel.org/soc/20240605161851.13911-7-kabel@kernel.org/
> > >
> > > Andy Shevchenko added you to that conversation asking you about how to
> > > correctly do the following part:
> > >
> > >   irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > >
> > > I am writing this to give some more light into the problem. What is
> > > going on:
> > > - the turris-omnia-mcu driver provides a gpio chip with interrupts
> > > - some lines are gpio + irq, but some lines are interrupt only
> > > - later, after the gpiochip is registered, another part of the
> > >   turris-omnia-mcu driver wants to use one interrupt only line
> > >
> > > To use the gpiod_to_irq() function, I need gpio descriptor for that
> > > line. I can get that with gpiochip_get_desc(), since this is within the
> > > driver, I have access to the gpiochip. But this is semantically a
> > > little weird, because
> > >
> > >   1. gpiochip_get_desc() is supposed to be used by gpio driver, not
> > >      consumer (and the trng part of the turris-omnia-mcu code is a
> > >      consumer of the gpio)
> > >
> > >   2. reference counting?
> > >
> > > Looking at gpiolib, maybe the better function to use would be
> > > gpiochip_request_own_desc(). This also is defined in
> > > include/gpio/driver.c instead of include/gpio/consumer.c, but at least
> > > it's name suggests that it is used by code that also owns the
> > > gpiochip...
> > >
> > > One problem is that gpiochip_request_own_desc() won't work, because the
> > > gpiochip initializes valid masks for both gpios and irqs, and the
> > > gpiochip_request_own_desc() function calls gpiod_request_commit(),
> > > which executes the following code
> > >
> > >   if (guard.gc->request) {
> > >     offset = gpio_chip_hwgpio(desc);
> > >     if (gpiochip_line_is_valid(guard.gc, offset))
> > >       ret = guard.gc->request(guard.gc, offset);
> > >     else
> > >       ret = -EINVAL;
> > >     ...
> > >   }
> > >
> > > So if a gpiochip line is not valid GPIO, only valid IRQchip line, then
> > > the GPIO cannot be requested, even for interrupts.
> > >
> > > What is the proper solution here?
> > >
> > > Thank you
> > >
> > > Marek  
> >
> > Bart, Andy,
> >
> > it seems that if I write the mcu DT node interrupt property which
> > refers to self, i.e.:
> >
> >   mcu: system-controller@2a {
> >     ...
> >
> >     interrupts-extended = <&gpio1 11 IRQ_TYPE_NONE>,
> >                           <&mcu 13 IRQ_TYPE_NONE>;
> >     interrupt-names = "irq", "trng";
> >
> >     ...
> >   };
> >
> > it seems to work and I can use
> >
> >   irq = fwnode_irq_get_byname(dev_fwnode(dev), "trng");
> >
> > even if this is called from the mcu probe method.
> >
> > Do you think this is a proper solution?
> >
> > I find it a little bit weird that the mcu DT node refers to itself in
> > it's interrupt properties.
> >
> > Marek  
> 
> Do I understand correctly that this is an I2C device visible under a
> single address (and represented by a single device-tree node) that
> registers with several kernel subsystems (among others: GPIO and RNG)?

Indeed. Signle device-tree node, single I2C device at one address,
signle driver, several kernel subsystems. The gpiochip is registered as
first one, it provides interrupts, the subsequent things can use the
interrupts.

> If so then the interrupts should not be visible as a device property.

And that is how I have been doing this. But the question is how should
I request for the gpio descriptor? If the own interrupts are not
described device-tree property, I can't use fwnode_irq_get() /
of_irq_get().

Originally, I used the low-level irq_create_mapping(), passing it the
gpiochip's IRQ domain, something like:

  irq =  irq_create_mapping(mcu->gc.irq.domain, TRNG_HWIRQ);

Andy said [1] that

  This looks like some workaround against existing gpiod_to_irq(). Why
  do you need this?

I should not poke into gpiolib's internals like that.

So I changed it to

  irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, TRNG_HWIRQ));

But now Andy rightly says:

  Okay, it's a bit more complicated than that. The gpiochip_get_desc()
  shouldn't be used. Bart, what can you suggest to do here? Opencoding
  it doesn't sound to me a (fully) correct approach in a long term.

The gpiochip_get_desc() function doesn't request the GPIO, it doesn't
change it's flags nor anything.

There is the gpiochip_request_own_desc() function, which does
requesting, but the problem there is that this GPIO line cannot be
requested, because it is not a valid GPIO line, only a valid IRQ line
(according to relevant valid_masks).

> If you have access to the GPIO chip, can you simply call
> gpiochip_lock_as_irq() and then request the interrupt?

I don't quite understand. The gpiochip's irqchip is immutable and uses
the GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition.

This means that the .irq_request_resources method is set to
gpiochip_irq_reqres from gpiolib.c, which already calls
gpiochip_lock_as_irq().

So gpiochip_lock_as_irq() is called once I request the irq with
request_threaded_irq().

> Users can still read the value of this pin but won't be able to set
> direction to output.

Users are not supposed to read value of this pin, because it is not a
GPIO pin. The corresponding bit is not set in gpiochip.valid_mask.
It is for example impossible to export it in /sys/class/gpio.

This line is valid only as an IRQ (the
corresponding bit is set in gpiochip.irq.valid_mask).

Marek

Re: how to request gpiochip line which is only valid as an interrupt?

[Marek Behún]

On Wed, 12 Jun 2024 10:55:55 +0200
Marek Behún <kabel@kernel.org> wrote:

> > Users can still read the value of this pin but won't be able to set
> > direction to output.  
> 
> Users are not supposed to read value of this pin, because it is not a
> GPIO pin. The corresponding bit is not set in gpiochip.valid_mask.
> It is for example impossible to export it in /sys/class/gpio.
> 
> This line is valid only as an IRQ (the
> corresponding bit is set in gpiochip.irq.valid_mask).

I am starting to thing that this might be the problem, that the line is
not valid as GPIO, only as an IRQ. gpiolib seems to be unable to handle
that.

Indeed, the definition of the function
  gpiochip_irqchip_irq_valid()
first checks if the line is valid as gpio:

  static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gc,
                                         unsigned int offset)
  {
          if (!gpiochip_line_is_valid(gc, offset))
                  return false;
          ...

Re: how to request gpiochip line which is only valid as an interrupt?

[Bartosz Golaszewski]

On Wed, Jun 12, 2024 at 11:03 AM Marek Behún <kabel@kernel.org> wrote:
>
> On Wed, 12 Jun 2024 10:55:55 +0200
> Marek Behún <kabel@kernel.org> wrote:
>
> > > Users can still read the value of this pin but won't be able to set
> > > direction to output.
> >
> > Users are not supposed to read value of this pin, because it is not a
> > GPIO pin. The corresponding bit is not set in gpiochip.valid_mask.
> > It is for example impossible to export it in /sys/class/gpio.
> >
> > This line is valid only as an IRQ (the
> > corresponding bit is set in gpiochip.irq.valid_mask).
>

Ah, I missed the bit about these pins not being marked as valid.

> I am starting to thing that this might be the problem, that the line is
> not valid as GPIO, only as an IRQ. gpiolib seems to be unable to handle
> that.
>
> Indeed, the definition of the function
>   gpiochip_irqchip_irq_valid()
> first checks if the line is valid as gpio:
>
>   static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gc,
>                                          unsigned int offset)
>   {
>           if (!gpiochip_line_is_valid(gc, offset))
>                   return false;
>           ...
>

TBH Maybe using the GPIO provider APIs for something that isn't a GPIO
doesn't make much sense? They are called GPIO irqchips for a reason
after all? How about handling the non-GPIO interrupts with regular
irqchip code and only exposing actual GPIOs using gpiolib?

What is the exact layout of these pins? Are the GPIOs and non-GPIOs
somehow grouped together? Maybe export several separate GPIO banks?

Bart

Re: how to request gpiochip line which is only valid as an interrupt?

[Marek Behún]

On Wed, 12 Jun 2024 14:30:58 +0200
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Wed, Jun 12, 2024 at 11:03 AM Marek Behún <kabel@kernel.org> wrote:
> >
> > On Wed, 12 Jun 2024 10:55:55 +0200
> > Marek Behún <kabel@kernel.org> wrote:
> >  
> > > > Users can still read the value of this pin but won't be able to set
> > > > direction to output.  
> > >
> > > Users are not supposed to read value of this pin, because it is not a
> > > GPIO pin. The corresponding bit is not set in gpiochip.valid_mask.
> > > It is for example impossible to export it in /sys/class/gpio.
> > >
> > > This line is valid only as an IRQ (the
> > > corresponding bit is set in gpiochip.irq.valid_mask).  
> >  
> 
> Ah, I missed the bit about these pins not being marked as valid.
> 
> > I am starting to thing that this might be the problem, that the line is
> > not valid as GPIO, only as an IRQ. gpiolib seems to be unable to handle
> > that.
> >
> > Indeed, the definition of the function
> >   gpiochip_irqchip_irq_valid()
> > first checks if the line is valid as gpio:
> >
> >   static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gc,
> >                                          unsigned int offset)
> >   {
> >           if (!gpiochip_line_is_valid(gc, offset))
> >                   return false;
> >           ...
> >  
> 
> TBH Maybe using the GPIO provider APIs for something that isn't a GPIO
> doesn't make much sense? They are called GPIO irqchips for a reason
> after all? How about handling the non-GPIO interrupts with regular
> irqchip code and only exposing actual GPIOs using gpiolib?
> 
> What is the exact layout of these pins? Are the GPIOs and non-GPIOs
> somehow grouped together? Maybe export several separate GPIO banks?

It is a little bit more complicated.

The MCU has some pins configured in GPIO mode, and the purpose of this
driver is to expose them to the operating system.
Their direction cannot be changed (with one exception).

Because of different MCU firmware versions, different GPIO groups are
readable/writable via different MCU commands. The driver exposes the
GPIOs via 3 banks:
 bank  cmd width  read command             write command
  1st  16 bits    GET_STATUS_WORD          GENERAL_CONTROL
  2nd  32 bits    GET_EXT_STATUS_DWORD     n/a
  3rd  16 bits    GET_EXT_CONTROL_STATUS   EXT_CONTROL

So for example the 1st bank has 16 lines, because the I2C command is 16
bits wide. But of these 16 bits, not all bits correspond to a real GPIO.
But the binding was created as if the bank has 16 lines to make things
simpler, and they can potentially been filled with firmware upgrade.
(More importantly we want to be compaitble with real U-Boot binaries
which already use this binding.).

Now, interrupt support was added to the MCU firmware later, and reading
interrupt status is done by a different MCU command (GET_INT_AND_CLEAR).
This command supports 32 interrupts, of which some map to the GPIOs,
but the mapping is not 1 to 1, since there is only one interrupt
command (32 bits wide) and three GPIO commands (64 bits wide in total).

So the driver exposes a gpiochip with 64 lines (of which not all are
valid), and to make it simple, it maps the GPIO lines that support
interrupts to the corresponding interrupt bit in the MCU interrupt
command. This way it is possible to use the gpiolib's internal irqchip.

Example:

  GPIO bank/line  IRQ line  Description
          0/   4         4  MiniPCIe0 Card Detect
          0/   5         5  MiniPCIe0 mSATA Indicator
          0/  12        12  Front Button

But the MCU also provides three interrupts that do not map to a real
GPIO, they are semantically different. For example the trng interrupt
is fired when MCU has generated entropy, to inform the system that it
can collect it.

I have mapped these 3 interrupts onto bits 11, 13 and 14 of bank 0,
because bank 0 corresponds to the GET_STATUS_WORD command, and these
bits of the GET_STATUS_WORD command reply can never correspond to a
GPIO (they have different meaning).

I do realize that this is rather complicated, but it is properly
documented in the device-tree binding, and is already being used this
way in the wild...

Marek

Re: how to request gpiochip line which is only valid as an interrupt?

[Marek Behún]

Dear Andy,

On Wed, 12 Jun 2024 10:55:55 +0200
Marek Behún <kabel@kernel.org> wrote:

> Originally, I used the low-level irq_create_mapping(), passing it the
> gpiochip's IRQ domain, something like:
> 
>   irq =  irq_create_mapping(mcu->gc.irq.domain, TRNG_HWIRQ);
> 
> Andy said [1] that
> 
>   This looks like some workaround against existing gpiod_to_irq(). Why
>   do you need this?
> 
> I should not poke into gpiolib's internals like that.
> 
> So I changed it to
> 
>   irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, TRNG_HWIRQ));

I think the original code 

  irq = irq_create_mapping(mcu->gc.irq.domain, TRNG_HWIRQ);

should be used. The trng part of the driver is interested in the
interrupt, it does not care if the interrupt is provided via a GPIO
chip.

The mcu device is a gpio-controller, but also an interrupt-controller.
That the interrupts are provided via gpiolib is irrelevant.

So I think that what we did back in April, changing to gpiod_to_irq(),
is not actually correct, from semantic point of view.

For example if some other driver wanted to use a MCU interrupt, it
would have simply used:
  of_get_irq()
not referring to gpio descriptors at all, i.e. not something like
  gpiod_to_irq(gpiod_find_and_request())

(I do actually have code prepared for another driver that will use
an interrupt provided by the MCU.)

What do you think?

Marek