[v2] wifi: ath9k: Obtain system GPIOS from descriptors

The ath9k has an odd use of system-wide GPIOs: if the chip
does not have internal GPIO capability, it will try to obtain a
GPIO line from the system GPIO controller:

  if (BIT(gpio) & ah->caps.gpio_mask)
        ath9k_hw_gpio_cfg_wmac(...);
  else if (AR_SREV_SOC(ah))
        ath9k_hw_gpio_cfg_soc(ah, gpio, out, label);

Where ath9k_hw_gpio_cfg_soc() will attempt to issue
gpio_request_one() passing the local GPIO number of the controller
(0..31) to gpio_request_one().

This is somewhat peculiar and possibly even dangerous: there is
nowadays no guarantee of the numbering of these system-wide
GPIOs, and assuming that GPIO 0..31 as used by ath9k would
correspond to GPIOs 0..31 on the system as a whole seems a bit
wild.

Register all 32 GPIOs at index 0..31 directly in the ATH79K
GPIO driver and associate with WIFI if and only if we are probing
ATH79K wifi from the AHB bus (used for SoCs).

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- Define all the descriptors directly in the ATH79K
  GPIO driver in case the driver want to request them directly.
- Link to v1: https://lore.kernel.org/r/20240131-descriptors-wireless-v1-0-e1c7c5d68746@linaro.org
---
 drivers/gpio/gpio-ath79.c           | 47 ++++++++++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath9k/hw.c | 29 ++++++++++++-----------
 drivers/net/wireless/ath/ath9k/hw.h |  3 ++-
 3 files changed, 63 insertions(+), 16 deletions(-)


---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240122-descriptors-wireless-b8da95dcab35

Best regards,

On Tue, Apr 23, 2024 at 02:12:33PM +0200, Linus Walleij wrote:
> The ath9k has an odd use of system-wide GPIOs: if the chip
> does not have internal GPIO capability, it will try to obtain a
> GPIO line from the system GPIO controller:
> 
>   if (BIT(gpio) & ah->caps.gpio_mask)
>         ath9k_hw_gpio_cfg_wmac(...);
>   else if (AR_SREV_SOC(ah))
>         ath9k_hw_gpio_cfg_soc(ah, gpio, out, label);
> 
> Where ath9k_hw_gpio_cfg_soc() will attempt to issue
> gpio_request_one() passing the local GPIO number of the controller
> (0..31) to gpio_request_one().
> 
> This is somewhat peculiar and possibly even dangerous: there is
> nowadays no guarantee of the numbering of these system-wide
> GPIOs, and assuming that GPIO 0..31 as used by ath9k would
> correspond to GPIOs 0..31 on the system as a whole seems a bit
> wild.
> 
> Register all 32 GPIOs at index 0..31 directly in the ATH79K
> GPIO driver and associate with WIFI if and only if we are probing
> ATH79K wifi from the AHB bus (used for SoCs).

...

> +/*
> + * This registers all of the ath79k GPIOs as descriptors to be picked
> + * directly from the ATH79K wifi driver if the two are jitted together
> + * in the same SoC.
> + */
> +#define ATH79K_WIFI_DESCS 32
> +static int ath79_gpio_register_wifi_descriptors(struct device *dev,
> +						const char *label)
> +{
> +	struct gpiod_lookup_table *lookup;
> +	int i;

unsigned ?

> +	/* Create a gpiod lookup using gpiochip-local offsets + 1 for NULL */
> +        lookup = devm_kzalloc(dev,
> +			      struct_size(lookup, table, ATH79K_WIFI_DESCS + 1),
> +			      GFP_KERNEL);

> +

Besides unneeded blank line the above has a broken indentation.

> +	if (!lookup)
> +		return -ENOMEM;
> +
> +	lookup->dev_id = "ath9k";
> +
> +	for (i = 0; i < ATH79K_WIFI_DESCS; i++) {

> +		lookup->table[i] = (struct gpiod_lookup)

This is not needed as GPIO_LOOKUP_IDX() is a compound literal.

> +			GPIO_LOOKUP_IDX(label, 0, NULL, i,
> +					GPIO_ACTIVE_HIGH);

Hence:

		lookup->table[i] =
			GPIO_LOOKUP_IDX(label, 0, NULL, i, GPIO_ACTIVE_HIGH);

> +	}
> +
> +	gpiod_add_lookup_table(lookup);
> +
> +	return 0;
> +}

...

> +	/* Obtains a system specific GPIO descriptor from another GPIO controller */
> +	gpiod = devm_gpiod_get_index(ah->dev, NULL, gpio, flags);

> +

Unneeded blank line.

> +	if (IS_ERR(gpiod)) {
> +		err = PTR_ERR(gpiod);
>  		ath_err(ath9k_hw_common(ah), "request GPIO%d failed:%d\n",
>  			gpio, err);
>  		return;
>  	}

Linus Walleij <linus.walleij@linaro.org> writes:

> The ath9k has an odd use of system-wide GPIOs: if the chip
> does not have internal GPIO capability, it will try to obtain a
> GPIO line from the system GPIO controller:
>
>   if (BIT(gpio) & ah->caps.gpio_mask)
>         ath9k_hw_gpio_cfg_wmac(...);
>   else if (AR_SREV_SOC(ah))
>         ath9k_hw_gpio_cfg_soc(ah, gpio, out, label);
>
> Where ath9k_hw_gpio_cfg_soc() will attempt to issue
> gpio_request_one() passing the local GPIO number of the controller
> (0..31) to gpio_request_one().
>
> This is somewhat peculiar and possibly even dangerous: there is
> nowadays no guarantee of the numbering of these system-wide
> GPIOs, and assuming that GPIO 0..31 as used by ath9k would
> correspond to GPIOs 0..31 on the system as a whole seems a bit
> wild.
>
> Register all 32 GPIOs at index 0..31 directly in the ATH79K
> GPIO driver and associate with WIFI if and only if we are probing
> ATH79K wifi from the AHB bus (used for SoCs).
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes in v2:
> - Define all the descriptors directly in the ATH79K
>   GPIO driver in case the driver want to request them directly.
> - Link to v1: https://lore.kernel.org/r/20240131-descriptors-wireless-v1-0-e1c7c5d68746@linaro.org

Linus, via which tree should this go?

Hi Linus,

> The ath9k has an odd use of system-wide GPIOs: if the chip
> does not have internal GPIO capability, it will try to obtain a
> GPIO line from the system GPIO controller:
> 
>   if (BIT(gpio) & ah->caps.gpio_mask)
>         ath9k_hw_gpio_cfg_wmac(...);
>   else if (AR_SREV_SOC(ah))
>         ath9k_hw_gpio_cfg_soc(ah, gpio, out, label);
> 
> Where ath9k_hw_gpio_cfg_soc() will attempt to issue
> gpio_request_one() passing the local GPIO number of the controller
> (0..31) to gpio_request_one().
> 
> This is somewhat peculiar and possibly even dangerous: there is
> nowadays no guarantee of the numbering of these system-wide
> GPIOs, and assuming that GPIO 0..31 as used by ath9k would
> correspond to GPIOs 0..31 on the system as a whole seems a bit
> wild.
> 
> Register all 32 GPIOs at index 0..31 directly in the ATH79K
> GPIO driver and associate with WIFI if and only if we are probing
> ATH79K wifi from the AHB bus (used for SoCs).

I don't know how likely it is today that this patch will get merged, but
it turned out to be useful for fixing an OpenWRT issue [1][2].  However,
the patch required some tweaking in order to make it work, so I assumed
it cannot hurt to provide some feedback on it.

> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes in v2:
> - Define all the descriptors directly in the ATH79K
>   GPIO driver in case the driver want to request them directly.
> - Link to v1: https://lore.kernel.org/r/20240131-descriptors-wireless-v1-0-e1c7c5d68746@linaro.org
> ---
>  drivers/gpio/gpio-ath79.c           | 47 ++++++++++++++++++++++++++++++++++++-
>  drivers/net/wireless/ath/ath9k/hw.c | 29 ++++++++++++-----------
>  drivers/net/wireless/ath/ath9k/hw.h |  3 ++-
>  3 files changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ath79.c b/drivers/gpio/gpio-ath79.c
> index f0c0c0f77eb0..f83ce0595ea8 100644
> --- a/drivers/gpio/gpio-ath79.c
> +++ b/drivers/gpio/gpio-ath79.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h> /* For WLAN GPIOs */
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/gpio-ath79.h>
>  #include <linux/of.h>
> @@ -222,6 +223,46 @@ static const struct of_device_id ath79_gpio_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, ath79_gpio_of_match);
>  
> +#if IS_ENABLED(CONFIG_ATH9K_AHB)
> +/*
> + * This registers all of the ath79k GPIOs as descriptors to be picked
> + * directly from the ATH79K wifi driver if the two are jitted together
> + * in the same SoC.
> + */
> +#define ATH79K_WIFI_DESCS 32
> +static int ath79_gpio_register_wifi_descriptors(struct device *dev,
> +						const char *label)
> +{
> +	struct gpiod_lookup_table *lookup;
> +	int i;
> +
> +	/* Create a gpiod lookup using gpiochip-local offsets + 1 for NULL */
> +        lookup = devm_kzalloc(dev,
> +			      struct_size(lookup, table, ATH79K_WIFI_DESCS + 1),
> +			      GFP_KERNEL);
> +
> +	if (!lookup)
> +		return -ENOMEM;
> +
> +	lookup->dev_id = "ath9k";

Since the devm_gpiod_get_index() call in ath9k_hw_gpio_cfg_soc() passes
ah->dev as the first argument, "ath9k" is not the string that
gpiod_find_lookup_table() will use for matching the lookup table;
instead, it will be the wireless device's name, e.g. "18100000.wmac" on
my router (which is built on Atheros 9344).  This causes
devm_gpiod_get_index() to return -ENOENT [3].

> +
> +	for (i = 0; i < ATH79K_WIFI_DESCS; i++) {
> +		lookup->table[i] = (struct gpiod_lookup)
> +			GPIO_LOOKUP_IDX(label, 0, NULL, i,

This sets the chip_hwnum member of every registered lookup table entry
to 0 (second GPIO_LOOKUP_IDX() argument), which causes all 32 GPIOs
registered here to be erroneously mapped to the GPIO chip's first line.
I believe the second argument for GPIO_LOOKUP_IDX() should also be 'i'
here - or at least that is what made the patch work for me (after fixing
the lookup table matching issue).

> +					GPIO_ACTIVE_HIGH);
> +	}
> +
> +	gpiod_add_lookup_table(lookup);
> +
> +	return 0;
> +}
> +#else
> +static int ath79_gpio_register_wifi_descriptors(struct device *dev,
> +						const char *label)
> +{
> +}
> +#endif
> +
>  static int ath79_gpio_probe(struct platform_device *pdev)
>  {
>  	struct ath79_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -291,7 +332,11 @@ static int ath79_gpio_probe(struct platform_device *pdev)
>  		girq->handler = handle_simple_irq;
>  	}
>  
> -	return devm_gpiochip_add_data(dev, &ctrl->gc, ctrl);
> +	err = devm_gpiochip_add_data(dev, &ctrl->gc, ctrl);
> +	if (err)
> +		return err;
> +
> +	return ath79_gpio_register_wifi_descriptors(dev, ctrl->gc.label);
>  }
>  
>  static struct platform_driver ath79_gpio_driver = {
> diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
> index 5982e0db45f9..ee6705836746 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.c
> +++ b/drivers/net/wireless/ath/ath9k/hw.c
> @@ -20,7 +20,7 @@
>  #include <linux/time.h>
>  #include <linux/bitops.h>
>  #include <linux/etherdevice.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <asm/unaligned.h>
>  
>  #include "hw.h"
> @@ -2727,19 +2727,25 @@ static void ath9k_hw_gpio_cfg_output_mux(struct ath_hw *ah, u32 gpio, u32 type)
>  static void ath9k_hw_gpio_cfg_soc(struct ath_hw *ah, u32 gpio, bool out,
>  				  const char *label)
>  {
> +	enum gpiod_flags flags = out ? GPIOD_OUT_LOW : GPIOD_IN;
> +	struct gpio_desc *gpiod;
>  	int err;
>  
> -	if (ah->caps.gpio_requested & BIT(gpio))
> +	if (ah->gpiods[gpio])
>  		return;
>  
> -	err = gpio_request_one(gpio, out ? GPIOF_OUT_INIT_LOW : GPIOF_IN, label);
> -	if (err) {
> +	/* Obtains a system specific GPIO descriptor from another GPIO controller */
> +	gpiod = devm_gpiod_get_index(ah->dev, NULL, gpio, flags);

Since using the resource-managed version of gpiod_get_index() requires
providing a valid pointer to a struct device as the first argument and
the name of that device is not going to be "ath9k", some other means of
matching this call with the lookup table registered in
ath79_gpio_register_wifi_descriptors() needs to be devised.

I resorted to the NULL-matching fallback in gpiod_find_lookup_table(),
which enables a lookup table with dev_id set to NULL to be matched for a
gpiod_get_index() call with dev also set to NULL, coupled with setting
con_id in all the lookup table entries and in the gpiod_get_index() call
to a matching string, e.g.:

	// in ath79_gpio_register_wifi_descriptors()

	for (i = 0; i < ATH79K_WIFI_DESCS; i++) {
		lookup->table[i] = (struct gpiod_lookup)
			GPIO_LOOKUP_IDX(label, i, "ath9k", i,
					GPIO_ACTIVE_HIGH);

	// in ath9k_hw_gpio_cfg_soc()

	gpiod = gpiod_get_index(NULL, "ath9k", gpio, flags);

This requires manually releasing the GPIO descriptor when the wireless
driver is done with it (because we're losing the benefits of using
resource-managed functions), so...

> +
> +	if (IS_ERR(gpiod)) {
> +		err = PTR_ERR(gpiod);
>  		ath_err(ath9k_hw_common(ah), "request GPIO%d failed:%d\n",
>  			gpio, err);
>  		return;
>  	}
>  
> -	ah->caps.gpio_requested |= BIT(gpio);
> +	gpiod_set_consumer_name(gpiod, label);
> +	ah->gpiods[gpio] = gpiod;
>  }
>  
>  static void ath9k_hw_gpio_cfg_wmac(struct ath_hw *ah, u32 gpio, bool out,
> @@ -2800,11 +2806,6 @@ void ath9k_hw_gpio_free(struct ath_hw *ah, u32 gpio)
>  		return;
>  
>  	WARN_ON(gpio >= ah->caps.num_gpio_pins);
> -
> -	if (ah->caps.gpio_requested & BIT(gpio)) {
> -		gpio_free(gpio);
> -		ah->caps.gpio_requested &= ~BIT(gpio);
> -	}

...ath9k_hw_gpio_free() would still need a bit like this:

	if (ah->gpiods[gpio]) {
		gpiod_put(ah->gpiods[gpio]);
		ah->gpiods[gpio] = NULL;
	}

I don't know if such an approach is appropriate, but it did at least
make the patch work for me.

Hope this helps,

[1] https://github.com/openwrt/openwrt/pull/17402#issuecomment-2566157016
[2] https://github.com/openwrt/openwrt/pull/17402#issuecomment-2566763575
[3] https://github.com/openwrt/openwrt/pull/17445#issuecomment-2569439459