Re: [ath9k-devel] [PATCH v6 1/3] Documentation: dt: net: add ath9k wireless device binding

[Rob Herring]

On Tue, Sep 06, 2016 at 11:46:21PM +0200, Martin Blumenstingl wrote:
> Add documentation how devicetree can be used to configure ath9k based
> devices.
> 
> Signed-off-by: Martin Blumenstingl 
> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 39 
> ++
>  1 file changed, 39 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 000..77b9202
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -0,0 +1,39 @@
> +* 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: For PCI and PCIe devices this should be an identifier following
> + the format as defined in "PCI Bus Binding to Open Firmware"
> + Revision 2.1. One of the possible formats is "pci,"
> + where  is the PCI vendor ID and  is PCI device ID.
> +
> +Optional properties:
> +- reg: Address and length of the register set for the device.

reg is not optional.

> +- qca,clk-25mhz: Defines that a 25MHz clock is used
> +- 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).
> +- qca,disable-2ghz: Overrides the settings from the EEPROM and disables the
> + 2.4GHz band. Setting this property is only needed
> + when the RF circuit does not support the 2.4GHz band
> + while it is enabled nevertheless in the EEPROM.
> +- qca,disable-5ghz: Overrides the settings from the EEPROM and disables the
> + 5GHz band. Setting this property is only needed when
> + the RF circuit does not support the 5GHz band while
> + it is enabled nevertheless in the EEPROM.
> +- 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:
> + {
> + ath9k@168c,002d {

unit address is not the vendor/device ID, but the reg value. So 
'@7000,0,0' I think in this case. Double check the OF PCI bus binding.


> + compatible = "pci168c,002d";
> + reg = <0x7000 0 0 0 0x1000>;
> + qca,disable-5ghz;
> + };
> +};
> -- 
> 2.9.3
> 
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v6 1/3] Documentation: dt: net: add ath9k wireless device binding

[Rob Herring]

On Fri, Sep 09, 2016 at 10:57:06PM +0200, Martin Blumenstingl wrote:
> On Fri, Sep 9, 2016 at 9:48 AM, Oleksij Rempel  wrote:
> >> +Optional properties:
> >> +- reg: Address and length of the register set for the device.
> >> +- qca,clk-25mhz: Defines that a 25MHz clock is used
> >
> > Some SoCs even Atheros WiSoCs use WiFi clock for System Clock. In this
> > case we need to use clock framework any way, so why not in this case too?
> > Provide dummy static clock in DT and connect it with this node?
> >
> > osc25m: oscillator {
> > compatible = "fixed-clock";
> > #clock-cells = <0>;
> > clock-frequency = <2500>;
> > clock-accuracy = <3>;
> > };
> >
> > acc: clock-controller@8004 {
> > compatible = "some-clock-controller";
> > #clock-cells = <1>;
> > clocks = <>;
> > reg = <0x8004 0x204>;
> > };
> >
> >
> >  {
> > ath9k@168c,002d {
> > compatible = "pci168c,002d";
> > reg = <0x7000 0 0 0 0x1000>;
> > clocks = <>;
> > qca,disable-5ghz;
> > };
> > };
> >
> >
> > driver will need to use clk_get and compare frequency instead of
> > of_property_read_bool(np, "qca,clk-25mhz"). In this case you will need
> > to define source clock only one time and reuse it by all affected DT
> > nodes. If we have 40MHz clock you will only need to change it in
> > fixed-clock.
> that does sound like a good idea, thanks for that input!
> However, I will remove the property for the first version because I am
> trying to get PCI(e) devices supported. OF support for AHB (WiSoC)
> needs more work (in other places, like ahb.c) anyways.
> But this suggestion should be remembered!
> 
> >> +- 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).
> >> +- qca,disable-2ghz: Overrides the settings from the EEPROM and disables 
> >> the
> >> + 2.4GHz band. Setting this property is only needed
> >> + when the RF circuit does not support the 2.4GHz band
> >> + while it is enabled nevertheless in the EEPROM.
> >> +- qca,disable-5ghz: Overrides the settings from the EEPROM and disables 
> >> the
> >> + 5GHz band. Setting this property is only needed when
> >> + the RF circuit does not support the 5GHz band while
> >> + it is enabled nevertheless in the EEPROM.
> >
> > This option can be reused for every WiFi controller. Not only for qca.
> > So may be instead of adding vendor specific name, make it reusable for
> > all vendors. Instead of qca,disable-5ghz and qca,disable-2ghz make
> > disable-5ghz and disable-2ghz?

Fine, but if you do this then document in a common location.

> I am personally fine with anything that fits best into the grand
> scheme of things.
> There are three scenarios I can think of which may be influenced by
> devicetree configuration:
> a) let the driver decide automatically whether the 2.4GHz and/or 5GHz
> band is/are enabled
> b) explicitly disable either bands (because the driver thinks due to
> whatever reason that a band is supported while in reality it isn't)
> c) explicitly enable a band (for example because the driver cannot
> automagically detect which band should be enabled)
> 
> for ath9k we default to a) but also allow b): the EEPROM (device
> specific calibration data) contains information about which bands are
> enabled (or not). But some manufacturers are shipping devices for
> example with the 5G band enabled, while the actual hardware doesn't
> support it.

And you can't determine that based on different device IDs? If you can 
use vendor/device ID, then you should. If not, then this is fine.

Is it possible to have no EEPROM at all? If so, you might want to just 
put the raw EEPROM data into DT rather than a property for every field 
(I'm assuming there's more than just what you have properties for now).

Rob
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v6 1/3] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Fri, Sep 16, 2016 at 2:45 PM, Rob Herring  wrote:
> On Fri, Sep 09, 2016 at 10:57:06PM +0200, Martin Blumenstingl wrote:
>> On Fri, Sep 9, 2016 at 9:48 AM, Oleksij Rempel  
>> wrote:
>> >> +Optional properties:
>> >> +- reg: Address and length of the register set for the device.
>> >> +- qca,clk-25mhz: Defines that a 25MHz clock is used
>> >
>> > Some SoCs even Atheros WiSoCs use WiFi clock for System Clock. In this
>> > case we need to use clock framework any way, so why not in this case too?
>> > Provide dummy static clock in DT and connect it with this node?
>> >
>> > osc25m: oscillator {
>> > compatible = "fixed-clock";
>> > #clock-cells = <0>;
>> > clock-frequency = <2500>;
>> > clock-accuracy = <3>;
>> > };
>> >
>> > acc: clock-controller@8004 {
>> > compatible = "some-clock-controller";
>> > #clock-cells = <1>;
>> > clocks = <>;
>> > reg = <0x8004 0x204>;
>> > };
>> >
>> >
>> >  {
>> > ath9k@168c,002d {
>> > compatible = "pci168c,002d";
>> > reg = <0x7000 0 0 0 0x1000>;
>> > clocks = <>;
>> > qca,disable-5ghz;
>> > };
>> > };
>> >
>> >
>> > driver will need to use clk_get and compare frequency instead of
>> > of_property_read_bool(np, "qca,clk-25mhz"). In this case you will need
>> > to define source clock only one time and reuse it by all affected DT
>> > nodes. If we have 40MHz clock you will only need to change it in
>> > fixed-clock.
>> that does sound like a good idea, thanks for that input!
>> However, I will remove the property for the first version because I am
>> trying to get PCI(e) devices supported. OF support for AHB (WiSoC)
>> needs more work (in other places, like ahb.c) anyways.
>> But this suggestion should be remembered!
>>
>> >> +- 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).
>> >> +- qca,disable-2ghz: Overrides the settings from the EEPROM and disables 
>> >> the
>> >> + 2.4GHz band. Setting this property is only needed
>> >> + when the RF circuit does not support the 2.4GHz band
>> >> + while it is enabled nevertheless in the EEPROM.
>> >> +- qca,disable-5ghz: Overrides the settings from the EEPROM and disables 
>> >> the
>> >> + 5GHz band. Setting this property is only needed when
>> >> + the RF circuit does not support the 5GHz band while
>> >> + it is enabled nevertheless in the EEPROM.
>> >
>> > This option can be reused for every WiFi controller. Not only for qca.
>> > So may be instead of adding vendor specific name, make it reusable for
>> > all vendors. Instead of qca,disable-5ghz and qca,disable-2ghz make
>> > disable-5ghz and disable-2ghz?
>
> Fine, but if you do this then document in a common location.
what about Documentation/devicetree/bindings/net/wireless/ieee80211-common.txt?

>> I am personally fine with anything that fits best into the grand
>> scheme of things.
>> There are three scenarios I can think of which may be influenced by
>> devicetree configuration:
>> a) let the driver decide automatically whether the 2.4GHz and/or 5GHz
>> band is/are enabled
>> b) explicitly disable either bands (because the driver thinks due to
>> whatever reason that a band is supported while in reality it isn't)
>> c) explicitly enable a band (for example because the driver cannot
>> automagically detect which band should be enabled)
>>
>> for ath9k we default to a) but also allow b): the EEPROM (device
>> specific calibration data) contains information about which bands are
>> enabled (or not). But some manufacturers are shipping devices for
>> example with the 5G band enabled, while the actual hardware doesn't
>> support it.
>
> And you can't determine that based on different device IDs? If you can
> use vendor/device ID, then you should. If not, then this is fine.
one example is the TP-Link TW-8970 which uses a AR9381. ath9k
identifies this chip as AR9380 (probably because both are *very*
similar). The former does not support the 5G band, the latter does
(but unfortunately - even though it's not supported - the EEPROM data
on the TW-8970 indicates that 5G is supported)

> Is it possible to have no EEPROM at all? If so, you might want to just
> put the raw EEPROM data into DT rather than a property for every field
> (I'm assuming there's more than just what you have properties for now).
In theory: yes
However, most devices we are talking about are legacy ones without
devicetree support in the bootloader (in other 

Re: [ath9k-devel] [PATCH v6 1/3] Documentation: dt: net: add ath9k wireless device binding

[Oleksij Rempel]

Am 09.09.2016 um 22:57 schrieb Martin Blumenstingl:
> On Fri, Sep 9, 2016 at 9:48 AM, Oleksij Rempel  wrote:
>>> +Optional properties:
>>> +- reg: Address and length of the register set for the device.
>>> +- qca,clk-25mhz: Defines that a 25MHz clock is used
>>
>> Some SoCs even Atheros WiSoCs use WiFi clock for System Clock. In this
>> case we need to use clock framework any way, so why not in this case too?
>> Provide dummy static clock in DT and connect it with this node?
>>
>> osc25m: oscillator {
>> compatible = "fixed-clock";
>> #clock-cells = <0>;
>> clock-frequency = <2500>;
>> clock-accuracy = <3>;
>> };
>>
>> acc: clock-controller@8004 {
>> compatible = "some-clock-controller";
>> #clock-cells = <1>;
>> clocks = <>;
>> reg = <0x8004 0x204>;
>> };
>>
>>
>>  {
>> ath9k@168c,002d {
>> compatible = "pci168c,002d";
>> reg = <0x7000 0 0 0 0x1000>;
>> clocks = <>;
>> qca,disable-5ghz;
>> };
>> };
>>
>>
>> driver will need to use clk_get and compare frequency instead of
>> of_property_read_bool(np, "qca,clk-25mhz"). In this case you will need
>> to define source clock only one time and reuse it by all affected DT
>> nodes. If we have 40MHz clock you will only need to change it in
>> fixed-clock.
> that does sound like a good idea, thanks for that input!
> However, I will remove the property for the first version because I am
> trying to get PCI(e) devices supported. OF support for AHB (WiSoC)
> needs more work (in other places, like ahb.c) anyways.
> But this suggestion should be remembered!
> 
>>> +- 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).
>>> +- qca,disable-2ghz: Overrides the settings from the EEPROM and disables the
>>> + 2.4GHz band. Setting this property is only needed
>>> + when the RF circuit does not support the 2.4GHz band
>>> + while it is enabled nevertheless in the EEPROM.
>>> +- qca,disable-5ghz: Overrides the settings from the EEPROM and disables the
>>> + 5GHz band. Setting this property is only needed when
>>> + the RF circuit does not support the 5GHz band while
>>> + it is enabled nevertheless in the EEPROM.
>>
>> This option can be reused for every WiFi controller. Not only for qca.
>> So may be instead of adding vendor specific name, make it reusable for
>> all vendors. Instead of qca,disable-5ghz and qca,disable-2ghz make
>> disable-5ghz and disable-2ghz?
> I am personally fine with anything that fits best into the grand
> scheme of things.
> There are three scenarios I can think of which may be influenced by
> devicetree configuration:
> a) let the driver decide automatically whether the 2.4GHz and/or 5GHz
> band is/are enabled
> b) explicitly disable either bands (because the driver thinks due to
> whatever reason that a band is supported while in reality it isn't)
> c) explicitly enable a band (for example because the driver cannot
> automagically detect which band should be enabled)
> 
> for ath9k we default to a) but also allow b): the EEPROM (device
> specific calibration data) contains information about which bands are
> enabled (or not). But some manufacturers are shipping devices for
> example with the 5G band enabled, while the actual hardware doesn't
> support it.
> 
> Felix' mt76 driver for example defaults to case a) but allows
> overriding (= forcefully enabling or disabling) a specific band.
> 
> If we decide how this should look like in the devicetree then I can go
> ahead and implement it accordingly.

To avoid possible conflict with other device-tree bindings i would
suggest this kind of naming:
ieee80211-5ghz-enalbe
ieee80211-5ghz-disable
ieee80211-2.4ghz-enalbe
ieee80211-2.4ghz-disable

i assume at some point we would get even more eeprom overrides. For
example disable/enable some modulation and so on.

we would need 80211 specific functions to read this overrides from
device-tree and/or acpi.

any feedback from DT maintainers?
-- 
Regards,
Oleksij



signature.asc
Description: OpenPGP digital signature
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v6 1/3] Documentation: dt: net: add ath9k wireless device binding

[Martin Blumenstingl]

On Fri, Sep 9, 2016 at 9:48 AM, Oleksij Rempel  wrote:
>> +Optional properties:
>> +- reg: Address and length of the register set for the device.
>> +- qca,clk-25mhz: Defines that a 25MHz clock is used
>
> Some SoCs even Atheros WiSoCs use WiFi clock for System Clock. In this
> case we need to use clock framework any way, so why not in this case too?
> Provide dummy static clock in DT and connect it with this node?
>
> osc25m: oscillator {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <2500>;
> clock-accuracy = <3>;
> };
>
> acc: clock-controller@8004 {
> compatible = "some-clock-controller";
> #clock-cells = <1>;
> clocks = <>;
> reg = <0x8004 0x204>;
> };
>
>
>  {
> ath9k@168c,002d {
> compatible = "pci168c,002d";
> reg = <0x7000 0 0 0 0x1000>;
> clocks = <>;
> qca,disable-5ghz;
> };
> };
>
>
> driver will need to use clk_get and compare frequency instead of
> of_property_read_bool(np, "qca,clk-25mhz"). In this case you will need
> to define source clock only one time and reuse it by all affected DT
> nodes. If we have 40MHz clock you will only need to change it in
> fixed-clock.
that does sound like a good idea, thanks for that input!
However, I will remove the property for the first version because I am
trying to get PCI(e) devices supported. OF support for AHB (WiSoC)
needs more work (in other places, like ahb.c) anyways.
But this suggestion should be remembered!

>> +- 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).
>> +- qca,disable-2ghz: Overrides the settings from the EEPROM and disables the
>> + 2.4GHz band. Setting this property is only needed
>> + when the RF circuit does not support the 2.4GHz band
>> + while it is enabled nevertheless in the EEPROM.
>> +- qca,disable-5ghz: Overrides the settings from the EEPROM and disables the
>> + 5GHz band. Setting this property is only needed when
>> + the RF circuit does not support the 5GHz band while
>> + it is enabled nevertheless in the EEPROM.
>
> This option can be reused for every WiFi controller. Not only for qca.
> So may be instead of adding vendor specific name, make it reusable for
> all vendors. Instead of qca,disable-5ghz and qca,disable-2ghz make
> disable-5ghz and disable-2ghz?
I am personally fine with anything that fits best into the grand
scheme of things.
There are three scenarios I can think of which may be influenced by
devicetree configuration:
a) let the driver decide automatically whether the 2.4GHz and/or 5GHz
band is/are enabled
b) explicitly disable either bands (because the driver thinks due to
whatever reason that a band is supported while in reality it isn't)
c) explicitly enable a band (for example because the driver cannot
automagically detect which band should be enabled)

for ath9k we default to a) but also allow b): the EEPROM (device
specific calibration data) contains information about which bands are
enabled (or not). But some manufacturers are shipping devices for
example with the 5G band enabled, while the actual hardware doesn't
support it.

Felix' mt76 driver for example defaults to case a) but allows
overriding (= forcefully enabling or disabling) a specific band.

If we decide how this should look like in the devicetree then I can go
ahead and implement it accordingly.


Regards,
Martin


[0] https://github.com/openwrt/mt76/blob/master/eeprom.c#L79
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v6 1/3] Documentation: dt: net: add ath9k wireless device binding

[Oleksij Rempel]

Hallo all,

if it is not too late i would add my two cents :)


Am 06.09.2016 um 23:46 schrieb Martin Blumenstingl:
> Add documentation how devicetree can be used to configure ath9k based
> devices.
> 
> Signed-off-by: Martin Blumenstingl 
> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 39 
> ++
>  1 file changed, 39 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 000..77b9202
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
> @@ -0,0 +1,39 @@
> +* 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: For PCI and PCIe devices this should be an identifier following
> + the format as defined in "PCI Bus Binding to Open Firmware"
> + Revision 2.1. One of the possible formats is "pci,"
> + where  is the PCI vendor ID and  is PCI device ID.
> +
> +Optional properties:
> +- reg: Address and length of the register set for the device.
> +- qca,clk-25mhz: Defines that a 25MHz clock is used

Some SoCs even Atheros WiSoCs use WiFi clock for System Clock. In this
case we need to use clock framework any way, so why not in this case too?
Provide dummy static clock in DT and connect it with this node?

osc25m: oscillator {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <2500>;
clock-accuracy = <3>;
};

acc: clock-controller@8004 {
compatible = "some-clock-controller";
#clock-cells = <1>;
clocks = <>;
reg = <0x8004 0x204>;
};


 {
ath9k@168c,002d {
compatible = "pci168c,002d";
reg = <0x7000 0 0 0 0x1000>;
clocks = <>;
qca,disable-5ghz;
};
};


driver will need to use clk_get and compare frequency instead of
of_property_read_bool(np, "qca,clk-25mhz"). In this case you will need
to define source clock only one time and reuse it by all affected DT
nodes. If we have 40MHz clock you will only need to change it in
fixed-clock.


> +- 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).
> +- qca,disable-2ghz: Overrides the settings from the EEPROM and disables the
> + 2.4GHz band. Setting this property is only needed
> + when the RF circuit does not support the 2.4GHz band
> + while it is enabled nevertheless in the EEPROM.
> +- qca,disable-5ghz: Overrides the settings from the EEPROM and disables the
> + 5GHz band. Setting this property is only needed when
> + the RF circuit does not support the 5GHz band while
> + it is enabled nevertheless in the EEPROM.

This option can be reused for every WiFi controller. Not only for qca.
So may be instead of adding vendor specific name, make it reusable for
all vendors. Instead of qca,disable-5ghz and qca,disable-2ghz make
disable-5ghz and disable-2ghz?


> +- 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:
> + {
> + ath9k@168c,002d {
> + compatible = "pci168c,002d";
> + reg = <0x7000 0 0 0 0x1000>;
> + qca,disable-5ghz;
> + };
> +};
> 


-- 
Regards,
Oleksij



signature.asc
Description: OpenPGP digital signature
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel