| Message ID | 1486768860-18237-1-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/10/2017 03:21 PM, Timur Tabi wrote: > The get_direction callback function allows gpiolib to know the current > direction (input vs output) for a given GPIO. > > This is particularly useful on ACPI systems, where the GPIOs are > configured only by firmware (typically UEFI), so the only way to > know the initial values to query the hardware directly. Without > this function, gpiolib thinks that all GPIOs are configured for > input. > > Signed-off-by: Timur Tabi <timur@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index f8e9e1c..c978be5 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -422,6 +422,20 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in > return 0; > } > > +static int msm_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) > +{ > + struct msm_pinctrl *pctrl = gpiochip_get_data(chip); > + const struct msm_pingroup *g; > + u32 val; > + > + g = &pctrl->soc->groups[offset]; > + > + val = readl(pctrl->regs + g->ctl_reg); > + > + /* 0 = output, 1 = input */ > + return val & BIT(g->oe_bit) ? 0 : 1; Maybe we can use the macros GPIOF_DIR_IN and GPIOF_DIR_OUT here instead?
Stephen Boyd wrote: >> > + /* 0 = output, 1 = input */ >> > + return val & BIT(g->oe_bit) ? 0 : 1; > Maybe we can use the macros GPIOF_DIR_IN and GPIOF_DIR_OUT here instead? Ok, I posted a v2. Is it too late for this patch to make the 4.11 merge window, or maybe 4.11-rc2? From the perspective of our server platform, it's a bug fix.
On Sat, Feb 11, 2017 at 12:25 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 02/10/2017 03:21 PM, Timur Tabi wrote: >> + return val & BIT(g->oe_bit) ? 0 : 1; > > Maybe we can use the macros GPIOF_DIR_IN and GPIOF_DIR_OUT here instead? Actually no. The GPIOF* #defines are for the consumer side of GPIO, not the driver side. We actually just use 0/1 in drivers. So I merged this v1 version. Yours, Linus Walleij
On Sat, Feb 11, 2017 at 10:32 PM, Timur Tabi <timur@codeaurora.org> wrote: > Is it too late for this patch to make the 4.11 merge window, or maybe > 4.11-rc2? From the perspective of our server platform, it's a bug fix. Not too late when you put it like that :D I've merged it for fixes. Yours, Linus Walleij
On 02/22/2017 09:49 AM, Linus Walleij wrote: > Actually no. The GPIOF* #defines are for the consumer side of GPIO, > not the driver side. We actually just use 0/1 in drivers. > > So I merged this v1 version. Thanks. So I have a follow-up question. On ACPI platforms, the kernel has no control over the muxing (aka function) of the various pins. Firmware configures the TLMM controller for all pins, and configures them for whatever functions they're supposed to be. You can see an example of this in pinctrl-qdf2xxx.c. As you can see, each group consists of 1 pin, and there are 0 functions defined. Function 0 is plain gpio I/O. The other functions (1-7, typically) are muxes for various devices, like UART TX, etc. Therefore, on ACPI, the driver should never change the function of any pin. If it's set to something other than 0, then it should never touch that pin. Don't write to it, don't change the direction, and definitely don't change the function. So would it be acceptable, for example, to change msm_gpio_set() such that if the function of that pin is non-zero, just return an error? After all, if pin #17 is set to UART and not GPIO, then there's no point in setting that value to 1 or 0, because it's not muxed for GPIO and therefore, that 1/0 is not actually going anywhere. It won't be written to the pin. I hope I'm making sense.
On Mon, Mar 6, 2017 at 10:52 PM, Timur Tabi <timur@codeaurora.org> wrote: > On ACPI platforms, the kernel has no control over the muxing (aka function) > of the various pins. Firmware configures the TLMM controller for all pins, > and configures them for whatever functions they're supposed to be. I think it would be better if pin control and ACPI play along, and I bet that will happen in the future. This is I guess a question for ACPI standardization work. > Therefore, on ACPI, the driver should never change the function of any pin. > If it's set to something other than 0, then it should never touch that pin. > Don't write to it, don't change the direction, and definitely don't change > the function. Does that mean that pins with 0 are free to play around with? > So would it be acceptable, for example, to change msm_gpio_set() such that > if the function of that pin is non-zero, just return an error? I would ask the driver maintainer about his opinion, and also whoever is an authority on ACPI for the TLMM-chips, I am no expert in ACPI. Hell I'm not even good at device tree. Not to mention SFI. Oh well... > After all, if pin #17 is set to UART and not GPIO, then there's no point in > setting that value to 1 or 0, because it's not muxed for GPIO and therefore, > that 1/0 is not actually going anywhere. It won't be written to the pin. > > I hope I'm making sense. In a way I guess, I'm just ignorant about how current ACPI implementations work with hardware in this case so it is likely that you know this better than me. Yours, Linus Walleij
On 03/14/2017 04:41 PM, Linus Walleij wrote: > On Mon, Mar 6, 2017 at 10:52 PM, Timur Tabi <timur@codeaurora.org> wrote: > >> On ACPI platforms, the kernel has no control over the muxing (aka function) >> of the various pins. Firmware configures the TLMM controller for all pins, >> and configures them for whatever functions they're supposed to be. > > I think it would be better if pin control and ACPI play along, and I bet that > will happen in the future. This is I guess a question for ACPI standardization > work. Maybe. During the development of the ACPI standard, everyone made a big stink about how muxing should be handled by the firmware and never touched by the OS. It would be a significant reversal if they decide to add mux configuration to ACPI. >> Therefore, on ACPI, the driver should never change the function of any pin. >> If it's set to something other than 0, then it should never touch that pin. >> Don't write to it, don't change the direction, and definitely don't change >> the function. > > Does that mean that pins with 0 are free to play around with? Yes. >> So would it be acceptable, for example, to change msm_gpio_set() such that >> if the function of that pin is non-zero, just return an error? > > I would ask the driver maintainer about his opinion, and also whoever > is an authority on ACPI for the TLMM-chips, I am no expert > in ACPI. Hell I'm not even good at device tree. Not to mention SFI. > Oh well... Well, I was hoping that Stephen would respond to this question. :-) My point is, if the driver knows that the GPIO cannot be written to (because it's muxed to something else), shouldn't the driver return with an error if the caller attempts to write?
On 03/14, Timur Tabi wrote: > On 03/14/2017 04:41 PM, Linus Walleij wrote: > > On Mon, Mar 6, 2017 at 10:52 PM, Timur Tabi <timur@codeaurora.org> wrote: > > >> So would it be acceptable, for example, to change msm_gpio_set() such that > >> if the function of that pin is non-zero, just return an error? > > > > I would ask the driver maintainer about his opinion, and also whoever > > is an authority on ACPI for the TLMM-chips, I am no expert > > in ACPI. Hell I'm not even good at device tree. Not to mention SFI. > > Oh well... > > Well, I was hoping that Stephen would respond to this question. :-) > > My point is, if the driver knows that the GPIO cannot be written to (because > it's muxed to something else), shouldn't the driver return with an error if the > caller attempts to write? > (I reply faster when my name is written!) I don't see any problem with failing msm_gpio_set() when the function is "not gpio", but I also wonder why it matters. Drivers shouldn't be doing that, because if the gpio is muxed to some other functionality they shouldn't be treating it as a gpio in the first place. Perhaps we can have some sort of gpio validation debug option that the check goes under. Then we could fail and print a big warning if this happens, but if we aren't debugging then we don't do any checking and rely on drivers to do the right thing.
Stephen Boyd wrote: > I don't see any problem with failing msm_gpio_set() when the > function is "not gpio", but I also wonder why it matters. Drivers > shouldn't be doing that, because if the gpio is muxed to some > other functionality they shouldn't be treating it as a gpio in > the first place. The idea is to notify drivers with an error code when they make a mistake. Perhaps the device tree or the ACPI table has an error? > Perhaps we can have some sort of gpio validation debug option > that the check goes under. Then we could fail and print a big > warning if this happens, but if we aren't debugging then we don't > do any checking and rely on drivers to do the right thing. I could add that, but I still think returning an error code is appropriate. On the TLMM, we know for sure that the pin must be set to function 0 in order for the read/write routines to operate correctly. I guess I should propose a patch and we can vote on it.
On 03/14, Timur Tabi wrote: > Stephen Boyd wrote: > >I don't see any problem with failing msm_gpio_set() when the > >function is "not gpio", but I also wonder why it matters. Drivers > >shouldn't be doing that, because if the gpio is muxed to some > >other functionality they shouldn't be treating it as a gpio in > >the first place. > > The idea is to notify drivers with an error code when they make a > mistake. Perhaps the device tree or the ACPI table has an error? In general the kernel isn't a firmware validator. At least that's the way I view it. Others may have different opinions here. > > >Perhaps we can have some sort of gpio validation debug option > >that the check goes under. Then we could fail and print a big > >warning if this happens, but if we aren't debugging then we don't > >do any checking and rely on drivers to do the right thing. > > I could add that, but I still think returning an error code is > appropriate. On the TLMM, we know for sure that the pin must be set > to function 0 in order for the read/write routines to operate > correctly. On ACPI we could make the gpio_get() path fail if the pin isn't in GPIO mode? That would work assuming ACPI can't change the pin muxing at runtime. On DT we might have runtime muxing though so I don't see how it would work there.
Stephen Boyd wrote: >> > The idea is to notify drivers with an error code when they make a >> > mistake. Perhaps the device tree or the ACPI table has an error? > In general the kernel isn't a firmware validator. At least that's > the way I view it. Others may have different opinions here. I would be okay with wrapping that check around #ifdef CONFIG_DEBUG_GPIO. >> > I could add that, but I still think returning an error code is >> > appropriate. On the TLMM, we know for sure that the pin must be set >> > to function 0 in order for the read/write routines to operate >> > correctly. > On ACPI we could make the gpio_get() path fail if the pin isn't > in GPIO mode? Did you mean the gpio_chip.request callback? Currently that points to gpiochip_generic_request in pinctrl-msm.c.
On Tue 14 Mar 17:12 PDT 2017, Timur Tabi wrote: > Stephen Boyd wrote: > > > > > The idea is to notify drivers with an error code when they make a > > > > mistake. Perhaps the device tree or the ACPI table has an error? > > > In general the kernel isn't a firmware validator. At least that's > > the way I view it. Others may have different opinions here. > > I would be okay with wrapping that check around #ifdef CONFIG_DEBUG_GPIO. > This will, more or less, only be useful for system integrators - whom likely won't enable DEBUG_GPIO. But I'm generally fine with failing gpio_set if we're not in function 0. My question is if we should wrap that check in a WARN(), just to make it easy for said system integrator to spot the issue - or if that will just leave another chunk of printouts that people will ignore in their products. > > > > I could add that, but I still think returning an error code is > > > > appropriate. On the TLMM, we know for sure that the pin must be set > > > > to function 0 in order for the read/write routines to operate > > > > correctly. > > > On ACPI we could make the gpio_get() path fail if the pin isn't > > in GPIO mode? > > Did you mean the gpio_chip.request callback? Currently that points to > gpiochip_generic_request in pinctrl-msm.c. > It might be useful to fail gpio_get, as that gives a much nicer error path in the client. But I'm slightly concerned about the few cases where one of the pinmux states is gpio and that this would force the gpio_get() only to be called after switching to that particular mode. @Linus, have there been any discussion around this in other drivers? Regards, Bjorn
On Wed, Mar 15, 2017 at 6:08 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 14 Mar 17:12 PDT 2017, Timur Tabi wrote: >> Did you mean the gpio_chip.request callback? Currently that points to >> gpiochip_generic_request in pinctrl-msm.c. > > It might be useful to fail gpio_get, as that gives a much nicer error > path in the client. But I'm slightly concerned about the few cases where > one of the pinmux states is gpio and that this would force the > gpio_get() only to be called after switching to that particular mode. > > @Linus, have there been any discussion around this in other drivers? Not more than the obvious, this driver has: .request = gpiochip_generic_request, .free = gpiochip_generic_free, Which will call: pinctrl_request_gpio() pinctrl_free_gpio() (Note: we now also have gpiochip_generic_config() and pinctrl_gpio_set_config(), you should maybe want to look into this because it makes it possible for the pin control back-end to support debouncing and open drain/source from the gpiolib side.) Anyways in some drivers (not pinctrl-msm.c) this ends up calling this helper callback in pinmux_ops: * @gpio_request_enable: requests and enables GPIO on a certain pin. * Implement this only if you can mux every pin individually as GPIO. The * affected GPIO range is passed along with an offset(pin number) into that * specific GPIO range - function selectors and pin groups are orthogonal * to this, the core will however make sure the pins do not collide. If you do not have this kind of BIOS/ACPI playing around with the pins behind your back, then this is really helpful to avoid having to (in the DTS) mux all these pins to the GPIO function explicitly like this for example: /* Interrupt line for the KXSD9 accelerometer */ dragon_kxsd9_gpios: kxsd9 { irq { pins = "gpio57"; /* IRQ line */ bias-pull-up; }; }; Well, in this case, since we also need to set up a pull-up the DT node is needed anyways, but you get the idea. So we have a bunch of fit-all callbacks but the subsystem was constructed for a scenario where the kernel has full control over the pins. In the begínning ACPI people were saying they simply should not devise any pin control driver at all, because ACPI would handle all muxing behind the scenes. It turns out they were mostly too ambitious about that, one usecase is power optimizations that are not readily understood when writing the BIOS, other reasons are when you are building embedded systems and randomly adding some extra peripherals, in theory every vendor should ideally write a new ACPI table and reflash the BIOS but it appears that in practice they do not, so I think most of that hardware has some hybrid model. I would ask the Intel people about their position of ACPI and pin control interwork, they have most experience in this. If you git-blame drivers/gpio/gpiolib-acpi.c you can see that it is mostly written by Mika and Rafael. If you look in intel_pinmux_set_mux() you find that they do avoid muxing some pins: /* * All pins in the groups needs to be accessible and writable * before we can enable the mux for this group. */ for (i = 0; i < grp->npins; i++) { if (!intel_pad_usable(pctrl, grp->pins[i])) { raw_spin_unlock_irqrestore(&pctrl->lock, flags); return -EBUSY; } } Yours, Linus Walleij
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index f8e9e1c..c978be5 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -422,6 +422,20 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in return 0; } +static int msm_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) +{ + struct msm_pinctrl *pctrl = gpiochip_get_data(chip); + const struct msm_pingroup *g; + u32 val; + + g = &pctrl->soc->groups[offset]; + + val = readl(pctrl->regs + g->ctl_reg); + + /* 0 = output, 1 = input */ + return val & BIT(g->oe_bit) ? 0 : 1; +} + static int msm_gpio_get(struct gpio_chip *chip, unsigned offset) { const struct msm_pingroup *g; @@ -510,6 +524,7 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) static 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,
The get_direction callback function allows gpiolib to know the current direction (input vs output) for a given GPIO. This is particularly useful on ACPI systems, where the GPIOs are configured only by firmware (typically UEFI), so the only way to know the initial values to query the hardware directly. Without this function, gpiolib thinks that all GPIOs are configured for input. Signed-off-by: Timur Tabi <timur@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)