[PATCH] pinctrl: qcom: add get_direction function

[PATCH] pinctrl: qcom: add get_direction function

[Timur Tabi]

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;
+}
+
 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,
-- 
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.

Re: [PATCH] pinctrl: qcom: add get_direction function

[Stephen Boyd]

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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] pinctrl: qcom: add get_direction function

[Timur Tabi]

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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH] pinctrl: qcom: add get_direction function

[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

Re: [PATCH] pinctrl: qcom: add get_direction function

[Linus Walleij]

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

Re: [PATCH] pinctrl: qcom: add get_direction function

[Timur Tabi]

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.

-- 
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.

Re: [PATCH] pinctrl: qcom: add get_direction function

[Linus Walleij]

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

Re: [PATCH] pinctrl: qcom: add get_direction function

[Timur Tabi]

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?

-- 
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.

Re: [PATCH] pinctrl: qcom: add get_direction function

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] pinctrl: qcom: add get_direction function

[Timur Tabi]

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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH] pinctrl: qcom: add get_direction function

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] pinctrl: qcom: add get_direction function

[Timur Tabi]

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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH] pinctrl: qcom: add get_direction function

[Bjorn Andersson]

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

Re: [PATCH] pinctrl: qcom: add get_direction function

[Linus Walleij]

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