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:
>
>>> 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
> 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
