RE: [PATCH 26/40] rtw88: 8723d: add IQ calibration

[Tony Chuang <yhchuang@realtek.com>]

Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes:

> On 2020-04-17 15:46:39 [+0800], yhchuang@realtek.com wrote:
> > diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723d.c
> b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
> > index 94784c7f0743..b66bd969e007 100644
> > --- a/drivers/net/wireless/realtek/rtw88/rtw8723d.c
> > +++ b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
> …
> > +struct iqk_backup_regs {
> > +	u32 adda[IQK_ADDA_REG_NUM];
> > +	u8 mac8[IQK_MAC8_REG_NUM];
> > +	u32 mac32[IQK_MAC32_REG_NUM];
> > +	u32 bb[IQK_BB_REG_NUM];
> > +
> > +	u32 lte_path;
> > +	u32 lte_gnt;
> > +
> > +	u8 btg_sel;
> > +	u32 bb_sel_btg;
> > +
> > +	u8 igia;
> > +	u8 igib;
> 
> The struct has 128 bytes. Putting btg_sel after bb_sel_btg will result
> in 124 bytes. How likely is it that it will grow? I'm asking because it
> is allocated on stack.

We need to backup a lot of the register values for doing IQK.
I think it's inevitable, just about where should we put them.
And as there's only 8723D is using SW IQK, this struct will only be
used by 8723D, so add them into rtwdev is not suitable.

Another way is that we can kmalloc() and then kfree() it after
IQK is done.

> 
> > +};
> > +
> > +static void rtw8723d_iqk_backup_regs(struct rtw_dev *rtwdev,
> > +				     struct iqk_backup_regs *backup)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < IQK_ADDA_REG_NUM; i++)
> > +		backup->adda[i] = rtw_read32(rtwdev, iqk_adda_regs[i]);
> > +
> > +	for (i = 0; i < IQK_MAC8_REG_NUM; i++)
> > +		backup->mac8[i] = rtw_read8(rtwdev, iqk_mac8_regs[i]);
> > +	for (i = 0; i < IQK_MAC32_REG_NUM; i++)
> > +		backup->mac32[i] = rtw_read32(rtwdev, iqk_mac32_regs[i]);
> > +
> > +	for (i = 0; i < IQK_BB_REG_NUM; i++)
> > +		backup->bb[i] = rtw_read32(rtwdev, iqk_bb_regs[i]);
> > +
> > +	backup->igia = (u8)rtw_read32_mask(rtwdev, REG_OFDM0_XAAGC1,
> MASKBYTE0);
> > +	backup->igib = (u8)rtw_read32_mask(rtwdev, REG_OFDM0_XBAGC1,
> MASKBYTE0);
> 
> igi[ab] is alreay u8, no need for cast.

It's because rtw_read32_mask() returns u32, but because we
mask with one byte only.

> 
> > +
> > +	backup->bb_sel_btg = rtw_read32(rtwdev, REG_BB_SEL_BTG);
> > +}
> …
> 
> > +static u8 rtw8723d_iqk_rx_path(struct rtw_dev *rtwdev,
> > +			       const struct rtw_8723d_iqk_cfg *iqk_cfg,
> > +			       const struct iqk_backup_regs *backup)
> > +{
> > +	u32 tx_x, tx_y;
> > +	u8 result = 0x00;
> 
> You could avoid the explicit init of `result' (maybe even use `ret' for
> less key strokes and avoiding the confusion with the `result' array used
> by the other functions here) and then

The result should be inited to zero here, because the value
of it is or-ed by the IQK status, such as:

	result |= rtw8723d_iqk_check_tx_failed(rtwdev, iqk_cfg);

And yes, the name is a little confused to be the same.
Should use different name for them.

> 
> …
> > +	rtw8723d_iqk_one_shot(rtwdev, false, iqk_cfg);
> > +	result |= rtw8723d_iqk_check_tx_failed(rtwdev, iqk_cfg);
> 
> not or the returned value here. Since you don't collect it from multiple
> functions I don't see the reason for it.

It actually does collect them from two functions, they are the
same, but are done twice, hence using |= here.

> 
> > +	if (!result)
> > +		goto restore;
> …
> > +	rtw8723d_iqk_one_shot(rtwdev, false, iqk_cfg);
> > +	result |= rtw8723d_iqk_check_rx_failed(rtwdev, iqk_cfg);
> 
> Same here.
> 
> > +restore:
> > +	rtw8723d_iqk_txrx_path_post(rtwdev, iqk_cfg, backup);
> > +
> > +	return result;
> > +}
> > +
> …
> > +
> > +static void rtw8723d_phy_calibration(struct rtw_dev *rtwdev)
> > +{
> > +	struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> > +	s32 result[IQK_ROUND_SIZE][IQK_NR];
> > +	struct iqk_backup_regs backup;
> 
> I don't know how deep you are in the call chain, but `result' takes 128
> bytes and `backup' as well (this could be 124).
> I'm not saying that this is bad, just that you keep an eye on it since
> those two take 256 bytes.

I can try to fix it, to see if we can reduce that.

> 
> > +	u8 i, j;
> > +	u8 final_candidate = IQK_ROUND_INVALID;
> > +	bool good;
> > +
> > +	rtw_dbg(rtwdev, RTW_DBG_RFK, "[IQK] Start!!!\n");
> > +
> > +	memset(result, 0, sizeof(result));
> 
> Sebastian
> 

Yen-Hsuan