On Mon, Jan 29, 2018 at 11:12:09AM +0100, Stefan Sperling wrote:
> Here's what the diff you sent looks like if generated with 'cvs diff -up':
It looks like you got most/all of these changes from Linux ath5k?
Please be very clear about the origin of code you submit.
Here's my review of this diff:
>
> Index: ar5212.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/ar5212.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 ar5212.c
> --- ar5212.c 15 Oct 2017 13:06:12 -0000 1.57
> +++ ar5212.c 27 Jan 2018 17:27:44 -0000
> @@ -235,9 +235,17 @@ ar5k_ar5212_attach(u_int16_t device, voi
> hal->ah_phy_spending = AR5K_AR5212_PHY_SPENDING_AR5424;
> hal->ah_radio_5ghz_revision = hal->ah_radio_2ghz_revision =
> AR5K_SREV_VER_AR5413;
> - } else if (srev == AR5K_SREV_VER_AR2425) {
> + } else if (hal->ah_mac_version == (AR5K_SREV_VER_AR2425 >> 4) ||
> + hal->ah_mac_version == (AR5K_SREV_VER_AR2417 >> 4) ||
Please intend by 4 spaces when an if statement spans multiple lines,
instead of indenting by 2 full tabs. We would format it like this:
if (very_long_condition_foo ||
very_long_condition_bar(with several,
long arguments)) {
/* do something */
}
See the style(9) man page (search for 'spaces' to find what I'm referring to).
Try to avoid using naked magic numbers, such as >> 4.
There is an AR5K_REG_MS (register mask+shift) macro you could use here
in conjunction with AR5K_AR5212_SREV_VER, for example:
} else if (hal->ah_mac_version ==
AR5K_REG_MS(AR5K_AR5212_SREV_VER, AR5K_SREV_VER_AR2425)
> + hal->ah_phy_revision == (AR5K_SREV_PHY_2425)) {
> hal->ah_radio = AR5K_AR2425;
> - hal->ah_phy_spending = AR5K_AR5212_PHY_SPENDING_AR5112;
> + hal->ah_single_chip = AH_TRUE;
> + hal->ah_radio_5ghz_revision= AR5K_SREV_RAD_2425;
> + } else if ((hal->ah_mac_version == (AR5K_SREV_VER_AR2424 >> 4)) ||
> + (hal->ah_phy_revision == AR5K_SREV_PHY_5413)) {
Same, please use AR5K_REG_MS(AR5K_AR5212_SREV_VER, ...).
> + hal->ah_radio = AR5K_AR5413;
> + hal->ah_single_chip = AH_TRUE;
> + hal->ah_radio_5ghz_revision = AR5K_SREV_RAD_5413;
> hal->ah_radio_5ghz_revision = AR5K_SREV_RAD_SC2;
> } else if (hal->ah_radio_5ghz_revision < AR5K_SREV_RAD_5112) {
> hal->ah_radio = AR5K_AR5111;
> @@ -2871,10 +2879,8 @@ ar5k_ar5212_get_capabilities(struct ath_
>
> if (b)
> hal->ah_capabilities.cap_mode |= HAL_MODE_11B;
> -#if 0
> if (g)
> hal->ah_capabilities.cap_mode |= HAL_MODE_11G;
> -#endif
> }
>
> /* GPIO */
> Index: ar5xxx.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/ar5xxx.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 ar5xxx.c
> --- ar5xxx.c 25 Aug 2017 10:04:36 -0000 1.62
> +++ ar5xxx.c 27 Jan 2018 17:28:07 -0000
> @@ -86,6 +86,7 @@ u_int32_t ar5k_ar5110_chan2athchan(HAL_
> HAL_BOOL ar5k_ar5111_channel(struct ath_hal *, HAL_CHANNEL *);
> HAL_BOOL ar5k_ar5111_chan2athchan(u_int, struct ar5k_athchan_2ghz *);
> HAL_BOOL ar5k_ar5112_channel(struct ath_hal *, HAL_CHANNEL *);
> +HAL_BOOL ar5k_ar2425_channel(struct ath_hal *, HAL_CHANNEL *);
> HAL_BOOL ar5k_check_channel(struct ath_hal *, u_int16_t, u_int flags);
>
> HAL_BOOL ar5k_ar5111_rfregs(struct ath_hal *, HAL_CHANNEL *, u_int);
> @@ -890,6 +891,12 @@ ar5k_eeprom_init(struct ath_hal *hal)
> ee->ee_db[AR5K_EEPROM_MODE_11G][0] = (val >> 3) & 0x7;
> }
>
> + AR5K_EEPROM_READ(AR5K_EEPROM_IS_HB63, val);
> + if ((hal->ah_mac_version == (AR5K_SREV_VER_AR2425 >> 4)) && val)
Same, please use AR5K_REG_MS(AR5K_AR5212_SREV_VER, ...).
> + ee->ee_is_hb63 = AH_TRUE;
> + else
Identation of above 'else' line is wrong.
> + ee->ee_is_hb63 = AH_FALSE;
> +
> /*
> * Get conformance test limit values
> */
> @@ -1109,6 +1116,8 @@ ar5k_channel(struct ath_hal *hal, HAL_CH
> ret = ar5k_ar5110_channel(hal, channel);
> else if (hal->ah_radio == AR5K_AR5111)
> ret = ar5k_ar5111_channel(hal, channel);
> + else if (hal->ah_radio == AR5K_AR2425)
> + ret = ar5k_ar2425_channel(hal, channel);
> else
> ret = ar5k_ar5112_channel(hal, channel);
>
> @@ -1260,6 +1269,44 @@ ar5k_ar5112_channel(struct ath_hal *hal,
> }
>
> data = (data0 << 4) | (data1 << 1) | (data2 << 2) | 0x1001;
> +
> + AR5K_PHY_WRITE(0x27, data & 0xff);
> + AR5K_PHY_WRITE(0x36, (data >> 8) & 0x7f);
Unfortunately these parts of the driver were reverse-engineered
and therefore contain a lof of magic register offsets like this.
This is generally considered bad style for OpenBSD code.
If you have time, while you're adding such register writes, please try
to look up these offsets in FreeBSD where the ath HAL source code has since
been released: https://svnweb.freebsd.org/base/head/sys/dev/ath/ath_hal/
And if you find one, add a macro name for a register used in your diff.
Not really required, but doing so would give this diff a nice touch :)
> +
> + return (AH_TRUE)
Complication fails because of missing semicolon on this line.
> +}
> +
> +HAL_BOOL
> +ar5k_ar2425_channel(struct ath_hal *hal, HAL_CHANNEL *channel)
> +{
> + u_int32_t data, data0, data2;
> + u_int16_t c;
This function seems to be derived from ath5k_hw_rf2425_channel() in ath5k.
FreeBSD's HAL has a corresponding function called ar2425SetChannel().
Please cross-check with their implementation if you haven't done so yet.
It would help if we started using better variable names here (another artifact
of the reverse engineering done years ago). FreeBSD uses the following names:
data -> reg32 (no good reason to change, I think)
data0 -> channelSel (we could change our name to e.g. chan_sel)
data1 -> aModeRefSel (we could change our name to e.g. mode_sel)
> +
> + data = data0 = data2 = 0;
> + c = channel->c_channel + hal->ah_chanoff;
> +
Please fix bad indentation of comment below:
> + /*
> + * Set the channel on the AR2425
> + */
> + if (c < 4800) {
> + data0 = ar5k_bitswap((c - 2272), 8);
> + data2 = 0;
> + } else if ((c - (c % 5)) != 2 || c > 5435) {
> + if (!(c % 20) && c < 5120)
> + data0 = ar5k_bitswap(((c - 4800) / 20 << 2), 8);
> + else if (!(c % 10))
> + data0 = ar5k_bitswap(((c - 4800) / 10 << 1), 8);
> + else if (!(c % 5))
> + data0 = ar5k_bitswap((c - 4800) / 5, 8);
> + else
> + return (AH_FALSE);
> + data2 = ar5k_bitswap(1, 2);
> + } else {
> + data0 = ar5k_bitswap((10 * (c - 2) - 4800) / 25 + 1, 8);
> + data2 = ar5k_bitswap(0, 2);
> + }
> +
> + data = (data0 << 4) | (data2 << 2) | 0x1001;
>
> AR5K_PHY_WRITE(0x27, data & 0xff);
> AR5K_PHY_WRITE(0x36, (data >> 8) & 0x7f);
> Index: ar5212reg.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/ar5212reg.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 ar5212reg.h
> --- ar5212reg.h 30 Jul 2008 07:15:39 -0000 1.12
> +++ ar5212reg.h 27 Jan 2018 17:27:57 -0000
> @@ -456,6 +456,7 @@
> */
> #define AR5K_AR5212_DCU_MISC(_n) AR5K_AR5212_DCU(_n, 0x1100)
> #define AR5K_AR5212_DCU_MISC_BACKOFF 0x000007ff
> +#define AR5K_AR5212_DCU_MISC_FRAG_WAIT 0x00000100
AR5K_AR5212_DCU_MISC_FRAG_WAIT is not used anywhere in your diff.
As an aside, Linux defines AR5K_DCU_MISC_BACKOFF as 0x0000003f.
FreeBSD's ar5212reg.h defines AR5K_AR5212_DCU_MISC_BACKOFF as
AR_D_MISC_BKOFF_THRESH and also uses the value 0x0000003F.
Your new (albeit unused) macro overlaps with our value but not theirs.
It looks like our value of AR5K_AR5212_DCU_MISC_BACKOFF must be changed?
> #define AR5K_AR5212_DCU_MISC_BACKOFF_FRAG 0x00000200
> #define AR5K_AR5212_DCU_MISC_HCFPOLL_ENABLE 0x00000800
> #define AR5K_AR5212_DCU_MISC_BACKOFF_PERSIST 0x00001000
> @@ -1099,6 +1100,8 @@ typedef enum {
> #define AR5K_AR5212_PHY_SLMT_32MHZ 0x0000007f
> #define AR5K_AR5212_PHY_SCAL 0x9878
> #define AR5K_AR5212_PHY_SCAL_32MHZ 0x0000000e
> +#define AR5K_AR5212_PHY_SCAL_32MHZ_2417 0x0000000a
> +#define AR5K_AR5212_PHY_SCAL_32MHZ_HB63 0x00000032
You're adding these (from Linux) but you aren't using them anywhere.
In Linux these values are used by ath5k_hw_set_sleep_clock() for AR5212.
Will our ar5k_ar5212_reset() perhaps need additional changes which make
use of these macros?
> /*
> * PHY PLL control register
> Index: ar5xxx.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/ar5xxx.h,v
> retrieving revision 1.60
> diff -u -p -r1.60 ar5xxx.h
> --- ar5xxx.h 25 Aug 2017 12:17:27 -0000 1.60
> +++ ar5xxx.h 27 Jan 2018 17:28:17 -0000
> @@ -623,6 +623,8 @@ struct ar5k_gain {
> #define AR5K_EEPROM_INFO_CKSUM 0xffff
> #define AR5K_EEPROM_INFO(_n) (AR5K_EEPROM_INFO_BASE + (_n))
>
> +#define AR5K_EEPROM_IS_HB63 0x000b /* Talon detect */
> +
Good. This agrees with code in FreeBSD's legacyEepromGet().
> #define AR5K_EEPROM_VERSION AR5K_EEPROM_INFO(1)
> #define AR5K_EEPROM_VERSION_3_0 0x3000
> #define AR5K_EEPROM_VERSION_3_1 0x3001
> @@ -765,6 +767,7 @@ struct ar5k_eeprom_info {
> int16_t ee_noise_floor_thr[AR5K_EEPROM_N_MODES];
> int8_t ee_adc_desired_size[AR5K_EEPROM_N_MODES];
> int8_t ee_pga_desired_size[AR5K_EEPROM_N_MODES];
> + HAL_BOOL ee_is_hb63;
> };
>
> /*
> @@ -1235,9 +1238,9 @@ struct ar5k_srev_name {
> #define AR5K_SREV_VER_AR5414 0xa5
> #define AR5K_SREV_VER_AR5416 0xc0 /* PCI-Express */
> #define AR5K_SREV_VER_AR5418 0xca /* PCI-Express */
> -#define AR5K_SREV_VER_AR2425 0xe2 /* PCI-Express */
> +#define AR5K_SREV_VER_AR2425 0xe0 /* PCI-Express (Swan, was 0xe2) */
> +#define AR5K_SREV_VER_AR2417 0xf0 /* PCI-Express (Nala) */
> #define AR5K_SREV_VER_UNSUPP 0xff
> -
> #define AR5K_SREV_RAD_5110 0x00
> #define AR5K_SREV_RAD_5111 0x10
> #define AR5K_SREV_RAD_5111A 0x15
> @@ -1249,9 +1252,12 @@ struct ar5k_srev_name {
> #define AR5K_SREV_RAD_SC0 0x56
> #define AR5K_SREV_RAD_SC1 0x63
> #define AR5K_SREV_RAD_SC2 0xa2
> +#define AR5K_SREV_RAD_5413 0x60
> +#define AR5K_SREV_RAD_2425 0xa2
Fine. 5413 matches FreeBSD's ar5212reg.h and 2425 (0xa2) matches Linux ath5k.
> #define AR5K_SREV_RAD_5133 0xc0
> #define AR5K_SREV_RAD_UNSUPP 0xff
> -
> +#define AR5K_SREV_PHY_5413 0x61
> +#define AR5K_SREV_PHY_2425 0x70
Good. These match ath5k.
> #define AR5K_DEVID_AR2413 0x001a
> #define AR5K_DEVID_AR5413 0x001b
> #define AR5K_DEVID_AR5424 0x001c
>
