Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding

Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Kalle Valo]

Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org> writes:

> Signed-off-by: Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org>
> ---
>  .../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 0000000..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 = <0x0000 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

Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Felix Fietkau]

On 2016-09-08 12:54, Kalle Valo wrote:
> Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org> writes:
> 
>> Signed-off-by: Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org>
>> ---
>>  .../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 0000000..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 = <0x0000 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

--
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 v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Arnd Bergmann]

On Thursday 29 September 2016, Felix Fietkau wrote:
> On 2016-09-08 12:54, Kalle Valo wrote:
> > Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org> writes:
> > 
> >> Signed-off-by: Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org>
> >> ---
> >>  .../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 0000000..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 = <0x0000 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

[Felix Fietkau]

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 <nbd-Vt+b4OUoWG0@public.gmane.org> writes:
>> > 
>> >> Signed-off-by: Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org>
>> >> ---
>> >>  .../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 0000000..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 = <0x0000 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
--
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 v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Arnd Bergmann]

On Friday 30 September 2016, Felix Fietkau wrote:
> >> >> + pcie0 {
> >> >> +         mt76@0,0 {
> >> >> +                 reg = <0x0000 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

[Felix Fietkau]

On 2016-09-30 16:36, Arnd Bergmann wrote:
> On Friday 30 September 2016, Felix Fietkau wrote:
>> >> >> + pcie0 {
>> >> >> +         mt76@0,0 {
>> >> >> +                 reg = <0x0000 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

[Arnd Bergmann]

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
--
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 v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Kalle Valo]

Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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
--
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 v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Rafał Miłecki]

On 3 October 2016 at 15:29, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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ł
--
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 v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Martin Blumenstingl]

On Wed, Dec 28, 2016 at 11:08 AM, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 3 October 2016 at 15:29, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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
--
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 v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Rafał Miłecki]

On 28 December 2016 at 11:43, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> On Wed, Dec 28, 2016 at 11:08 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 3 October 2016 at 15:29, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>> Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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.
--
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 v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Rafał Miłecki]

On 28 December 2016 at 14:28, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 28 December 2016 at 11:43, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> 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ł