Message-ID: <dcbbd9fce1f12393c5e2cd1271c60507a8475bc6.camel@egauge.net>
Subject: Re: RFC: wilc1000 module parameter to disable chip sleep
From:   David Mosberger-Tang <davidm@egauge.net>
To:     Ajay.Kathat@microchip.com
Cc:     Claudiu.Beznea@microchip.com, linux-wireless@vger.kernel.org
Date:   Thu, 09 Dec 2021 12:28:11 -0700
In-Reply-To: <efee7360e8be88d58251e38c848637c8163bdf19.camel@egauge.net>
References: <0baed35e98144bc7e29681264caf954b659cd481.camel@egauge.net>
         <4bf158ac-18d0-6feb-fbef-dc0739d74487@microchip.com>
         <a03e133e46ff305dbad796913b79c994c91581d5.camel@egauge.net>
         <efee7360e8be88d58251e38c848637c8163bdf19.camel@egauge.net>
Organization: eGauge Systems LLC
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2021-12-09 at 12:12 -0700, David Mosberger-Tang wrote:
> On Thu, 2021-12-09 at 11:15 -0700, David Mosberger-Tang wrote:
> 
> > > Did you test by enabling the power-save mode with "wpa_supplicant" or 
> > > using "iw" command. For power consumption measurement, please try by 
> > > disabling the PSM mode.
> > 
> > No, I have not played with power-saving mode.  There are some disconcerting
> > messages when turning PSM on:
> > 
> > [ 1764.070375] wilc1000_spi spi1.0: Failed cmd, cmd (ca), resp (00)
> > [ 1764.076403] wilc1000_spi spi1.0: Failed cmd, read reg (000013f4)...
> 
> As far as I can see, chip_wakeup() can trigger this legitimately:
> 
> 		do {
> 			h->hif_read_reg(wilc, WILC_SPI_WAKEUP_REG, &reg);
> 			h->hif_write_reg(wilc, WILC_SPI_WAKEUP_REG,
> 					 reg | WILC_SPI_WAKEUP_BIT);
> 			h->hif_write_reg(wilc, WILC_SPI_WAKEUP_REG,
> 					 reg & ~WILC_SPI_WAKEUP_BIT);
> 
> 			do {
> 				usleep_range(2000, 2500);
> 				wilc_get_chipid(wilc, true);
> 			} while (wilc_get_chipid(wilc, true) == 0);
> 		} while (wilc_get_chipid(wilc, true) == 0);
> 
> Actually, the above looks rather gross.  wilc_get_chip() reads both WILC_CHIPID
> and WILC_RF_REVISION_ID and we do this at least three times in a row on each
> chip_wakeup().  Wow.  Is this really necessary?

Looks like I'm on to something.  If I replace the gross code with:

		h->hif_read_reg(wilc, WILC_SPI_WAKEUP_REG, &reg);
		h->hif_write_reg(wilc, WILC_SPI_WAKEUP_REG, reg | WILC_SPI_WAKEUP_BIT);
		h->hif_write_reg(wilc, WILC_SPI_WAKEUP_REG, reg & ~WILC_SPI_WAKEUP_BIT);

		while (1) {
			if (wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid) == 0)
				break;
			usleep_range(2000, 2500);
		}

Look what happens to TX throughput:

[  4]   0.00-120.00 sec   142 MBytes  9.92 Mbits/sec    6             sender

This is with power_save mode turned on.  Almost exactly 3 times faster than the
3.33 Mbits/sec I was seeing before.

  --david