Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/24, Nick Kossifidis <mick@madwifi.org>:
>  * Fix nic_wakeup for PCI-E chips (don't set AR5K_RESET_CTL_PCI bit)
>
>   * Fix dma size setting for PCI-E chips (thanx to Bob Copeland).
>
>  Changes-licensed-under: ISC
>  Signed-off-by: Nick Kossifidis <mickflemm@gmail.com>
>
>  ---
>  diff --git a/drivers/net/wireless/ath5k/ath5k.h b/drivers/net/wireless/ath5k/ath5k.h
>  index c0b6596..d8ec373 100644
>  --- a/drivers/net/wireless/ath5k/ath5k.h
>  +++ b/drivers/net/wireless/ath5k/ath5k.h
>  @@ -971,6 +971,7 @@ struct ath5k_hw {
>         enum ath5k_version      ah_version;
>         enum ath5k_radio        ah_radio;
>         u32                     ah_phy;
>  +       bool                    ah_pcie;
>
>         bool                    ah_5ghz;
>         bool                    ah_2ghz;
>  diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
>  index 3ae5522..cd640ed 100644
>  --- a/drivers/net/wireless/ath5k/hw.c
>  +++ b/drivers/net/wireless/ath5k/hw.c
>  @@ -214,6 +214,14 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
>                 ah->ah_single_chip = false;
>         }
>
>  +       /* Identify PCI-E cards */
>  +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  +       srev >= AR5K_SREV_VER_AR5416) {
>  +               ah->ah_pcie = true;
>  +       } else {
>  +               ah->ah_pcie = false;
>  +       }
>  +
>         /* Single chip radio */
>         if (ah->ah_radio_2ghz_revision == ah->ah_radio_5ghz_revision)
>                 ah->ah_radio_2ghz_revision = 0;
>  @@ -377,9 +385,12 @@ static int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial)
>                                         AR5K_PHY_TURBO);
>         }
>
>  -       /* ...reset chipset and PCI device */
>  -       if (ah->ah_single_chip == false && ath5k_hw_nic_reset(ah,
>  -                               AR5K_RESET_CTL_CHIP | AR5K_RESET_CTL_PCI)) {
>  +       /* ...reset chipset
>  +        * Warning: reseting PCI on PCI-E cards results card to hang
>  +        * and always return 0xffff...
>  +        */
>  +       if (ath5k_hw_nic_reset(ah, AR5K_RESET_CTL_PCU | AR5K_RESET_CTL_BASEBAND |
>  +                               (ah->ah_pcie == false ? AR5K_RESET_CTL_PCI : 0) )) {
>                 ATH5K_ERR(ah->ah_sc, "failed to reset the MAC Chip + PCI\n");
>                 return -EIO;
>         }
>  @@ -900,13 +911,25 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum ieee80211_if_types op_mode,
>
>         /*
>          * Set Rx/Tx DMA Configuration
>  -        *(passing dma size not available on 5210)
>  +        *
>  +        * Set maximum DMA size (512) except for PCI-E cards since
>  +        * it causes rx overruns and tx errors (tested on 5424 but since
>  +        * rx overruns also occur on 5416/5418 with madwifi we set 128
>  +        * for all PCI-E cards to be safe).
>  +        *
>  +        * In dumps this is 128 for allchips.
>  +        *
>  +        * XXX: need to check 5210 for this
>  +        * TODO: Check out tx triger level, it's always 64 on dumps but i
>  +        * guess we can tweak it and see how it goes ;-)
>          */
>         if (ah->ah_version != AR5K_AR5210) {
>                 AR5K_REG_WRITE_BITS(ah, AR5K_TXCFG, AR5K_TXCFG_SDMAMR,
>  -                               AR5K_DMASIZE_512B | AR5K_TXCFG_DMASIZE);
>  +                               (ah->ah_pcie == true ?
>  +                               AR5K_DMASIZE_128B : AR5K_DMASIZE_512B));
>                 AR5K_REG_WRITE_BITS(ah, AR5K_RXCFG, AR5K_RXCFG_SDMAMW,
>  -                               AR5K_DMASIZE_512B);
>  +                               (ah->ah_pcie == true ?
>  +                               AR5K_DMASIZE_128B : AR5K_DMASIZE_512B));
>         }
>
>         /*
>

Sorry, forgot to CC Bob...

Bob can you test this on your 5242 (together with the next patch) and
see what happens ?

-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/24, Nick Kossifidis <mickflemm@gmail.com>:
> 2008/2/24, Nick Kossifidis <mick@madwifi.org>:
>
> >  * Fix nic_wakeup for PCI-E chips (don't set AR5K_RESET_CTL_PCI bit)
>  >
>  >   * Fix dma size setting for PCI-E chips (thanx to Bob Copeland).
>  >
>  >  Changes-licensed-under: ISC
>  >  Signed-off-by: Nick Kossifidis <mickflemm@gmail.com>
>  >
>  >  ---
>  >  diff --git a/drivers/net/wireless/ath5k/ath5k.h b/drivers/net/wireless/ath5k/ath5k.h
>  >  index c0b6596..d8ec373 100644
>  >  --- a/drivers/net/wireless/ath5k/ath5k.h
>  >  +++ b/drivers/net/wireless/ath5k/ath5k.h
>  >  @@ -971,6 +971,7 @@ struct ath5k_hw {
>  >         enum ath5k_version      ah_version;
>  >         enum ath5k_radio        ah_radio;
>  >         u32                     ah_phy;
>  >  +       bool                    ah_pcie;
>  >
>  >         bool                    ah_5ghz;
>  >         bool                    ah_2ghz;
>  >  diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
>  >  index 3ae5522..cd640ed 100644
>  >  --- a/drivers/net/wireless/ath5k/hw.c
>  >  +++ b/drivers/net/wireless/ath5k/hw.c
>  >  @@ -214,6 +214,14 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
>  >                 ah->ah_single_chip = false;
>  >         }
>  >
>  >  +       /* Identify PCI-E cards */
>  >  +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  >  +       srev >= AR5K_SREV_VER_AR5416) {
>  >  +               ah->ah_pcie = true;
>  >  +       } else {
>  >  +               ah->ah_pcie = false;
>  >  +       }
>  >  +
>  >         /* Single chip radio */
>  >         if (ah->ah_radio_2ghz_revision == ah->ah_radio_5ghz_revision)
>  >                 ah->ah_radio_2ghz_revision = 0;
>  >  @@ -377,9 +385,12 @@ static int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial)
>  >                                         AR5K_PHY_TURBO);
>  >         }
>  >
>  >  -       /* ...reset chipset and PCI device */
>  >  -       if (ah->ah_single_chip == false && ath5k_hw_nic_reset(ah,
>  >  -                               AR5K_RESET_CTL_CHIP | AR5K_RESET_CTL_PCI)) {
>  >  +       /* ...reset chipset
>  >  +        * Warning: reseting PCI on PCI-E cards results card to hang
>  >  +        * and always return 0xffff...
>  >  +        */
>  >  +       if (ath5k_hw_nic_reset(ah, AR5K_RESET_CTL_PCU | AR5K_RESET_CTL_BASEBAND |
>  >  +                               (ah->ah_pcie == false ? AR5K_RESET_CTL_PCI : 0) )) {
>  >                 ATH5K_ERR(ah->ah_sc, "failed to reset the MAC Chip + PCI\n");
>  >                 return -EIO;
>  >         }
>  >  @@ -900,13 +911,25 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum ieee80211_if_types op_mode,
>  >
>  >         /*
>  >          * Set Rx/Tx DMA Configuration
>  >  -        *(passing dma size not available on 5210)
>  >  +        *
>  >  +        * Set maximum DMA size (512) except for PCI-E cards since
>  >  +        * it causes rx overruns and tx errors (tested on 5424 but since
>  >  +        * rx overruns also occur on 5416/5418 with madwifi we set 128
>  >  +        * for all PCI-E cards to be safe).
>  >  +        *
>  >  +        * In dumps this is 128 for allchips.
>  >  +        *
>  >  +        * XXX: need to check 5210 for this
>  >  +        * TODO: Check out tx triger level, it's always 64 on dumps but i
>  >  +        * guess we can tweak it and see how it goes ;-)
>  >          */
>  >         if (ah->ah_version != AR5K_AR5210) {
>  >                 AR5K_REG_WRITE_BITS(ah, AR5K_TXCFG, AR5K_TXCFG_SDMAMR,
>  >  -                               AR5K_DMASIZE_512B | AR5K_TXCFG_DMASIZE);
>  >  +                               (ah->ah_pcie == true ?
>  >  +                               AR5K_DMASIZE_128B : AR5K_DMASIZE_512B));
>  >                 AR5K_REG_WRITE_BITS(ah, AR5K_RXCFG, AR5K_RXCFG_SDMAMW,
>  >  -                               AR5K_DMASIZE_512B);
>  >  +                               (ah->ah_pcie == true ?
>  >  +                               AR5K_DMASIZE_128B : AR5K_DMASIZE_512B));
>  >         }
>  >
>  >         /*
>  >
>
>
> Sorry, forgot to CC Bob...
>
>  Bob can you test this on your 5242 (together with the next patch) and
>  see what happens ?
>

hmm i mean 5424, time for sleep :P


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Bob Copeland]

> Sorry, forgot to CC Bob...
> 
> Bob can you test this on your 5242 (together with the next patch) and
> see what happens ?

Yep, I'll give the patchset a spin today...

-- 
Bob Copeland %% www.bobcopeland.com 

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Bob Copeland]

On Sun, Feb 24, 2008 at 06:45:33AM +0200, Nick Kossifidis wrote:
> >  +       /* Identify PCI-E cards */
> >  +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
> >  +       srev >= AR5K_SREV_VER_AR5416) {
> >  +               ah->ah_pcie = true;
> >  +       } else {
> >  +               ah->ah_pcie = false;
> >  +       }

This won't work, because reset is called before we ever set ah_pcie.
Moving the set before the first call to wakeup (patch below) does the
trick.  Then it works at least as well as I had it working (still with
random calibration failures, but I am again sending this mail using the
driver).

Thanks!

>From f59b2bc93059e7f1bd504714ca22654f4242d78d Mon Sep 17 00:00:00 2001
From: Bob Copeland <me@bobcopeland.com>
Date: Sun, 24 Feb 2008 10:30:50 -0500
Subject: [PATCH] Grab srev before resetting card.

Setting ah_pcie variable must be done before the first call to
ath5k_hw_nic_wakeup.
---
 drivers/net/wireless/ath5k/hw.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
index adcce6f..71cdbd9 100644
--- a/drivers/net/wireless/ath5k/hw.c
+++ b/drivers/net/wireless/ath5k/hw.c
@@ -178,13 +178,21 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
 	else if (ah->ah_version <= AR5K_AR5211)
 		ah->ah_proc_rx_desc = ath5k_hw_proc_old_rx_status;
 
+	/* Get MAC, PHY and RADIO revisions */
+	srev = ath5k_hw_reg_read(ah, AR5K_SREV);
+	/* Identify PCI-E cards */
+	if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
+	srev >= AR5K_SREV_VER_AR5416) {
+		ah->ah_pcie = true;
+	} else {
+		ah->ah_pcie = false;
+	}
+
 	/* Bring device out of sleep and reset it's units */
 	ret = ath5k_hw_nic_wakeup(ah, AR5K_INIT_MODE, true);
 	if (ret)
 		goto err_free;
 
-	/* Get MAC, PHY and RADIO revisions */
-	srev = ath5k_hw_reg_read(ah, AR5K_SREV);
 	ah->ah_mac_srev = srev;
 	ah->ah_mac_version = AR5K_REG_MS(srev, AR5K_SREV_VER);
 	ah->ah_mac_revision = AR5K_REG_MS(srev, AR5K_SREV_REV);
@@ -214,14 +222,6 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
 		ah->ah_single_chip = false;
 	}
 
-	/* Identify PCI-E cards */
-	if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
-	srev >= AR5K_SREV_VER_AR5416) {
-		ah->ah_pcie = true;
-	} else {
-		ah->ah_pcie = false;
-	}
-
 	/* Single chip radio */
 	if (ah->ah_radio_2ghz_revision == ah->ah_radio_5ghz_revision)
 		ah->ah_radio_2ghz_revision = 0;
-- 
1.5.4.2.182.gb3092

-- 
Bob Copeland %% www.bobcopeland.com 

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/24, Bob Copeland <me@bobcopeland.com>:
> On Sun, Feb 24, 2008 at 06:45:33AM +0200, Nick Kossifidis wrote:
>  > >  +       /* Identify PCI-E cards */
>  > >  +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  > >  +       srev >= AR5K_SREV_VER_AR5416) {
>  > >  +               ah->ah_pcie = true;
>  > >  +       } else {
>  > >  +               ah->ah_pcie = false;
>  > >  +       }
>
>
> This won't work, because reset is called before we ever set ah_pcie.
>  Moving the set before the first call to wakeup (patch below) does the
>  trick.  Then it works at least as well as I had it working (still with
>  random calibration failures, but I am again sending this mail using the
>  driver).
>
>  Thanks!
>
>  From f59b2bc93059e7f1bd504714ca22654f4242d78d Mon Sep 17 00:00:00 2001
>  From: Bob Copeland <me@bobcopeland.com>
>  Date: Sun, 24 Feb 2008 10:30:50 -0500
>  Subject: [PATCH] Grab srev before resetting card.
>
>  Setting ah_pcie variable must be done before the first call to
>  ath5k_hw_nic_wakeup.
>  ---
>   drivers/net/wireless/ath5k/hw.c |   20 ++++++++++----------
>   1 files changed, 10 insertions(+), 10 deletions(-)
>
>
>  diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
>
> index adcce6f..71cdbd9 100644
>
> --- a/drivers/net/wireless/ath5k/hw.c
>  +++ b/drivers/net/wireless/ath5k/hw.c
>
> @@ -178,13 +178,21 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
>         else if (ah->ah_version <= AR5K_AR5211)
>                 ah->ah_proc_rx_desc = ath5k_hw_proc_old_rx_status;
>
>  +       /* Get MAC, PHY and RADIO revisions */
>  +       srev = ath5k_hw_reg_read(ah, AR5K_SREV);
>  +       /* Identify PCI-E cards */
>
> +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  +       srev >= AR5K_SREV_VER_AR5416) {
>  +               ah->ah_pcie = true;
>  +       } else {
>  +               ah->ah_pcie = false;
>  +       }
>  +
>
>         /* Bring device out of sleep and reset it's units */
>         ret = ath5k_hw_nic_wakeup(ah, AR5K_INIT_MODE, true);
>         if (ret)
>                 goto err_free;
>
>  -       /* Get MAC, PHY and RADIO revisions */
>  -       srev = ath5k_hw_reg_read(ah, AR5K_SREV);
>         ah->ah_mac_srev = srev;
>         ah->ah_mac_version = AR5K_REG_MS(srev, AR5K_SREV_VER);
>         ah->ah_mac_revision = AR5K_REG_MS(srev, AR5K_SREV_REV);
>  @@ -214,14 +222,6 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc, u8 mac_version)
>
>                 ah->ah_single_chip = false;
>         }
>
>  -       /* Identify PCI-E cards */
>  -       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  -       srev >= AR5K_SREV_VER_AR5416) {
>  -               ah->ah_pcie = true;
>  -       } else {
>  -               ah->ah_pcie = false;
>  -       }
>  -
>         /* Single chip radio */
>         if (ah->ah_radio_2ghz_revision == ah->ah_radio_5ghz_revision)
>                 ah->ah_radio_2ghz_revision = 0;
>

Thanx, just plz put a new line here ->

+	/* Get MAC, PHY and RADIO revisions */
+	srev = ath5k_hw_reg_read(ah, AR5K_SREV);
+
+	/* Identify PCI-E cards */
+	if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
+	srev >= AR5K_SREV_VER_AR5416) {
+		ah->ah_pcie = true;
+	} else {
+		ah->ah_pcie = false;
+	}
+

Acked-by: Nick Kossifidis <mickflemm@gmail.com>

-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Christoph Hellwig]

On Sun, Feb 24, 2008 at 06:45:33AM +0200, Nick Kossifidis wrote:
> Sorry, forgot to CC Bob...
> 
> Bob can you test this on your 5242 (together with the next patch) and
> see what happens ?

Talking about unsupported chips.  My MacBook Pro has a AR5418, do you
guys need any kind of help for this chip, e.g. mmio traces?

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Bob Copeland]

On Sun, Feb 24, 2008 at 08:58:09PM +0200, Nick Kossifidis wrote:
> Thanx, just plz put a new line here ->
> 
> +	/* Get MAC, PHY and RADIO revisions */
> +	srev = ath5k_hw_reg_read(ah, AR5K_SREV);
> +
> +	/* Identify PCI-E cards */

> Acked-by: Nick Kossifidis <mickflemm@gmail.com>

Do you want me to respin and post the patch with the newline, or
are you just going to roll it into your changes?  I don't mind
either way, but if you pick it up, feel free to add my:

Tested-by: Bob Copeland <me@bobcopeland.com>

-- 
Bob Copeland %% www.bobcopeland.com 

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/24, Bob Copeland <me@bobcopeland.com>:
> On Sun, Feb 24, 2008 at 08:58:09PM +0200, Nick Kossifidis wrote:
>  > Thanx, just plz put a new line here ->
>  >
>  > +     /* Get MAC, PHY and RADIO revisions */
>  > +     srev = ath5k_hw_reg_read(ah, AR5K_SREV);
>  > +
>  > +     /* Identify PCI-E cards */
>
>
> > Acked-by: Nick Kossifidis <mickflemm@gmail.com>
>
>
> Do you want me to respin and post the patch with the newline, or
>  are you just going to roll it into your changes?  I don't mind
>  either way, but if you pick it up, feel free to add my:
>
>  Tested-by: Bob Copeland <me@bobcopeland.com>
>
>

Yup, can you post a patch on top of the series ? Also remove braces
etc as Christoph said if you like (i prefer to keep the "else" there).


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/24, Christoph Hellwig <hch@infradead.org>:
> On Sun, Feb 24, 2008 at 06:45:33AM +0200, Nick Kossifidis wrote:
>
> > Sorry, forgot to CC Bob...
>  >
>  > Bob can you test this on your 5242 (together with the next patch) and
>  > see what happens ?
>
>
> Talking about unsupported chips.  My MacBook Pro has a AR5418, do you
>  guys need any kind of help for this chip, e.g. mmio traces?
>

I've got a MacBook pro myself, we'll work on AR5416/8 sometime in the
future but we have other things to fix first (e.g. AR5212/5213 +
RF5111/5112/2112 can reach up to 27Mbit/s while 5413/2413 only go as
far as 13Mbit/s with ath5k). Thanx for the offer, i'll keep you posted
if we need anything ;-)


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/24, Bob Copeland <me@bobcopeland.com>:
>  Then it works at least as well as I had it working (still with
>  random calibration failures, but I am again sending this mail using the
>  driver).

Some thoughts...

It seems we are doing calibration wrong, i'll check out the other
chips as well to come up with something, until then can you check out
how does your card perform on b rates (eg. 11Mbits). Do an iperf test
and see what you get eg. for 5.5M and 11M, we might force b-only mode
for these cards until we fix these issues (this would probably also
make rate control work).


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Bob Copeland]

> Some thoughts...
> 
> It seems we are doing calibration wrong, i'll check out the other
> chips as well to come up with something, until then can you check out
> how does your card perform on b rates (eg. 11Mbits). Do an iperf test

Hey, that's a good clue... I just switched over to b-only and it seems to
be much more stable.  It may be a few days before I can do the iperf tests,
gotta reconfigure my LAN somewhat.

-- 
Bob Copeland %% www.bobcopeland.com 

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/25, Bob Copeland <me@bobcopeland.com>:
> > Some thoughts...
>  >
>  > It seems we are doing calibration wrong, i'll check out the other
>  > chips as well to come up with something, until then can you check out
>  > how does your card perform on b rates (eg. 11Mbits). Do an iperf test
>
>
> Hey, that's a good clue... I just switched over to b-only and it seems to
>  be much more stable.  It may be a few days before I can do the iperf tests,
>  gotta reconfigure my LAN somewhat.
>
>

If i'm correct you should get 4-7Mbit/sec @ 11Mbit. Plz let me know if
you have some results, meanwhile i'll try to figure out the i/q
calibration algo (we are ok for noise floor calibration i believe).


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Bob Copeland]

> > Hey, that's a good clue... I just switched over to b-only and it seems to
> >  be much more stable.

...or not.  I still got some calibration errors last night in b-mode.  Just
so we're on the same page, I see things in the dmesg like:

ath0: failed to restore operational channel after scan
ath5k phy0: calibration timeout (2412 MHz)
ath5k phy0: ath5k_chan_set: unable to reset channel (2412 Mhz)
ath0: failed to set freq to 2412 MHz for scan
ath5k phy0: calibration timeout (2417 MHz)

> If i'm correct you should get 4-7Mbit/sec @ 11Mbit. Plz let me know if
> you have some results, meanwhile i'll try to figure out the i/q
> calibration algo (we are ok for noise floor calibration i believe).

One thing I noticed from my traces is that the binary driver sets
bits AR5K_PHY_AGCCTL_NF | AR5K_PHY_AGCCTL_CAL in AR5K_PHY_AGCCTL.
Then it makes a whole lot of misc register writes, then re-reads
AR5K_PHY_AGCCTL; in that time only the noise floor bit got cleared
but _CAL is still high.  Dunno if that means anything to you or not.

e.g:

R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL  <-- set (_CAL | _NF)
W: 0x1000 = 0x00000001 - AR5K_DCU_QCUMASK_BASE
W: 0x1004 = 0x00000002 - unknown
W: 0x1008 = 0x00000004 - unknown
[... lots more writes to DCU & IMR regs ...]
W: 0x00a0 = 0x00080965 - AR5K_PIMR
R: 0x00ac = 0x00000000 - AR5K_SIMR2
W: 0x00ac = 0x00070000 - AR5K_SIMR2
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL  <-- _NF cleared

Maybe a red herring as obviously the current method of doing things
works for me sometimes...

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[bruno randolf]

On Tuesday 26 February 2008 00:43:07 Bob Copeland wrote:
> > > Hey, that's a good clue... I just switched over to b-only and it seems
> > > to be much more stable.
>
> ...or not.  I still got some calibration errors last night in b-mode.  Just
> so we're on the same page, I see things in the dmesg like:
>
> ath0: failed to restore operational channel after scan
> ath5k phy0: calibration timeout (2412 MHz)
> ath5k phy0: ath5k_chan_set: unable to reset channel (2412 Mhz)
> ath0: failed to set freq to 2412 MHz for scan
> ath5k phy0: calibration timeout (2417 MHz)
>
> > If i'm correct you should get 4-7Mbit/sec @ 11Mbit. Plz let me know if
> > you have some results, meanwhile i'll try to figure out the i/q
> > calibration algo (we are ok for noise floor calibration i believe).
>
> One thing I noticed from my traces is that the binary driver sets
> bits AR5K_PHY_AGCCTL_NF | AR5K_PHY_AGCCTL_CAL in AR5K_PHY_AGCCTL.
> Then it makes a whole lot of misc register writes, then re-reads
> AR5K_PHY_AGCCTL; in that time only the noise floor bit got cleared
> but _CAL is still high.  Dunno if that means anything to you or not.
>
> e.g:
>
> R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL
> W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL  <-- set (_CAL | _NF)
> W: 0x1000 = 0x00000001 - AR5K_DCU_QCUMASK_BASE
> W: 0x1004 = 0x00000002 - unknown
> W: 0x1008 = 0x00000004 - unknown
> [... lots more writes to DCU & IMR regs ...]
> W: 0x00a0 = 0x00080965 - AR5K_PIMR
> R: 0x00ac = 0x00000000 - AR5K_SIMR2
> W: 0x00ac = 0x00070000 - AR5K_SIMR2
> R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
> R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
> R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
> R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
> R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
> R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL  <-- _NF cleared
>
> Maybe a red herring as obviously the current method of doing things
> works for me sometimes...

so when is AR5K_PHY_AGCCTL_CAL getting cleared?

i'm not sure if that is related because the error message you are seeing is 
not due to noise floor calibration timeout but because AR5K_PHY_AGCCTL_CAL is 
not cleared in the time that ath5k expects, but i want to mention it anyways:

the HAL enables noise floor calibration, and reads the result on then next 
calibration interval, then enables calibration again and reads on the next... 
i think that way it makes sure there is always enough time for the noise 
floor calibration to take place in the mean time (the calibration can only 
happen when the channel is otherwise silent).

if i understand correctly, ath5k currenty expects the noise floor calibration 
to finish within 300ms (ath5k_hw_register_timeout) and then we give it 
another 20ms for the noise floor value to settle ( for (i = 20; i > 0; i--) 
mdelay(1); ) that worked well for older chipsets, but also for 5424 this 
results in more noise floor calibration timeouts.

bruno

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Bob Copeland]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On Tue, Feb 26, 2008 at 11:13:34AM +0900, bruno randolf wrote:
> so when is AR5K_PHY_AGCCTL_CAL getting cleared?

Actually in my trace I didn't see it resetting on its own.  The only
reads with _CAL == 0 happen right after a write that clears it.  
(See attached from grep "^[RW].*AGCCTL").  It would be nice if my
mmio trace had timing info...

On the other hand, it must happen or else ath5k_hw_reset would never
work on the 5424.

> the HAL enables noise floor calibration, and reads the result on then next 
> calibration interval, then enables calibration again and reads on the next... 
> i think that way it makes sure there is always enough time for the noise 
> floor calibration to take place in the mean time (the calibration can only 
> happen when the channel is otherwise silent).

Good to know.  I guess calibration timeouts could also be symptomatic of
general problems as well.  Incidentally, in my current session I've
disabled G mode from the capabilities set.  All is not well though, I'll
get a run of:

ath0: invalid Michael MIC in data frame from 00:1a:70:da:a9:cb
ath0: invalid Michael MIC in data frame from 00:1a:70:da:a9:cb
ath0: invalid Michael MIC in data frame from 00:1a:70:da:a9:cb
ath0: deauthenticate(reason=14)

(then it reassociates)

> another 20ms for the noise floor value to settle ( for (i = 20; i > 0; i--) 
> mdelay(1); ) that worked well for older chipsets, but also for 5424 this 
> results in more noise floor calibration timeouts.

Sounds plausible, though I didn't have a lot of success bumping up the
timeout there.  But then, I have no idea what I'm doing so I'll test any 
patches :)

-- 
Bob Copeland %% www.bobcopeland.com 

[-- Attachment #2: agc.txt --]
[-- Type: text/plain, Size: 5565 bytes --]

W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL                ................1..111.1...11... (ath_hal_reset)
W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL                ................1..111.1...11.11 (ath_hal_reset)
R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL                ................1..111.1...11.1. (ath_hal_reset)
W: 0x9860 = 0x00009d10 - AR5K_PHY_AGCCTL                ................1..111.1...1.... (ath_hal_reset)
R: 0x9860 = 0x00009d10 - AR5K_PHY_AGCCTL                ................1..111.1...1.... (ath_hal_reset)
W: 0x9860 = 0x00009d13 - AR5K_PHY_AGCCTL                ................1..111.1...1..11 (ath_hal_reset)
R: 0x9860 = 0x00009d12 - AR5K_PHY_AGCCTL                ................1..111.1...1..1. (ath_hal_reset)
W: 0x9860 = 0x00009d10 - AR5K_PHY_AGCCTL                ................1..111.1...1.... (ath_hal_reset)
R: 0x9860 = 0x00009d10 - AR5K_PHY_AGCCTL                ................1..111.1...1.... (ath_hal_reset)
W: 0x9860 = 0x00009d13 - AR5K_PHY_AGCCTL                ................1..111.1...1..11 (ath_hal_reset)
R: 0x9860 = 0x00009d12 - AR5K_PHY_AGCCTL                ................1..111.1...1..1. (ath_hal_reset)

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/26, bruno randolf <bruno@thinktube.com>:
> On Tuesday 26 February 2008 00:43:07 Bob Copeland wrote:
>  > > > Hey, that's a good clue... I just switched over to b-only and it seems
>  > > > to be much more stable.
>  >
>  > ...or not.  I still got some calibration errors last night in b-mode.  Just
>  > so we're on the same page, I see things in the dmesg like:
>  >
>  > ath0: failed to restore operational channel after scan
>  > ath5k phy0: calibration timeout (2412 MHz)
>  > ath5k phy0: ath5k_chan_set: unable to reset channel (2412 Mhz)
>  > ath0: failed to set freq to 2412 MHz for scan
>  > ath5k phy0: calibration timeout (2417 MHz)
>  >
>  > > If i'm correct you should get 4-7Mbit/sec @ 11Mbit. Plz let me know if
>  > > you have some results, meanwhile i'll try to figure out the i/q
>  > > calibration algo (we are ok for noise floor calibration i believe).
>  >
>  > One thing I noticed from my traces is that the binary driver sets
>  > bits AR5K_PHY_AGCCTL_NF | AR5K_PHY_AGCCTL_CAL in AR5K_PHY_AGCCTL.
>  > Then it makes a whole lot of misc register writes, then re-reads
>  > AR5K_PHY_AGCCTL; in that time only the noise floor bit got cleared
>  > but _CAL is still high.  Dunno if that means anything to you or not.
>  >
>  > e.g:
>  >
>  > R: 0x9860 = 0x00009d18 - AR5K_PHY_AGCCTL
>  > W: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL  <-- set (_CAL | _NF)
>  > W: 0x1000 = 0x00000001 - AR5K_DCU_QCUMASK_BASE
>  > W: 0x1004 = 0x00000002 - unknown
>  > W: 0x1008 = 0x00000004 - unknown
>  > [... lots more writes to DCU & IMR regs ...]
>  > W: 0x00a0 = 0x00080965 - AR5K_PIMR
>  > R: 0x00ac = 0x00000000 - AR5K_SIMR2
>  > W: 0x00ac = 0x00070000 - AR5K_SIMR2
>  > R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
>  > R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
>  > R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
>  > R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
>  > R: 0x9860 = 0x00009d1b - AR5K_PHY_AGCCTL
>  > R: 0x9860 = 0x00009d1a - AR5K_PHY_AGCCTL  <-- _NF cleared
>  >
>  > Maybe a red herring as obviously the current method of doing things
>  > works for me sometimes...
>
>
> so when is AR5K_PHY_AGCCTL_CAL getting cleared?
>
>  i'm not sure if that is related because the error message you are seeing is
>  not due to noise floor calibration timeout but because AR5K_PHY_AGCCTL_CAL is
>  not cleared in the time that ath5k expects, but i want to mention it anyways:
>
>  the HAL enables noise floor calibration, and reads the result on then next
>  calibration interval, then enables calibration again and reads on the next...
>  i think that way it makes sure there is always enough time for the noise
>  floor calibration to take place in the mean time (the calibration can only
>  happen when the channel is otherwise silent).
>
>  if i understand correctly, ath5k currenty expects the noise floor calibration
>  to finish within 300ms (ath5k_hw_register_timeout) and then we give it
>  another 20ms for the noise floor value to settle ( for (i = 20; i > 0; i--)
>  mdelay(1); ) that worked well for older chipsets, but also for 5424 this
>  results in more noise floor calibration timeouts.
>
>
>  bruno
>

There are 3 types of calibration, noise floor calibration (which gets
us the noise floor value), I/Q calibration (which fixes QAM
constellation on OFDM rates) and the _CAL bit which is another kind of
calibration we don't know about (i'll check out Atheros patent docs
again, they might give us a clue)... I know that NF and I/Q
calibration must be done periodically (i also had an idea about
forcing I/Q calibration on certain rate changes since QAM
constellation changes) but i have no idea about _CAL stuff...

Noise floor calibration is not the problem, I/Q calibration is where
we are failing, that's why we have problems in OFDM rates. Even in
madwifi we get weird noise floor values sometimes (and calibration
function returns failure sometimes). We might have to add some extra
time for noise floor calibration, HAL has one function (phy_calibrate)
for both of them (I/Q + noise floor), i guess we can treat them in a
different way in our implementation.

Anyway we don't get a NF calibration timeout but we get timeout for
_CAL stuff and since HAL doesn't seem to check for it (it only sets
that bit during reset and during phy_calibrate but doesn't check for
it) we can also skip the test i guess...

Bob try removing this and see what happens (in terms of
performance/stability)...

	if (ath5k_hw_register_timeout(ah, AR5K_PHY_AGCCTL,
			AR5K_PHY_AGCCTL_CAL, 0, false)) {
		ATH5K_ERR(ah->ah_sc, "calibration timeout (%uMHz)\n",
			channel->center_freq);
		return -EAGAIN;
	}


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Bob Copeland]

On Tue, Feb 26, 2008 at 05:57:22AM +0200, Nick Kossifidis wrote:
> Bob try removing this and see what happens (in terms of
> performance/stability)...
> 
> 	if (ath5k_hw_register_timeout(ah, AR5K_PHY_AGCCTL,
> 			AR5K_PHY_AGCCTL_CAL, 0, false)) {
> 		ATH5K_ERR(ah->ah_sc, "calibration timeout (%uMHz)\n",
> 			channel->center_freq);
> 		return -EAGAIN;
> 	}

It doesn't really have an effect, because even without this, the noise
floor calibration fails anyway.  I suspect by the time it gets in this
state the device is just hung generally.

So, I can't sustain a connection long enough to do iperf.  I got numbers
from madwifi though :)

Seems when it gets a big hunk of data the device barfs.  When I'm doing
something like ssh or just browsing a little, it's annoying but still
useable because most of the time it'll disassociate with the AP for a
few seconds, reassociate, and it's working again.

-- 
Bob Copeland %% www.bobcopeland.com 

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Luis R. Rodriguez]

On Sun, Feb 24, 2008 at 1:58 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> 2008/2/24, Bob Copeland <me@bobcopeland.com>:
>
>
> > On Sun, Feb 24, 2008 at 06:45:33AM +0200, Nick Kossifidis wrote:
>  >  > >  +       /* Identify PCI-E cards */
>  >  > >  +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  >  > >  +       srev >= AR5K_SREV_VER_AR5416) {
>  >  > >  +               ah->ah_pcie = true;

struct pci_dev now has member,  "is_pcie" which could be used here I
think instead. No need to add to ath5k_hw the ah_pcie then.

  Luis

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/27, Luis R. Rodriguez <mcgrof@gmail.com>:
> On Sun, Feb 24, 2008 at 1:58 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>  > 2008/2/24, Bob Copeland <me@bobcopeland.com>:
>  >
>  >
>  > > On Sun, Feb 24, 2008 at 06:45:33AM +0200, Nick Kossifidis wrote:
>  >  >  > >  +       /* Identify PCI-E cards */
>  >  >  > >  +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  >  >  > >  +       srev >= AR5K_SREV_VER_AR5416) {
>  >  >  > >  +               ah->ah_pcie = true;
>
>
> struct pci_dev now has member,  "is_pcie" which could be used here I
>  think instead. No need to add to ath5k_hw the ah_pcie then.
>
>
>   Luis
>

What if there is a pci-e card in the future that needs _PCI bit ? I
guess for now we can work on with srevs to be safe but it's good to
know of that feature, thanx ;-)


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Luis R. Rodriguez]

On Wed, Feb 27, 2008 at 12:54 AM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> 2008/2/27, Luis R. Rodriguez <mcgrof@gmail.com>:
>
>
> > On Sun, Feb 24, 2008 at 1:58 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>  >  > 2008/2/24, Bob Copeland <me@bobcopeland.com>:
>  >  >
>  >  >
>  >  > > On Sun, Feb 24, 2008 at 06:45:33AM +0200, Nick Kossifidis wrote:
>  >  >  >  > >  +       /* Identify PCI-E cards */
>  >  >  >  > >  +       if((srev >= AR5K_SREV_VER_AR2424 && srev <= AR5K_SREV_VER_AR5424) ||
>  >  >  >  > >  +       srev >= AR5K_SREV_VER_AR5416) {
>  >  >  >  > >  +               ah->ah_pcie = true;
>  >
>  >
>  > struct pci_dev now has member,  "is_pcie" which could be used here I
>  >  think instead. No need to add to ath5k_hw the ah_pcie then.
>  >
>  >
>  >   Luis
>  >
>
>  What if there is a pci-e card in the future that needs _PCI bit ?

I don't follow, the member is for struct pci_dev so its either a pci
or pci-express device. If you mean what if we later have some srev in
the range that is not pci-e, well then we'd use srevs wouldn't we?

> I guess for now we can work on with srevs to be safe but it's good to
>  know of that feature, thanx ;-)

The point is that if there is already a variable we can use to detect
if a device is pci-e then we shouldn't introduce any other new ones.

   Luis

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[John W. Linville]

On Wed, Feb 27, 2008 at 08:30:18AM -0500, Luis R. Rodriguez wrote:
> On Wed, Feb 27, 2008 at 12:54 AM, Nick Kossifidis <mickflemm@gmail.com> wrote:

> >  What if there is a pci-e card in the future that needs _PCI bit ?
> 
> I don't follow, the member is for struct pci_dev so its either a pci
> or pci-express device. If you mean what if we later have some srev in
> the range that is not pci-e, well then we'd use srevs wouldn't we?
> 
> > I guess for now we can work on with srevs to be safe but it's good to
> >  know of that feature, thanx ;-)
> 
> The point is that if there is already a variable we can use to detect
> if a device is pci-e then we shouldn't introduce any other new ones.

ACK for Luis's point.

-- 
John W. Linville
linville@tuxdriver.com

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Nick Kossifidis]

2008/2/27, John W. Linville <linville@tuxdriver.com>:
> On Wed, Feb 27, 2008 at 08:30:18AM -0500, Luis R. Rodriguez wrote:
>  > On Wed, Feb 27, 2008 at 12:54 AM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>
>
> > >  What if there is a pci-e card in the future that needs _PCI bit ?
>  >
>  > I don't follow, the member is for struct pci_dev so its either a pci
>  > or pci-express device. If you mean what if we later have some srev in
>  > the range that is not pci-e, well then we'd use srevs wouldn't we?
>  >
>  > > I guess for now we can work on with srevs to be safe but it's good to
>  > >  know of that feature, thanx ;-)
>  >
>  > The point is that if there is already a variable we can use to detect
>  > if a device is pci-e then we shouldn't introduce any other new ones.
>
>
> ACK for Luis's point.
>
>

Sorry for not being clear on my previous post...

We don't have access to pci_dev on nic_wakeup and other functions that
is possible to need that info in the future, so having a local ah_pcie
variable is more flexible. I don't think it's usefull to rewrite the
whole thing so that these functions take pci_dev for argument. So
"introducing a new variable" is not that bad IMHO.

Have in mind that there are also ar5k devices that are not attached an
a pci or pci-e bus at all (ahb) that we want to support in the future.
Right now hw_attach does not make use of any pci-related stuff, it
only takes ath5k_softc for argument and mac_version and i intend to
keep it that way to be abstract (when we start supporting WiSOC, we
'll only have to tweak base.c/base.h as in MadWiFi, not hw-related
code and our code will be less noisy and easier to maintain).

The way i see it, if we make use of is_pcie we'll do it during
pci_probe and pass a bool to hw_attach if it's pcie. This will save us
from passing pci_dev directly to hw_attach that 'll create noise
during ahb implementation (we 'll need to make an abstract dev
structure for both pci/ahb to pass to hw_attach etc).

So all i'm saying is that i want to think about it a little more and
i'll come up with a patch for it. What i also want to see is check
traces from my 5418 card which is also pci-e and see if it sets _PCI
bit, if it does current flag will have no meaning (eg. not setting
_PCI bit wont be a property of pci-e cards but only of a specific
srev-range) so i'll submit a patch that eg. renames ah_pcie to
"ah_no_pcibit_durring_reset" or something like that.

-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 6/8] ath5k: Fixes for PCI-E cards

[Luis R. Rodriguez]

On Wed, Feb 27, 2008 at 1:44 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> 2008/2/27, John W. Linville <linville@tuxdriver.com>:
>
>
> > On Wed, Feb 27, 2008 at 08:30:18AM -0500, Luis R. Rodriguez wrote:
>  >  > On Wed, Feb 27, 2008 at 12:54 AM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>  >
>  >
>  > > >  What if there is a pci-e card in the future that needs _PCI bit ?
>  >  >
>  >  > I don't follow, the member is for struct pci_dev so its either a pci
>  >  > or pci-express device. If you mean what if we later have some srev in
>  >  > the range that is not pci-e, well then we'd use srevs wouldn't we?
>  >  >
>  >  > > I guess for now we can work on with srevs to be safe but it's good to
>  >  > >  know of that feature, thanx ;-)
>  >  >
>  >  > The point is that if there is already a variable we can use to detect
>  >  > if a device is pci-e then we shouldn't introduce any other new ones.
>  >
>  >
>  > ACK for Luis's point.
>  >
>  >
>
>  Sorry for not being clear on my previous post...
>
>  We don't have access to pci_dev on nic_wakeup and other functions that
>  is possible to need that info in the future,

Yes we do, I'll post a patch based on yours, it needs testing on
PCI-E. Also noticed what may have been a typo on another patch, will
comment on that one next. I'll just post my series based on yours.

>  Have in mind that there are also ar5k devices that are not attached an
>  a pci or pci-e bus at all (ahb) that we want to support in the future.
>  Right now hw_attach does not make use of any pci-related stuff, it
>  only takes ath5k_softc for argument and mac_version and i intend to
>  keep it that way to be abstract (when we start supporting WiSOC, we
>  'll only have to tweak base.c/base.h as in MadWiFi, not hw-related
>  code and our code will be less noisy and easier to maintain).

We can take care of ahb when we get there.

>  The way i see it, if we make use of is_pcie we'll do it during
>  pci_probe and pass a bool to hw_attach if it's pcie. This will save us
>  from passing pci_dev directly to hw_attach that 'll create noise
>  during ahb implementation (we 'll need to make an abstract dev
>  structure for both pci/ahb to pass to hw_attach etc).

Its not that complicated, you'll see.

>  So all i'm saying is that i want to think about it a little more and
>  i'll come up with a patch for it. What i also want to see is check
>  traces from my 5418 card which is also pci-e and see if it sets _PCI
>  bit, if it does current flag will have no meaning (eg. not setting
>  _PCI bit wont be a property of pci-e cards but only of a specific
>  srev-range) so i'll submit a patch that eg. renames ah_pcie to
>  "ah_no_pcibit_durring_reset" or something like that.

Point taken, we can fix this if you find that.

  Luis