| Message ID | 1498164850-4738-1-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 22, 2017 at 10:54 PM, Timur Tabi <timur@codeaurora.org> wrote: > On Qualcomm Technologies QDF2xxx platforms, only a subset of the GPIOs > are actually available to the HLOS. The others are blocked by the XPU, > and any attempt to access them will cause an XPU violation that halts > the system. That's a bummer. Looking for Björn's review on this patch. > Instead, the ACPI table now lists only specific GPIOs that are exposed > externally as generic GPIO pins. The ACPI table or the device tree property "gpios" right? (But maybe this system is only using ACPI.) > To maintain consistency, the GPIOs are > enumerated 0 .. (N-1) as before, so that "gpio0" is really whatever is > the first GPIO listed in ACPI. The actual mapping is available via > /sys/kernel/debug/gpio. I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect the available GPIOs. If you haven't tested the GPIO tools, please try it out. Yours, Linus Walleij
On 6/23/17 7:33 AM, Linus Walleij wrote: >> Instead, the ACPI table now lists only specific GPIOs that are exposed >> externally as generic GPIO pins. > > The ACPI table or the device tree property "gpios" right? > > (But maybe this system is only using ACPI.) It's only ACPI. > I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect > the available GPIOs. > > If you haven't tested the GPIO tools, please try it out. I was not aware of these tools. I will try them out, thanks.
On Fri 23 Jun 05:33 PDT 2017, Linus Walleij wrote: > On Thu, Jun 22, 2017 at 10:54 PM, Timur Tabi <timur@codeaurora.org> wrote: > > > On Qualcomm Technologies QDF2xxx platforms, only a subset of the GPIOs > > are actually available to the HLOS. The others are blocked by the XPU, > > and any attempt to access them will cause an XPU violation that halts > > the system. > > That's a bummer. Looking for Björn's review on this patch. > The TLMM block on this platform has N gpios, regardless of the XPU configuration. The ACPI table provides the kernel with a list of available "logical" GPIOs based on some system specification (or reference design perhaps?). I have not seen the Qualcomm server platforms, but I imagine that customers can alter this list. I think the numbering of the GPIOs should be that of the TLMM and the "logical" names should be populated from ACPI by using gpio_chip->names. The question left is how to represent the XPU locked gpios - a problem we do share with in the mobile TLMM drivers. It seems that if we extend the msm_pingroup with a flag to carry the XPU lock information and then implement pinmux_ops->request() and deny any requests on the locked pins, we should cover all[*] the normal GPIO code paths. [*] Except the reported case of gpiochip_add_data() looping over all pins and calling get_direction() without first requesting the pin. @Linus, do you see any flaws in this scheme? PS. gpiochip_generic_request() ends up calling pinmux_ops->request() Regards, Bjorn
On 06/28/2017 02:12 PM, Bjorn Andersson wrote: > On Fri 23 Jun 05:33 PDT 2017, Linus Walleij wrote: > >> On Thu, Jun 22, 2017 at 10:54 PM, Timur Tabi <timur@codeaurora.org> wrote: >> >>> On Qualcomm Technologies QDF2xxx platforms, only a subset of the GPIOs >>> are actually available to the HLOS. The others are blocked by the XPU, >>> and any attempt to access them will cause an XPU violation that halts >>> the system. >> >> That's a bummer. Looking for Björn's review on this patch. >> > > The TLMM block on this platform has N gpios, regardless of the XPU > configuration. The ACPI table provides the kernel with a list of > available "logical" GPIOs based on some system specification (or > reference design perhaps?). It's the list of available physical GPIOs. Here's the table: Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { // Expose only the qdss_tracedata pins as GPIOs, // numbered sequentially, so that "gpio X" maps // to qdss_tracedata[X]. These pins are configured // as GPIOs (function 0) and wired externally // to a header on the board, so it's safe to // expose them to the HLOS driver. Package (2) {"gpios", Package () {116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 50, 36, 37, 38, 39}} } }) > I have not seen the Qualcomm server platforms, but I imagine that > customers can alter this list. Not easily, as it requires recompiling the ACPI tables. I also don't know how much control customers have over the XPU settings. The XPU is supposed to block access to any GPIOs that have been muxed to other purposes, but it's a hand-crafted list that could be anything. > I think the numbering of the GPIOs should be that of the TLMM and the > "logical" names should be populated from ACPI by using gpio_chip->names. Is that not what my driver is doing? Or do I misunderstand you? > The question left is how to represent the XPU locked gpios - a problem > we do share with in the mobile TLMM drivers. > > > It seems that if we extend the msm_pingroup with a flag to carry the XPU > lock information and then implement pinmux_ops->request() and deny any > requests on the locked pins, we should cover all[*] the normal GPIO code > paths. Instead of a flag, I was thinking if npins == 0, then this GPIO should be disabled. > [*] Except the reported case of gpiochip_add_data() looping over all > pins and calling get_direction() without first requesting the pin. This is what triggered the problem in the first place. Every time the kernel boots, it's queries all of the GPIOs. Ironically, I created that feature because on server platforms, that was the only way to know the current setting.
On 06/28/2017 02:12 PM, Bjorn Andersson wrote: > It seems that if we extend the msm_pingroup with a flag to carry the XPU > lock information and then implement pinmux_ops->request() and deny any > requests on the locked pins, we should cover all[*] the normal GPIO code > paths. I managed to get this to work, so I'll post a patchset by tomorrow that implements this.
On 06/23/2017 07:33 AM, Linus Walleij wrote: > I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect > the available GPIOs. > > If you haven't tested the GPIO tools, please try it out. Unfortunately, this tool isn't very useful: $ sudo ./lsgpio GPIO chip: gpiochip0, "QCOM8001:00", 150 GPIO lines line 0: unnamed unused [output] line 1: unnamed unused [output] line 2: unnamed unused [output] line 3: unnamed unused [output] line 4: unnamed unused [output] line 5: unnamed unused [output] line 6: unnamed unused [output] line 7: unnamed unused [output] line 8: unnamed unused [output] line 9: unnamed unused [output] line 10: unnamed unused [output] line 11: unnamed unused [output] line 12: unnamed unused [output] line 13: unnamed unused [output] line 14: unnamed unused [output] line 15: unnamed unused [output] line 16: unnamed unused [output] line 17: unnamed unused [output] line 18: unnamed unused [output] line 19: unnamed unused [output] line 20: unnamed unused [output] line 21: unnamed unused [output] line 22: unnamed unused [output] line 23: unnamed unused [output] line 24: unnamed unused [output] line 25: unnamed unused [output] line 26: unnamed unused [output] line 27: unnamed unused [output] line 28: unnamed unused [output] line 29: unnamed unused [output] line 30: unnamed unused [output] line 31: unnamed unused [output] line 32: unnamed unused [output] line 33: unnamed unused [output] line 34: unnamed unused [output] line 35: unnamed unused [output] line 36: unnamed unused line 37: unnamed "sysfs" [kernel] line 38: unnamed unused line 39: unnamed unused line 40: unnamed unused [output] ...
On Thu 29 Jun 12:37 PDT 2017, Timur Tabi wrote: > On 06/23/2017 07:33 AM, Linus Walleij wrote: > > I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect > > the available GPIOs. > > > > If you haven't tested the GPIO tools, please try it out. > > Unfortunately, this tool isn't very useful: > I believe you have to come up with a separate array of names and attach that to the gpio_chip in msm_gpio_init() for the _gpio_ pins to have names. The names we specify in the pin driver is only the name of the pinctrl pin... Regards, Bjorn > $ sudo ./lsgpio > GPIO chip: gpiochip0, "QCOM8001:00", 150 GPIO lines > line 0: unnamed unused [output] > line 1: unnamed unused [output] > line 2: unnamed unused [output] > line 3: unnamed unused [output] > line 4: unnamed unused [output] > line 5: unnamed unused [output] > line 6: unnamed unused [output] > line 7: unnamed unused [output] > line 8: unnamed unused [output] > line 9: unnamed unused [output] > line 10: unnamed unused [output] > line 11: unnamed unused [output] > line 12: unnamed unused [output] > line 13: unnamed unused [output] > line 14: unnamed unused [output] > line 15: unnamed unused [output] > line 16: unnamed unused [output] > line 17: unnamed unused [output] > line 18: unnamed unused [output] > line 19: unnamed unused [output] > line 20: unnamed unused [output] > line 21: unnamed unused [output] > line 22: unnamed unused [output] > line 23: unnamed unused [output] > line 24: unnamed unused [output] > line 25: unnamed unused [output] > line 26: unnamed unused [output] > line 27: unnamed unused [output] > line 28: unnamed unused [output] > line 29: unnamed unused [output] > line 30: unnamed unused [output] > line 31: unnamed unused [output] > line 32: unnamed unused [output] > line 33: unnamed unused [output] > line 34: unnamed unused [output] > line 35: unnamed unused [output] > line 36: unnamed unused > line 37: unnamed "sysfs" [kernel] > line 38: unnamed unused > line 39: unnamed unused > line 40: unnamed unused [output] > ... > > -- > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm > Technologies, Inc. Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project.
On Thu, Jun 29, 2017 at 9:37 PM, Timur Tabi <timur@codeaurora.org> wrote: > On 06/23/2017 07:33 AM, Linus Walleij wrote: >> >> I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect >> the available GPIOs. >> >> If you haven't tested the GPIO tools, please try it out. > > > Unfortunately, this tool isn't very useful: > > $ sudo ./lsgpio > GPIO chip: gpiochip0, "QCOM8001:00", 150 GPIO lines > line 0: unnamed unused [output] > line 1: unnamed unused [output] > line 2: unnamed unused [output] They can be given reasonable names using the line-names property in the DTS file, preferably the top-level file. arch/arm/boot/dts/bcm2835-rpi-b-plus.dts is a good example. Do the same with your target board and things will be very much more helpful. gpio-hammer can help you test a line (try it with a LED) and gpio-event-mon can help you listen at a key for example. Yours, Linus Walleij
diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c index bb3ce5c..983df72 100644 --- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c +++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c @@ -32,9 +32,6 @@ static struct msm_pinctrl_soc_data qdf2xxx_pinctrl; -/* A reasonable limit to the number of GPIOS */ -#define MAX_GPIOS 256 - /* maximum size of each gpio name (enough room for "gpioXXX" + null) */ #define NAME_SIZE 8 @@ -43,22 +40,35 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) struct pinctrl_pin_desc *pins; struct msm_pingroup *groups; char (*names)[NAME_SIZE]; - unsigned int i; - u32 num_gpios; + unsigned int i, num_gpios; + u32 *gpios; int ret; /* Query the number of GPIOs from ACPI */ - ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios); + num_gpios = ret = device_property_read_u32_array(&pdev->dev, "gpios", + NULL, 0); if (ret < 0) { - dev_warn(&pdev->dev, "missing num-gpios property\n"); + dev_err(&pdev->dev, + "missing or invalid 'gpios' property (ret=%i)\n", ret); return ret; } - - if (!num_gpios || num_gpios > MAX_GPIOS) { - dev_warn(&pdev->dev, "invalid num-gpios property\n"); + if (ret == 0) { + dev_warn(&pdev->dev, "no GPIOs defined\n"); return -ENODEV; } + gpios = devm_kcalloc(&pdev->dev, num_gpios, sizeof(u32), GFP_KERNEL); + if (!gpios) + return -ENOMEM; + + ret = device_property_read_u32_array(&pdev->dev, "gpios", gpios, + num_gpios); + if (ret < 0) { + dev_err(&pdev->dev, + "could not read list of GPIOs (ret=%i)\n", ret); + return ret; + } + pins = devm_kcalloc(&pdev->dev, num_gpios, sizeof(struct pinctrl_pin_desc), GFP_KERNEL); groups = devm_kcalloc(&pdev->dev, num_gpios, @@ -69,7 +79,9 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) return -ENOMEM; for (i = 0; i < num_gpios; i++) { - snprintf(names[i], NAME_SIZE, "gpio%u", i); + unsigned int gpio = gpios[i]; + + snprintf(names[i], NAME_SIZE, "gpio%u", gpio); pins[i].number = i; pins[i].name = names[i]; @@ -78,11 +90,11 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) groups[i].name = names[i]; groups[i].pins = &pins[i].number; - groups[i].ctl_reg = 0x10000 * i; - groups[i].io_reg = 0x04 + 0x10000 * i; - groups[i].intr_cfg_reg = 0x08 + 0x10000 * i; - groups[i].intr_status_reg = 0x0c + 0x10000 * i; - groups[i].intr_target_reg = 0x08 + 0x10000 * i; + groups[i].ctl_reg = 0x10000 * gpio; + groups[i].io_reg = 0x04 + 0x10000 * gpio; + groups[i].intr_cfg_reg = 0x08 + 0x10000 * gpio; + groups[i].intr_status_reg = 0x0c + 0x10000 * gpio; + groups[i].intr_target_reg = 0x08 + 0x10000 * gpio; groups[i].mux_bit = 2; groups[i].pull_bit = 0; @@ -100,6 +112,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) groups[i].intr_detection_width = 2; } + devm_kfree(&pdev->dev, gpios); + qdf2xxx_pinctrl.pins = pins; qdf2xxx_pinctrl.groups = groups; qdf2xxx_pinctrl.npins = num_gpios;