Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
@ 2020-02-05 19:48 Michal Stanek
  2020-02-06  8:31 ` Mika Westerberg
  2020-02-06  9:25 ` Andy Shevchenko
  0 siblings, 2 replies; 12+ messages in thread
From: Michal Stanek @ 2020-02-05 19:48 UTC (permalink / raw)
  To: linux-gpio, linux-acpi, linux-kernel
  Cc: stanekm, stable, mw, levinale, mika.westerberg,
	andriy.shevchenko, linus.walleij, bgolaszewski, rafael.j.wysocki,
	Michal Stanek

Dropping custom Linux GPIO translation caused some Intel_Strago based Chromebooks
with old firmware to detect GPIOs incorrectly. Add quirk which restores some code removed by
commit 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation").

Fixes: 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation")
Cc: <stable@vger.kernel.org>
Reported-by: Alex Levin <levinale@chromium.org>
Signed-off-by: Michal Stanek <mst@semihalf.com>
---
 drivers/gpio/gpiolib-acpi.c                | 102 ++++++++++++++++++++-
 drivers/pinctrl/intel/pinctrl-cherryview.c |  63 ++++++++++---
 2 files changed, 150 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 31fee5e918b7..b5358facf3fb 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -23,6 +23,7 @@
 
 #define QUIRK_NO_EDGE_EVENTS_ON_BOOT		0x01l
 #define QUIRK_NO_WAKEUP				0x02l
+#define QUIRK_NEED_ACPI_GPIO_TRANSLATION	0x04l
 
 static int run_edge_events_on_boot = -1;
 module_param(run_edge_events_on_boot, int, 0444);
@@ -34,6 +35,11 @@ module_param(honor_wakeup, int, 0444);
 MODULE_PARM_DESC(honor_wakeup,
 		 "Honor the ACPI wake-capable flag: 0=no, 1=yes, -1=auto");
 
+static int need_acpi_gpio_translation = -1;
+module_param(need_acpi_gpio_translation, int, 0444);
+MODULE_PARM_DESC(need_acpi_gpio_translation,
+		 "Do custom ACPI GPIO number translation: 0=no, 1=yes, -1=auto");
+
 /**
  * struct acpi_gpio_event - ACPI GPIO event handler data
  *
@@ -98,6 +104,62 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 	return ACPI_HANDLE(gc->parent) == data;
 }
 
+#ifdef CONFIG_PINCTRL
+/**
+ * acpi_gpiochip_pin_to_gpio_offset() - translates ACPI GPIO to Linux GPIO
+ * @gdev: GPIO device
+ * @pin: ACPI GPIO pin number from GpioIo/GpioInt resource
+ *
+ * Function takes ACPI GpioIo/GpioInt pin number as a parameter and
+ * translates it to a corresponding offset suitable to be passed to a
+ * GPIO controller driver.
+ *
+ * Typically the returned offset is same as @pin, but if the GPIO
+ * controller uses pin controller and the mapping is not contiguous the
+ * offset might be different.
+ */
+static int acpi_gpiochip_pin_to_gpio_offset(struct gpio_device *gdev, int pin)
+{
+	struct gpio_pin_range *pin_range;
+
+	/* Only do translation on selected platforms */
+	if (!need_acpi_gpio_translation)
+		return pin;
+
+	/* If there are no ranges in this chip, use 1:1 mapping */
+	if (list_empty(&gdev->pin_ranges))
+		return pin;
+
+	list_for_each_entry(pin_range, &gdev->pin_ranges, node) {
+		const struct pinctrl_gpio_range *range = &pin_range->range;
+		int i;
+
+		if (range->pins) {
+			for (i = 0; i < range->npins; i++) {
+				if (range->pins[i] == pin)
+					return range->base + i - gdev->base;
+			}
+		} else {
+			if (pin >= range->pin_base &&
+			    pin < range->pin_base + range->npins) {
+				unsigned int gpio_base;
+
+				gpio_base = range->base - gdev->base;
+				return gpio_base + pin - range->pin_base;
+			}
+		}
+	}
+
+	return -EINVAL;
+}
+#else
+static inline int acpi_gpiochip_pin_to_gpio_offset(struct gpio_device *gdev,
+						   int pin)
+{
+	return pin;
+}
+#endif
+
 /**
  * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
  * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
@@ -113,6 +175,7 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 	struct gpio_chip *chip;
 	acpi_handle handle;
 	acpi_status status;
+	int offset;
 
 	status = acpi_get_handle(NULL, path, &handle);
 	if (ACPI_FAILURE(status))
@@ -122,7 +185,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 	if (!chip)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	return gpiochip_get_desc(chip, pin);
+	offset = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
+	if (offset < 0)
+		return ERR_PTR(offset);
+
+	return gpiochip_get_desc(chip, offset);
 }
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
@@ -236,6 +303,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 	if (!handler)
 		return AE_OK;
 
+	pin = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
+	if (pin < 0)
+		return AE_BAD_PARAMETER;
+
 	desc = gpiochip_request_own_desc(chip, pin, "ACPI:Event",
 					 GPIO_ACTIVE_HIGH, GPIOD_IN);
 	if (IS_ERR(desc)) {
@@ -958,6 +1029,12 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		struct gpio_desc *desc;
 		bool found;
 
+		pin = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
+		if (pin < 0) {
+			status = AE_BAD_PARAMETER;
+			goto out;
+		}
+
 		mutex_lock(&achip->conn_lock);
 
 		found = false;
@@ -1086,7 +1163,11 @@ acpi_gpiochip_parse_own_gpio(struct acpi_gpio_chip *achip,
 	if (ret < 0)
 		return ERR_PTR(ret);
 
-	desc = gpiochip_get_desc(chip, gpios[0]);
+	ret = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, gpios[0]);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	desc = gpiochip_get_desc(chip, ret);
 	if (IS_ERR(desc))
 		return desc;
 
@@ -1360,6 +1441,17 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
 		},
 		.driver_data = (void *)QUIRK_NO_WAKEUP,
 	},
+	{
+		/*
+		 * Some Braswell based Google Chromebooks need custom ACPI GPIO
+		 * number translation due to hardcoded GPIO numbers in firmware.
+		 */
+		.matches = {
+		       DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
+		       DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_Strago"),
+		},
+		.driver_data = (void *)QUIRK_NEED_ACPI_GPIO_TRANSLATION,
+	},
 	{} /* Terminating entry */
 };
 
@@ -1385,6 +1477,12 @@ static int acpi_gpio_setup_params(void)
 		else
 			honor_wakeup = 1;
 	}
+	if (need_acpi_gpio_translation < 0) {
+		if (quirks & QUIRK_NEED_ACPI_GPIO_TRANSLATION)
+			need_acpi_gpio_translation = 1;
+		else
+			need_acpi_gpio_translation = 0;
+	}
 
 	return 0;
 }
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 4c74fdde576d..dd190ac6b11c 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -119,6 +119,7 @@ struct chv_gpio_pinrange {
  * @nfunctions: Number of functions
  * @gpio_ranges: An array of GPIO ranges in this community
  * @ngpio_ranges: Number of GPIO ranges
+ * @ngpios: Total number of GPIOs in this community
  * @nirqs: Total number of IRQs this community can generate
  * @acpi_space_id: An address space ID for ACPI OpRegion handler
  */
@@ -132,6 +133,7 @@ struct chv_community {
 	size_t nfunctions;
 	const struct chv_gpio_pinrange *gpio_ranges;
 	size_t ngpio_ranges;
+	size_t ngpios;
 	size_t nirqs;
 	acpi_adr_space_type acpi_space_id;
 };
@@ -380,6 +382,7 @@ static const struct chv_community southwest_community = {
 	.nfunctions = ARRAY_SIZE(southwest_functions),
 	.gpio_ranges = southwest_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(southwest_gpio_ranges),
+	.ngpios = ARRAY_SIZE(southwest_pins),
 	/*
 	 * Southwest community can generate GPIO interrupts only for the
 	 * first 8 interrupts. The upper half (8-15) can only be used to
@@ -469,6 +472,7 @@ static const struct chv_community north_community = {
 	.npins = ARRAY_SIZE(north_pins),
 	.gpio_ranges = north_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(north_gpio_ranges),
+	.ngpios = ARRAY_SIZE(north_pins),
 	/*
 	 * North community can generate GPIO interrupts only for the first
 	 * 8 interrupts. The upper half (8-15) can only be used to trigger
@@ -517,6 +521,7 @@ static const struct chv_community east_community = {
 	.npins = ARRAY_SIZE(east_pins),
 	.gpio_ranges = east_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(east_gpio_ranges),
+	.ngpios = ARRAY_SIZE(east_pins),
 	.nirqs = 16,
 	.acpi_space_id = 0x93,
 };
@@ -643,6 +648,7 @@ static const struct chv_community southeast_community = {
 	.nfunctions = ARRAY_SIZE(southeast_functions),
 	.gpio_ranges = southeast_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(southeast_gpio_ranges),
+	.ngpios = ARRAY_SIZE(southeast_pins),
 	.nirqs = 16,
 	.acpi_space_id = 0x94,
 };
@@ -1236,14 +1242,39 @@ static struct pinctrl_desc chv_pinctrl_desc = {
 	.owner = THIS_MODULE,
 };
 
+static const struct dmi_system_id need_acpi_gpio_translation[] = {
+	{
+		/*
+		 * Some Braswell based Google Chromebooks need custom ACPI GPIO
+		 * number translation due to hardcoded GPIO numbers in firmware.
+		 */
+		.ident = "Intel_Strago based Chromebooks (All models)",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
+			DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_Strago"),
+		},
+	},
+};
+
+static unsigned int chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
+				      unsigned int offset)
+{
+	/* Mapping is not 1:1 on some platforms */
+	if (dmi_check_system(need_acpi_gpio_translation))
+		return pctrl->community->pins[offset].number;
+	else
+		return offset;
+}
+
 static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
 	unsigned long flags;
 	u32 ctrl0, cfg;
 
 	raw_spin_lock_irqsave(&chv_lock, flags);
-	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
+	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
@@ -1257,13 +1288,14 @@ static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
 static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
 	unsigned long flags;
 	void __iomem *reg;
 	u32 ctrl0;
 
 	raw_spin_lock_irqsave(&chv_lock, flags);
 
-	reg = chv_padreg(pctrl, offset, CHV_PADCTRL0);
+	reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
 	ctrl0 = readl(reg);
 
 	if (value)
@@ -1279,11 +1311,12 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
 	u32 ctrl0, direction;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&chv_lock, flags);
-	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
+	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	direction = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
@@ -1322,7 +1355,7 @@ static void chv_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
-	int pin = irqd_to_hwirq(d);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
 	u32 intr_line;
 
 	raw_spin_lock(&chv_lock);
@@ -1339,7 +1372,7 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
-	int pin = irqd_to_hwirq(d);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
 	u32 value, intr_line;
 	unsigned long flags;
 
@@ -1384,7 +1417,8 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_NONE) {
 		struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 		struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
-		unsigned int pin = irqd_to_hwirq(d);
+		unsigned int offset = irqd_to_hwirq(d);
+		unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
 		irq_flow_handler_t handler;
 		unsigned long flags;
 		u32 intsel, value;
@@ -1402,7 +1436,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 
 		if (!pctrl->intr_lines[intsel]) {
 			irq_set_handler_locked(d, handler);
-			pctrl->intr_lines[intsel] = pin;
+			pctrl->intr_lines[intsel] = offset;
 		}
 		raw_spin_unlock_irqrestore(&chv_lock, flags);
 	}
@@ -1415,7 +1449,8 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
-	unsigned int pin = irqd_to_hwirq(d);
+	unsigned int offset = irqd_to_hwirq(d);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
 	unsigned long flags;
 	u32 value;
 
@@ -1461,7 +1496,7 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 	value &= CHV_PADCTRL0_INTSEL_MASK;
 	value >>= CHV_PADCTRL0_INTSEL_SHIFT;
 
-	pctrl->intr_lines[value] = pin;
+	pctrl->intr_lines[value] = offset;
 
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		irq_set_handler_locked(d, handle_edge_irq);
@@ -1591,17 +1626,19 @@ static int chv_gpio_add_pin_ranges(struct gpio_chip *chip)
 	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
 	const struct chv_community *community = pctrl->community;
 	const struct chv_gpio_pinrange *range;
-	int ret, i;
+	int ret, i, offset;
 
-	for (i = 0; i < community->ngpio_ranges; i++) {
+	for (i = 0, offset = 0; i < community->ngpio_ranges; i++) {
 		range = &community->gpio_ranges[i];
 		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
-					     range->base, range->base,
+					     offset, range->base,
 					     range->npins);
 		if (ret) {
 			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
 			return ret;
 		}
+
+		offset += range->npins;
 	}
 
 	return 0;
@@ -1617,7 +1654,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 
 	*chip = chv_gpio_chip;
 
-	chip->ngpio = community->pins[community->npins - 1].number + 1;
+	chip->ngpio = community->ngpios;
 	chip->label = dev_name(pctrl->dev);
 	chip->add_pin_ranges = chv_gpio_add_pin_ranges;
 	chip->parent = pctrl->dev;
-- 
2.17.1


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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-05 19:48 [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers Michal Stanek
@ 2020-02-06  8:31 ` Mika Westerberg
  2020-02-06 22:26   ` Michał Stanek
  2020-02-06  9:25 ` Andy Shevchenko
  1 sibling, 1 reply; 12+ messages in thread
From: Mika Westerberg @ 2020-02-06  8:31 UTC (permalink / raw)
  To: Michal Stanek
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable, mw,
	levinale, andriy.shevchenko, linus.walleij, bgolaszewski,
	rafael.j.wysocki, Dmitry Torokhov

On Wed, Feb 05, 2020 at 08:48:04PM +0100, Michal Stanek wrote:
> Dropping custom Linux GPIO translation caused some Intel_Strago based Chromebooks
> with old firmware to detect GPIOs incorrectly. Add quirk which restores some code removed by
> commit 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation").

Hi,

Can you elaborate this? I was under the impression that all the
different Strago systems have been already worked around by patches from
Dmitry (Cc'd).

What GPIO(s) we are talking about and how does it show up to the user?

> Fixes: 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation")
> Cc: <stable@vger.kernel.org>
> Reported-by: Alex Levin <levinale@chromium.org>
> Signed-off-by: Michal Stanek <mst@semihalf.com>
> ---
>  drivers/gpio/gpiolib-acpi.c                | 102 ++++++++++++++++++++-
>  drivers/pinctrl/intel/pinctrl-cherryview.c |  63 ++++++++++---
>  2 files changed, 150 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 31fee5e918b7..b5358facf3fb 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -23,6 +23,7 @@
>  
>  #define QUIRK_NO_EDGE_EVENTS_ON_BOOT		0x01l
>  #define QUIRK_NO_WAKEUP				0x02l
> +#define QUIRK_NEED_ACPI_GPIO_TRANSLATION	0x04l
>  
>  static int run_edge_events_on_boot = -1;
>  module_param(run_edge_events_on_boot, int, 0444);
> @@ -34,6 +35,11 @@ module_param(honor_wakeup, int, 0444);
>  MODULE_PARM_DESC(honor_wakeup,
>  		 "Honor the ACPI wake-capable flag: 0=no, 1=yes, -1=auto");
>  
> +static int need_acpi_gpio_translation = -1;
> +module_param(need_acpi_gpio_translation, int, 0444);
> +MODULE_PARM_DESC(need_acpi_gpio_translation,
> +		 "Do custom ACPI GPIO number translation: 0=no, 1=yes, -1=auto");
> +
>  /**
>   * struct acpi_gpio_event - ACPI GPIO event handler data
>   *
> @@ -98,6 +104,62 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  	return ACPI_HANDLE(gc->parent) == data;
>  }
>  
> +#ifdef CONFIG_PINCTRL
> +/**
> + * acpi_gpiochip_pin_to_gpio_offset() - translates ACPI GPIO to Linux GPIO
> + * @gdev: GPIO device
> + * @pin: ACPI GPIO pin number from GpioIo/GpioInt resource
> + *
> + * Function takes ACPI GpioIo/GpioInt pin number as a parameter and
> + * translates it to a corresponding offset suitable to be passed to a
> + * GPIO controller driver.
> + *
> + * Typically the returned offset is same as @pin, but if the GPIO
> + * controller uses pin controller and the mapping is not contiguous the
> + * offset might be different.
> + */
> +static int acpi_gpiochip_pin_to_gpio_offset(struct gpio_device *gdev, int pin)
> +{
> +	struct gpio_pin_range *pin_range;
> +
> +	/* Only do translation on selected platforms */
> +	if (!need_acpi_gpio_translation)
> +		return pin;
> +
> +	/* If there are no ranges in this chip, use 1:1 mapping */
> +	if (list_empty(&gdev->pin_ranges))
> +		return pin;
> +
> +	list_for_each_entry(pin_range, &gdev->pin_ranges, node) {
> +		const struct pinctrl_gpio_range *range = &pin_range->range;
> +		int i;
> +
> +		if (range->pins) {
> +			for (i = 0; i < range->npins; i++) {
> +				if (range->pins[i] == pin)
> +					return range->base + i - gdev->base;
> +			}
> +		} else {
> +			if (pin >= range->pin_base &&
> +			    pin < range->pin_base + range->npins) {
> +				unsigned int gpio_base;
> +
> +				gpio_base = range->base - gdev->base;
> +				return gpio_base + pin - range->pin_base;
> +			}
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +#else
> +static inline int acpi_gpiochip_pin_to_gpio_offset(struct gpio_device *gdev,
> +						   int pin)
> +{
> +	return pin;
> +}
> +#endif
> +
>  /**
>   * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
>   * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
> @@ -113,6 +175,7 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>  	struct gpio_chip *chip;
>  	acpi_handle handle;
>  	acpi_status status;
> +	int offset;
>  
>  	status = acpi_get_handle(NULL, path, &handle);
>  	if (ACPI_FAILURE(status))
> @@ -122,7 +185,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>  	if (!chip)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> -	return gpiochip_get_desc(chip, pin);
> +	offset = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
> +	if (offset < 0)
> +		return ERR_PTR(offset);
> +
> +	return gpiochip_get_desc(chip, offset);
>  }
>  
>  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> @@ -236,6 +303,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
>  	if (!handler)
>  		return AE_OK;
>  
> +	pin = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
> +	if (pin < 0)
> +		return AE_BAD_PARAMETER;
> +
>  	desc = gpiochip_request_own_desc(chip, pin, "ACPI:Event",
>  					 GPIO_ACTIVE_HIGH, GPIOD_IN);
>  	if (IS_ERR(desc)) {
> @@ -958,6 +1029,12 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  		struct gpio_desc *desc;
>  		bool found;
>  
> +		pin = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, pin);
> +		if (pin < 0) {
> +			status = AE_BAD_PARAMETER;
> +			goto out;
> +		}
> +
>  		mutex_lock(&achip->conn_lock);
>  
>  		found = false;
> @@ -1086,7 +1163,11 @@ acpi_gpiochip_parse_own_gpio(struct acpi_gpio_chip *achip,
>  	if (ret < 0)
>  		return ERR_PTR(ret);
>  
> -	desc = gpiochip_get_desc(chip, gpios[0]);
> +	ret = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, gpios[0]);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	desc = gpiochip_get_desc(chip, ret);
>  	if (IS_ERR(desc))
>  		return desc;
>  
> @@ -1360,6 +1441,17 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] = {
>  		},
>  		.driver_data = (void *)QUIRK_NO_WAKEUP,
>  	},
> +	{
> +		/*
> +		 * Some Braswell based Google Chromebooks need custom ACPI GPIO
> +		 * number translation due to hardcoded GPIO numbers in firmware.
> +		 */
> +		.matches = {
> +		       DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> +		       DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_Strago"),
> +		},

AFAIK this now applies pretty much all Strago systems so at least this
one should explicitly match only the affected firmware versions.

> +		.driver_data = (void *)QUIRK_NEED_ACPI_GPIO_TRANSLATION,
> +	},
>  	{} /* Terminating entry */
>  };
>  
> @@ -1385,6 +1477,12 @@ static int acpi_gpio_setup_params(void)
>  		else
>  			honor_wakeup = 1;
>  	}
> +	if (need_acpi_gpio_translation < 0) {
> +		if (quirks & QUIRK_NEED_ACPI_GPIO_TRANSLATION)
> +			need_acpi_gpio_translation = 1;
> +		else
> +			need_acpi_gpio_translation = 0;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 4c74fdde576d..dd190ac6b11c 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -119,6 +119,7 @@ struct chv_gpio_pinrange {
>   * @nfunctions: Number of functions
>   * @gpio_ranges: An array of GPIO ranges in this community
>   * @ngpio_ranges: Number of GPIO ranges
> + * @ngpios: Total number of GPIOs in this community
>   * @nirqs: Total number of IRQs this community can generate
>   * @acpi_space_id: An address space ID for ACPI OpRegion handler
>   */
> @@ -132,6 +133,7 @@ struct chv_community {
>  	size_t nfunctions;
>  	const struct chv_gpio_pinrange *gpio_ranges;
>  	size_t ngpio_ranges;
> +	size_t ngpios;
>  	size_t nirqs;
>  	acpi_adr_space_type acpi_space_id;
>  };
> @@ -380,6 +382,7 @@ static const struct chv_community southwest_community = {
>  	.nfunctions = ARRAY_SIZE(southwest_functions),
>  	.gpio_ranges = southwest_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(southwest_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(southwest_pins),
>  	/*
>  	 * Southwest community can generate GPIO interrupts only for the
>  	 * first 8 interrupts. The upper half (8-15) can only be used to
> @@ -469,6 +472,7 @@ static const struct chv_community north_community = {
>  	.npins = ARRAY_SIZE(north_pins),
>  	.gpio_ranges = north_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(north_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(north_pins),
>  	/*
>  	 * North community can generate GPIO interrupts only for the first
>  	 * 8 interrupts. The upper half (8-15) can only be used to trigger
> @@ -517,6 +521,7 @@ static const struct chv_community east_community = {
>  	.npins = ARRAY_SIZE(east_pins),
>  	.gpio_ranges = east_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(east_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(east_pins),
>  	.nirqs = 16,
>  	.acpi_space_id = 0x93,
>  };
> @@ -643,6 +648,7 @@ static const struct chv_community southeast_community = {
>  	.nfunctions = ARRAY_SIZE(southeast_functions),
>  	.gpio_ranges = southeast_gpio_ranges,
>  	.ngpio_ranges = ARRAY_SIZE(southeast_gpio_ranges),
> +	.ngpios = ARRAY_SIZE(southeast_pins),
>  	.nirqs = 16,
>  	.acpi_space_id = 0x94,
>  };
> @@ -1236,14 +1242,39 @@ static struct pinctrl_desc chv_pinctrl_desc = {
>  	.owner = THIS_MODULE,
>  };
>  
> +static const struct dmi_system_id need_acpi_gpio_translation[] = {
> +	{
> +		/*
> +		 * Some Braswell based Google Chromebooks need custom ACPI GPIO
> +		 * number translation due to hardcoded GPIO numbers in firmware.
> +		 */
> +		.ident = "Intel_Strago based Chromebooks (All models)",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_Strago"),

Same comment here. Are you saying all Stragos are broken in the current
mainline?

> +		},
> +	},
> +};
> +
> +static unsigned int chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
> +				      unsigned int offset)
> +{
> +	/* Mapping is not 1:1 on some platforms */
> +	if (dmi_check_system(need_acpi_gpio_translation))
> +		return pctrl->community->pins[offset].number;
> +	else
> +		return offset;
> +}
> +
>  static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
> +	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	unsigned long flags;
>  	u32 ctrl0, cfg;
>  
>  	raw_spin_lock_irqsave(&chv_lock, flags);
> -	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
> +	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
>  	raw_spin_unlock_irqrestore(&chv_lock, flags);
>  
>  	cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
> @@ -1257,13 +1288,14 @@ static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
>  static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
>  {
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
> +	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	unsigned long flags;
>  	void __iomem *reg;
>  	u32 ctrl0;
>  
>  	raw_spin_lock_irqsave(&chv_lock, flags);
>  
> -	reg = chv_padreg(pctrl, offset, CHV_PADCTRL0);
> +	reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
>  	ctrl0 = readl(reg);
>  
>  	if (value)
> @@ -1279,11 +1311,12 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
>  static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
> +	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	u32 ctrl0, direction;
>  	unsigned long flags;
>  
>  	raw_spin_lock_irqsave(&chv_lock, flags);
> -	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
> +	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
>  	raw_spin_unlock_irqrestore(&chv_lock, flags);
>  
>  	direction = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
> @@ -1322,7 +1355,7 @@ static void chv_gpio_irq_ack(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int pin = irqd_to_hwirq(d);
> +	unsigned int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
>  	u32 intr_line;
>  
>  	raw_spin_lock(&chv_lock);
> @@ -1339,7 +1372,7 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int pin = irqd_to_hwirq(d);
> +	unsigned int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
>  	u32 value, intr_line;
>  	unsigned long flags;
>  
> @@ -1384,7 +1417,8 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  	if (irqd_get_trigger_type(d) == IRQ_TYPE_NONE) {
>  		struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  		struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
> -		unsigned int pin = irqd_to_hwirq(d);
> +		unsigned int offset = irqd_to_hwirq(d);
> +		unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  		irq_flow_handler_t handler;
>  		unsigned long flags;
>  		u32 intsel, value;
> @@ -1402,7 +1436,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  
>  		if (!pctrl->intr_lines[intsel]) {
>  			irq_set_handler_locked(d, handler);
> -			pctrl->intr_lines[intsel] = pin;
> +			pctrl->intr_lines[intsel] = offset;
>  		}
>  		raw_spin_unlock_irqrestore(&chv_lock, flags);
>  	}
> @@ -1415,7 +1449,8 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
> -	unsigned int pin = irqd_to_hwirq(d);
> +	unsigned int offset = irqd_to_hwirq(d);
> +	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
>  	unsigned long flags;
>  	u32 value;
>  
> @@ -1461,7 +1496,7 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
>  	value &= CHV_PADCTRL0_INTSEL_MASK;
>  	value >>= CHV_PADCTRL0_INTSEL_SHIFT;
>  
> -	pctrl->intr_lines[value] = pin;
> +	pctrl->intr_lines[value] = offset;
>  
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		irq_set_handler_locked(d, handle_edge_irq);
> @@ -1591,17 +1626,19 @@ static int chv_gpio_add_pin_ranges(struct gpio_chip *chip)
>  	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
>  	const struct chv_community *community = pctrl->community;
>  	const struct chv_gpio_pinrange *range;
> -	int ret, i;
> +	int ret, i, offset;
>  
> -	for (i = 0; i < community->ngpio_ranges; i++) {
> +	for (i = 0, offset = 0; i < community->ngpio_ranges; i++) {
>  		range = &community->gpio_ranges[i];
>  		ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev),
> -					     range->base, range->base,
> +					     offset, range->base,
>  					     range->npins);
>  		if (ret) {
>  			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
>  			return ret;
>  		}
> +
> +		offset += range->npins;
>  	}
>  
>  	return 0;
> @@ -1617,7 +1654,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>  
>  	*chip = chv_gpio_chip;
>  
> -	chip->ngpio = community->pins[community->npins - 1].number + 1;
> +	chip->ngpio = community->ngpios;
>  	chip->label = dev_name(pctrl->dev);
>  	chip->add_pin_ranges = chv_gpio_add_pin_ranges;
>  	chip->parent = pctrl->dev;
> -- 
> 2.17.1

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-05 19:48 [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers Michal Stanek
  2020-02-06  8:31 ` Mika Westerberg
@ 2020-02-06  9:25 ` Andy Shevchenko
  2020-02-06 18:31   ` Dmitry Torokhov
  1 sibling, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-02-06  9:25 UTC (permalink / raw)
  To: Michal Stanek
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable, mw,
	levinale, mika.westerberg, linus.walleij, bgolaszewski,
	rafael.j.wysocki

On Wed, Feb 05, 2020 at 08:48:04PM +0100, Michal Stanek wrote:
> Dropping custom Linux GPIO translation caused some Intel_Strago based Chromebooks
> with old firmware to detect GPIOs incorrectly. Add quirk which restores some code removed by
> commit 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation").

And on top what Mika asked already, is it a real problem to update firmware?

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-06  9:25 ` Andy Shevchenko
@ 2020-02-06 18:31   ` Dmitry Torokhov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2020-02-06 18:31 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Michal Stanek, open list:GPIO SUBSYSTEM, linux-acpi, lkml,
	stanekm, 3.8+,
	mw, Alex Levin, Mika Westerberg, Linus Walleij,
	Bartosz Golaszewski, Rafael J. Wysocki

On Thu, Feb 6, 2020 at 1:45 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Feb 05, 2020 at 08:48:04PM +0100, Michal Stanek wrote:
> > Dropping custom Linux GPIO translation caused some Intel_Strago based Chromebooks
> > with old firmware to detect GPIOs incorrectly. Add quirk which restores some code removed by
> > commit 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation").
>
> And on top what Mika asked already, is it a real problem to update firmware?

Even if we qualify new firmware (which is an involved process) how do
you communicate to users that are not running Chrome OS that they need
to update firmware? So no, we have to work around issues in existing
firmware even if there is newer one that might address the issue.

Thanks.

--
Dmitry

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-06  8:31 ` Mika Westerberg
@ 2020-02-06 22:26   ` Michał Stanek
  2020-02-07  7:56     ` Mika Westerberg
  0 siblings, 1 reply; 12+ messages in thread
From: Michał Stanek @ 2020-02-06 22:26 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, andriy.shevchenko, Linus Walleij,
	bgolaszewski, rafael.j.wysocki, Dmitry Torokhov

On Thu, Feb 6, 2020 at 9:31 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Wed, Feb 05, 2020 at 08:48:04PM +0100, Michal Stanek wrote:
> > Dropping custom Linux GPIO translation caused some Intel_Strago based Chromebooks
> > with old firmware to detect GPIOs incorrectly. Add quirk which restores some code removed by
> > commit 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation").
>
> Hi,
>
> Can you elaborate this? I was under the impression that all the
> different Strago systems have been already worked around by patches from
> Dmitry (Cc'd).

Hi Mika,

The previous patches from Dmitry handled IRQ numbering, here we have a
similar issue with GPIO to pin translation - hardcoded values in FW
which do not agree with the (non-consecutive) numbering in newer
kernels.

> What GPIO(s) we are talking about and how does it show up to the user?

As an example, the issue manifests itself when you run 'crossystem
wpsw_cur'. On my Kefka it incorrectly reports the value as 1 instead
of 0 when the write protect screw is removed.


> > +             /*
> > +              * Some Braswell based Google Chromebooks need custom ACPI GPIO
> > +              * number translation due to hardcoded GPIO numbers in firmware.
> > +              */
> > +             .ident = "Intel_Strago based Chromebooks (All models)",
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> > +                     DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_Strago"),
>
> Same comment here. Are you saying all Stragos are broken in the current
> mainline?

Yes, I believe all Braswell based Chromebooks are affected.
Yes, I believe all Braswell based Chromebooks are affected.

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-06 22:26   ` Michał Stanek
@ 2020-02-07  7:56     ` Mika Westerberg
  2020-02-08 18:43       ` Michał Stanek
  0 siblings, 1 reply; 12+ messages in thread
From: Mika Westerberg @ 2020-02-07  7:56 UTC (permalink / raw)
  To: Michał Stanek
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, andriy.shevchenko, Linus Walleij,
	bgolaszewski, rafael.j.wysocki, Dmitry Torokhov

On Thu, Feb 06, 2020 at 11:26:54PM +0100, Michał Stanek wrote:
> On Thu, Feb 6, 2020 at 9:31 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Wed, Feb 05, 2020 at 08:48:04PM +0100, Michal Stanek wrote:
> > > Dropping custom Linux GPIO translation caused some Intel_Strago based Chromebooks
> > > with old firmware to detect GPIOs incorrectly. Add quirk which restores some code removed by
> > > commit 03c4749dd6c7ff94 ("gpio / ACPI: Drop unnecessary ACPI GPIO to Linux GPIO translation").
> >
> > Hi,
> >
> > Can you elaborate this? I was under the impression that all the
> > different Strago systems have been already worked around by patches from
> > Dmitry (Cc'd).
> 
> Hi Mika,
> 
> The previous patches from Dmitry handled IRQ numbering, here we have a
> similar issue with GPIO to pin translation - hardcoded values in FW
> which do not agree with the (non-consecutive) numbering in newer
> kernels.

Hmm, so instead of passing GpioIo/GpioInt resources to devices the
firmware uses some hard-coded Linux GPIO numbering scheme? Would you
able to share the exact firmware description where this happens?

> > What GPIO(s) we are talking about and how does it show up to the user?
> 
> As an example, the issue manifests itself when you run 'crossystem
> wpsw_cur'. On my Kefka it incorrectly reports the value as 1 instead
> of 0 when the write protect screw is removed.

Is it poking GPIOs directly through sysfs relying the Linux GPIO
numbering (which can change and is fragile anyway)?

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-07  7:56     ` Mika Westerberg
@ 2020-02-08 18:43       ` Michał Stanek
  2020-02-10 10:14         ` Mika Westerberg
  2020-02-10 12:13         ` Linus Walleij
  0 siblings, 2 replies; 12+ messages in thread
From: Michał Stanek @ 2020-02-08 18:43 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, andriy.shevchenko, Linus Walleij,
	bgolaszewski, rafael.j.wysocki, Dmitry Torokhov

> >
> > Hi Mika,
> >
> > The previous patches from Dmitry handled IRQ numbering, here we have a
> > similar issue with GPIO to pin translation - hardcoded values in FW
> > which do not agree with the (non-consecutive) numbering in newer
> > kernels.
>
> Hmm, so instead of passing GpioIo/GpioInt resources to devices the
> firmware uses some hard-coded Linux GPIO numbering scheme? Would you
> able to share the exact firmware description where this happens?

Actually it is a GPIO offset in ACPI tables for Braswell that was
hardcoded in the old firmware to match the previous (consecutive)
Linux GPIO numbering.

> > > What GPIO(s) we are talking about and how does it show up to the user?
> >
> > As an example, the issue manifests itself when you run 'crossystem
> > wpsw_cur'. On my Kefka it incorrectly reports the value as 1 instead
> > of 0 when the write protect screw is removed.
>
> Is it poking GPIOs directly through sysfs relying the Linux GPIO
> numbering (which can change and is fragile anyway)?

I believe so, yes.

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-08 18:43       ` Michał Stanek
@ 2020-02-10 10:14         ` Mika Westerberg
  2020-03-10 14:12           ` Michał Stanek
  2020-02-10 12:13         ` Linus Walleij
  1 sibling, 1 reply; 12+ messages in thread
From: Mika Westerberg @ 2020-02-10 10:14 UTC (permalink / raw)
  To: Michał Stanek
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, andriy.shevchenko, Linus Walleij,
	bgolaszewski, rafael.j.wysocki, Dmitry Torokhov

On Sat, Feb 08, 2020 at 07:43:24PM +0100, Michał Stanek wrote:
> > >
> > > Hi Mika,
> > >
> > > The previous patches from Dmitry handled IRQ numbering, here we have a
> > > similar issue with GPIO to pin translation - hardcoded values in FW
> > > which do not agree with the (non-consecutive) numbering in newer
> > > kernels.
> >
> > Hmm, so instead of passing GpioIo/GpioInt resources to devices the
> > firmware uses some hard-coded Linux GPIO numbering scheme? Would you
> > able to share the exact firmware description where this happens?
> 
> Actually it is a GPIO offset in ACPI tables for Braswell that was
> hardcoded in the old firmware to match the previous (consecutive)
> Linux GPIO numbering.

Can you share the ACPI tables and point me to the GPIO that is using
Linux number?

> > > > What GPIO(s) we are talking about and how does it show up to the user?
> > >
> > > As an example, the issue manifests itself when you run 'crossystem
> > > wpsw_cur'. On my Kefka it incorrectly reports the value as 1 instead
> > > of 0 when the write protect screw is removed.
> >
> > Is it poking GPIOs directly through sysfs relying the Linux GPIO
> > numbering (which can change and is fragile anyway)?
> 
> I believe so, yes.

This is something that should be fixed in userspace. Using global Linux
GPIO or IRQ numbers is fragile and source of issues like this. There are
correct ways of using GPIOs from userspace: in case of sysfs, you can
find the base of the chip and then user relative numbering against it or
switch to use libgpiod that does the same but uses the newer char
device. Both cases the GPIO number are relative against the GPIO chip so
they work even if global Linux GPIO numbering changes.

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-08 18:43       ` Michał Stanek
  2020-02-10 10:14         ` Mika Westerberg
@ 2020-02-10 12:13         ` Linus Walleij
  1 sibling, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2020-02-10 12:13 UTC (permalink / raw)
  To: Michał Stanek
  Cc: Mika Westerberg, open list:GPIO SUBSYSTEM,
	ACPI Devel Maling List, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, Andy Shevchenko, Bartosz Golaszewski,
	Rafael J. Wysocki, Dmitry Torokhov

On Sat, Feb 8, 2020 at 7:43 PM Michał Stanek <mst@semihalf.com> wrote:

> Actually it is a GPIO offset in ACPI tables for Braswell that was
> hardcoded in the old firmware to match the previous (consecutive)
> Linux GPIO numbering.

That's bad practice by the firmware authors. (Sad face)

We have enough problem saving ourselves from our own
legacy ABIs without someone handling out more guns to shoot
oneself in the foot with. :(

I'm also speaking on behalf of Microsoft and others in this I
think. In the past we would find Windowsisms in the BIOSes
and get really annoyed that we had to work around them.
Now there are Linuxisms in the BIOS, that's not any better.

Yours,
Linus Walleij

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-02-10 10:14         ` Mika Westerberg
@ 2020-03-10 14:12           ` Michał Stanek
  2020-03-10 14:49             ` Mika Westerberg
  0 siblings, 1 reply; 12+ messages in thread
From: Michał Stanek @ 2020-03-10 14:12 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, andriy.shevchenko, Linus Walleij,
	bgolaszewski, rafael.j.wysocki, Dmitry Torokhov

On Mon, Feb 10, 2020 at 11:14 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Sat, Feb 08, 2020 at 07:43:24PM +0100, Michał Stanek wrote:
> > > >
> > > > Hi Mika,
> > > >
> > > > The previous patches from Dmitry handled IRQ numbering, here we have a
> > > > similar issue with GPIO to pin translation - hardcoded values in FW
> > > > which do not agree with the (non-consecutive) numbering in newer
> > > > kernels.
> > >
> > > Hmm, so instead of passing GpioIo/GpioInt resources to devices the
> > > firmware uses some hard-coded Linux GPIO numbering scheme? Would you
> > > able to share the exact firmware description where this happens?
> >
> > Actually it is a GPIO offset in ACPI tables for Braswell that was
> > hardcoded in the old firmware to match the previous (consecutive)
> > Linux GPIO numbering.
>
> Can you share the ACPI tables and point me to the GPIO that is using
> Linux number?

I think this is the one:
https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/%2B/286534/2/src/mainboard/google/cyan/acpi/chromeos.asl

On Kefka the sysfs GPIO number for wpsw_cur was gpio392 before the
translation change occurred in Linux.

> > > > > What GPIO(s) we are talking about and how does it show up to the user?
> > > >
> > > > As an example, the issue manifests itself when you run 'crossystem
> > > > wpsw_cur'. On my Kefka it incorrectly reports the value as 1 instead
> > > > of 0 when the write protect screw is removed.
> > >
> > > Is it poking GPIOs directly through sysfs relying the Linux GPIO
> > > numbering (which can change and is fragile anyway)?
> >
> > I believe so, yes.
>
> This is something that should be fixed in userspace. Using global Linux
> GPIO or IRQ numbers is fragile and source of issues like this. There are
> correct ways of using GPIOs from userspace: in case of sysfs, you can
> find the base of the chip and then user relative numbering against it or
> switch to use libgpiod that does the same but uses the newer char
> device. Both cases the GPIO number are relative against the GPIO chip so
> they work even if global Linux GPIO numbering changes.

I analyzed crossystem source code and it looks like it is doing
exactly what you're saying without any hardcoded assumptions. It gets
the absolute offset of the GPIO pin from sysfs using its ACPI
identifier, then it subtracts the base offset of the GPIO bank from it
and the result is added to the bank's gpiochip%d number as it shows up
in sysfs. The result is what is used to export and read the state of
the pin.

With the newer kernel the gpiochip%d number is different so crossystem
ends up reading the wrong pin.

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-03-10 14:12           ` Michał Stanek
@ 2020-03-10 14:49             ` Mika Westerberg
  2020-03-25 22:50               ` Linus Walleij
  0 siblings, 1 reply; 12+ messages in thread
From: Mika Westerberg @ 2020-03-10 14:49 UTC (permalink / raw)
  To: Michał Stanek
  Cc: linux-gpio, linux-acpi, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, andriy.shevchenko, Linus Walleij,
	bgolaszewski, rafael.j.wysocki, Dmitry Torokhov

On Tue, Mar 10, 2020 at 03:12:00PM +0100, Michał Stanek wrote:
> On Mon, Feb 10, 2020 at 11:14 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Sat, Feb 08, 2020 at 07:43:24PM +0100, Michał Stanek wrote:
> > > > >
> > > > > Hi Mika,
> > > > >
> > > > > The previous patches from Dmitry handled IRQ numbering, here we have a
> > > > > similar issue with GPIO to pin translation - hardcoded values in FW
> > > > > which do not agree with the (non-consecutive) numbering in newer
> > > > > kernels.
> > > >
> > > > Hmm, so instead of passing GpioIo/GpioInt resources to devices the
> > > > firmware uses some hard-coded Linux GPIO numbering scheme? Would you
> > > > able to share the exact firmware description where this happens?
> > >
> > > Actually it is a GPIO offset in ACPI tables for Braswell that was
> > > hardcoded in the old firmware to match the previous (consecutive)
> > > Linux GPIO numbering.
> >
> > Can you share the ACPI tables and point me to the GPIO that is using
> > Linux number?
> 
> I think this is the one:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/%2B/286534/2/src/mainboard/google/cyan/acpi/chromeos.asl
> 
> On Kefka the sysfs GPIO number for wpsw_cur was gpio392 before the
> translation change occurred in Linux.

But that table does not seem to have any GPIO numbers in it.

> > > > > > What GPIO(s) we are talking about and how does it show up to the user?
> > > > >
> > > > > As an example, the issue manifests itself when you run 'crossystem
> > > > > wpsw_cur'. On my Kefka it incorrectly reports the value as 1 instead
> > > > > of 0 when the write protect screw is removed.
> > > >
> > > > Is it poking GPIOs directly through sysfs relying the Linux GPIO
> > > > numbering (which can change and is fragile anyway)?
> > >
> > > I believe so, yes.
> >
> > This is something that should be fixed in userspace. Using global Linux
> > GPIO or IRQ numbers is fragile and source of issues like this. There are
> > correct ways of using GPIOs from userspace: in case of sysfs, you can
> > find the base of the chip and then user relative numbering against it or
> > switch to use libgpiod that does the same but uses the newer char
> > device. Both cases the GPIO number are relative against the GPIO chip so
> > they work even if global Linux GPIO numbering changes.
> 
> I analyzed crossystem source code and it looks like it is doing
> exactly what you're saying without any hardcoded assumptions. It gets
> the absolute offset of the GPIO pin from sysfs using its ACPI
> identifier, then it subtracts the base offset of the GPIO bank from it
> and the result is added to the bank's gpiochip%d number as it shows up
> in sysfs. The result is what is used to export and read the state of
> the pin.
> 
> With the newer kernel the gpiochip%d number is different so crossystem
> ends up reading the wrong pin.

Hmm, so gpiochipX is also not considered a stable number. It is based on
ARCH_NR_GPIOS which may change. So if the userspace is relaying certain GPIO
chip is always gpichip200 for example then it is wrong.

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

* Re: [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers
  2020-03-10 14:49             ` Mika Westerberg
@ 2020-03-25 22:50               ` Linus Walleij
  0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2020-03-25 22:50 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Michał Stanek, open list:GPIO SUBSYSTEM,
	ACPI Devel Maling List, linux-kernel, stanekm, stable,
	Marcin Wojtas, levinale, Andy Shevchenko, Bartosz Golaszewski,
	Rafael J. Wysocki, Dmitry Torokhov

On Tue, Mar 10, 2020 at 3:49 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> > With the newer kernel the gpiochip%d number is different so crossystem
> > ends up reading the wrong pin.
>
> Hmm, so gpiochipX is also not considered a stable number. It is based on
> ARCH_NR_GPIOS which may change. So if the userspace is relaying certain GPIO
> chip is always gpichip200 for example then it is wrong.

Yes it has always been clear that sysfs is what one shall use for establishing
the topology of the hardware, so /sys/bus/gpiochip etc.

For example on my laptop:
$ pwd
/sys/bus/gpio/devices
$ ls -al
total 0
drwxr-xr-x. 2 root root 0 25 mar 23.49 .
drwxr-xr-x. 4 root root 0 25 mar 23.49 ..
lrwxrwxrwx. 1 root root 0 25 mar 23.49 gpiochip0 ->
../../../devices/pci0000:00/INT344B:00/gpiochip0

Here we see that this gpiochip is on this PCI card and so on.

Linus

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

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 19:48 [PATCH] pinctrl: cherryview: Add quirk with custom translation of ACPI GPIO numbers Michal Stanek
2020-02-06  8:31 ` Mika Westerberg
2020-02-06 22:26   ` Michał Stanek
2020-02-07  7:56     ` Mika Westerberg
2020-02-08 18:43       ` Michał Stanek
2020-02-10 10:14         ` Mika Westerberg
2020-03-10 14:12           ` Michał Stanek
2020-03-10 14:49             ` Mika Westerberg
2020-03-25 22:50               ` Linus Walleij
2020-02-10 12:13         ` Linus Walleij
2020-02-06  9:25 ` Andy Shevchenko
2020-02-06 18:31   ` Dmitry Torokhov

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git