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

[Daniel Golle <daniel@makrotopia.org>]

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