[PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Alban]

The current binding only cover PCI devices so extend it for SoC devices.

Most SoC platforms use an MTD partition for the calibration data
instead of an EEPROM. The qca,no-eeprom property was added to allow
loading the EEPROM content using firmware loading. This new binding
replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
property as deprecated in case anyone ever used it.

Signed-off-by: Alban <albeu@free.fr>
---
 .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++--
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
index b7396c8..61f5f6d 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
@@ -27,16 +27,34 @@ Required properties:
 			- 0034 for AR9462
 			- 0036 for AR9565
 			- 0037 for AR9485
+		For SoC devices the compatible should be "qca,<soctype>-wmac"
+		and one of the following fallbacks:
+			- "qca,ar9100-wmac"
+			- "qca,ar9330-wmac"
+			- "qca,ar9340-wmac"
+			- "qca,qca9550-wmac"
+			- "qca,qca9530-wmac"
 - reg: Address and length of the register set for the device.
 
+Required properties for SoC devices:
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupts: Interrupt specifier for the controllers interrupt.
+
 Optional properties:
+- mac-address: See ethernet.txt in the parent directory
+- local-mac-address: See ethernet.txt in the parent directory
+- clock-names: has to be "ref"
+- clocks: phandle of the reference clock
+- resets: phandle of the reset line
+- nvmem-cell-names: has to be "eeprom" and/or "address"
+- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address
+		nvmem cell.
+
+Deprecated properties:
 - qca,no-eeprom: Indicates that there is no physical EEPROM connected to the
 			ath9k wireless chip (in this case the calibration /
 			EEPROM data will be loaded from userspace using the
 			kernel firmware loader).
-- mac-address: See ethernet.txt in the parent directory
-- local-mac-address: See ethernet.txt in the parent directory
-
 
 In this example, the node is defined as child node of the PCI controller:
 &pci0 {
@@ -46,3 +64,20 @@ In this example, the node is defined as child node of the PCI controller:
 		qca,no-eeprom;
 	};
 };
+
+In this example it is defined as a SoC device:
+	wmac@180c0000 {
+		compatible = "qca,ar9132-wmac", "qca,ar9100-wmac";
+		reg = <0x180c0000 0x30000>;
+
+		interrupt-parent = <&cpuintc>;
+		interrupts = <2>;
+
+		clock-names = "ref";
+		clocks = <&extosc>;
+
+		nvmem-cell-names = "eeprom", "address";
+		nvmem-cells = <&wmac_eeprom>, <&wmac_address>;
+
+		resets = <&rst 22>;
+	};
-- 
2.7.4

[PATCH 2/7] ath9k: ahb: Add OF support

[Alban]

Allow registering ath9k AHB devices defined in DT. This just add the
compatible strings to allow matching the driver and setting the proper
device ID.

Signed-off-by: Alban <albeu@free.fr>
---
 drivers/net/wireless/ath/ath9k/ahb.c | 47 +++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
index 2bd982c..36a2645 100644
--- a/drivers/net/wireless/ath/ath9k/ahb.c
+++ b/drivers/net/wireless/ath/ath9k/ahb.c
@@ -18,6 +18,7 @@
 
 #include <linux/nl80211.h>
 #include <linux/platform_device.h>
+#include <linux/of_device.h>
 #include <linux/module.h>
 #include "ath9k.h"
 
@@ -49,6 +50,33 @@ static const struct platform_device_id ath9k_platform_id_table[] = {
 	{},
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id ath_ahb_of_match[] = {
+	{
+		.compatible = "qca,ar9100-wmac",
+		.data = (void *)AR5416_AR9100_DEVID
+	},
+	{
+		.compatible = "qca,ar9330-wmac",
+		.data = (void *)AR9300_DEVID_AR9330
+	},
+	{
+		.compatible = "qca,ar9340-wmac",
+		.data = (void *)AR9300_DEVID_AR9340
+	},
+	{
+		.compatible = "qca,qca9550-wmac",
+		.data = (void *)AR9300_DEVID_QCA955X
+	},
+	{
+		.compatible = "qca,qca9530-wmac",
+		.data = (void *)AR9300_DEVID_AR953X
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ath_ahb_of_match);
+#endif
+
 /* return bus cachesize in 4B word units */
 static void ath_ahb_read_cachesize(struct ath_common *common, int *csz)
 {
@@ -79,10 +107,20 @@ static int ath_ahb_probe(struct platform_device *pdev)
 	int ret = 0;
 	struct ath_hw *ah;
 	char hw_name[64];
+	u16 devid;
 
-	if (!dev_get_platdata(&pdev->dev)) {
-		dev_err(&pdev->dev, "no platform data specified\n");
-		return -EINVAL;
+	if (id) {
+		devid = id->driver_data;
+	} else {
+		const struct of_device_id *match;
+
+		match = of_match_device(ath_ahb_of_match, &pdev->dev);
+		if (!match) {
+			dev_err(&pdev->dev, "no device match found\n");
+			return -EINVAL;
+		}
+
+		devid = (u16)(unsigned long)match->data;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -127,7 +165,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
 		goto err_free_hw;
 	}
 
-	ret = ath9k_init_device(id->driver_data, sc, &ath_ahb_bus_ops);
+	ret = ath9k_init_device(devid, sc, &ath_ahb_bus_ops);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to initialize device\n");
 		goto err_irq;
@@ -167,6 +205,7 @@ static struct platform_driver ath_ahb_driver = {
 	.remove     = ath_ahb_remove,
 	.driver		= {
 		.name	= "ath9k",
+		.of_match_table = of_match_ptr(ath_ahb_of_match),
 	},
 	.id_table    = ath9k_platform_id_table,
 };
-- 
2.7.4

Re: [PATCH 2/7] ath9k: ahb: Add OF support

[Sergei Shtylyov]

Hello!

On 3/14/2017 12:05 AM, Alban wrote:

> Allow registering ath9k AHB devices defined in DT. This just add the
> compatible strings to allow matching the driver and setting the proper
> device ID.
>
> Signed-off-by: Alban <albeu@free.fr>
> ---
>  drivers/net/wireless/ath/ath9k/ahb.c | 47 +++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
> index 2bd982c..36a2645 100644
> --- a/drivers/net/wireless/ath/ath9k/ahb.c
> +++ b/drivers/net/wireless/ath/ath9k/ahb.c
[...]
> @@ -79,10 +107,20 @@ static int ath_ahb_probe(struct platform_device *pdev)
>  	int ret = 0;
>  	struct ath_hw *ah;
>  	char hw_name[64];
> +	u16 devid;
>
> -	if (!dev_get_platdata(&pdev->dev)) {
> -		dev_err(&pdev->dev, "no platform data specified\n");
> -		return -EINVAL;
> +	if (id) {
> +		devid = id->driver_data;
> +	} else {
> +		const struct of_device_id *match;
> +
> +		match = of_match_device(ath_ahb_of_match, &pdev->dev);
> +		if (!match) {
> +			dev_err(&pdev->dev, "no device match found\n");
> +			return -EINVAL;
> +		}
> +
> +		devid = (u16)(unsigned long)match->data;

    of_device_get_match_data() instead of the above perhaps?

[...]

MBR, Sergei

[PATCH 3/7] ath9k: Add support for reading the EEPROM data using the nvmem API

[Alban]

Currently SoC platforms use a firmware request to get the EEPROM data.
This is mostly a hack and rely on using a user-helper scripts which is
deprecated. A nicer alternative is to use the nvmem API which was
designed for this kind of task.

Furthermore we let CONFIG_ATH9K_AHB select CONFIG_NVMEM as such
devices will generally use this method for loading the EEPROM data.

Signed-off-by: Alban <albeu@free.fr>
---
 drivers/net/wireless/ath/ath9k/Kconfig  |  1 +
 drivers/net/wireless/ath/ath9k/eeprom.c | 10 ++++++++++
 drivers/net/wireless/ath/ath9k/hw.h     |  2 ++
 drivers/net/wireless/ath/ath9k/init.c   | 21 +++++++++++++++++++++
 4 files changed, 34 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index 783a38f..1558c03 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -49,6 +49,7 @@ config ATH9K_PCI
 config ATH9K_AHB
 	bool "Atheros ath9k AHB bus support"
 	depends on ATH9K
+	select NVMEM
 	default n
 	---help---
 	  This option enables the AHB bus support in ath9k.
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.c b/drivers/net/wireless/ath/ath9k/eeprom.c
index fb80ec8..1f28222 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom.c
@@ -127,6 +127,14 @@ static bool ath9k_hw_nvram_read_pdata(struct ath9k_platform_data *pdata,
 					 offset, data);
 }
 
+static bool ath9k_hw_nvram_read_data(struct ath_hw *ah,
+				     off_t offset, u16 *data)
+{
+	return ath9k_hw_nvram_read_array(ah->eeprom_data,
+					 ah->eeprom_size / 2,
+					 offset, data);
+}
+
 static bool ath9k_hw_nvram_read_firmware(const struct firmware *eeprom_blob,
 					 off_t offset, u16 *data)
 {
@@ -143,6 +151,8 @@ bool ath9k_hw_nvram_read(struct ath_hw *ah, u32 off, u16 *data)
 
 	if (ah->eeprom_blob)
 		ret = ath9k_hw_nvram_read_firmware(ah->eeprom_blob, off, data);
+	else if (ah->eeprom_data)
+		ret = ath9k_hw_nvram_read_data(ah, off, data);
 	else if (pdata && !pdata->use_eeprom && pdata->eeprom_data)
 		ret = ath9k_hw_nvram_read_pdata(pdata, off, data);
 	else
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 9cbca12..7f17c2a 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -970,6 +970,8 @@ struct ath_hw {
 	bool disable_5ghz;
 
 	const struct firmware *eeprom_blob;
+	void *eeprom_data;
+	size_t eeprom_size;
 
 	struct ath_dynack dynack;
 
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index fa4b3cc..054f254 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/relay.h>
 #include <net/ieee80211_radiotap.h>
 
@@ -511,6 +512,7 @@ static int ath9k_eeprom_request(struct ath_softc *sc, const char *name)
 static void ath9k_eeprom_release(struct ath_softc *sc)
 {
 	release_firmware(sc->sc_ah->eeprom_blob);
+	kfree(sc->sc_ah->eeprom_data);
 }
 
 static int ath9k_init_platform(struct ath_softc *sc)
@@ -654,6 +656,25 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	if (ret)
 		return ret;
 
+	/* If the EEPROM hasn't been retrieved via firmware request
+	 * use the nvmem API insted.
+	 */
+	if (!ah->eeprom_blob) {
+		struct nvmem_cell *eeprom_cell;
+
+		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
+		if (!IS_ERR(eeprom_cell)) {
+			ah->eeprom_data = nvmem_cell_read(
+				eeprom_cell, &ah->eeprom_size);
+			nvmem_cell_put(eeprom_cell);
+
+			if (IS_ERR(ah->eeprom_data)) {
+				dev_err(ah->dev, "failed to read eeprom");
+				return PTR_ERR(ah->eeprom_data);
+			}
+		}
+	}
+
 	if (ath9k_led_active_high != -1)
 		ah->config.led_active_high = ath9k_led_active_high == 1;
 
-- 
2.7.4

Re: [PATCH 3/7] ath9k: Add support for reading the EEPROM data using the nvmem API

[Rafał Miłecki]

On 03/13/2017 10:05 PM, Alban wrote:
> @@ -654,6 +656,25 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
>  	if (ret)
>  		return ret;
>
> +	/* If the EEPROM hasn't been retrieved via firmware request
> +	 * use the nvmem API insted.
> +	 */
> +	if (!ah->eeprom_blob) {
> +		struct nvmem_cell *eeprom_cell;
> +
> +		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
> +		if (!IS_ERR(eeprom_cell)) {
> +			ah->eeprom_data = nvmem_cell_read(
> +				eeprom_cell, &ah->eeprom_size);
> +			nvmem_cell_put(eeprom_cell);
> +
> +			if (IS_ERR(ah->eeprom_data)) {
> +				dev_err(ah->dev, "failed to read eeprom");

One trivial thing: missing line break.


> +				return PTR_ERR(ah->eeprom_data);
> +			}
> +		}
> +	}
> +
>  	if (ath9k_led_active_high != -1)
>  		ah->config.led_active_high = ath9k_led_active_high == 1;

Re: [PATCH 3/7] ath9k: Add support for reading the EEPROM data using the nvmem API

[Christian Lamparter]

On Monday, March 13, 2017 10:05:11 PM CET Alban wrote:
> Currently SoC platforms use a firmware request to get the EEPROM data.
> This is mostly a hack and rely on using a user-helper scripts which is
> deprecated. A nicer alternative is to use the nvmem API which was
> designed for this kind of task.
> 
> Furthermore we let CONFIG_ATH9K_AHB select CONFIG_NVMEM as such
> devices will generally use this method for loading the EEPROM data.
> 
> Signed-off-by: Alban <albeu@free.fr>
> ---
> @@ -654,6 +656,25 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
>  	if (ret)
>  		return ret;
>  
> +	/* If the EEPROM hasn't been retrieved via firmware request
> +	 * use the nvmem API insted.
> +	 */
> +	if (!ah->eeprom_blob) {
> +		struct nvmem_cell *eeprom_cell;
> +
> +		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
> +		if (!IS_ERR(eeprom_cell)) {
> +			ah->eeprom_data = nvmem_cell_read(
> +				eeprom_cell, &ah->eeprom_size);
> +			nvmem_cell_put(eeprom_cell);
> +
> +			if (IS_ERR(ah->eeprom_data)) {
> +				dev_err(ah->dev, "failed to read eeprom");
> +				return PTR_ERR(ah->eeprom_data);
> +			}
> +		}
> +	}
> +
>  	if (ath9k_led_active_high != -1)
>  		ah->config.led_active_high = ath9k_led_active_high == 1;
>  
Are you sure this works with AR93XX SoCs that have the calibration data
in the OTP?

I've added Chris to the CC, since he has a HiveAP 121 that has this
configuration, so he can test, whenever this is a problem or not.

But from what I can tell, devices with the calibration data in the
OTP would fail now. This is because the OTP is done by ath9k_hw_init()
which hasn't run yet (it's a bit further down the road in the same
function though). 

Note: the OTP doesn't store the whole caldata. Just a few parts.
A temporary "eeprom" gets constructed with the help of the 
ar9300_eep_templates in ar9003_eeprom.c's
ar9300_eeprom_restore_internal(). But from what I don't think, 
that the eeprom_blob is constructed/set by the functions in
ar9003_eeprom.

I think this will require an additional property like
qca,calibration-in-otp. What's your opinion?

Thanks,
Christian

Re: [PATCH 3/7] ath9k: Add support for reading the EEPROM data using the nvmem API

[Alban]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Tue, 14 Mar 2017 00:53:55 +0100
Christian Lamparter <chunkeey@googlemail.com> wrote:

> On Monday, March 13, 2017 10:05:11 PM CET Alban wrote:
> > Currently SoC platforms use a firmware request to get the EEPROM data.
> > This is mostly a hack and rely on using a user-helper scripts which is
> > deprecated. A nicer alternative is to use the nvmem API which was
> > designed for this kind of task.
> > 
> > Furthermore we let CONFIG_ATH9K_AHB select CONFIG_NVMEM as such
> > devices will generally use this method for loading the EEPROM data.
> > 
> > Signed-off-by: Alban <albeu@free.fr>
> > ---
> > @@ -654,6 +656,25 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* If the EEPROM hasn't been retrieved via firmware request
> > +	 * use the nvmem API insted.
> > +	 */
> > +	if (!ah->eeprom_blob) {
> > +		struct nvmem_cell *eeprom_cell;
> > +
> > +		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
> > +		if (!IS_ERR(eeprom_cell)) {
> > +			ah->eeprom_data = nvmem_cell_read(
> > +				eeprom_cell, &ah->eeprom_size);
> > +			nvmem_cell_put(eeprom_cell);
> > +
> > +			if (IS_ERR(ah->eeprom_data)) {
> > +				dev_err(ah->dev, "failed to read eeprom");
> > +				return PTR_ERR(ah->eeprom_data);
> > +			}
> > +		}
> > +	}
> > +
> >  	if (ath9k_led_active_high != -1)
> >  		ah->config.led_active_high = ath9k_led_active_high == 1;
> >    
> Are you sure this works with AR93XX SoCs that have the calibration data
> in the OTP?

I only tested this on an ar9132 platform, so it might well be that a
few things are missing for the newer SoC. However this shouldn't break
anything existing as a platform needs to define an nvmem cell on the
athk9 device for this code to be used at all.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/7] ath9k: Add support for reading the EEPROM data using the nvmem API

[Christian Lamparter]

On Thursday, March 23, 2017 3:43:28 PM CET Alban wrote:
> On Tue, 14 Mar 2017 00:53:55 +0100
> Christian Lamparter <chunkeey@googlemail.com> wrote:
> 
> > On Monday, March 13, 2017 10:05:11 PM CET Alban wrote:
> > > Currently SoC platforms use a firmware request to get the EEPROM data.
> > > This is mostly a hack and rely on using a user-helper scripts which is
> > > deprecated. A nicer alternative is to use the nvmem API which was
> > > designed for this kind of task.
> > > 
> > > Furthermore we let CONFIG_ATH9K_AHB select CONFIG_NVMEM as such
> > > devices will generally use this method for loading the EEPROM data.
> > > 
> > > Signed-off-by: Alban <albeu@free.fr>
> > > ---
> > > @@ -654,6 +656,25 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/* If the EEPROM hasn't been retrieved via firmware request
> > > +	 * use the nvmem API insted.
> > > +	 */
> > > +	if (!ah->eeprom_blob) {
> > > +		struct nvmem_cell *eeprom_cell;
> > > +
> > > +		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
> > > +		if (!IS_ERR(eeprom_cell)) {
> > > +			ah->eeprom_data = nvmem_cell_read(
> > > +				eeprom_cell, &ah->eeprom_size);
> > > +			nvmem_cell_put(eeprom_cell);
> > > +
> > > +			if (IS_ERR(ah->eeprom_data)) {
> > > +				dev_err(ah->dev, "failed to read eeprom");
> > > +				return PTR_ERR(ah->eeprom_data);
> > > +			}
> > > +		}
> > > +	}
> > > +
> > >  	if (ath9k_led_active_high != -1)
> > >  		ah->config.led_active_high = ath9k_led_active_high == 1;
> > >    
> > Are you sure this works with AR93XX SoCs that have the calibration data
> > in the OTP?
> 
> I only tested this on an ar9132 platform, so it might well be that a
> few things are missing for the newer SoC. However this shouldn't break
> anything existing as a platform needs to define an nvmem cell on the
> athk9 device for this code to be used a all.

Yes, I looked at it.
+		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
+		if (!IS_ERR(eeprom_cell)) {
+			...
+		}
So if there's a error with the "eeprom" nvmem property,
(i.e.: it's not present) the driver should works as before.

Thanks,
Christian

Re: [PATCH 3/7] ath9k: Add support for reading the EEPROM data using the nvmem API

[Christian Lamparter]

On Thursday, March 23, 2017 3:43:28 PM CET Alban wrote:
> On Tue, 14 Mar 2017 00:53:55 +0100
> Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> 
> > On Monday, March 13, 2017 10:05:11 PM CET Alban wrote:
> > > Currently SoC platforms use a firmware request to get the EEPROM data.
> > > This is mostly a hack and rely on using a user-helper scripts which is
> > > deprecated. A nicer alternative is to use the nvmem API which was
> > > designed for this kind of task.
> > > 
> > > Furthermore we let CONFIG_ATH9K_AHB select CONFIG_NVMEM as such
> > > devices will generally use this method for loading the EEPROM data.
> > > 
> > > Signed-off-by: Alban <albeu-GANU6spQydw@public.gmane.org>
> > > ---
> > > @@ -654,6 +656,25 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/* If the EEPROM hasn't been retrieved via firmware request
> > > +	 * use the nvmem API insted.
> > > +	 */
> > > +	if (!ah->eeprom_blob) {
> > > +		struct nvmem_cell *eeprom_cell;
> > > +
> > > +		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
> > > +		if (!IS_ERR(eeprom_cell)) {
> > > +			ah->eeprom_data = nvmem_cell_read(
> > > +				eeprom_cell, &ah->eeprom_size);
> > > +			nvmem_cell_put(eeprom_cell);
> > > +
> > > +			if (IS_ERR(ah->eeprom_data)) {
> > > +				dev_err(ah->dev, "failed to read eeprom");
> > > +				return PTR_ERR(ah->eeprom_data);
> > > +			}
> > > +		}
> > > +	}
> > > +
> > >  	if (ath9k_led_active_high != -1)
> > >  		ah->config.led_active_high = ath9k_led_active_high == 1;
> > >    
> > Are you sure this works with AR93XX SoCs that have the calibration data
> > in the OTP?
> 
> I only tested this on an ar9132 platform, so it might well be that a
> few things are missing for the newer SoC. However this shouldn't break
> anything existing as a platform needs to define an nvmem cell on the
> athk9 device for this code to be used a all.

Yes, I looked at it.
+		eeprom_cell = nvmem_cell_get(ah->dev, "eeprom");
+		if (!IS_ERR(eeprom_cell)) {
+			...
+		}
So if there's a error with the "eeprom" nvmem property,
(i.e.: it's not present) the driver should works as before.

Thanks,
Christian

[PATCH 4/7] ath9k: Add support for reading the MAC address with nvmem

[Alban]

On embedded platforms the MAC address is often stored in flash,
use nvmem to read it if the platform data or DT didn't specify one.

Signed-off-by: Alban <albeu@free.fr>
---
 drivers/net/wireless/ath/ath9k/init.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 054f254..36b51a5 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -594,6 +594,35 @@ static int ath9k_of_init(struct ath_softc *sc)
 	return 0;
 }
 
+static int ath9k_get_nvmem_address(struct ath_softc *sc)
+{
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	struct nvmem_cell *cell;
+	size_t cell_size;
+	int err = 0;
+	void *mac;
+
+	cell = nvmem_cell_get(sc->dev, "address");
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	mac = nvmem_cell_read(cell, &cell_size);
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(mac))
+		return PTR_ERR(mac);
+
+	if (cell_size == 6) {
+		ether_addr_copy(common->macaddr, mac);
+	} else {
+		dev_err(sc->dev, "nvmem 'address' cell has invalid size\n");
+		err = -EINVAL;
+	}
+
+	kfree(mac);
+	return err;
+}
+
 static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 			    const struct ath_bus_ops *bus_ops)
 {
@@ -656,6 +685,10 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	if (ret)
 		return ret;
 
+	/* If no MAC address has been set yet try to use nvmem */
+	if (!is_valid_ether_addr(common->macaddr))
+		ath9k_get_nvmem_address(sc);
+
 	/* If the EEPROM hasn't been retrieved via firmware request
 	 * use the nvmem API insted.
 	 */
-- 
2.7.4

[PATCH 5/7] ath9k: of: Use the clk API to get the reference clock rate

[Alban]

If a clock named "ref" exists use it to get the reference clock rate.

Signed-off-by: Alban <albeu@free.fr>
---
 drivers/net/wireless/ath/ath9k/init.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 36b51a5..5cb9c61 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -24,6 +24,7 @@
 #include <linux/of_net.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/relay.h>
+#include <linux/clk.h>
 #include <net/ieee80211_radiotap.h>
 
 #include "ath9k.h"
@@ -564,6 +565,7 @@ static int ath9k_of_init(struct ath_softc *sc)
 	struct ath_hw *ah = sc->sc_ah;
 	struct ath_common *common = ath9k_hw_common(ah);
 	enum ath_bus_type bus_type = common->bus_ops->ath_bus_type;
+	struct clk *clk;
 	const char *mac;
 	char eeprom_name[100];
 	int ret;
@@ -573,6 +575,12 @@ static int ath9k_of_init(struct ath_softc *sc)
 
 	ath_dbg(common, CONFIG, "parsing configuration from OF node\n");
 
+	clk = clk_get(sc->dev, "ref");
+	if (!IS_ERR(clk)) {
+		ah->is_clk_25mhz = (clk_get_rate(clk) == 25000000);
+		clk_put(clk);
+	}
+
 	if (of_property_read_bool(np, "qca,no-eeprom")) {
 		/* ath9k-eeprom-<bus>-<id>.bin */
 		scnprintf(eeprom_name, sizeof(eeprom_name),
-- 
2.7.4

Re: [PATCH 5/7] ath9k: of: Use the clk API to get the reference clock rate

[Rafał Miłecki]

On 03/13/2017 10:05 PM, Alban wrote:
> @@ -573,6 +575,12 @@ static int ath9k_of_init(struct ath_softc *sc)
>
>  	ath_dbg(common, CONFIG, "parsing configuration from OF node\n");
>
> +	clk = clk_get(sc->dev, "ref");
> +	if (!IS_ERR(clk)) {
> +		ah->is_clk_25mhz = (clk_get_rate(clk) == 25000000);

One trivial thing: you don't need these extra braces.


> +		clk_put(clk);
> +	}

[PATCH 6/7] ath9k: Allow using the reset API for the external reset

[Alban]

From: Alban Bedel <albeu@free.fr>

Allow using the reset API instead of a platform specific callback to
reset the device.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/net/wireless/ath/ath9k/hw.c   | 26 +++++++++++++++++++++-----
 drivers/net/wireless/ath/ath9k/hw.h   |  1 +
 drivers/net/wireless/ath/ath9k/init.c | 10 ++++++++++
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 8c5c2dd..efc0435 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -21,6 +21,7 @@
 #include <linux/bitops.h>
 #include <linux/etherdevice.h>
 #include <linux/gpio.h>
+#include <linux/reset.h>
 #include <asm/unaligned.h>
 
 #include "hw.h"
@@ -551,6 +552,24 @@ static int ath9k_hw_attach_ops(struct ath_hw *ah)
 	return 0;
 }
 
+static int ath9k_hw_external_reset(struct ath_hw *ah)
+{
+	int ret = 0;
+
+	ath_dbg(ath9k_hw_common(ah), RESET,
+		"reset MAC via external reset\n");
+
+	if (ah->external_reset) {
+		ret = ah->external_reset();
+	} else if (ah->reset) {
+		ret = reset_control_assert(ah->reset);
+		if (!ret)
+			ret = reset_control_deassert(ah->reset);
+	}
+
+	return ret;
+}
+
 /* Called for all hardware families */
 static int __ath9k_hw_init(struct ath_hw *ah)
 {
@@ -1286,14 +1305,11 @@ static bool ath9k_hw_ar9330_reset_war(struct ath_hw *ah, int type)
 			break;
 	}
 
-	if (ah->external_reset &&
+	if ((ah->reset || ah->external_reset) &&
 	    (npend || type == ATH9K_RESET_COLD)) {
 		int reset_err = 0;
 
-		ath_dbg(ath9k_hw_common(ah), RESET,
-			"reset MAC via external reset\n");
-
-		reset_err = ah->external_reset();
+		reset_err = ath9k_hw_external_reset(ah);
 		if (reset_err) {
 			ath_err(ath9k_hw_common(ah),
 				"External reset failed, err=%d\n",
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 7f17c2a..53b67e3 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -966,6 +966,7 @@ struct ath_hw {
 	bool is_clk_25mhz;
 	int (*get_mac_revision)(void);
 	int (*external_reset)(void);
+	struct reset_control *reset;
 	bool disable_2ghz;
 	bool disable_5ghz;
 
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 5cb9c61..f1cb806 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -25,6 +25,7 @@
 #include <linux/nvmem-consumer.h>
 #include <linux/relay.h>
 #include <linux/clk.h>
+#include <linux/reset.h>
 #include <net/ieee80211_radiotap.h>
 
 #include "ath9k.h"
@@ -697,6 +698,15 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	if (!is_valid_ether_addr(common->macaddr))
 		ath9k_get_nvmem_address(sc);
 
+	/* Try to get a reset controller */
+	ah->reset = devm_reset_control_get_optional(sc->dev, NULL);
+	if (IS_ERR(ah->reset)) {
+		if (PTR_ERR(ah->reset) != -ENOENT &&
+		    PTR_ERR(ah->reset) != -ENOTSUPP)
+			return PTR_ERR(ah->reset);
+		ah->reset = NULL;
+	}
+
 	/* If the EEPROM hasn't been retrieved via firmware request
 	 * use the nvmem API insted.
 	 */
-- 
2.7.4

[PATCH 7/7] ath9k: hw: Reset the device with the external reset before init

[Alban]

On the SoC platform the board code often manually reset the device
before registering it. To allow the same to happen on DT platforms
let the driver call the reset before init.

Signed-off-by: Alban <albeu@free.fr>
---
 drivers/net/wireless/ath/ath9k/hw.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index efc0435..dfb13bc 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -576,6 +576,13 @@ static int __ath9k_hw_init(struct ath_hw *ah)
 	struct ath_common *common = ath9k_hw_common(ah);
 	int r = 0;
 
+	/* Reset the device before using it */
+	r = ath9k_hw_external_reset(ah);
+	if (r) {
+		ath_err(common, "Failed to reset chip\n");
+		return r;
+	}
+
 	ath9k_hw_read_revisions(ah);
 
 	switch (ah->hw_version.macVersion) {
-- 
2.7.4

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Rob Herring]

On Mon, Mar 13, 2017 at 10:05:09PM +0100, Alban wrote:
> The current binding only cover PCI devices so extend it for SoC devices.
> 
> Most SoC platforms use an MTD partition for the calibration data
> instead of an EEPROM. The qca,no-eeprom property was added to allow
> loading the EEPROM content using firmware loading. This new binding
> replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
> property as deprecated in case anyone ever used it.
> 
> Signed-off-by: Alban <albeu@free.fr>
> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 3 deletions(-)

For the subject, "dt-bindings: net: ..." and one nit below, otherwise:

Acked-by: Rob Herring <robh@kernel.org>

> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> index b7396c8..61f5f6d 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -27,16 +27,34 @@ Required properties:
>  			- 0034 for AR9462
>  			- 0036 for AR9565
>  			- 0037 for AR9485
> +		For SoC devices the compatible should be "qca,<soctype>-wmac"
> +		and one of the following fallbacks:
> +			- "qca,ar9100-wmac"
> +			- "qca,ar9330-wmac"
> +			- "qca,ar9340-wmac"
> +			- "qca,qca9550-wmac"
> +			- "qca,qca9530-wmac"
>  - reg: Address and length of the register set for the device.
>  
> +Required properties for SoC devices:
> +- interrupt-parent: phandle of the parent interrupt controller.
> +- interrupts: Interrupt specifier for the controllers interrupt.
> +
>  Optional properties:
> +- mac-address: See ethernet.txt in the parent directory
> +- local-mac-address: See ethernet.txt in the parent directory
> +- clock-names: has to be "ref"
> +- clocks: phandle of the reference clock
> +- resets: phandle of the reset line
> +- nvmem-cell-names: has to be "eeprom" and/or "address"
> +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address
> +		nvmem cell.
> +
> +Deprecated properties:
>  - qca,no-eeprom: Indicates that there is no physical EEPROM connected to the
>  			ath9k wireless chip (in this case the calibration /
>  			EEPROM data will be loaded from userspace using the
>  			kernel firmware loader).
> -- mac-address: See ethernet.txt in the parent directory
> -- local-mac-address: See ethernet.txt in the parent directory
> -
>  
>  In this example, the node is defined as child node of the PCI controller:
>  &pci0 {
> @@ -46,3 +64,20 @@ In this example, the node is defined as child node of the PCI controller:
>  		qca,no-eeprom;
>  	};
>  };
> +
> +In this example it is defined as a SoC device:
> +	wmac@180c0000 {

wifi@...

> +		compatible = "qca,ar9132-wmac", "qca,ar9100-wmac";
> +		reg = <0x180c0000 0x30000>;
> +
> +		interrupt-parent = <&cpuintc>;
> +		interrupts = <2>;
> +
> +		clock-names = "ref";
> +		clocks = <&extosc>;
> +
> +		nvmem-cell-names = "eeprom", "address";
> +		nvmem-cells = <&wmac_eeprom>, <&wmac_address>;
> +
> +		resets = <&rst 22>;
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Christian Lamparter]

On Monday, March 13, 2017 10:05:09 PM CEST Alban wrote:
> The current binding only cover PCI devices so extend it for SoC devices.
> 
> Most SoC platforms use an MTD partition for the calibration data
> instead of an EEPROM. The qca,no-eeprom property was added to allow
> loading the EEPROM content using firmware loading. This new binding
> replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
> property as deprecated in case anyone ever used it.

Please don't mark "qca,no-eeprom" as deprecated then.
If some devices geniously need to rely on userspace for extracting 
and processing the calibration data, it should be stay a
optional properties.

For example: A device that can't do easily without "qca,no-eeprom" is
the AVM FRITZ!WLAN Repeater 300E. For this device, the caldata 
is stored in the flash, however for whatever reason the vendor
choose to "reverse" it. (like completely back to front, not byteswapped
or something). So an extra "unreversing step" is required. So, it would
require some sort of a special nvmem-provider-processor as an 
alternative.

> Signed-off-by: Alban <albeu@free.fr>
> ---
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> index b7396c8..61f5f6d 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -27,16 +27,34 @@ Required properties:
>  			- 0034 for AR9462
>  			- 0036 for AR9565
>  			- 0037 for AR9485
> +		For SoC devices the compatible should be "qca,<soctype>-wmac"
> +		and one of the following fallbacks:
> +			- "qca,ar9100-wmac"
> +			- "qca,ar9330-wmac"
> +			- "qca,ar9340-wmac"
> +			- "qca,qca9550-wmac"
> +			- "qca,qca9530-wmac"
>  - reg: Address and length of the register set for the device.
>  
> +Required properties for SoC devices:
> +- interrupt-parent: phandle of the parent interrupt controller.
> +- interrupts: Interrupt specifier for the controllers interrupt.
> +
>  Optional properties:
> +- mac-address: See ethernet.txt in the parent directory
> +- local-mac-address: See ethernet.txt in the parent directory
> +- clock-names: has to be "ref"
> +- clocks: phandle of the reference clock
> +- resets: phandle of the reset line
> +- nvmem-cell-names: has to be "eeprom" and/or "address"
> +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address
> +		nvmem cell.
> +
> +Deprecated properties:
>  - qca,no-eeprom: Indicates that there is no physical EEPROM connected to the
>  			ath9k wireless chip (in this case the calibration /
>  			EEPROM data will be loaded from userspace using the
>  			kernel firmware loader).
> -- mac-address: See ethernet.txt in the parent directory
> -- local-mac-address: See ethernet.txt in the parent directory
> -
It sounds like you want to deprecate mac-address and local-mac-address as well.
If so you sould add this to the commit as well. From my point of view, people
mostly flat-out patched the eeprom-image if they wanted to set the mac-address.
However, this was an extra step, if nvmem does away with it, I'm completely
fine with deprecating these properties.

Thanks,
Christian

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Alban]

[-- Attachment #1: Type: text/plain, Size: 4098 bytes --]

On Mon, 27 Mar 2017 18:11:15 +0200
Christian Lamparter <chunkeey@googlemail.com> wrote:

> On Monday, March 13, 2017 10:05:09 PM CEST Alban wrote:
> > The current binding only cover PCI devices so extend it for SoC devices.
> > 
> > Most SoC platforms use an MTD partition for the calibration data
> > instead of an EEPROM. The qca,no-eeprom property was added to allow
> > loading the EEPROM content using firmware loading. This new binding
> > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
> > property as deprecated in case anyone ever used it.  
> 
> Please don't mark "qca,no-eeprom" as deprecated then.
> If some devices geniously need to rely on userspace for extracting 
> and processing the calibration data, it should be stay a
> optional properties.

Deprecated just mean that it shouldn't be used for new devices. But as
it is not used by any board, misuse the firmware loading API and
firmware loader user helper are deprecated in udev, I find we could also
just drop it.

> For example: A device that can't do easily without "qca,no-eeprom" is
> the AVM FRITZ!WLAN Repeater 300E. For this device, the caldata 
> is stored in the flash, however for whatever reason the vendor
> choose to "reverse" it. (like completely back to front, not byteswapped
> or something). So an extra "unreversing step" is required. So, it would
> require some sort of a special nvmem-provider-processor as an 
> alternative.

Or just handle this special eeprom format in the ath9k driver. I doubt
that this case is so common that it would justify adding a whole new
layer to nvmem.

> > Signed-off-by: Alban <albeu@free.fr>
> > ---
> > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> > index b7396c8..61f5f6d 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> > @@ -27,16 +27,34 @@ Required properties:
> >  			- 0034 for AR9462
> >  			- 0036 for AR9565
> >  			- 0037 for AR9485
> > +		For SoC devices the compatible should be "qca,<soctype>-wmac"
> > +		and one of the following fallbacks:
> > +			- "qca,ar9100-wmac"
> > +			- "qca,ar9330-wmac"
> > +			- "qca,ar9340-wmac"
> > +			- "qca,qca9550-wmac"
> > +			- "qca,qca9530-wmac"
> >  - reg: Address and length of the register set for the device.
> >  
> > +Required properties for SoC devices:
> > +- interrupt-parent: phandle of the parent interrupt controller.
> > +- interrupts: Interrupt specifier for the controllers interrupt.
> > +
> >  Optional properties:
> > +- mac-address: See ethernet.txt in the parent directory
> > +- local-mac-address: See ethernet.txt in the parent directory
> > +- clock-names: has to be "ref"
> > +- clocks: phandle of the reference clock
> > +- resets: phandle of the reset line
> > +- nvmem-cell-names: has to be "eeprom" and/or "address"
> > +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address
> > +		nvmem cell.
> > +
> > +Deprecated properties:
> >  - qca,no-eeprom: Indicates that there is no physical EEPROM
> > connected to the ath9k wireless chip (in this case the calibration /
> >  			EEPROM data will be loaded from userspace
> > using the kernel firmware loader).
> > -- mac-address: See ethernet.txt in the parent directory
> > -- local-mac-address: See ethernet.txt in the parent directory
> > -  
> It sounds like you want to deprecate mac-address and
> local-mac-address as well. If so you sould add this to the commit as
> well. From my point of view, people mostly flat-out patched the
> eeprom-image if they wanted to set the mac-address. However, this was
> an extra step, if nvmem does away with it, I'm completely fine with
> deprecating these properties.

The produced diff is very misleading because the mac-address properties
get lumped with the other new optional  properties. But if you look
closely it just move qca,no-eeprom to the deprecated section.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Alban]

[-- Attachment #1: Type: text/plain, Size: 4144 bytes --]

On Mon, 27 Mar 2017 18:11:15 +0200
Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:

> On Monday, March 13, 2017 10:05:09 PM CEST Alban wrote:
> > The current binding only cover PCI devices so extend it for SoC devices.
> > 
> > Most SoC platforms use an MTD partition for the calibration data
> > instead of an EEPROM. The qca,no-eeprom property was added to allow
> > loading the EEPROM content using firmware loading. This new binding
> > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
> > property as deprecated in case anyone ever used it.  
> 
> Please don't mark "qca,no-eeprom" as deprecated then.
> If some devices geniously need to rely on userspace for extracting 
> and processing the calibration data, it should be stay a
> optional properties.

Deprecated just mean that it shouldn't be used for new devices. But as
it is not used by any board, misuse the firmware loading API and
firmware loader user helper are deprecated in udev, I find we could also
just drop it.

> For example: A device that can't do easily without "qca,no-eeprom" is
> the AVM FRITZ!WLAN Repeater 300E. For this device, the caldata 
> is stored in the flash, however for whatever reason the vendor
> choose to "reverse" it. (like completely back to front, not byteswapped
> or something). So an extra "unreversing step" is required. So, it would
> require some sort of a special nvmem-provider-processor as an 
> alternative.

Or just handle this special eeprom format in the ath9k driver. I doubt
that this case is so common that it would justify adding a whole new
layer to nvmem.

> > Signed-off-by: Alban <albeu-GANU6spQydw@public.gmane.org>
> > ---
> > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> > index b7396c8..61f5f6d 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> > @@ -27,16 +27,34 @@ Required properties:
> >  			- 0034 for AR9462
> >  			- 0036 for AR9565
> >  			- 0037 for AR9485
> > +		For SoC devices the compatible should be "qca,<soctype>-wmac"
> > +		and one of the following fallbacks:
> > +			- "qca,ar9100-wmac"
> > +			- "qca,ar9330-wmac"
> > +			- "qca,ar9340-wmac"
> > +			- "qca,qca9550-wmac"
> > +			- "qca,qca9530-wmac"
> >  - reg: Address and length of the register set for the device.
> >  
> > +Required properties for SoC devices:
> > +- interrupt-parent: phandle of the parent interrupt controller.
> > +- interrupts: Interrupt specifier for the controllers interrupt.
> > +
> >  Optional properties:
> > +- mac-address: See ethernet.txt in the parent directory
> > +- local-mac-address: See ethernet.txt in the parent directory
> > +- clock-names: has to be "ref"
> > +- clocks: phandle of the reference clock
> > +- resets: phandle of the reset line
> > +- nvmem-cell-names: has to be "eeprom" and/or "address"
> > +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address
> > +		nvmem cell.
> > +
> > +Deprecated properties:
> >  - qca,no-eeprom: Indicates that there is no physical EEPROM
> > connected to the ath9k wireless chip (in this case the calibration /
> >  			EEPROM data will be loaded from userspace
> > using the kernel firmware loader).
> > -- mac-address: See ethernet.txt in the parent directory
> > -- local-mac-address: See ethernet.txt in the parent directory
> > -  
> It sounds like you want to deprecate mac-address and
> local-mac-address as well. If so you sould add this to the commit as
> well. From my point of view, people mostly flat-out patched the
> eeprom-image if they wanted to set the mac-address. However, this was
> an extra step, if nvmem does away with it, I'm completely fine with
> deprecating these properties.

The produced diff is very misleading because the mac-address properties
get lumped with the other new optional  properties. But if you look
closely it just move qca,no-eeprom to the deprecated section.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Christian Lamparter]

On Tuesday, March 28, 2017 10:44:41 AM CEST Alban wrote:
> On Mon, 27 Mar 2017 18:11:15 +0200
> Christian Lamparter <chunkeey@googlemail.com> wrote:
> 
> > On Monday, March 13, 2017 10:05:09 PM CEST Alban wrote:
> > > The current binding only cover PCI devices so extend it for SoC devices.
> > > 
> > > Most SoC platforms use an MTD partition for the calibration data
> > > instead of an EEPROM. The qca,no-eeprom property was added to allow
> > > loading the EEPROM content using firmware loading. This new binding
> > > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
> > > property as deprecated in case anyone ever used it.  
> > 
> > Please don't mark "qca,no-eeprom" as deprecated then.
> > If some devices geniously need to rely on userspace for extracting 
> > and processing the calibration data, it should be stay a
> > optional properties.
> 
> Deprecated just mean that it shouldn't be used for new devices. 
> But as it is not used by any board, misuse the firmware loading API and
> firmware loader user helper are deprecated in udev, I find we could also
> just drop it.
But LEDE/OpenWRT rely on the firmware loading API more than ever and 
currently there is not a replacement for it. From what I know, Luis 
tried to replace it with his sysdata approach:

<https://lkml.org/lkml/2016/12/16/204>

however, this was disliked by Greg KH and Linus for the following reasons.
<https://lkml.org/lkml/2016/6/16/995>:

|So I absolutely abhor "changes for changes sake".
|
|If the existing code works for existing drivers, let them keep it.
|
|And if a new interface is truly more flexible, then it should be able
|to implement the old interface with no changes, so that drivers
|shouldn't need to be changed/upgraded.
|
|Then, drivers that actually _want_ new features, or that can take
|advantage of new interfaces to actually make things *simpler*, can
|choose to make those changes. But those changes should have real
|advantages.
|[...]


your nvmem approach would need to be as universal and
powerful as the "qca,no-eeprom" + userspace solutions
in order to deprecate it.
 
> > For example: A device that can't do easily without "qca,no-eeprom" is
> > the AVM FRITZ!WLAN Repeater 300E. For this device, the caldata 
> > is stored in the flash, however for whatever reason the vendor
> > choose to "reverse" it. (like completely back to front, not byteswapped
> > or something). So an extra "unreversing step" is required. So, it would
> > require some sort of a special nvmem-provider-processor as an 
> > alternative.
> 
> Or just handle this special eeprom format in the ath9k driver. I doubt
> that this case is so common that it would justify adding a whole new
> layer to nvmem.
Well, you'll have to deal with it in nvmem, if you want it to deprecate
"qca,no-eeprom".

I looked into 10-ath9k-eeprom [0] of LEDE's AR71XX target and I noticed
that quite a few devices patch the MACs of the wifi.
If you look at the code for the Airtight C-55 and C-60, Meraki MR18,
Meraki Z1, you'll notice that each one has to add a fixed value (+1,
+2, ...) to the extraced MAC-Address. So how would you replicate this,
with "nvmem-cell-names = address" without some sort of 
nvmem-provider-processor?

Also, there's another usecase of a nvmem-provider-processor. 
For example, one could be convert all the different types of
ascii-macs (Either strings like "00:11:22:33:44:55", 
"00.11.22.33..." or "00112233..." ) to their binary representation.
For AR71XX, this is mostly done by ath79_parse_ascii_mac:

https://github.com/lede-project/source/blob/master/target/linux/ar71xx/files/arch/mips/ath79/dev-eth.c#L1204

and grep lists the following devices:
mach-dgl-5500-a1.c, mach-dhp-1565-a1.c, mach-dir-505-a1.c, mach-dir-615-c1.c
mach-dir-615-i1.c, mach-dir-825-b1.c, mach-dir-825-c1.c, mach-tew-673gru.c
mach-tew-712br.c, mach-tew-732br.c, mach-tew-823dru.c

I did a quick check: All of them use the extracted MACs for ath9k
and/or ethernet.

Note: I think Ralink/MediaTek will have the same issues.

> > >  Optional properties:
> > > +- mac-address: See ethernet.txt in the parent directory
> > > +- local-mac-address: See ethernet.txt in the parent directory
> > > [...]
> > > +
> > > +Deprecated properties:
> > >  - qca,no-eeprom: Indicates that there is no physical EEPROM
> > > connected to the ath9k wireless chip (in this case the calibration /
> > >  			EEPROM data will be loaded from userspace
> > > using the kernel firmware loader).
> > > -- mac-address: See ethernet.txt in the parent directory
> > > -- local-mac-address: See ethernet.txt in the parent directory
> > > -  
> > It sounds like you want to deprecate mac-address and
> > local-mac-address as well. If so you sould add this to the commit as
> > well. From my point of view, people mostly flat-out patched the
> > eeprom-image if they wanted to set the mac-address. However, this was
> > an extra step, if nvmem does away with it, I'm completely fine with
> > deprecating these properties.
> 
> The produced diff is very misleading because the mac-address properties
> get lumped with the other new optional  properties. But if you look
> closely it just move qca,no-eeprom to the deprecated section.
Yes ok. This is the case.

Thanks,
Christian

[0] <https://github.com/lede-project/source/blob/master/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom#L85>

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Andrew Lunn]

> But LEDE/OpenWRT rely on the firmware loading API more than ever and 
> currently there is not a replacement for it.

....


> I looked into 10-ath9k-eeprom [0] of LEDE's AR71XX target and I noticed
> that quite a few devices patch the MACs of the wifi.
> If you look at the code for the Airtight C-55 and C-60, Meraki MR18,
> Meraki Z1, you'll notice that each one has to add a fixed value (+1,
> +2, ...) to the extraced MAC-Address. So how would you replicate this,
> with "nvmem-cell-names = address" without some sort of 
> nvmem-provider-processor?

...
 
> https://github.com/lede-project/source/blob/master/target/linux/ar71xx/files/arch/mips/ath79/dev-eth.c#L1204
> 
> and grep lists the following devices:
> mach-dgl-5500-a1.c, mach-dhp-1565-a1.c, mach-dir-505-a1.c, mach-dir-615-c1.c
> mach-dir-615-i1.c, mach-dir-825-b1.c, mach-dir-825-c1.c, mach-tew-673gru.c
> mach-tew-712br.c, mach-tew-732br.c, mach-tew-823dru.c

I would say a big part of the problem is that all of these use cases
are outside of mainline. Why should mainline support something which
is not actually used in mainline.

So i would suggest your first step is to bring some of these devices
into mainline. Once in mainline, it becomes a mainline issue, and
people will help get it solved.

       Andrew

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Andrew Lunn]

> But LEDE/OpenWRT rely on the firmware loading API more than ever and 
> currently there is not a replacement for it.

....


> I looked into 10-ath9k-eeprom [0] of LEDE's AR71XX target and I noticed
> that quite a few devices patch the MACs of the wifi.
> If you look at the code for the Airtight C-55 and C-60, Meraki MR18,
> Meraki Z1, you'll notice that each one has to add a fixed value (+1,
> +2, ...) to the extraced MAC-Address. So how would you replicate this,
> with "nvmem-cell-names = address" without some sort of 
> nvmem-provider-processor?

...
 
> https://github.com/lede-project/source/blob/master/target/linux/ar71xx/files/arch/mips/ath79/dev-eth.c#L1204
> 
> and grep lists the following devices:
> mach-dgl-5500-a1.c, mach-dhp-1565-a1.c, mach-dir-505-a1.c, mach-dir-615-c1.c
> mach-dir-615-i1.c, mach-dir-825-b1.c, mach-dir-825-c1.c, mach-tew-673gru.c
> mach-tew-712br.c, mach-tew-732br.c, mach-tew-823dru.c

I would say a big part of the problem is that all of these use cases
are outside of mainline. Why should mainline support something which
is not actually used in mainline.

So i would suggest your first step is to bring some of these devices
into mainline. Once in mainline, it becomes a mainline issue, and
people will help get it solved.

       Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Christian Lamparter]

On Tuesday, March 28, 2017 5:18:37 PM CEST Andrew Lunn wrote:
> > But LEDE/OpenWRT rely on the firmware loading API more than ever and 
> > currently there is not a replacement for it.
> 
> ....
> 
> 
> > I looked into 10-ath9k-eeprom [0] of LEDE's AR71XX target and I noticed
> > that quite a few devices patch the MACs of the wifi.
> > If you look at the code for the Airtight C-55 and C-60, Meraki MR18,
> > Meraki Z1, you'll notice that each one has to add a fixed value (+1,
> > +2, ...) to the extraced MAC-Address. So how would you replicate this,
> > with "nvmem-cell-names = address" without some sort of 
> > nvmem-provider-processor?
> 
> ...
>  
> > https://github.com/lede-project/source/blob/master/target/linux/ar71xx/files/arch/mips/ath79/dev-eth.c#L1204
> > 
> > and grep lists the following devices:
> > mach-dgl-5500-a1.c, mach-dhp-1565-a1.c, mach-dir-505-a1.c, mach-dir-615-c1.c
> > mach-dir-615-i1.c, mach-dir-825-b1.c, mach-dir-825-c1.c, mach-tew-673gru.c
> > mach-tew-712br.c, mach-tew-732br.c, mach-tew-823dru.c
> 
> I would say a big part of the problem is that all of these use cases
> are outside of mainline. Why should mainline support something which
> is not actually used in mainline.
> 
> So i would suggest your first step is to bring some of these devices
> into mainline. Once in mainline, it becomes a mainline issue, and
> people will help get it solved.
>
Oh, in that case you should probably go "all out" and ask on the 
LKML to remove all of the ath9k and ath10k ahb work. From what I
know all the "users" are running some sort of OpenWRT/LEDE or a 
derivative. This is because Atheros/QCA provided a SDK based on
OpenWRT.

Alban has been trying to convert the platform to device-tree
and add them to the mainline for a while now:
 
https://patchwork.kernel.org/patch/6514551/

So, you are questioning this work as well.

Thanks,
Christian

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Andrew Lunn]

> Oh, in that case you should probably go "all out" and ask on the 
> LKML to remove all of the ath9k and ath10k ahb work. From what I
> know all the "users" are running some sort of OpenWRT/LEDE or a 
> derivative. This is because Atheros/QCA provided a SDK based on
> OpenWRT.
> 
> Alban has been trying to convert the platform to device-tree
> and add them to the mainline for a while now:
>  
> https://patchwork.kernel.org/patch/6514551/
> 
> So, you are questioning this work as well.

Not at all. Ralph Sennhauser has been doing a great job of getting all
the Marvell devices into Mainline, and i help as much as i can, being
one of the Marvell SoC Maintainers.

I'm just saying, get a few boards which require these facilities into
the mainline, and then you have a much stranger base to argue from.

    Andrew

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Christian Lamparter]

On Tuesday, March 28, 2017 6:41:59 PM CEST Andrew Lunn wrote:
> > Oh, in that case you should probably go "all out" and ask on the 
> > LKML to remove all of the ath9k and ath10k ahb work. From what I
> > know all the "users" are running some sort of OpenWRT/LEDE or a 
> > derivative. This is because Atheros/QCA provided a SDK based on
> > OpenWRT.
> > 
> > Alban has been trying to convert the platform to device-tree
> > and add them to the mainline for a while now:
> >  
> > https://patchwork.kernel.org/patch/6514551/
> > 
> > So, you are questioning this work as well.
> 
> Not at all. Ralph Sennhauser has been doing a great job of getting all
> the Marvell devices into Mainline, and i help as much as i can, being
> one of the Marvell SoC Maintainers.
> 
> I'm just saying, get a few boards which require these facilities into
> the mainline, and then you have a much stronger base to argue from.

I was arguing not to deprecate "qca,no-eeprom" property.

based on this quote from Linus' <https://lkml.org/lkml/2016/6/16/995>:
|if a new interface is truly more flexible, then it should be able
|to implement the old interface with no changes, so that drivers
|shouldn't need to be changed/upgraded.

what stronger point to do you want?

Thanks,
Christian

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Christian Lamparter]

On Tuesday, March 28, 2017 6:41:59 PM CEST Andrew Lunn wrote:
> > Oh, in that case you should probably go "all out" and ask on the 
> > LKML to remove all of the ath9k and ath10k ahb work. From what I
> > know all the "users" are running some sort of OpenWRT/LEDE or a 
> > derivative. This is because Atheros/QCA provided a SDK based on
> > OpenWRT.
> > 
> > Alban has been trying to convert the platform to device-tree
> > and add them to the mainline for a while now:
> >  
> > https://patchwork.kernel.org/patch/6514551/
> > 
> > So, you are questioning this work as well.
> 
> Not at all. Ralph Sennhauser has been doing a great job of getting all
> the Marvell devices into Mainline, and i help as much as i can, being
> one of the Marvell SoC Maintainers.
> 
> I'm just saying, get a few boards which require these facilities into
> the mainline, and then you have a much stronger base to argue from.

I was arguing not to deprecate "qca,no-eeprom" property.

based on this quote from Linus' <https://lkml.org/lkml/2016/6/16/995>:
|if a new interface is truly more flexible, then it should be able
|to implement the old interface with no changes, so that drivers
|shouldn't need to be changed/upgraded.

what stronger point to do you want?

Thanks,
Christian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Kalle Valo]

Alban <albeu@free.fr> writes:

> The current binding only cover PCI devices so extend it for SoC devices.
>
> Most SoC platforms use an MTD partition for the calibration data
> instead of an EEPROM. The qca,no-eeprom property was added to allow
> loading the EEPROM content using firmware loading. This new binding
> replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
> property as deprecated in case anyone ever used it.

A patchset like this should definitely have a cover letter and explain
the backround and reasoning for this patchset. Currently I'm just
guessing.

> Signed-off-by: Alban <albeu@free.fr>

Please use your full name both in From and S-o-b lines.

--=20
Kalle Valo=

Re: [PATCH 1/7] Documentation: dt: net: Update the ath9k binding for SoC devices

[Kalle Valo]

Alban <albeu@free.fr> writes:

> The current binding only cover PCI devices so extend it for SoC devices.
>
> Most SoC platforms use an MTD partition for the calibration data
> instead of an EEPROM. The qca,no-eeprom property was added to allow
> loading the EEPROM content using firmware loading. This new binding
> replace this hack with NVMEM cells, so we also mark the qca,no-eeprom
> property as deprecated in case anyone ever used it.

A patchset like this should definitely have a cover letter and explain
the backround and reasoning for this patchset. Currently I'm just
guessing.

> Signed-off-by: Alban <albeu@free.fr>

Please use your full name both in From and S-o-b lines.

-- 
Kalle Valo