Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 27 May 2023 16:45:21 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Amisha.Patel@microchip.com
Cc:     linux-wireless@vger.kernel.org, Ajay.Kathat@microchip.com,
        Claudiu.Beznea@microchip.com
Subject: Re: [PATCH] wifi: wilc1000: add SPI commands retry mechanism
Message-ID: <ZHIXgbSw0kx+YnXG@corigine.com>
References: <20230525181739.30063-1-amisha.patel@microchip.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230525181739.30063-1-amisha.patel@microchip.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?jwDoiD3iF2CUQQKJB9FyYs0P0DYeoYrm2G9xVxIISALjOq8c1YxAAwBWtwMF?=
 =?us-ascii?Q?/4c0hvhAOUMKYoOTXrAlo4VWxjhWDBQ2cRGLAqyPsN/252XOI8eCReoKRFlk?=
 =?us-ascii?Q?lu/yHuDG5uwR82urxQ30O48DRHbfJwEE31ymEsnWnnAd6uyw9BYMOXxm45xz?=
 =?us-ascii?Q?efrbHupHr3jWG/suPyQyJogAZ9RqiysVIAiaZhw+Pzsf+sK+XbUP5LlLCWTo?=
 =?us-ascii?Q?CXEmUibDeinLA+L+eo8v1+trD4iwgL5Dhlh6OU/faxznbNimt3MiC/i1GGB/?=
 =?us-ascii?Q?FKjE8d2QPe8hk/9rYvNoRNZ7rBJZ9ZXSo7gxSoZkkNtOqIlg7MFNFllu7U5Y?=
 =?us-ascii?Q?DEcH5FFrQNxSecYTHnkW2aXSjsbRaXl7xzuqtjIRn13/1bcxkSmbQLI/1QdN?=
 =?us-ascii?Q?Q0sYYraCE/fMOK4hR8hgxbCfQ0oof4K7eEyMpI96bDWBI891bK30CNwVm7Ha?=
 =?us-ascii?Q?QtvuPOi6EU4WQzYvabnkyREBOR8MnQOwnDBteYraR4qKz67LQiLWcRIKgFTq?=
 =?us-ascii?Q?mT42dxXgSRtdpOAYmZ84uBCBV6n3XvBK8q0RMd7NWjnVJOkllGqGT6HLG5AL?=
 =?us-ascii?Q?FB1Vc+klF/rXDRHM7qLwmL0JEMbTOHVAKDfZwtCMKWrQDOyFYtr4DhlsXLv5?=
 =?us-ascii?Q?BY46q71jDGQrbHoZpN4htzYk4HzoPIztBr8sSEnBCOSIQpfIMQ9vUVz+C3+p?=
 =?us-ascii?Q?s36VBAB2auRHte+pY9YHIEJ9P2bYspkqsNTs+5lZkozh25Ik69t4bGncrbIZ?=
 =?us-ascii?Q?9CtFwewbhBs4xI+6wN0MKrDO4dtFqif6n8W8BMsOIhonBURC4bMAGs3cr+MG?=
 =?us-ascii?Q?Wp37o0kW6Zpd4rAJCUML9LH6u85zbxP0aqObzzeNLxVU8/McdFAkXiCiAuzV?=
 =?us-ascii?Q?aWpIJFXuXd1UYaMfr9Ym5D7hfGYJWV4mBFe5duIWHKPYqP5gRJLAaFjc+xXT?=
 =?us-ascii?Q?mBYdCGj/Qz15Po6gJn5/HcDhFcrLafxX0cfyK6nr8nD175OE+0pw8NLP9lac?=
 =?us-ascii?Q?H+nvsZCV0HuczhdidH1j6mc0zZQYV2byv1x37PkB26sxY8/KgGJOkWlRzd0B?=
 =?us-ascii?Q?qJeLkw2B1J6xWnHQDr2629gOxkL17ZO33SS2VoyRe+JJxPKdTc5FySeE4bg9?=
 =?us-ascii?Q?q5jNGX5mn/cEefSPaOSmVY4rAtu61UWz4l1jO5Du2XwWkKRMV3fYH3SEabmx?=
 =?us-ascii?Q?A+8JDpFB58F2rua4FwTM21XHFVPdYaFl0zRvs1USQ3O/SNLQeDnPknwnE9PT?=
 =?us-ascii?Q?iMxW92LZMivqftWQzgavq6S7Q36V2kwI6YnagnXz9JULe4sFGoisGgT9OsHS?=
 =?us-ascii?Q?IJx3CsN1mjufT5qwMlu9IV6D3bcmVKDjvYMDwUOoz4M1oYKnkPAvpW02z3SL?=
 =?us-ascii?Q?rQOfAmniR0n4Qar9AE8k6Mp626gcBFqNt5In7lnARc0/CHdaV7kTC6alPXxb?=
 =?us-ascii?Q?6kMl2r8FYUQvj8yo6NqXwQ8rTUUUX7+6DcIGHWJ9R5isPAQoJbb5A6d3X3RZ?=
 =?us-ascii?Q?2rUhRjKZbZszdmf4if2n/qU+OmPul2BhqnNWMLAW1RzGT6qj9aVTUedlT0Sz?=
 =?us-ascii?Q?dYOsTH5/5b+6QFmgRuQPHWbjJd08qCSkXyZXyFLsrbveIbYpUWWZT9943SPn?=
 =?us-ascii?Q?7YCqmMqtJZ4YpSHAPpOkKL86P2X4zDoIPr9M8X/TqPV2EF6C1RgJK8ACj0xV?=
 =?us-ascii?Q?NKVcTA=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8408cc22-5331-40d9-7f66-08db5ec1034d
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 May 2023 14:45:28.3157
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zj0x31lx0wgG1kznleB9ta5ab18Rvb0Gt8T0F00m8CnwWpKWhZm/ys/BGN22ugtcQVpDx7KJrCq8k5AOo2ijD2LonA23pu5O5sFTj5hyjwQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR13MB4741
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

On Thu, May 25, 2023 at 06:17:48PM +0000, Amisha.Patel@microchip.com wrote:
> From: Amisha Patel <amisha.patel@microchip.com>
> 
> In some situations like, chip wake-up with powersave enabled, SPI
> commands are failing temporarily. Reissuing commands after reset helps
> to overcome the failure. So, add the retry limit and reset command
> sequence API for read/write SPI commands.
> 
> Signed-off-by: Amisha Patel <amisha.patel@microchip.com>

Hi Amisha,

thanks for your patch.
Some minor nits from my side.

...

>  static int wilc_spi_read_reg(struct wilc *wilc, u32 addr, u32 *data)
>  {
>  	struct spi_device *spi = to_spi_device(wilc->dev);
>  	int result;
>  	u8 cmd = CMD_SINGLE_READ;
>  	u8 clockless = 0;
> +	u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>  
> -	if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
> +	if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
>  		/* Clockless register */
>  		cmd = CMD_INTERNAL_READ;
>  		clockless = 1;
>  	}
> -
> +	

nit: trailing tabs on the line above

> +retry:
>  	result = wilc_spi_single_read(wilc, cmd, addr, data, clockless);
>  	if (result) {
>  		dev_err(&spi->dev, "Failed cmd, read reg (%08x)...\n", addr);
> -		return result;
> +		

Ditto

> +		/* retry is not applicable for clockless registers */
> +		if (clockless || !retry_limit)
> +			return result;
> +
> +		wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> +		retry_limit--;
> +		goto retry;
>  	}
>  
>  	le32_to_cpus(data);
> @@ -858,14 +881,22 @@ static int wilc_spi_read(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>  {
>  	struct spi_device *spi = to_spi_device(wilc->dev);
>  	int result;
> +	u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>  
>  	if (size <= 4)
>  		return -EINVAL;
>  
> +retry:
>  	result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_READ, addr, buf, size);
>  	if (result) {
>  		dev_err(&spi->dev, "Failed cmd, read block (%08x)...\n", addr);
> -		return result;
> +		

Ditto

> +		if (!retry_limit)
> +			return result;
> +
> +		wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> +		retry_limit--;
> +		goto retry;
>  	}
>  
>  	return 0;
> @@ -875,11 +906,19 @@ static int spi_internal_write(struct wilc *wilc, u32 adr, u32 dat)
>  {
>  	struct spi_device *spi = to_spi_device(wilc->dev);
>  	int result;
> +	u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>  
> +retry:
>  	result = wilc_spi_write_cmd(wilc, CMD_INTERNAL_WRITE, adr, dat, 0);
>  	if (result) {
>  		dev_err(&spi->dev, "Failed internal write cmd...\n");
> -		return result;
> +		

Ditto

> +		if (!retry_limit)
> +			return result;
> +
> +		wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
> +		retry_limit--;
> +		goto retry;
>  	}
>  
>  	return 0;
> @@ -890,12 +929,20 @@ static int spi_internal_read(struct wilc *wilc, u32 adr, u32 *data)
>  	struct spi_device *spi = to_spi_device(wilc->dev);
>  	struct wilc_spi *spi_priv = wilc->bus_data;
>  	int result;
> +	u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>  
> +retry:
>  	result = wilc_spi_single_read(wilc, CMD_INTERNAL_READ, adr, data, 0);
>  	if (result) {
>  		if (!spi_priv->probing_crc)
>  			dev_err(&spi->dev, "Failed internal read cmd...\n");
> -		return result;
> +			

Ditto

> +		if (!retry_limit)
> +			return result;
> +
> +		wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
> +		retry_limit--;
> +		goto retry;
>  	}
>  
>  	le32_to_cpus(data);
> @@ -915,19 +962,26 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data)
>  	int result;
>  	u8 cmd = CMD_SINGLE_WRITE;
>  	u8 clockless = 0;
> -
> -	if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
> +	u8 retry_limit = SPI_RETRY_MAX_LIMIT;
> +	

Ditto

> +	if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
>  		/* Clockless register */
>  		cmd = CMD_INTERNAL_WRITE;
>  		clockless = 1;
>  	}
>  
> +retry:
>  	result = wilc_spi_write_cmd(wilc, cmd, addr, data, clockless);
>  	if (result) {
>  		dev_err(&spi->dev, "Failed cmd, write reg (%08x)...\n", addr);
> -		return result;
> -	}
> +		

Ditto

> +		if (clockless || !retry_limit)
> +			return result;
>  
> +		wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> +		retry_limit--;
> +		goto retry;
> +	}
>  	return 0;
>  }
>  
> @@ -981,6 +1035,7 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>  {
>  	struct spi_device *spi = to_spi_device(wilc->dev);
>  	int result;
> +	u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>  
>  	/*
>  	 * has to be greated than 4
> @@ -988,11 +1043,12 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>  	if (size <= 4)
>  		return -EINVAL;
>  
> +retry:
>  	result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_WRITE, addr, NULL, size);
>  	if (result) {
>  		dev_err(&spi->dev,
>  			"Failed cmd, write block (%08x)...\n", addr);
> -		return result;
> +		goto fail;
>  	}
>  
>  	/*
> @@ -1001,13 +1057,27 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>  	result = spi_data_write(wilc, buf, size);
>  	if (result) {
>  		dev_err(&spi->dev, "Failed block data write...\n");
> -		return result;
> +		goto fail;
>  	}
>  
>  	/*
>  	 * Data response
>  	 */
> -	return spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
> +	result = spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
> +	if (result) {
> +		dev_err(&spi->dev, "Failed block data rsp...\n");
> +		goto fail;
> +	}
> +
> +	return 0;
> +
> +fail:
> +	if (result && retry_limit) {

nit: is result always non-zero here?

> +		wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> +		retry_limit--;
> +		goto retry;
> +	}
> +	return result;
>  }
>  
>  /********************************************