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

From: Yinbo Zhu
Date: Wed Nov 23 2022 - 03:02:23 EST

Next message: Tian, Kevin: "RE: [patch V2 07/33] genirq/msi: Provide msi_create/free_device_irq_domain()"
Previous message: Zheng Yejian: "Re: [PATCH v2] tracing: Optimize event type allocation with IDA"
In reply to: Linus Walleij: "Re: [PATCH v5 2/3] gpio: loongson: add gpio driver support"
Next in thread: Linus Walleij: "Re: [PATCH v5 2/3] gpio: loongson: add gpio driver support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

在 2022/11/21 下午9:24, Linus Walleij 写道:

On Mon, Nov 21, 2022 at 1:38 PM Yinbo Zhu <zhuyinbo@xxxxxxxxxxx> 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@xxxxxxxxxxx>
Signed-off-by: Hongchen Zhang <zhanghongchen@xxxxxxxxxxx>
Signed-off-by: Liu Peibao <liupeibao@xxxxxxxxxxx>
Signed-off-by: Juxin Gao <gaojuxin@xxxxxxxxxxx>
Signed-off-by: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
---
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

Next message: Tian, Kevin: "RE: [patch V2 07/33] genirq/msi: Provide msi_create/free_device_irq_domain()"
Previous message: Zheng Yejian: "Re: [PATCH v2] tracing: Optimize event type allocation with IDA"
In reply to: Linus Walleij: "Re: [PATCH v5 2/3] gpio: loongson: add gpio driver support"
Next in thread: Linus Walleij: "Re: [PATCH v5 2/3] gpio: loongson: add gpio driver support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]