Re: RFC: wilc1000 module parameter to disable chip sleep
flat view
From: <Ajay.Kathat@microchip.com>
Date: 2021-12-10 09:05:23
On 10/12/21 00:42, David Mosberger-Tang wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe On Thu, 2021-12-09 at 11:15 -0700, David Mosberger-Tang wrote:quoted
quoted
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, ®); 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?
I believe you are using the old driver code so please check with latest code. 'chip_wakeup()' was changed recently so I don't think 'wilc_get_chipid()' call is valid anymore inside this API. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/wireless/microchip/wilc1000?h=v5.15&id=5bb9de8bcb18c38ea089a287b77944ef8ee71abd
quoted hunk ↗ jump to hunk
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?
I have not observed the failure log specifically during 'chip_wakeup' but if you see them often then we can add the information in comments. Regards, Ajay