[v5,2/3] gpio: loongson: add gpio driver support

  The Loongson platforms GPIO controller contains 60 GPIO pins in total,
4 of which are dedicated GPIO pins, and the remaining 56 are reused
with other functions. Each GPIO can set input/output and has the
interrupt capability.

This driver added support for Loongson GPIO controller and support to
use DTS or ACPI to descibe GPIO device resources.

Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v5:
		1. Move legacy gpio driver to proper location.
		2. Remove the "gpio_base".
		3. Add of_device_id and acpi_device_id data field for platform
		   data. 
		4. Remove the ACPI_PTR().
		5. Remove the gpio label judgement logic and use mode instead.  
		6. Drop platform_loongson_gpio_get_props.
		7. Using devres for all resource. 
		8. Remove the loongson_gpio_remove.
		9. Remove the unmatched print information.
		10. Remove the loongson_gpio_exit.
Change in v4:
		1. Fixup name spelling about Signed-off-by. 
		2. Drop "series" here and everywhere else.
		3. Fixup the copyright in driver.
		4. Drop the "else" in loongson_gpio_request.
		5. Use trinocular operation replace the related logic.
		6. Remove lable judgement in context about "lgpio->chip.to_irq = 
		   loongson_gpio_to_irq"
		7. Use dev_err replace pr_err in probe.
		8. Make legacy platform_data should be left out of this patch.
		9. Remove the mips config in gpio Kconfig.
Change in v3:
		1. Move the gpio platform data struct from arch/ into include/linux/
		   platform_data/.
		2. Replace platform_gpio_data with loongson_gpio_platform_data in .c.
		3. Add maintainer in MAINTAINERS file for include/linux/platform_data/
		   gpio-loongson.h and gpio-loongson.c
Change in v2:
		1. Fixup of_loongson_gpio_get_props and remove the parse logic about
	           "loongson,conf_offset", "loongson,out_offset", "loongson,in_offset",
		   "loongson,gpio_base", "loongson,support_irq" then kernel driver will
		   initial them that depend compatible except "loongson,gpio_base".

 MAINTAINERS                  |   7 +
 drivers/gpio/Kconfig         |  10 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-loongson.c | 306 +++++++++++++++++++++++++++++++++++
 4 files changed, 324 insertions(+)
 create mode 100644 drivers/gpio/gpio-loongson.c
  

On Mon, Nov 21, 2022 at 1:38 PM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:

> The Loongson platforms GPIO controller contains 60 GPIO pins in total,
> 4 of which are dedicated GPIO pins, and the remaining 56 are reused
> with other functions. Each GPIO can set input/output and has the
> interrupt capability.
>
> This driver added support for Loongson GPIO controller and support to
> use DTS or ACPI to descibe GPIO device resources.
>
> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> Change in v5:

This is starting to look really good! We are getting to the final polish.

> +config GPIO_LOONGSON
> +       tristate "Loongson GPIO support"
> +       depends on LOONGARCH || COMPILE_TEST

select GPIO_GENERIC

You should use this in the "bit mode".

>  obj-$(CONFIG_GPIO_LOONGSON1)           += gpio-loongson1.o
> +obj-$(CONFIG_GPIO_LOONGSON)            += gpio-loongson.o

Isn't this a bit confusing? What about naming it
gpio-loongson2.c?

> +enum loongson_gpio_mode {
> +       BIT_CTRL_MODE,
> +       BYTE_CTRL_MODE,
> +};

I don't think you will need to track this, jus assume BYTE_CTRL_MODE
in your callbacks because we will replace the bit mode with assigned
accessors from GPIO_GENERIC.

> +
> +struct loongson_gpio_platform_data {
> +       const char              *label;
> +       enum loongson_gpio_mode mode;

So drop this.

> +static int loongson_gpio_request(
> +                       struct gpio_chip *chip, unsigned int pin)
> +{
> +       if (pin >= chip->ngpio)
> +               return -EINVAL;

This is not needed, the gpiolib core already checks this. Drop it.

> +static inline void __set_direction(struct loongson_gpio_chip *lgpio,
> +                       unsigned int pin, int input)
> +{
> +       u64 qval;
> +       u8  bval;
> +
> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
> +               qval = readq(LOONGSON_GPIO_OEN(lgpio));
> +               if (input)
> +                       qval |= 1ULL << pin;
> +               else
> +                       qval &= ~(1ULL << pin);
> +               writeq(qval, LOONGSON_GPIO_OEN(lgpio));
> +       } else {
> +               bval = input ? 1 : 0;
> +               writeb(bval, LOONGSON_GPIO_OEN_BYTE(lgpio, pin));
> +       }

Drop bit mode keep only byte mode.

> +static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
> +                       int high)
> +{
> +       u64 qval;
> +       u8 bval;
> +
> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
> +               qval = readq(LOONGSON_GPIO_OUT(lgpio));
> +               if (high)
> +                       qval |= 1ULL << pin;
> +               else
> +                       qval &= ~(1ULL << pin);
> +               writeq(qval, LOONGSON_GPIO_OUT(lgpio));
> +       } else {
> +               bval = high ? 1 : 0;
> +               writeb(bval, LOONGSON_GPIO_OUT_BYTE(lgpio, pin));
> +       }

Dito.

> +static int loongson_gpio_get(struct gpio_chip *chip, unsigned int pin)
> +{
> +       u64 qval;
> +       u8  bval;
> +       int val;
> +
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +
> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
> +               qval = readq(LOONGSON_GPIO_IN(lgpio));
> +               val = (qval & (1ULL << pin)) != 0;
> +       } else {
> +               bval = readb(LOONGSON_GPIO_IN_BYTE(lgpio, pin));
> +               val = bval & 1;
> +       }

Dito.

> +static int loongson_gpio_to_irq(
> +                       struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct platform_device *pdev =
> +               container_of(chip->parent, struct platform_device, dev);
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +
> +       if (offset >= chip->ngpio)
> +               return -EINVAL;
> +
> +       if ((lgpio->gsi_idx_map != NULL) && (offset < lgpio->mapsize))
> +               offset = lgpio->gsi_idx_map[offset];
> +       else
> +               return -EINVAL;
> +
> +       return platform_get_irq(pdev, offset);
> +}

I'm a bit suspicious about this. See the following in
Documentation/driver-api/gpio/driver.rst:

------------------
It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.

gpiod_to_irq() is just a convenience function to figure out the IRQ for a
certain GPIO line and should not be relied upon to have been called before
the IRQ is used.

Always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having
been called first.
------------------

I am bit suspicious that your IRQchip implementation expects consumers
to call gpiod_to_irq() first and this is not legal.

> +static int loongson_gpio_init(
> +                       struct device *dev, struct loongson_gpio_chip *lgpio,
> +                       struct device_node *np, void __iomem *base)
> +{

Do something like this:

#define LOONGSON_GPIO_IN(x)            (x->base + x->p_data->in_offset)
+#define LOONGSON_GPIO_OUT(x)           (x->base + x->p_data->out_offset)
+#define LOONGSON_GPIO_OEN(x)           (x->base + x->p_data->conf_offset)

if (lgpio->p_data->mode == BIT_CTRL_MODE) {
       ret = bgpio_init(&g->gc, dev, 8,
                         lgpio->base + lgpio->p_data->in_offset,
                         lgpio->base + lgpio->p_data->out_offset,
                         0,
                         lgpio->base + lgpio->p_data->conf_offset,
                         NULL,
                         0);
        if (ret) {
                dev_err(dev, "unable to init generic GPIO\n");
                goto dis_clk;
        }

If you actually have a special purpose clear register in your hardware
which is not included here, then add it in the line with just 0 for that
function.

See the kerneldoc in bgpio_init() in drivers/gpio/gpio-mmio.c.

Then:

}  else {

> +       lgpio->chip.request = loongson_gpio_request;
> +       lgpio->chip.direction_input = loongson_gpio_direction_input;
> +       lgpio->chip.get = loongson_gpio_get;
> +       lgpio->chip.direction_output = loongson_gpio_direction_output;
> +       lgpio->chip.set = loongson_gpio_set;

Note also: implement loongson_gpio_get_direction(). To read the setting
of the conf register on startup. You now only need to implement it for
byte mode.

}

After this you should set ngpios, because bgpio_init() will overwrite it
with 64, so it cannot be done directly when parsing platform data,
cache it somewhere and write it here.

(...)

Yours,
Linus Walleij
  

在 2022/11/21 下午9:24, Linus Walleij 写道:
> On Mon, Nov 21, 2022 at 1:38 PM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 
>> The Loongson platforms GPIO controller contains 60 GPIO pins in total,
>> 4 of which are dedicated GPIO pins, and the remaining 56 are reused
>> with other functions. Each GPIO can set input/output and has the
>> interrupt capability.
>>
>> This driver added support for Loongson GPIO controller and support to
>> use DTS or ACPI to descibe GPIO device resources.
>>
>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>> Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
>> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
>> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>> Change in v5:
> 
> This is starting to look really good! We are getting to the final polish.
> 
>> +config GPIO_LOONGSON
>> +       tristate "Loongson GPIO support"
>> +       depends on LOONGARCH || COMPILE_TEST
> 
> select GPIO_GENERIC
> 
> You should use this in the "bit mode".
> 
>>   obj-$(CONFIG_GPIO_LOONGSON1)           += gpio-loongson1.o
>> +obj-$(CONFIG_GPIO_LOONGSON)            += gpio-loongson.o
> 
> Isn't this a bit confusing? What about naming it
> gpio-loongson2.c?
This driver was plan to cover all loongson platform, not only Loongson-2
SoC but also other loongson platform. and I adop your advice in another
mail. I named this driver was gpio-loongson-64bit.c.
> 
>> +enum loongson_gpio_mode {
>> +       BIT_CTRL_MODE,
>> +       BYTE_CTRL_MODE,
>> +};
> 
> I don't think you will need to track this, jus assume BYTE_CTRL_MODE
> in your callbacks because we will replace the bit mode with assigned
> accessors from GPIO_GENERIC.
  yes, but in loongson_gpio_init need a distinction, so I kept this.
> 
>> +
>> +struct loongson_gpio_platform_data {
>> +       const char              *label;
>> +       enum loongson_gpio_mode mode;
> 
> So drop this.

> 
>> +static int loongson_gpio_request(
>> +                       struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       if (pin >= chip->ngpio)
>> +               return -EINVAL;
> 
> This is not needed, the gpiolib core already checks this. Drop it.
I check gpio_request in gpilib, I notice gpio_is_valid is not equal to
this condition, so I still kept it for byte mode.
> 
>> +static inline void __set_direction(struct loongson_gpio_chip *lgpio,
>> +                       unsigned int pin, int input)
>> +{
>> +       u64 qval;
>> +       u8  bval;
>> +
>> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
>> +               qval = readq(LOONGSON_GPIO_OEN(lgpio));
>> +               if (input)
>> +                       qval |= 1ULL << pin;
>> +               else
>> +                       qval &= ~(1ULL << pin);
>> +               writeq(qval, LOONGSON_GPIO_OEN(lgpio));
>> +       } else {
>> +               bval = input ? 1 : 0;
>> +               writeb(bval, LOONGSON_GPIO_OEN_BYTE(lgpio, pin));
>> +       }
> 
> Drop bit mode keep only byte mode.
okay, I will drop it.
> 
>> +static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
>> +                       int high)
>> +{
>> +       u64 qval;
>> +       u8 bval;
>> +
>> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
>> +               qval = readq(LOONGSON_GPIO_OUT(lgpio));
>> +               if (high)
>> +                       qval |= 1ULL << pin;
>> +               else
>> +                       qval &= ~(1ULL << pin);
>> +               writeq(qval, LOONGSON_GPIO_OUT(lgpio));
>> +       } else {
>> +               bval = high ? 1 : 0;
>> +               writeb(bval, LOONGSON_GPIO_OUT_BYTE(lgpio, pin));
>> +       }
> 
> Dito.
okay, I will drop bit mode.
> 
>> +static int loongson_gpio_get(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       u64 qval;
>> +       u8  bval;
>> +       int val;
>> +
>> +       struct loongson_gpio_chip *lgpio =
>> +               container_of(chip, struct loongson_gpio_chip, chip);
>> +
>> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
>> +               qval = readq(LOONGSON_GPIO_IN(lgpio));
>> +               val = (qval & (1ULL << pin)) != 0;
>> +       } else {
>> +               bval = readb(LOONGSON_GPIO_IN_BYTE(lgpio, pin));
>> +               val = bval & 1;
>> +       }
> 
> Dito.
okay, I will drop bit mode.
> 
>> +static int loongson_gpio_to_irq(
>> +                       struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct platform_device *pdev =
>> +               container_of(chip->parent, struct platform_device, dev);
>> +       struct loongson_gpio_chip *lgpio =
>> +               container_of(chip, struct loongson_gpio_chip, chip);
>> +
>> +       if (offset >= chip->ngpio)
>> +               return -EINVAL;
>> +
>> +       if ((lgpio->gsi_idx_map != NULL) && (offset < lgpio->mapsize))
>> +               offset = lgpio->gsi_idx_map[offset];
>> +       else
>> +               return -EINVAL;
>> +
>> +       return platform_get_irq(pdev, offset);
>> +}
> 
> I'm a bit suspicious about this. See the following in
> Documentation/driver-api/gpio/driver.rst:
> 
> ------------------
> It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
> is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
> irq_chip are orthogonal, and offering their services independent of each
> other.
> 
> gpiod_to_irq() is just a convenience function to figure out the IRQ for a
> certain GPIO line and should not be relied upon to have been called before
> the IRQ is used.
> 
> Always prepare the hardware and make it ready for action in respective
> callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having
> been called first.
> ------------------
> 
> I am bit suspicious that your IRQchip implementation expects consumers
> to call gpiod_to_irq() first and this is not legal.
okay, I got it, and other driver use gpio interrupt doesn't rely on 
gpiod_to_irq, but can use gpiod_to_irq.

The reason is that gpio interrupt wasn't an independent module,  The
loongson interrupt controller liointc include lots of interrupt was
route to perpherial, such as i2c/spi .. gpio, so gpio interrupt as
normal perpherial interrupt, It is unnecessary and redundant to
implement a gpio irq chip. The liointc controller driver had cover all
interrupt.
> 
>> +static int loongson_gpio_init(
>> +                       struct device *dev, struct loongson_gpio_chip *lgpio,
>> +                       struct device_node *np, void __iomem *base)
>> +{
> 
> Do something like this:
> 
> #define LOONGSON_GPIO_IN(x)            (x->base + x->p_data->in_offset)
> +#define LOONGSON_GPIO_OUT(x)           (x->base + x->p_data->out_offset)
> +#define LOONGSON_GPIO_OEN(x)           (x->base + x->p_data->conf_offset)
> 
> if (lgpio->p_data->mode == BIT_CTRL_MODE) {
>         ret = bgpio_init(&g->gc, dev, 8,
>                           lgpio->base + lgpio->p_data->in_offset,
>                           lgpio->base + lgpio->p_data->out_offset,
>                           0,
>                           lgpio->base + lgpio->p_data->conf_offset,
>                           NULL,
>                           0);
>          if (ret) {
>                  dev_err(dev, "unable to init generic GPIO\n");
>                  goto dis_clk;
>          }
> 
> If you actually have a special purpose clear register in your hardware
> which is not included here, then add it in the line with just 0 for that
> function.
okay, I had do it.
in addition,  I don't like to use the dynamically allocated gpio base, 
so I set the gpio base after call bgpio_init.

> 
> See the kerneldoc in bgpio_init() in drivers/gpio/gpio-mmio.c.
> 
> Then:
> 
> }  else {
> 
>> +       lgpio->chip.request = loongson_gpio_request;
>> +       lgpio->chip.direction_input = loongson_gpio_direction_input;
>> +       lgpio->chip.get = loongson_gpio_get;
>> +       lgpio->chip.direction_output = loongson_gpio_direction_output;
>> +       lgpio->chip.set = loongson_gpio_set;
> 
> Note also: implement loongson_gpio_get_direction(). To read the setting
> of the conf register on startup. You now only need to implement it for
> byte mode.
okay, I had implement it.
> 
> }
> 
> After this you should set ngpios, because bgpio_init() will overwrite it
> with 64, so it cannot be done directly when parsing platform data,
> cache it somewhere and write it here.
okay, I will do it.
> 
> (...)
> 
> Yours,
> Linus Walleij
>
  

On Wed, Nov 23, 2022 at 9:02 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 在 2022/11/21 下午9:24, Linus Walleij 写道:

> >> +static int loongson_gpio_request(
> >> +                       struct gpio_chip *chip, unsigned int pin)
> >> +{
> >> +       if (pin >= chip->ngpio)
> >> +               return -EINVAL;
> >
> > This is not needed, the gpiolib core already checks this. Drop it.
> I check gpio_request in gpilib, I notice gpio_is_valid is not equal to
> this condition, so I still kept it for byte mode.

This is because descriptors can only be obtained from gpiod_get() and
similar and gpiod_get() falls to gpiod_get_index() which will not
return a valid descriptor from either HW backend. gpiod_get()
will call gpiod_request() for if and only if the descriptor is valid.

The only reason to implement something like this is because of
using the legacy GPIO numberspace which we are getting rid
of so it is irrelevant, the consumers of your driver will only be
using gpio descriptors, will only come in through gpiod_get_index()
and will have desc validity check done before calling gpiod_request().

So drop this.

> > I am bit suspicious that your IRQchip implementation expects consumers
> > to call gpiod_to_irq() first and this is not legal.
>
> okay, I got it, and other driver use gpio interrupt doesn't rely on
> gpiod_to_irq, but can use gpiod_to_irq.

Yes it can be used to look up the irq corresponding to a GPIO
but it is not mandatory to do that.

> The reason is that gpio interrupt wasn't an independent module,  The
> loongson interrupt controller liointc include lots of interrupt was
> route to perpherial, such as i2c/spi .. gpio, so gpio interrupt as
> normal perpherial interrupt, It is unnecessary and redundant to
> implement a gpio irq chip. The liointc controller driver had cover all
> interrupt.

This is fine, and it is common for GPIO drivers to implement
their own IRQchips.

But these drivers can not rely on the .gpio_to_irq() callback
to be called before an IRQ is requested and used.

> in addition,  I don't like to use the dynamically allocated gpio base,
> so I set the gpio base after call bgpio_init.

Don't do that because the GPIO maintainers love the
dynamic base and hate hardcoded bases. Set the base to -1
If you wonder why, read drivers/gpio/TODO.

Yours,
Linus Walleij
  

在 2022/11/24 上午6:05, Linus Walleij 写道:
> On Wed, Nov 23, 2022 at 9:02 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>> 在 2022/11/21 下午9:24, Linus Walleij 写道:
> 
>>>> +static int loongson_gpio_request(
>>>> +                       struct gpio_chip *chip, unsigned int pin)
>>>> +{
>>>> +       if (pin >= chip->ngpio)
>>>> +               return -EINVAL;
>>>
>>> This is not needed, the gpiolib core already checks this. Drop it.
>> I check gpio_request in gpilib, I notice gpio_is_valid is not equal to
>> this condition, so I still kept it for byte mode.
> 
> This is because descriptors can only be obtained from gpiod_get() and
> similar and gpiod_get() falls to gpiod_get_index() which will not
> return a valid descriptor from either HW backend. gpiod_get()
> will call gpiod_request() for if and only if the descriptor is valid.
> 
> The only reason to implement something like this is because of
> using the legacy GPIO numberspace which we are getting rid
> of so it is irrelevant, the consumers of your driver will only be
> using gpio descriptors, will only come in through gpiod_get_index()
> and will have desc validity check done before calling gpiod_request().
> 
> So drop this.
okay , I got it.
Thank you for your detailed explanation.
> 
>>> I am bit suspicious that your IRQchip implementation expects consumers
>>> to call gpiod_to_irq() first and this is not legal.
>>
>> okay, I got it, and other driver use gpio interrupt doesn't rely on
>> gpiod_to_irq, but can use gpiod_to_irq.
> 
> Yes it can be used to look up the irq corresponding to a GPIO
> but it is not mandatory to do that.
> 
>> The reason is that gpio interrupt wasn't an independent module,  The
>> loongson interrupt controller liointc include lots of interrupt was
>> route to perpherial, such as i2c/spi .. gpio, so gpio interrupt as
>> normal perpherial interrupt, It is unnecessary and redundant to
>> implement a gpio irq chip. The liointc controller driver had cover all
>> interrupt.
> 
> This is fine, and it is common for GPIO drivers to implement
> their own IRQchips.
> 
> But these drivers can not rely on the .gpio_to_irq() callback
> to be called before an IRQ is requested and used.

I may not have made it clear before that the gpio irq chip for other
platforms may need to be implemented, but the loongson platform may be
special.

I mean that the loongson platform use gpio irq does not need to rely on
gpio_to_irq, because loongson interrupt controller driver has covered
gpio irq.  The specific reason is my above explanation.

so, Can I not realize gpio irq chip?
> 
>> in addition,  I don't like to use the dynamically allocated gpio base,
>> so I set the gpio base after call bgpio_init.
> 
> Don't do that because the GPIO maintainers love the
> dynamic base and hate hardcoded bases. Set the base to -1
> If you wonder why, read drivers/gpio/TODO.
okay, I got it.
> 
> Yours,
> Linus Walleij
>
  

On Thu, Nov 24, 2022 at 3:22 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 在 2022/11/24 上午6:05, Linus Walleij 写道:

> > But these drivers can not rely on the .gpio_to_irq() callback
> > to be called before an IRQ is requested and used.
>
> I may not have made it clear before that the gpio irq chip for other
> platforms may need to be implemented, but the loongson platform may be
> special.
>
> I mean that the loongson platform use gpio irq does not need to rely on
> gpio_to_irq, because loongson interrupt controller driver has covered
> gpio irq.  The specific reason is my above explanation.
>
> so, Can I not realize gpio irq chip?

Isn't this a hierarchical irqchip then?

Please consult the following from
Documentation/driver-api/gpio/driver.rst:

---------------------------------

GPIO drivers providing IRQs
===========================

It is custom that GPIO drivers (GPIO chips) are also providing interrupts,
most often cascaded off a parent interrupt controller, and in some special
cases the GPIO logic is melded with a SoC's primary interrupt controller.

The IRQ portions of the GPIO block are implemented using an irq_chip, using
the header <linux/irq.h>. So this combined driver is utilizing two sub-
systems simultaneously: gpio and irq.

It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.

gpiod_to_irq() is just a convenience function to figure out the IRQ for a
certain GPIO line and should not be relied upon to have been called before
the IRQ is used.

Always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having
been called first.

We can divide GPIO irqchips in two broad categories:

- CASCADED INTERRUPT CHIPS: this means that the GPIO chip has one common
  interrupt output line, which is triggered by any enabled GPIO line on that
  chip. The interrupt output line will then be routed to an parent interrupt
  controller one level up, in the most simple case the systems primary
  interrupt controller. This is modeled by an irqchip that will inspect bits
  inside the GPIO controller to figure out which line fired it. The irqchip
  part of the driver needs to inspect registers to figure this out and it
  will likely also need to acknowledge that it is handling the interrupt
  by clearing some bit (sometime implicitly, by just reading a status
  register) and it will often need to set up the configuration such as
  edge sensitivity (rising or falling edge, or high/low level interrupt for
  example).

- HIERARCHICAL INTERRUPT CHIPS: this means that each GPIO line has a dedicated
  irq line to a parent interrupt controller one level up. There is no need
  to inquire the GPIO hardware to figure out which line has fired, but it
  may still be necessary to acknowledge the interrupt and set up configuration
  such as edge sensitivity.

---------------------------------

You find an example of a hierarchical GPIO irqchip using the
GPIOLIB_IRQCHIP in drivers/gpio/gpio-ixp4xx.c.

Yours,
Linus Walleij
  

在 2022/11/24 下午4:54, Linus Walleij 写道:
> On Thu, Nov 24, 2022 at 3:22 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>> 在 2022/11/24 上午6:05, Linus Walleij 写道:
> 
>>> But these drivers can not rely on the .gpio_to_irq() callback
>>> to be called before an IRQ is requested and used.
>>
>> I may not have made it clear before that the gpio irq chip for other
>> platforms may need to be implemented, but the loongson platform may be
>> special.
>>
>> I mean that the loongson platform use gpio irq does not need to rely on
>> gpio_to_irq, because loongson interrupt controller driver has covered
>> gpio irq.  The specific reason is my above explanation.
>>
>> so, Can I not realize gpio irq chip?
> 
> Isn't this a hierarchical irqchip then?
> 
> Please consult the following from
> Documentation/driver-api/gpio/driver.rst:
> 
> ---------------------------------
> 
> GPIO drivers providing IRQs
> ===========================
> 
> It is custom that GPIO drivers (GPIO chips) are also providing interrupts,
> most often cascaded off a parent interrupt controller, and in some special
> cases the GPIO logic is melded with a SoC's primary interrupt controller.
> 
> The IRQ portions of the GPIO block are implemented using an irq_chip, using
> the header <linux/irq.h>. So this combined driver is utilizing two sub-
> systems simultaneously: gpio and irq.
> 
> It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
> is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
> irq_chip are orthogonal, and offering their services independent of each
> other.
> 
> gpiod_to_irq() is just a convenience function to figure out the IRQ for a
> certain GPIO line and should not be relied upon to have been called before
> the IRQ is used.
> 
> Always prepare the hardware and make it ready for action in respective
> callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having
> been called first.
> 
> We can divide GPIO irqchips in two broad categories:
> 
> - CASCADED INTERRUPT CHIPS: this means that the GPIO chip has one common
>    interrupt output line, which is triggered by any enabled GPIO line on that
>    chip. The interrupt output line will then be routed to an parent interrupt
>    controller one level up, in the most simple case the systems primary
>    interrupt controller. This is modeled by an irqchip that will inspect bits
>    inside the GPIO controller to figure out which line fired it. The irqchip
>    part of the driver needs to inspect registers to figure this out and it
>    will likely also need to acknowledge that it is handling the interrupt
>    by clearing some bit (sometime implicitly, by just reading a status
>    register) and it will often need to set up the configuration such as
>    edge sensitivity (rising or falling edge, or high/low level interrupt for
>    example).
> 
> - HIERARCHICAL INTERRUPT CHIPS: this means that each GPIO line has a dedicated
>    irq line to a parent interrupt controller one level up. There is no need
>    to inquire the GPIO hardware to figure out which line has fired, but it
>    may still be necessary to acknowledge the interrupt and set up configuration
>    such as edge sensitivity.
Hi Linus,

My patch had send it to v11, but I have some issues. it seems more
appropriate add them here. the issue as follows:


mask_irq/unmask_irq/irq_ack/ function always be called by
handle_level_irq/handle_edge_irq in current irq domain. and the
handle_level_irq/handle_edge_irq will be called  by handle_irq_desc that
ask know which irq is.

when a peripheral need to use a gpio irq that gpio irq driver need know
irq status and call irq desc->irq_handler.

so I don't got it about which case it is unnecessary to know which irq.
> 
> ---------------------------------
> 
> You find an example of a hierarchical GPIO irqchip using the
> GPIOLIB_IRQCHIP in drivers/gpio/gpio-ixp4xx.c.


Loongson-2 gpio irq hardware only a enable register, and when a gpio irq 
happen, then will has a such flow:  "cpuintc -> liointc -> gpioinc ->
generic_handle_domain_irq -> handle_level_irq ->
peripheral-action(action->handler)"

generic_handle_domain_irq need rely on specific hwirq that ask gpio irq 
hardware has a status register but Loongson-2 gpio irq hardware doesn't 
have it.

so I still think it wasn't appropriate that for loongson-2 gpio driver
add a irq chip.

Yinbo.
> 
> Yours,
> Linus Walleij
>
  

在 2022/11/24 上午6:05, Linus Walleij 写道:
> On Wed, Nov 23, 2022 at 9:02 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>> 在 2022/11/21 下午9:24, Linus Walleij 写道:
> 
>>>> +static int loongson_gpio_request(
>>>> +                       struct gpio_chip *chip, unsigned int pin)
>>>> +{
>>>> +       if (pin >= chip->ngpio)
>>>> +               return -EINVAL;
>>>
>>> This is not needed, the gpiolib core already checks this. Drop it.
>> I check gpio_request in gpilib, I notice gpio_is_valid is not equal to
>> this condition, so I still kept it for byte mode.
> 
> This is because descriptors can only be obtained from gpiod_get() and
> similar and gpiod_get() falls to gpiod_get_index() which will not
> return a valid descriptor from either HW backend. gpiod_get()
> will call gpiod_request() for if and only if the descriptor is valid.
> 
> The only reason to implement something like this is because of
> using the legacy GPIO numberspace which we are getting rid
> of so it is irrelevant, the consumers of your driver will only be
> using gpio descriptors, will only come in through gpiod_get_index()
> and will have desc validity check done before calling gpiod_request().
> 
> So drop this.
> 
>>> I am bit suspicious that your IRQchip implementation expects consumers
>>> to call gpiod_to_irq() first and this is not legal.
>>
>> okay, I got it, and other driver use gpio interrupt doesn't rely on
>> gpiod_to_irq, but can use gpiod_to_irq.
> 
> Yes it can be used to look up the irq corresponding to a GPIO
> but it is not mandatory to do that.
> 
>> The reason is that gpio interrupt wasn't an independent module,  The
>> loongson interrupt controller liointc include lots of interrupt was
>> route to perpherial, such as i2c/spi .. gpio, so gpio interrupt as
>> normal perpherial interrupt, It is unnecessary and redundant to
>> implement a gpio irq chip. The liointc controller driver had cover all
>> interrupt.
> 
> This is fine, and it is common for GPIO drivers to implement
> their own IRQchips.
> 
> But these drivers can not rely on the .gpio_to_irq() callback
> to be called before an IRQ is requested and used.
> 
>> in addition,  I don't like to use the dynamically allocated gpio base,
>> so I set the gpio base after call bgpio_init.
> 
> Don't do that because the GPIO maintainers love the
> dynamic base and hate hardcoded bases. Set the base to -1
> If you wonder why, read drivers/gpio/TODO.
Hi Linus,

I recenly verfied other peripheral on upstream, some peripheral driver
need use gpio number, but if use dynamic base that gpio number will be
meaningless.  in additon I notice that many gpio driver don't use
dynamic base, although bgpio_int was called.

so I think the gpio number should be keep consistent with datasheet for
some platform that need use gpio number.

Yinbo.

> 
> Yours,
> Linus Walleij
>
  

On Mon, Dec 12, 2022 at 9:13 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:

> mask_irq/unmask_irq/irq_ack/ function always be called by
> handle_level_irq/handle_edge_irq in current irq domain. and the
> handle_level_irq/handle_edge_irq will be called  by handle_irq_desc that
> ask know which irq is.
>
> when a peripheral need to use a gpio irq that gpio irq driver need know
> irq status and call irq desc->irq_handler.
>
> so I don't got it about which case it is unnecessary to know which irq.

Sorry I don't understand what you are asking, can you elaborate?

Do you mean that you don't know which driver will not call ->to_irq()
on the gpiochip? That would be any driver that takes an IRQ directly in
the device tree:

gpio: gpio {
    interrupt-controller;
    #interrupt-cells = <2>;
    ....
};

device {
    interrupts = <&gpio 14 IRQ_TYPE_LEVEL_HIGH>;
    ....
};

This case will only call the irqchip callbacks and will never call
the .to_irq() on the gpio_chip.

> > You find an example of a hierarchical GPIO irqchip using the
> > GPIOLIB_IRQCHIP in drivers/gpio/gpio-ixp4xx.c.
>
> Loongson-2 gpio irq hardware only a enable register, and when a gpio irq
> happen, then will has a such flow:  "cpuintc -> liointc -> gpioinc ->
> generic_handle_domain_irq -> handle_level_irq ->
> peripheral-action(action->handler)"
>
> generic_handle_domain_irq need rely on specific hwirq that ask gpio irq
> hardware has a status register but Loongson-2 gpio irq hardware doesn't
> have it.
>
> so I still think it wasn't appropriate that for loongson-2 gpio driver
> add a irq chip.

generic_handle_domain_irq() is of no concern, what matters is if
your interrupt is hierarchical or not, the callback in the GPIO chip
can be a simple remapping of the numberspace followed by
a call to the parent callbacks.

Yours,
Linus Walleij
  

On Mon, Dec 12, 2022 at 9:34 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:

> > Don't do that because the GPIO maintainers love the
> > dynamic base and hate hardcoded bases. Set the base to -1
> > If you wonder why, read drivers/gpio/TODO.
>
> Hi Linus,
>
> I recenly verfied other peripheral on upstream, some peripheral driver
> need use gpio number,

What do you mean by upstream? If it is your own derivate kernel
(what we call *downstream) then it should be fixed, not
accommodated for.

If it is the mainline (Torvalds) kernel you are looking at legacy code
(such as boardfiles) which should be fixed or deleted. Beware that
the kernel contains much old code and bad examples which should
not be followed.

> but if use dynamic base that gpio number will be
> meaningless.  in additon I notice that many gpio driver don't use
> dynamic base, although bgpio_int was called.

Two wrongs does not make one right.
It is always possible to find bad examples in the kernel, because we
maintain lots of legacy code.
When in doubt, do what the maintainers say.
This maintainer says this: use a dynamic base.

> so I think the gpio number should be keep consistent with datasheet for
> some platform that need use gpio number.

So someone wrote a datasheet for a derivative, home-cooked kernel
that they had not bothered to synchronize with the community and
now the community should accommodate this mistake?

Sorry that is not how we work.

That datasheet is probably recommending old and bad practices, such
as using global GPIO numbers in drivers or using GPIO sysfs. The
GPIO maintainers do not approve of such stuff.

What about fixing the datasheet to say:
- We use dynamic GPIO number allocation
- Assign GPIOs to devices in the device tree with only HW GPIO number
  references = <&gpio HW_NUM GPIO_ACTIVE_HIGH>; etc
- For userspace access use libgpiod and /dev/gpiochipN, do not
  enable the legacy GPIO sysfs.

Yours,
Linus Walleij
  

在 2022/12/13 下午5:36, Linus Walleij 写道:
> On Mon, Dec 12, 2022 at 9:13 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 
>> mask_irq/unmask_irq/irq_ack/ function always be called by
>> handle_level_irq/handle_edge_irq in current irq domain. and the
>> handle_level_irq/handle_edge_irq will be called  by handle_irq_desc that
>> ask know which irq is.
>>
>> when a peripheral need to use a gpio irq that gpio irq driver need know
>> irq status and call irq desc->irq_handler.
>>
>> so I don't got it about which case it is unnecessary to know which irq.
> 
> Sorry I don't understand what you are asking, can you elaborate?
> 
> Do you mean that you don't know which driver will not call ->to_irq()
> on the gpiochip? That would be any driver that takes an IRQ directly in
> the device tree:
> 
> gpio: gpio {
>      interrupt-controller;
>      #interrupt-cells = <2>;
>      ....
> };
> 
> device {
>      interrupts = <&gpio 14 IRQ_TYPE_LEVEL_HIGH>;
>      ....
> };
> 
> This case will only call the irqchip callbacks and will never call
> the .to_irq() on the gpio_chip.

I mean that if the current domain is a valid doamin, every interrupt 
must have an interrupt status.

If there is no interrupt status, no one can cover the interrupt,
and no one will call 14 irq's desc->handle_irq (handle_level_irq)
and Loongson-2 gpio doesn't have irq status register, so Loongson-2
gpio irqchip shouldn't be implemented.
> 
>>> You find an example of a hierarchical GPIO irqchip using the
>>> GPIOLIB_IRQCHIP in drivers/gpio/gpio-ixp4xx.c.
>>
>> Loongson-2 gpio irq hardware only a enable register, and when a gpio irq
>> happen, then will has a such flow:  "cpuintc -> liointc -> gpioinc ->
>> generic_handle_domain_irq -> handle_level_irq ->
>> peripheral-action(action->handler)"
>>
>> generic_handle_domain_irq need rely on specific hwirq that ask gpio irq
>> hardware has a status register but Loongson-2 gpio irq hardware doesn't
>> have it.
>>
>> so I still think it wasn't appropriate that for loongson-2 gpio driver
>> add a irq chip.
> 
> generic_handle_domain_irq() is of no concern, what matters is if
> your interrupt is hierarchical or not, the callback in the GPIO chip
> can be a simple remapping of the numberspace followed by
> a call to the parent callbacks.
> 
> Yours,
> Linus Walleij
Hi

I think gpio irq domain should be a normal irq domain that like other 
irq domain. that need a function A to call virq desc handler in gpio irq 
domain.  if no use generic_handler_domain_irq or similar interface,
that will no one to call irq_desc->irq_handler.

"cpuintc -> liointc -> gpioinc -> generic_handle_domain_irq ->
handle_level_irq -> peripheral-action(action->handler)"
>