Authenticated-By: 
From:   Ping-Ke Shih <pkshih@realtek.com>
To:     Kalle Valo <kvalo@kernel.org>
CC:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: RE: [PATCH] wifi: rtw89: add counters of register-based H2C/C2H
Thread-Topic: [PATCH] wifi: rtw89: add counters of register-based H2C/C2H
Thread-Index: AQHZUwUX5oym8Upi60mueYnXnELqdK77obDCgAAfjeA=
Date:   Wed, 15 Mar 2023 11:56:03 +0000
Message-ID: <485422aa276d49878278592861a103f8@realtek.com>
References: <20230310035943.49020-1-pkshih@realtek.com>
 <87r0tqmje1.fsf@kernel.org>
In-Reply-To: <87r0tqmje1.fsf@kernel.org>
Accept-Language: en-US, zh-TW
Content-Language: zh-TW
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Precedence: bulk


> -----Original Message-----
> From: Kalle Valo <kvalo@kernel.org>
> Sent: Wednesday, March 15, 2023 5:52 PM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: linux-wireless@vger.kernel.org
> Subject: Re: [PATCH] wifi: rtw89: add counters of register-based H2C/C2H
> 
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > The register-based H2C/C2H are used to exchange information between driver
> > and firmware, but only apply to narrow area because its data size is
> > smaller than regular packet-based H2C/C2H.
> >
> > This kind of H2C/C2H must be paired. To identify if any H2C/C2H is missing,
> > update counters to help diagnose this kind of problems.
> >
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> 
> [...]
> 
> > +static inline void
> > +rtw89_write8_mask_add(struct rtw89_dev *rtwdev, u32 addr, u32 mask, u8 add)
> > +{
> > +     u32 shift;
> > +     u8 orig, set;
> > +     u8 data;
> > +
> > +     mask &= 0xff;
> > +     shift = __ffs(mask);
> > +
> > +     orig = rtw89_read8(rtwdev, addr);
> > +     data = ((orig & mask) >> shift) + add;
> > +     set = (orig & ~mask) | ((data << shift) & mask);
> > +     rtw89_write8(rtwdev, addr, set);
> > +}
> 
> This function has a lot of shifting etc which feels like reinventing the
> wheel, doesn't linux/bitfield.h contain what you need? For example,
> u32_get_bits() and u32_replace_bits()?
> 

The mask argument of u32_get_bits() and his friends should be const, but our
usage could be a variable. For now, we have only one use case that the mask is
definitely const, but I remember it could lead some warning if we don't define
this as 'static __always_inline'.


My original thought to implement this function is 

rtw89_write8_mask_add(struct rtw89_dev *rtwdev, u32 addr, u32 mask, u8 add)
{
	u8 tmp;

	tmp = rtw89_read8_mask(rtwdev, addr, mask);
	tmp += add;
	rtw89_write8_mask(rtwdev, addr, mask, tmp);
}

But, this needs three IO (two reading and one writing IO), so I implement this
a little odd patch. 


I'm thinking I can have another implementation that adds variables to maintain
counters by driver, and then I only need existing rtw89_write8_mask() to update
counters instead. Therefore, no need rtw89_write8_mask_add(). I will use this
method by v2.

Ping-Ke