| Message ID | 20171220081556.GA30524@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/20/2017 02:15 AM, Stephen Boyd wrote: > Here's the patch. I get a hang when dumping debugfs, but at least > sysfs expose fails when trying to request blocked gpios. I need > to check if we need to say "yes" to pins that are above the gpio > max for pinctrl. I'll do that tomorrow. Sorry, I just don't see how this is better than my patches. I don't understand the need for involving the IRQ valid mask. I also don't see the value in adding code to look for a property that exists only in one ACPI HID (QCOM8002) as if it were generic. The "num-gpios" and "gpios" DSDs are not supposed to exist in any other HID, so there should be no code that reads it in pinctrl-msm. I'm going on vacation soon. I will post a v11 that eliminates support for QCOM8001. Maybe that version is "good enough" for now and you can add DT and/or IRQ support on top of it.
On 12/20, Timur Tabi wrote: > On 12/20/2017 02:15 AM, Stephen Boyd wrote: > >Here's the patch. I get a hang when dumping debugfs, but at least > >sysfs expose fails when trying to request blocked gpios. I need > >to check if we need to say "yes" to pins that are above the gpio > >max for pinctrl. I'll do that tomorrow. > > Sorry, I just don't see how this is better than my patches. I don't > understand the need for involving the IRQ valid mask. I also don't > see the value in adding code to look for a property that exists only > in one ACPI HID (QCOM8002) as if it were generic. The "num-gpios" > and "gpios" DSDs are not supposed to exist in any other HID, so > there should be no code that reads it in pinctrl-msm. I don't see how it hurts to treat it generically. Presumably that's the way it will be done on ACPI platforms going forward? No need to tie it to some ACPI HID. I'm trying to resolve everything at once: gpios, pinctrl pins, and irqs exposed by the TLMM hardware. The value is that we solve it all, once, now. The DT binding can also be resolved at the same time, so when we need to express this in DT it's already done. Otherwise, something can request irqs from the irqdomain even if the irq can't be enabled, or it can try to mux the pin to some other function, even if the function selection can't be configured. Boiling everything down into the irq valid mask should cover all these cases, and not require us to strip const from all the data in the non-ACPI pinctrl drivers to replace the value in the npins field at runtime.
On 12/20/17 6:39 PM, Stephen Boyd wrote: > I don't see how it hurts to treat it generically. Presumably > that's the way it will be done on ACPI platforms going forward? > No need to tie it to some ACPI HID. But it is tied to a HID. The "num-gpios" and "gpios" properties belong to a specific HID. Someone could create a new HID with different properties, and then what? That's why I want all the ACPI stuff in the client driver. At this point I don't really care any more about what the patches look like, but I really do think that putting the ACPI code in pinctrl-msm is a bad idea. We're debating adding support for multiple TLMMs, and we may create a new HID for that, so that we can define all pins on all TLMMs in one device. We would need to create a new HID and new DSDs to go with it. > I'm trying to resolve everything at once: gpios, pinctrl pins, > and irqs exposed by the TLMM hardware. The value is that we solve > it all, once, now. Keep in mind that I am now in vacation, and so I won't be able to submit any more patches for a while. > The DT binding can also be resolved at the > same time, so when we need to express this in DT it's already > done. Ok. > Otherwise, something can request irqs from the irqdomain > even if the irq can't be enabled, or it can try to mux the pin to > some other function, even if the function selection can't be > configured. Is it possible to request an IRQ for a pin if the pin itself can't be requested? > Boiling everything down into the irq valid mask should cover all > these cases, and not require us to strip const from all the data > in the non-ACPI pinctrl drivers to replace the value in the npins > field at runtime. Ok.
On 12/20, Timur Tabi wrote: > On 12/20/17 6:39 PM, Stephen Boyd wrote: > >I don't see how it hurts to treat it generically. Presumably > >that's the way it will be done on ACPI platforms going forward? > >No need to tie it to some ACPI HID. > > But it is tied to a HID. The "num-gpios" and "gpios" properties > belong to a specific HID. Someone could create a new HID with > different properties, and then what? That's why I want all the ACPI > stuff in the client driver. > > At this point I don't really care any more about what the patches > look like, but I really do think that putting the ACPI code in > pinctrl-msm is a bad idea. > > We're debating adding support for multiple TLMMs, and we may create > a new HID for that, so that we can define all pins on all TLMMs in > one device. We would need to create a new HID and new DSDs to go > with it. Ok. That's testable with acpi_match_device_ids() though. I can add that into pinctrl-msm.c so we don't have to pass info about available gpios from ACPI specific driver into the pinctrl-msm core driver. That's why I'm trying to avoid doing it in the ACPI specific driver. Do it close to where the gpiochip is created instead. Maybe future HIDs could follow the DT design and then we can look for the same device property name in both firmwares. Parsing ranges is simpler. > > >I'm trying to resolve everything at once: gpios, pinctrl pins, > >and irqs exposed by the TLMM hardware. The value is that we solve > >it all, once, now. > > Keep in mind that I am now in vacation, and so I won't be able to > submit any more patches for a while. > > >The DT binding can also be resolved at the > >same time, so when we need to express this in DT it's already > >done. > > Ok. > > >Otherwise, something can request irqs from the irqdomain > >even if the irq can't be enabled, or it can try to mux the pin to > >some other function, even if the function selection can't be > >configured. > > Is it possible to request an IRQ for a pin if the pin itself can't > be requested? I don't see any place blocking GPIOs turning into IRQs once we setup the irqdomain with interrupts. Maybe I missed something, but I think you can request an IRQ once the domain has the hwirqs associated with it. I will test it out.
On 12/21/2017 07:46 PM, Stephen Boyd wrote: > Ok. That's testable with acpi_match_device_ids() though. I can > add that into pinctrl-msm.c so we don't have to pass info about > available gpios from ACPI specific driver into the pinctrl-msm > core driver. That's why I'm trying to avoid doing it in the ACPI > specific driver. Do it close to where the gpiochip is created > instead. I guess we're just going to have to agree to disagree. I see the DSDs as being SOC-specific, and therefore belong in the SOC driver. I'm still going to need an SOC driver to define the TLMM register layout. But as I said earlier, I've already spent way too much time working on a driver that, in all likelihood, never be used in any production system anyway. I look forward to reviewing the next version of your patch. > Maybe future HIDs could follow the DT design and then we can look > for the same device property name in both firmwares. DSDs generally don't have the vendor prefix that DT properties do. > Parsing > ranges is simpler. I'm not sure I agree with that.
On Thu, 2018-01-04 at 09:46 -0600, Timur Tabi wrote: > On 12/21/2017 07:46 PM, Stephen Boyd wrote: > > > > Maybe future HIDs could follow the DT design and then we can look > > for the same device property name in both firmwares. > > DSDs generally don't have the vendor prefix that DT properties do. There are more means to check hardware revisions: HID - Hardware ID CID - Compatible ID UID - Unique ID (good to distinguish instances of the same device on the board) _HRV - Hardware Revision (6.1.6 describes this one) Everything is described in the spec. Does anybody care to read? P.S. More I reading this thread more I become thinking that people screw ACPI use in many ways...
On Thu, Jan 4, 2018 at 5:04 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2018-01-04 at 09:46 -0600, Timur Tabi wrote: >> On 12/21/2017 07:46 PM, Stephen Boyd wrote: >> > >> > Maybe future HIDs could follow the DT design and then we can look >> > for the same device property name in both firmwares. >> >> DSDs generally don't have the vendor prefix that DT properties do. > > There are more means to check hardware revisions: > HID - Hardware ID > CID - Compatible ID > UID - Unique ID (good to distinguish instances of the same device on the > board) > _HRV - Hardware Revision (6.1.6 describes this one) Thanks Andy. I expect a patch using these features, also include Andy on CC as it appears he knows how this should be done. (In difference from me...) I'm pretty much relying on other people to understand ACPI for me, maybe I should attend a course or something. Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 8db2680bf872..5f118f044caa 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1475,8 +1475,8 @@ static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip) gpiochip->irq_valid_mask = NULL; } -static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip, - unsigned int offset) +bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip, + unsigned int offset) { /* No mask means all valid */ if (likely(!gpiochip->irq_valid_mask)) diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 273badd92561..4c2ce1f7d449 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -105,6 +105,17 @@ static const struct pinctrl_ops msm_pinctrl_ops = { .dt_free_map = pinctrl_utils_free_map, }; +static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset) +{ + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + struct gpio_chip *chip = &pctrl->chip; + + if (gpiochip_irqchip_irq_valid(chip, offset)) + return 0; + + return -EINVAL; +} + static int msm_get_functions_count(struct pinctrl_dev *pctldev) { struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); @@ -166,6 +177,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, } static const struct pinmux_ops msm_pinmux_ops = { + .request = msm_pinmux_request, .get_functions_count = msm_get_functions_count, .get_function_name = msm_get_function_name, .get_function_groups = msm_get_function_groups, @@ -493,6 +505,9 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, "pull up" }; + if (!gpiochip_irqchip_irq_valid(chip, offset)) + return; + g = &pctrl->soc->groups[offset]; ctl_reg = readl(pctrl->regs + g->ctl_reg); @@ -503,7 +518,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func); seq_printf(s, " %dmA", msm_regval_to_drive(drive)); - seq_printf(s, " %s", pulls[pull]); + seq_printf(s, " %s\n", pulls[pull]); } static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) @@ -511,10 +526,8 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) unsigned gpio = chip->base; unsigned i; - for (i = 0; i < chip->ngpio; i++, gpio++) { + for (i = 0; i < chip->ngpio; i++, gpio++) msm_gpio_dbg_show_one(s, NULL, chip, i, gpio); - seq_puts(s, "\n"); - } } #else @@ -795,6 +808,76 @@ static void msm_gpio_irq_handler(struct irq_desc *desc) chained_irq_exit(chip, desc); } +static int msm_gpio_init_irq_valid_mask(struct gpio_chip *chip, + struct msm_pinctrl *pctrl) +{ + int ret; + unsigned int len, i; + unsigned int max_gpios = pctrl->soc->ngpios; + struct device_node *np = pctrl->dev->of_node; + + /* The number of GPIOs in the ACPI tables */ + ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0); + if (ret > 0 && ret < max_gpios) { + u16 *tmp; + + len = ret; + tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL); + if (!tmp) + return -ENOMEM; + + ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp, + len); + if (ret < 0) { + dev_err(pctrl->dev, "could not read list of GPIOs\n"); + kfree(tmp); + return ret; + } + + bitmap_zero(chip->irq_valid_mask, max_gpios); + for (i = 0; i < len; i++) + set_bit(tmp[i], chip->irq_valid_mask); + + return 0; + } + + /* If there's a DT ngpios-ranges property then add those ranges */ + ret = of_property_count_u32_elems(np, "ngpios-ranges"); + if (ret > 0 && ret % 2 == 0 && ret / 2 < max_gpios) { + u32 start; + u32 count; + + len = ret / 2; + bitmap_zero(chip->irq_valid_mask, max_gpios); + + for (i = 0; i < len; i++) { + of_property_read_u32_index(np, "ngpios-ranges", + i * 2, &start); + of_property_read_u32_index(np, "ngpios-ranges", + i * 2 + 1, &count); + bitmap_set(chip->irq_valid_mask, start, count); + } + } + + return 0; +} + +static bool msm_gpio_needs_irq_valid_mask(struct msm_pinctrl *pctrl) +{ + int ret; + struct device_node *np = pctrl->dev->of_node; + + ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0); + if (ret > 0) + return true; + + ret = of_property_count_u32_elems(np, "ngpios-ranges"); + if (ret > 0 && ret % 2 == 0) + return true; + + return false; +} + static int msm_gpio_init(struct msm_pinctrl *pctrl) { struct gpio_chip *chip; @@ -811,6 +894,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) chip->parent = pctrl->dev; chip->owner = THIS_MODULE; chip->of_node = pctrl->dev->of_node; + chip->irq_need_valid_mask = msm_gpio_needs_irq_valid_mask(pctrl); ret = gpiochip_add_data(&pctrl->chip, pctrl); if (ret) { @@ -818,6 +902,12 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) return ret; } + ret = msm_gpio_init_irq_valid_mask(chip, pctrl); + if (ret) { + dev_err(pctrl->dev, "Failed to setup irq valid bits\n"); + return ret; + } + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio); if (ret) { dev_err(pctrl->dev, "Failed to add pin range\n"); diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index af20369ec8e7..be977c1c7498 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -262,6 +262,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, bool nested, struct lock_class_key *lock_key); +bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip, + unsigned int offset); + #ifdef CONFIG_LOCKDEP /*