Re: [net-next] net: dsa: qca8k: only change the MIB_EN bit in MODULE_EN register

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:
> On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Timed out waiting for ACK/NACK from John.
> >
> > On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > > From: Gabor Juhos <j4g8y7@gmail.com>
> > >
> > > The MIB module needs to be enabled in the MODULE_EN register in
> > > order to make it to counting. This is done in the qca8k_mib_init()
> > > function. However instead of only changing the MIB module enable
> > > bit, the function writes the whole register. As a side effect other
> > > internal modules gets disabled.
> >
> > Please be more specific.
> > The MODULE_EN register contains these other bits:
> > BIT(0): MIB_EN
> > BIT(1): ACL_EN (ACL module enable)
> > BIT(2): L3_EN (Layer 3 offload enable)
> > BIT(10): SPECIAL_DIP_EN (Enable special DIP (224.0.0.x or ff02::1) broadcast
> > 0 = Use multicast DP
> > 1 = Use broadcast DP)
> >
> > >
> > > Fix up the code to only change the MIB module specific bit.
> >
> > Clearing which one of the above bits bothers you? The driver for the
> > qca8k switch supports neither layer 3 offloading nor ACLs, and I don't
> > really know what this special DIP packet/header is).
> >
> > Generally the assumption for OF-based drivers is that one should not
> > rely on any configuration done by prior boot stages, so please explain
> > what should have worked but doesn't.
> 
> Hi,
> I think that the commit message wasn't clear enough and that's my fault for not
> fixing it up before sending.

Yes, it is not. If things turn out to need changing, you should resend
with an updated commit message.

> MODULE_EN register has 3 more bits that aren't documented in the QCA8337
> datasheet but only in the IPQ4019 one but they are there.
> Those are:
> BIT(31) S17C_INT (This one is IPQ4019 specific)
> BIT(9) LOOKUP_ERR_RST_EN
> BIT(10) QM_ERR_RST_EN

Are you sure that BIT(10) is QM_ERR_RST_EN on IPQ4019? Because in the
QCA8334 document I'm looking at, it is SPECIAL_DIP_EN.

> Lookup and QM bits as well as the DIP default to 1 while the INT bit is 0.
> 
> Clearing the QM and Lookup bits is what is bothering me, why should we clear HW
> default bits without mentioning that they are being cleared and for what reason?

To be fair, BIT(9) is marked as RESERVED and documented as being set to 1,
so writing a zero is probably not very smart.

> We aren't depending on the bootloader or whatever configuring the switch, we are
> even invoking the HW reset before doing anything to make sure that the
> whole networking
> subsystem in IPQ4019 is back to HW defaults to get rid of various
> bootloader hackery.
> 
> Gabor found this while working on IPQ4019 support and to him and to me it looks
> like a bug.

A bug with what impact? I don't have a description of those bits that
get unset. What do they do, what doesn't work?

> I hope this clears up things a bit.
> Regards,
> Robert
> >
> > >
> > > Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")
> > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > ---
> > >  drivers/net/dsa/qca8k.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > index a984f06f6f04..a229776924f8 100644
> > > --- a/drivers/net/dsa/qca8k.c
> > > +++ b/drivers/net/dsa/qca8k.c
> > > @@ -583,7 +583,7 @@ qca8k_mib_init(struct qca8k_priv *priv)
> > >       if (ret)
> > >               goto exit;
> > >
> > > -     ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> > > +     ret = qca8k_reg_set(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> > >
> > >  exit:
> > >       mutex_unlock(&priv->reg_mutex);
> > > --
> > > 2.33.1
> > >
> 
> 
> 
> -- 
> Robert Marko
> Staff Embedded Linux Engineer
> Sartura Ltd.
> Lendavska ulica 16a
> 10000 Zagreb, Croatia
> Email: robert.marko@sartura.hr
> Web: www.sartura.hr