Message ID | 20191107214812.Horde.z_ETelVXryT7bme5Ed6oB16@www.vdorst.com |
---|---|
State | New |
Headers | show |
Series | mt7621: gpio-hog not working properly | expand |
On Thu, Nov 7, 2019 at 10:48 PM René van Dorst <opensource@vdorst.com> wrote: > DTS: > > &gpio { > sfp_i2c_clk_gate { > gpio-hog; > gpios = <7 GPIO_ACTIVE_LOW>; > output-high; > }; > }; > > root@OpenWrt:/# dmesg | grep hog > [ 3.095360] GPIO line 487 (sfp_i2c_clk_gate) hogged as output/high This looks correct, output/high and "high" in this case means asserted and it's active low so that should mean it is driven low. > gpiochip0: GPIOs 480-511, parent: platform/1e000600.gpio, 1e000600.gpio-bank0: > gpio-487 ( |sfp_i2c_clk_gate ) out hi ACTIVE LOW So that is wrong :( > DTS: > > &gpio { > sfp_i2c_clk_gate { > gpio-hog; > gpios = <7 GPIO_ACTIVE_HIGH>; > output-high; > }; > }; (...) > gpio-487 ( |sfp_i2c_clk_gate ) out hi OK that worked... sheer luck I guess. > DTS: > > &gpio { > sfp_i2c_clk_gate { > gpio-hog; > gpios = <7 GPIO_ACTIVE_HIGH>; > output-low; > }; > }; (...) > gpio-487 ( |sfp_i2c_clk_gate ) out hi Yeah now it is wrong again... > So as you can see gpio-hog is parsed well by the kernel. > But it setting up the data value is not. Please drill into the functions. What happens down in the callback to the actual driver? Can you check whether the .set_value() gets the right value or not so we see if there is a logical or physical problem? The mt7621 driver uses gpio-mmio so you should patch drivers/gpio/gpio-mmio.c function bgpio_set() or bgpio_set_set() I think. Yours, Linus Walleij
Quoting Linus Walleij <linus.walleij@linaro.org>: > On Thu, Nov 7, 2019 at 10:48 PM René van Dorst <opensource@vdorst.com> wrote: > >> DTS: >> >> &gpio { >> sfp_i2c_clk_gate { >> gpio-hog; >> gpios = <7 GPIO_ACTIVE_LOW>; >> output-high; >> }; >> }; >> >> root@OpenWrt:/# dmesg | grep hog >> [ 3.095360] GPIO line 487 (sfp_i2c_clk_gate) hogged as output/high > > This looks correct, output/high and "high" in this case means > asserted and it's active low so that should mean it is driven > low. > >> gpiochip0: GPIOs 480-511, parent: platform/1e000600.gpio, >> 1e000600.gpio-bank0: >> gpio-487 ( |sfp_i2c_clk_gate ) out hi ACTIVE LOW > > So that is wrong :( > >> DTS: >> >> &gpio { >> sfp_i2c_clk_gate { >> gpio-hog; >> gpios = <7 GPIO_ACTIVE_HIGH>; >> output-high; >> }; >> }; > (...) >> gpio-487 ( |sfp_i2c_clk_gate ) out hi > > OK that worked... sheer luck I guess. > >> DTS: >> >> &gpio { >> sfp_i2c_clk_gate { >> gpio-hog; >> gpios = <7 GPIO_ACTIVE_HIGH>; >> output-low; >> }; >> }; > (...) >> gpio-487 ( |sfp_i2c_clk_gate ) out hi > > Yeah now it is wrong again... > >> So as you can see gpio-hog is parsed well by the kernel. >> But it setting up the data value is not. > > Please drill into the functions. > > What happens down in the callback to the actual driver? > Can you check whether the .set_value() gets the right value > or not so we see if there is a logical or physical problem? > The mt7621 driver uses gpio-mmio so you should patch > drivers/gpio/gpio-mmio.c function bgpio_set() or > bgpio_set_set() I think. Hi Linus, Thanks for pointing me in the direction where to start debuging. mt7621 used bgpio_set_with_clear() because we have `set` and `clear` registers. [ 3.096085] gpiochip_find_base: found new base at 480 [ 3.106063] bgpio_set_with_clear: gpio-480 gpio:7 val:0x0 mask:0x80 reg:0xbe000640 [ 3.118968] bgpio_set_with_clear: before: gpio:7 reg:0xbe000640: value-of-data-reg:0xb75f7de, masked data:0x80 [ 3.134845] bgpio_write32: 0xbe000640 <= 0x80 [ 3.142116] bgpio_set_with_clear: after: gpio:7 reg:0xbe000640: value-of-data-reg:0xb75f7de, masked data:0x80 It seems that writing to the 'clear' register doesn't do anything. I noticed that register address is 0x1e000000 in the DTS but in the code it is 0xbe000000. [ 3.158002] bgpio_write32: 0xbe000600 <= 0x80 [ 3.165258] GPIO line 487 (sfp_i2c_clk_gate) hogged as output/high [ 3.177532] gpiochip_add_data_with_key: gpiochip0 gpio7: 0x843 When using program 'io' writing to the `clear` register 0x1e000640 does have effect. root@OpenWrt:/# io -4 0x1e000620 1e000620: 0b75f7de root@OpenWrt:/# io -4 -w 0x1e000640 0x80 root@OpenWrt:/# io -4 0x1e000620 1e000620: 0b75f74e If I change the bgpio_init() values so that we don't have the `set` and `clear` registers. With the patch below I do get right results. diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c index d1d785f983a7..186e8d6f3c64 100644 --- a/drivers/gpio/gpio-mt7621.c +++ b/drivers/gpio/gpio-mt7621.c @@ -228,7 +228,8 @@ mediatek_gpio_bank_probe(struct device *dev, diro = mtk->base + GPIO_REG_CTRL + (rg->bank * GPIO_BANK_STRIDE); ret = bgpio_init(&rg->chip, dev, 4, - dat, set, ctrl, diro, NULL, 0); + dat, NULL, NULL, diro, NULL, 0); + if (ret) { dev_err(dev, "bgpio_init() failed\n"); return ret; Any idea what this can be? Greats, René > > Yours, > Linus Walleij
On Wed, Nov 13, 2019 at 9:54 PM René van Dorst <opensource@vdorst.com> wrote: > mt7621 used bgpio_set_with_clear() because we have `set` and `clear` > registers. (...) > It seems that writing to the 'clear' register doesn't do anything. > I noticed that register address is 0x1e000000 in the DTS but in the > code it is 0xbe000000. Do you mean physical memory 0x1e000000 gets remapped at virtual memory 0xbe000000? > [ 3.158002] bgpio_write32: 0xbe000600 <= 0x80 > [ 3.165258] GPIO line 487 (sfp_i2c_clk_gate) hogged as output/high > [ 3.177532] gpiochip_add_data_with_key: gpiochip0 gpio7: 0x843 > > When using program 'io' writing to the `clear` register 0x1e000640 > does have effect. > > root@OpenWrt:/# io -4 0x1e000620 > 1e000620: 0b75f7de > root@OpenWrt:/# io -4 -w 0x1e000640 0x80 > root@OpenWrt:/# io -4 0x1e000620 > 1e000620: 0b75f74e (...) > If I change the bgpio_init() values so that we don't have the `set` > and `clear` registers. > With the patch below I do get right results. That's weird! > Any idea what this can be? Try to pass the flag BGPIOF_UNREADABLE_REG_SET, it seems the code is using the set register to read back the value. In this case it is actually working you just don't see it in debugfs. Cc:ed Sergio maybe he has some ideas. It seems to be hardware related then, hm. Yours, Linus Walleij
Quoting Linus Walleij <linus.walleij@linaro.org>: > On Wed, Nov 13, 2019 at 9:54 PM René van Dorst <opensource@vdorst.com> wrote: > >> mt7621 used bgpio_set_with_clear() because we have `set` and `clear` >> registers. > (...) >> It seems that writing to the 'clear' register doesn't do anything. >> I noticed that register address is 0x1e000000 in the DTS but in the >> code it is 0xbe000000. > > Do you mean physical memory 0x1e000000 gets remapped > at virtual memory 0xbe000000? Yes. mtk->base = be000600. [ 3.094301] mediatek_gpio_bank_probe: mtk->base = be000600 [ 3.105165] gpiochip_find_base: found new base at 480 [ 3.115135] bgpio_set_with_clear: gc-base-480 gpio:7 val:0x0 mask:0x80 data-reg-val:0xbe000640 [ 3.132196] bgpio_set_with_clear: before: g7 reg:0xbe000640: data:0xb75f7da, masked data:0x80 [ 3.149297] bgpio_write32: 0xbe000640 <= 0x80 [ 3.157935] bgpio_set_with_clear: after: g7 reg:0xbe000640: data:0xb75f7da, masked data:0x80 [ 3.175040] bgpio_write32: 0xbe000600 <= 0x80 [ 3.183682] GPIO line 487 (sfp_i2c_clk_gate) hogged as output/high [ 3.195955] gpiochip_add_data_with_key: gpiochip0 gpio7: 0x843 [ 3.207692] gpio gpiochip0: (1e000600.gpio-bank0): added GPIO chardev (254:0) > >> [ 3.158002] bgpio_write32: 0xbe000600 <= 0x80 >> [ 3.165258] GPIO line 487 (sfp_i2c_clk_gate) hogged as output/high >> [ 3.177532] gpiochip_add_data_with_key: gpiochip0 gpio7: 0x843 >> >> When using program 'io' writing to the `clear` register 0x1e000640 >> does have effect. >> >> root@OpenWrt:/# io -4 0x1e000620 >> 1e000620: 0b75f7de >> root@OpenWrt:/# io -4 -w 0x1e000640 0x80 >> root@OpenWrt:/# io -4 0x1e000620 >> 1e000620: 0b75f74e > (...) >> If I change the bgpio_init() values so that we don't have the `set` >> and `clear` registers. >> With the patch below I do get right results. > > That's weird! So this means that changing data register thru the 'set'/'clear' registers is not working but writing direct to the data register does. So the question is: why is writel() in bgpio_write32() not working on the 'set'/'clear' registers. > >> Any idea what this can be? > > Try to pass the flag BGPIOF_UNREADABLE_REG_SET, > it seems the code is using the set register to read back the > value. In this case it is actually working you just don't see > it in debugfs. debugfs is showing the actual results. Also I know when it is working. The kernel prints the SFP module info to the dmesg. read the gpio status: root@OpenWrt:/# cat /sys/kernel/debug/gpio gpiochip3: GPIOs 400-415, parent: i2c/0-0025, 0-0025, can sleep: gpio-405 ( |mod-def0 ) in lo ACTIVE LOW gpiochip2: GPIOs 416-447, parent: platform/1e000600.gpio, 1e000600.gpio-bank2: gpiochip1: GPIOs 448-479, parent: platform/1e000600.gpio, 1e000600.gpio-bank1: gpiochip0: GPIOs 480-511, parent: platform/1e000600.gpio, 1e000600.gpio-bank0: gpio-487 ( |sfp_i2c_clk_gate ) out hi ACTIVE LOW gpio-492 ( |reset ) in hi IRQ ACTIVE LOW gpio-496 ( |sysfs ) out hi gpio-497 ( |sysfs ) out lo gpio-498 ( |sysfs ) out hi gpio-499 ( |sysfs ) out lo gpio-500 ( |sysfs ) out hi root@OpenWrt:/# io -4 0x1e000620 1e000620: 0355f7de Clear gpio7 via 'clear' register: root@OpenWrt:/# io -4 -w 0x1e000640 0x80 read the gpio status again: root@OpenWrt:/# cat /sys/kernel/debug/gpio gpiochip3: GPIOs 400-415, parent: i2c/0-0025, 0-0025, can sleep: gpio-405 ( |mod-def0 ) in lo ACTIVE LOW gpiochip2: GPIOs 416-447, parent: platform/1e000600.gpio, 1e000600.gpio-bank2: gpiochip1: GPIOs 448-479, parent: platform/1e000600.gpio, 1e000600.gpio-bank1: gpiochip0: GPIOs 480-511, parent: platform/1e000600.gpio, 1e000600.gpio-bank0: gpio-487 ( |sfp_i2c_clk_gate ) out lo ACTIVE LOW gpio-492 ( |reset ) in hi IRQ ACTIVE LOW gpio-496 ( |sysfs ) out hi gpio-497 ( |sysfs ) out lo gpio-498 ( |sysfs ) out hi gpio-499 ( |sysfs ) out lo gpio-500 ( |sysfs ) out hi root@OpenWrt:/# io -4 0x1e000620 1e000620: 0b75f74e Values shown by debugfs are correct. Below the diff of my debug code and assembly of the bgpio_write32(); Greats, René > > Cc:ed Sergio maybe he has some ideas. > > It seems to be hardware related then, hm. > > Yours, > Linus Walleij diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index cd07c948649f..18d1b6937092 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -82,6 +82,7 @@ static unsigned long bgpio_read16(void __iomem *reg) static void bgpio_write32(void __iomem *reg, unsigned long data) { + pr_info("%s: 0x%px <= 0x%lx\n", __func__, reg, data); writel(data, reg); } @@ -150,6 +151,8 @@ static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask, unsigned long get_mask = 0; unsigned long set_mask = 0; + pr_info("%s: gpio-%d b0x%lx m0x%lx", __func__, gc->base, *bits, *mask); + /* Make sure we first clear any bits that are zero when we read the register */ *bits &= ~*mask; @@ -222,6 +225,8 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) unsigned long mask = bgpio_line2mask(gc, gpio); unsigned long flags; + pr_info("%s: gpio:%d val:0x%x mask:0x%lx", __func__, gpio, val, mask); + spin_lock_irqsave(&gc->bgpio_lock, flags); if (val) @@ -239,10 +244,22 @@ static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, { unsigned long mask = bgpio_line2mask(gc, gpio); + pr_info("%s: gc-base-%d gpio:%d val:0x%x mask:0x%lx data-reg-val:0x%px\n", __func__, gc->base, + gpio, val, mask, val ? gc->reg_set : gc->reg_clr); + + pr_info("%s: before: g%d reg:0x%px: data:0x%lx, masked data:0x%lx \n", __func__, + gpio, val ? gc->reg_set : gc->reg_clr, + gc->read_reg(gc->reg_dat), gc->read_reg(gc->reg_dat) & mask); + if (val) gc->write_reg(gc->reg_set, mask); else gc->write_reg(gc->reg_clr, mask); + + pr_info("%s: after: g%d reg:0x%px: data:0x%lx, masked data:0x%lx \n", __func__, + gpio, val ? gc->reg_set : gc->reg_clr, + gc->read_reg(gc->reg_dat), gc->read_reg(gc->reg_dat) & mask); + } static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) @@ -250,6 +267,8 @@ static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) unsigned long mask = bgpio_line2mask(gc, gpio); unsigned long flags; + pr_info("%s: g%d v0x%x m0x%lx", __func__, gpio, val, mask); + spin_lock_irqsave(&gc->bgpio_lock, flags); if (val) @@ -322,6 +341,8 @@ static void bgpio_set_multiple_with_clear(struct gpio_chip *gc, { + pr_info("%s: g%d v0x%x m0x%lx", __func__, gpio, val, mask); + spin_lock_irqsave(&gc->bgpio_lock, flags); if (val) @@ -322,6 +341,8 @@ static void bgpio_set_multiple_with_clear(struct gpio_chip *gc, { unsigned long set_mask, clear_mask; + pr_info("%s: gpio-%d b%lx m%lx\n", __func__, gc->base, *bits, *mask); + bgpio_multiple_get_masks(gc, mask, bits, &set_mask, &clear_mask); if (set_mask) diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c index d1d785f983a7..cff3c79e3561 100644 --- a/drivers/gpio/gpio-mt7621.c +++ b/drivers/gpio/gpio-mt7621.c @@ -222,13 +222,19 @@ mediatek_gpio_bank_probe(struct device *dev, rg->chip.of_node = node; rg->bank = bank; + pr_info("%s: mtk->base = %px\n", __func__, mtk->base); + dat = mtk->base + GPIO_REG_DATA + (rg->bank * GPIO_BANK_STRIDE); set = mtk->base + GPIO_REG_DSET + (rg->bank * GPIO_BANK_STRIDE); ctrl = mtk->base + GPIO_REG_DCLR + (rg->bank * GPIO_BANK_STRIDE); diro = mtk->base + GPIO_REG_CTRL + (rg->bank * GPIO_BANK_STRIDE); + //ret = bgpio_init(&rg->chip, dev, 4, + // dat, NULL, NULL, diro, NULL, 0); + ret = bgpio_init(&rg->chip, dev, 4, dat, set, ctrl, diro, NULL, 0); + if (ret) { dev_err(dev, "bgpio_init() failed\n"); return ret; @@ -283,6 +289,8 @@ mediatek_gpio_bank_probe(struct device *dev, return ret; } + pr_info("%s: data: %px(0x%x), set: %px, clr: %px\n", __func__, dat, mtk_gpio_r32(rg, GPIO_REG_DATA) , set, ctrl); + /* set polarity to low for all gpios */ mtk_gpio_w32(rg, GPIO_REG_POL, 0); diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index dba5f08f308c..ca797fc1528e 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1518,6 +1518,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, else clear_bit(FLAG_IS_OUT, &desc->flags); } + if (i == 7) + pr_info("%s: gpiochip%d gpio7: 0x%lx\n", __func__, gdev->id, desc->flags); } acpi_gpiochip_add(chip); bgpio_write32 decompiled. Normal: 8037c5b0 <bgpio_write32>: 8037c5b0: 0000000f sync 8037c5b4: ac850000 sw a1,0(a0) 8037c5b8: 03e00008 jr ra 8037c5bc: 00000000 nop With the debug pr_info() 8037cfa8 <bgpio_write32>: 8037cfa8: 27bdffe0 addiu sp,sp,-32 8037cfac: afb10018 sw s1,24(sp) 8037cfb0: 00a08825 move s1,a1 8037cfb4: afb00014 sw s0,20(sp) 8037cfb8: 3c058077 lui a1,0x8077 8037cfbc: 00808025 move s0,a0 8037cfc0: afbf001c sw ra,28(sp) 8037cfc4: 3c048077 lui a0,0x8077 8037cfc8: 24a5789c addiu a1,a1,30876 8037cfcc: 24847864 addiu a0,a0,30820 8037cfd0: 02203825 move a3,s1 8037cfd4: 0c01d729 jal 80075ca4 <printk> 8037cfd8: 02003025 move a2,s0 8037cfdc: 0000000f sync 8037cfe0: ae110000 sw s1,0(s0) 8037cfe4: 8fbf001c lw ra,28(sp) 8037cfe8: 8fb10018 lw s1,24(sp) 8037cfec: 8fb00014 lw s0,20(sp) 8037cff0: 03e00008 jr ra 8037cff4: 27bd0020 addiu sp,sp,32
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 9afbc0612126..f5d5f34701fe 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1405,6 +1405,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, else clear_bit(FLAG_IS_OUT, &desc->flags); } + if (i == 7) + pr_info("%s: gpiochip%d gpio7: 0x%lx\n", __func__, gdev->id, desc->flags); }