| Message ID | 20221121123803.3786-2-zhuyinbo@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/3] gpio: loongson2ef: move driver to original location | expand |
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)" >
diff --git a/MAINTAINERS b/MAINTAINERS index 7b80a64b70b9..47721a25249f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12048,6 +12048,13 @@ S: Maintained F: Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml F: drivers/soc/loongson/loongson2_guts.c +LOONGSON GPIO DRIVER +M: Yinbo Zhu <zhuyinbo@loongson.cn> +L: linux-gpio@vger.kernel.org +S: Maintained +F: drivers/gpio/gpio-loongson.c +F: include/linux/platform_data/gpio-loongson.h + LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI) M: Sathya Prakash <sathya.prakash@broadcom.com> M: Sreekanth Reddy <sreekanth.reddy@broadcom.com> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index bc55b80f212a..3986b471647d 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -375,6 +375,16 @@ config GPIO_LOGICVC Say yes here to support GPIO functionality of the Xylon LogiCVC programmable logic block. +config GPIO_LOONGSON + tristate "Loongson GPIO support" + depends on LOONGARCH || COMPILE_TEST + help + Say yes here to support the GPIO functionality of a number of + Loongson series of chips. The Loongson GPIO controller supports + up to 60 GPIOS in total, 4 of which are dedicated GPIO pins, and + the remaining 56 are reused with other functions, with edge or + level triggered interrupts. + config GPIO_LPC18XX tristate "NXP LPC18XX/43XX GPIO support" default y if ARCH_LPC18XX diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index cfd298e00737..29e3beb6548c 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -77,6 +77,7 @@ obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o obj-$(CONFIG_GPIO_KEMPLD) += gpio-kempld.o obj-$(CONFIG_GPIO_LOGICVC) += gpio-logicvc.o obj-$(CONFIG_GPIO_LOONGSON1) += gpio-loongson1.o +obj-$(CONFIG_GPIO_LOONGSON) += gpio-loongson.o obj-$(CONFIG_GPIO_LP3943) += gpio-lp3943.o obj-$(CONFIG_GPIO_LP873X) += gpio-lp873x.o obj-$(CONFIG_GPIO_LP87565) += gpio-lp87565.o diff --git a/drivers/gpio/gpio-loongson.c b/drivers/gpio/gpio-loongson.c new file mode 100644 index 000000000000..9fb65ba2c4bd --- /dev/null +++ b/drivers/gpio/gpio-loongson.c @@ -0,0 +1,306 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Loongson GPIO Support + * + * Copyright (C) 2022 Loongson Technology Corporation Limited + */ + +#include <linux/acpi.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/spinlock.h> +#include <linux/err.h> +#include <linux/gpio/driver.h> +#include <linux/platform_device.h> +#include <linux/bitops.h> +#include <asm/types.h> + +#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) + +#define LOONGSON_GPIO_IN_BYTE(x, gpio) (x->base +\ + x->p_data->in_offset + gpio) +#define LOONGSON_GPIO_OUT_BYTE(x, gpio) (x->base +\ + x->p_data->out_offset + gpio) +#define LOONGSON_GPIO_OEN_BYTE(x, gpio) (x->base +\ + x->p_data->conf_offset + gpio) + +enum loongson_gpio_mode { + BIT_CTRL_MODE, + BYTE_CTRL_MODE, +}; + +struct loongson_gpio_platform_data { + const char *label; + enum loongson_gpio_mode mode; + int conf_offset; + int out_offset; + int in_offset; +}; + +struct loongson_gpio_chip { + struct gpio_chip chip; + spinlock_t lock; + void __iomem *base; + u16 *gsi_idx_map; + u16 mapsize; + const struct loongson_gpio_platform_data *p_data; +}; + +static int loongson_gpio_request( + struct gpio_chip *chip, unsigned int pin) +{ + if (pin >= chip->ngpio) + return -EINVAL; + + return 0; +} + +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)); + } +} + +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)); + } +} + +static int loongson_gpio_direction_input( + struct gpio_chip *chip, unsigned int pin) +{ + unsigned long flags; + struct loongson_gpio_chip *lgpio = + container_of(chip, struct loongson_gpio_chip, chip); + + spin_lock_irqsave(&lgpio->lock, flags); + __set_direction(lgpio, pin, 1); + spin_unlock_irqrestore(&lgpio->lock, flags); + + return 0; +} + +static int loongson_gpio_direction_output( + struct gpio_chip *chip, unsigned int pin, + int value) +{ + struct loongson_gpio_chip *lgpio = + container_of(chip, struct loongson_gpio_chip, chip); + unsigned long flags; + + spin_lock_irqsave(&lgpio->lock, flags); + __set_level(lgpio, pin, value); + __set_direction(lgpio, pin, 0); + spin_unlock_irqrestore(&lgpio->lock, flags); + + return 0; +} + +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; + } + + return val; +} + +static void loongson_gpio_set(struct gpio_chip *chip, unsigned int pin, + int value) +{ + unsigned long flags; + struct loongson_gpio_chip *lgpio = + container_of(chip, struct loongson_gpio_chip, chip); + + spin_lock_irqsave(&lgpio->lock, flags); + __set_level(lgpio, pin, value); + spin_unlock_irqrestore(&lgpio->lock, flags); +} + +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); +} + +static int loongson_gpio_init( + struct device *dev, struct loongson_gpio_chip *lgpio, + struct device_node *np, void __iomem *base) +{ + 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; + lgpio->chip.can_sleep = 0; + lgpio->chip.of_node = np; + lgpio->chip.parent = dev; + lgpio->chip.label = lgpio->p_data->label; + spin_lock_init(&lgpio->lock); + lgpio->base = (void __iomem *)base; + lgpio->chip.to_irq = loongson_gpio_to_irq; + + devm_gpiochip_add_data(dev, &lgpio->chip, lgpio); + + return 0; +} + +static void loongson_gpio_get_props(struct platform_device *pdev, + struct loongson_gpio_chip *lgpio) +{ + int rval; + u32 ngpios; + struct device *dev = &pdev->dev; + + device_property_read_u32(dev, "ngpios", &ngpios); + lgpio->chip.ngpio = ngpios; + + rval = device_property_read_u16_array(dev, "gsi_idx_map", NULL, 0); + if (rval > 0) { + lgpio->gsi_idx_map = + devm_kmalloc_array(dev, rval, sizeof(*lgpio->gsi_idx_map), + GFP_KERNEL); + if (lgpio->gsi_idx_map) { + lgpio->mapsize = rval; + device_property_read_u16_array(dev, "gsi_idx_map", + lgpio->gsi_idx_map, lgpio->mapsize); + } + } +} + +static int loongson_gpio_probe(struct platform_device *pdev) +{ + void __iomem *base; + struct loongson_gpio_chip *lgpio; + struct device_node *np = pdev->dev.of_node; + struct device *dev = &pdev->dev; + + lgpio = devm_kzalloc(dev, sizeof(*lgpio), GFP_KERNEL); + if (!lgpio) + return -ENOMEM; + + loongson_gpio_get_props(pdev, lgpio); + + lgpio->p_data = device_get_match_data(&pdev->dev); + + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return PTR_ERR(base); + + platform_set_drvdata(pdev, lgpio); + + loongson_gpio_init(dev, lgpio, np, base); + + return 0; +} + +static const struct loongson_gpio_platform_data loongson_gpio_pdata0 = { + .label = "ls2k_gpio", + .mode = BIT_CTRL_MODE, + .conf_offset = 0x0, + .in_offset = 0x10, + .out_offset = 0x20, +}; + +static const struct loongson_gpio_platform_data loongson_gpio_pdata1 = { + .label = "ls7a_gpio", + .mode = BYTE_CTRL_MODE, + .conf_offset = 0x800, + .in_offset = 0x900, + .out_offset = 0xa00, +}; + +static const struct of_device_id loongson_gpio_of_match[] = { + { + .compatible = "loongson,ls2k-gpio", + .data = &loongson_gpio_pdata0, + }, + { + .compatible = "loongson,ls7a-gpio", + .data = &loongson_gpio_pdata1, + }, + {} +}; +MODULE_DEVICE_TABLE(of, loongson_gpio_of_match); + +static const struct acpi_device_id loongson_gpio_acpi_match[] = { + { + .id = "LOON0002", + .driver_data = (kernel_ulong_t)&loongson_gpio_pdata1, + }, + {} +}; +MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match); + +static struct platform_driver loongson_gpio_driver = { + .driver = { + .name = "loongson-gpio", + .owner = THIS_MODULE, + .of_match_table = loongson_gpio_of_match, + .acpi_match_table = loongson_gpio_acpi_match, + }, + .probe = loongson_gpio_probe, +}; + +static int __init loongson_gpio_setup(void) +{ + return platform_driver_register(&loongson_gpio_driver); +} +postcore_initcall(loongson_gpio_setup); + +MODULE_DESCRIPTION("Loongson gpio driver"); +MODULE_LICENSE("GPL");