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

[Rob Herring]

On Sun, Oct 16, 2016 at 10:59:05PM +0200, Martin Blumenstingl wrote:
> Add documentation how devicetree can be used to configure ath9k based
> devices.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumensti...@googlemail.com>
> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.txt | 48 
> ++
>  1 file changed, 48 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt

Acked-by: Rob Herring <r...@kernel.org>
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

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

[Rob Herring]

On Wed, Oct 5, 2016 at 1:36 PM, Felix Fietkau <n...@nbd.name> wrote:
> On 2016-10-05 20:25, Martin Blumenstingl wrote:
>> On Mon, Oct 3, 2016 at 5:22 PM, Rob Herring <robh...@kernel.org> wrote:
>>> On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl
>>> <martin.blumensti...@googlemail.com> wrote:
>>>> There are at least two drivers (ath9k and mt76) out there, where
>>>> devicetree authors need to override the enabled bands.
>>>>
>>>> For ath9k there is only one use-case: disabling a band which has been
>>>> incorrectly enabled by the vendor in the EEPROM (enabling a band is not
>>>> possible because the calibration data would be missing in the EEPROM).
>>>> The mt76 driver (currently pending for review) however allows enabling
>>>> and disabling the 2.4GHz and 5GHz band, see [0].
>>>>
>>>> Based on the discussion of (earlier versions of) my ath9k devicetree
>>>> patch it was suggested [1] that we use enable- and disable- properties.
>>>> The current patch implements:
>>>> - bands can be enabled or disabled with the corresponding property
>>>> - if both (disable and enable) properties are given and a driver asks
>>>>   whether the band is enabled then the logic will return false (= not
>>>>   enabled, preferring the disabled flag)
>>>> - if both (disable and enable) properties are given and a driver asks
>>>>   whether the band is disabled then the logic will return true (again,
>>>>   preferring the disabled flag over the enabled flag)
>>>>
>>>> We can still change the logic to do what the mt76 driver does (I am
>>>> fine with both solutions):
>>>> - property not available: driver decides which bands to enable
>>>> - property set to 0: disable the band
>>>> - property set to 1: enable the band
>>>
>>> I prefer this way. Really, I'd prefer just a boolean disable property.
>>> I'm not sure why you need the enable. We usually have these tri-state
>>> properties when you want not present to mean use the bootloader or
>>> default setting. Try to make not present the most common case.
>> Felix: could you please give a few details why mt76 can not only
>> disable but also enable a specific band?
>> There is no specific comment in the sources [0] why this is needed.
>> All other drivers that I am aware of (ath9k, rt2x00) only allow
>> disabling a specific band, they never enable it (this has to be done
>> explicitly by the EEPROM).
> None of the devices use it at the moment, I probably added it when I
> played with a device that had broken EEPROM data. I guess I decided to
> do it this way because on many devices the band capability field simply
> cannot be trusted.
> I guess I would be okay with just having a disable property and adding
> an enable property later only if we end up actually needing it.

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

The other approach I've mentioned before is just put raw EEPROM data
into DT to override the EEPROM. This would be better if there's a
large number of settings you need.

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

[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