| Message ID | 1376751410-14560-1-git-send-email-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/17/2013 08:56 AM, Linus Walleij wrote: > We currently defer probing of the caller if a pinctrl GPIO > request or direction setting comes in before the range mapping > a certain GPIO to a certain pin controller is available. > > This can end up with a circular dependency: the GPIO driver > needs the pin controller to be ready So that much is explained above; it's because some GPIO APIs call into pinctrl to manage GPIO-vs-pinmux-function setup. > and the pin controller need the GPIO driver to be ready. Why does that happen? > This also happens if > pin controllers and GPIO controllers compiled as modules > are inserted in a certain order. Shouldn't deferred probe resolve that just fine, assuming there are no circular dependencies? In other words, this is just a special case of the explanation above, so probably not worth explicitly mentioning. ... > On the Nomadik we get this situation with the pinctrl > driver when moving to requesting GPIOs off the gpiochip > right after it has been added, So, the pinctrl driver calls gpio_request()? Surely the solution is simply not to do that?
On 08/17/2013 08:56 AM, Linus Walleij wrote: > We currently defer probing of the caller if a pinctrl GPIO > request or direction setting comes in before the range mapping > a certain GPIO to a certain pin controller is available. > > This can end up with a circular dependency: the GPIO driver > needs the pin controller to be ready and the pin controller > need the GPIO driver to be ready. This also happens if > pin controllers and GPIO controllers compiled as modules > are inserted in a certain order. > > To break this circular dependence, queue any GPIO > operations coming to the framework until the range is ready, > instead of deferring the probe of the caller. I just noticed that next-20130819 is failing on the Tegra114 Dalmore board because of this patch. The kernel log is below. Reverting this patch solves the problem > [ 12.427751] tegra114-pinctrl 70000868.pinmux: pin USB_VBUS_EN0 PN4 already requested by Tegra GPIOs:108; cannot claim for Tegra GPIOs:108 > [ 12.440320] tegra114-pinctrl 70000868.pinmux: pin-108 (Tegra GPIOs:108) status -22 > [ 12.447965] usb1_vbus: Failed to request enable GPIO108: -22 > [ 12.454306] reg-fixed-voltage 3.regulator: Failed to register regulator: -22 > [ 12.461508] reg-fixed-voltage: probe of 3.regulator failed with error -22 > [ 12.469659] tegra114-pinctrl 70000868.pinmux: pin SPDIF_IN PK6 already requested by Tegra GPIOs:86; cannot claim for Tegra GPIOs:86 > [ 12.481606] tegra114-pinctrl 70000868.pinmux: pin-86 (Tegra GPIOs:86) status -22 > [ 12.489052] usb2_vbus: Failed to request enable GPIO86: -22 > [ 12.495226] reg-fixed-voltage 4.regulator: Failed to register regulator: -22 > [ 12.502449] reg-fixed-voltage: probe of 4.regulator failed with error -22 > [ 12.510542] tegra114-pinctrl 70000868.pinmux: pin GMI_CLK PK1 already requested by Tegra GPIOs:81; cannot claim for Tegra GPIOs:81 > [ 12.522403] tegra114-pinctrl 70000868.pinmux: pin-81 (Tegra GPIOs:81) status -22 > [ 12.529896] vdd_hdmi_5v0: Failed to request enable GPIO81: -22 > [ 12.536281] reg-fixed-voltage 5.regulator: Failed to register regulator: -22 > [ 12.543465] reg-fixed-voltage: probe of 5.regulator failed with error -22 > [ 12.552509] vdd_cam_1v8_reg: 1800 mV > [ 12.558205] platform 7d008000.usb: Driver tegra-ehci requests probe deferral > [ 12.566600] platform 7d008000.usb-phy: Driver tegra-phy requests probe deferral > [ 12.577475] input: gpio-keys.2 as /devices/soc0/gpio-keys.2/input/input1 > [ 12.588329] platform 7d008000.usb: Driver tegra-ehci requests probe deferral > [ 12.596967] platform 7d008000.usb-phy: Driver tegra-phy requests probe deferral
On 08/19/2013 01:21 PM, Stephen Warren wrote: > On 08/17/2013 08:56 AM, Linus Walleij wrote: >> We currently defer probing of the caller if a pinctrl GPIO >> request or direction setting comes in before the range mapping >> a certain GPIO to a certain pin controller is available. >> >> This can end up with a circular dependency: the GPIO driver >> needs the pin controller to be ready and the pin controller >> need the GPIO driver to be ready. This also happens if >> pin controllers and GPIO controllers compiled as modules >> are inserted in a certain order. >> >> To break this circular dependence, queue any GPIO >> operations coming to the framework until the range is ready, >> instead of deferring the probe of the caller. > > I just noticed that next-20130819 is failing on the Tegra114 Dalmore > board because of this patch. The kernel log is below. Reverting this > patch solves the problem Linus, any thoughts here? >> [ 12.427751] tegra114-pinctrl 70000868.pinmux: pin USB_VBUS_EN0 PN4 already requested by Tegra GPIOs:108; cannot claim for Tegra GPIOs:108 >> [ 12.440320] tegra114-pinctrl 70000868.pinmux: pin-108 (Tegra GPIOs:108) status -22 >> [ 12.447965] usb1_vbus: Failed to request enable GPIO108: -22 >> [ 12.454306] reg-fixed-voltage 3.regulator: Failed to register regulator: -22 >> [ 12.461508] reg-fixed-voltage: probe of 3.regulator failed with error -22 >> [ 12.469659] tegra114-pinctrl 70000868.pinmux: pin SPDIF_IN PK6 already requested by Tegra GPIOs:86; cannot claim for Tegra GPIOs:86 >> [ 12.481606] tegra114-pinctrl 70000868.pinmux: pin-86 (Tegra GPIOs:86) status -22 >> [ 12.489052] usb2_vbus: Failed to request enable GPIO86: -22 >> [ 12.495226] reg-fixed-voltage 4.regulator: Failed to register regulator: -22 >> [ 12.502449] reg-fixed-voltage: probe of 4.regulator failed with error -22 >> [ 12.510542] tegra114-pinctrl 70000868.pinmux: pin GMI_CLK PK1 already requested by Tegra GPIOs:81; cannot claim for Tegra GPIOs:81 >> [ 12.522403] tegra114-pinctrl 70000868.pinmux: pin-81 (Tegra GPIOs:81) status -22 >> [ 12.529896] vdd_hdmi_5v0: Failed to request enable GPIO81: -22 >> [ 12.536281] reg-fixed-voltage 5.regulator: Failed to register regulator: -22 >> [ 12.543465] reg-fixed-voltage: probe of 5.regulator failed with error -22 >> [ 12.552509] vdd_cam_1v8_reg: 1800 mV >> [ 12.558205] platform 7d008000.usb: Driver tegra-ehci requests probe deferral >> [ 12.566600] platform 7d008000.usb-phy: Driver tegra-phy requests probe deferral >> [ 12.577475] input: gpio-keys.2 as /devices/soc0/gpio-keys.2/input/input1 >> [ 12.588329] platform 7d008000.usb: Driver tegra-ehci requests probe deferral >> [ 12.596967] platform 7d008000.usb-phy: Driver tegra-phy requests probe deferral
On Wed, Aug 21, 2013 at 7:45 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/19/2013 01:21 PM, Stephen Warren wrote: >> On 08/17/2013 08:56 AM, Linus Walleij wrote: >>> We currently defer probing of the caller if a pinctrl GPIO >>> request or direction setting comes in before the range mapping >>> a certain GPIO to a certain pin controller is available. >>> >>> This can end up with a circular dependency: the GPIO driver >>> needs the pin controller to be ready and the pin controller >>> need the GPIO driver to be ready. This also happens if >>> pin controllers and GPIO controllers compiled as modules >>> are inserted in a certain order. >>> >>> To break this circular dependence, queue any GPIO >>> operations coming to the framework until the range is ready, >>> instead of deferring the probe of the caller. >> >> I just noticed that next-20130819 is failing on the Tegra114 Dalmore >> board because of this patch. The kernel log is below. Reverting this >> patch solves the problem > > Linus, any thoughts here? None whatsoever, but logically it should be that your pinctrl+GPIO driver pair depends on the GPIO driver coming first and having it's probing deferred. I'll rip out that patch for the time being... Yours, Linus Walleij
On Mon, Aug 19, 2013 at 9:15 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/17/2013 08:56 AM, Linus Walleij wrote: >> and the pin controller need the GPIO driver to be ready. > > Why does that happen? The pin controller call back into the GPIO-side controller functions by utilizing the GPIO ranges. (Maybe the code is silly, I dunno, check drivers/pinctrl/pinctrl-nomadik.c) >> This also happens if >> pin controllers and GPIO controllers compiled as modules >> are inserted in a certain order. > > Shouldn't deferred probe resolve that just fine, assuming there are no > circular dependencies? The above leads to circular dependencies so that is what I'm trying to fix with this. >> On the Nomadik we get this situation with the pinctrl >> driver when moving to requesting GPIOs off the gpiochip >> right after it has been added, > > So, the pinctrl driver calls gpio_request()? Surely the solution is > simply not to do that? This is what the other patch we're discussing is doing. The one that harvests and requests interrupt GPIO's when a gpiochip is added... Yours, Linus Walleij
On 08/21/2013 05:07 PM, Linus Walleij wrote: > On Mon, Aug 19, 2013 at 9:15 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 08/17/2013 08:56 AM, Linus Walleij wrote: > >>> and the pin controller need the GPIO driver to be ready. >> >> Why does that happen? > > The pin controller call back into the GPIO-side controller > functions by utilizing the GPIO ranges. > (Maybe the code is silly, I dunno, check drivers/pinctrl/pinctrl-nomadik.c) > >>> This also happens if >>> pin controllers and GPIO controllers compiled as modules >>> are inserted in a certain order. >> >> Shouldn't deferred probe resolve that just fine, assuming there are no >> circular dependencies? > > The above leads to circular dependencies so that is what I'm > trying to fix with this. But the circular dependencies are imposed by a specific driver, not by the GPIO and pinctrl subsystems. gpiolib operations call into pinctrl to acquire pin ownership. Hence, no pinctrl operation (or implementation in a driver) can call into gpiolib, or a GPIO driver. Shouldn't this simply be fixed inside whichever driver is performing circular operations, rather than leaving the driver performing circular operations, and attemping to make the subsystems support something that can't be supported? >>> On the Nomadik we get this situation with the pinctrl >>> driver when moving to requesting GPIOs off the gpiochip >>> right after it has been added, >> >> So, the pinctrl driver calls gpio_request()? Surely the solution is >> simply not to do that? > > This is what the other patch we're discussing is doing. > The one that harvests and requests interrupt GPIO's when > a gpiochip is added... It would have been good to mention the two patches were related, or did I just miss that? So that's another reason for me to object to that other patch. If that whole process was triggered inside the IRQ controller implementation, which is logically part of the GPIO controller implementation for the same HW, the call direction would also be GPIO -> pinctrl or IRQ -> GPIO -> pinctrl, and hence have no circular issues.
On Thu, Aug 22, 2013 at 1:23 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > Shouldn't this simply be fixed inside whichever driver is performing > circular operations, rather than leaving the driver performing circular > operations, and attemping to make the subsystems support something that > can't be supported? It was part of that series, but I thought about it and saw that it'd be nice if the GPIO probing would not have to defer due to the pin controller not being up yet anyway. I'll just reword the subject like that... like some nice optimization. Actually I think we should be able to register GPIO ranges before the backing pin controller is up as well for the same reason, letting such things queue up will make the boot more optimal in some cases and get people away from playing around with initcall levels. >> This is what the other patch we're discussing is doing. >> The one that harvests and requests interrupt GPIO's when >> a gpiochip is added... > > It would have been good to mention the two patches were related, or did > I just miss that? In the first posting it was. I need to refine the motivation per above instead. > So that's another reason for me to object to that other patch. If that > whole process was triggered inside the IRQ controller implementation, > which is logically part of the GPIO controller implementation for the > same HW, the call direction would also be GPIO -> pinctrl or IRQ -> GPIO > -> pinctrl, and hence have no circular issues. Hm not quite following, but it was not triggered from the IRQ controller. It was triggered from the gpiolib-of.c trying to do gpio_request() and gpio_set_input() right after registering a GPIO controller. (Which should be possible, it's a bug in its own right that this is not possible on the Nomadik driver...) Yours, Linus Walleij
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index a97b717..41c7479 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -404,7 +404,111 @@ static int pinctrl_get_device_gpio_range(unsigned gpio, } } - return -EPROBE_DEFER; + return -EINVAL; +} + +/** + * enum pinctrl_gpio_operation - queued operations type for GPIOs + * @PINCTRL_GPIO_REQUEST: queued request operation + * @PINCTRL_SET_DIR_OUT: queued set as output operation + * @PINCTRL_SET_DIR_IN: queued set as input operation + */ +enum pinctrl_gpio_operation { + PINCTRL_GPIO_REQUEST, + PINCTRL_GPIO_DIR_OUT, + PINCTRL_GPIO_DIR_IN, +}; + +/** + * struct pinctrl_gpio_op - a queue list holder + * @node: a list node for list processing + * @gpio: the targeted gpio for this operation + * @request: true of this is a request, then the gpio will be requested + * @input: if this is not a request, we're setting the direction + * Queued GPIO range-based deferred operations are stored in this list. + */ +struct pinctrl_gpio_op { + struct list_head node; + int gpio; + enum pinctrl_gpio_operation op; +}; + +static LIST_HEAD(pinctrl_queued_gpio_ops); + +static int pinctrl_queue_gpio_operation(int gpio, + enum pinctrl_gpio_operation op) +{ + struct pinctrl_gpio_op *qop; + + /* + * We get to this point if pinctrl_*_gpio() is called + * from a GPIO driver which does not yet have a registered + * pinctrl driver backend, and thus no ranges are defined for + * it. This could happen during system start up or if we're + * probing pin controllers as modules. Queue the request and + * handle it when and if the range arrives. + */ + qop = kzalloc(sizeof(struct pinctrl_gpio_op), GFP_KERNEL); + if (!qop) + return -ENOMEM; + qop->gpio = gpio; + qop->op = op; + list_add_tail(&qop->node, &pinctrl_queued_gpio_ops); + pr_info("queueing pinctrl request for GPIO %d\n", qop->gpio); + return 0; +} + + +/** + * pinctrl_process_queued_gpio_ops() - process queued GPIO operations + * + * This is called whenever a new GPIO range is added to see if some GPIO + * driver has outstanding operations to GPIOs in the range, and then these + * get processed at this point. + */ +static void pinctrl_process_queued_gpio_ops(void) +{ + struct list_head *node, *tmp; + + list_for_each_safe(node, tmp, &pinctrl_queued_gpio_ops) { + struct pinctrl_gpio_op *op = + list_entry(node, struct pinctrl_gpio_op, node); + struct pinctrl_dev *pctldev; + struct pinctrl_gpio_range *range; + int pin; + int ret; + + ret = pinctrl_get_device_gpio_range(op->gpio, &pctldev, &range); + if (ret) + continue; + + /* Convert to the pin controllers number space */ + pin = gpio_to_pin(range, op->gpio); + + switch(op->op) { + case PINCTRL_GPIO_REQUEST: + ret = pinmux_request_gpio(pctldev, range, pin, op->gpio); + if (ret) + pr_err("failed to request queued GPIO %d\n", + op->gpio); + else + pr_info("requested queued GPIO %d\n", op->gpio); + break; + case PINCTRL_GPIO_DIR_IN: + case PINCTRL_GPIO_DIR_OUT: + ret = pinmux_gpio_direction(pctldev, range, pin, + op->op == PINCTRL_GPIO_DIR_IN); + if (ret) + pr_err("failed to set direction on queued GPIO %d\n", + op->gpio); + break; + default: + pr_err("unknown queued GPIO operation\n"); + break; + } + list_del(node); + kfree(op); + } } /** @@ -421,6 +525,8 @@ void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev, mutex_lock(&pctldev->mutex); list_add_tail(&range->node, &pctldev->gpio_ranges); mutex_unlock(&pctldev->mutex); + /* Maybe we have outstanding GPIO requests for this range? */ + pinctrl_process_queued_gpio_ops(); } EXPORT_SYMBOL_GPL(pinctrl_add_gpio_range); @@ -552,8 +658,8 @@ int pinctrl_request_gpio(unsigned gpio) ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); if (ret) { if (pinctrl_ready_for_gpio_range(gpio)) - ret = 0; - return ret; + return 0; + return pinctrl_queue_gpio_operation(gpio, PINCTRL_GPIO_REQUEST); } /* Convert to the pin controllers number space */ @@ -604,7 +710,16 @@ static int pinctrl_gpio_direction(unsigned gpio, bool input) ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); if (ret) { - return ret; + /* Maybe this pin does not have a pinctrl back-end at all? */ + if (pinctrl_ready_for_gpio_range(gpio)) + return 0; + /* Que the operation until the range arrives */ + if (input) + return pinctrl_queue_gpio_operation(gpio, + PINCTRL_GPIO_DIR_IN); + else + return pinctrl_queue_gpio_operation(gpio, + PINCTRL_GPIO_DIR_OUT); } mutex_lock(&pctldev->mutex);
We currently defer probing of the caller if a pinctrl GPIO request or direction setting comes in before the range mapping a certain GPIO to a certain pin controller is available. This can end up with a circular dependency: the GPIO driver needs the pin controller to be ready and the pin controller need the GPIO driver to be ready. This also happens if pin controllers and GPIO controllers compiled as modules are inserted in a certain order. To break this circular dependence, queue any GPIO operations coming to the framework until the range is ready, instead of deferring the probe of the caller. On the Nomadik we get this situation with the pinctrl driver when moving to requesting GPIOs off the gpiochip right after it has been added, and with this patch the boot dilemma is sorted out nicely, as can be seen in this condensed bootlog: pinctrl core: initialized pinctrl subsystem gpio 101e4000.gpio: at address cc852000 gpio 101e5000.gpio: at address cc854000 gpio 101e6000.gpio: at address cc856000 pinctrl core: queueing pinctrl request for GPIO 104 gpio 101e7000.gpio: at address cc858000 pinctrl core: requested queued GPIO 104 pinctrl-nomadik pinctrl.0: initialized Nomadik pin control driver ChangeLog v1->v2: - Apart from queueing requests, also queue direction setting operations. Cc: Haojian Zhuang <haojian.zhuang@linaro.org> Cc: Lars Poeschel <poeschel@lemonage.de> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/pinctrl/core.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 119 insertions(+), 4 deletions(-)