| Message ID | 1504798409-32041-2-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu 07 Sep 08:33 PDT 2017, Timur Tabi wrote: Sorry for the slow response, I finally met with Linus last week to discuss this. > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c [..] > @@ -825,13 +897,39 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > chip->owner = THIS_MODULE; > chip->of_node = pctrl->dev->of_node; > > + /* If the GPIO map is sparse, then we need to disable specific IRQs */ > + chip->irq_need_valid_mask = pctrl->soc->sparse; > + > ret = gpiochip_add_data(&pctrl->chip, pctrl); > if (ret) { > dev_err(pctrl->dev, "Failed register gpiochip\n"); > return ret; > } > > - ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio); > + /* > + * If irq_need_valid_mask is true, then gpiochip_add_data() will > + * initialize irq_valid_mask to all 1s. We need to clear all the > + * GPIOs that are unavailable, and we need to find each block > + * of consecutive available GPIOs are add them as pin ranges. > + */ > + if (chip->irq_need_valid_mask) { > + for (i = 0; i < ngpio; i++) > + if (!groups[i].npins) > + clear_bit(i, pctrl->chip.irq_valid_mask); > + > + while ((count = msm_gpio_get_next_range(pctrl, &start))) { > + ret = gpiochip_add_pin_range(&pctrl->chip, > + dev_name(pctrl->dev), > + start, start, count); > + if (ret) > + break; > + start += count; I do not fancy the idea of specifying a bitmap of valid irq pins and then having the driver register the pin-ranges in-between. If we provide a bitmap of validity to the core it should support using this for the pins as well. (Which I believe is what Linus answered in the discussion following patch 0/2) > + } > + } else { > + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), > + 0, 0, ngpio); > + } > + Regards, Bjorn
On 10/02/2017 12:44 PM, Bjorn Andersson wrote: >> + /* >> + * If irq_need_valid_mask is true, then gpiochip_add_data() will >> + * initialize irq_valid_mask to all 1s. We need to clear all the >> + * GPIOs that are unavailable, and we need to find each block >> + * of consecutive available GPIOs are add them as pin ranges. >> + */ >> + if (chip->irq_need_valid_mask) { >> + for (i = 0; i < ngpio; i++) >> + if (!groups[i].npins) >> + clear_bit(i, pctrl->chip.irq_valid_mask); >> + >> + while ((count = msm_gpio_get_next_range(pctrl, &start))) { >> + ret = gpiochip_add_pin_range(&pctrl->chip, >> + dev_name(pctrl->dev), >> + start, start, count); >> + if (ret) >> + break; >> + start += count; > I do not fancy the idea of specifying a bitmap of valid irq pins and > then having the driver register the pin-ranges in-between. But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. Here's even a reference to holes in the GPIO space: /* * Compute number of GPIOs from the last SoC gpio range descriptors * These ranges may include "holes" but the GPIO number space shall * still be homogeneous, so we need to detect and account for any * such holes so that these are included in the number of GPIO pins. */ > If we provide > a bitmap of validity to the core it should support using this for the > pins as well. (Which I believe is what Linus answered in the discussion > following patch 0/2) So you want to change "gpio_chip" to add an "available" callback? And every time gpiolib wants to call a gpio_chip callback, it should call ->available first? Like this: if (chip->available && chip->available()) status = chip->direction_input(chip, gpio_chip_hwgpio(desc)); I can do that, but it just seems very redundant. The core already knows not to touch GPIOs that are not in a pin range. The only exception is gpiochip_add_data(), as I've stated before. It just seems wrong to call an API every time to ask permission before we can call any other API. But since the API may not be defined, we have to first check if the API exists before we can ask permission.
On Mon, Oct 2, 2017 at 10:47 PM, Timur Tabi <timur@codeaurora.org> wrote: > On 10/02/2017 12:44 PM, Bjorn Andersson wrote: >>> >>> + /* >>> + * If irq_need_valid_mask is true, then gpiochip_add_data() will >>> + * initialize irq_valid_mask to all 1s. We need to clear all the >>> + * GPIOs that are unavailable, and we need to find each block >>> + * of consecutive available GPIOs are add them as pin ranges. >>> + */ >>> + if (chip->irq_need_valid_mask) { >>> + for (i = 0; i < ngpio; i++) >>> + if (!groups[i].npins) >>> + clear_bit(i, pctrl->chip.irq_valid_mask); >>> + >>> + while ((count = msm_gpio_get_next_range(pctrl, &start))) >>> { >>> + ret = gpiochip_add_pin_range(&pctrl->chip, >>> + >>> dev_name(pctrl->dev), >>> + start, start, >>> count); >>> + if (ret) >>> + break; >>> + start += count; >> >> I do not fancy the idea of specifying a bitmap of valid irq pins and >> then having the driver register the pin-ranges in-between. > > But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. Here's > even a reference to holes in the GPIO space: This driver is not a good example of what is desireable. I am sorry that the kernel contains a lot of bad examples. These are historical artifacts, they cannot be used as an argument to write code in the same style. >> If we provide >> >> a bitmap of validity to the core it should support using this for the >> pins as well. (Which I believe is what Linus answered in the discussion >> following patch 0/2) > > So you want to change "gpio_chip" to add an "available" callback? And every > time gpiolib wants to call a gpio_chip callback, it should call ->available > first? Like this: > > if (chip->available && chip->available()) > status = chip->direction_input(chip, gpio_chip_hwgpio(desc)); I mean that you add unsigned long *line_valid_mask; to struct gpio_chip using the same type of logic as .irq_valid_mask. The mask should be optional. Then the operation gpiod_get[_index]() will FAIL if the line is not valid. There is no need to check on every operation, there should be no way to get a handle on the pin any other way. All new code should be using descriptors at this point so we only need to design for that case. Yours, Linus Walleij
On 10/07/2017 06:07 AM, Linus Walleij wrote: > I mean that you add > unsigned long *line_valid_mask; > to struct gpio_chip using the same type of logic > as .irq_valid_mask. The mask should be optional. Hmmm okay > Then the operation gpiod_get[_index]() will FAIL if the > line is not valid. Just to be clear, you want gpiod_get_index() to check line_valid_mask (if present) and return -ENODEV if not valid? > There is no need to check on every operation, there should > be no way to get a handle on the pin any other way. > > All new code should be using descriptors at this point so we > only need to design for that case. Ok, I will check this.
On Thu, Sep 07, 2017 at 10:33:28AM -0500, Timur Tabi wrote: [...] > ret = gpiochip_add_data(&pctrl->chip, pctrl); > if (ret) { > dev_err(pctrl->dev, "Failed register gpiochip\n"); > return ret; > } > > - ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio); > + /* > + * If irq_need_valid_mask is true, then gpiochip_add_data() will > + * initialize irq_valid_mask to all 1s. We need to clear all the > + * GPIOs that are unavailable, and we need to find each block > + * of consecutive available GPIOs are add them as pin ranges. > + */ > + if (chip->irq_need_valid_mask) { > + for (i = 0; i < ngpio; i++) > + if (!groups[i].npins) > + clear_bit(i, pctrl->chip.irq_valid_mask); > + > + while ((count = msm_gpio_get_next_range(pctrl, &start))) { > + ret = gpiochip_add_pin_range(&pctrl->chip, > + dev_name(pctrl->dev), > + start, start, count); > + if (ret) > + break; > + start += count; > + } > + } else { > + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), > + 0, 0, ngpio); > + } This is the bit that I don't understand. If you only tell gpiolib about the GPIOs that exist, then it won't be initializing the .irq_valid_mask in a wrong way. In other words, what I'm trying to say is that in your case, ngpio isn't equal to the sum of .npins over all groups. If instead you make the chip register only the lines that actually exist, .irq_valid_mask will only contain bits that do exist. The reason I'm bringing this up is because we had the same discussion back in November last year (yes, that driver still isn't upstream...): https://lkml.org/lkml/2016/11/22/543 In a nutshell: the Tegra driver was assuming that each port had a fixed number (8) of lines, but when gpiolib changed to query the direction of GPIOs at driver probe time this broke badly because on of the instances of the GPIO controller is very strict about what it allows access to. This seems to be similar to what you're experiencing. In our case we'd run into a hard hang trying to access a register for a non-existing GPIO. One of the possibilities discussed in the thread was to introduce something akin to what's being proposed here. In the end it turned out to be easiest to just don't tell gpiolib about any non-existing GPIOs, because then you don't get to deal with any of these workarounds. The downside is that you need a little more code to find out exactly what GPIO you're talking about, or to determine how many you have. In the end I think it's all worth it by making it a lot easier in general to deal with. I think it's really confusing if you expose, say, 200 pins, and for 50 of them users will get -EACCES, or -ENOENT or whatever if they try to use them. Thierry
On 10/16/2017 03:01 AM, Thierry Reding wrote: > This is the bit that I don't understand. If you only tell gpiolib about > the GPIOs that exist, then it won't be initializing the .irq_valid_mask > in a wrong way. I know, and that's what my patches do. I tell gpiolib that I need a valid mask: /* If the GPIO map is sparse, then we need to disable specific IRQs */ chip->irq_need_valid_mask = pctrl->soc->sparse; Then I proceed to provide the specific GPIOs that exist, one block at a time: ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), start, start, count); > In other words, what I'm trying to say is that in your case, ngpio isn't > equal to the sum of .npins over all groups. That's true. I set it to the maximum number of GPIOs that exist on the chip. I could set it to the highest number of GPIOs that I register (e.g. the highest value of "start + count"), but that's not necessary for my patches to work. > If instead you make the chip > register only the lines that actually exist, .irq_valid_mask will only > contain bits that do exist. That's what I'm doing. > The reason I'm bringing this up is because we had the same discussion > back in November last year (yes, that driver still isn't upstream...): > > https://lkml.org/lkml/2016/11/22/543 > > In a nutshell: the Tegra driver was assuming that each port had a fixed > number (8) of lines, but when gpiolib changed to query the direction of > GPIOs at driver probe time this broke badly because on of the instances > of the GPIO controller is very strict about what it allows access to. It appear we're re-hashing that same exact argument. > This seems to be similar to what you're experiencing. In our case we'd > run into a hard hang trying to access a register for a non-existing > GPIO. One of the possibilities discussed in the thread was to introduce > something akin to what's being proposed here. I guess great minds do think alike! > In the end it turned out to be easiest to just don't tell gpiolib about > any non-existing GPIOs, because then you don't get to deal with any of > these workarounds. I agree. > The downside is that you need a little more code to > find out exactly what GPIO you're talking about, or to determine how > many you have. In my case, the ACPI table provides an exact list that I use at probe() time. > In the end I think it's all worth it by making it a lot > easier in general to deal with. I think it's really confusing if you > expose, say, 200 pins, and for 50 of them users will get -EACCES, or > -ENOENT or whatever if they try to use them. True, but /sys/kernel/debug/gpio is the only mechanism I found where the user can get a list of valid GPIOs. Maybe we need something similar in /sys/class/gpio/.
On 10/07/2017 06:07 AM, Linus Walleij wrote: > I mean that you add > unsigned long *line_valid_mask; > to struct gpio_chip using the same type of logic > as .irq_valid_mask. The mask should be optional. > > Then the operation gpiod_get[_index]() will FAIL if the > line is not valid. > > There is no need to check on every operation, there should > be no way to get a handle on the pin any other way. > > All new code should be using descriptors at this point so we > only need to design for that case. I think I'm missing something, because I implemented what you're asking for, but whenever I try to expose a GPIO from sysfs (e.g. echo 37 > /sys/class/gpio/export), gpiod_get_index() is not called. The only thing preventing the GPIO from being exported is the ->request callback: static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset) { struct msm_pinctrl *pctrl = gpiochip_get_data(chip); const struct msm_pingroup *g = &pctrl->soc->groups[offset]; if (!g->npins) return -ENODEV; What operation is supposed to trigger a call to gpiod_get_index?
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index ff491da64dab..ca4ae3d76eb4 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -443,6 +443,14 @@ static int msm_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) g = &pctrl->soc->groups[offset]; + /* + * During initialization, gpiolib may query all GPIOs for their + * initial direction, regardless if they exist, so block access + * to those that are unavailable. + */ + if (!g->npins) + return -ENODEV; + val = readl(pctrl->regs + g->ctl_reg); /* 0 = output, 1 = input */ @@ -507,6 +515,11 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, }; g = &pctrl->soc->groups[offset]; + + /* If the GPIO group has no pins, then don't show it. */ + if (!g->npins) + return; + ctl_reg = readl(pctrl->regs + g->ctl_reg); is_out = !!(ctl_reg & BIT(g->oe_bit)); @@ -516,7 +529,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) @@ -524,23 +537,36 @@ 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 #define msm_gpio_dbg_show NULL #endif +/* + * If the requested GPIO has no pins, then treat it as unavailable. + * Otherwise, call the standard request function. + */ +static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset) +{ + struct msm_pinctrl *pctrl = gpiochip_get_data(chip); + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; + + if (!g->npins) + return -ENODEV; + + return gpiochip_generic_request(chip, offset); +} + static const struct gpio_chip msm_gpio_template = { .direction_input = msm_gpio_direction_input, .direction_output = msm_gpio_direction_output, .get_direction = msm_gpio_get_direction, .get = msm_gpio_get, .set = msm_gpio_set, - .request = gpiochip_generic_request, + .request = msm_gpio_request, .free = gpiochip_generic_free, .dbg_show = msm_gpio_dbg_show, }; @@ -808,11 +834,57 @@ static void msm_gpio_irq_handler(struct irq_desc *desc) chained_irq_exit(chip, desc); } +/** + * msm_gpio_get_next_range() - find next range of consecutive gpios + * @pctrl: msm_pinctrl object + * @pstart: pointer to index of next starting position + * + * In a sparse GPIO map, available GPIOs are typically collected into blocks. + * Given a starting index into the groups[] array, this function will scan all + * the gpios until it finds the next block of available GPIOs. + * + * If groups[*pstart] already points to an available GPIO, then assume that + * it is the start of a block. + * + * The caller is responsible for moving *pstart to after the end of the + * previous block so that it this function can find the next block. + * + * Return: The count of the block starting at @pstart, or 0 if there are no + * more blocks. + */ +static unsigned int msm_gpio_get_next_range(struct msm_pinctrl *pctrl, + unsigned int *pstart) +{ + const struct msm_pingroup *groups = pctrl->soc->groups; + unsigned int ngpio = pctrl->soc->ngpios; + unsigned int count = 0; + unsigned int start = *pstart; + + /* Find the first available GPIO */ + while (!groups[start].npins) + if (++start == ngpio) + /* None found, so just exit */ + return 0; + + *pstart = start; + + /* Count the number of those GPIOs in a row */ + while (groups[start].npins) { + count++; + if (++start == ngpio) + break; + } + + return count; +} + static int msm_gpio_init(struct msm_pinctrl *pctrl) { struct gpio_chip *chip; int ret; unsigned ngpio = pctrl->soc->ngpios; + const struct msm_pingroup *groups = pctrl->soc->groups; + unsigned int i, start = 0, count; if (WARN_ON(ngpio > MAX_NR_GPIO)) return -EINVAL; @@ -825,13 +897,39 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) chip->owner = THIS_MODULE; chip->of_node = pctrl->dev->of_node; + /* If the GPIO map is sparse, then we need to disable specific IRQs */ + chip->irq_need_valid_mask = pctrl->soc->sparse; + ret = gpiochip_add_data(&pctrl->chip, pctrl); if (ret) { dev_err(pctrl->dev, "Failed register gpiochip\n"); return ret; } - ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio); + /* + * If irq_need_valid_mask is true, then gpiochip_add_data() will + * initialize irq_valid_mask to all 1s. We need to clear all the + * GPIOs that are unavailable, and we need to find each block + * of consecutive available GPIOs are add them as pin ranges. + */ + if (chip->irq_need_valid_mask) { + for (i = 0; i < ngpio; i++) + if (!groups[i].npins) + clear_bit(i, pctrl->chip.irq_valid_mask); + + while ((count = msm_gpio_get_next_range(pctrl, &start))) { + ret = gpiochip_add_pin_range(&pctrl->chip, + dev_name(pctrl->dev), + start, start, count); + if (ret) + break; + start += count; + } + } else { + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), + 0, 0, ngpio); + } + if (ret) { dev_err(pctrl->dev, "Failed to add pin range\n"); gpiochip_remove(&pctrl->chip); diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h index 9b9feea540ff..70762bcb84cb 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.h +++ b/drivers/pinctrl/qcom/pinctrl-msm.h @@ -107,6 +107,7 @@ struct msm_pingroup { * @ngroups: The numbmer of entries in @groups. * @ngpio: The number of pingroups the driver should expose as GPIOs. * @pull_no_keeper: The SoC does not support keeper bias. + * @sparse: The GPIO map is sparse (some GPIOs have npins == 0) */ struct msm_pinctrl_soc_data { const struct pinctrl_pin_desc *pins; @@ -117,6 +118,7 @@ struct msm_pinctrl_soc_data { unsigned ngroups; unsigned ngpios; bool pull_no_keeper; + bool sparse; }; int msm_pinctrl_probe(struct platform_device *pdev,
pinctrl-msm only accepts an array of GPIOs from 0 to n-1, and it expects each group to support have only one pin (npins == 1). We can support "sparse" GPIO maps if we allow for some groups to have zero pins (npins == 0). These pins are "hidden" from the rest of the driver and gpiolib. A new boolean 'sparse' indicates whether the GPIO map is sparse. If any GPIO has an 'npins' value of 0, then 'sparse' must be set to True. Most access to unavailable GPIOs can be blocked via the gpio_chip.request function. The one exception is when gpiochip_add_data() scans all of the GPIOs without "requesting" them. To cover this case, msm_gpio_get_direction() separately checks if the GPIO is available. Signed-off-by: Timur Tabi <timur@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 110 +++++++++++++++++++++++++++++++++++-- drivers/pinctrl/qcom/pinctrl-msm.h | 2 + 2 files changed, 106 insertions(+), 6 deletions(-)