Message-ID: <CA+HBbNGvg43wMNbte827wmK_fnWuweKSgA-nWW+UPGCvunUwGA@mail.gmail.com>
Date:   Mon, 8 Nov 2021 22:39:27 +0100
From:   Robert Marko <robert.marko@...tura.hr>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, vivien.didelot@...il.com,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>, kuba@...nel.org,
        netdev@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Gabor Juhos <j4g8y7@...il.com>, John Crispin <john@...ozen.org>
Subject: Re: [net-next] net: dsa: qca8k: only change the MIB_EN bit in
 MODULE_EN register

On Mon, Nov 8, 2021 at 10:18 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> 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@...il.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@...il.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.

Sorry, QM_ERR_RST_EN is BIT(8) and it as well as LOOKUP_ERR_RST_EN should
be exactly the same on QCA833x switches as well as IPQ4019 uses a
variant of QCA8337N.
>
> > 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?

LOOKUP_ERR_RST_EN:
1b1:Enableautomatic software reset by hardware due to
lookup error.

QM_ERR_RST_EN:
1b1:enableautomatic software reset by hardware due to qm
error.

So clearing these 2 disables the built-in error recovery essentially.

To me clearing the bits even if they are not breaking something now
should at least have a comment in the code that indicates that it's intentional
for some reason.
I wish John would explain the logic behind this.

Regards,
Robert
>
> > 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@...il.com>
> > > > Signed-off-by: Robert Marko <robert.marko@...tura.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@...tura.hr
> > Web: www.sartura.hr



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@...tura.hr
Web: www.sartura.hr

Powered by blists - more mailing lists