Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: wifi: infineon: Add SPI support to AIROC WiFi driver #78773

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ThreeEights
Copy link
Contributor

Added support for SPI to the Infineon AIROC WiFi driver (drivers/wifi/infineon). Includes support for 3-wire SPI required by the Raspberry Pi Pico W.

Configuration for the Pico W is updated to activate the WiFi chip on that board.

Copy link
Member

@parthitce parthitce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split the changes. bindings, driver and consumer

by the WiFi section of AIROC Wi-Fi device.
type: phandle-array

wifi-host-wake-gpios:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be renamed to interrupt IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"WIFI_HOST_WAKE" comes from the chip datasheet, and that's how Infineon defined the property. I didn't want to change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, same is called interrupt in Linux Kernel context, so was my suggestion. prefix wifi- is implicit already, so can be removed

Copy link
Contributor Author

@ThreeEights ThreeEights Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names:

  • wifi_host_wake
  • wifi_dev_wake
  • wifi_reg_on
  • bt_reg_on

are already in use in the Infineon WiFi and Bluetooth drivers, and match names used throughout modules/hal/infineon and zephyr/modules/hal_infineon, as well as following the naming convention used in their hardware datasheets. Since I'm not part of Infineon, and I'm only adding a new capability on top of their existing driver, I think it's not my role to modify their names.

dts/bindings/wifi/infineon,airoc-wifi-common.yaml Outdated Show resolved Hide resolved
@ThreeEights
Copy link
Contributor Author

@parthitce - You wrote "split the changes. bindings, driver and consumer". Do you mean create 3 separate pull requests? Or do you mean something else?

@parthitce
Copy link
Member

@parthitce - You wrote "split the changes. bindings, driver and consumer". Do you mean create 3 separate pull requests? Or do you mean something else?

No, split into multiple commits in this PR

Additional bindings to configure SPI support.

Signed-off-by: Steve Boylan <stephen.boylan@beechwoods.com>
@ThreeEights
Copy link
Contributor Author

@parthitce - I divided this into 3 commits as you requested. Please let me know if these meet your expectations.

Unfortunately, these changes depend on a pull request for modules/hal/infineon, which is still being reviewed by Infineon. I'll keep following that change so this can proceed.

@@ -17,6 +16,24 @@ menuconfig WIFI_AIROC

if WIFI_AIROC

choice
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this configuration replace with DT_ON_BUS?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes looks like it a good idea to select spi/sdio(sdhc) by DT_ON_BUS, so it will simplify bus configuration. For now we need disable one, enable another...:

CONFIG_AIROC_WIFI_BUS_SDIO=n
CONFIG_AIROC_WIFI_BUS_SPI=y

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soburi - Looks like most of the cases can be replaced with DT_ON_BUS(). The two I don't know how to address are in CMakeLists.txt, where the appropriate bus interface source file is selected, and making the option AIROC_WIFI_BUS_SPI_DATA_IRQ_MUX available in Kconfig.airoc. The latter enables logic to change use of the GPIO line in the special case where the SPI data and IRQ are shared, which is the case for the Pico W.
Do you have any suggestions for how to handle those?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be configured by pinctrl?
IMO, The pin function should not be configured in the wifi driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes looks like it a good idea to select spi/sdio(sdhc) by DT_ON_BUS, so it will simplify bus configuration. For now we need disable one, enable another...:

CONFIG_AIROC_WIFI_BUS_SDIO=n
CONFIG_AIROC_WIFI_BUS_SPI=y

@npal-cy - Yes, that's why I made it a choice. Now that I know about DT_ON_BUS, yes, that makes things cleaner ... if we can figure out how to configure cmake and the data/IRQ multiplexing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be configured by pinctrl?
IMO, The pin function should not be configured in the wifi driver.

I struggled with this issue. On the Pico W, GPIO 24 is used for bidirectional data as well as HOST_WAKE. In order to make this work, the pin has to be assigned to SIO (function 5) when serving as an interrupt, but switched to the PIO (function 6 or 7, depending) when used for SPI data. The Pico SPI driver doesn't know about this silliness; it seems to belong in the layer within the WiFi driver that deals with the SPI bus. This was the best compromise I could come up with. I'd be happy to hear any cleaner solutions.

Copy link
Contributor

@npal-cy npal-cy Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npal-cy - Yes, that's why I made it a choice. Now that I know about DT_ON_BUS, yes, that makes things cleaner ... if we can figure out how to configure cmake and the data/IRQ multiplexing.

I mean to keep AIROC_WIFI_BUS_SPI/AIROC_WIFI_BUS_SDIO, make them hidden and automatically enabled by on_bus, something like snipped below (not tested...)

config AIROC_WIFI_BUS_SDIO
    bool
    default $(dt_compat_on_bus,$(DT_COMPAT_INFINEON_AIROC_WIFI),sd)
    select SDIO_STACK
	select SDHC

config AIROC_WIFI_BUS_SPI
    bool
    default $(dt_compat_on_bus,$(DT_COMPAT_INFINEON_AIROC_WIFI),spi)
    select SPI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThreeEights

Do you have any suggestions for how to handle those?

We can use pinctrl_apply_state with a private state.

ret = pinctrl_apply_state(cfg->pincfg, PINCTRL_STATE_NOPULL);

#include <bus_protocols/whd_bus_spi_protocol.h>
#include <bus_protocols/whd_spi.h>
#include <whd_wifi_api.h>
#include <hardware/gpio.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hardware/gpio.h is it something specific to RPI_PICO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops! Yes, that's Pico W specific. I wrapped it in #if/#endif.

#include <hardware/gpio.h>
#include <zephyr/drivers/spi.h>

#define DT_DRV_COMPAT infineon_airoc_wifi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move define DT_DRV_COMPAT infineon_airoc_wifi to airoc_w....common.h and remove from airoc_w....common.c, ***sdio.c, ***spi.c?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've moved it as you suggested.

CONFIG_AIROC_WIFI_BUS_SDIO=n
CONFIG_AIROC_WIFI_BUS_SPI=y
CONFIG_AIROC_WIFI_BUS_SPI_DATA_IRQ_MUX=y
CONFIG_CYW43439=y
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONFIG_CYW43439 will select FW:

# CYW43439 firmware
if(CONFIG_CYW43439 AND NOT CONFIG_AIROC_WIFI_CUSTOM)
  zephyr_include_directories(${hal_wifi_dir_resources}/firmware/COMPONENT_43439)
  # firmware
  if(CONFIG_AIROC_WLAN_MFG_FIRMWARE)
    set(cyw43xx_fw_bin     ${hal_blobs_dir}/firmware/COMPONENT_43439/43439A0-mfgtest.bin)
    zephyr_library_sources(${hal_wifi_dir_resources}/firmware/COMPONENT_43439/43439A0-mfgtest_bin.c)
  else()
    set(cyw43xx_fw_bin     ${hal_blobs_dir}/firmware/COMPONENT_43439/43439a0.bin)
    zephyr_library_sources(${hal_wifi_dir_resources}/firmware/COMPONENT_43439/43439a0_bin.c)
  endif()
endif()

But it doesn't select any CLM, NVRAM as in e.g CONFIG_CYW43439_MURATA_1YN .... Do we missed something here?

# CYW43439_MURATA_1YN
if(CONFIG_CYW43439_MURATA_1YN AND NOT CONFIG_AIROC_WIFI_CUSTOM)
  zephyr_include_directories(${hal_wifi_dir_resources}/clm/COMPONENT_43439)
  # clm
  if(CONFIG_AIROC_WLAN_MFG_FIRMWARE)
    set(cyw43xx_clm_bin    ${hal_blobs_dir}/clm/COMPONENT_43439/43439A0-mfgtest.clm_blob)
    zephyr_library_sources(${hal_wifi_dir_resources}/clm/COMPONENT_43439/43439A0-mfgtest_clm_blob.c)
  else()
    set(cyw43xx_clm_bin    ${hal_blobs_dir}/clm/COMPONENT_43439/43439A0.clm_blob)
    zephyr_library_sources(${hal_wifi_dir_resources}/clm/COMPONENT_43439/43439A0_clm_blob.c)
  endif()
  # nvram
  zephyr_include_directories(${hal_wifi_dir_resources}/nvram/COMPONENT_43439/COMPONENT_MURATA-1YN)
endif()

Copy link
Contributor Author

@ThreeEights ThreeEights Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious - yet the NVRAM image is included in the result ...
... and curiouser, CONFIG_CYW43439_MURATA_1YN is also selected, even though I didn't put it in the configuration for the Pico W:

sboylan@system76-pc:/media/sboylan/LaCie/zephyr/bare-test/build/zephyr$ grep "CYW4343" .config
# CONFIG_CYW4343W is not set
# CONFIG_CYW43438 is not set
CONFIG_CYW43439=y
CONFIG_CYW43439_MURATA_1YN=y

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes looks like if you do not select any module it takes default one.. anyway need to double check if for Raspberry Pi Pico W we need to use some specific FW/CLM/Nvram...

/** Map Zephyr country codes (2 octets) to whd_country_code_t. */
#if defined(CONFIG_AIROC_WIFI_COUNTRY)
#if defined(CONFIG_BIG_ENDIAN)
#define AIROC_MAP_COUNTRY_CODE(code) ((code[1]) + (code[0] << 8))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do you use AIROC_MAP_COUNTRY_CODE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that's a vestige of an issue that was fixed a long time ago. I took it out.

Added support for SPI in 4-wire and 3-wire configurations to
the Infineon AIROC WiFi driver (drivers/wifi/infineon).

Review changes:
Move DT_DRV_COMPAT to common header file
Correct board-specific preprocessor lines
Removed AIROC_MAP_COUNTRY_CODE

Signed-off-by: Steve Boylan <stephen.boylan@beechwoods.com>
Configure the Raspberry Pi Pico W for WiFi.

Signed-off-by: Steve Boylan <stephen.boylan@beechwoods.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Wi-Fi Wi-Fi platform: Infineon Infineon Technologies AG platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants