[ath9k-devel] [RFC 0/3] of: add common bindings to (de)activate IEEE 802.11 bands

Discussion:

Martin Blumenstingl

2016-10-02 22:50:56 UTC

There are at least two drivers (ath9k and mt76) out there, where
devicetree authors need to override the enabled bands.

For ath9k there is only one use-case: disabling a band which has been
incorrectly enabled by the vendor in the EEPROM (enabling a band is not
possible because the calibration data would be missing in the EEPROM).
The mt76 driver (currently pending for review) however allows enabling
and disabling the 2.4GHz and 5GHz band, see [0].

Based on the discussion of (earlier versions of) my ath9k devicetree
patch it was suggested [1] that we use enable- and disable- properties.
The current patch implements:
- bands can be enabled or disabled with the corresponding property
- if both (disable and enable) properties are given and a driver asks
whether the band is enabled then the logic will return false (= not
enabled, preferring the disabled flag)
- if both (disable and enable) properties are given and a driver asks
whether the band is disabled then the logic will return true (again,
preferring the disabled flag over the enabled flag)

We can still change the logic to do what the mt76 driver does (I am
fine with both solutions):
- property not available: driver decides which bands to enable
- property set to 0: disable the band
- property set to 1: enable the band

The new code has been integrated into ath9k to demonstrate how to use
it (with the benefit that the disable_2ghz and disable_5ghz settings
from the ath9k_platform_data can now also be configured via .dts).

open questions/decisions needed:
- where to place this new code? I put it into drivers/of/of_ieee80211
because that's where most other functions live.
However, I found that this makes backporting the code harder (using
wireless-backports from the driver-backports project [2])
- we need a decision whether we want to go with two flags for each
band (enable-ieee80211-band and disable-ieee80211-band) or if we
prefer the solution from the mt76 driver (which means that the
property for a band is absent for auto-detection, or
ieee80211-band-enabled = <0/1> is specified. we could also move
the 0 and 1 to a header file of course to make it easer to read,
such as IEEE80211_BAND_ENABLED and IEEE80211_BAND_DISABLED)

[0] https://marc.info/?l=linux-wireless&m=147524754427890&w=2
[1] https://marc.info/?l=linux-wireless&m=147375173804817&w=2
[2] https://backports.wiki.kernel.org/index.php/Main_Page

Martin Blumenstingl (3):
Documentation: dt-bindings: add IEEE 802.11 binding documentation
of: add IEEE 802.11 device configuration support code
ath9k: add OF configuration to disable the 2.4GHz or 5GHz band

.../devicetree/bindings/net/wireless/ieee80211.txt | 12 ++++
.../devicetree/bindings/net/wireless/qca,ath9k.txt | 2 +
drivers/net/wireless/ath/ath9k/init.c | 4 ++
drivers/of/Kconfig | 4 ++
drivers/of/Makefile | 1 +
drivers/of/of_ieee80211.c | 83 ++++++++++++++++++++++
include/linux/of_ieee80211.h | 38 ++++++++++
7 files changed, 144 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/wireless/ieee80211.txt
create mode 100644 drivers/of/of_ieee80211.c
create mode 100644 include/linux/of_ieee80211.h

--
2.10.0

Martin Blumenstingl

2016-10-02 22:50:57 UTC

Permalink

This adds the documentation for the generic IEEE 802.111 binding, which
currently allows enabling and disabling the 2.4GHz and 5GHz band.

Signed-off-by: Martin Blumenstingl <***@googlemail.com>
---
Documentation/devicetree/bindings/net/wireless/ieee80211.txt | 12 ++++++++++++
1 file changed, 12 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/wireless/ieee80211.txt

diff --git a/Documentation/devicetree/bindings/net/wireless/ieee80211.txt b/Documentation/devicetree/bindings/net/wireless/ieee80211.txt
new file mode 100644
index 0000000..41fab97
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/ieee80211.txt
@@ -0,0 +1,12 @@
+The following properties are common to the IEEE 802.11 controllers:
+
+- enable-ieee80211-2ghz: indicates that the 2.4GHz frequency band should be
+ enabled for this device. This must not be used together with the
+ "disable-ieee80211-2ghz" at the same time.
+- enable-ieee80211-5ghz: indicates that the 5GHz frequency band should be
+ enabled for this device. This must not be used together with the
+ "disable-ieee80211-5ghz" at the same time.
+- disable-ieee80211-2ghz: indicates that the 2.4GHz frequency band should be
+ disabled for this device.
+- disable-ieee80211-5ghz: indicates that the 5GHz frequency band should be
+ disabled for this device

--
2.10.0

Martin Blumenstingl

2016-10-02 22:50:58 UTC

Permalink

Add support for parsing the device tree of IEEE 802.11 devices.

Some drivers (for example ath9k) already have custom platform_data
code to enable/disable specific bands. There are some other drivers
(for example the currently out-of-tree mt76 driver) which need similar
logic.
Thus we provide a generic implementation for IEEE 802.11 device
configuration OF properties, which can be used by all drivers.

Signed-off-by: Martin Blumenstingl <***@googlemail.com>
---
drivers/of/Kconfig | 4 +++
drivers/of/Makefile | 1 +
drivers/of/of_ieee80211.c | 83 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_ieee80211.h | 38 ++++++++++++++++++++
4 files changed, 126 insertions(+)
create mode 100644 drivers/of/of_ieee80211.c
create mode 100644 include/linux/of_ieee80211.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index bc07ad3..3ef5e12 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -116,4 +116,8 @@ config OF_OVERLAY
config OF_NUMA
bool

+config OF_IEEE80211
+ depends on CFG80211 || WLAN
+ def_bool y
+
endif # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index d7efd9d..00bd746 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -14,5 +14,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
obj-$(CONFIG_OF_RESOLVE) += resolver.o
obj-$(CONFIG_OF_OVERLAY) += overlay.o
obj-$(CONFIG_OF_NUMA) += of_numa.o
+obj-$(CONFIG_OF_IEEE80211) += of_ieee80211.o

obj-$(CONFIG_OF_UNITTEST) += unittest-data/
diff --git a/drivers/of/of_ieee80211.c b/drivers/of/of_ieee80211.c
new file mode 100644
index 0000000..f529058
--- /dev/null
+++ b/drivers/of/of_ieee80211.c
@@ -0,0 +1,83 @@
+/*
+ * OF helpers for IEEE 802.11 devices.
+ *
+ * Copyright (C) 2016 Martin Blumenstingl <***@googlemail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This file contains the code used to make IRQ descriptions in the
+ * device tree to actual irq numbers on an interrupt controller
+ * driver.
+ */
+#include <linux/export.h>
+#include <linux/of.h>
+#include <linux/of_ieee80211.h>
+
+/**
+ * of_ieee80211_is_2ghz_enabled - Check if the IEEE 802.11 2.4GHz frequency
+ * band is enabled for the given device_node
+ * @np: Pointer to the given device_node
+ *
+ * When the the 2.4GHz band is defined in an conflicting state (both enabled
+ * and disabled are set) then we leave it disabled.
+ */
+bool of_ieee80211_is_2ghz_enabled(struct device_node *np)
+{
+ bool enabled = of_property_read_bool(np, "enable-ieee80211-2ghz");
+
+ if (enabled && of_ieee80211_is_2ghz_disabled(np)) {
+ pr_err("%s: conflicting configuration for the 2.4GHz band\n",
+ of_node_full_name(np));
+ return false;
+ }
+
+ return enabled;
+}
+EXPORT_SYMBOL(of_ieee80211_is_2ghz_enabled);
+
+/**
+ * of_ieee80211_is_5ghz_enabled - Check if the IEEE 802.11 5GHz frequency band
+ * is enabled for the given device_node
+ * @np: Pointer to the given device_node
+ *
+ * When the the 5GHz band is defined in an conflicting state (both enabled
+ * and disabled are set) then we leave it disabled.
+ */
+bool of_ieee80211_is_5ghz_enabled(struct device_node *np)
+{
+ bool enabled = of_property_read_bool(np, "enable-ieee80211-5ghz");
+
+ if (enabled && of_ieee80211_is_2ghz_disabled(np)) {
+ pr_err("%s: conflicting configuration for the 5GHz band\n",
+ of_node_full_name(np));
+ return false;
+ }
+
+ return enabled;
+}
+EXPORT_SYMBOL(of_ieee80211_is_5ghz_enabled);
+
+/**
+ * of_ieee80211_is_2ghz_disabled - Check if the IEEE 802.11 2.4GHz frequency
+ * band is disabled for the given device_node
+ * @np: Pointer to the given device_node
+ */
+bool of_ieee80211_is_2ghz_disabled(struct device_node *np)
+{
+ return of_property_read_bool(np, "disable-ieee80211-2ghz");
+}
+EXPORT_SYMBOL(of_ieee80211_is_2ghz_disabled);
+
+/**
+ * of_ieee80211_is_5ghz_disabled - Check if the IEEE 802.11 5GHz frequency
+ * band is disabled for the given device_node
+ * @np: Pointer to the given device_node
+ */
+bool of_ieee80211_is_5ghz_disabled(struct device_node *np)
+{
+ return of_property_read_bool(np, "disable-ieee80211-5ghz");
+}
+EXPORT_SYMBOL(of_ieee80211_is_5ghz_disabled);
diff --git a/include/linux/of_ieee80211.h b/include/linux/of_ieee80211.h
new file mode 100644
index 0000000..a25a66e
--- /dev/null
+++ b/include/linux/of_ieee80211.h
@@ -0,0 +1,38 @@
+#ifndef __OF_IEEE80211_H
+#define __OF_IEEE80211_H
+
+struct device_node;
+
+#ifdef CONFIG_OF_IEEE80211
+
+bool of_ieee80211_is_2ghz_enabled(struct device_node *np);
+bool of_ieee80211_is_5ghz_enabled(struct device_node *np);
+
+bool of_ieee80211_is_2ghz_disabled(struct device_node *np);
+bool of_ieee80211_is_5ghz_disabled(struct device_node *np);
+
+#else /* CONFIG_OF_IEEE80211 */
+
+static inline bool of_ieee80211_is_2ghz_enabled(struct device_node *np)
+{
+ return false;
+}
+
+static inline bool of_ieee80211_is_5ghz_enabled(struct device_node *np)
+{
+ return false;
+}
+
+static inline bool of_ieee80211_is_2ghz_disabled(struct device_node *np)
+{
+ return false;
+}
+
+static inline bool of_ieee80211_is_5ghz_disabled(struct device_node *np)
+{
+ return false;
+}
+
+#endif /* CONFIG_OF_IEEE80211 */
+
+#endif /* __OF_IEEE80211_H */

--
2.10.0

Martin Blumenstingl

2016-10-02 22:50:59 UTC

Permalink

Some devices are shipped with EEPROMs where a band is enabled which is
not supported by the actual hardware. Allow disabling the affected
bands using the new generic IEEE 802.11 bindings.

This is the OF equivalent to using ath9k_platform_data's disable_2ghz
and disable_5ghz attributes.

Signed-off-by: Martin Blumenstingl <***@googlemail.com>
---
Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt | 2 ++
drivers/net/wireless/ath/ath9k/init.c | 4 ++++
2 files changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
index 9b58ede..042319a 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
@@ -18,6 +18,8 @@ Optional properties:
kernel firmware loader).
- mac-address: See ethernet.txt in the parent directory
- local-mac-address: See ethernet.txt in the parent directory
+- disable-ieee80211-2ghz: See ieee80211.txt in the current directory
+- disable-ieee80211-5ghz: See ieee80211.txt in the current directory

In this example, the node is defined as child node of the PCI controller:
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index b7c8ff9..8b3f906 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -21,6 +21,7 @@
#include <linux/ath9k_platform.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_ieee80211.h>
#include <linux/of_net.h>
#include <linux/relay.h>
#include <net/ieee80211_radiotap.h>
@@ -572,6 +573,9 @@ static int ath9k_of_init(struct ath_softc *sc)

ath_dbg(common, CONFIG, "parsing configuration from OF node\n");

+ ah->disable_2ghz = of_ieee80211_is_2ghz_disabled(np);
+ ah->disable_5ghz = of_ieee80211_is_5ghz_disabled(np);
+
if (of_property_read_bool(np, "qca,no-eeprom")) {
/* ath9k-eeprom-<bus>-<id>.bin */
scnprintf(eeprom_name, sizeof(eeprom_name),

--
2.10.0

Martin Blumenstingl

2016-10-05 18:25:00 UTC

Permalink

On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl

Post by Martin Blumenstingl
There are at least two drivers (ath9k and mt76) out there, where
devicetree authors need to override the enabled bands.
For ath9k there is only one use-case: disabling a band which has been
incorrectly enabled by the vendor in the EEPROM (enabling a band is not
possible because the calibration data would be missing in the EEPROM).
The mt76 driver (currently pending for review) however allows enabling
and disabling the 2.4GHz and 5GHz band, see [0].
Based on the discussion of (earlier versions of) my ath9k devicetree
patch it was suggested [1] that we use enable- and disable- properties.
- bands can be enabled or disabled with the corresponding property
- if both (disable and enable) properties are given and a driver asks
whether the band is enabled then the logic will return false (= not
enabled, preferring the disabled flag)
- if both (disable and enable) properties are given and a driver asks
whether the band is disabled then the logic will return true (again,
preferring the disabled flag over the enabled flag)
We can still change the logic to do what the mt76 driver does (I am
- property not available: driver decides which bands to enable
- property set to 0: disable the band
- property set to 1: enable the band

I prefer this way. Really, I'd prefer just a boolean disable property.
I'm not sure why you need the enable. We usually have these tri-state
properties when you want not present to mean use the bootloader or
default setting. Try to make not present the most common case.

Felix: could you please give a few details why mt76 can not only
disable but also enable a specific band?
There is no specific comment in the sources [0] why this is needed.
All other drivers that I am aware of (ath9k, rt2x00) only allow
disabling a specific band, they never enable it (this has to be done
explicitly by the EEPROM).

Post by Martin Blumenstingl
The new code has been integrated into ath9k to demonstrate how to use
it (with the benefit that the disable_2ghz and disable_5ghz settings
from the ath9k_platform_data can now also be configured via .dts).
- where to place this new code? I put it into drivers/of/of_ieee80211
because that's where most other functions live.
However, I found that this makes backporting the code harder (using
wireless-backports from the driver-backports project [2])

We are generally moving subsystem specific code like this out of
drivers/of/, so please do that here. Maybe someone will get motivated
to move the other networking code out too.

OK, thanks for the hint.

[0] https://github.com/openwrt/mt76/blob/master/eeprom.c

Felix Fietkau

2016-10-05 18:36:27 UTC

Permalink

Post by Martin Blumenstingl

On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl

None of the devices use it at the moment, I probably added it when I
played with a device that had broken EEPROM data. I guess I decided to
do it this way because on many devices the band capability field simply
cannot be trusted.
I guess I would be okay with just having a disable property and adding
an enable property later only if we end up actually needing it.

- Felix

Felix Fietkau

2016-10-05 20:34:14 UTC

Permalink

Post by Felix Fietkau

Post by Martin Blumenstingl

On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl

If EEPROM is commonly wrong or missing, then seems like you want to
plan ahead and support both enable and disable.
The other approach I've mentioned before is just put raw EEPROM data
into DT to override the EEPROM. This would be better if there's a
large number of settings you need.

So far, other EEPROM settings didn't need to be changed.

- Felix

Martin Blumenstingl

2016-10-16 21:20:38 UTC

Permalink

Post by Felix Fietkau

Post by Martin Blumenstingl

On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl

If EEPROM is commonly wrong or missing, then seems like you want to
plan ahead and support both enable and disable.

if we want to plan ahead we could use two properties "ieee80211-2ghz"
and "ieee80211-5ghz", each of them allows the values: 0 = disabled, 1
= enabled, absent = auto-detect (for example based on the card's
EEPROM), any other value is invalid

How should our API look in this case?
Would we still have bool ..._enabled() and bool ..._disabled() - what
if the property is absent (would we add another boolean
ieee80211_is_[2,5]ghz_configured(np) to handle that)?
We could also introduce int ieee80211_get_[2,5]ghz_enabled(struct
device_node *np, bool *enabled) and leave it up to the caller?

Regards,
Martin