[PATCH] rt2800: remove erroneous duplicate condition

[PATCH] rt2800: remove erroneous duplicate condition

[Daniel Golle]

On 2019-10-28 06:07, wbob wrote:
> Hello Roman,
>
> while reading around drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> I stumbled on what I think is an edit of yours made in error in march
> 2017:
>
> https://github.com/torvalds/linux/commit/41977e86#diff-dae5dc10da180f3b055809a48118e18aR5281
>
> RT6352 in line 5281 should not have been introduced as the "else if"
> below line 5291 can then not take effect for a RT6352 device. Another
> possibility is for line 5291 to be not for RT6352, but this seems
> very unlikely. Are you able to clarify still after this substantial time?
>
> 5277: static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> ...
> 5279:  } else if (rt2x00_rt(rt2x00dev, RT5390) ||
> 5280:         rt2x00_rt(rt2x00dev, RT5392) ||
> 5281:         rt2x00_rt(rt2x00dev, RT6352)) {
> ...
> 5291:  } else if (rt2x00_rt(rt2x00dev, RT6352)) {
> ...

Hence remove erroneous line 5281 to make the driver actually
execute the correct initialization routine for MT7620 chips.

Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
Reported-by: wbob <wbob@jify.de>
Reported-by: Roman Yeryomin <roman@advem.lv>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index f1cdcd61c54a..c85456c8c193 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -5839,8 +5839,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
 		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_0, 0x8000fc21);
 		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_3, 0x00009c40);
 	} else if (rt2x00_rt(rt2x00dev, RT5390) ||
-		   rt2x00_rt(rt2x00dev, RT5392) ||
-		   rt2x00_rt(rt2x00dev, RT6352)) {
+		   rt2x00_rt(rt2x00dev, RT5392)) {
 		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
-- 
2.23.0

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Stanislaw Gruszka]

On Mon, Oct 28, 2019 at 10:22:44PM +0100, Daniel Golle wrote:
> On 2019-10-28 06:07, wbob wrote:
> > Hello Roman,
> >
> > while reading around drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > I stumbled on what I think is an edit of yours made in error in march
> > 2017:
> >
> > https://github.com/torvalds/linux/commit/41977e86#diff-dae5dc10da180f3b055809a48118e18aR5281
> >
> > RT6352 in line 5281 should not have been introduced as the "else if"
> > below line 5291 can then not take effect for a RT6352 device. Another
> > possibility is for line 5291 to be not for RT6352, but this seems
> > very unlikely. Are you able to clarify still after this substantial time?
> >
> > 5277: static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> > ...
> > 5279:  } else if (rt2x00_rt(rt2x00dev, RT5390) ||
> > 5280:         rt2x00_rt(rt2x00dev, RT5392) ||
> > 5281:         rt2x00_rt(rt2x00dev, RT6352)) {
> > ...
> > 5291:  } else if (rt2x00_rt(rt2x00dev, RT6352)) {
> > ...
> 
> Hence remove erroneous line 5281 to make the driver actually
> execute the correct initialization routine for MT7620 chips.
> 
> Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
> Reported-by: wbob <wbob@jify.de>
> Reported-by: Roman Yeryomin <roman@advem.lv>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> index f1cdcd61c54a..c85456c8c193 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> @@ -5839,8 +5839,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
>  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_0, 0x8000fc21);
>  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_3, 0x00009c40);
>  	} else if (rt2x00_rt(rt2x00dev, RT5390) ||
> -		   rt2x00_rt(rt2x00dev, RT5392) ||
> -		   rt2x00_rt(rt2x00dev, RT6352)) {
> +		   rt2x00_rt(rt2x00dev, RT5392)) {
>  		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
>  		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
>  		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);

I'm not sure if initialization on different path, is proper for all
variants of RT6352 chipset. Particularly I noticed that configuring
MIMO_PS_CFG can cause problems on wt3020.

Stanislaw

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Daniel Golle]

Hi Stanislaw,

On Tue, Oct 29, 2019 at 10:18:57AM +0100, Stanislaw Gruszka wrote:
> On Mon, Oct 28, 2019 at 10:22:44PM +0100, Daniel Golle wrote:
> > On 2019-10-28 06:07, wbob wrote:
> > > Hello Roman,
> > >
> > > while reading around drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > > I stumbled on what I think is an edit of yours made in error in march
> > > 2017:
> > >
> > > https://github.com/torvalds/linux/commit/41977e86#diff-dae5dc10da180f3b055809a48118e18aR5281
> > >
> > > RT6352 in line 5281 should not have been introduced as the "else if"
> > > below line 5291 can then not take effect for a RT6352 device. Another
> > > possibility is for line 5291 to be not for RT6352, but this seems
> > > very unlikely. Are you able to clarify still after this substantial time?
> > >
> > > 5277: static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> > > ...
> > > 5279:  } else if (rt2x00_rt(rt2x00dev, RT5390) ||
> > > 5280:         rt2x00_rt(rt2x00dev, RT5392) ||
> > > 5281:         rt2x00_rt(rt2x00dev, RT6352)) {
> > > ...
> > > 5291:  } else if (rt2x00_rt(rt2x00dev, RT6352)) {
> > > ...
> > 
> > Hence remove erroneous line 5281 to make the driver actually
> > execute the correct initialization routine for MT7620 chips.
> > 
> > Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
> > Reported-by: wbob <wbob@jify.de>
> > Reported-by: Roman Yeryomin <roman@advem.lv>
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > index f1cdcd61c54a..c85456c8c193 100644
> > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > @@ -5839,8 +5839,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> >  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_0, 0x8000fc21);
> >  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_3, 0x00009c40);
> >  	} else if (rt2x00_rt(rt2x00dev, RT5390) ||
> > -		   rt2x00_rt(rt2x00dev, RT5392) ||
> > -		   rt2x00_rt(rt2x00dev, RT6352)) {
> > +		   rt2x00_rt(rt2x00dev, RT5392)) {
> >  		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
> >  		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
> >  		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
> 
> I'm not sure if initialization on different path, is proper for all
> variants of RT6352 chipset. Particularly I noticed that configuring
> MIMO_PS_CFG can cause problems on wt3020.

That's pretty odd, as this register is also written unconditionally
by the vendor driver, see:
https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L529
https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L696

As only ChipVer >= 2 has been seen in the wild apparently, it seems
Roman implemented support for MT7620 along that codepath in the
original driver:
https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L713

However, now looking at this more, also
rt2800_register_write(rt2x00dev, TX_ALC_VGA3, 0x00000000);
doesn't match that codepath in the vendor driver which sets 0x06060606.

Now we could really implement all the codepaths for all pkg, ver, eco
variants of MT7620 using the accessors like I patched here:
https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/patches-4.14/300-mt7620-export-chip-version-and-pkg.patch
(accessor for mt7620_get_eco was already in place as it is used also
by MMC/SD driver afair)

Which MT7620 chip package, version and eco is found inside the wt3020?
(printed early on dmesg)


Cheers


Daniel

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Stanislaw Gruszka]

On Tue, Oct 29, 2019 at 11:05:03AM +0100, Daniel Golle wrote:
> > > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > > @@ -5839,8 +5839,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> > >  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_0, 0x8000fc21);
> > >  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_3, 0x00009c40);
> > >  	} else if (rt2x00_rt(rt2x00dev, RT5390) ||
> > > -		   rt2x00_rt(rt2x00dev, RT5392) ||
> > > -		   rt2x00_rt(rt2x00dev, RT6352)) {
> > > +		   rt2x00_rt(rt2x00dev, RT5392)) {
> > >  		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
> > >  		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
> > >  		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
> > 
> > I'm not sure if initialization on different path, is proper for all
> > variants of RT6352 chipset. Particularly I noticed that configuring
> > MIMO_PS_CFG can cause problems on wt3020.
> 
> That's pretty odd, as this register is also written unconditionally
> by the vendor driver, see:
> https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L529
> https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L696

Today I had time to debug this a bit more. Problems on WT3020 are not
caused by MIMO_PS_CFG, but by TX_PIN_CFG setting. On this device we
should not overwrite TX_PIN_CFG. Presumably this is correct for
other devices, since code path that set TX_PIN_CFG to 0x00150F0F
was not used before due to this erroneous 'else if RT6352'.

Even if setting MMIO_PS_CFG does not cause problems, I think we
do not need to configure it and can stay with default HW value,
which is 4.

Please repost patch with TX_PIN_CFG and MIMO_PS_CFG settings removed.

> As only ChipVer >= 2 has been seen in the wild apparently, it seems

Ok, so we do not need to implement ChipVer 1 support.

> Roman implemented support for MT7620 along that codepath in the
> original driver:
> https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L713
> 
> However, now looking at this more, also
> rt2800_register_write(rt2x00dev, TX_ALC_VGA3, 0x00000000);
> doesn't match that codepath in the vendor driver which sets 0x06060606.

This was changed by:

commit c2e28ef7711ffcb083474ee5f154264c6ec1ec07
Author: Tomislav Požega <pozega.tomislav@gmail.com>
Date:   Thu Dec 27 15:05:25 2018 +0100

    rt2x00: reduce tx power to nominal level on RT6352

and I think it is correct.

> Now we could really implement all the codepaths for all pkg, ver, eco
> variants of MT7620 using the accessors like I patched here:
> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/patches-4.14/300-mt7620-export-chip-version-and-pkg.patch
> (accessor for mt7620_get_eco was already in place as it is used also
> by MMC/SD driver afair)
>
> Which MT7620 chip package, version and eco is found inside the wt3020?
> (printed early on dmesg)

chipver 2 eco 6 pkg 0,

Thanks
Stanislaw

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Daniel Golle]

Hi Stanislaw,

On Sat, Nov 02, 2019 at 04:46:40PM +0100, Stanislaw Gruszka wrote:
> On Tue, Oct 29, 2019 at 11:05:03AM +0100, Daniel Golle wrote:
> > > > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > > > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > > > @@ -5839,8 +5839,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> > > >  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_0, 0x8000fc21);
> > > >  		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_3, 0x00009c40);
> > > >  	} else if (rt2x00_rt(rt2x00dev, RT5390) ||
> > > > -		   rt2x00_rt(rt2x00dev, RT5392) ||
> > > > -		   rt2x00_rt(rt2x00dev, RT6352)) {
> > > > +		   rt2x00_rt(rt2x00dev, RT5392)) {
> > > >  		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
> > > >  		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
> > > >  		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
> > > 
> > > I'm not sure if initialization on different path, is proper for all
> > > variants of RT6352 chipset. Particularly I noticed that configuring
> > > MIMO_PS_CFG can cause problems on wt3020.
> > 
> > That's pretty odd, as this register is also written unconditionally
> > by the vendor driver, see:
> > https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L529
> > https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L696
> 
> Today I had time to debug this a bit more. Problems on WT3020 are not
> caused by MIMO_PS_CFG, but by TX_PIN_CFG setting. On this device we
> should not overwrite TX_PIN_CFG. Presumably this is correct for
> other devices, since code path that set TX_PIN_CFG to 0x00150F0F
> was not used before due to this erroneous 'else if RT6352'.
> 
> Even if setting MMIO_PS_CFG does not cause problems, I think we
> do not need to configure it and can stay with default HW value,
> which is 4.

Ack. This seems to be a mistake in the vendor driver, my datasheet
also states that bit 1:2 have initial value of '2', which results
in a value of 4. Anyway it doesn't matter as long as MIMO_PS isn't
enabled (bit 3), so it's safe to remove it or set the correct default
value.

> 
> Please repost patch with TX_PIN_CFG and MIMO_PS_CFG settings removed.

TX_PIN_CFG is also set in rt2800_config_channel() as well as
rt2800_vco_calibration(), so no need to touch it during init.

> 
> > As only ChipVer >= 2 has been seen in the wild apparently, it seems
> 
> Ok, so we do not need to implement ChipVer 1 support.
> 
> > Roman implemented support for MT7620 along that codepath in the
> > original driver:
> > https://github.com/wuqiong/rt2860v2-for-openwrt-mt7620/blob/master/rt2860v2/chips/rt6352.c#L713
> > 
> > However, now looking at this more, also
> > rt2800_register_write(rt2x00dev, TX_ALC_VGA3, 0x00000000);
> > doesn't match that codepath in the vendor driver which sets 0x06060606.
> 
> This was changed by:
> 
> commit c2e28ef7711ffcb083474ee5f154264c6ec1ec07
> Author: Tomislav Požega <pozega.tomislav@gmail.com>
> Date:   Thu Dec 27 15:05:25 2018 +0100
> 
>     rt2x00: reduce tx power to nominal level on RT6352
> 
> and I think it is correct.

Ah, ok, that's a bit funny, because it means that this change actually
never made any difference, because the codepath wasn't executed.

I'm on my way to post v2.


Cheers


Daniel

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Stanislaw Gruszka]

On Sat, Nov 02, 2019 at 06:42:27PM +0100, Daniel Golle wrote:
> > This was changed by:
> > 
> > commit c2e28ef7711ffcb083474ee5f154264c6ec1ec07
> > Author: Tomislav Požega <pozega.tomislav@gmail.com>
> > Date:   Thu Dec 27 15:05:25 2018 +0100
> > 
> >     rt2x00: reduce tx power to nominal level on RT6352
> > 
> > and I think it is correct.
> 
> Ah, ok, that's a bit funny, because it means that this change actually
> never made any difference, because the codepath wasn't executed.

Yes, this was used/tested on patched rt2x00 driver that switch to this
different codepath. Now it will be used by default :-)

Stanislaw

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Tom Psyborg]

On 03/11/2019, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> On Sat, Nov 02, 2019 at 06:42:27PM +0100, Daniel Golle wrote:
>> > This was changed by:
>> >
>> > commit c2e28ef7711ffcb083474ee5f154264c6ec1ec07
>> > Author: Tomislav Požega <pozega.tomislav@gmail.com>
>> > Date:   Thu Dec 27 15:05:25 2018 +0100
>> >
>> >     rt2x00: reduce tx power to nominal level on RT6352
>> >
>> > and I think it is correct.
>>
>> Ah, ok, that's a bit funny, because it means that this change actually
>> never made any difference, because the codepath wasn't executed.
>
> Yes, this was used/tested on patched rt2x00 driver that switch to this
> different codepath. Now it will be used by default :-)
>
> Stanislaw
>
>

Hi

For your reference: rt2x00: reduce tx power to nominal level on RT6352

iPA/eLNA - fixes too high power output
ePA/eLNA - doesn't have any effect
iPA/iLNA - not tested

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Stanislaw Gruszka]

On Sun, Nov 03, 2019 at 04:41:11PM +0100, Tom Psyborg wrote:
> On 03/11/2019, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> > On Sat, Nov 02, 2019 at 06:42:27PM +0100, Daniel Golle wrote:
> >> > This was changed by:
> >> >
> >> > commit c2e28ef7711ffcb083474ee5f154264c6ec1ec07
> >> > Author: Tomislav Požega <pozega.tomislav@gmail.com>
> >> > Date:   Thu Dec 27 15:05:25 2018 +0100
> >> >
> >> >     rt2x00: reduce tx power to nominal level on RT6352
> >> >
> >> > and I think it is correct.
> >>
> >> Ah, ok, that's a bit funny, because it means that this change actually
> >> never made any difference, because the codepath wasn't executed.
> >
> > Yes, this was used/tested on patched rt2x00 driver that switch to this
> > different codepath. Now it will be used by default :-)
> >
> > Stanislaw
> >
> >
> 
> Hi
> 
> For your reference: rt2x00: reduce tx power to nominal level on RT6352
> 
> iPA/eLNA - fixes too high power output
> ePA/eLNA - doesn't have any effect
> iPA/iLNA - not tested

Does someone have iPA/iLNA device so this can be tested?
Or it is not used combination on available devices? 

Stanislaw

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Daniel Golle]

On Mon, Nov 04, 2019 at 09:48:23AM +0100, Stanislaw Gruszka wrote:
> On Sun, Nov 03, 2019 at 04:41:11PM +0100, Tom Psyborg wrote:
> > On 03/11/2019, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> > > On Sat, Nov 02, 2019 at 06:42:27PM +0100, Daniel Golle wrote:
> > >> > This was changed by:
> > >> >
> > >> > commit c2e28ef7711ffcb083474ee5f154264c6ec1ec07
> > >> > Author: Tomislav Požega <pozega.tomislav@gmail.com>
> > >> > Date:   Thu Dec 27 15:05:25 2018 +0100
> > >> >
> > >> >     rt2x00: reduce tx power to nominal level on RT6352
> > >> >
> > >> > and I think it is correct.
> > >>
> > >> Ah, ok, that's a bit funny, because it means that this change actually
> > >> never made any difference, because the codepath wasn't executed.
> > >
> > > Yes, this was used/tested on patched rt2x00 driver that switch to this
> > > different codepath. Now it will be used by default :-)
> > >
> > > Stanislaw
> > >
> > >
> > 
> > Hi
> > 
> > For your reference: rt2x00: reduce tx power to nominal level on RT6352
> > 
> > iPA/eLNA - fixes too high power output
> > ePA/eLNA - doesn't have any effect
> > iPA/iLNA - not tested
> 
> Does someone have iPA/iLNA device so this can be tested?
> Or it is not used combination on available devices? 

iPA/iLNA the most commonly found combination in cheap devices.
iPA/eLNA is more rare, but found in some higher-quality devices.
ePA/eLNA is available mostly in markets which allow higher TX power.
ePA/iLNA haven't seen it yet, but theoretically possible.

Looking at the internal photos of Nexx WT3020, I'm very certain this
is an iPA/iLNA device -- apart from regulators, magnetics, MT7620N
itself and flash memory, another magnetics and RAM on the backside,
there are no other parts on the board. Also afaik MT7620N only supports
iLNA/iPA (due to the limited number of pins of the DRQFN package).

See:
https://apps.fcc.gov/eas/GetApplicationAttachment.html?id=2241504


Cheers


Daniel


> 
> Stanislaw
> 

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Stanislaw Gruszka]

On Mon, Nov 04, 2019 at 10:00:58AM +0100, Daniel Golle wrote:
> On Mon, Nov 04, 2019 at 09:48:23AM +0100, Stanislaw Gruszka wrote:
> > On Sun, Nov 03, 2019 at 04:41:11PM +0100, Tom Psyborg wrote:
> > > On 03/11/2019, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> > > > On Sat, Nov 02, 2019 at 06:42:27PM +0100, Daniel Golle wrote:
> > > >> > This was changed by:
> > > >> >
> > > >> > commit c2e28ef7711ffcb083474ee5f154264c6ec1ec07
> > > >> > Author: Tomislav Požega <pozega.tomislav@gmail.com>
> > > >> > Date:   Thu Dec 27 15:05:25 2018 +0100
> > > >> >
> > > >> >     rt2x00: reduce tx power to nominal level on RT6352
> > > >> >
> > > >> > and I think it is correct.
> > > >>
> > > >> Ah, ok, that's a bit funny, because it means that this change actually
> > > >> never made any difference, because the codepath wasn't executed.
> > > >
> > > > Yes, this was used/tested on patched rt2x00 driver that switch to this
> > > > different codepath. Now it will be used by default :-)
> > > >
> > > > Stanislaw
> > > >
> > > >
> > > 
> > > Hi
> > > 
> > > For your reference: rt2x00: reduce tx power to nominal level on RT6352
> > > 
> > > iPA/eLNA - fixes too high power output
> > > ePA/eLNA - doesn't have any effect
> > > iPA/iLNA - not tested
> > 
> > Does someone have iPA/iLNA device so this can be tested?
> > Or it is not used combination on available devices? 
> 
> iPA/iLNA the most commonly found combination in cheap devices.
> iPA/eLNA is more rare, but found in some higher-quality devices.
> ePA/eLNA is available mostly in markets which allow higher TX power.
> ePA/iLNA haven't seen it yet, but theoretically possible.
> 
> Looking at the internal photos of Nexx WT3020, I'm very certain this
> is an iPA/iLNA device -- apart from regulators, magnetics, MT7620N
> itself and flash memory, another magnetics and RAM on the backside,
> there are no other parts on the board. Also afaik MT7620N only supports
> iLNA/iPA (due to the limited number of pins of the DRQFN package).

With the change on WT3020 I observed better RX throughput and more
or less the same TX throughput. Not sure why, since the settings
is about TX? I'll do more test, but so far Tom's change looks like
good improvment for me.

> See:
> https://apps.fcc.gov/eas/GetApplicationAttachment.html?id=2241504

I get 'You are not authorized to access this page.' :-/

Stanislaw

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Daniel Golle]

On Mon, Nov 04, 2019 at 10:15:26AM +0100, Stanislaw Gruszka wrote:
> > See:
> > https://apps.fcc.gov/eas/GetApplicationAttachment.html?id=2241504
> 
> I get 'You are not authorized to access this page.' :-/

Sorry for that. Seems like FCC doesn't like their content to be linked
by 3rd parties...

Try this and use links to internal photos:
https://fcc.io/N28/WT3020H

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Stanislaw Gruszka]

On Mon, Nov 04, 2019 at 10:15:25AM +0100, Stanislaw Gruszka wrote:
> > > > For your reference: rt2x00: reduce tx power to nominal level on RT6352
> > > > 
> > > > iPA/eLNA - fixes too high power output
> > > > ePA/eLNA - doesn't have any effect
> > > > iPA/iLNA - not tested
> > > 
> > > Does someone have iPA/iLNA device so this can be tested?
> > > Or it is not used combination on available devices? 
> > 
> > iPA/iLNA the most commonly found combination in cheap devices.
> > iPA/eLNA is more rare, but found in some higher-quality devices.
> > ePA/eLNA is available mostly in markets which allow higher TX power.
> > ePA/iLNA haven't seen it yet, but theoretically possible.
> > 
> > Looking at the internal photos of Nexx WT3020, I'm very certain this
> > is an iPA/iLNA device -- apart from regulators, magnetics, MT7620N
> > itself and flash memory, another magnetics and RAM on the backside,
> > there are no other parts on the board. Also afaik MT7620N only supports
> > iLNA/iPA (due to the limited number of pins of the DRQFN package).
> 
> With the change on WT3020 I observed better RX throughput and more
> or less the same TX throughput. Not sure why, since the settings
> is about TX? I'll do more test, but so far Tom's change looks like
> good improvment for me.

So, first of all I confused RX and TX testing in iperf. I observed
better TX throughput before (now I'm using netperf, which allow to
initialize performance tests in both directions from station, so 
I'm no longer confusing).

However better TX throughput come from TX{0,1}_{RF/BB}_GAIN_ATTEN
and TX_ALC_CFG_1 settings, because TX_ALC_VGA is initialized as 0
by hardware, so we have this settings before, when not switched
to the different codepath.

I also checked setting TX_ALC_VGA to 0x06060606 and it vastly degrade
performance for me in both directions on Nexx WT3020 and ASUS RT-AC51U.

Stanislaw

Re: [PATCH] rt2800: remove erroneous duplicate condition

[Tom Psyborg]

On 04/11/2019, Daniel Golle <daniel@makrotopia.org> wrote:
>
> ePA/eLNA is available mostly in markets which allow higher TX power.
> ePA/iLNA haven't seen it yet, but theoretically possible.
>

ePA/eLNA design can be done in different ways:
1. using separate chips for TX path (PA), RX path (LNA) and switch
2. using combo chip for all functions. in this case you can configure
chip's LNA setting to bypass mode, effectively switching it to
ePA/iLNA mode

[PATCH v2] rt2800: remove errornous duplicate condition

[Daniel Golle]

On 2019-10-28 06:07, wbob wrote:
> Hello Roman,
>
> while reading around drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> I stumbled on what I think is an edit of yours made in error in march
> 2017:
>
> https://github.com/torvalds/linux/commit/41977e86#diff-dae5dc10da180f3b055809a48118e18aR5281
>
> RT6352 in line 5281 should not have been introduced as the "else if"
> below line 5291 can then not take effect for a RT6352 device. Another
> possibility is for line 5291 to be not for RT6352, but this seems
> very unlikely. Are you able to clarify still after this substantial time?
>
> 5277: static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> ...
> 5279:  } else if (rt2x00_rt(rt2x00dev, RT5390) ||
> 5280:         rt2x00_rt(rt2x00dev, RT5392) ||
> 5281:         rt2x00_rt(rt2x00dev, RT6352)) {
> ...
> 5291:  } else if (rt2x00_rt(rt2x00dev, RT6352)) {
> ...

Hence remove errornous line 5281 to make the driver actually
execute the correct initialization routine for MT7620 chips.

As it was requested by Stanislaw Gruszka remove setting values of
MIMO_PS_CFG and TX_PIN_CFG. MIMO_PS_CFG is responsible for MIMO
power-safe mode (which is disabled), hence we can drop setting it.
TX_PIN_CFG is set correctly in other functions, and as setting this
value breaks some devices, rather don't set it here during init, but
only modify it later on.

Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
Reported-by: wbob <wbob@jify.de>
Reported-by: Roman Yeryomin <roman@advem.lv>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index f1cdcd61c54a..c99f1912e266 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -5839,8 +5839,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
 		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_0, 0x8000fc21);
 		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_3, 0x00009c40);
 	} else if (rt2x00_rt(rt2x00dev, RT5390) ||
-		   rt2x00_rt(rt2x00dev, RT5392) ||
-		   rt2x00_rt(rt2x00dev, RT6352)) {
+		   rt2x00_rt(rt2x00dev, RT5392)) {
 		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
@@ -5854,8 +5853,6 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
 		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000401);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x000C0000);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
-		rt2800_register_write(rt2x00dev, MIMO_PS_CFG, 0x00000002);
-		rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x00150F0F);
 		rt2800_register_write(rt2x00dev, TX_ALC_VGA3, 0x00000000);
 		rt2800_register_write(rt2x00dev, TX0_BB_GAIN_ATTEN, 0x0);
 		rt2800_register_write(rt2x00dev, TX1_BB_GAIN_ATTEN, 0x0);
-- 
2.23.0

Re: [PATCH v2] rt2800: remove errornous duplicate condition

[Stanislaw Gruszka]

On Sat, Nov 02, 2019 at 06:47:01PM +0100, Daniel Golle wrote:
> On 2019-10-28 06:07, wbob wrote:
> > Hello Roman,
> >
> > while reading around drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > I stumbled on what I think is an edit of yours made in error in march
> > 2017:
> >
> > https://github.com/torvalds/linux/commit/41977e86#diff-dae5dc10da180f3b055809a48118e18aR5281
> >
> > RT6352 in line 5281 should not have been introduced as the "else if"
> > below line 5291 can then not take effect for a RT6352 device. Another
> > possibility is for line 5291 to be not for RT6352, but this seems
> > very unlikely. Are you able to clarify still after this substantial time?
> >
> > 5277: static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> > ...
> > 5279:  } else if (rt2x00_rt(rt2x00dev, RT5390) ||
> > 5280:         rt2x00_rt(rt2x00dev, RT5392) ||
> > 5281:         rt2x00_rt(rt2x00dev, RT6352)) {
> > ...
> > 5291:  } else if (rt2x00_rt(rt2x00dev, RT6352)) {
> > ...
> 
> Hence remove errornous line 5281 to make the driver actually
> execute the correct initialization routine for MT7620 chips.
> 
> As it was requested by Stanislaw Gruszka remove setting values of
> MIMO_PS_CFG and TX_PIN_CFG. MIMO_PS_CFG is responsible for MIMO
> power-safe mode (which is disabled), hence we can drop setting it.
> TX_PIN_CFG is set correctly in other functions, and as setting this
> value breaks some devices, rather don't set it here during init, but
> only modify it later on.
> 
> Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
> Reported-by: wbob <wbob@jify.de>
> Reported-by: Roman Yeryomin <roman@advem.lv>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

Re: [PATCH v2] rt2800: remove errornous duplicate condition

[Kalle Valo]

Daniel Golle <daniel@makrotopia.org> wrote:

> On 2019-10-28 06:07, wbob wrote:
> > Hello Roman,
> >
> > while reading around drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> > I stumbled on what I think is an edit of yours made in error in march
> > 2017:
> >
> > https://github.com/torvalds/linux/commit/41977e86#diff-dae5dc10da180f3b055809a48118e18aR5281
> >
> > RT6352 in line 5281 should not have been introduced as the "else if"
> > below line 5291 can then not take effect for a RT6352 device. Another
> > possibility is for line 5291 to be not for RT6352, but this seems
> > very unlikely. Are you able to clarify still after this substantial time?
> >
> > 5277: static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> > ...
> > 5279:  } else if (rt2x00_rt(rt2x00dev, RT5390) ||
> > 5280:         rt2x00_rt(rt2x00dev, RT5392) ||
> > 5281:         rt2x00_rt(rt2x00dev, RT6352)) {
> > ...
> > 5291:  } else if (rt2x00_rt(rt2x00dev, RT6352)) {
> > ...
> 
> Hence remove errornous line 5281 to make the driver actually
> execute the correct initialization routine for MT7620 chips.
> 
> As it was requested by Stanislaw Gruszka remove setting values of
> MIMO_PS_CFG and TX_PIN_CFG. MIMO_PS_CFG is responsible for MIMO
> power-safe mode (which is disabled), hence we can drop setting it.
> TX_PIN_CFG is set correctly in other functions, and as setting this
> value breaks some devices, rather don't set it here during init, but
> only modify it later on.
> 
> Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
> Reported-by: wbob <wbob@jify.de>
> Reported-by: Roman Yeryomin <roman@advem.lv>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

Patch applied to wireless-drivers-next.git, thanks.

a1f7c2cabf70 rt2800: remove errornous duplicate condition

-- 
https://patchwork.kernel.org/patch/11224189/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH] rt2800: remove erroneous duplicate condition

[Daniel Golle]

On 2019-10-28 06:07, wbob wrote:
> Hello Roman,
>
> while reading around drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> I stumbled on what I think is an edit of yours made in error in march
> 2017:
>
> https://github.com/torvalds/linux/commit/41977e86#diff-dae5dc10da180f3b055809a48118e18aR5281
>
> RT6352 in line 5281 should not have been introduced as the "else if"
> below line 5291 can then not take effect for a RT6352 device. Another
> possibility is for line 5291 to be not for RT6352, but this seems
> very unlikely. Are you able to clarify still after this substantial time?
>
> 5277: static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
> ...
> 5279:  } else if (rt2x00_rt(rt2x00dev, RT5390) ||
> 5280:         rt2x00_rt(rt2x00dev, RT5392) ||
> 5281:         rt2x00_rt(rt2x00dev, RT6352)) {
> ...
> 5291:  } else if (rt2x00_rt(rt2x00dev, RT6352)) {
> ...

Hence remove erroneous line 5281 to make the driver actually
execute the correct initialization routine for MT7620 chips.

Fixes: 41977e86c984 ("rt2x00: add support for MT7620")
Reported-by: wbob <wbob@jify.de>
Reported-by: Roman Yeryomin <roman@advem.lv>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index f1cdcd61c54a..c85456c8c193 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -5839,8 +5839,7 @@ static int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
 		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_0, 0x8000fc21);
 		rt2800_register_write(rt2x00dev, TX_TXBF_CFG_3, 0x00009c40);
 	} else if (rt2x00_rt(rt2x00dev, RT5390) ||
-		   rt2x00_rt(rt2x00dev, RT5392) ||
-		   rt2x00_rt(rt2x00dev, RT6352)) {
+		   rt2x00_rt(rt2x00dev, RT5392)) {
 		rt2800_register_write(rt2x00dev, TX_SW_CFG0, 0x00000404);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG1, 0x00080606);
 		rt2800_register_write(rt2x00dev, TX_SW_CFG2, 0x00000000);
-- 
2.23.0