| Message ID | 20240829004510.178016-1-marex@denx.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v4,1/5] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hello everyone, On 8/29/24 02:44, Marek Vasut wrote: > 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> [...] > .../bindings/net/wireless/microchip,wilc1000.yaml | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > 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 Following this series first revision, I have been taking a look at how to implement bluetooth feature for wilc3000 (the chip supports Bluetooth LE through a separated UART, see [1]), and I am facing some constraints. I feel like the possible solutions would conflict with this new binding, so even if I am a bit late to the party, I would like to expose the issue before the binding is merged in case we can find something which would allow to add bluetooth support without too much pain after the wlan part. Downstream driver currently does not implement bluetooth as a standard bluetooth driver (module in drivers/bluetooth, registering a HCI device) but only performs a minimal set of operations directly in the wlan part ([2]). Getting a version valid for upstream would need the following points to be addressed: 1. despite being controlled from a serial port for nominal operations, the bluetooth part also depends on the "wlan" bus (spi or sdio) for initialization 2. yet init steps are not performed on any kind of subsystem ops but through writes to a custom chardev 3. the driver does not register itself a hci interface, it is expected to be done by userspace (hciattach). It is only after those 3 steps that the chip can be used with standard hci commands over serial port. IMHO 1 is the biggest point, because it means that **a bluetooth driver for wilc3000 needs access to the bus used by wlan part** (so only describing the bluetooth part of the chip as a child node of an uart controller is not enough). Aside from bus access, I also expect some interactions between bluetooth and wifi (eg: power management, sleep/wakeup) After considering multiple solutions to try to share this bus between existing wlan driver and a new bt driver (mfd device, auxiliary bus, device link + some handles, etc), my current best guess is to convert wilc driver to a MFD driver for wilc3000. I guess some work can be done so that the driver can still be shared between wilc1000 and wilc3000 _while_ remaining compatible with current wilc1000 description, but it would impact the DT description for wilc3000, which would need to switch from this: spi { wifi@0 { compatible = "microchip,wilc3000"; [...] }; }; To something like this: spi { wilc@0 { compatible = "microchip,wilc3000"; /* mfd driver */ wifi { compatible = "microchip,wilc3000-wlan"; [...] }; bt { compatible = "microchip,wilc3000-bt"; XXXX; /* some link to the uart controller connected to the chip */ [...] }; }; }; (and similar thing when wilc is driven over a sdio bus) Any opinion on this ? Would it make sense to describe wilc3000 chip that way ? Thanks, Alexis [1] https://www.microchip.com/en-us/product/atwilc3000 [2] https://github.com/linux4sam/linux-at91/blob/linux-6.6-mchp/drivers/net/wireless/microchip/wilc1000/bt.c
On 03/09/2024 18:09, Alexis Lothoré wrote: > Hello everyone, > > On 8/29/24 02:44, Marek Vasut wrote: >> 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> > > [...] > >> .../bindings/net/wireless/microchip,wilc1000.yaml | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> 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 > > Following this series first revision, I have been taking a look at how to > implement bluetooth feature for wilc3000 (the chip supports Bluetooth LE through > a separated UART, see [1]), and I am facing some constraints. I feel like the > possible solutions would conflict with this new binding, so even if I am a bit > late to the party, I would like to expose the issue before the binding is merged > in case we can find something which would allow to add bluetooth support without > too much pain after the wlan part. > > Downstream driver currently does not implement bluetooth as a standard bluetooth > driver (module in drivers/bluetooth, registering a HCI device) but only performs > a minimal set of operations directly in the wlan part ([2]). Getting a version > valid for upstream would need the following points to be addressed: > 1. despite being controlled from a serial port for nominal operations, the > bluetooth part also depends on the "wlan" bus (spi or sdio) for initialization > 2. yet init steps are not performed on any kind of subsystem ops but through > writes to a custom chardev > 3. the driver does not register itself a hci interface, it is expected to be > done by userspace (hciattach). > > It is only after those 3 steps that the chip can be used with standard hci > commands over serial port. IMHO 1 is the biggest point, because it means that > **a bluetooth driver for wilc3000 needs access to the bus used by wlan part** > (so only describing the bluetooth part of the chip as a child node of an uart > controller is not enough). Aside from bus access, I also expect some > interactions between bluetooth and wifi (eg: power management, sleep/wakeup) > > After considering multiple solutions to try to share this bus between existing > wlan driver and a new bt driver (mfd device, auxiliary bus, device link + some Driver design should not have impact on bindings. > handles, etc), my current best guess is to convert wilc driver to a MFD driver > for wilc3000. I guess some work can be done so that the driver can still be > shared between wilc1000 and wilc3000 _while_ remaining compatible with current > wilc1000 description, but it would impact the DT description for wilc3000, which > would need to switch from this: > > spi { > wifi@0 { > compatible = "microchip,wilc3000"; > [...] > }; > }; > > To something like this: > > spi { > wilc@0 { > compatible = "microchip,wilc3000"; /* mfd driver */ I do not see any reason why... or rather: What is MFD here? MFD is Linux stuff and we talk about hardware. > wifi { > compatible = "microchip,wilc3000-wlan"; Why? Just merge it to parent... > [...] > }; > bt { > compatible = "microchip,wilc3000-bt"; > XXXX; /* some link to the uart controller connected to the chip */ That's not how we represent UART devices. I don't understand why do you need these - if for power sequencing, then use power sequencing framework and describe associated hardware (there are some talks coming about it in 2 weeks). If for something else, then for what? > [...] > }; > }; > }; > > (and similar thing when wilc is driven over a sdio bus) > > Any opinion on this ? Would it make sense to describe wilc3000 chip that way ? > You described drivers, not wilc3000 chip... Best regards, Krzysztof
On 9/3/24 6:09 PM, Alexis Lothoré wrote: > Hello everyone, Hi, > On 8/29/24 02:44, Marek Vasut wrote: >> 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> > > [...] > >> .../bindings/net/wireless/microchip,wilc1000.yaml | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> 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 > > Following this series first revision, I have been taking a look at how to > implement bluetooth feature for wilc3000 (the chip supports Bluetooth LE through > a separated UART, see [1]), and I am facing some constraints. I feel like the > possible solutions would conflict with this new binding, so even if I am a bit > late to the party, I would like to expose the issue before the binding is merged > in case we can find something which would allow to add bluetooth support without > too much pain after the wlan part. > > Downstream driver currently does not implement bluetooth as a standard bluetooth > driver (module in drivers/bluetooth, registering a HCI device) but only performs > a minimal set of operations directly in the wlan part ([2]). Getting a version > valid for upstream would need the following points to be addressed: > 1. despite being controlled from a serial port for nominal operations, the > bluetooth part also depends on the "wlan" bus (spi or sdio) for initialization > 2. yet init steps are not performed on any kind of subsystem ops but through > writes to a custom chardev > 3. the driver does not register itself a hci interface, it is expected to be > done by userspace (hciattach). > > It is only after those 3 steps that the chip can be used with standard hci > commands over serial port. IMHO 1 is the biggest point, because it means that > **a bluetooth driver for wilc3000 needs access to the bus used by wlan part** > (so only describing the bluetooth part of the chip as a child node of an uart > controller is not enough). Aside from bus access, I also expect some > interactions between bluetooth and wifi (eg: power management, sleep/wakeup) Just a quick idea -- what about having a phandle to the BT UART node in the wilc3000 node ? Then the wilc driver can check if the phandle is available and valid, and attach the BT part to the UART, while also doing all the necessary power sequencing and bus accesses via SDIO/SPI. Like this: &uart10 { status = "okay"; }; &mmc20 { ... wifi@0 { compatible = "microchip,wilc1000"; microchip,bt-uart = <&uart10>; // OPTIONAL ... }; };
Hi Krzysztof, On 9/3/24 20:47, Krzysztof Kozlowski wrote: > On 03/09/2024 18:09, Alexis Lothoré wrote: [...] >> After considering multiple solutions to try to share this bus between existing >> wlan driver and a new bt driver (mfd device, auxiliary bus, device link + some > > Driver design should not have impact on bindings. > >> handles, etc), my current best guess is to convert wilc driver to a MFD driver >> for wilc3000. I guess some work can be done so that the driver can still be >> shared between wilc1000 and wilc3000 _while_ remaining compatible with current >> wilc1000 description, but it would impact the DT description for wilc3000, which >> would need to switch from this: >> >> spi { >> wifi@0 { >> compatible = "microchip,wilc3000"; >> [...] >> }; >> }; >> >> To something like this: >> >> spi { >> wilc@0 { >> compatible = "microchip,wilc3000"; /* mfd driver */ > > I do not see any reason why... or rather: What is MFD here? MFD is Linux > stuff and we talk about hardware. > >> wifi { >> compatible = "microchip,wilc3000-wlan"; > > Why? Just merge it to parent... > >> [...] >> }; >> bt { >> compatible = "microchip,wilc3000-bt"; >> XXXX; /* some link to the uart controller connected to the chip */ > > That's not how we represent UART devices. I don't understand why do you > need these - if for power sequencing, then use power sequencing > framework and describe associated hardware (there are some talks coming > about it in 2 weeks). If for something else, then for what? I have to check more for this power sequencing framework, it look likes it could handle parts of the wifi/bt shared power management, but it will not cover everything. The need for this bus on the BT side is not only for power sequencing, there is some chip initialization to be performed over this bus, like firmware upload to the chip (not the wifi firmware, it is an additional bluetooth firmware). I guess you are referring to Bartosz Golaszewski's talk at Plumbers. Unfortunately I can not attend, but I'll make sure to check the materials once available :) Thanks, Alexis
Hello Marek, On 9/3/24 21:30, Marek Vasut wrote: > On 9/3/24 6:09 PM, Alexis Lothoré wrote: [...] >> It is only after those 3 steps that the chip can be used with standard hci >> commands over serial port. IMHO 1 is the biggest point, because it means that >> **a bluetooth driver for wilc3000 needs access to the bus used by wlan part** >> (so only describing the bluetooth part of the chip as a child node of an uart >> controller is not enough). Aside from bus access, I also expect some >> interactions between bluetooth and wifi (eg: power management, sleep/wakeup) > > Just a quick idea -- what about having a phandle to the BT UART node in the > wilc3000 node ? Then the wilc driver can check if the phandle is available and > valid, and attach the BT part to the UART, while also doing all the necessary > power sequencing and bus accesses via SDIO/SPI. > > Like this: > > &uart10 { > status = "okay"; > }; > > &mmc20 { > ... > wifi@0 { > compatible = "microchip,wilc1000"; > microchip,bt-uart = <&uart10>; // OPTIONAL > ... > }; > }; I thought about something like this too, indeed (but somehow inverted, a reference to wilc node in the bt node under uart, to allow the bluetooth part to ask wilc to perform operations over sdio/spi). The design would likely be simpler in this case, but some internal discussions with colleagues raised some concerns, for example with power management (but Krzysztof's suggestion about power sequencing may help with this). Thanks, Alexis
On 9/4/24 4:50 PM, Alexis Lothoré wrote: > Hello Marek, Hi, >>> It is only after those 3 steps that the chip can be used with standard hci >>> commands over serial port. IMHO 1 is the biggest point, because it means that >>> **a bluetooth driver for wilc3000 needs access to the bus used by wlan part** >>> (so only describing the bluetooth part of the chip as a child node of an uart >>> controller is not enough). Aside from bus access, I also expect some >>> interactions between bluetooth and wifi (eg: power management, sleep/wakeup) >> >> Just a quick idea -- what about having a phandle to the BT UART node in the >> wilc3000 node ? Then the wilc driver can check if the phandle is available and >> valid, and attach the BT part to the UART, while also doing all the necessary >> power sequencing and bus accesses via SDIO/SPI. >> >> Like this: >> >> &uart10 { >> status = "okay"; >> }; >> >> &mmc20 { >> ... >> wifi@0 { >> compatible = "microchip,wilc1000"; >> microchip,bt-uart = <&uart10>; // OPTIONAL >> ... >> }; >> }; > > I thought about something like this too, indeed (but somehow inverted, a > reference to wilc node in the bt node under uart, to allow the bluetooth part to > ask wilc to perform operations over sdio/spi). The design would likely be > simpler in this case, but some internal discussions with colleagues raised some > concerns, for example with power management (but Krzysztof's suggestion about > power sequencing may help with this). Maybe switching the current WILC power management to runtime PM would simplify things greatly ?
Marek Vasut <marex@denx.de> writes: > 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> [...] > --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c > +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c > @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, > > vif->connecting = true; > > + if (sme->auth_type == NL80211_AUTHTYPE_SAE && > + is_wilc3000(vif->wilc->chipid)) { > + netdev_err(dev, "WILC3000: WPA3 not supported\n"); > + ret = -EOPNOTSUPP; > + goto out_error; > + } This looks wrong. If wilc3000 doesn't support SAE you shouldn't advertise NL80211_FEATURE_SAE to user space. I think the check for wilc3000 should be in wilc_create_wiphy(): if (!is_wilc3000(vif->wilc->chipid)) wiphy->features |= NL80211_FEATURE_SAE;
On 9/9/24 11:35 AM, Kalle Valo wrote: > Marek Vasut <marex@denx.de> writes: > >> 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> > > [...] > >> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >> >> vif->connecting = true; >> >> + if (sme->auth_type == NL80211_AUTHTYPE_SAE && >> + is_wilc3000(vif->wilc->chipid)) { >> + netdev_err(dev, "WILC3000: WPA3 not supported\n"); >> + ret = -EOPNOTSUPP; >> + goto out_error; >> + } > > This looks wrong. If wilc3000 doesn't support SAE you shouldn't > advertise NL80211_FEATURE_SAE to user space. I think the check for > wilc3000 should be in wilc_create_wiphy(): > > if (!is_wilc3000(vif->wilc->chipid)) It is probably better to do "if (is_wilc1000(wl->chipid))" here. This way, fixed in V5, thanks. > wiphy->features |= NL80211_FEATURE_SAE;
Marek Vasut <marex@denx.de> writes: > On 9/9/24 11:35 AM, Kalle Valo wrote: > >> Marek Vasut <marex@denx.de> writes: >> >>> 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> >> [...] >> >>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >>> vif->connecting = true; >>> + if (sme->auth_type == NL80211_AUTHTYPE_SAE && >>> + is_wilc3000(vif->wilc->chipid)) { >>> + netdev_err(dev, "WILC3000: WPA3 not supported\n"); >>> + ret = -EOPNOTSUPP; >>> + goto out_error; >>> + } >> This looks wrong. If wilc3000 doesn't support SAE you shouldn't >> advertise NL80211_FEATURE_SAE to user space. I think the check for >> wilc3000 should be in wilc_create_wiphy(): >> if (!is_wilc3000(vif->wilc->chipid)) > > It is probably better to do "if (is_wilc1000(wl->chipid))" here. Good point.
On 9/9/24 02:35, Kalle Valo wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Marek Vasut <marex@denx.de> writes: > >> 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> > > [...] > >> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >> >> vif->connecting = true; >> >> + if (sme->auth_type == NL80211_AUTHTYPE_SAE && >> + is_wilc3000(vif->wilc->chipid)) { >> + netdev_err(dev, "WILC3000: WPA3 not supported\n"); >> + ret = -EOPNOTSUPP; >> + goto out_error; >> + } > > This looks wrong. If wilc3000 doesn't support SAE you shouldn't > advertise NL80211_FEATURE_SAE to user space. I think the check for > wilc3000 should be in wilc_create_wiphy(): > Actually, the chip ID is not available when wilc_create_wiphy() is called but is set later in the device probe function. Therefore, adding the 'is_wilc3000(vif->wilc->chipid)' condition may not work as expected. Also, I think there is no API to change "wiphy->features" after wiphy is registered to set it later when chip ID information is available. Does it make sense to add a module parameter for device type(wilc1000 or wilc3000) to address device-specific featurization. > if (!is_wilc3000(vif->wilc->chipid)) > wiphy->features |= NL80211_FEATURE_SAE; > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
<Ajay.Kathat@microchip.com> writes: > On 9/9/24 02:35, Kalle Valo wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> Marek Vasut <marex@denx.de> writes: >> >>> 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> >> >> [...] >> >>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >>> >>> vif->connecting = true; >>> >>> + if (sme->auth_type == NL80211_AUTHTYPE_SAE && >>> + is_wilc3000(vif->wilc->chipid)) { >>> + netdev_err(dev, "WILC3000: WPA3 not supported\n"); >>> + ret = -EOPNOTSUPP; >>> + goto out_error; >>> + } >> >> This looks wrong. If wilc3000 doesn't support SAE you shouldn't >> advertise NL80211_FEATURE_SAE to user space. I think the check for >> wilc3000 should be in wilc_create_wiphy(): >> > > Actually, the chip ID is not available when wilc_create_wiphy() is called but > is set later in the device probe function. Therefore, adding the > 'is_wilc3000(vif->wilc->chipid)' condition may not work as expected. > Also, I think there is no API to change "wiphy->features" after wiphy is > registered to set it later when chip ID information is available. Sounds like the driver is doing something funky in the registration, the idea is that the device capabilities are probed before calling wiphy_register(). > Does it make sense to add a module parameter for device type(wilc1000 or > wilc3000) to address device-specific featurization. We don't do hacks like that in upstream, it's expected that the driver does this all automatically.
On 9/9/24 5:04 PM, Kalle Valo wrote: > Marek Vasut <marex@denx.de> writes: > >> On 9/9/24 11:35 AM, Kalle Valo wrote: >> >>> Marek Vasut <marex@denx.de> writes: >>> >>>> 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> >>> [...] >>> >>>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >>>> vif->connecting = true; >>>> + if (sme->auth_type == NL80211_AUTHTYPE_SAE && >>>> + is_wilc3000(vif->wilc->chipid)) { >>>> + netdev_err(dev, "WILC3000: WPA3 not supported\n"); >>>> + ret = -EOPNOTSUPP; >>>> + goto out_error; >>>> + } >>> This looks wrong. If wilc3000 doesn't support SAE you shouldn't >>> advertise NL80211_FEATURE_SAE to user space. I think the check for >>> wilc3000 should be in wilc_create_wiphy(): >>> if (!is_wilc3000(vif->wilc->chipid)) >> >> It is probably better to do "if (is_wilc1000(wl->chipid))" here. > > Good point. I did send v5 which grew a few more patches to address getting chipid early.
On 9/9/24 10:31, Kalle Valo wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > <Ajay.Kathat@microchip.com> writes: > >> On 9/9/24 02:35, Kalle Valo wrote: >> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Marek Vasut <marex@denx.de> writes: >>> >>>> 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> >>> >>> [...] >>> >>>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>>> @@ -313,6 +313,13 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >>>> >>>> vif->connecting = true; >>>> >>>> + if (sme->auth_type == NL80211_AUTHTYPE_SAE && >>>> + is_wilc3000(vif->wilc->chipid)) { >>>> + netdev_err(dev, "WILC3000: WPA3 not supported\n"); >>>> + ret = -EOPNOTSUPP; >>>> + goto out_error; >>>> + } >>> >>> This looks wrong. If wilc3000 doesn't support SAE you shouldn't >>> advertise NL80211_FEATURE_SAE to user space. I think the check for >>> wilc3000 should be in wilc_create_wiphy(): >>> >> >> Actually, the chip ID is not available when wilc_create_wiphy() is called but >> is set later in the device probe function. Therefore, adding the >> 'is_wilc3000(vif->wilc->chipid)' condition may not work as expected. >> Also, I think there is no API to change "wiphy->features" after wiphy is >> registered to set it later when chip ID information is available. > > Sounds like the driver is doing something funky in the registration, the > idea is that the device capabilities are probed before calling > wiphy_register(). > Agree, this approach will configure the wiphy based on connected device. >> Does it make sense to add a module parameter for device type(wilc1000 or >> wilc3000) to address device-specific featurization. > > We don't do hacks like that in upstream, it's expected that the driver > does this all automatically. Marek has already submitted the patch to delay calling wiphy_register() so it should work at run time for both the devices. I'm just curious to know for which scenario the module parameters should be used. Can it be used for enabling or disabling any feature, configuring any parameters, instead of complete device-specific featurization. Regards, Ajay
On 9/9/24 11:11 PM, Ajay.Kathat@microchip.com wrote: [...] >>> Does it make sense to add a module parameter for device type(wilc1000 or >>> wilc3000) to address device-specific featurization. >> >> We don't do hacks like that in upstream, it's expected that the driver >> does this all automatically. > > Marek has already submitted the patch to delay calling wiphy_register() so it > should work at run time for both the devices. > I'm just curious to know for which scenario the module parameters should be > used. Can it be used for enabling or disabling any feature, configuring any > parameters, instead of complete device-specific featurization. Module parameters are applicable for tunables which cannot be otherwise configured at runtime. Runtime configuration is always preferable. For the wilc, I don't think there is anything which has to be a module parameter, maybe firmware filename could be it.
Marek Vasut <marex@denx.de> writes: > On 9/9/24 11:11 PM, Ajay.Kathat@microchip.com wrote: > > [...] > >>>> Does it make sense to add a module parameter for device type(wilc1000 or >>>> wilc3000) to address device-specific featurization. >>> >>> We don't do hacks like that in upstream, it's expected that the driver >>> does this all automatically. >> >> Marek has already submitted the patch to delay calling >> wiphy_register() so it >> should work at run time for both the devices. >> I'm just curious to know for which scenario the module parameters should be >> used. Can it be used for enabling or disabling any feature, configuring any >> parameters, instead of complete device-specific featurization. > > Module parameters are applicable for tunables which cannot be > otherwise configured at runtime. Runtime configuration is always > preferable. For the wilc, I don't think there is anything which has to > be a module parameter, maybe firmware filename could be it. Nowadays module parameters are frowned upon, apparently some subsystems have even banned adding new module parameters. In wireless we have been more lax and have allowed new module parameters in some cases with good justification, but it's still rare.
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