| Message ID | 20241029172354.4027886-1-michael.nemanov@ti.com |
|---|---|
| Headers | show |
| Series | wifi: cc33xx: Add driver for new TI CC33xx wireless device family | expand |
On 29/10/2024 18:23, Michael Nemanov wrote: > diff --git a/drivers/net/wireless/ti/Makefile b/drivers/net/wireless/ti/Makefile > index 05ee016594f8..9e028a91ec30 100644 > --- a/drivers/net/wireless/ti/Makefile > +++ b/drivers/net/wireless/ti/Makefile > @@ -3,3 +3,4 @@ obj-$(CONFIG_WLCORE) += wlcore/ > obj-$(CONFIG_WL12XX) += wl12xx/ > obj-$(CONFIG_WL1251) += wl1251/ > obj-$(CONFIG_WL18XX) += wl18xx/ > +obj-$(CONFIG_CC33XX) += cc33xx/ > \ No newline at end of file Patch error. Best regards, Krzysztof
On 10/29/2024 7:28 PM, Krzysztof Kozlowski wrote: > On 29/10/2024 18:23, Michael Nemanov wrote: >> Add device-tree bindings for the CC33xx family. >> >> Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> >> --- >> .../bindings/net/wireless/ti,cc33xx.yaml | 59 +++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml >> new file mode 100644 >> index 000000000000..12a0a2f52f44 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml >> @@ -0,0 +1,59 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/wireless/ti,cc33xx.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Texas Instruments CC33xx Wireless LAN Controller >> + >> +maintainers: >> + - Michael Nemanov <michael.nemanov@ti.com> >> + >> +description: >> + The CC33xx is a family of IEEE 802.11ax chips from Texas Instruments. >> + These chips must be connected via SDIO and support in-band / out-of-band IRQ. >> + >> +properties: >> + $nodename: >> + pattern: "^wifi@2" > > This wasn't here, please drop. > In the previous patch you noted there was a mismatch between the reg address in the schema (const: 2) and the used in the example (wifi@1). The dt_binding_check did not flag this because SDIO is not a simple bus. Using this regex seemed like a good alternative. Still drop it? > >> + >> + compatible: >> + oneOf: > > Why oneOf appeared? Do you plan to grow it? > >> + - items: >> + - enum: >> + - ti,cc3300 >> + - ti,cc3301 >> + - ti,cc3350 >> + - ti,cc3351 >> + - const: ti,cc33xx > > And how cc33xx could appear? That's a no. Generic compatibles are not > allowed. Please do not introduce some completely different changes than > asked for. > > Your changelog does not explain these three. "Fixed compatibility" is > way too vague, especially that you do not fix anything here. > I was trying to address the feedback from previous patch. You said: >>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>> + { } >>>> +}; >>> >>> >>> Eh? What happened here? So devices are compatibles thus make them >>> compatible in the bindings. >>> >> >> I thought this is the right way to do it (originally taken from [1]). >> How can I solve it via DT bindings? > > It's all over the bindings (also example-schema). Use fallback and oneOf. > Looking at [2] and [3] as an example I tried to do the same (make cc33xx driver compatible with all chip variants). How should have I done it? Regards, Michael. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/sdio.c#n204 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/qcom-wdt.c
On 30/10/2024 11:59, Nemanov, Michael wrote: >> >> Your changelog does not explain these three. "Fixed compatibility" is >> way too vague, especially that you do not fix anything here. >> > > I was trying to address the feedback from previous patch. You said: > >>>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>>> + { } >>>>> +}; >>>> >>>> >>>> Eh? What happened here? So devices are compatibles thus make them >>>> compatible in the bindings. >>>> >>> >>> I thought this is the right way to do it (originally taken from [1]). >>> How can I solve it via DT bindings? >> >> It's all over the bindings (also example-schema). Use fallback and oneOf. >> > > Looking at [2] and [3] as an example I tried to do the same (make cc33xx > driver compatible with all chip variants). > How should have I done it? qcom-wdt is quite a different device. It's true you should have here oneOf, but for a purpose. oneOf without purpose does not make sense, right? I think other TI bindings would serve you as an example. Or this one: https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31 Best regards, Krzysztof
On 10/30/2024 1:09 PM, Krzysztof Kozlowski wrote: > On 30/10/2024 11:59, Nemanov, Michael wrote: >>> >>> Your changelog does not explain these three. "Fixed compatibility" is >>> way too vague, especially that you do not fix anything here. >>> >> >> I was trying to address the feedback from previous patch. You said: >> >>>>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>>>> + { } >>>>>> +}; >>>>> >>>>> >>>>> Eh? What happened here? So devices are compatibles thus make them >>>>> compatible in the bindings. >>>>> >>>> >>>> I thought this is the right way to do it (originally taken from [1]). >>>> How can I solve it via DT bindings? >>> >>> It's all over the bindings (also example-schema). Use fallback and oneOf. >>> >> >> Looking at [2] and [3] as an example I tried to do the same (make cc33xx >> driver compatible with all chip variants). >> How should have I done it? > > qcom-wdt is quite a different device. It's true you should have here > oneOf, but for a purpose. oneOf without purpose does not make sense, right? > > I think other TI bindings would serve you as an example. Or this one: > > https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31 > > > Best regards, > Krzysztof > OK. So I should make one of the variants the base and declare others as compatible? i.e: --Bindings-- compatible: oneOf: - const: ti,cc3300 - items: - enum: - ti,cc3301 - ti,cc3350 - ti,cc3351 - const: ti,cc3300 --Driver-- static const struct of_device_id cc33xx_sdio_of_match_table[] = { { .compatible = "ti,cc3300", .data = &cc33xx_data }, { } };
On 30/10/2024 13:14, Nemanov, Michael wrote: > On 10/30/2024 1:09 PM, Krzysztof Kozlowski wrote: >> On 30/10/2024 11:59, Nemanov, Michael wrote: >>>> >>>> Your changelog does not explain these three. "Fixed compatibility" is >>>> way too vague, especially that you do not fix anything here. >>>> >>> >>> I was trying to address the feedback from previous patch. You said: >>> >>>>>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>>>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>>>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>>>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>>>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>>>>> + { } >>>>>>> +}; >>>>>> >>>>>> >>>>>> Eh? What happened here? So devices are compatibles thus make them >>>>>> compatible in the bindings. >>>>>> >>>>> >>>>> I thought this is the right way to do it (originally taken from [1]). >>>>> How can I solve it via DT bindings? >>>> >>>> It's all over the bindings (also example-schema). Use fallback and oneOf. >>>> >>> >>> Looking at [2] and [3] as an example I tried to do the same (make cc33xx >>> driver compatible with all chip variants). >>> How should have I done it? >> >> qcom-wdt is quite a different device. It's true you should have here >> oneOf, but for a purpose. oneOf without purpose does not make sense, right? >> >> I think other TI bindings would serve you as an example. Or this one: >> >> https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31 >> >> >> Best regards, >> Krzysztof >> > > OK. > So I should make one of the variants the base and declare others as > compatible? i.e: > Yes Best regards, Krzysztof
On Tue, Oct 29, 2024 at 07:23:46PM +0200, Michael Nemanov wrote: > General code and structures. > Notably: > > cc33xx_irq - Handles IRQs received from the device. > > process_core_status - Core status is a new concept in CC33xx. > it's a structure that is appended to each transfer from the device and > contains its most up-to-date status report (IRQs, buffers, etc.). > See struct core_status for details. > > process_event_and_cmd_result - Responses to driver commands and > FW events both arrive asynchronously. Therefore, driver cannot know what > he read from HW until inspecting the payload. This code reads and > dispatches the data accordingly. > > cc33xx_recovery_work - Driver supports basic recovery on FW crash and > other illegal conditions. This implements the recovery flow > (Remove all vifs, turn device off and on, download FW, > let ieee80211_restart_hw do the rest). > > irq_deferred_work - Does irq-related work that requires holding the > cc->mutex. Thisd is mostly in response to HW's Tx/Rx IRQs. > > cc33xx_nvs_cb - Callback for the NVS FW request API. Similar to wlcore, > this is where the init of the HW is performed. > > cc33xx_load_ini_bin_file - Loads a configuration file from user-space > via the request FW API. The structure is described in a separate patch. > > cc33xx_op_X - MAC80211 operation handlers. > > Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> > --- > drivers/net/wireless/ti/cc33xx/main.c | 5689 +++++++++++++++++++++++++ > 1 file changed, 5689 insertions(+) > create mode 100644 drivers/net/wireless/ti/cc33xx/main.c > > diff --git a/drivers/net/wireless/ti/cc33xx/main.c b/drivers/net/wireless/ti/cc33xx/main.c ... > +static struct ieee80211_sband_iftype_data iftype_data_2ghz[] = {{ Hi Michael, Sparse seems a bit unhappy about this: .../main.c:332:24: warning: incorrect type in initializer (different address spaces) .../main.c:332:24: expected struct ieee80211_sband_iftype_data const [noderef] __iftype_data *iftype_data .../main.c:332:24: got struct ieee80211_sband_iftype_data * So perhaps it should be: static const struct ieee80211_sband_iftype_data __iftd iftype_data_2ghz[] = {{ Likewise for iftype_data_5ghz. ...
On 11/2/2024 3:25 PM, Simon Horman wrote: ... > > Hi Michael, > > Sparse seems a bit unhappy about this: > > .../main.c:332:24: warning: incorrect type in initializer (different address spaces) > .../main.c:332:24: expected struct ieee80211_sband_iftype_data const [noderef] __iftype_data *iftype_data > .../main.c:332:24: got struct ieee80211_sband_iftype_data * > > So perhaps it should be: > > static const struct ieee80211_sband_iftype_data __iftd iftype_data_2ghz[] = {{ > > Likewise for iftype_data_5ghz. > Hi Simon, Yeah I saw that and was unsure how to properly solve it, Sparse does not seems to like direct use of this variable. The above is based on old code and I see newer implementations are using ieee80211_set_sband_iftype_data which has the casting that should make Sparse at ease. I'll try migrating the CC33xx code to this convention which will require moving some structs around. Thanks and regards, Michael.
On 10/29/2024 7:34 PM, Krzysztof Kozlowski wrote: ... >> + >> +void cc33xx_disable_interrupts_nosync(struct cc33xx *cc) >> +{ >> + cc->if_ops->disable_irq(cc->dev); >> +} >> + >> +void cc33xx_irq(void *cookie); > > Why do you need forward declaration of non-static function? If you need > it, it means you had W=1 warning which you fixed incorrect way. > > Regardless, be sure this code has 0 warnings on clang with W=1. > Indeed a refactoring leftover (already moved to a header file) will fix. ... >> + >> +static struct sdio_driver cc33xx_sdio_driver = { >> + .name = "cc33xx_sdio", >> + .id_table = cc33xx_devices, >> + .probe = sdio_cc33xx_probe, >> + .remove = sdio_cc33xx_remove, >> +#ifdef CONFIG_PM >> + .drv = { >> + .pm = &cc33xx_sdio_pm_ops, >> + }, >> +#endif /* CONFIG_PM */ >> +}; >> + >> +MODULE_DEVICE_TABLE(sdio, cc33xx_devices); > > This is always next to the table. > Will fix. Regards, Michael.