* [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep() @ 2025-11-17 3:29 Ping-Ke Shih 2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih 2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih 0 siblings, 2 replies; 11+ messages in thread From: Ping-Ke Shih @ 2025-11-17 3:29 UTC (permalink / raw) To: linux-wireless; +Cc: geert As Geert mentioned [1], two more tricky cases are pointed out by implementation of field_prep(). Change the code to resolve the tricks. [1] https://lore.kernel.org/linux-wireless/fccadfe07e4244b993f44ac7315d3d52@realtek.com/T/#mbdfd08117f67b39fabb72dd71acb00f4b3c22bd8 Ping-Ke Shih (2): wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() drivers/net/wireless/realtek/rtw89/mac.h | 20 ++++++++++++++ drivers/net/wireless/realtek/rtw89/rtw8851b.c | 26 +++++-------------- .../net/wireless/realtek/rtw89/rtw8852a_rfk.c | 8 +++--- drivers/net/wireless/realtek/rtw89/rtw8852b.c | 26 +++++-------------- drivers/net/wireless/realtek/rtw89/rtw8852c.c | 26 +++++-------------- 5 files changed, 45 insertions(+), 61 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO 2025-11-17 3:29 [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep() Ping-Ke Shih @ 2025-11-17 3:29 ` Ping-Ke Shih 2025-11-18 10:24 ` Geert Uytterhoeven 2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih 1 sibling, 1 reply; 11+ messages in thread From: Ping-Ke Shih @ 2025-11-17 3:29 UTC (permalink / raw) To: linux-wireless; +Cc: geert The field mask should be bits 16-31, but suddenly use wrong bits 24-31, rarely causing a little performance degraded if DAC/DAC FIFO stays on an unexpected state. Found this by Geert who works on bit field functions. Cc: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> --- drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c b/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c index e74257d19412..463399413318 100644 --- a/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c +++ b/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c @@ -756,8 +756,8 @@ static void _iqk_rxk_setting(struct rtw89_dev *rtwdev, u8 path) rtw89_phy_write32_mask(rtwdev, R_ANAPAR, B_ANAPAR_FLTRST, 0x1); rtw89_phy_write32_mask(rtwdev, R_ANAPAR_PW15, B_ANAPAR_PW15_H2, 0x0); udelay(1); - rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0303); - rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0000); + rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0303); + rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0000); switch (iqk_info->iqk_band[path]) { case RTW89_BAND_2G: @@ -1239,8 +1239,8 @@ static void _iqk_txk_setting(struct rtw89_dev *rtwdev, u8 path) udelay(1); rtw89_phy_write32_mask(rtwdev, R_ANAPAR, B_ANAPAR_15, 0x0041); udelay(1); - rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0303); - rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0000); + rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0303); + rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0000); switch (iqk_info->iqk_band[path]) { case RTW89_BAND_2G: rtw89_write_rf(rtwdev, path, RR_XALNA2, RR_XALNA2_SW, 0x00); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO 2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih @ 2025-11-18 10:24 ` Geert Uytterhoeven 2025-11-19 0:44 ` Ping-Ke Shih 0 siblings, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2025-11-18 10:24 UTC (permalink / raw) To: Ping-Ke Shih; +Cc: linux-wireless On Mon, 17 Nov 2025 at 04:29, Ping-Ke Shih <pkshih@realtek.com> wrote: > The field mask should be bits 16-31, but suddenly use wrong bits 24-31, > rarely causing a little performance degraded if DAC/DAC FIFO stays on > an unexpected state. > > Found this by Geert who works on bit field functions. > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO 2025-11-18 10:24 ` Geert Uytterhoeven @ 2025-11-19 0:44 ` Ping-Ke Shih 2025-11-19 8:35 ` Geert Uytterhoeven 0 siblings, 1 reply; 11+ messages in thread From: Ping-Ke Shih @ 2025-11-19 0:44 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, 17 Nov 2025 at 04:29, Ping-Ke Shih <pkshih@realtek.com> wrote: > > The field mask should be bits 16-31, but suddenly use wrong bits 24-31, > > rarely causing a little performance degraded if DAC/DAC FIFO stays on > > an unexpected state. > > > > Found this by Geert who works on bit field functions. > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > I suppose you meant Reviewed-by, but you also the reporter. I will add these tags by v2. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO 2025-11-19 0:44 ` Ping-Ke Shih @ 2025-11-19 8:35 ` Geert Uytterhoeven 0 siblings, 0 replies; 11+ messages in thread From: Geert Uytterhoeven @ 2025-11-19 8:35 UTC (permalink / raw) To: Ping-Ke Shih; +Cc: linux-wireless@vger.kernel.org Hi Ping-Ke, On Wed, 19 Nov 2025 at 01:44, Ping-Ke Shih <pkshih@realtek.com> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, 17 Nov 2025 at 04:29, Ping-Ke Shih <pkshih@realtek.com> wrote: > > > The field mask should be bits 16-31, but suddenly use wrong bits 24-31, > > > rarely causing a little performance degraded if DAC/DAC FIFO stays on > > > an unexpected state. > > > > > > Found this by Geert who works on bit field functions. > > > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > I suppose you meant Reviewed-by, but you also the reporter. I will add > these tags by v2. I did mean Reported-by. I cannot review the actual values, as I do not have hardware documentation. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() 2025-11-17 3:29 [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep() Ping-Ke Shih 2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih @ 2025-11-17 3:29 ` Ping-Ke Shih 2025-11-18 10:40 ` Geert Uytterhoeven 1 sibling, 1 reply; 11+ messages in thread From: Ping-Ke Shih @ 2025-11-17 3:29 UTC (permalink / raw) To: linux-wireless; +Cc: geert The power value and enable bit fields can be not consecutive mask, but normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is consecutive bit mask. Therefore, change the code accordingly. Cc: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> --- drivers/net/wireless/realtek/rtw89/mac.h | 20 ++++++++++++++ drivers/net/wireless/realtek/rtw89/rtw8851b.c | 26 +++++-------------- drivers/net/wireless/realtek/rtw89/rtw8852b.c | 26 +++++-------------- drivers/net/wireless/realtek/rtw89/rtw8852c.c | 26 +++++-------------- 4 files changed, 41 insertions(+), 57 deletions(-) diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h index 3cc97fd0c0ec..01a9fb7c9e31 100644 --- a/drivers/net/wireless/realtek/rtw89/mac.h +++ b/drivers/net/wireless/realtek/rtw89/mac.h @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev, return 0; } +static inline +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val, + u32 mask_en, bool cond) +{ + u32 wrt = u32_encode_bits(val, mask); + u32 val32; + int ret; + + if (cond) + wrt |= mask_en; + + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32); + if (ret) + return; + + val32 &= ~(mask | mask_en); + val32 |= wrt; + rtw89_mac_txpwr_write32(rtwdev, RTW89_PHY_0, reg, val32); +} + static inline void rtw89_mac_ctrl_hci_dma_tx(struct rtw89_dev *rtwdev, bool enable) { diff --git a/drivers/net/wireless/realtek/rtw89/rtw8851b.c b/drivers/net/wireless/realtek/rtw89/rtw8851b.c index 2019f6022cbb..1253c4af2fb2 100644 --- a/drivers/net/wireless/realtek/rtw89/rtw8851b.c +++ b/drivers/net/wireless/realtek/rtw89/rtw8851b.c @@ -2291,18 +2291,6 @@ rtw8851b_btc_set_wl_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 txpwr_val) union rtw8851b_btc_wl_txpwr_ctrl arg = { .txpwr_val = txpwr_val }; s32 val; -#define __write_ctrl(_reg, _msk, _val, _en, _cond) \ -do { \ - u32 _wrt = FIELD_PREP(_msk, _val); \ - BUILD_BUG_ON(!!(_msk & _en)); \ - if (_cond) \ - _wrt |= _en; \ - else \ - _wrt &= ~_en; \ - rtw89_mac_txpwr_write32_mask(rtwdev, RTW89_PHY_0, _reg, \ - _msk | _en, _wrt); \ -} while (0) - switch (arg.ctrl_all_time) { case 0xffff: val = 0; @@ -2312,9 +2300,10 @@ do { \ break; } - __write_ctrl(R_AX_PWR_RATE_CTRL, B_AX_FORCE_PWR_BY_RATE_VALUE_MASK, - val, B_AX_FORCE_PWR_BY_RATE_EN, - arg.ctrl_all_time != 0xffff); + rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_RATE_CTRL, + B_AX_FORCE_PWR_BY_RATE_VALUE_MASK, + val, B_AX_FORCE_PWR_BY_RATE_EN, + arg.ctrl_all_time != 0xffff); switch (arg.ctrl_gnt_bt) { case 0xffff: @@ -2325,10 +2314,9 @@ do { \ break; } - __write_ctrl(R_AX_PWR_COEXT_CTRL, B_AX_TXAGC_BT_MASK, val, - B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff); - -#undef __write_ctrl + rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_COEXT_CTRL, + B_AX_TXAGC_BT_MASK, val, + B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff); } static diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852b.c b/drivers/net/wireless/realtek/rtw89/rtw8852b.c index 38cd151f8c3f..de02d52150c1 100644 --- a/drivers/net/wireless/realtek/rtw89/rtw8852b.c +++ b/drivers/net/wireless/realtek/rtw89/rtw8852b.c @@ -761,18 +761,6 @@ rtw8852b_btc_set_wl_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 txpwr_val) union rtw8852b_btc_wl_txpwr_ctrl arg = { .txpwr_val = txpwr_val }; s32 val; -#define __write_ctrl(_reg, _msk, _val, _en, _cond) \ -do { \ - u32 _wrt = FIELD_PREP(_msk, _val); \ - BUILD_BUG_ON(!!(_msk & _en)); \ - if (_cond) \ - _wrt |= _en; \ - else \ - _wrt &= ~_en; \ - rtw89_mac_txpwr_write32_mask(rtwdev, RTW89_PHY_0, _reg, \ - _msk | _en, _wrt); \ -} while (0) - switch (arg.ctrl_all_time) { case 0xffff: val = 0; @@ -782,9 +770,10 @@ do { \ break; } - __write_ctrl(R_AX_PWR_RATE_CTRL, B_AX_FORCE_PWR_BY_RATE_VALUE_MASK, - val, B_AX_FORCE_PWR_BY_RATE_EN, - arg.ctrl_all_time != 0xffff); + rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_RATE_CTRL, + B_AX_FORCE_PWR_BY_RATE_VALUE_MASK, + val, B_AX_FORCE_PWR_BY_RATE_EN, + arg.ctrl_all_time != 0xffff); switch (arg.ctrl_gnt_bt) { case 0xffff: @@ -795,10 +784,9 @@ do { \ break; } - __write_ctrl(R_AX_PWR_COEXT_CTRL, B_AX_TXAGC_BT_MASK, val, - B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff); - -#undef __write_ctrl + rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_COEXT_CTRL, + B_AX_TXAGC_BT_MASK, val, + B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff); } static const struct rtw89_chip_ops rtw8852b_chip_ops = { diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852c.c b/drivers/net/wireless/realtek/rtw89/rtw8852c.c index ee81a6792eee..7fd1485d1eb7 100644 --- a/drivers/net/wireless/realtek/rtw89/rtw8852c.c +++ b/drivers/net/wireless/realtek/rtw89/rtw8852c.c @@ -2760,18 +2760,6 @@ rtw8852c_btc_set_wl_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 txpwr_val) union rtw8852c_btc_wl_txpwr_ctrl arg = { .txpwr_val = txpwr_val }; s32 val; -#define __write_ctrl(_reg, _msk, _val, _en, _cond) \ -do { \ - u32 _wrt = FIELD_PREP(_msk, _val); \ - BUILD_BUG_ON((_msk & _en) != 0); \ - if (_cond) \ - _wrt |= _en; \ - else \ - _wrt &= ~_en; \ - rtw89_mac_txpwr_write32_mask(rtwdev, RTW89_PHY_0, _reg, \ - _msk | _en, _wrt); \ -} while (0) - switch (arg.ctrl_all_time) { case 0xffff: val = 0; @@ -2781,9 +2769,10 @@ do { \ break; } - __write_ctrl(R_AX_PWR_RATE_CTRL, B_AX_FORCE_PWR_BY_RATE_VALUE_MASK, - val, B_AX_FORCE_PWR_BY_RATE_EN, - arg.ctrl_all_time != 0xffff); + rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_RATE_CTRL, + B_AX_FORCE_PWR_BY_RATE_VALUE_MASK, + val, B_AX_FORCE_PWR_BY_RATE_EN, + arg.ctrl_all_time != 0xffff); switch (arg.ctrl_gnt_bt) { case 0xffff: @@ -2794,10 +2783,9 @@ do { \ break; } - __write_ctrl(R_AX_PWR_COEXT_CTRL, B_AX_TXAGC_BT_MASK, val, - B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff); - -#undef __write_ctrl + rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_COEXT_CTRL, + B_AX_TXAGC_BT_MASK, val, + B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff); } static -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() 2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih @ 2025-11-18 10:40 ` Geert Uytterhoeven 2025-11-19 1:15 ` Ping-Ke Shih 0 siblings, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2025-11-18 10:40 UTC (permalink / raw) To: Ping-Ke Shih; +Cc: linux-wireless Hi Ping-Ke, On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote: > The power value and enable bit fields can be not consecutive mask, but > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is > consecutive bit mask. Therefore, change the code accordingly. > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/net/wireless/realtek/rtw89/mac.h > +++ b/drivers/net/wireless/realtek/rtw89/mac.h > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev, > return 0; > } > > +static inline > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val, > + u32 mask_en, bool cond) > +{ > + u32 wrt = u32_encode_bits(val, mask); Nit: you could do without this variable... > + u32 val32; > + int ret; > + > + if (cond) > + wrt |= mask_en; > + > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32); > + if (ret) > + return; > + > + val32 &= ~(mask | mask_en); > + val32 |= wrt; val32 |= u32_encode_bits(val, mask); if (cond) cal32 |= mask_en; > + rtw89_mac_txpwr_write32(rtwdev, RTW89_PHY_0, reg, val32); As this calls mac->get_txpwr_cr() a second time, perhaps it is better to open-code rtw89_mac_txpwr_read32() and rtw89_mac_txpwr_write32() in this function? > +} > + > static inline void rtw89_mac_ctrl_hci_dma_tx(struct rtw89_dev *rtwdev, > bool enable) > { Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() 2025-11-18 10:40 ` Geert Uytterhoeven @ 2025-11-19 1:15 ` Ping-Ke Shih 2025-11-19 8:38 ` Geert Uytterhoeven 0 siblings, 1 reply; 11+ messages in thread From: Ping-Ke Shih @ 2025-11-19 1:15 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ping-Ke, > > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote: > > The power value and enable bit fields can be not consecutive mask, but > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is > > consecutive bit mask. Therefore, change the code accordingly. > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- a/drivers/net/wireless/realtek/rtw89/mac.h > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev, > > return 0; > > } > > > > +static inline > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val, > > + u32 mask_en, bool cond) > > +{ > > + u32 wrt = u32_encode_bits(val, mask); > > Nit: you could do without this variable... > > > + u32 val32; > > + int ret; > > + > > + if (cond) > > + wrt |= mask_en; > > + > > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32); > > + if (ret) > > + return; > > + > > + val32 &= ~(mask | mask_en); > > + val32 |= wrt; > > val32 |= u32_encode_bits(val, mask); > if (cond) > cal32 |= mask_en; With this change, ARCH arm is failed to build (x86 is well): In file included from /build/rtw89/core.h:9, from /build/rtw89/coex.h:8, from /build/rtw89/rtw8851b.c:5: In function 'field_multiplier', inlined from 'field_mask' at ./include/linux/bitfield.h:170:17, inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1, inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11: ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad bitfield mask 165 | __bad_mask(); | ^~~~~~~~~~~~ In function 'field_multiplier', > > > + rtw89_mac_txpwr_write32(rtwdev, RTW89_PHY_0, reg, val32); > > As this calls mac->get_txpwr_cr() a second time, perhaps it is better to > open-code rtw89_mac_txpwr_read32() and rtw89_mac_txpwr_write32() > in this function? This function isn't in hot path, and using rtw89_mac_txpwr_read32() and rtw89_mac_txpwr_write32() is clearer than open-code, so I'd keep it as was. Then I don't plan to send v2. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() 2025-11-19 1:15 ` Ping-Ke Shih @ 2025-11-19 8:38 ` Geert Uytterhoeven 2025-11-19 8:53 ` Ping-Ke Shih 0 siblings, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2025-11-19 8:38 UTC (permalink / raw) To: Ping-Ke Shih; +Cc: linux-wireless@vger.kernel.org Hi Ping-Ke, On Wed, 19 Nov 2025 at 02:15, Ping-Ke Shih <pkshih@realtek.com> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote: > > > The power value and enable bit fields can be not consecutive mask, but > > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is > > > consecutive bit mask. Therefore, change the code accordingly. > > > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > > > Thanks for your patch! > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > --- a/drivers/net/wireless/realtek/rtw89/mac.h > > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h > > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev, > > > return 0; > > > } > > > > > > +static inline > > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val, > > > + u32 mask_en, bool cond) > > > +{ > > > + u32 wrt = u32_encode_bits(val, mask); > > > > Nit: you could do without this variable... > > > > > + u32 val32; > > > + int ret; > > > + > > > + if (cond) > > > + wrt |= mask_en; > > > + > > > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32); > > > + if (ret) > > > + return; > > > + > > > + val32 &= ~(mask | mask_en); > > > + val32 |= wrt; > > > > val32 |= u32_encode_bits(val, mask); > > if (cond) > > cal32 |= mask_en; > > With this change, ARCH arm is failed to build (x86 is well): > > In file included from /build/rtw89/core.h:9, > from /build/rtw89/coex.h:8, > from /build/rtw89/rtw8851b.c:5: > In function 'field_multiplier', > inlined from 'field_mask' at ./include/linux/bitfield.h:170:17, > inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1, > inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11: > ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad bitfield mask > 165 | __bad_mask(); > | ^~~~~~~~~~~~ > In function 'field_multiplier', Hmm... Note that u32_encode_bits() really requires a constant mask, just like FIELD_PREP(). So probably the compiler can no longer deduce it is called with a constant after restructuring the code... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() 2025-11-19 8:38 ` Geert Uytterhoeven @ 2025-11-19 8:53 ` Ping-Ke Shih 2025-12-12 2:09 ` Ping-Ke Shih 0 siblings, 1 reply; 11+ messages in thread From: Ping-Ke Shih @ 2025-11-19 8:53 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ping-Ke, > > On Wed, 19 Nov 2025 at 02:15, Ping-Ke Shih <pkshih@realtek.com> wrote: > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote: > > > > The power value and enable bit fields can be not consecutive mask, but > > > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is > > > > consecutive bit mask. Therefore, change the code accordingly. > > > > > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > > > > > Thanks for your patch! > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > --- a/drivers/net/wireless/realtek/rtw89/mac.h > > > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h > > > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev, > > > > return 0; > > > > } > > > > > > > > +static inline > > > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val, > > > > + u32 mask_en, bool cond) > > > > +{ > > > > + u32 wrt = u32_encode_bits(val, mask); > > > > > > Nit: you could do without this variable... > > > > > > > + u32 val32; > > > > + int ret; > > > > + > > > > + if (cond) > > > > + wrt |= mask_en; > > > > + > > > > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32); > > > > + if (ret) > > > > + return; > > > > + > > > > + val32 &= ~(mask | mask_en); > > > > + val32 |= wrt; > > > > > > val32 |= u32_encode_bits(val, mask); > > > if (cond) > > > cal32 |= mask_en; > > > > With this change, ARCH arm is failed to build (x86 is well): > > > > In file included from /build/rtw89/core.h:9, > > from /build/rtw89/coex.h:8, > > from /build/rtw89/rtw8851b.c:5: > > In function 'field_multiplier', > > inlined from 'field_mask' at ./include/linux/bitfield.h:170:17, > > inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1, > > inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11: > > ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad > bitfield mask > > 165 | __bad_mask(); > > | ^~~~~~~~~~~~ > > In function 'field_multiplier', > > Hmm... > > Note that u32_encode_bits() really requires a constant mask, just > like FIELD_PREP(). So probably the compiler can no longer deduce it > is called with a constant after restructuring the code... Do you think I need to add a macro to generate mask with u32_encode_bits() and then call this inline function? Or, compiler can always deduce it in the inline function, and current version is okay? ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() 2025-11-19 8:53 ` Ping-Ke Shih @ 2025-12-12 2:09 ` Ping-Ke Shih 0 siblings, 0 replies; 11+ messages in thread From: Ping-Ke Shih @ 2025-12-12 2:09 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org Hi Geert, Ping-Ke Shih wrote: > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ping-Ke, > > > > On Wed, 19 Nov 2025 at 02:15, Ping-Ke Shih <pkshih@realtek.com> wrote: > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote: > > > > > The power value and enable bit fields can be not consecutive mask, but > > > > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is > > > > > consecutive bit mask. Therefore, change the code accordingly. > > > > > > > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > > > > > > > Thanks for your patch! > > > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > --- a/drivers/net/wireless/realtek/rtw89/mac.h > > > > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h > > > > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev, > > > > > return 0; > > > > > } > > > > > > > > > > +static inline > > > > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val, > > > > > + u32 mask_en, bool cond) > > > > > +{ > > > > > + u32 wrt = u32_encode_bits(val, mask); > > > > > > > > Nit: you could do without this variable... Can you change u32_encode_bits() to field_prep() you are going to add? And take this patch (v1) with your suggestion into your patchset. Then the code looks better and thing can be smooth. I will drop my v2 that looks not good. > > > > > > > > > + u32 val32; > > > > > + int ret; > > > > > + > > > > > + if (cond) > > > > > + wrt |= mask_en; > > > > > + > > > > > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32); > > > > > + if (ret) > > > > > + return; > > > > > + > > > > > + val32 &= ~(mask | mask_en); > > > > > + val32 |= wrt; > > > > > > > > val32 |= u32_encode_bits(val, mask); > > > > if (cond) > > > > cal32 |= mask_en; > > > > > > With this change, ARCH arm is failed to build (x86 is well): > > > > > > In file included from /build/rtw89/core.h:9, > > > from /build/rtw89/coex.h:8, > > > from /build/rtw89/rtw8851b.c:5: > > > In function 'field_multiplier', > > > inlined from 'field_mask' at ./include/linux/bitfield.h:170:17, > > > inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1, > > > inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11: > > > ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad > > bitfield mask > > > 165 | __bad_mask(); > > > | ^~~~~~~~~~~~ > > > In function 'field_multiplier', > > > > Hmm... > > > > Note that u32_encode_bits() really requires a constant mask, just > > like FIELD_PREP(). So probably the compiler can no longer deduce it > > is called with a constant after restructuring the code... > > Do you think I need to add a macro to generate mask with u32_encode_bits() > and then call this inline function? > > Or, compiler can always deduce it in the inline function, and current version > is okay? > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-12-12 2:09 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-17 3:29 [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep() Ping-Ke Shih 2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih 2025-11-18 10:24 ` Geert Uytterhoeven 2025-11-19 0:44 ` Ping-Ke Shih 2025-11-19 8:35 ` Geert Uytterhoeven 2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih 2025-11-18 10:40 ` Geert Uytterhoeven 2025-11-19 1:15 ` Ping-Ke Shih 2025-11-19 8:38 ` Geert Uytterhoeven 2025-11-19 8:53 ` Ping-Ke Shih 2025-12-12 2:09 ` Ping-Ke Shih
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.