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

[Robert Marko]

On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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.
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
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?

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.

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@xxxxxxxxx>
> > Signed-off-by: Robert Marko <robert.marko@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx
Web: www.sartura.hr