Re: [PATCH v4 4/8] rt2800: fix registers init for MT7620

Re: [PATCH v4 4/8] rt2800: fix registers init for MT7620

[Daniel Golle]

On Tue, Oct 16, 2018 at 10:11:16AM +0200, Stanislaw Gruszka wrote:
> On Fri, Oct 12, 2018 at 12:48:07PM +0200, Tom Psyborg wrote:
> > chip version support exist in daniel's tree since a long time ago. so
> > don't disable registers initialization but try to upstream his
> > changes.
> 
> Where is this patch ? I can not find it.

So this requires to make the chip version and package available to
drivers like rt2x00. First of all, this is a patch for linux-mips:

https://git.openwrt.org/?p=openwrt/staging/dangole.git;a=blob;f=target/linux/ramips/patches-4.4/300-mt7620-export-chip-version-and-pkg.patch;h=f6aca6c90516f9c534b3c51e9f99dff6a3f41b75;hb=709fe05dfea58728d6accb9fe56c7056d9d0715b

It belongs to this (very outdated) tree:
https://git.openwrt.org/?p=openwrt/staging/dangole.git;a=shortlog;h=refs/heads/differentiate-pkg-ver-eco

I'm not sure whether this is the right way to do this, but it worked.


Cheers


Daniel



> 
> Thanks
> Stanislaw

[PATCH] mips: ralink: add accessors for MT7620 chipver and pkg

[Daniel Golle]

The RT6352 wireless core included in all MT7620 chips is implemented
differently in MT7620A (TFBGA) and MT7620N (DR-QFN).
Hence provide accessor functions similar to the already existing
mt7620_get_eco() function which allow the rt2x00 wireless driver to
figure out which WiSoC it is being run on.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 arch/mips/include/asm/mach-ralink/mt7620.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/mips/include/asm/mach-ralink/mt7620.h b/arch/mips/include/asm/mach-ralink/mt7620.h
index 66af4ccb5c6c..d0310a92a63f 100644
--- a/arch/mips/include/asm/mach-ralink/mt7620.h
+++ b/arch/mips/include/asm/mach-ralink/mt7620.h
@@ -137,4 +137,16 @@ static inline int mt7620_get_eco(void)
 	return rt_sysc_r32(SYSC_REG_CHIP_REV) & CHIP_REV_ECO_MASK;
 }
 
+static inline int mt7620_get_chipver(void)
+{
+	return (rt_sysc_r32(SYSC_REG_CHIP_REV) >> CHIP_REV_VER_SHIFT) &
+		CHIP_REV_VER_MASK;
+}
+
+static inline int mt7620_get_pkg(void)
+{
+	return (rt_sysc_r32(SYSC_REG_CHIP_REV) >> CHIP_REV_PKG_SHIFT) &
+		CHIP_REV_PKG_MASK;
+}
+
 #endif
-- 
2.19.1

Re: [PATCH] mips: ralink: add accessors for MT7620 chipver and pkg

[Paul Burton]

Hi Daniel,

On Fri, Nov 02, 2018 at 03:07:19AM +0100, Daniel Golle wrote:
> The RT6352 wireless core included in all MT7620 chips is implemented
> differently in MT7620A (TFBGA) and MT7620N (DR-QFN).
> Hence provide accessor functions similar to the already existing
> mt7620_get_eco() function which allow the rt2x00 wireless driver to
> figure out which WiSoC it is being run on.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  arch/mips/include/asm/mach-ralink/mt7620.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/mips/include/asm/mach-ralink/mt7620.h b/arch/mips/include/asm/mach-ralink/mt7620.h
> index 66af4ccb5c6c..d0310a92a63f 100644
> --- a/arch/mips/include/asm/mach-ralink/mt7620.h
> +++ b/arch/mips/include/asm/mach-ralink/mt7620.h
> @@ -137,4 +137,16 @@ static inline int mt7620_get_eco(void)
>  	return rt_sysc_r32(SYSC_REG_CHIP_REV) & CHIP_REV_ECO_MASK;
>  }
>  
> +static inline int mt7620_get_chipver(void)
> +{
> +	return (rt_sysc_r32(SYSC_REG_CHIP_REV) >> CHIP_REV_VER_SHIFT) &
> +		CHIP_REV_VER_MASK;
> +}
> +
> +static inline int mt7620_get_pkg(void)
> +{
> +	return (rt_sysc_r32(SYSC_REG_CHIP_REV) >> CHIP_REV_PKG_SHIFT) &
> +		CHIP_REV_PKG_MASK;
> +}
> +
>  #endif

Is there an in-tree user for these?

Looking at it I don't see any in-tree code which uses the existing
mt7620_get_eco() function. I'm not fond of adding code which isn't used
at all in-tree, I'd much rather we either:

 1) Get the driver that needs these upstreamed, and these functions
    could be added at the same time.

or

 2) Keep functions only used by out-of-tree code out-of-tree.

Thanks,
    Paul

Re: [PATCH] mips: ralink: add accessors for MT7620 chipver and pkg

[Daniel Golle]

Hi Paul,

thank you for the review!

On Mon, Nov 05, 2018 at 06:36:16PM +0000, Paul Burton wrote:
> Hi Daniel,
> 
> On Fri, Nov 02, 2018 at 03:07:19AM +0100, Daniel Golle wrote:
> > The RT6352 wireless core included in all MT7620 chips is implemented
> > differently in MT7620A (TFBGA) and MT7620N (DR-QFN).
> > Hence provide accessor functions similar to the already existing
> > mt7620_get_eco() function which allow the rt2x00 wireless driver to
> > figure out which WiSoC it is being run on.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  arch/mips/include/asm/mach-ralink/mt7620.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/arch/mips/include/asm/mach-ralink/mt7620.h b/arch/mips/include/asm/mach-ralink/mt7620.h
> > index 66af4ccb5c6c..d0310a92a63f 100644
> > --- a/arch/mips/include/asm/mach-ralink/mt7620.h
> > +++ b/arch/mips/include/asm/mach-ralink/mt7620.h
> > @@ -137,4 +137,16 @@ static inline int mt7620_get_eco(void)
> >  	return rt_sysc_r32(SYSC_REG_CHIP_REV) & CHIP_REV_ECO_MASK;
> >  }
> >  
> > +static inline int mt7620_get_chipver(void)
> > +{
> > +	return (rt_sysc_r32(SYSC_REG_CHIP_REV) >> CHIP_REV_VER_SHIFT) &
> > +		CHIP_REV_VER_MASK;
> > +}
> > +
> > +static inline int mt7620_get_pkg(void)
> > +{
> > +	return (rt_sysc_r32(SYSC_REG_CHIP_REV) >> CHIP_REV_PKG_SHIFT) &
> > +		CHIP_REV_PKG_MASK;
> > +}
> > +
> >  #endif
> 
> Is there an in-tree user for these?

Not yet, OpenWrt's out-of-tree Ethernet driver needs the already
existing int mt7620_get_eco(void) and is going to be upstreamed once
MT7530 DSA for has been completed to work with MT7621. See the driver
in [1].

The two newly introduced accessors are going to be used by the in-tree
rt2x00 driver which gained support for the RT6352 wireless core
included in that SoC recently. In order to be able to carry out tuning
in the same way the vendor driver does, rt2x00 will need to access the
pkg and chipver fields. See [2] for example.

> 
> Looking at it I don't see any in-tree code which uses the existing
> mt7620_get_eco() function. I'm not fond of adding code which isn't used
> at all in-tree, I'd much rather we either:
> 
>  1) Get the driver that needs these upstreamed, and these functions
>     could be added at the same time.
> 
> or
> 
>  2) Keep functions only used by out-of-tree code out-of-tree.

I understand your concerns with regard to the mt7620_get_eco(void)
function which is currently only used by the out-of-tree Ethernet
driver. However, the to-be-introduced functions mt7620_get_pkg(void)
and mt7620_get_chipver(void) are to be used in-tree by
drivers/net/wireless/ralink/rt2x00 in the very near future. I just
wanted to consult whether the introductions of such accessors is
generally acceptable before implementing the changes in rt2x00.


Best regards


Daniel


[1]: git://git.openwrt.org/openwrt.git:/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/soc_mt7620.c

[2]: https://github.com/i80s/mtk-sources/blob/master/mt7620/src/chips/rt6352.c#L1019

> 
> Thanks,
>     Paul

Re: [PATCH] mips: ralink: add accessors for MT7620 chipver and pkg

[Paul Burton]

Hi Daniel,

On Mon, Nov 05, 2018 at 09:19:35PM +0100, Daniel Golle wrote:
> Hi Paul,
> 
> thank you for the review!

You're very welcome :)

> On Mon, Nov 05, 2018 at 06:36:16PM +0000, Paul Burton wrote:
> > Is there an in-tree user for these?
> 
> Not yet, OpenWrt's out-of-tree Ethernet driver needs the already
> existing int mt7620_get_eco(void) and is going to be upstreamed once
> MT7530 DSA for has been completed to work with MT7621. See the driver
> in [1].
> 
> The two newly introduced accessors are going to be used by the in-tree
> rt2x00 driver which gained support for the RT6352 wireless core
> included in that SoC recently. In order to be able to carry out tuning
> in the same way the vendor driver does, rt2x00 will need to access the
> pkg and chipver fields. See [2] for example.
> 
> > 
> > Looking at it I don't see any in-tree code which uses the existing
> > mt7620_get_eco() function. I'm not fond of adding code which isn't used
> > at all in-tree, I'd much rather we either:
> > 
> >  1) Get the driver that needs these upstreamed, and these functions
> >     could be added at the same time.
> > 
> > or
> > 
> >  2) Keep functions only used by out-of-tree code out-of-tree.
> 
> I understand your concerns with regard to the mt7620_get_eco(void)
> function which is currently only used by the out-of-tree Ethernet
> driver. However, the to-be-introduced functions mt7620_get_pkg(void)
> and mt7620_get_chipver(void) are to be used in-tree by
> drivers/net/wireless/ralink/rt2x00 in the very near future. I just
> wanted to consult whether the introductions of such accessors is
> generally acceptable before implementing the changes in rt2x00.

OK, then my suggestion is that you include this patch in the rt2x00
series you create & feel free to add:

    Acked-by: Paul Burton <paul.burton@mips.com>

Presuming that the functions are used by something else in the patch
series.

Does that sound OK to you?

Thanks,
    Paul

Re: [PATCH] mips: ralink: add accessors for MT7620 chipver and pkg

[Tom Psyborg]

Hi

ACK from me on this one. Tested and working.