Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On 28 December 2016 at 14:28, Rafał Miłecki wrote: > On 28 December 2016 at 11:43, Martin Blumenstingl > wrote: >> should we describe the center freq in Hz or MHz (cfg80211's >> ieee80211_channel uses the latter)? > > Is there any case that may require HZ accuracy? I was thinking about using > MHz. Regulatory code uses KHz, so we may better do the same. -- Rafał
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On 28 December 2016 at 11:43, Martin Blumenstingl wrote: > On Wed, Dec 28, 2016 at 11:08 AM, Rafał Miłecki wrote: >> On 3 October 2016 at 15:29, Kalle Valo wrote: >>> Arnd Bergmann writes: >>> On Friday 30 September 2016, Felix Fietkau wrote: > >> >> >> + device_type = "pci"; > >> >> >> + mediatek,mtd-eeprom = <&factory 0x8000>; > >> >> >> + mediatek,2ghz = <0>; > >> > > >> > It's not clear what the possible values for the 2ghz property are, > >> > can you be more verbose in the description? How is <0> different > >> > from no property? > >> 0 means disabled, no property means unchanged (compared to EEPROM). > > > > Maybe have a boolean property instead then to say > > "mediatek,2ghz-disabled" ? > > > > If zero is the only possible value, there is no need to put a number in > > there. > 1 is also possible, which will force-enable the capability. Ok, then both those values should be documented in the binding. >>> >>> Related to this, Martin sent patches which add generic bindings for >>> enabling 2 Ghz and 5 Ghz bands. >>> >>> [RFC,1/3] Documentation: dt-bindings: add IEEE 802.11 binding documentation >>> https://patchwork.kernel.org/patch/9359833/ >>> >>> [RFC,2/3] of: add IEEE 802.11 device configuration support code >>> https://patchwork.kernel.org/patch/9359837/ >> >> I would prefer something more generic. Many tri-band routers split 5 >> GHz band into 2 sets of channels and they have separated radios for >> them. >> >> E.g. Netgear R8000 has phy0 which should be used for higher part of 5 >> GHz band (channels 149+) and phy2 which should be used for lower part >> of 5 GHz band (channels from 36 to 48 or 64). >> >> What do you think about some more flexible properties like: >> ieee80211-min-center-freq >> ieee80211-max-center-freq > what would happen if only one of these properties was given or would > we forbid that (because the .dts should always describe the hardware, > and if we describe a lower bound then we should also describe the > upper bound)? I didn't think about requiring both properties to be set, but if this is more correct from DT point of view, we can always require that. Let's see if we get DT guys opinion. > the benefits of your solution are: > - this would allow *enabling* bands as well (my proposal allows this > as well, but the .dts is indeed a bit hard to read - unlike your > solution which looks nice to me) > - we could handle this within generic cfg80211/mac80211 code instead > of "duplicating" it per driver i'm happy to hear that :) > should we describe the center freq in Hz or MHz (cfg80211's > ieee80211_channel uses the latter)? Is there any case that may require HZ accuracy? I was thinking about using MHz.
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On Wed, Dec 28, 2016 at 11:08 AM, Rafał Miłecki wrote: > On 3 October 2016 at 15:29, Kalle Valo wrote: >> Arnd Bergmann writes: >> >>> On Friday 30 September 2016, Felix Fietkau wrote: >> >> >> + device_type = "pci"; >> >> >> + mediatek,mtd-eeprom = <&factory 0x8000>; >> >> >> + mediatek,2ghz = <0>; >> > >> > It's not clear what the possible values for the 2ghz property are, >> > can you be more verbose in the description? How is <0> different >> > from no property? >> 0 means disabled, no property means unchanged (compared to EEPROM). > > Maybe have a boolean property instead then to say > "mediatek,2ghz-disabled" ? > > If zero is the only possible value, there is no need to put a number in > there. 1 is also possible, which will force-enable the capability. >>> >>> Ok, then both those values should be documented in the binding. >> >> Related to this, Martin sent patches which add generic bindings for >> enabling 2 Ghz and 5 Ghz bands. >> >> [RFC,1/3] Documentation: dt-bindings: add IEEE 802.11 binding documentation >> https://patchwork.kernel.org/patch/9359833/ >> >> [RFC,2/3] of: add IEEE 802.11 device configuration support code >> https://patchwork.kernel.org/patch/9359837/ > > I would prefer something more generic. Many tri-band routers split 5 > GHz band into 2 sets of channels and they have separated radios for > them. > > E.g. Netgear R8000 has phy0 which should be used for higher part of 5 > GHz band (channels 149+) and phy2 which should be used for lower part > of 5 GHz band (channels from 36 to 48 or 64). > > What do you think about some more flexible properties like: > ieee80211-min-center-freq > ieee80211-max-center-freq what would happen if only one of these properties was given or would we forbid that (because the .dts should always describe the hardware, and if we describe a lower bound then we should also describe the upper bound)? the benefits of your solution are: - this would allow *enabling* bands as well (my proposal allows this as well, but the .dts is indeed a bit hard to read - unlike your solution which looks nice to me) - we could handle this within generic cfg80211/mac80211 code instead of "duplicating" it per driver should we describe the center freq in Hz or MHz (cfg80211's ieee80211_channel uses the latter)? @Arnd: what do you think from devicetree perspective? Regards, Martin [0] http://lxr.free-electrons.com/source/include/net/cfg80211.h#L130
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On 3 October 2016 at 15:29, Kalle Valo wrote: > Arnd Bergmann writes: > >> On Friday 30 September 2016, Felix Fietkau wrote: >>> >> >> >> + device_type = "pci"; >>> >> >> >> + mediatek,mtd-eeprom = <&factory 0x8000>; >>> >> >> >> + mediatek,2ghz = <0>; >>> >> > >>> >> > It's not clear what the possible values for the 2ghz property are, >>> >> > can you be more verbose in the description? How is <0> different >>> >> > from no property? >>> >> 0 means disabled, no property means unchanged (compared to EEPROM). >>> > >>> > Maybe have a boolean property instead then to say >>> > "mediatek,2ghz-disabled" ? >>> > >>> > If zero is the only possible value, there is no need to put a number in >>> > there. >>> 1 is also possible, which will force-enable the capability. >> >> Ok, then both those values should be documented in the binding. > > Related to this, Martin sent patches which add generic bindings for > enabling 2 Ghz and 5 Ghz bands. > > [RFC,1/3] Documentation: dt-bindings: add IEEE 802.11 binding documentation > https://patchwork.kernel.org/patch/9359833/ > > [RFC,2/3] of: add IEEE 802.11 device configuration support code > https://patchwork.kernel.org/patch/9359837/ I would prefer something more generic. Many tri-band routers split 5 GHz band into 2 sets of channels and they have separated radios for them. E.g. Netgear R8000 has phy0 which should be used for higher part of 5 GHz band (channels 149+) and phy2 which should be used for lower part of 5 GHz band (channels from 36 to 48 or 64). What do you think about some more flexible properties like: ieee80211-min-center-freq ieee80211-max-center-freq -- Rafał
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
Arnd Bergmann writes: > On Friday 30 September 2016, Felix Fietkau wrote: >> >> >> >> + device_type = "pci"; >> >> >> >> + mediatek,mtd-eeprom = <&factory 0x8000>; >> >> >> >> + mediatek,2ghz = <0>; >> >> > >> >> > It's not clear what the possible values for the 2ghz property are, >> >> > can you be more verbose in the description? How is <0> different >> >> > from no property? >> >> 0 means disabled, no property means unchanged (compared to EEPROM). >> > >> > Maybe have a boolean property instead then to say "mediatek,2ghz-disabled" >> > ? >> > >> > If zero is the only possible value, there is no need to put a number in >> > there. >> 1 is also possible, which will force-enable the capability. > > Ok, then both those values should be documented in the binding. Related to this, Martin sent patches which add generic bindings for enabling 2 Ghz and 5 Ghz bands. [RFC,1/3] Documentation: dt-bindings: add IEEE 802.11 binding documentation https://patchwork.kernel.org/patch/9359833/ [RFC,2/3] of: add IEEE 802.11 device configuration support code https://patchwork.kernel.org/patch/9359837/ -- Kalle Valo
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On Friday 30 September 2016, Felix Fietkau wrote: > >> >> >> + device_type = "pci"; > >> >> >> + mediatek,mtd-eeprom = <&factory 0x8000>; > >> >> >> + mediatek,2ghz = <0>; > >> > > >> > It's not clear what the possible values for the 2ghz property are, > >> > can you be more verbose in the description? How is <0> different > >> > from no property? > >> 0 means disabled, no property means unchanged (compared to EEPROM). > > > > Maybe have a boolean property instead then to say "mediatek,2ghz-disabled" ? > > > > If zero is the only possible value, there is no need to put a number in > > there. > 1 is also possible, which will force-enable the capability. Ok, then both those values should be documented in the binding. Arnd
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On 2016-09-30 16:36, Arnd Bergmann wrote:
> On Friday 30 September 2016, Felix Fietkau wrote:
>> >> >> + pcie0 {
>> >> >> + mt76@0,0 {
>> >> >> + reg = <0x 0 0 0 0>;
>> >
>> > Maybe have an examplep of a real register address other than zero?
>> This is a real example referring to the first device on a PCI bus.
>> I copy&pasted this from a .dts file that we use in LEDE.
>
> Ok, I see.
>
>> >> >> + device_type = "pci";
>> >> >> + mediatek,mtd-eeprom = <&factory 0x8000>;
>> >> >> + mediatek,2ghz = <0>;
>> >
>> > It's not clear what the possible values for the 2ghz property are,
>> > can you be more verbose in the description? How is <0> different
>> > from no property?
>> 0 means disabled, no property means unchanged (compared to EEPROM).
>
> Maybe have a boolean property instead then to say "mediatek,2ghz-disabled" ?
>
> If zero is the only possible value, there is no need to put a number in there.
1 is also possible, which will force-enable the capability.
- Felix
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On Friday 30 September 2016, Felix Fietkau wrote:
> >> >> + pcie0 {
> >> >> + mt76@0,0 {
> >> >> + reg = <0x 0 0 0 0>;
> >
> > Maybe have an examplep of a real register address other than zero?
> This is a real example referring to the first device on a PCI bus.
> I copy&pasted this from a .dts file that we use in LEDE.
Ok, I see.
> >> >> + device_type = "pci";
> >> >> + mediatek,mtd-eeprom = <&factory 0x8000>;
> >> >> + mediatek,2ghz = <0>;
> >
> > It's not clear what the possible values for the 2ghz property are,
> > can you be more verbose in the description? How is <0> different
> > from no property?
> 0 means disabled, no property means unchanged (compared to EEPROM).
Maybe have a boolean property instead then to say "mediatek,2ghz-disabled" ?
If zero is the only possible value, there is no need to put a number in there.
Arnd
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On 2016-09-30 00:41, Arnd Bergmann wrote:
> On Thursday 29 September 2016, Felix Fietkau wrote:
>> On 2016-09-08 12:54, Kalle Valo wrote:
>> > Felix Fietkau writes:
>> >
>> >> Signed-off-by: Felix Fietkau
>> >> ---
>> >> .../bindings/net/wireless/mediatek,mt76.txt| 26
>> >> ++
>> >> 1 file changed, 26 insertions(+)
>> >> create mode 100644
>> >> Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >>
>> >> diff --git
>> >> a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >> b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >> new file mode 100644
>> >> index 000..d51c35f
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >> @@ -0,0 +1,26 @@
>> >> +* MediaTek mt76xx devices
>> >> +
>> >> +This node provides properties for configuring the MediaTek mt76xx
>> >> wireless
>> >> +device. The node is expected to be specified as a child node of the PCI
>> >> +controller to which the wireless chip is connected.
>> >> +
>> >> +Optional properties:
>> >> +
>> >> +- mac-address: See ethernet.txt in the parent directory
>> >> +- local-mac-address: See ethernet.txt in the parent directory
>> >> +- mediatek,2ghz: Override the 2.4 GHz band capability from EEPROM
>> >> +- mediatek,5ghz: Override the 5 GHz band capability from EEPROM
>> >> +- mediatek,mtd-eeprom: Specify a MTD partition + offset containing
>> >> EEPROM data
>> >> +
>> >> +&pcie {
>> >> + status = "okay";
>> >> +
>> >> + pcie0 {
>> >> + mt76@0,0 {
>> >> + reg = <0x 0 0 0 0>;
>
> Maybe have an examplep of a real register address other than zero?
This is a real example referring to the first device on a PCI bus.
I copy&pasted this from a .dts file that we use in LEDE.
>> >> + device_type = "pci";
>> >> + mediatek,mtd-eeprom = <&factory 0x8000>;
>> >> + mediatek,2ghz = <0>;
>
> It's not clear what the possible values for the 2ghz property are,
> can you be more verbose in the description? How is <0> different
> from no property?
0 means disabled, no property means unchanged (compared to EEPROM).
- Felix
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On Thursday 29 September 2016, Felix Fietkau wrote:
> On 2016-09-08 12:54, Kalle Valo wrote:
> > Felix Fietkau writes:
> >
> >> Signed-off-by: Felix Fietkau
> >> ---
> >> .../bindings/net/wireless/mediatek,mt76.txt| 26
> >> ++
> >> 1 file changed, 26 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
> >> b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
> >> new file mode 100644
> >> index 000..d51c35f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
> >> @@ -0,0 +1,26 @@
> >> +* MediaTek mt76xx devices
> >> +
> >> +This node provides properties for configuring the MediaTek mt76xx wireless
> >> +device. The node is expected to be specified as a child node of the PCI
> >> +controller to which the wireless chip is connected.
> >> +
> >> +Optional properties:
> >> +
> >> +- mac-address: See ethernet.txt in the parent directory
> >> +- local-mac-address: See ethernet.txt in the parent directory
> >> +- mediatek,2ghz: Override the 2.4 GHz band capability from EEPROM
> >> +- mediatek,5ghz: Override the 5 GHz band capability from EEPROM
> >> +- mediatek,mtd-eeprom: Specify a MTD partition + offset containing EEPROM
> >> data
> >> +
> >> +&pcie {
> >> + status = "okay";
> >> +
> >> + pcie0 {
> >> + mt76@0,0 {
> >> + reg = <0x 0 0 0 0>;
Maybe have an examplep of a real register address other than zero?
> >> + device_type = "pci";
> >> + mediatek,mtd-eeprom = <&factory 0x8000>;
> >> + mediatek,2ghz = <0>;
It's not clear what the possible values for the 2ghz property are,
can you be more verbose in the description? How is <0> different
from no property?
Arnd
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
On 2016-09-08 12:54, Kalle Valo wrote:
> Felix Fietkau writes:
>
>> Signed-off-by: Felix Fietkau
>> ---
>> .../bindings/net/wireless/mediatek,mt76.txt| 26
>> ++
>> 1 file changed, 26 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> new file mode 100644
>> index 000..d51c35f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> @@ -0,0 +1,26 @@
>> +* MediaTek mt76xx devices
>> +
>> +This node provides properties for configuring the MediaTek mt76xx wireless
>> +device. The node is expected to be specified as a child node of the PCI
>> +controller to which the wireless chip is connected.
>> +
>> +Optional properties:
>> +
>> +- mac-address: See ethernet.txt in the parent directory
>> +- local-mac-address: See ethernet.txt in the parent directory
>> +- mediatek,2ghz: Override the 2.4 GHz band capability from EEPROM
>> +- mediatek,5ghz: Override the 5 GHz band capability from EEPROM
>> +- mediatek,mtd-eeprom: Specify a MTD partition + offset containing EEPROM
>> data
>> +
>> +&pcie {
>> +status = "okay";
>> +
>> +pcie0 {
>> +mt76@0,0 {
>> +reg = <0x 0 0 0 0>;
>> +device_type = "pci";
>> +mediatek,mtd-eeprom = <&factory 0x8000>;
>> +mediatek,2ghz = <0>;
>> +};
>> +};
>> +};
>
> I need an ack from device tree maintainers, CCing the devicetree list.
> Do we need to resend or this ok?
>
> Patchwork link:
>
> https://patchwork.kernel.org/patch/9313309/
Ping?
- Felix
Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
Felix Fietkau writes:
> Signed-off-by: Felix Fietkau
> ---
> .../bindings/net/wireless/mediatek,mt76.txt| 26
> ++
> 1 file changed, 26 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
> b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
> new file mode 100644
> index 000..d51c35f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
> @@ -0,0 +1,26 @@
> +* MediaTek mt76xx devices
> +
> +This node provides properties for configuring the MediaTek mt76xx wireless
> +device. The node is expected to be specified as a child node of the PCI
> +controller to which the wireless chip is connected.
> +
> +Optional properties:
> +
> +- mac-address: See ethernet.txt in the parent directory
> +- local-mac-address: See ethernet.txt in the parent directory
> +- mediatek,2ghz: Override the 2.4 GHz band capability from EEPROM
> +- mediatek,5ghz: Override the 5 GHz band capability from EEPROM
> +- mediatek,mtd-eeprom: Specify a MTD partition + offset containing EEPROM
> data
> +
> +&pcie {
> + status = "okay";
> +
> + pcie0 {
> + mt76@0,0 {
> + reg = <0x 0 0 0 0>;
> + device_type = "pci";
> + mediatek,mtd-eeprom = <&factory 0x8000>;
> + mediatek,2ghz = <0>;
> + };
> + };
> +};
I need an ack from device tree maintainers, CCing the devicetree list.
Do we need to resend or this ok?
Patchwork link:
https://patchwork.kernel.org/patch/9313309/
--
Kalle Valo
[PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
Signed-off-by: Felix Fietkau
---
.../bindings/net/wireless/mediatek,mt76.txt| 26 ++
1 file changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
diff --git a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
new file mode 100644
index 000..d51c35f
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
@@ -0,0 +1,26 @@
+* MediaTek mt76xx devices
+
+This node provides properties for configuring the MediaTek mt76xx wireless
+device. The node is expected to be specified as a child node of the PCI
+controller to which the wireless chip is connected.
+
+Optional properties:
+
+- mac-address: See ethernet.txt in the parent directory
+- local-mac-address: See ethernet.txt in the parent directory
+- mediatek,2ghz: Override the 2.4 GHz band capability from EEPROM
+- mediatek,5ghz: Override the 5 GHz band capability from EEPROM
+- mediatek,mtd-eeprom: Specify a MTD partition + offset containing EEPROM data
+
+&pcie {
+ status = "okay";
+
+ pcie0 {
+ mt76@0,0 {
+ reg = <0x 0 0 0 0>;
+ device_type = "pci";
+ mediatek,mtd-eeprom = <&factory 0x8000>;
+ mediatek,2ghz = <0>;
+ };
+ };
+};
--
2.8.4
