[RFC v2] ath9k: add devicetree support to ath9k

[RFC v2] ath9k: add devicetree support to ath9k

[Martin Blumenstingl]

This series adds support for configuring ath9k based devices via
devicetree. This was tested on PCI(e) based devices. This should work
for AHB based devices as well as soon as the ath79 platform is ready
to populate the ath9k wmac via devicetree.

This series depends on my previous series:
"ath9k: extend and improve handling of ath9k_platform_data"

changes in v1 -> v2:
This addresses all issues found by Christian Lamparter (thanks!), the
detailed changes are documented in each patch.

[ath9k-devel] [RFC v2] ath9k: add devicetree support to ath9k

[Martin Blumenstingl]

This series adds support for configuring ath9k based devices via
devicetree. This was tested on PCI(e) based devices. This should work
for AHB based devices as well as soon as the ath79 platform is ready
to populate the ath9k wmac via devicetree.

This series depends on my previous series:
"ath9k: extend and improve handling of ath9k_platform_data"

changes in v1 -> v2:
This addresses all issues found by Christian Lamparter (thanks!), the
detailed changes are documented in each patch.

[PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

Add documentation how devicetree can be used to configure ath9k based
devices.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
changes in v1 -> v2:
- use vendor prefix "qca" instead of "ath"
- extend the example so it includes the "compatible" property

 .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
new file mode 100644
index 0000000..bb78f68
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
@@ -0,0 +1,41 @@
+* Qualcomm Atheros ath9k wireless devices
+
+This node provides properties for configuring the ath9k wireless device. The
+node is expected to be specified as a child node of the PCI controller to
+which the wireless chip is connected.
+
+Required properties:
+- compatible: Should be "qca,ath9k"
+
+Optional properties:
+- reg: Address and length of the register set for the device.
+- qca,gpio-mask: The GPIO mask
+- qca,gpio-val: The GPIO value
+- qca,led-pin: The GPIO number to which the LED is connected
+- qca,led-active-high: The LED is active when the GPIO is HIGH
+- qca,clk-25mhz: Defines that at 25MHz clock is used
+- qca,eeprom-name: The name of the file which contains the EEPROM data (which
+			will be loaded via request_firmware)
+- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
+				swapping of the EEPROM data if required
+- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
+- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
+
+In this example, the node is defined as child node of the PCI controller.
+
+pci {
+	pcie@0 {
+		reg = <0 0 0 0 0>;
+		#interrupt-cells = <1>;
+		#size-cells = <2>;
+		#address-cells = <3>;
+		device_type = "pci";
+
+		ath9k@0,0 {
+			compatible = "qca,ath9k";
+			reg = <0 0 0 0 0>;
+			device_type = "pci";
+			qca,disable-5ghz;
+		};
+	};
+};
-- 
2.9.0

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

Add documentation how devicetree can be used to configure ath9k based
devices.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
changes in v1 -> v2:
- use vendor prefix "qca" instead of "ath"
- extend the example so it includes the "compatible" property

 .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
new file mode 100644
index 0000000..bb78f68
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
@@ -0,0 +1,41 @@
+* Qualcomm Atheros ath9k wireless devices
+
+This node provides properties for configuring the ath9k wireless device. The
+node is expected to be specified as a child node of the PCI controller to
+which the wireless chip is connected.
+
+Required properties:
+- compatible: Should be "qca,ath9k"
+
+Optional properties:
+- reg: Address and length of the register set for the device.
+- qca,gpio-mask: The GPIO mask
+- qca,gpio-val: The GPIO value
+- qca,led-pin: The GPIO number to which the LED is connected
+- qca,led-active-high: The LED is active when the GPIO is HIGH
+- qca,clk-25mhz: Defines that at 25MHz clock is used
+- qca,eeprom-name: The name of the file which contains the EEPROM data (which
+			will be loaded via request_firmware)
+- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
+				swapping of the EEPROM data if required
+- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
+- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
+
+In this example, the node is defined as child node of the PCI controller.
+
+pci {
+	pcie at 0 {
+		reg = <0 0 0 0 0>;
+		#interrupt-cells = <1>;
+		#size-cells = <2>;
+		#address-cells = <3>;
+		device_type = "pci";
+
+		ath9k at 0,0 {
+			compatible = "qca,ath9k";
+			reg = <0 0 0 0 0>;
+			device_type = "pci";
+			qca,disable-5ghz;
+		};
+	};
+};
-- 
2.9.0

Re: [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Mark Rutland]

On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
> Add documentation how devicetree can be used to configure ath9k based
> devices.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> changes in v1 -> v2:
> - use vendor prefix "qca" instead of "ath"
> - extend the example so it includes the "compatible" property
> 
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> new file mode 100644
> index 0000000..bb78f68
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -0,0 +1,41 @@
> +* Qualcomm Atheros ath9k wireless devices
> +
> +This node provides properties for configuring the ath9k wireless device. The
> +node is expected to be specified as a child node of the PCI controller to
> +which the wireless chip is connected.
> +
> +Required properties:
> +- compatible: Should be "qca,ath9k"
> +
> +Optional properties:
> +- reg: Address and length of the register set for the device.
> +- qca,gpio-mask: The GPIO mask
> +- qca,gpio-val: The GPIO value
> +- qca,led-pin: The GPIO number to which the LED is connected
> +- qca,led-active-high: The LED is active when the GPIO is HIGH
> +- qca,clk-25mhz: Defines that at 25MHz clock is used

I must assume these apply to internal GPIOs, LEDs and clocks, so I'm
somewhat surprised any description is necessary.

How variable are these in practice?

> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
> +			will be loaded via request_firmware)

The binding shouldn't know anything about the host filesystem,
request_firmware, etc. So the description is a best a little off.

What happens when a new FW comes out? I shouldn't have to update my DT
to cater for that.

Please find a better way to identify relevant FW. What exactly affects
which FW can be used, or would ideally be used? Are different FWs
required for the same HW in some contexts?

Can we not figure out the relevant FW names in the driver based on some
identification mechanism (e.g. a more thoroughly defined set of
compatible strings)?

> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
> +				swapping of the EEPROM data if required

CAn we not simply always do this?

> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM

When/why would these be necessary?

Thanks,
Mark.

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Mark Rutland]

On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
> Add documentation how devicetree can be used to configure ath9k based
> devices.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> changes in v1 -> v2:
> - use vendor prefix "qca" instead of "ath"
> - extend the example so it includes the "compatible" property
> 
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> new file mode 100644
> index 0000000..bb78f68
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -0,0 +1,41 @@
> +* Qualcomm Atheros ath9k wireless devices
> +
> +This node provides properties for configuring the ath9k wireless device. The
> +node is expected to be specified as a child node of the PCI controller to
> +which the wireless chip is connected.
> +
> +Required properties:
> +- compatible: Should be "qca,ath9k"
> +
> +Optional properties:
> +- reg: Address and length of the register set for the device.
> +- qca,gpio-mask: The GPIO mask
> +- qca,gpio-val: The GPIO value
> +- qca,led-pin: The GPIO number to which the LED is connected
> +- qca,led-active-high: The LED is active when the GPIO is HIGH
> +- qca,clk-25mhz: Defines that at 25MHz clock is used

I must assume these apply to internal GPIOs, LEDs and clocks, so I'm
somewhat surprised any description is necessary.

How variable are these in practice?

> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
> +			will be loaded via request_firmware)

The binding shouldn't know anything about the host filesystem,
request_firmware, etc. So the description is a best a little off.

What happens when a new FW comes out? I shouldn't have to update my DT
to cater for that.

Please find a better way to identify relevant FW. What exactly affects
which FW can be used, or would ideally be used? Are different FWs
required for the same HW in some contexts?

Can we not figure out the relevant FW names in the driver based on some
identification mechanism (e.g. a more thoroughly defined set of
compatible strings)?

> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
> +				swapping of the EEPROM data if required

CAn we not simply always do this?

> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM

When/why would these be necessary?

Thanks,
Mark.

Re: [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Thu, Jun 23, 2016 at 7:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
>> Add documentation how devicetree can be used to configure ath9k based
>> devices.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>> changes in v1 -> v2:
>> - use vendor prefix "qca" instead of "ath"
>> - extend the example so it includes the "compatible" property
>>
>>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
>> new file mode 100644
>> index 0000000..bb78f68
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
>> @@ -0,0 +1,41 @@
>> +* Qualcomm Atheros ath9k wireless devices
>> +
>> +This node provides properties for configuring the ath9k wireless device. The
>> +node is expected to be specified as a child node of the PCI controller to
>> +which the wireless chip is connected.
>> +
>> +Required properties:
>> +- compatible: Should be "qca,ath9k"
>> +
>> +Optional properties:
>> +- reg: Address and length of the register set for the device.
>> +- qca,gpio-mask: The GPIO mask
>> +- qca,gpio-val: The GPIO value
>> +- qca,led-pin: The GPIO number to which the LED is connected
>> +- qca,led-active-high: The LED is active when the GPIO is HIGH
>> +- qca,clk-25mhz: Defines that at 25MHz clock is used
>
> I must assume these apply to internal GPIOs, LEDs and clocks, so I'm
> somewhat surprised any description is necessary.
>
> How variable are these in practice?
led-pin and led-active-high are definitely used on various OpenWrt devices.
However, I am afraid that I have to pass this question to the ath9k
developers for the other properties (gpio-mask, gpio-val and
clk-25mhz).
If you want we can skip gpio-mask, gpio-val and clk-25mhz for now, but
keep led-pin and led-active-high.

>> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
>> +                     will be loaded via request_firmware)
>
> The binding shouldn't know anything about the host filesystem,
> request_firmware, etc. So the description is a best a little off.
>
> What happens when a new FW comes out? I shouldn't have to update my DT
> to cater for that.
This is not exactly a "firmware" but rather device-specific
calibration data (RF settings, MAC address, etc). Usually there is an
eeprom connected directly to the wifi chip, but on embedded devices
this is usually skipped and instead the calibration data is shipped
somewhere on the main flash (directly on SPI-/NOR-/NAND flash,
sometimes even inside an UBI volume).

> Please find a better way to identify relevant FW. What exactly affects
> which FW can be used, or would ideally be used? Are different FWs
> required for the same HW in some contexts?
>
> Can we not figure out the relevant FW names in the driver based on some
> identification mechanism (e.g. a more thoroughly defined set of
> compatible strings)?
The only way of auto-detecting a "correct" name would be via
dev_name() (with some prefix this could give something like
ath9k-pci-0000:00:0e.0.bin).

>> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
>> +                             swapping of the EEPROM data if required
>
> CAn we not simply always do this?
I've asked myself this question as well, but unfortunately some
manufacturers ship the EEPROM data with incorrect endianness magic.
Thus I decided to stay consistent with ath9k_platform_data which also
has a boolean (which defaults to false).

>> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
>> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
>
> When/why would these be necessary?
sometimes a manufacturer (accidentally) leaves both bands enabled in
the EEPROM data,while the RF hardware is only suitable for one of both
bands. The same settings exist in ath9k_platform_data, serving exactly
the same purpose


Regards,
Martin

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Thu, Jun 23, 2016 at 7:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
>> Add documentation how devicetree can be used to configure ath9k based
>> devices.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>> changes in v1 -> v2:
>> - use vendor prefix "qca" instead of "ath"
>> - extend the example so it includes the "compatible" property
>>
>>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
>> new file mode 100644
>> index 0000000..bb78f68
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
>> @@ -0,0 +1,41 @@
>> +* Qualcomm Atheros ath9k wireless devices
>> +
>> +This node provides properties for configuring the ath9k wireless device. The
>> +node is expected to be specified as a child node of the PCI controller to
>> +which the wireless chip is connected.
>> +
>> +Required properties:
>> +- compatible: Should be "qca,ath9k"
>> +
>> +Optional properties:
>> +- reg: Address and length of the register set for the device.
>> +- qca,gpio-mask: The GPIO mask
>> +- qca,gpio-val: The GPIO value
>> +- qca,led-pin: The GPIO number to which the LED is connected
>> +- qca,led-active-high: The LED is active when the GPIO is HIGH
>> +- qca,clk-25mhz: Defines that at 25MHz clock is used
>
> I must assume these apply to internal GPIOs, LEDs and clocks, so I'm
> somewhat surprised any description is necessary.
>
> How variable are these in practice?
led-pin and led-active-high are definitely used on various OpenWrt devices.
However, I am afraid that I have to pass this question to the ath9k
developers for the other properties (gpio-mask, gpio-val and
clk-25mhz).
If you want we can skip gpio-mask, gpio-val and clk-25mhz for now, but
keep led-pin and led-active-high.

>> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
>> +                     will be loaded via request_firmware)
>
> The binding shouldn't know anything about the host filesystem,
> request_firmware, etc. So the description is a best a little off.
>
> What happens when a new FW comes out? I shouldn't have to update my DT
> to cater for that.
This is not exactly a "firmware" but rather device-specific
calibration data (RF settings, MAC address, etc). Usually there is an
eeprom connected directly to the wifi chip, but on embedded devices
this is usually skipped and instead the calibration data is shipped
somewhere on the main flash (directly on SPI-/NOR-/NAND flash,
sometimes even inside an UBI volume).

> Please find a better way to identify relevant FW. What exactly affects
> which FW can be used, or would ideally be used? Are different FWs
> required for the same HW in some contexts?
>
> Can we not figure out the relevant FW names in the driver based on some
> identification mechanism (e.g. a more thoroughly defined set of
> compatible strings)?
The only way of auto-detecting a "correct" name would be via
dev_name() (with some prefix this could give something like
ath9k-pci-0000:00:0e.0.bin).

>> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
>> +                             swapping of the EEPROM data if required
>
> CAn we not simply always do this?
I've asked myself this question as well, but unfortunately some
manufacturers ship the EEPROM data with incorrect endianness magic.
Thus I decided to stay consistent with ath9k_platform_data which also
has a boolean (which defaults to false).

>> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
>> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
>
> When/why would these be necessary?
sometimes a manufacturer (accidentally) leaves both bands enabled in
the EEPROM data,while the RF hardware is only suitable for one of both
bands. The same settings exist in ath9k_platform_data, serving exactly
the same purpose


Regards,
Martin

Re: [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Mark Rutland]

On Thu, Jun 23, 2016 at 08:14:29PM +0200, Martin Blumenstingl wrote:
> On Thu, Jun 23, 2016 at 7:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
> >> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
> >> +                     will be loaded via request_firmware)
> >
> > The binding shouldn't know anything about the host filesystem,
> > request_firmware, etc. So the description is a best a little off.
> >
> > What happens when a new FW comes out? I shouldn't have to update my DT
> > to cater for that.
> This is not exactly a "firmware" but rather device-specific
> calibration data (RF settings, MAC address, etc). Usually there is an
> eeprom connected directly to the wifi chip, but on embedded devices
> this is usually skipped and instead the calibration data is shipped
> somewhere on the main flash (directly on SPI-/NOR-/NAND flash,
> sometimes even inside an UBI volume).

Ok. I believe that previously, for ath10k, it was suggested that this
calibration data be placed directly in the DT (assuming it's small
enough).

> > Please find a better way to identify relevant FW. What exactly affects
> > which FW can be used, or would ideally be used? Are different FWs
> > required for the same HW in some contexts?
> >
> > Can we not figure out the relevant FW names in the driver based on some
> > identification mechanism (e.g. a more thoroughly defined set of
> > compatible strings)?
> The only way of auto-detecting a "correct" name would be via
> dev_name() (with some prefix this could give something like
> ath9k-pci-0000:00:0e.0.bin).

That may work, if the above is not an option.

> 
> >> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
> >> +                             swapping of the EEPROM data if required
> >
> > CAn we not simply always do this?
> I've asked myself this question as well, but unfortunately some
> manufacturers ship the EEPROM data with incorrect endianness magic.
> Thus I decided to stay consistent with ath9k_platform_data which also
> has a boolean (which defaults to false).

Ah. It's probably worth a note in the binding that this is not always
safe, and should only be set if the eeprom is known to have valid
endianness magic.

It would also be worth specifying teh behaviour in the absence of this
property.

> 
> >> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
> >> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
> >
> > When/why would these be necessary?
> sometimes a manufacturer (accidentally) leaves both bands enabled in
> the EEPROM data,while the RF hardware is only suitable for one of both
> bands. The same settings exist in ath9k_platform_data, serving exactly
> the same purpose

Ok. Can we invert these instead (i.e. describe when the feature is
available)? e.g. qca,supports-2ghz.

Thanks,
Mark.

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Mark Rutland]

On Thu, Jun 23, 2016 at 08:14:29PM +0200, Martin Blumenstingl wrote:
> On Thu, Jun 23, 2016 at 7:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
> >> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
> >> +                     will be loaded via request_firmware)
> >
> > The binding shouldn't know anything about the host filesystem,
> > request_firmware, etc. So the description is a best a little off.
> >
> > What happens when a new FW comes out? I shouldn't have to update my DT
> > to cater for that.
> This is not exactly a "firmware" but rather device-specific
> calibration data (RF settings, MAC address, etc). Usually there is an
> eeprom connected directly to the wifi chip, but on embedded devices
> this is usually skipped and instead the calibration data is shipped
> somewhere on the main flash (directly on SPI-/NOR-/NAND flash,
> sometimes even inside an UBI volume).

Ok. I believe that previously, for ath10k, it was suggested that this
calibration data be placed directly in the DT (assuming it's small
enough).

> > Please find a better way to identify relevant FW. What exactly affects
> > which FW can be used, or would ideally be used? Are different FWs
> > required for the same HW in some contexts?
> >
> > Can we not figure out the relevant FW names in the driver based on some
> > identification mechanism (e.g. a more thoroughly defined set of
> > compatible strings)?
> The only way of auto-detecting a "correct" name would be via
> dev_name() (with some prefix this could give something like
> ath9k-pci-0000:00:0e.0.bin).

That may work, if the above is not an option.

> 
> >> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
> >> +                             swapping of the EEPROM data if required
> >
> > CAn we not simply always do this?
> I've asked myself this question as well, but unfortunately some
> manufacturers ship the EEPROM data with incorrect endianness magic.
> Thus I decided to stay consistent with ath9k_platform_data which also
> has a boolean (which defaults to false).

Ah. It's probably worth a note in the binding that this is not always
safe, and should only be set if the eeprom is known to have valid
endianness magic.

It would also be worth specifying teh behaviour in the absence of this
property.

> 
> >> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
> >> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
> >
> > When/why would these be necessary?
> sometimes a manufacturer (accidentally) leaves both bands enabled in
> the EEPROM data,while the RF hardware is only suitable for one of both
> bands. The same settings exist in ath9k_platform_data, serving exactly
> the same purpose

Ok. Can we invert these instead (i.e. describe when the feature is
available)? e.g. qca,supports-2ghz.

Thanks,
Mark.

Re: [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Felix Fietkau]

On 2016-06-27 14:57, Mark Rutland wrote:
> On Thu, Jun 23, 2016 at 08:14:29PM +0200, Martin Blumenstingl wrote:
>> On Thu, Jun 23, 2016 at 7:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
>> >> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
>> >> +                     will be loaded via request_firmware)
>> >
>> > The binding shouldn't know anything about the host filesystem,
>> > request_firmware, etc. So the description is a best a little off.
>> >
>> > What happens when a new FW comes out? I shouldn't have to update my DT
>> > to cater for that.
>> This is not exactly a "firmware" but rather device-specific
>> calibration data (RF settings, MAC address, etc). Usually there is an
>> eeprom connected directly to the wifi chip, but on embedded devices
>> this is usually skipped and instead the calibration data is shipped
>> somewhere on the main flash (directly on SPI-/NOR-/NAND flash,
>> sometimes even inside an UBI volume).
> 
> Ok. I believe that previously, for ath10k, it was suggested that this
> calibration data be placed directly in the DT (assuming it's small
> enough).
I don't think the data should go directly into DT, because then we need
a lot more complex kernel loader stubs. There are hundreds of devices
out there with calibration data in flash, and many of them have the data
in different places, and almost all of them don't support passing DT via
boot loader.
The actual RF settings are calibrated for every individual device, so
they need to be read from the flash partition anyway.
I think it makes sense to add an optional reference to a mtd partition
and allow the kernel to read from it directly.

>> > Please find a better way to identify relevant FW. What exactly affects
>> > which FW can be used, or would ideally be used? Are different FWs
>> > required for the same HW in some contexts?
>> >
>> > Can we not figure out the relevant FW names in the driver based on some
>> > identification mechanism (e.g. a more thoroughly defined set of
>> > compatible strings)?
>> The only way of auto-detecting a "correct" name would be via
>> dev_name() (with some prefix this could give something like
>> ath9k-pci-0000:00:0e.0.bin).
> 
> That may work, if the above is not an option.
I think that's a good idea.

- Felix

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Felix Fietkau]

On 2016-06-27 14:57, Mark Rutland wrote:
> On Thu, Jun 23, 2016 at 08:14:29PM +0200, Martin Blumenstingl wrote:
>> On Thu, Jun 23, 2016 at 7:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Jun 23, 2016 at 07:45:35PM +0200, Martin Blumenstingl wrote:
>> >> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
>> >> +                     will be loaded via request_firmware)
>> >
>> > The binding shouldn't know anything about the host filesystem,
>> > request_firmware, etc. So the description is a best a little off.
>> >
>> > What happens when a new FW comes out? I shouldn't have to update my DT
>> > to cater for that.
>> This is not exactly a "firmware" but rather device-specific
>> calibration data (RF settings, MAC address, etc). Usually there is an
>> eeprom connected directly to the wifi chip, but on embedded devices
>> this is usually skipped and instead the calibration data is shipped
>> somewhere on the main flash (directly on SPI-/NOR-/NAND flash,
>> sometimes even inside an UBI volume).
> 
> Ok. I believe that previously, for ath10k, it was suggested that this
> calibration data be placed directly in the DT (assuming it's small
> enough).
I don't think the data should go directly into DT, because then we need
a lot more complex kernel loader stubs. There are hundreds of devices
out there with calibration data in flash, and many of them have the data
in different places, and almost all of them don't support passing DT via
boot loader.
The actual RF settings are calibrated for every individual device, so
they need to be read from the flash partition anyway.
I think it makes sense to add an optional reference to a mtd partition
and allow the kernel to read from it directly.

>> > Please find a better way to identify relevant FW. What exactly affects
>> > which FW can be used, or would ideally be used? Are different FWs
>> > required for the same HW in some contexts?
>> >
>> > Can we not figure out the relevant FW names in the driver based on some
>> > identification mechanism (e.g. a more thoroughly defined set of
>> > compatible strings)?
>> The only way of auto-detecting a "correct" name would be via
>> dev_name() (with some prefix this could give something like
>> ath9k-pci-0000:00:0e.0.bin).
> 
> That may work, if the above is not an option.
I think that's a good idea.

- Felix

Re: [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Mon, Jun 27, 2016 at 2:57 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Please find a better way to identify relevant FW. What exactly affects
>> > which FW can be used, or would ideally be used? Are different FWs
>> > required for the same HW in some contexts?
>> >
>> > Can we not figure out the relevant FW names in the driver based on some
>> > identification mechanism (e.g. a more thoroughly defined set of
>> > compatible strings)?
>> The only way of auto-detecting a "correct" name would be via
>> dev_name() (with some prefix this could give something like
>> ath9k-pci-0000:00:0e.0.bin).
>
> That may work, if the above is not an option.
I would also prefer this (Felix' email already contains an explanation
why this way is preferred and I fully agree with him).

>> >> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
>> >> +                             swapping of the EEPROM data if required
>> >
>> > CAn we not simply always do this?
>> I've asked myself this question as well, but unfortunately some
>> manufacturers ship the EEPROM data with incorrect endianness magic.
>> Thus I decided to stay consistent with ath9k_platform_data which also
>> has a boolean (which defaults to false).
>
> Ah. It's probably worth a note in the binding that this is not always
> safe, and should only be set if the eeprom is known to have valid
> endianness magic.
>
> It would also be worth specifying teh behaviour in the absence of this
> property.
noted, I will fix this in the next round

>>
>> >> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
>> >> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
>> >
>> > When/why would these be necessary?
>> sometimes a manufacturer (accidentally) leaves both bands enabled in
>> the EEPROM data,while the RF hardware is only suitable for one of both
>> bands. The same settings exist in ath9k_platform_data, serving exactly
>> the same purpose
>
> Ok. Can we invert these instead (i.e. describe when the feature is
> available)? e.g. qca,supports-2ghz.
we could invert these, but I think the "disable" logic was chosen with
a good reason:
the ath9k calibration data already contains the information which
bands are enabled/disabled. Enabling a band via devicetree / platform
data is not possible, because that would mean we would have to pass
additional calibration data for this band.
The only use-case where these disable-Xghz properties are used is when
the device vendor forgot to disable one of the bands. I can improve
the documentation for this one, but I would prefer to stay with the
disable naming/logic


Martin

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Mon, Jun 27, 2016 at 2:57 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Please find a better way to identify relevant FW. What exactly affects
>> > which FW can be used, or would ideally be used? Are different FWs
>> > required for the same HW in some contexts?
>> >
>> > Can we not figure out the relevant FW names in the driver based on some
>> > identification mechanism (e.g. a more thoroughly defined set of
>> > compatible strings)?
>> The only way of auto-detecting a "correct" name would be via
>> dev_name() (with some prefix this could give something like
>> ath9k-pci-0000:00:0e.0.bin).
>
> That may work, if the above is not an option.
I would also prefer this (Felix' email already contains an explanation
why this way is preferred and I fully agree with him).

>> >> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
>> >> +                             swapping of the EEPROM data if required
>> >
>> > CAn we not simply always do this?
>> I've asked myself this question as well, but unfortunately some
>> manufacturers ship the EEPROM data with incorrect endianness magic.
>> Thus I decided to stay consistent with ath9k_platform_data which also
>> has a boolean (which defaults to false).
>
> Ah. It's probably worth a note in the binding that this is not always
> safe, and should only be set if the eeprom is known to have valid
> endianness magic.
>
> It would also be worth specifying teh behaviour in the absence of this
> property.
noted, I will fix this in the next round

>>
>> >> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
>> >> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
>> >
>> > When/why would these be necessary?
>> sometimes a manufacturer (accidentally) leaves both bands enabled in
>> the EEPROM data,while the RF hardware is only suitable for one of both
>> bands. The same settings exist in ath9k_platform_data, serving exactly
>> the same purpose
>
> Ok. Can we invert these instead (i.e. describe when the feature is
> available)? e.g. qca,supports-2ghz.
we could invert these, but I think the "disable" logic was chosen with
a good reason:
the ath9k calibration data already contains the information which
bands are enabled/disabled. Enabling a band via devicetree / platform
data is not possible, because that would mean we would have to pass
additional calibration data for this band.
The only use-case where these disable-Xghz properties are used is when
the device vendor forgot to disable one of the bands. I can improve
the documentation for this one, but I would prefer to stay with the
disable naming/logic


Martin

Re: [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Arend Van Spriel]

On 23-6-2016 19:45, Martin Blumenstingl wrote:
> Add documentation how devicetree can be used to configure ath9k based
> devices.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> changes in v1 -> v2:
> - use vendor prefix "qca" instead of "ath"
> - extend the example so it includes the "compatible" property
> 
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> new file mode 100644
> index 0000000..bb78f68
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -0,0 +1,41 @@
> +* Qualcomm Atheros ath9k wireless devices
> +
> +This node provides properties for configuring the ath9k wireless device. The
> +node is expected to be specified as a child node of the PCI controller to
> +which the wireless chip is connected.
> +
> +Required properties:
> +- compatible: Should be "qca,ath9k"
> +
> +Optional properties:
> +- reg: Address and length of the register set for the device.

Is 'reg' property handled. I don't see it in patch 2/2.

> +- qca,gpio-mask: The GPIO mask
> +- qca,gpio-val: The GPIO value
> +- qca,led-pin: The GPIO number to which the LED is connected
> +- qca,led-active-high: The LED is active when the GPIO is HIGH
> +- qca,clk-25mhz: Defines that at 25MHz clock is used

For the above I can somehow see them as variables for different hardware
platforms.

> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
> +			will be loaded via request_firmware)
> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
> +				swapping of the EEPROM data if required
> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM

These not really. Storing filename information in device tree seems
wrong as it does not describe hardware configuration. The other three
also seem more like driver module parameters. I think what you are
trying here with the last two properties is to use the same eeprom file
for different types of hardware, ie. for dual-band, 2g-only, and 5g-only
devices. From device tree perspective I would use those types, eg.:

qca,2g-capable: Device can operate in 2.4GHz band.
qca,5g-capable: Device can operate in 5GHz band.

The other patch also looks for a MAC address for the device. I suppose
that should be documented as well.

> +In this example, the node is defined as child node of the PCI controller.
> +
> +pci {
> +	pcie@0 {
> +		reg = <0 0 0 0 0>;
> +		#interrupt-cells = <1>;
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		device_type = "pci";
> +
> +		ath9k@0,0 {
> +			compatible = "qca,ath9k";
> +			reg = <0 0 0 0 0>;
> +			device_type = "pci";

Is this just a copy-paste or should device_type be specified?

Regards,
Arend

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Arend Van Spriel]

On 23-6-2016 19:45, Martin Blumenstingl wrote:
> Add documentation how devicetree can be used to configure ath9k based
> devices.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> changes in v1 -> v2:
> - use vendor prefix "qca" instead of "ath"
> - extend the example so it includes the "compatible" property
> 
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> new file mode 100644
> index 0000000..bb78f68
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -0,0 +1,41 @@
> +* Qualcomm Atheros ath9k wireless devices
> +
> +This node provides properties for configuring the ath9k wireless device. The
> +node is expected to be specified as a child node of the PCI controller to
> +which the wireless chip is connected.
> +
> +Required properties:
> +- compatible: Should be "qca,ath9k"
> +
> +Optional properties:
> +- reg: Address and length of the register set for the device.

Is 'reg' property handled. I don't see it in patch 2/2.

> +- qca,gpio-mask: The GPIO mask
> +- qca,gpio-val: The GPIO value
> +- qca,led-pin: The GPIO number to which the LED is connected
> +- qca,led-active-high: The LED is active when the GPIO is HIGH
> +- qca,clk-25mhz: Defines that at 25MHz clock is used

For the above I can somehow see them as variables for different hardware
platforms.

> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
> +			will be loaded via request_firmware)
> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
> +				swapping of the EEPROM data if required
> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM

These not really. Storing filename information in device tree seems
wrong as it does not describe hardware configuration. The other three
also seem more like driver module parameters. I think what you are
trying here with the last two properties is to use the same eeprom file
for different types of hardware, ie. for dual-band, 2g-only, and 5g-only
devices. From device tree perspective I would use those types, eg.:

qca,2g-capable: Device can operate in 2.4GHz band.
qca,5g-capable: Device can operate in 5GHz band.

The other patch also looks for a MAC address for the device. I suppose
that should be documented as well.

> +In this example, the node is defined as child node of the PCI controller.
> +
> +pci {
> +	pcie at 0 {
> +		reg = <0 0 0 0 0>;
> +		#interrupt-cells = <1>;
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		device_type = "pci";
> +
> +		ath9k at 0,0 {
> +			compatible = "qca,ath9k";
> +			reg = <0 0 0 0 0>;
> +			device_type = "pci";

Is this just a copy-paste or should device_type be specified?

Regards,
Arend

Re: [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Thu, Jun 23, 2016 at 9:25 PM, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>> +Optional properties:
>> +- reg: Address and length of the register set for the device.
>
> Is 'reg' property handled. I don't see it in patch 2/2.
for AHB we would probably have to handle it separately, but AHB
support is not scope of my patch. For PCI(e) this is parsed
automatically by of_pci_find_child_device when the PCI controller is
found and the child nodes are enumerated. See [0]

>> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
>> +                     will be loaded via request_firmware)
>> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
>> +                             swapping of the EEPROM data if required
>> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
>> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
>
> These not really. Storing filename information in device tree seems
> wrong as it does not describe hardware configuration. The other three
> also seem more like driver module parameters. I think what you are
> trying here with the last two properties is to use the same eeprom file
> for different types of hardware, ie. for dual-band, 2g-only, and 5g-only
> devices. From device tree perspective I would use those types, eg.:
>
> qca,2g-capable: Device can operate in 2.4GHz band.
> qca,5g-capable: Device can operate in 5GHz band.
please let me explain this a bit more detailed (and clean up the
confusion which I may have created):
ath9k itself does not need a firmware to run.
But it needs to know some details of the actual RF hardware (let's
call it calibration/EEPROM data) to which the ath9k chip is wired
(which frequencies/bands are enabled, how many RX/TX antennas are
there, max TX power, if there's a LNA, and so on).
This calibration/EEPROM data is unique for each "ath9k + RF hardware"
combination and usually stored inside an EEPROM connected directly to
the ath9k chip. However, many embedded devices do not have a physical
EEPROM connected to ath9k. For these devices we have to obtain the
calibration/EEPROM data from "somewhere" (which is usually stored
somewhere on SPI/NOR/NAND flash, but can sometimes even be stored
inside an UBI volume).

"qca,eeprom-name" tries to solve the problem that ath9k needs
calibration/EEPROM data even if there is no physical EEPROM attached
to the wifi chip.
One important thing here is that we need to be able to load two
different calibration/EEPROM data files in case one system uses two
ath9k chips (there might for example be a dedicated 2.4G and dedicated
5G chip).
If this should not be part of devicetree then we could do it like
ath10k does and generate the filename for request_firmware based on
the dev_name() - see [1].
I would then replace "qca,eeprom-name" with a boolean
"qca,use-external-eeprom" to signal ath9k that this chip does not have
a (physical) EEPROM attached (ath9k would then start the
request_firmware mechanism).

qca,check-eeprom-endianness is unfortunately required because some
vendors use the little endian magic while the data is actually big
endian.
Endianness swapping needs to be done inside ath9k because it's a quite
complex operation (due to the fact that there are some 16bit and 32bit
fields which have to be swapped differently).
So one would set qca,check-eeprom-endianness if the device vendor
chose the wrong endianness magic, while the data inside the
calibration/EEPROM data is correct.
Enabling it unconditionally would result in the calibration/EEPROM
data being endianness-swapped.

qca,disable-2ghz/qca,disable-5ghz exist due to a similar issue as we
find with the eeprom endianness magic:
Normally the information which bands are enabled for this specific
"ath9k + RF hardware" combination is stored inside the
calibration/EEPROM data.
Unfortunately some vendors left for example the 5GHz band enabled in
the calibration/EEPROM data while the RF hardware only supports
2.4GHz.
Using the 5GHz band on such devices will lead to issues, so it should
be possible to disable those "incorrectly allowed" frequencies/bands.

> The other patch also looks for a MAC address for the device. I suppose
> that should be documented as well.
good catch, thanks!

>> +In this example, the node is defined as child node of the PCI controller.
>> +
>> +pci {
>> +     pcie@0 {
>> +             reg = <0 0 0 0 0>;
>> +             #interrupt-cells = <1>;
>> +             #size-cells = <2>;
>> +             #address-cells = <3>;
>> +             device_type = "pci";
>> +
>> +             ath9k@0,0 {
>> +                     compatible = "qca,ath9k";
>> +                     reg = <0 0 0 0 0>;
>> +                     device_type = "pci";
>
> Is this just a copy-paste or should device_type be specified?
indeed, as far as I understand it should only be defined on the bridge
- thus it should be removed from the ath9k node.
again, thanks for noting this!


Regards,
Martin


[0] http://lxr.free-electrons.com/source/drivers/of/of_pci.c?v=4.6#L21
[1] http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/core.c#L729

[ath9k-devel] [PATCH RFC v2 1/2] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Thu, Jun 23, 2016 at 9:25 PM, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>> +Optional properties:
>> +- reg: Address and length of the register set for the device.
>
> Is 'reg' property handled. I don't see it in patch 2/2.
for AHB we would probably have to handle it separately, but AHB
support is not scope of my patch. For PCI(e) this is parsed
automatically by of_pci_find_child_device when the PCI controller is
found and the child nodes are enumerated. See [0]

>> +- qca,eeprom-name: The name of the file which contains the EEPROM data (which
>> +                     will be loaded via request_firmware)
>> +- qca,check-eeprom-endianness: Allow checking the EEPROM endianness and
>> +                             swapping of the EEPROM data if required
>> +- qca,disable-2ghz: Disables the 2.4GHz band, even if enabled in the EEPROM
>> +- qca,disable-5ghz: Disables the 5GHz band, even if enabled in the EEPROM
>
> These not really. Storing filename information in device tree seems
> wrong as it does not describe hardware configuration. The other three
> also seem more like driver module parameters. I think what you are
> trying here with the last two properties is to use the same eeprom file
> for different types of hardware, ie. for dual-band, 2g-only, and 5g-only
> devices. From device tree perspective I would use those types, eg.:
>
> qca,2g-capable: Device can operate in 2.4GHz band.
> qca,5g-capable: Device can operate in 5GHz band.
please let me explain this a bit more detailed (and clean up the
confusion which I may have created):
ath9k itself does not need a firmware to run.
But it needs to know some details of the actual RF hardware (let's
call it calibration/EEPROM data) to which the ath9k chip is wired
(which frequencies/bands are enabled, how many RX/TX antennas are
there, max TX power, if there's a LNA, and so on).
This calibration/EEPROM data is unique for each "ath9k + RF hardware"
combination and usually stored inside an EEPROM connected directly to
the ath9k chip. However, many embedded devices do not have a physical
EEPROM connected to ath9k. For these devices we have to obtain the
calibration/EEPROM data from "somewhere" (which is usually stored
somewhere on SPI/NOR/NAND flash, but can sometimes even be stored
inside an UBI volume).

"qca,eeprom-name" tries to solve the problem that ath9k needs
calibration/EEPROM data even if there is no physical EEPROM attached
to the wifi chip.
One important thing here is that we need to be able to load two
different calibration/EEPROM data files in case one system uses two
ath9k chips (there might for example be a dedicated 2.4G and dedicated
5G chip).
If this should not be part of devicetree then we could do it like
ath10k does and generate the filename for request_firmware based on
the dev_name() - see [1].
I would then replace "qca,eeprom-name" with a boolean
"qca,use-external-eeprom" to signal ath9k that this chip does not have
a (physical) EEPROM attached (ath9k would then start the
request_firmware mechanism).

qca,check-eeprom-endianness is unfortunately required because some
vendors use the little endian magic while the data is actually big
endian.
Endianness swapping needs to be done inside ath9k because it's a quite
complex operation (due to the fact that there are some 16bit and 32bit
fields which have to be swapped differently).
So one would set qca,check-eeprom-endianness if the device vendor
chose the wrong endianness magic, while the data inside the
calibration/EEPROM data is correct.
Enabling it unconditionally would result in the calibration/EEPROM
data being endianness-swapped.

qca,disable-2ghz/qca,disable-5ghz exist due to a similar issue as we
find with the eeprom endianness magic:
Normally the information which bands are enabled for this specific
"ath9k + RF hardware" combination is stored inside the
calibration/EEPROM data.
Unfortunately some vendors left for example the 5GHz band enabled in
the calibration/EEPROM data while the RF hardware only supports
2.4GHz.
Using the 5GHz band on such devices will lead to issues, so it should
be possible to disable those "incorrectly allowed" frequencies/bands.

> The other patch also looks for a MAC address for the device. I suppose
> that should be documented as well.
good catch, thanks!

>> +In this example, the node is defined as child node of the PCI controller.
>> +
>> +pci {
>> +     pcie at 0 {
>> +             reg = <0 0 0 0 0>;
>> +             #interrupt-cells = <1>;
>> +             #size-cells = <2>;
>> +             #address-cells = <3>;
>> +             device_type = "pci";
>> +
>> +             ath9k at 0,0 {
>> +                     compatible = "qca,ath9k";
>> +                     reg = <0 0 0 0 0>;
>> +                     device_type = "pci";
>
> Is this just a copy-paste or should device_type be specified?
indeed, as far as I understand it should only be defined on the bridge
- thus it should be removed from the ath9k node.
again, thanks for noting this!


Regards,
Martin


[0] http://lxr.free-electrons.com/source/drivers/of/of_pci.c?v=4.6#L21
[1] http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/core.c#L729

[PATCH RFC v2 2/2] ath9k: parse the device configuration from an OF node

[Martin Blumenstingl]

This makes it possible to configure ath9k based devices using
devicetree. That makes some out-of-tree "convert devicetree to
ath9k_platform_data glue"-code obsolete.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
changes in v1 -> v2:
- use vendor prefix "qca" instead of "ath"
- use of_device_is_available instead of simply checking if "np" is NULL
- removed code which read some properties twice

 drivers/net/wireless/ath/ath9k/init.c | 61 +++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index a0f4a52..2b5e3db 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -20,6 +20,8 @@
 #include <linux/slab.h>
 #include <linux/ath9k_platform.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
 #include <linux/relay.h>
 #include <net/ieee80211_radiotap.h>
 
@@ -555,6 +557,61 @@ static int ath9k_init_platform(struct ath_softc *sc)
 	return 0;
 }
 
+static int ath9k_of_init(struct ath_softc *sc)
+{
+	struct device_node *np = sc->dev->of_node;
+	struct ath_hw *ah = sc->sc_ah;
+	struct ath_common *common = ath9k_hw_common(ah);
+	const char *mac, *eeprom_name;
+	int led_pin, ret;
+	u32 gpio_data;
+
+	if (!of_device_is_available(np))
+		return 0;
+
+	ath_dbg(common, CONFIG, "parsing configuration from OF node\n");
+
+	if (!of_property_read_u32(np, "qca,led-pin", &led_pin))
+		ah->led_pin = led_pin;
+
+	if (!of_property_read_u32(np, "qca,gpio-mask", &gpio_data))
+		ah->gpio_mask = gpio_data;
+
+	if (!of_property_read_u32(np, "qca,gpio-val", &gpio_data))
+		ah->gpio_val = gpio_data;
+
+	if (of_property_read_bool(np, "qca,clk-25mhz"))
+		ah->is_clk_25mhz = true;
+
+	if (of_property_read_bool(np, "qca,led-active-high"))
+		ah->config.led_active_high = true;
+
+	if (of_property_read_bool(np, "qca,disable-2ghz"))
+		ah->disable_2ghz = true;
+
+	if (of_property_read_bool(np, "qca,disable-5ghz"))
+		ah->disable_5ghz = true;
+
+	if (of_property_read_bool(np, "qca,check-eeprom-endianness"))
+		ah->ah_flags &= ~AH_NO_EEP_SWAP;
+	else
+		ah->ah_flags |= AH_NO_EEP_SWAP;
+
+	if (!of_property_read_string(np, "qca,eeprom-name", &eeprom_name)) {
+		ret = ath9k_eeprom_request(sc, eeprom_name);
+		if (ret)
+			return ret;
+	}
+
+	mac = of_get_mac_address(np);
+	if (mac)
+		ether_addr_copy(common->macaddr, mac);
+
+	ah->ah_flags &= ~AH_USE_EEPROM;
+
+	return 0;
+}
+
 static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 			    const struct ath_bus_ops *bus_ops)
 {
@@ -611,6 +668,10 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	if (ret)
 		return ret;
 
+	ret = ath9k_of_init(sc);
+	if (ret)
+		return ret;
+
 	if (ath9k_led_active_high != -1)
 		ah->config.led_active_high = ath9k_led_active_high == 1;
 
-- 
2.9.0

[ath9k-devel] [PATCH RFC v2 2/2] ath9k: parse the device configuration from an OF node

[Martin Blumenstingl]

This makes it possible to configure ath9k based devices using
devicetree. That makes some out-of-tree "convert devicetree to
ath9k_platform_data glue"-code obsolete.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
changes in v1 -> v2:
- use vendor prefix "qca" instead of "ath"
- use of_device_is_available instead of simply checking if "np" is NULL
- removed code which read some properties twice

 drivers/net/wireless/ath/ath9k/init.c | 61 +++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index a0f4a52..2b5e3db 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -20,6 +20,8 @@
 #include <linux/slab.h>
 #include <linux/ath9k_platform.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
 #include <linux/relay.h>
 #include <net/ieee80211_radiotap.h>
 
@@ -555,6 +557,61 @@ static int ath9k_init_platform(struct ath_softc *sc)
 	return 0;
 }
 
+static int ath9k_of_init(struct ath_softc *sc)
+{
+	struct device_node *np = sc->dev->of_node;
+	struct ath_hw *ah = sc->sc_ah;
+	struct ath_common *common = ath9k_hw_common(ah);
+	const char *mac, *eeprom_name;
+	int led_pin, ret;
+	u32 gpio_data;
+
+	if (!of_device_is_available(np))
+		return 0;
+
+	ath_dbg(common, CONFIG, "parsing configuration from OF node\n");
+
+	if (!of_property_read_u32(np, "qca,led-pin", &led_pin))
+		ah->led_pin = led_pin;
+
+	if (!of_property_read_u32(np, "qca,gpio-mask", &gpio_data))
+		ah->gpio_mask = gpio_data;
+
+	if (!of_property_read_u32(np, "qca,gpio-val", &gpio_data))
+		ah->gpio_val = gpio_data;
+
+	if (of_property_read_bool(np, "qca,clk-25mhz"))
+		ah->is_clk_25mhz = true;
+
+	if (of_property_read_bool(np, "qca,led-active-high"))
+		ah->config.led_active_high = true;
+
+	if (of_property_read_bool(np, "qca,disable-2ghz"))
+		ah->disable_2ghz = true;
+
+	if (of_property_read_bool(np, "qca,disable-5ghz"))
+		ah->disable_5ghz = true;
+
+	if (of_property_read_bool(np, "qca,check-eeprom-endianness"))
+		ah->ah_flags &= ~AH_NO_EEP_SWAP;
+	else
+		ah->ah_flags |= AH_NO_EEP_SWAP;
+
+	if (!of_property_read_string(np, "qca,eeprom-name", &eeprom_name)) {
+		ret = ath9k_eeprom_request(sc, eeprom_name);
+		if (ret)
+			return ret;
+	}
+
+	mac = of_get_mac_address(np);
+	if (mac)
+		ether_addr_copy(common->macaddr, mac);
+
+	ah->ah_flags &= ~AH_USE_EEPROM;
+
+	return 0;
+}
+
 static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 			    const struct ath_bus_ops *bus_ops)
 {
@@ -611,6 +668,10 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	if (ret)
 		return ret;
 
+	ret = ath9k_of_init(sc);
+	if (ret)
+		return ret;
+
 	if (ath9k_led_active_high != -1)
 		ah->config.led_active_high = ath9k_led_active_high == 1;
 
-- 
2.9.0