Message-ID: <bbaf1b15-2d0e-4699-91cc-17fa7a18559b@bootlin.com>
Date: Thu, 22 Aug 2024 14:10:25 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: Marek Vasut <marex@...x.de>, linux-wireless@...r.kernel.org
Cc: Ajay Singh <ajay.kathat@...rochip.com>,
 "David S. Miller" <davem@...emloft.net>,
 Adham Abozaeid <adham.abozaeid@...rochip.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>, Conor Dooley
 <conor+dt@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Kalle Valo <kvalo@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
 netdev@...r.kernel.org, Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support

Hello Marek,

I was coincidentally working on adding wilc3000 support upstream too. My work is
also based on downstream tree, so my comments will likely reflect the reworks I
was doing or intended to do.
For the record, I have some wilc1000 and wilc3000 modules, in both  sdio and spi
setups.

On 8/21/24 20:42, Marek Vasut wrote:
> From: Ajay Singh <ajay.kathat@...rochip.com>

[...]

>  	if (!resume) {
> -		ret = wilc_sdio_read_reg(wilc, WILC_CHIPID, &chipid);
> -		if (ret) {
> -			dev_err(&func->dev, "Fail cmd read chip id...\n");
> +		chipid = wilc_get_chipid(wilc, true);
> +		if (is_wilc3000(chipid)) {
> +			wilc->chip = WILC_3000;
> +		} else if (is_wilc1000(chipid)) {
> +			wilc->chip = WILC_1000;
> +		} else {
> +			dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
>  			return ret;
>  		}

I wonder if this additional enum (enum wilc_chip_type)  is really useful. We
already store the raw chipid, which just needs to be masked to know about the
device type. We should likely store one or the other but not both, otherwise we
may just risk to create desync without really saving useful info.

Also, this change makes wilc1000-sdio failing to build as module (missing symbol
export on wilc_get_chipid)

[...]

> -	/* select VMM table 0 */
> -	if (val & SEL_VMM_TBL0)
> -		reg |= BIT(5);
> -	/* select VMM table 1 */
> -	if (val & SEL_VMM_TBL1)
> -		reg |= BIT(6);
> -	/* enable VMM */
> -	if (val & EN_VMM)
> -		reg |= BIT(7);
> +	if (wilc->chip == WILC_1000) {

wilc1000 should likely remain the default/fallback ?

[...]

> @@ -1232,10 +1234,7 @@ static int wilc_validate_chipid(struct wilc *wilc)
>  		dev_err(&spi->dev, "Fail cmd read chip id...\n");
>  		return ret;
>  	}
> -	if (!is_wilc1000(chipid)) {
> -		dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
> -		return -ENODEV;
> -	}
> +

Instead of dropping any filtering (and then making the function name become
irrelevant), why not ensuring that it is at least either a wilc1000 or a wilc3000 ?

>  	return 0;
>  }
>  
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 533939e71534a..a7cc8c0ea5de4 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -555,7 +555,7 @@ static struct rxq_entry_t *wilc_wlan_rxq_remove(struct wilc *wilc)
>  	return rqe;
>  }

[...]

> +static int chip_allow_sleep_wilc3000(struct wilc *wilc)
> +{
> +	u32 reg = 0;
> +	int ret;
> +	const struct wilc_hif_func *hif_func = wilc->hif_func;
> +
> +	if (wilc->io_type == WILC_HIF_SDIO) {
> +		ret = hif_func->hif_read_reg(wilc, WILC_SDIO_WAKEUP_REG, &reg);
> +		if (ret)
> +			return ret;
> +		ret = hif_func->hif_write_reg(wilc, WILC_SDIO_WAKEUP_REG,
> +					      reg & ~WILC_SDIO_WAKEUP_BIT);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = hif_func->hif_read_reg(wilc, WILC_SPI_WAKEUP_REG, &reg);
> +		if (ret)
> +			return ret;
> +		ret = hif_func->hif_write_reg(wilc, WILC_SPI_WAKEUP_REG,
> +					      reg & ~WILC_SPI_WAKEUP_BIT);
> +		if (ret)
> +			return ret;
>  	}
> +	return 0;
> +}
> +
> +void chip_allow_sleep(struct wilc *wilc)
> +{
> +	if (wilc->chip == WILC_1000)
> +		chip_allow_sleep_wilc1000(wilc);
> +	else
> +		chip_allow_sleep_wilc3000(wilc);
>  }
>  EXPORT_SYMBOL_GPL(chip_allow_sleep);
>  
> -void chip_wakeup(struct wilc *wilc)
> +static void chip_wakeup_wilc1000(struct wilc *wilc)
>  {
>  	u32 ret = 0;
>  	u32 clk_status_val = 0, trials = 0;
> @@ -627,15 +662,15 @@ void chip_wakeup(struct wilc *wilc)
>  	if (wilc->io_type == WILC_HIF_SDIO) {
>  		wakeup_reg = WILC_SDIO_WAKEUP_REG;
>  		wakeup_bit = WILC_SDIO_WAKEUP_BIT;
> -		clk_status_reg = WILC_SDIO_CLK_STATUS_REG;
> -		clk_status_bit = WILC_SDIO_CLK_STATUS_BIT;
> +		clk_status_reg = WILC1000_SDIO_CLK_STATUS_REG;
> +		clk_status_bit = WILC1000_SDIO_CLK_STATUS_BIT;
>  		from_host_to_fw_reg = WILC_SDIO_HOST_TO_FW_REG;
>  		from_host_to_fw_bit = WILC_SDIO_HOST_TO_FW_BIT;
>  	} else {
>  		wakeup_reg = WILC_SPI_WAKEUP_REG;
>  		wakeup_bit = WILC_SPI_WAKEUP_BIT;
> -		clk_status_reg = WILC_SPI_CLK_STATUS_REG;
> -		clk_status_bit = WILC_SPI_CLK_STATUS_BIT;
> +		clk_status_reg = WILC1000_SPI_CLK_STATUS_REG;
> +		clk_status_bit = WILC1000_SPI_CLK_STATUS_BIT;
>  		from_host_to_fw_reg = WILC_SPI_HOST_TO_FW_REG;
>  		from_host_to_fw_bit = WILC_SPI_HOST_TO_FW_BIT;
>  	}
> @@ -674,12 +709,80 @@ void chip_wakeup(struct wilc *wilc)
>  	if (wilc->io_type == WILC_HIF_SPI)
>  		wilc->hif_func->hif_reset(wilc);
>  }
> +
> +static void chip_wakeup_wilc3000(struct wilc *wilc)
> +{
> +	u32 wakeup_reg_val, clk_status_reg_val, trials = 0;
> +	u32 wakeup_reg, wakeup_bit;
> +	u32 clk_status_reg, clk_status_bit;
> +	int wake_seq_trials = 5;
> +	const struct wilc_hif_func *hif_func = wilc->hif_func;
> +
> +	if (wilc->io_type == WILC_HIF_SDIO) {
> +		wakeup_reg = WILC_SDIO_WAKEUP_REG;
> +		wakeup_bit = WILC_SDIO_WAKEUP_BIT;
> +		clk_status_reg = WILC3000_SDIO_CLK_STATUS_REG;
> +		clk_status_bit = WILC3000_SDIO_CLK_STATUS_BIT;
> +	} else {
> +		wakeup_reg = WILC_SPI_WAKEUP_REG;
> +		wakeup_bit = WILC_SPI_WAKEUP_BIT;
> +		clk_status_reg = WILC3000_SPI_CLK_STATUS_REG;
> +		clk_status_bit = WILC3000_SPI_CLK_STATUS_BIT;
> +	}
> +
> +	hif_func->hif_read_reg(wilc, wakeup_reg, &wakeup_reg_val);
> +	do {
> +		hif_func->hif_write_reg(wilc, wakeup_reg, wakeup_reg_val |
> +							  wakeup_bit);
> +		/* Check the clock status */
> +		hif_func->hif_read_reg(wilc, clk_status_reg,
> +				       &clk_status_reg_val);
> +
> +		/* In case of clocks off, wait 1ms, and check it again.
> +		 * if still off, wait for another 1ms, for a total wait of 3ms.
> +		 * If still off, redo the wake up sequence
> +		 */
> +		while ((clk_status_reg_val & clk_status_bit) == 0 &&
> +		       (++trials % 4) != 0) {
> +			/* Wait for the chip to stabilize*/
> +			usleep_range(1000, 1100);
> +
> +			/* Make sure chip is awake. This is an extra step that
> +			 * can be removed later to avoid the bus access
> +			 * overhead
> +			 */
> +			hif_func->hif_read_reg(wilc, clk_status_reg,
> +					       &clk_status_reg_val);
> +		}
> +		/* in case of failure, Reset the wakeup bit to introduce a new
> +		 * edge on the next loop
> +		 */
> +		if ((clk_status_reg_val & clk_status_bit) == 0) {
> +			hif_func->hif_write_reg(wilc, wakeup_reg,
> +						wakeup_reg_val & (~wakeup_bit));
> +			/* added wait before wakeup sequence retry */
> +			usleep_range(200, 300);
> +		}
> +	} while ((clk_status_reg_val & clk_status_bit) == 0 && wake_seq_trials-- > 0);
> +	if (!wake_seq_trials)
> +		dev_err(wilc->dev, "clocks still OFF. Wake up failed\n");
> +}
> +
> +void chip_wakeup(struct wilc *wilc)
> +{
> +	if (wilc->chip == WILC_1000)
> +		chip_wakeup_wilc1000(wilc);
> +	else
> +		chip_wakeup_wilc3000(wilc);
> +}
>  EXPORT_SYMBOL_GPL(chip_wakeup);

This new support makes a few places in wlan.c, netdev.c and in bus files
(sdio.c, spi.c) install (sometimes big) branches on the device type (chip init,
sleep, wakeup, read interrupt, clear interrupt, txq handling, etc), because the
registers are different, the masks are different, the number of involved
registers may not be the same, wilc3000 may need more operations to perform the
same thing... I feel like it will make it harder in the long run to maintain the
driver, especially if some new variants are added later. Those branches tend to
show that some operations in those files are too specific to the targeted
device. I was examining the possibility to start creating device-type specific
files (wilc1000.c, wilc3000.c) and move those operations as "device-specific"
ops. Then wlan/netdev would call those chip-specific ops, which in turn may call
the hif_func ops. It may need some rework in the bus files to fit this new
hierarchy, but it may allow to keep netdev and wlan unaware of the device type,
and since wilc3000 has bluetooth, it may also make it easier to introduce the
corresponding support later. What do you think about it ? Ajay, any opinion on
this ?

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists