On Wed, Jan 16, 2013 at 09:31:19PM +0300, Dinar Talypov wrote:
> Hi,
>
> My D-link DWL-G520 card attaches on ath(4):
>
> ath0 at pci0 dev 9 function 0 "Atheros AR5212" rev 0x01: irq 11
> ath0: AR2414 7.9 phy 4.5 rf2413 5.6, FCC2A*, address 00:17:9a:09:f4:5a
>
> On ifconfig ath0 down && ifconfig ath0 up I've got system freeze.
>
> Googleing showed that system can freeze on register read or write
> while AR5212 chip in full sleep mode. And it is not possible to wake it up.
> This implies only for some AR5212 chips.
>
> In linux this is solved by making warm reset, instead
> of putting it in full sleep mode.
> The diff below does the same thing.
> What do you think about it? Or warm_reset function
> must be called only for particular chips?
This doesn't seem to break my AP with an ar5212.
ath0 at pci0 dev 12 function 0 "Atheros AR5212" rev 0x01: irq 9
ath0: AR5213A 5.9 phy 4.3 rf5112a 3.6, FCC2A*, address 00:02:6f:21:ea:8b
However, I have a few nits:
> Index: ar5212.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/ar5212.c,v
> retrieving revision 1.52
> diff -u -r1.52 ar5212.c
> --- ar5212.c 14 Oct 2011 17:08:09 -0000 1.52
> +++ ar5212.c 16 Jan 2013 16:58:33 -0000
> @@ -30,6 +30,7 @@
> u_int16_t ar5k_ar5212_radio_revision(struct ath_hal *, HAL_CHIP);
> void ar5k_ar5212_fill(struct ath_hal *);
> HAL_BOOL ar5k_ar5212_txpower(struct ath_hal *, HAL_CHANNEL *, u_int);
> +HAL_BOOL ar5k_ar5212_warm_reset(struct ath_hal *);
>
> /*
> * Initial register setting for the AR5212
> @@ -2420,6 +2421,44 @@
> }
>
> /*
> + * Put MAC and Baseband on warm reset and keep that state
> + * (don't clean sleep control register). After this MAC
> + * and Baseband are disabled and a full reset is needed
> + * to come back. This way we save as much power as possible
> + * without putting the card on full sleep.
> + */
The above comment is copied verbatim from Linux. It's probably OK
to copy comments from a legal perspective, but I would prefer an
original comment written from scratch.
> +
> +HAL_BOOL
> +ar5k_ar5212_warm_reset(struct ath_hal *hal)
> +{
> + u_int32_t flags;
> +
> + flags = AR5K_AR5212_RC_PCU | AR5K_AR5212_RC_BB;
> + if (hal->ah_pci_express == AH_FALSE)
> + flags |= AR5K_AR5212_RC_PCI;
> +
> + if (ar5k_ar5212_nic_reset(hal, flags) == AH_FALSE) {
> + AR5K_PRINT("failed to reset the AR5212 + PCI chipset\n");
I don't see the point of printing anything here.
> + return (AH_FALSE);
> + }
> +
> + /* ...wakeup */
This comment is also adapted from Linux. And not very useful.
> + if (ar5k_ar5212_set_power(hal,
> + HAL_PM_AWAKE, AH_TRUE, 0) == AH_FALSE) {
> + AR5K_PRINT("failed to resume the AR5212 (again)\n");
Again, it doesn't seem useful to print something here.
> + return (AH_FALSE);
> + }
> +
> + /* Put chipset on warm reset... */
> + if (ar5k_ar5212_nic_reset(hal, 0) == AH_FALSE) {
> + AR5K_PRINT("failed to warm reset the AR5212\n");
Likewise.
> + return (AH_FALSE);
> + }
> +
> + return (AH_TRUE);
> +}
> +
> +/*
> * Power management functions
> */
>
> @@ -2445,10 +2484,8 @@
> break;
>
> case HAL_PM_FULL_SLEEP:
> - if (set_chip == AH_TRUE) {
> - AR5K_REG_WRITE(AR5K_AR5212_SCR,
> - AR5K_AR5212_SCR_SLE_SLP);
> - }
> + if (set_chip == AH_TRUE)
> + ar5k_ar5212_warm_reset(hal);
> staid |= AR5K_AR5212_STA_ID1_PWR_SV;
> break;
>
>
>
> --
> Dinar Talypov <[email protected]>
