diff mbox series

[v7,1/7] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string

Message ID 20241003111529.41232-1-marex@denx.de
State Not Applicable
Headers show
Series [v7,1/7] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Marek Vasut Oct. 3, 2024, 11:14 a.m. UTC 
Document compatible string for the WILC3000 chip. The chip is similar
to WILC1000, except that the register layout is slightly different and
it does not support WPA3/SAE.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: - Use WILC1000 as fallback compatible string for WILC3000
V3: - Swap the wilc1000/wilc3000 compatible order
V4: - Add RB from Krzysztof
V5: No change
V6: - Rebase on next-20240926
V7: - Rebase on next-20241003 / wireless-next/main 5a4d42c1688c
      with v2 wifi: wilc1000: Keep slot powered on during suspend/resume
---
 .../bindings/net/wireless/microchip,wilc1000.yaml           | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Alexis Lothoré Oct. 3, 2024, 5:33 p.m. UTC | #1 
On 10/3/24 13:14, Marek Vasut wrote:
> Reduce the use of wilc_get_chipid(), use cached chip ID wherever
> possible. Remove duplicated partial chip ID read implementations
> from the driver. Update wilc_get_chipid() to always read the chip
> ID out of the hardware and update the cached chip ID, and make it
> return a proper return value instead of a chipid. Call wilc_get_chipid()
> early to make the cached chip ID available to various sites using
> is_wilc1000() to access the cached chip ID.
> 
> Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> Signed-off-by: Marek Vasut <marex@denx.de>

[...]

> +int wilc_get_chipid(struct wilc *wilc)
> +{
> +	u32 chipid = 0;
> +	u32 rfrevid = 0;
> +
> +	if (wilc->chipid == 0) {
> +		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
> +		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
> +					     &rfrevid);
> +		if (!is_wilc1000(chipid)) {
> +			wilc->chipid = 0;
> +			return -EINVAL;
> +		}
> +		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
> +			if (rfrevid != 0x1)
> +				chipid = WILC_1000_BASE_ID_2A_REV1;
> +		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
> +			if (rfrevid == 0x4)
> +				chipid = WILC_1000_BASE_ID_2B_REV1;
> +			else if (rfrevid != 0x3)
> +				chipid = WILC_1000_BASE_ID_2B_REV2;
> +		}
> +
> +		wilc->chipid = chipid;
> +	}
> +
> +	return 0;
> +}

My bad for not having spotted it in v6, but you are still missing an
EXPORT_SYMBOL_GPL(wilc_get_chipid) here, making the build fail if wilc support
is built as module:

ERROR: modpost: "wilc_get_chipid"
[drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
ERROR: modpost: "wilc_get_chipid"
[drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
make[1]: *** [/home/alexis/src/microchip/linux/Makefile:1878: modpost] Error 2
make: *** [Makefile:224: __sub-make] Error 2
Marek Vasut Oct. 3, 2024, 6:31 p.m. UTC | #2 
On 10/3/24 7:33 PM, Alexis Lothoré wrote:
> On 10/3/24 13:14, Marek Vasut wrote:
>> Reduce the use of wilc_get_chipid(), use cached chip ID wherever
>> possible. Remove duplicated partial chip ID read implementations
>> from the driver. Update wilc_get_chipid() to always read the chip
>> ID out of the hardware and update the cached chip ID, and make it
>> return a proper return value instead of a chipid. Call wilc_get_chipid()
>> early to make the cached chip ID available to various sites using
>> is_wilc1000() to access the cached chip ID.
>>
>> Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]
> 
>> +int wilc_get_chipid(struct wilc *wilc)
>> +{
>> +	u32 chipid = 0;
>> +	u32 rfrevid = 0;
>> +
>> +	if (wilc->chipid == 0) {
>> +		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
>> +		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
>> +					     &rfrevid);
>> +		if (!is_wilc1000(chipid)) {
>> +			wilc->chipid = 0;
>> +			return -EINVAL;
>> +		}
>> +		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
>> +			if (rfrevid != 0x1)
>> +				chipid = WILC_1000_BASE_ID_2A_REV1;
>> +		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
>> +			if (rfrevid == 0x4)
>> +				chipid = WILC_1000_BASE_ID_2B_REV1;
>> +			else if (rfrevid != 0x3)
>> +				chipid = WILC_1000_BASE_ID_2B_REV2;
>> +		}
>> +
>> +		wilc->chipid = chipid;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> My bad for not having spotted it in v6, but you are still missing an
> EXPORT_SYMBOL_GPL(wilc_get_chipid) here, making the build fail if wilc support
> is built as module:
> 
> ERROR: modpost: "wilc_get_chipid"
> [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
> ERROR: modpost: "wilc_get_chipid"
> [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
> make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
> make[1]: *** [/home/alexis/src/microchip/linux/Makefile:1878: modpost] Error 2
> make: *** [Makefile:224: __sub-make] Error 2
Fixed in V8, thanks.

Before I send V8, can you have a look at the last two patches in this 
series? They need some RB/TB.

Thanks !
Alexis Lothoré Oct. 4, 2024, 8:39 a.m. UTC | #3 
Hello Marek,

On 10/3/24 13:14, Marek Vasut wrote:
> Register wiphy after reading out chipid, so the chipid can be
> used to determine chip features and not advertise WPA3/SAE
> support to userspace on WILC3000. Note that wilc_netdev_cleanup()
> will deregister the wiphy in fail path.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

[...]

> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> index 11e0f8a473467..30015c5920467 100644
> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> @@ -1761,7 +1761,6 @@ static struct wilc *wilc_create_wiphy(struct device *dev)
>  {
>  	struct wiphy *wiphy;
>  	struct wilc *wl;
> -	int ret;
>  
>  	wiphy = wiphy_new(&wilc_cfg80211_ops, sizeof(*wl));
>  	if (!wiphy)
> @@ -1804,14 +1803,8 @@ static struct wilc *wilc_create_wiphy(struct device *dev)
>  				BIT(NL80211_IFTYPE_P2P_GO) |
>  				BIT(NL80211_IFTYPE_P2P_CLIENT);
>  	wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
> -	wiphy->features |= NL80211_FEATURE_SAE;
>  	set_wiphy_dev(wiphy, dev);
>  	wl->wiphy = wiphy;
> -	ret = wiphy_register(wiphy);

I think you did not address one of my comment in v5 about this change: in
wilc_cfg80211_init (wilc_create_wiphy's caller), there is still a
wiphy_unregister in the failure path, which does not make sense anymore since
this function does not register wiphy anymore.

Once fixed: with this patch iw phy shows correctly on my platform that wilc3000
does not support SAE authenticate command while wilc1000 does. And wilc1000
still works correctly, even with wpa3.

Tested-by: Alexis Lothoré <alexis.lothore@bootlin.com>
Tested-on: WILC1000SD 07 SDIO WILC_WIFI_FW_REL_16_1_2
Tested-on: WILC3000 A SDIO WILC_WIFI_FW_REL_16_1_1

Thanks,

Alexis
Alexis Lothoré Oct. 4, 2024, 8:46 a.m. UTC | #4 
On 10/3/24 13:14, Marek Vasut wrote:
> From: Ajay Singh <ajay.kathat@microchip.com>
> 
> Add support for the WILC3000 chip. The chip is similar to WILC1000,
> except that the register layout is slightly different and it does
> not support WPA3/SAE.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> Signed-off-by: Marek Vasut <marex@denx.de>


Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>

And for the basic feature set (basic sta scenario, basic ap scenario):

Tested-by: Alexis Lothoré <alexis.lothore@bootlin.com>
Tested-on: WILC1000SD 07 SDIO WILC_WIFI_FW_REL_16_1_2
Tested-on: WILC1000SD 07 SPI WILC_WIFI_FW_REL_16_1_2
Tested-on: WILC3000 A SDIO WILC_WIFI_FW_REL_16_1_1
Tested-on: WILC3000 A SPI WILC_WIFI_FW_REL_16_1_1
Marek Vasut Oct. 4, 2024, 11:41 a.m. UTC | #5 
On 10/4/24 10:39 AM, Alexis Lothoré wrote:
> Hello Marek,

Hi,

> On 10/3/24 13:14, Marek Vasut wrote:
>> Register wiphy after reading out chipid, so the chipid can be
>> used to determine chip features and not advertise WPA3/SAE
>> support to userspace on WILC3000. Note that wilc_netdev_cleanup()
>> will deregister the wiphy in fail path.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]
> 
>> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> index 11e0f8a473467..30015c5920467 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
>> @@ -1761,7 +1761,6 @@ static struct wilc *wilc_create_wiphy(struct device *dev)
>>   {
>>   	struct wiphy *wiphy;
>>   	struct wilc *wl;
>> -	int ret;
>>   
>>   	wiphy = wiphy_new(&wilc_cfg80211_ops, sizeof(*wl));
>>   	if (!wiphy)
>> @@ -1804,14 +1803,8 @@ static struct wilc *wilc_create_wiphy(struct device *dev)
>>   				BIT(NL80211_IFTYPE_P2P_GO) |
>>   				BIT(NL80211_IFTYPE_P2P_CLIENT);
>>   	wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
>> -	wiphy->features |= NL80211_FEATURE_SAE;
>>   	set_wiphy_dev(wiphy, dev);
>>   	wl->wiphy = wiphy;
>> -	ret = wiphy_register(wiphy);
> 
> I think you did not address one of my comment in v5 about this change: in
> wilc_cfg80211_init (wilc_create_wiphy's caller), there is still a
> wiphy_unregister in the failure path, which does not make sense anymore since
> this function does not register wiphy anymore.
> 
> Once fixed: with this patch iw phy shows correctly on my platform that wilc3000
> does not support SAE authenticate command while wilc1000 does. And wilc1000
> still works correctly, even with wpa3.
> 
> Tested-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> Tested-on: WILC1000SD 07 SDIO WILC_WIFI_FW_REL_16_1_2
> Tested-on: WILC3000 A SDIO WILC_WIFI_FW_REL_16_1_1
Fixed in V8, thank you.
Marek Vasut Oct. 4, 2024, 11:42 a.m. UTC | #6 
On 10/4/24 10:46 AM, Alexis Lothoré wrote:
> On 10/3/24 13:14, Marek Vasut wrote:
>> From: Ajay Singh <ajay.kathat@microchip.com>
>>
>> Add support for the WILC3000 chip. The chip is similar to WILC1000,
>> except that the register layout is slightly different and it does
>> not support WPA3/SAE.
>>
>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> 
> Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> And for the basic feature set (basic sta scenario, basic ap scenario):
> 
> Tested-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> Tested-on: WILC1000SD 07 SDIO WILC_WIFI_FW_REL_16_1_2
> Tested-on: WILC1000SD 07 SPI WILC_WIFI_FW_REL_16_1_2
> Tested-on: WILC3000 A SDIO WILC_WIFI_FW_REL_16_1_1
> Tested-on: WILC3000 A SPI WILC_WIFI_FW_REL_16_1_1
Thank you
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
index 2460ccc082371..5d40f22765bb6 100644
--- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
@@ -16,7 +16,11 @@  description:
 
 properties:
   compatible:
-    const: microchip,wilc1000
+    oneOf:
+      - items:
+          - const: microchip,wilc3000
+          - const: microchip,wilc1000
+      - const: microchip,wilc1000
 
   reg: true