mbox series

[v2,0/9] ath9k: add support for QCN550x

Message ID 20220512195319.14635-1-wlooi@ucalgary.ca
Headers show
Series ath9k: add support for QCN550x | expand

Message

Wenli Looi May 12, 2022, 7:53 p.m. UTC
This patchset adds support for QCN550x. It is an attempt to write a
cleaner version of the following patch:

https://github.com/openwrt/openwrt/pull/9389

I'm definitely open to ideas on how to better implement this change.

Notes:

- While QCN5502 is a 4-stream device, ath9k only supports 3 streams, and
  that is unchanged.
- The QCN550x EEPROM format is different from other AR9003 devices due
  to the 4th stream. An abstraction layer has been added over the EEPROM
  to support both formats.
- The QCN550x initvals here are a hacked version of the QCA956x
  initvals, and have only been partially tested.
- This change has been tested on the following devices, which appear to
  work as an access point:
  - QCN5502: Netgear EX6400v2
  - QCA9558: Netgear EX7300v1

Relevant links:

- QCN550x SoC support, which appears to be identical to QCA956x:
  https://github.com/torvalds/linux/commit/a0b8cd5c223042efe764864b8dd9396ee127c763
- QSDK patch for QCN550x:
  https://source.codeaurora.org/quic/qsdk/oss/system/openwrt/tree/target/linux/ar71xx/patches-3.3/9991-MIPS-ath79-add-SoC-detection-code-for-QCN550X.patch?h=aa/banana

v2:
 1. fix empty commit logs and missing s-o-b

Wenli Looi (9):
  ath9k: add QCN550x device IDs
  ath9k: basic support for QCN550x
  ath9k: add QCN550x initvals
  ath9k: implement QCN550x rx
  ath9k: implement QCN550x tx
  ath9k: group some ar9300 eeprom functions at the top
  ath9k: add abstractions over ar9300 eeprom
  ath9k: rename ar9300_eeprom to ar9300_eeprom_v1
  ath9k: add ar9300_eeprom_v2

 drivers/net/wireless/ath/ath9k/ahb.c          |    4 +
 drivers/net/wireless/ath/ath9k/ani.c          |    2 +-
 .../net/wireless/ath/ath9k/ar550x_initvals.h  | 1296 +++++++++++++++++
 drivers/net/wireless/ath/ath9k/ar9003_calib.c |    4 +-
 .../net/wireless/ath/ath9k/ar9003_eeprom.c    |  949 ++++++++----
 .../net/wireless/ath/ath9k/ar9003_eeprom.h    |  144 +-
 drivers/net/wireless/ath/ath9k/ar9003_hw.c    |   59 +
 drivers/net/wireless/ath/ath9k/ar9003_mac.c   |   79 +-
 drivers/net/wireless/ath/ath9k/ar9003_mac.h   |   10 +
 drivers/net/wireless/ath/ath9k/ar9003_paprd.c |    4 +-
 drivers/net/wireless/ath/ath9k/ar9003_phy.c   |   35 +-
 drivers/net/wireless/ath/ath9k/ar9003_phy.h   |   83 +-
 drivers/net/wireless/ath/ath9k/hw.c           |   38 +-
 drivers/net/wireless/ath/ath9k/hw.h           |    4 +-
 drivers/net/wireless/ath/ath9k/mac.c          |    2 +-
 drivers/net/wireless/ath/ath9k/mac.h          |   12 +
 drivers/net/wireless/ath/ath9k/recv.c         |    2 +-
 drivers/net/wireless/ath/ath9k/reg.h          |   11 +-
 drivers/net/wireless/ath/ath9k/reg_aic.h      |    6 +-
 19 files changed, 2330 insertions(+), 414 deletions(-)
 create mode 100644 drivers/net/wireless/ath/ath9k/ar550x_initvals.h

Comments

Jeff Johnson May 12, 2022, 9:45 p.m. UTC | #1
On 5/12/2022 12:53 PM, Wenli Looi wrote:
> QCN550x is very similar to QCA956x. Note that AR_CH0_XTAL is
> intentionally unchanged. Certain arrays are no longer static because
> they are no longer constant.

I don't understand the last sentence. You removed the 'static' keyword 
but left the 'const' keyword, hence they are still constant.

And I didn't actually see any instances where the arrays are being modified.

Can you highlight which are being modified?

> 
> Signed-off-by: Wenli Looi <wlooi@ucalgary.ca>
> ---
>   drivers/net/wireless/ath/ath9k/ani.c          |  2 +-
>   drivers/net/wireless/ath/ath9k/ar9003_calib.c |  4 +-
>   .../net/wireless/ath/ath9k/ar9003_eeprom.c    | 17 ++--
>   drivers/net/wireless/ath/ath9k/ar9003_paprd.c |  4 +-
>   drivers/net/wireless/ath/ath9k/ar9003_phy.c   | 35 ++++----
>   drivers/net/wireless/ath/ath9k/ar9003_phy.h   | 83 ++++++++++++-------
>   drivers/net/wireless/ath/ath9k/hw.c           | 28 ++++---
>   drivers/net/wireless/ath/ath9k/mac.c          |  2 +-
>   drivers/net/wireless/ath/ath9k/recv.c         |  2 +-
>   drivers/net/wireless/ath/ath9k/reg.h          |  4 +-
>   drivers/net/wireless/ath/ath9k/reg_aic.h      |  6 +-
>   11 files changed, 112 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/ani.c b/drivers/net/wireless/ath/ath9k/ani.c
> index 41d192709..281103865 100644
> --- a/drivers/net/wireless/ath/ath9k/ani.c
> +++ b/drivers/net/wireless/ath/ath9k/ani.c
> @@ -261,7 +261,7 @@ static void ath9k_hw_set_cck_nil(struct ath_hw *ah, u_int8_t immunityLevel,
>   
>   	/* Skip MRC CCK for pre AR9003 families */
>   	if (!AR_SREV_9300_20_OR_LATER(ah) || AR_SREV_9485(ah) ||
> -	    AR_SREV_9565(ah) || AR_SREV_9561(ah))
> +	    AR_SREV_9565(ah) || AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   		return;
>   
>   	if (aniState->mrcCCK != entry_cck->mrc_cck_on)
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_calib.c b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> index 6ca089f15..e2cf4b733 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> @@ -201,7 +201,7 @@ static void ar9003_hw_iqcalibrate(struct ath_hw *ah, u8 numChains)
>   	u32 qCoffDenom, iCoffDenom;
>   	int32_t qCoff, iCoff;
>   	int iqCorrNeg, i;
> -	static const u_int32_t offset_array[3] = {
> +	const u_int32_t offset_array[3] = {
>   		AR_PHY_RX_IQCAL_CORR_B0,
>   		AR_PHY_RX_IQCAL_CORR_B1,
>   		AR_PHY_RX_IQCAL_CORR_B2,
> @@ -1202,7 +1202,7 @@ static void ar9003_hw_manual_peak_cal(struct ath_hw *ah, u8 chain, bool is_2g)
>   
>   	if (AR_SREV_9550(ah) || AR_SREV_9531(ah))
>   		peak_detect_threshold = 8;
> -	else if (AR_SREV_9561(ah))
> +	else if (AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   		peak_detect_threshold = 11;
>   
>   	/*
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
> index 16bfcd0a1..49be3a315 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
> @@ -3608,6 +3608,8 @@ static void ar9003_hw_xpa_bias_level_apply(struct ath_hw *ah, bool is2ghz)
>   	if (AR_SREV_9485(ah) || AR_SREV_9330(ah) || AR_SREV_9340(ah) ||
>   	    AR_SREV_9531(ah) || AR_SREV_9561(ah))
>   		REG_RMW_FIELD(ah, AR_CH0_TOP2, AR_CH0_TOP2_XPABIASLVL, bias);
> +	else if (AR_SREV_5502(ah))
> +		REG_RMW_FIELD(ah, AR_CH0_TOP3, AR_CH0_TOP3_XPABIASLVL, bias);
>   	else if (AR_SREV_9462(ah) || AR_SREV_9550(ah) || AR_SREV_9565(ah))
>   		REG_RMW_FIELD(ah, AR_CH0_TOP, AR_CH0_TOP_XPABIASLVL, bias);
>   	else {
> @@ -3648,7 +3650,7 @@ static void ar9003_hw_ant_ctrl_apply(struct ath_hw *ah, bool is2ghz)
>   	struct ath9k_hw_capabilities *pCap = &ah->caps;
>   	int chain;
>   	u32 regval, value, gpio;
> -	static const u32 switch_chain_reg[AR9300_MAX_CHAINS] = {
> +	const u32 switch_chain_reg[AR9300_MAX_CHAINS] = {
>   			AR_PHY_SWITCH_CHAIN_0,
>   			AR_PHY_SWITCH_CHAIN_1,
>   			AR_PHY_SWITCH_CHAIN_2,
> @@ -3669,7 +3671,8 @@ static void ar9003_hw_ant_ctrl_apply(struct ath_hw *ah, bool is2ghz)
>   	if (AR_SREV_9462(ah) || AR_SREV_9565(ah)) {
>   		REG_RMW_FIELD(ah, AR_PHY_SWITCH_COM,
>   				AR_SWITCH_TABLE_COM_AR9462_ALL, value);
> -	} else if (AR_SREV_9550(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah)) {
> +	} else if (AR_SREV_9550(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah) ||
> +			   AR_SREV_5502(ah)) {
>   		REG_RMW_FIELD(ah, AR_PHY_SWITCH_COM,
>   				AR_SWITCH_TABLE_COM_AR9550_ALL, value);
>   	} else
> @@ -4000,11 +4003,11 @@ void ar9003_hw_internal_regulator_apply(struct ath_hw *ah)
>   			if (!is_pmu_set(ah, AR_PHY_PMU2, reg_pmu_set))
>   				return;
>   		} else if (AR_SREV_9462(ah) || AR_SREV_9565(ah) ||
> -			   AR_SREV_9561(ah)) {
> +			   AR_SREV_9561(ah) || AR_SREV_5502(ah)) {
>   			reg_val = le32_to_cpu(pBase->swreg);
>   			REG_WRITE(ah, AR_PHY_PMU1, reg_val);
>   
> -			if (AR_SREV_9561(ah))
> +			if (AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   				REG_WRITE(ah, AR_PHY_PMU2, 0x10200000);
>   		} else {
>   			/* Internal regulator is ON. Write swreg register. */
> @@ -4109,7 +4112,8 @@ static void ar9003_hw_xpa_timing_control_apply(struct ath_hw *ah, bool is2ghz)
>   	    !AR_SREV_9340(ah) &&
>   	    !AR_SREV_9580(ah) &&
>   	    !AR_SREV_9531(ah) &&
> -	    !AR_SREV_9561(ah))
> +	    !AR_SREV_9561(ah) &&
> +	    !AR_SREV_5502(ah))
>   		return;
>   
>   	xpa_ctl = ar9003_modal_header(ah, is2ghz)->txFrameToXpaOn;
> @@ -4889,7 +4893,8 @@ static void ar9003_hw_power_control_override(struct ath_hw *ah,
>   	}
>   
>   tempslope:
> -	if (AR_SREV_9550(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah)) {
> +	if (AR_SREV_9550(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah) ||
> +	    AR_SREV_5502(ah)) {
>   		u8 txmask = (eep->baseEepHeader.txrxMask & 0xf0) >> 4;
>   
>   		/*
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_paprd.c b/drivers/net/wireless/ath/ath9k/ar9003_paprd.c
> index b2d53b6c0..ff45abd14 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_paprd.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_paprd.c
> @@ -138,12 +138,12 @@ static int ar9003_get_training_power_5g(struct ath_hw *ah)
>   static int ar9003_paprd_setup_single_table(struct ath_hw *ah)
>   {
>   	struct ath_common *common = ath9k_hw_common(ah);
> -	static const u32 ctrl0[3] = {
> +	const u32 ctrl0[3] = {
>   		AR_PHY_PAPRD_CTRL0_B0,
>   		AR_PHY_PAPRD_CTRL0_B1,
>   		AR_PHY_PAPRD_CTRL0_B2
>   	};
> -	static const u32 ctrl1[3] = {
> +	const u32 ctrl1[3] = {
>   		AR_PHY_PAPRD_CTRL1_B0,
>   		AR_PHY_PAPRD_CTRL1_B1,
>   		AR_PHY_PAPRD_CTRL1_B2
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_phy.c b/drivers/net/wireless/ath/ath9k/ar9003_phy.c
> index dc0e5ea25..7cbe30bc4 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_phy.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_phy.c
> @@ -157,9 +157,9 @@ static int ar9003_hw_set_channel(struct ath_hw *ah, struct ath9k_channel *chan)
>   	freq = centers.synth_center;
>   
>   	if (freq < 4800) {     /* 2 GHz, fractional mode */
> -		if (AR_SREV_9330(ah) || AR_SREV_9485(ah) ||
> -		    AR_SREV_9531(ah) || AR_SREV_9550(ah) ||
> -		    AR_SREV_9561(ah) || AR_SREV_9565(ah)) {
> +		if (AR_SREV_9330(ah) || AR_SREV_9485(ah) || AR_SREV_9531(ah) ||
> +		    AR_SREV_9550(ah) || AR_SREV_9561(ah) || AR_SREV_5502(ah) ||
> +		    AR_SREV_9565(ah)) {
>   			if (ah->is_clk_25mhz)
>   				div = 75;
>   			else
> @@ -182,8 +182,8 @@ static int ar9003_hw_set_channel(struct ath_hw *ah, struct ath9k_channel *chan)
>   		/* Set to 2G mode */
>   		bMode = 1;
>   	} else {
> -		if ((AR_SREV_9340(ah) || AR_SREV_9550(ah) ||
> -		     AR_SREV_9531(ah) || AR_SREV_9561(ah)) &&
> +		if ((AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
> +		     AR_SREV_9561(ah) || AR_SREV_5502(ah)) &&
>   		    ah->is_clk_25mhz) {
>   			channelSel = freq / 75;
>   			chan_frac = ((freq % 75) * 0x20000) / 75;
> @@ -250,7 +250,7 @@ static void ar9003_hw_spur_mitigate_mrc_cck(struct ath_hw *ah,
>   	 */
>   
>   	if (AR_SREV_9485(ah) || AR_SREV_9340(ah) || AR_SREV_9330(ah) ||
> -	    AR_SREV_9550(ah) || AR_SREV_9561(ah)) {
> +	    AR_SREV_9550(ah) || AR_SREV_9561(ah) || AR_SREV_5502(ah)) {
>   		if (spur_fbin_ptr[0] == 0) /* No spur */
>   			return;
>   		max_spur_cnts = 5;
> @@ -277,7 +277,7 @@ static void ar9003_hw_spur_mitigate_mrc_cck(struct ath_hw *ah,
>   
>   		negative = 0;
>   		if (AR_SREV_9485(ah) || AR_SREV_9340(ah) || AR_SREV_9330(ah) ||
> -		    AR_SREV_9550(ah) || AR_SREV_9561(ah))
> +		    AR_SREV_9550(ah) || AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   			cur_bb_spur = ath9k_hw_fbin2freq(spur_fbin_ptr[i],
>   							 IS_CHAN_2GHZ(chan));
>   		else
> @@ -619,7 +619,7 @@ static void ar9003_hw_set_channel_regs(struct ath_hw *ah,
>   	/* Enable 11n HT, 20 MHz */
>   	phymode = AR_PHY_GC_HT_EN | AR_PHY_GC_SHORT_GI_40 | enableDacFifo;
>   
> -	if (!AR_SREV_9561(ah))
> +	if (!(AR_SREV_9561(ah) || AR_SREV_5502(ah)))
>   		phymode |= AR_PHY_GC_SINGLE_HT_LTF1;
>   
>   	/* Configure baseband for dynamic 20/40 operation */
> @@ -724,7 +724,7 @@ static void ar9003_hw_override_ini(struct ath_hw *ah)
>   		ah->enabled_cals &= ~TX_CL_CAL;
>   
>   	if (AR_SREV_9340(ah) || AR_SREV_9531(ah) || AR_SREV_9550(ah) ||
> -	    AR_SREV_9561(ah)) {
> +	    AR_SREV_9561(ah) || AR_SREV_5502(ah)) {
>   		if (ah->is_clk_25mhz) {
>   			REG_WRITE(ah, AR_RTC_DERIVED_CLK, 0x17c << 1);
>   			REG_WRITE(ah, AR_SLP32_MODE, 0x0010f3d7);
> @@ -905,23 +905,25 @@ static int ar9003_hw_process_ini(struct ath_hw *ah,
>   		}
>   	}
>   
> -	if (AR_SREV_9550(ah) || AR_SREV_9561(ah))
> +	if (AR_SREV_9550(ah) || AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   		REG_WRITE_ARRAY(&ah->ini_modes_rx_gain_bounds, modesIndex,
>   				regWrites);
>   
> -	if (AR_SREV_9561(ah) && (ar9003_hw_get_rx_gain_idx(ah) == 0))
> +	if ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) &&
> +	    (ar9003_hw_get_rx_gain_idx(ah) == 0))
>   		REG_WRITE_ARRAY(&ah->ini_modes_rxgain_xlna,
>   				modesIndex, regWrites);
>   	/*
>   	 * TXGAIN initvals.
>   	 */
> -	if (AR_SREV_9550(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah)) {
> +	if (AR_SREV_9550(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah) ||
> +	    AR_SREV_5502(ah)) {
>   		int modes_txgain_index = 1;
>   
>   		if (AR_SREV_9550(ah))
>   			modes_txgain_index = ar9550_hw_get_modes_txgain_index(ah, chan);
>   
> -		if (AR_SREV_9561(ah))
> +		if (AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   			modes_txgain_index =
>   				ar9561_hw_get_modes_txgain_index(ah, chan);
>   
> @@ -1922,7 +1924,7 @@ void ar9003_hw_attach_phy_ops(struct ath_hw *ah)
>   {
>   	struct ath_hw_private_ops *priv_ops = ath9k_hw_private_ops(ah);
>   	struct ath_hw_ops *ops = ath9k_hw_ops(ah);
> -	static const u32 ar9300_cca_regs[6] = {
> +	const u32 ar9300_cca_regs[6] = {
>   		AR_PHY_CCA_0,
>   		AR_PHY_CCA_1,
>   		AR_PHY_CCA_2,
> @@ -1935,7 +1937,7 @@ void ar9003_hw_attach_phy_ops(struct ath_hw *ah)
>   	priv_ops->spur_mitigate_freq = ar9003_hw_spur_mitigate;
>   
>   	if (AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
> -	    AR_SREV_9561(ah))
> +	    AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   		priv_ops->compute_pll_control = ar9003_hw_compute_pll_control_soc;
>   	else
>   		priv_ops->compute_pll_control = ar9003_hw_compute_pll_control;
> @@ -2021,7 +2023,8 @@ bool ar9003_hw_bb_watchdog_check(struct ath_hw *ah)
>   	case 0x04000b09:
>   		return true;
>   	case 0x04000409:
> -		if (AR_SREV_9340(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah))
> +		if (AR_SREV_9340(ah) || AR_SREV_9531(ah) || AR_SREV_9561(ah) ||
> +		    AR_SREV_5502(ah))
>   			return false;
>   		else
>   			return true;
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_phy.h b/drivers/net/wireless/ath/ath9k/ar9003_phy.h
> index ad949eb02..2557de36d 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_phy.h
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_phy.h
> @@ -20,7 +20,7 @@
>   /*
>    * Channel Register Map
>    */
> -#define AR_CHAN_BASE	0x9800
> +#define AR_CHAN_BASE	(AR_SREV_5502(ah) ? 0x29800 : 0x9800)

this violates the coding style:
<https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl>

Things to avoid when using macros:
macros that depend on having a local variable with a magic name

So you should add ah as a parameter to the macro

Repeat for all instances below where ah is being used

>   
>   #define AR_PHY_TIMING1      (AR_CHAN_BASE + 0x0)
>   #define AR_PHY_TIMING2      (AR_CHAN_BASE + 0x4)
> @@ -214,7 +214,7 @@
>   /*
>    * MRC Register Map
>    */
> -#define AR_MRC_BASE	0x9c00
> +#define AR_MRC_BASE	(AR_SREV_5502(ah) ? 0x29c00 : 0x9c00)
>   
>   #define AR_PHY_TIMING_3A       (AR_MRC_BASE + 0x0)
>   #define AR_PHY_LDPC_CNTL1      (AR_MRC_BASE + 0x4)
> @@ -255,12 +255,12 @@
>   /*
>    * BBB Register Map
>    */
> -#define AR_BBB_BASE	0x9d00
> +#define AR_BBB_BASE	(AR_SREV_5502(ah) ? 0x29d00 : 0x9d00)
>   
>   /*
>    * AGC Register Map
>    */
> -#define AR_AGC_BASE	0x9e00
> +#define AR_AGC_BASE	(AR_SREV_5502(ah) ? 0x29e00 : 0x9e00)
>   
>   #define AR_PHY_SETTLING         (AR_AGC_BASE + 0x0)
>   #define AR_PHY_FORCEMAX_GAINS_0 (AR_AGC_BASE + 0x4)
> @@ -448,14 +448,16 @@
>   /*
>    * SM Register Map
>    */
> -#define AR_SM_BASE	0xa200
> +#define AR_SM_BASE	(AR_SREV_5502(ah) ? 0x2a200 : 0xa200)
>   
>   #define AR_PHY_D2_CHIP_ID        (AR_SM_BASE + 0x0)
>   #define AR_PHY_GEN_CTRL          (AR_SM_BASE + 0x4)
>   #define AR_PHY_MODE              (AR_SM_BASE + 0x8)
>   #define AR_PHY_ACTIVE            (AR_SM_BASE + 0xc)
> -#define AR_PHY_SPUR_MASK_A       (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x18 : 0x20))
> -#define AR_PHY_SPUR_MASK_B       (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x1c : 0x24))
> +#define AR_PHY_SPUR_MASK_A                                                     \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x18 : 0x20))
> +#define AR_PHY_SPUR_MASK_B                                                     \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x1c : 0x24))
>   #define AR_PHY_SPECTRAL_SCAN     (AR_SM_BASE + 0x28)
>   #define AR_PHY_RADAR_BW_FILTER   (AR_SM_BASE + 0x2c)
>   #define AR_PHY_SEARCH_START_DELAY (AR_SM_BASE + 0x30)
> @@ -498,7 +500,8 @@
>   #define AR_PHY_SPUR_MASK_A_CF_PUNC_MASK_A                       0x3FF
>   #define AR_PHY_SPUR_MASK_A_CF_PUNC_MASK_A_S                     0
>   
> -#define AR_PHY_TEST              (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x15c : 0x160))
> +#define AR_PHY_TEST                                                            \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x15c : 0x160))
>   
>   #define AR_PHY_TEST_BBB_OBS_SEL       0x780000
>   #define AR_PHY_TEST_BBB_OBS_SEL_S     19
> @@ -509,7 +512,8 @@
>   #define AR_PHY_TEST_CHAIN_SEL      0xC0000000
>   #define AR_PHY_TEST_CHAIN_SEL_S    30
>   
> -#define AR_PHY_TEST_CTL_STATUS   (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x160 : 0x164))
> +#define AR_PHY_TEST_CTL_STATUS                                                 \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x160 : 0x164))
>   #define AR_PHY_TEST_CTL_TSTDAC_EN         0x1
>   #define AR_PHY_TEST_CTL_TSTDAC_EN_S       0
>   #define AR_PHY_TEST_CTL_TX_OBS_SEL        0x1C
> @@ -523,23 +527,32 @@
>   #define AR_PHY_TEST_CTL_DEBUGPORT_SEL	  0xe0000000
>   #define AR_PHY_TEST_CTL_DEBUGPORT_SEL_S	  29
>   
> +#define AR_PHY_TSTDAC                                                          \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x164 : 0x168))
>   
> -#define AR_PHY_TSTDAC            (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x164 : 0x168))
> +#define AR_PHY_CHAN_STATUS                                                     \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x168 : 0x16c))
>   
> -#define AR_PHY_CHAN_STATUS       (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x168 : 0x16c))
> -
> -#define AR_PHY_CHAN_INFO_MEMORY (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x16c : 0x170))
> +#define AR_PHY_CHAN_INFO_MEMORY                                                \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x16c : 0x170))
>   #define AR_PHY_CHAN_INFO_MEMORY_CHANINFOMEM_S2_READ	0x00000008
>   #define AR_PHY_CHAN_INFO_MEMORY_CHANINFOMEM_S2_READ_S	3
>   
> -#define AR_PHY_CHNINFO_NOISEPWR  (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x170 : 0x174))
> -#define AR_PHY_CHNINFO_GAINDIFF  (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x174 : 0x178))
> -#define AR_PHY_CHNINFO_FINETIM   (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x178 : 0x17c))
> -#define AR_PHY_CHAN_INFO_GAIN_0  (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x17c : 0x180))
> -#define AR_PHY_SCRAMBLER_SEED    (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x184 : 0x190))
> -#define AR_PHY_CCK_TX_CTRL       (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x188 : 0x194))
> -
> -#define AR_PHY_HEAVYCLIP_CTL     (AR_SM_BASE + (AR_SREV_9561(ah) ? 0x198 : 0x1a4))
> +#define AR_PHY_CHNINFO_NOISEPWR                                                \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x170 : 0x174))
> +#define AR_PHY_CHNINFO_GAINDIFF                                                \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x174 : 0x178))
> +#define AR_PHY_CHNINFO_FINETIM                                                 \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x178 : 0x17c))
> +#define AR_PHY_CHAN_INFO_GAIN_0                                                \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x17c : 0x180))
> +#define AR_PHY_SCRAMBLER_SEED                                                  \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x184 : 0x190))
> +#define AR_PHY_CCK_TX_CTRL                                                     \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x188 : 0x194))
> +
> +#define AR_PHY_HEAVYCLIP_CTL                                                   \
> +	(AR_SM_BASE + ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x198 : 0x1a4))
>   #define AR_PHY_HEAVYCLIP_20      (AR_SM_BASE + 0x1a8)
>   #define AR_PHY_HEAVYCLIP_40      (AR_SM_BASE + 0x1ac)
>   #define AR_PHY_HEAVYCLIP_1	 (AR_SM_BASE + 0x19c)
> @@ -722,6 +735,10 @@
>   #define AR_CH0_TOP2_XPABIASLVL		(AR_SREV_9561(ah) ? 0x1e00 : 0xf000)
>   #define AR_CH0_TOP2_XPABIASLVL_S	(AR_SREV_9561(ah) ? 9 : 12)
>   
> +#define AR_CH0_TOP3		0x16288
> +#define AR_CH0_TOP3_XPABIASLVL		0xf0000
> +#define AR_CH0_TOP3_XPABIASLVL_S	16
> +
>   #define AR_CH0_XTAL		(AR_SREV_9300(ah) ? 0x16294 : \
>   				 ((AR_SREV_9462(ah) || AR_SREV_9565(ah)) ? 0x16298 : \
>   				  (AR_SREV_9561(ah) ? 0x162c0 : 0x16290)))
> @@ -730,13 +747,17 @@
>   #define AR_CH0_XTAL_CAPOUTDAC	0x00fe0000
>   #define AR_CH0_XTAL_CAPOUTDAC_S	17
>   
> -#define AR_PHY_PMU1		((AR_SREV_9462(ah) || AR_SREV_9565(ah)) ? 0x16340 : \
> -				 (AR_SREV_9561(ah) ? 0x16cc0 : 0x16c40))
> +#define AR_PHY_PMU1                                                            \
> +	((AR_SREV_9462(ah) || AR_SREV_9565(ah)) ?                              \
> +		 0x16340 :                                                     \
> +		       ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x16cc0 : 0x16c40))
>   #define AR_PHY_PMU1_PWD		0x1
>   #define AR_PHY_PMU1_PWD_S	0
>   
> -#define AR_PHY_PMU2		((AR_SREV_9462(ah) || AR_SREV_9565(ah)) ? 0x16344 : \
> -				 (AR_SREV_9561(ah) ? 0x16cc4 : 0x16c44))
> +#define AR_PHY_PMU2                                                            \
> +	((AR_SREV_9462(ah) || AR_SREV_9565(ah)) ?                              \
> +		 0x16344 :                                                     \
> +		       ((AR_SREV_9561(ah) || AR_SREV_5502(ah)) ? 0x16cc4 : 0x16c44))
>   #define AR_PHY_PMU2_PGM		0x00200000
>   #define AR_PHY_PMU2_PGM_S	21
>   
> @@ -925,7 +946,7 @@
>   /*
>    * Channel 1 Register Map
>    */
> -#define AR_CHAN1_BASE	0xa800
> +#define AR_CHAN1_BASE	(AR_SREV_5502(ah) ? 0x2a800 : 0xa800)
>   
>   #define AR_PHY_EXT_CCA_1            (AR_CHAN1_BASE + 0x30)
>   #define AR_PHY_TX_PHASE_RAMP_1      (AR_CHAN1_BASE + 0xd0)
> @@ -944,7 +965,7 @@
>   /*
>    * AGC 1 Register Map
>    */
> -#define AR_AGC1_BASE	0xae00
> +#define AR_AGC1_BASE	(AR_SREV_5502(ah) ? 0x2ae00 : 0xae00)
>   
>   #define AR_PHY_FORCEMAX_GAINS_1      (AR_AGC1_BASE + 0x4)
>   #define AR_PHY_EXT_ATTEN_CTL_1       (AR_AGC1_BASE + 0x18)
> @@ -963,7 +984,7 @@
>   /*
>    * SM 1 Register Map
>    */
> -#define AR_SM1_BASE	0xb200
> +#define AR_SM1_BASE	(AR_SREV_5502(ah) ? 0x2b200 : 0xb200)
>   
>   #define AR_PHY_SWITCH_CHAIN_1   (AR_SM1_BASE + 0x84)
>   #define AR_PHY_FCAL_2_1         (AR_SM1_BASE + 0xd0)
> @@ -989,7 +1010,7 @@
>   /*
>    * Channel 2 Register Map
>    */
> -#define AR_CHAN2_BASE	0xb800
> +#define AR_CHAN2_BASE	(AR_SREV_5502(ah) ? 0x2b800 : 0xb800)
>   
>   #define AR_PHY_EXT_CCA_2            (AR_CHAN2_BASE + 0x30)
>   #define AR_PHY_TX_PHASE_RAMP_2      (AR_CHAN2_BASE + 0xd0)
> @@ -1007,7 +1028,7 @@
>   /*
>    * AGC 2 Register Map
>    */
> -#define AR_AGC2_BASE	0xbe00
> +#define AR_AGC2_BASE	(AR_SREV_5502(ah) ? 0x2be00 : 0xbe00)
>   
>   #define AR_PHY_FORCEMAX_GAINS_2      (AR_AGC2_BASE + 0x4)
>   #define AR_PHY_EXT_ATTEN_CTL_2       (AR_AGC2_BASE + 0x18)
> @@ -1024,7 +1045,7 @@
>   /*
>    * SM 2 Register Map
>    */
> -#define AR_SM2_BASE	0xc200
> +#define AR_SM2_BASE	(AR_SREV_5502(ah) ? 0x2c200 : 0xc200)
>   
>   #define AR_PHY_SWITCH_CHAIN_2    (AR_SM2_BASE + 0x84)
>   #define AR_PHY_FCAL_2_2          (AR_SM2_BASE + 0xd0)
> diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
> index 047512ce1..c32b201a3 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.c
> +++ b/drivers/net/wireless/ath/ath9k/hw.c
> @@ -834,7 +834,7 @@ static void ath9k_hw_init_pll(struct ath_hw *ah,
>   		REG_RMW_FIELD(ah, AR_CH0_BB_DPLL3,
>   			      AR_CH0_BB_DPLL3_PHASE_SHIFT, 0x1);
>   	} else if (AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
> -		   AR_SREV_9561(ah)) {
> +		   AR_SREV_9561(ah) || AR_SREV_5502(ah)) {
>   		u32 regval, pll2_divint, pll2_divfrac, refdiv;
>   
>   		REG_WRITE(ah, AR_RTC_PLL_CONTROL,
> @@ -845,7 +845,8 @@ static void ath9k_hw_init_pll(struct ath_hw *ah,
>   		udelay(100);
>   
>   		if (ah->is_clk_25mhz) {
> -			if (AR_SREV_9531(ah) || AR_SREV_9561(ah)) {
> +			if (AR_SREV_9531(ah) || AR_SREV_9561(ah) ||
> +			    AR_SREV_5502(ah)) {
>   				pll2_divint = 0x1c;
>   				pll2_divfrac = 0xa3d2;
>   				refdiv = 1;
> @@ -861,15 +862,16 @@ static void ath9k_hw_init_pll(struct ath_hw *ah,
>   				refdiv = 5;
>   			} else {
>   				pll2_divint = 0x11;
> -				pll2_divfrac = (AR_SREV_9531(ah) ||
> -						AR_SREV_9561(ah)) ?
> +				pll2_divfrac =
> +					(AR_SREV_9531(ah) || AR_SREV_9561(ah) ||
> +					 AR_SREV_5502(ah)) ?
>   						0x26665 : 0x26666;
>   				refdiv = 1;
>   			}
>   		}
>   
>   		regval = REG_READ(ah, AR_PHY_PLL_MODE);
> -		if (AR_SREV_9531(ah) || AR_SREV_9561(ah))
> +		if (AR_SREV_9531(ah) || AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   			regval |= (0x1 << 22);
>   		else
>   			regval |= (0x1 << 16);
> @@ -887,7 +889,8 @@ static void ath9k_hw_init_pll(struct ath_hw *ah,
>   				(0x1 << 13) |
>   				(0x4 << 26) |
>   				(0x18 << 19);
> -		else if (AR_SREV_9531(ah) || AR_SREV_9561(ah)) {
> +		else if (AR_SREV_9531(ah) || AR_SREV_9561(ah) ||
> +			 AR_SREV_5502(ah)) {
>   			regval = (regval & 0x01c00fff) |
>   				(0x1 << 31) |
>   				(0x2 << 29) |
> @@ -904,7 +907,7 @@ static void ath9k_hw_init_pll(struct ath_hw *ah,
>   				(0x60 << 19);
>   		REG_WRITE(ah, AR_PHY_PLL_MODE, regval);
>   
> -		if (AR_SREV_9531(ah) || AR_SREV_9561(ah))
> +		if (AR_SREV_9531(ah) || AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   			REG_WRITE(ah, AR_PHY_PLL_MODE,
>   				  REG_READ(ah, AR_PHY_PLL_MODE) & 0xffbfffff);
>   		else
> @@ -945,7 +948,7 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
>   	u32 msi_cfg = 0;
>   
>   	if (AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
> -	    AR_SREV_9561(ah))
> +	    AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   		sync_default &= ~AR_INTR_SYNC_HOST1_FATAL;
>   
>   	if (AR_SREV_9300_20_OR_LATER(ah)) {
> @@ -1777,7 +1780,7 @@ static void ath9k_hw_init_desc(struct ath_hw *ah)
>   #ifdef __BIG_ENDIAN
>   		else if (AR_SREV_9330(ah) || AR_SREV_9340(ah) ||
>   			 AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
> -			 AR_SREV_9561(ah))
> +			 AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   			REG_RMW(ah, AR_CFG, AR_CFG_SWRB | AR_CFG_SWTB, 0);
>   		else
>   			REG_WRITE(ah, AR_CFG, AR_CFG_SWTD | AR_CFG_SWRD);
> @@ -2474,6 +2477,9 @@ static void ath9k_gpio_cap_init(struct ath_hw *ah)
>   	} else if (AR_SREV_9561(ah)) {
>   		pCap->num_gpio_pins = AR9561_NUM_GPIO;
>   		pCap->gpio_mask = AR9561_GPIO_MASK;
> +	} else if (AR_SREV_5502(ah)) {
> +		pCap->num_gpio_pins = AR5502_NUM_GPIO;
> +		pCap->gpio_mask = AR5502_GPIO_MASK;
>   	} else if (AR_SREV_9565(ah)) {
>   		pCap->num_gpio_pins = AR9565_NUM_GPIO;
>   		pCap->gpio_mask = AR9565_GPIO_MASK;
> @@ -2610,7 +2616,7 @@ int ath9k_hw_fill_cap_info(struct ath_hw *ah)
>   	if (AR_SREV_9300_20_OR_LATER(ah)) {
>   		pCap->hw_caps |= ATH9K_HW_CAP_EDMA | ATH9K_HW_CAP_FASTCLOCK;
>   		if (!AR_SREV_9330(ah) && !AR_SREV_9485(ah) &&
> -		    !AR_SREV_9561(ah) && !AR_SREV_9565(ah))
> +		    !AR_SREV_9561(ah) && !AR_SREV_5502(ah) && !AR_SREV_9565(ah))
>   			pCap->hw_caps |= ATH9K_HW_CAP_LDPC;
>   
>   		pCap->rx_hp_qdepth = ATH9K_HW_RX_HP_QDEPTH;
> @@ -2627,7 +2633,7 @@ int ath9k_hw_fill_cap_info(struct ath_hw *ah)
>   	if (AR_SREV_9300_20_OR_LATER(ah))
>   		pCap->hw_caps |= ATH9K_HW_CAP_RAC_SUPPORTED;
>   
> -	if (AR_SREV_9561(ah))
> +	if (AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   		ah->ent_mode = 0x3BDA000;
>   	else if (AR_SREV_9300_20_OR_LATER(ah))
>   		ah->ent_mode = REG_READ(ah, AR_ENT_OTP);
> diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
> index 58d02c19b..08f91f558 100644
> --- a/drivers/net/wireless/ath/ath9k/mac.c
> +++ b/drivers/net/wireless/ath/ath9k/mac.c
> @@ -813,7 +813,7 @@ static void __ath9k_hw_enable_interrupts(struct ath_hw *ah)
>   	u32 async_mask;
>   
>   	if (AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
> -	    AR_SREV_9561(ah))
> +	    AR_SREV_9561(ah) || AR_SREV_5502(ah))
>   		sync_default &= ~AR_INTR_SYNC_HOST1_FATAL;
>   
>   	async_mask = AR_INTR_MAC_IRQ;
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index 0c0624a3b..3299aef65 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -422,7 +422,7 @@ u32 ath_calcrxfilter(struct ath_softc *sc)
>   	}
>   
>   	if (AR_SREV_9550(sc->sc_ah) || AR_SREV_9531(sc->sc_ah) ||
> -	    AR_SREV_9561(sc->sc_ah))
> +	    AR_SREV_9561(sc->sc_ah) || AR_SREV_5502(sc->sc_ah))
>   		rfilt |= ATH9K_RX_FILTER_4ADDRESS;
>   
>   	if (AR_SREV_9462(sc->sc_ah) || AR_SREV_9565(sc->sc_ah))
> diff --git a/drivers/net/wireless/ath/ath9k/reg.h b/drivers/net/wireless/ath/ath9k/reg.h
> index 5a07cf7d8..d465167ac 100644
> --- a/drivers/net/wireless/ath/ath9k/reg.h
> +++ b/drivers/net/wireless/ath/ath9k/reg.h
> @@ -1138,6 +1138,7 @@ enum {
>   #define AR9531_NUM_GPIO				 18
>   #define AR9550_NUM_GPIO				 24
>   #define AR9561_NUM_GPIO				 23
> +#define AR5502_NUM_GPIO				 20
>   #define AR9565_NUM_GPIO				 14
>   #define AR9580_NUM_GPIO				 16
>   #define AR7010_NUM_GPIO                          16
> @@ -1155,6 +1156,7 @@ enum {
>   #define AR9531_GPIO_MASK			 0x0000000F
>   #define AR9550_GPIO_MASK			 0x0000000F
>   #define AR9561_GPIO_MASK			 0x0000000F
> +#define AR5502_GPIO_MASK			 0x0000000F
>   #define AR9565_GPIO_MASK			 0x00003FFF
>   #define AR9580_GPIO_MASK			 0x0000F4FF
>   #define AR7010_GPIO_MASK			 0x0000FFFF
> @@ -2114,7 +2116,7 @@ enum {
>   						     * pcu_txsm.
>   						     */
>   
> -#define AR9300_SM_BASE				0xa200
> +#define AR9300_SM_BASE				(AR_SREV_5502(ah) ? 0x2a200 : 0xa200)
>   #define AR9002_PHY_AGC_CONTROL			0x9860
>   #define AR9003_PHY_AGC_CONTROL			AR9300_SM_BASE + 0xc4
>   #define AR_PHY_AGC_CONTROL			(AR_SREV_9300_20_OR_LATER(ah) ? AR9003_PHY_AGC_CONTROL : AR9002_PHY_AGC_CONTROL)
> diff --git a/drivers/net/wireless/ath/ath9k/reg_aic.h b/drivers/net/wireless/ath/ath9k/reg_aic.h
> index 955147ab4..abd0370e0 100644
> --- a/drivers/net/wireless/ath/ath9k/reg_aic.h
> +++ b/drivers/net/wireless/ath/ath9k/reg_aic.h
> @@ -17,9 +17,9 @@
>   #ifndef REG_AIC_H
>   #define REG_AIC_H
>   
> -#define AR_SM_BASE                              0xa200
> -#define AR_SM1_BASE                             0xb200
> -#define AR_AGC_BASE                             0x9e00
> +#define AR_SM_BASE                              (AR_SREV_5502(ah) ? 0x2a200 : 0xa200)
> +#define AR_SM1_BASE                             (AR_SREV_5502(ah) ? 0x2b200 : 0xb200)
> +#define AR_AGC_BASE                             (AR_SREV_5502(ah) ? 0x29e00 : 0x9e00)
>   
>   #define AR_PHY_AIC_CTRL_0_B0                    (AR_SM_BASE + 0x4b0)
>   #define AR_PHY_AIC_CTRL_1_B0                    (AR_SM_BASE + 0x4b4)
Wenli Looi May 18, 2022, 6:08 a.m. UTC | #2
On Thu, May 12, 2022 at 2:45 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>
> On 5/12/2022 12:53 PM, Wenli Looi wrote:
> > QCN550x is very similar to QCA956x. Note that AR_CH0_XTAL is
> > intentionally unchanged. Certain arrays are no longer static because
> > they are no longer constant.
>
> I don't understand the last sentence. You removed the 'static' keyword
> but left the 'const' keyword, hence they are still constant.
>
> And I didn't actually see any instances where the arrays are being modified.
>
> Can you highlight which are being modified?

I changed several arrays from "static const" to "const" because their
values now depend on ah, so it won't compile if they are static. For
example, offset_array contains AR_PHY_RX_IQCAL_CORR_B0 which depends
on AR_CHAN_BASE (which now depends on ah but did not before).

> > -#define AR_CHAN_BASE 0x9800
> > +#define AR_CHAN_BASE (AR_SREV_5502(ah) ? 0x29800 : 0x9800)
>
> this violates the coding style:
> <https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl>
>
> Things to avoid when using macros:
> macros that depend on having a local variable with a magic name
>
> So you should add ah as a parameter to the macro
>
> Repeat for all instances below where ah is being used
>

The macros like AR_CHAN_BASE and AR_SM_BASE are referenced by a lot of
other macros, some of which already have a "magic name" dependency on
ah like these ones:

https://github.com/torvalds/linux/blob/210e04ff768142b96452030c4c2627512b30ad95/drivers/net/wireless/ath/ath9k/ar9003_phy.h#L527

However I think it's probably good to avoid introducing new macros
with the magic name dependency. If desired, for every macro that I
have modified, I could try to add the ah parameter to all dependent
macros. This would probably be a rather large change so it might make
sense to be a separate commit.
Jeff Johnson May 18, 2022, 9:19 p.m. UTC | #3
On 5/17/2022 11:08 PM, Wenli Looi wrote:
> On Thu, May 12, 2022 at 2:45 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>>
>> On 5/12/2022 12:53 PM, Wenli Looi wrote:
>>> QCN550x is very similar to QCA956x. Note that AR_CH0_XTAL is
>>> intentionally unchanged. Certain arrays are no longer static because
>>> they are no longer constant.
>>
>> I don't understand the last sentence. You removed the 'static' keyword
>> but left the 'const' keyword, hence they are still constant.
>>
>> And I didn't actually see any instances where the arrays are being modified.
>>
>> Can you highlight which are being modified?
> 
> I changed several arrays from "static const" to "const" because their
> values now depend on ah, so it won't compile if they are static. For
> example, offset_array contains AR_PHY_RX_IQCAL_CORR_B0 which depends
> on AR_CHAN_BASE (which now depends on ah but did not before).

So the values are no longer constant, hence IMO the 'const' qualifier 
should also be removed

> 
>>> -#define AR_CHAN_BASE 0x9800
>>> +#define AR_CHAN_BASE (AR_SREV_5502(ah) ? 0x29800 : 0x9800)
>>
>> this violates the coding style:
>> <https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl>
>>
>> Things to avoid when using macros:
>> macros that depend on having a local variable with a magic name
>>
>> So you should add ah as a parameter to the macro
>>
>> Repeat for all instances below where ah is being used
>>
> 
> The macros like AR_CHAN_BASE and AR_SM_BASE are referenced by a lot of
> other macros, some of which already have a "magic name" dependency on
> ah like these ones:
> 
> https://github.com/torvalds/linux/blob/210e04ff768142b96452030c4c2627512b30ad95/drivers/net/wireless/ath/ath9k/ar9003_phy.h#L527
> 
> However I think it's probably good to avoid introducing new macros
> with the magic name dependency. If desired, for every macro that I
> have modified, I could try to add the ah parameter to all dependent
> macros. This would probably be a rather large change so it might make
> sense to be a separate commit.

Your current change illustrates one of the many pitfalls of using magic 
names. IMO you should have a precursor patch that changes all of the 
"bad" macros to take ah as a parameter rather than be a magic name.

At a minimum you should add ah as a parameter to the ones you are 
modifying so that it is clear why you need to change the attributes of 
those arrays.

/jeff