Message-ID: <efee7360e8be88d58251e38c848637c8163bdf19.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:12:04 -0700
In-Reply-To: <a03e133e46ff305dbad796913b79c994c91581d5.camel@egauge.net>
References: <0baed35e98144bc7e29681264caf954b659cd481.camel@egauge.net>
         <4bf158ac-18d0-6feb-fbef-dc0739d74487@microchip.com>
         <a03e133e46ff305dbad796913b79c994c91581d5.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 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?

In any case, for now, the below supresses the error messages:

diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index b778284247f7..516787aa4a23 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -498,7 +498,7 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
 	r = (struct wilc_spi_rsp_data *)&rb[cmd_len];
 	if (r->rsp_cmd_type != cmd) {
 		if (!spi_priv->probing_crc)
-			dev_err(&spi->dev,
+			dev_dbg(&spi->dev,
 				"Failed cmd, cmd (%02x), resp (%02x)\n",
 				cmd, r->rsp_cmd_type);
 		return -EINVAL;
@@ -754,7 +754,8 @@ static int wilc_spi_read_reg(struct wilc *wilc, u32 addr, u32 *data)
 
 	result = wilc_spi_single_read(wilc, cmd, addr, data, clockless);
 	if (result) {
-		dev_err(&spi->dev, "Failed cmd, read reg (%08x)...\n", addr);
+		/* Register reads may fail legitimately, e.g., during chip_wakeup(). */
+		dev_dbg(&spi->dev, "Failed cmd, read reg (%08x)...\n", addr);
 		return result;
 	}
 

Maybe a parameter should be added to hif_read_reg() to indicate whether failure is an option?

  --david