linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
@ 2019-12-02 12:36 Stanimir, Vasile-Laurentiu
  2019-12-02 13:05 ` andriy.shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Stanimir, Vasile-Laurentiu @ 2019-12-02 12:36 UTC (permalink / raw)
  To: linux-gpio; +Cc: linus.walleij, mika.westerberg, andriy.shevchenko

From f8093f2c73c636b75fcf4dee4178af0e24c2f878 Mon Sep 17 00:00:00 2001
From: Vasile-Laurentiu Stanimir <vasile-laurentiu.stanimir@windriver.com>
Date: Mon, 2 Dec 2019 14:20:11 +0200
Subject: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based
 on pullup and polarity

ACPI GPIO resources don't contain an initial value for the
GPIO. Therefore instead of deducting its value based on pullup field
we should deduce that value from the polarity and the pull field.
Typical scenario is when ACPI is defined in acpi-table and its polarity
is defined as ACTIVE-LOW in the following call:

acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
  acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)

it will return GPIOD_OUT_HIGH if pull_up is set no matter if
polarity is GPIO_ACTIVE_LOW, so it will return the current level instead
of the logical level.

Signed-off-by: Vasile-Laurentiu Stanimir <vasile-laurentiu.stanimir@windriver.com>
---
 drivers/gpio/gpiolib-acpi.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index a3e43cacd78e..a3e327d05e26 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -420,7 +420,7 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 }
 
 static enum gpiod_flags
-acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
+acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio, int polarity)
 {
 	bool pull_up = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;
 
@@ -431,9 +431,16 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
 		/*
 		 * ACPI GPIO resources don't contain an initial value for the
 		 * GPIO. Therefore we deduce that value from the pull field
-		 * instead. If the pin is pulled up we assume default to be
-		 * high, otherwise low.
+		 * and the polarity instead.
+		 * By default if the pin is pulled up we assume default to be
+		 * high, otherwise low. For ACTIVE LOW polarity values will be
+		 * switched.
 		 */
+		if (agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_IO &&
+		    polarity == GPIO_ACTIVE_LOW) {
+			return pull_up ? GPIOD_OUT_LOW : GPIOD_OUT_HIGH;
+		}
+
 		return pull_up ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
 	default:
 		/*
@@ -532,8 +539,9 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 			lookup->info.polarity = agpio->polarity;
 			lookup->info.triggering = agpio->triggering;
 		} else {
-			lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio);
 			lookup->info.polarity = lookup->active_low;
+			lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio,
+							lookup->info.polarity);
 		}
 	}
 
@@ -881,7 +889,8 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		}
 
 		if (!found) {
-			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio);
+			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio,
+							agpio->polarity);
 			const char *label = "ACPI:OpRegion";
 			int err;
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
  2019-12-02 12:36 [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity Stanimir, Vasile-Laurentiu
@ 2019-12-02 13:05 ` andriy.shevchenko
  2019-12-02 13:44   ` Stanimir, Vasile-Laurentiu
  0 siblings, 1 reply; 7+ messages in thread
From: andriy.shevchenko @ 2019-12-02 13:05 UTC (permalink / raw)
  To: Stanimir, Vasile-Laurentiu; +Cc: linux-gpio, linus.walleij, mika.westerberg

On Mon, Dec 02, 2019 at 12:36:47PM +0000, Stanimir, Vasile-Laurentiu wrote:
> From f8093f2c73c636b75fcf4dee4178af0e24c2f878 Mon Sep 17 00:00:00 2001
> From: Vasile-Laurentiu Stanimir <vasile-laurentiu.stanimir@windriver.com>
> Date: Mon, 2 Dec 2019 14:20:11 +0200
> Subject: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based
>  on pullup and polarity
> 
> ACPI GPIO resources don't contain an initial value for the
> GPIO. Therefore instead of deducting its value based on pullup field
> we should deduce that value from the polarity and the pull field.
> Typical scenario is when ACPI is defined in acpi-table and its polarity
> is defined as ACTIVE-LOW in the following call:
> 
> acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
>   acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
> 
> it will return GPIOD_OUT_HIGH if pull_up is set no matter if
> polarity is GPIO_ACTIVE_LOW, so it will return the current level instead
> of the logical level.

Thank you for the patch.

I have question in general. If we have Active Low polarity and Pull Down,
isn't it simple a bad ACPI table and rather quirk is needed here?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
  2019-12-02 13:05 ` andriy.shevchenko
@ 2019-12-02 13:44   ` Stanimir, Vasile-Laurentiu
  2019-12-02 14:08     ` andriy.shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Stanimir, Vasile-Laurentiu @ 2019-12-02 13:44 UTC (permalink / raw)
  To: andriy.shevchenko; +Cc: linux-gpio, linus.walleij, mika.westerberg

________________________________________
From: andriy.shevchenko@linux.intel.com [andriy.shevchenko@linux.intel.com]
Sent: Monday, December 02, 2019 3:05 PM
To: Stanimir, Vasile-Laurentiu
Cc: linux-gpio@vger.kernel.org; linus.walleij@linaro.org; mika.westerberg@linux.intel.com
Subject: Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity

On Mon, Dec 02, 2019 at 12:36:47PM +0000, Stanimir, Vasile-Laurentiu wrote:
> From f8093f2c73c636b75fcf4dee4178af0e24c2f878 Mon Sep 17 00:00:00 2001
> From: Vasile-Laurentiu Stanimir <vasile-laurentiu.stanimir@windriver.com>
> Date: Mon, 2 Dec 2019 14:20:11 +0200
> Subject: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based
>  on pullup and polarity
>
> ACPI GPIO resources don't contain an initial value for the
> GPIO. Therefore instead of deducting its value based on pullup field
> we should deduce that value from the polarity and the pull field.
> Typical scenario is when ACPI is defined in acpi-table and its polarity
> is defined as ACTIVE-LOW in the following call:
>
> acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
>   acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
>
> it will return GPIOD_OUT_HIGH if pull_up is set no matter if
> polarity is GPIO_ACTIVE_LOW, so it will return the current level instead
> of the logical level.

Thank you for the patch.

I have question in general. If we have Active Low polarity and Pull Down,
isn't it simple a bad ACPI table and rather quirk is needed here?

--
With Best Regards,
Andy Shevchenko


Hi, 

It may be, also it may be a bad hardware design but it is also a possible situation.

In our case here we have an FPGA whose pcie link is held in reset by BIOS during 
boot, the reset pin is active low, and its configuration is specified in the Acpi DSDT 
table. When Linux starts, our userspace driver shall load the FPGA, and the first 
step is to request all GPIO's needed to configure the pcie phy on the FPGA side; 
the pcie link reset should be held active while this configuration (loading of the 
Altera fpp image) is ongoing.
Now all active low pins have their initial value inverted by the kernel. This means 
that the pcie link reset is briefly released, which generates a pcie hot unplug event, 
which in turn delays start of a successful loading sequence, and so SW has to make 
a second reload attempt which will delay too much the normal boot sequence.

Sorry for giving you too much details which probability are not important but I only
wanted to emphasise that it may be a real situation whether or not it is a good
design.

Regards,
 vls

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
  2019-12-02 13:44   ` Stanimir, Vasile-Laurentiu
@ 2019-12-02 14:08     ` andriy.shevchenko
  2019-12-03 15:24       ` Stanimir, Vasile-Laurentiu
  0 siblings, 1 reply; 7+ messages in thread
From: andriy.shevchenko @ 2019-12-02 14:08 UTC (permalink / raw)
  To: Stanimir, Vasile-Laurentiu; +Cc: linux-gpio, linus.walleij, mika.westerberg

On Mon, Dec 02, 2019 at 01:44:04PM +0000, Stanimir, Vasile-Laurentiu wrote:
> ________________________________________
> From: andriy.shevchenko@linux.intel.com [andriy.shevchenko@linux.intel.com]
> Sent: Monday, December 02, 2019 3:05 PM
> To: Stanimir, Vasile-Laurentiu
> Cc: linux-gpio@vger.kernel.org; linus.walleij@linaro.org; mika.westerberg@linux.intel.com
> Subject: Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
> 
> On Mon, Dec 02, 2019 at 12:36:47PM +0000, Stanimir, Vasile-Laurentiu wrote:
> > From f8093f2c73c636b75fcf4dee4178af0e24c2f878 Mon Sep 17 00:00:00 2001
> > From: Vasile-Laurentiu Stanimir <vasile-laurentiu.stanimir@windriver.com>
> > Date: Mon, 2 Dec 2019 14:20:11 +0200
> > Subject: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based
> >  on pullup and polarity
> >
> > ACPI GPIO resources don't contain an initial value for the
> > GPIO. Therefore instead of deducting its value based on pullup field
> > we should deduce that value from the polarity and the pull field.
> > Typical scenario is when ACPI is defined in acpi-table and its polarity
> > is defined as ACTIVE-LOW in the following call:
> >
> > acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
> >   acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
> >
> > it will return GPIOD_OUT_HIGH if pull_up is set no matter if
> > polarity is GPIO_ACTIVE_LOW, so it will return the current level instead
> > of the logical level.
> 
> Thank you for the patch.
> 
> I have question in general. If we have Active Low polarity and Pull Down,
> isn't it simple a bad ACPI table and rather quirk is needed here?
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
> Hi, 
> 
> It may be, also it may be a bad hardware design but it is also a possible situation.
> 
> In our case here we have an FPGA whose pcie link is held in reset by BIOS during 
> boot, the reset pin is active low, and its configuration is specified in the Acpi DSDT 
> table. When Linux starts, our userspace driver shall load the FPGA, and the first 
> step is to request all GPIO's needed to configure the pcie phy on the FPGA side; 
> the pcie link reset should be held active while this configuration (loading of the 
> Altera fpp image) is ongoing.
> Now all active low pins have their initial value inverted by the kernel. This means 
> that the pcie link reset is briefly released, which generates a pcie hot unplug event, 
> which in turn delays start of a successful loading sequence, and so SW has to make 
> a second reload attempt which will delay too much the normal boot sequence.
> 
> Sorry for giving you too much details which probability are not important but I only
> wanted to emphasise that it may be a real situation whether or not it is a good
> design.

No need to sorry, the details are useful as well as knowing your use case.

Q1: who is providing DSDT?
Q2: why it can't be fixed?
Q3: how the GPIO lines are being requested?
Q4: how are they described, btw, in DSDT?

Side note, if it will be no other choice left, the above code still can be
added as a quirk (see code related to ACPI_GPIO_QUIRK_NO_IO_RESTRICTION
and ACPI_GPIO_QUIRK_ONLY_GPIOIO).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
  2019-12-02 14:08     ` andriy.shevchenko
@ 2019-12-03 15:24       ` Stanimir, Vasile-Laurentiu
  2019-12-04 14:08         ` andriy.shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Stanimir, Vasile-Laurentiu @ 2019-12-03 15:24 UTC (permalink / raw)
  To: andriy.shevchenko; +Cc: linux-gpio, linus.walleij, mika.westerberg


________________________________________
From: andriy.shevchenko@linux.intel.com [andriy.shevchenko@linux.intel.com]
Sent: Monday, December 02, 2019 4:08 PM
To: Stanimir, Vasile-Laurentiu
Cc: linux-gpio@vger.kernel.org; linus.walleij@linaro.org; mika.westerberg@linux.intel.com
Subject: Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity

On Mon, Dec 02, 2019 at 01:44:04PM +0000, Stanimir, Vasile-Laurentiu wrote:
> ________________________________________
> From: andriy.shevchenko@linux.intel.com [andriy.shevchenko@linux.intel.com]
> Sent: Monday, December 02, 2019 3:05 PM
> To: Stanimir, Vasile-Laurentiu
> Cc: linux-gpio@vger.kernel.org; linus.walleij@linaro.org; mika.westerberg@linux.intel.com
> Subject: Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
>
> On Mon, Dec 02, 2019 at 12:36:47PM +0000, Stanimir, Vasile-Laurentiu wrote:
> > From f8093f2c73c636b75fcf4dee4178af0e24c2f878 Mon Sep 17 00:00:00 2001
> > From: Vasile-Laurentiu Stanimir <vasile-laurentiu.stanimir@windriver.com>
> > Date: Mon, 2 Dec 2019 14:20:11 +0200
> > Subject: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based
> >  on pullup and polarity
> >
> > ACPI GPIO resources don't contain an initial value for the
> > GPIO. Therefore instead of deducting its value based on pullup field
> > we should deduce that value from the polarity and the pull field.
> > Typical scenario is when ACPI is defined in acpi-table and its polarity
> > is defined as ACTIVE-LOW in the following call:
> >
> > acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
> >   acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
> >
> > it will return GPIOD_OUT_HIGH if pull_up is set no matter if
> > polarity is GPIO_ACTIVE_LOW, so it will return the current level instead
> > of the logical level.
>
> Thank you for the patch.
>
> I have question in general. If we have Active Low polarity and Pull Down,
> isn't it simple a bad ACPI table and rather quirk is needed here?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
> Hi,
>
> It may be, also it may be a bad hardware design but it is also a possible situation.
>
> In our case here we have an FPGA whose pcie link is held in reset by BIOS during
> boot, the reset pin is active low, and its configuration is specified in the Acpi DSDT
> table. When Linux starts, our userspace driver shall load the FPGA, and the first
> step is to request all GPIO's needed to configure the pcie phy on the FPGA side;
> the pcie link reset should be held active while this configuration (loading of the
> Altera fpp image) is ongoing.
> Now all active low pins have their initial value inverted by the kernel. This means
> that the pcie link reset is briefly released, which generates a pcie hot unplug event,
> which in turn delays start of a successful loading sequence, and so SW has to make
> a second reload attempt which will delay too much the normal boot sequence.
>
> Sorry for giving you too much details which probability are not important but I only
> wanted to emphasise that it may be a real situation whether or not it is a good
> design.

No need to sorry, the details are useful as well as knowing your use case.

Q1: who is providing DSDT?
Q2: why it can't be fixed?
Q3: how the GPIO lines are being requested?
Q4: how are they described, btw, in DSDT?

Side note, if it will be no other choice left, the above code still can be
added as a quirk (see code related to ACPI_GPIO_QUIRK_NO_IO_RESTRICTION
and ACPI_GPIO_QUIRK_ONLY_GPIOIO).

--
With Best Regards,
Andy Shevchenko

Hi, 

Unfortunately the DSDT is confidential, also I'm not the one that did the DSDT table, but I can say 
that the GpioIo macro is used to define the pins, and a label is assigned. This label is used for the 
lookup (gpiod_get(device, label, 0)). 
The requester knows nothing about the specific configuration and polarity of each pin, this is only 
available through DSDT, so it's not possible to supply flags in the gpiod_get-call.
The pin itself is a reset pin that must (as defined by rfc2119) be asserted during Linux boot (it controls
 the reset logic of a soc external device), the polarity is active low, which is why it's configured with a PullDown. 

Also we discussed here about using IoRestrictionNone/IoRestrictionNonePreserve as a potential solution to 
this problem, but this is a pure output pin, so that didn't seem right to us either.

In the end the main question, no matter the use case,  is how an active low pin, that's being 
asserted from BIOS, should be configured in the DSDT to be correctly represented by the code.
The patch was the result of not finding any solution to the problem above. Speaking strictly of the 
patch it should solve the problem of this case of active-low pins. 

Regards,
 vls


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
  2019-12-03 15:24       ` Stanimir, Vasile-Laurentiu
@ 2019-12-04 14:08         ` andriy.shevchenko
  2019-12-05 17:49           ` andriy.shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: andriy.shevchenko @ 2019-12-04 14:08 UTC (permalink / raw)
  To: Stanimir, Vasile-Laurentiu; +Cc: linux-gpio, linus.walleij, mika.westerberg

On Tue, Dec 03, 2019 at 03:24:57PM +0000, Stanimir, Vasile-Laurentiu wrote:
> On Mon, Dec 02, 2019 at 01:44:04PM +0000, Stanimir, Vasile-Laurentiu wrote:
> > On Mon, Dec 02, 2019 at 12:36:47PM +0000, Stanimir, Vasile-Laurentiu wrote:

> > > ACPI GPIO resources don't contain an initial value for the
> > > GPIO. Therefore instead of deducting its value based on pullup field
> > > we should deduce that value from the polarity and the pull field.
> > > Typical scenario is when ACPI is defined in acpi-table and its polarity
> > > is defined as ACTIVE-LOW in the following call:
> > >
> > > acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
> > >   acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
> > >
> > > it will return GPIOD_OUT_HIGH if pull_up is set no matter if
> > > polarity is GPIO_ACTIVE_LOW, so it will return the current level instead
> > > of the logical level.
> >
> > Thank you for the patch.
> >
> > I have question in general. If we have Active Low polarity and Pull Down,
> > isn't it simple a bad ACPI table and rather quirk is needed here?

> > It may be, also it may be a bad hardware design but it is also a possible situation.
> >
> > In our case here we have an FPGA whose pcie link is held in reset by BIOS during
> > boot, the reset pin is active low, and its configuration is specified in the Acpi DSDT
> > table. When Linux starts, our userspace driver shall load the FPGA, and the first
> > step is to request all GPIO's needed to configure the pcie phy on the FPGA side;
> > the pcie link reset should be held active while this configuration (loading of the
> > Altera fpp image) is ongoing.
> > Now all active low pins have their initial value inverted by the kernel. This means
> > that the pcie link reset is briefly released, which generates a pcie hot unplug event,
> > which in turn delays start of a successful loading sequence, and so SW has to make
> > a second reload attempt which will delay too much the normal boot sequence.
> >
> > Sorry for giving you too much details which probability are not important but I only
> > wanted to emphasise that it may be a real situation whether or not it is a good
> > design.
> 
> No need to sorry, the details are useful as well as knowing your use case.
> 
> Q1: who is providing DSDT?
> Q2: why it can't be fixed?
> Q3: how the GPIO lines are being requested?
> Q4: how are they described, btw, in DSDT?
> 
> Side note, if it will be no other choice left, the above code still can be
> added as a quirk (see code related to ACPI_GPIO_QUIRK_NO_IO_RESTRICTION
> and ACPI_GPIO_QUIRK_ONLY_GPIOIO).
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> Hi, 
> 
> Unfortunately the DSDT is confidential,

OK, I assume that the devices with it are not in the wild (yet).

> also I'm not the one that did the DSDT table,

Then they should fix that, no?

>      	but I can say 
> that the GpioIo macro is used to define the pins, and a label is assigned. This label is used for the 
> lookup (gpiod_get(device, label, 0)). 
> The requester knows nothing about the specific configuration and polarity of each pin, this is only 
> available through DSDT, so it's not possible to supply flags in the gpiod_get-call.

Right.

> The pin itself is a reset pin that must (as defined by rfc2119) be asserted during Linux boot (it controls
>  the reset logic of a soc external device), the polarity is active low, which is why it's configured with a PullDown. 

And here is the issue AFAICS. The Active low should be configured with PullUp
and set to whatever BIOS wants.

Correct me if I'm wrong, but code does the following in
the properly configured DSDT:

0/ (not OS) firmware prepares ACPI DSDT with parameters (see 2/) and sets pin
   to active state - electrical low;
1/ we request GPIO via gpiod_get(..., GPIOD_ASIS);
2/ ACPI provides: IoOutput + Active Low + PullUp;
3/ above is translated to GPIO flags = GPIOD_OUT_HIGH and look up flags =
   GPIO_PULL_UP | GPIO_ACTIVE_LOW;
4/ ...which calls gpiod_direction_output() with value = 1 and being inverted
   to 0 due to Active Low in the descriptor flags;
5/ 0, i.e. electrically low signal, comes out from the SoC (compare
   with 0/ above).

If it's not, we need to fix a root cause of it.

> Also we discussed here about using IoRestrictionNone/IoRestrictionNonePreserve as a potential solution to 
> this problem, but this is a pure output pin, so that didn't seem right to us either.
> 
> In the end the main question, no matter the use case,  is how an active low pin, that's being 
> asserted from BIOS, should be configured in the DSDT to be correctly represented by the code.
> The patch was the result of not finding any solution to the problem above. Speaking strictly of the 
> patch it should solve the problem of this case of active-low pins. 

P.S. Before we are going further we need to see the dmesg when
CONFIG_DEBUG_GPIO is set along with GpioIo() macro and corresponding
_DSD() excerpts from DSDT.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity
  2019-12-04 14:08         ` andriy.shevchenko
@ 2019-12-05 17:49           ` andriy.shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: andriy.shevchenko @ 2019-12-05 17:49 UTC (permalink / raw)
  To: Stanimir, Vasile-Laurentiu; +Cc: linux-gpio, linus.walleij, mika.westerberg

On Wed, Dec 04, 2019 at 04:08:34PM +0200, andriy.shevchenko@linux.intel.com wrote:
> On Tue, Dec 03, 2019 at 03:24:57PM +0000, Stanimir, Vasile-Laurentiu wrote:
> > On Mon, Dec 02, 2019 at 01:44:04PM +0000, Stanimir, Vasile-Laurentiu wrote:
> > > On Mon, Dec 02, 2019 at 12:36:47PM +0000, Stanimir, Vasile-Laurentiu wrote:

> > The pin itself is a reset pin that must (as defined by rfc2119) be asserted during Linux boot (it controls
> >  the reset logic of a soc external device), the polarity is active low, which is why it's configured with a PullDown. 
> 
> And here is the issue AFAICS. The Active low should be configured with PullUp
> and set to whatever BIOS wants.

I stand corrected:
whatever -> active state

So, basically BIOS settings in ACPI and in hardware should be in align.
Otherwise it's badly created / configured BIOS.

> Correct me if I'm wrong, but code does the following in
> the properly configured DSDT:
> 
> 0/ (not OS) firmware prepares ACPI DSDT with parameters (see 2/) and sets pin
>    to active state - electrical low;
> 1/ we request GPIO via gpiod_get(..., GPIOD_ASIS);
> 2/ ACPI provides: IoOutput + Active Low + PullUp;
> 3/ above is translated to GPIO flags = GPIOD_OUT_HIGH and look up flags =
>    GPIO_PULL_UP | GPIO_ACTIVE_LOW;
> 4/ ...which calls gpiod_direction_output() with value = 1 and being inverted
>    to 0 due to Active Low in the descriptor flags;
> 5/ 0, i.e. electrically low signal, comes out from the SoC (compare
>    with 0/ above).
> 
> If it's not, we need to fix a root cause of it.
> 
> > Also we discussed here about using IoRestrictionNone/IoRestrictionNonePreserve as a potential solution to 
> > this problem, but this is a pure output pin, so that didn't seem right to us either.
> > 
> > In the end the main question, no matter the use case,  is how an active low pin, that's being 
> > asserted from BIOS, should be configured in the DSDT to be correctly represented by the code.
> > The patch was the result of not finding any solution to the problem above. Speaking strictly of the 
> > patch it should solve the problem of this case of active-low pins. 

> P.S. Before we are going further we need to see the dmesg when
> CONFIG_DEBUG_GPIO is set along with GpioIo() macro and corresponding
> _DSD() excerpts from DSDT.

You can send it privately if something is not okay to share (though I don't see
such in this case).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-12-05 17:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 12:36 [PATCH] gpiolib-acpi: Set gpiod flags for ACPI GPIO resources based on pullup and polarity Stanimir, Vasile-Laurentiu
2019-12-02 13:05 ` andriy.shevchenko
2019-12-02 13:44   ` Stanimir, Vasile-Laurentiu
2019-12-02 14:08     ` andriy.shevchenko
2019-12-03 15:24       ` Stanimir, Vasile-Laurentiu
2019-12-04 14:08         ` andriy.shevchenko
2019-12-05 17:49           ` andriy.shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).