| Message ID | 20230310202922.2459680-1-martin.blumenstingl@googlemail.com |
|---|---|
| Headers | show |
| Series | rtw88: Add SDIO support | expand |
On 3/10/23 14:29, Martin Blumenstingl wrote: > Recently the rtw88 driver has gained locking support for the "slow" bus > types (USB, SDIO) as part of USB support. Thanks to everyone who helped > make this happen! > > Based on the USB work (especially the locking part and various > bugfixes) this series adds support for SDIO based cards. It's the > result of a collaboration between Jernej and myself. Neither of us has > access to the rtw88 datasheets. All of our work is based on studying > the RTL8822BS and RTL8822CS vendor drivers and trial and error. > > Jernej and myself have tested this with RTL8822BS and RTL8822CS cards. > Other users have confirmed that RTL8821CS support is working as well. > RTL8723DS may also work (we tried our best to handle rtw_chip_wcpu_11n > where needed) but has not been tested at this point. > > Jernej's results with a RTL8822BS: > - Main functionality works > - Had a case where no traffic got across the link until he issued a > scan > > My results with a RTL8822CS: > - 2.4GHz and 5GHz bands are both working > - TX throughput on a 5GHz network is between 50 Mbit/s and 90 Mbit/s > - RX throughput on a 5GHz network is at 19 Mbit/s (this seems to be > an combination of the location of my board and the cheap antenna > which are both hurting RX performance) > > A user shared his results on his own RTL8822CS off-list with me: > - 50Mbit/s throughput in both directions > > A user shared his results on RTL8821CS off-list with me: > - 50Mbps down and 25Mbps on a 5GHz network > > Why is this an RFC? > - I think it's worth to get another round of feedback from the rtw88 > maintainers > - As with most patches: testing is very welcome. If things are working > fine then a Tested-by is appreciated (with some details about the > card, throughput, ...). If something doesn't work for you: please > still report back so we can investigate that problem! > > Changes since v1 at [0]: > - removed patches 1-8 as they have been submitted and separately (they > were indepdent and this helped cutting down the size of this series) > - dropped patch "rtw88: ps: Increase LEAVE_LPS_TRY_CNT for SDIO based > chipsets" as the underlying issue has been fixed - most likely with > upstream commit 823092a53556eb ("wifi: rtw88: fix race condition > when doing H2C command") > - rework the code so we don't need a new HCI specific power_switch > callback by utilizing the RTW_FLAG_POWERON flag which was recently > introduced > - various patches include the feedback from reviewers and build > testing robots (see the individual patches for details) > > > [0] https://lore.kernel.org/lkml/a2449a2d1e664bcc8962af4667aa1290@realtek.com/T/ > > > Jernej Skrabec (1): > wifi: rtw88: Add support for the SDIO based RTL8822BS chipset > > Martin Blumenstingl (8): > wifi: rtw88: Clear RTW_FLAG_POWERON early in rtw_mac_power_switch() > wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets > wifi: rtw88: mac: Support SDIO specific bits in the power on sequence > wifi: rtw88: main: Add the {cpwm,rpwm}_addr for SDIO based chipsets > wifi: rtw88: main: Reserve 8 bytes of extra TX headroom for SDIO cards > mmc: sdio: add Realtek SDIO vendor ID and various wifi device IDs > wifi: rtw88: Add support for the SDIO based RTL8822CS chipset > wifi: rtw88: Add support for the SDIO based RTL8821CS chipset > > drivers/net/wireless/realtek/rtw88/Kconfig | 36 + > drivers/net/wireless/realtek/rtw88/Makefile | 12 + > drivers/net/wireless/realtek/rtw88/debug.h | 1 + > drivers/net/wireless/realtek/rtw88/mac.c | 51 +- > drivers/net/wireless/realtek/rtw88/mac.h | 1 - > drivers/net/wireless/realtek/rtw88/main.c | 9 +- > drivers/net/wireless/realtek/rtw88/reg.h | 12 + > .../net/wireless/realtek/rtw88/rtw8821cs.c | 35 + > .../net/wireless/realtek/rtw88/rtw8822bs.c | 35 + > .../net/wireless/realtek/rtw88/rtw8822cs.c | 35 + > drivers/net/wireless/realtek/rtw88/sdio.c | 1251 +++++++++++++++++ > drivers/net/wireless/realtek/rtw88/sdio.h | 175 +++ > include/linux/mmc/sdio_ids.h | 9 + > 13 files changed, 1654 insertions(+), 8 deletions(-) > create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8821cs.c > create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822bs.c > create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822cs.c > create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.c > create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.h Martin, I am not qualified to review the code, but I am integrating this version into my rtw88 repo at GitHub.com. It is essential that a successful build is possible after every patch is applied so that an arbitrary bisection will not fail to build. This patch series fails after #2 is committed. File mac.c needs symbol SDIO_LOCAL_OFFSET, which was moved from mac.h to sdio.h. I resolved this be including sdio.h in mac.c. This breaks #3, where you add the include to mac.c. It needs to happen one patch earlier. The other problem for my repo is that it cannot modify include/linux/mmc/sdio_ids.h, thus I have to create a local sdio_ids.h to contain the new definitions. Once your patches are in the kernel, I will be able to eliminate this work around. I do not have any rtw88 SDIO devices, thus I will not be able to test, but I will pass any information that I get from my users. Larry
Hello Larry, On Sat, Mar 11, 2023 at 3:16 AM Larry Finger <Larry.Finger@lwfinger.net> wrote: [...] > I am not qualified to review the code, but I am integrating this version into my > rtw88 repo at GitHub.com. > > It is essential that a successful build is possible after every patch is applied > so that an arbitrary bisection will not fail to build. This patch series fails > after #2 is committed. File mac.c needs symbol SDIO_LOCAL_OFFSET, which was > moved from mac.h to sdio.h. I resolved this be including sdio.h in mac.c. This > breaks #3, where you add the include to mac.c. It needs to happen one patch earlier. Thank you for spotting and reporting this issue! You are right with this, I'll add the sdio.h include to mac.c with patch 2 to resolve this issue as you suggested. > The other problem for my repo is that it cannot modify > include/linux/mmc/sdio_ids.h, thus I have to create a local sdio_ids.h to > contain the new definitions. Once your patches are in the kernel, I will be able > to eliminate this work around. You can also modify the three SDIO driver files (rtw8821cs.c, rtw8822bs.c, rtw8822cs.c) and use the literal IDs there if you have to patch those files anyways. > I do not have any rtw88 SDIO devices, thus I will not be able to test, but I > will pass any information that I get from my users. That sounds great - thank you! Best regards, Martin
> -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Saturday, March 11, 2023 4:29 AM > To: linux-wireless@vger.kernel.org > Cc: Yan-Hsuan Chuang <tony0620emma@gmail.com>; Kalle Valo <kvalo@kernel.org>; Ulf Hansson > <ulf.hansson@linaro.org>; linux-kernel@vger.kernel.org; netdev@vger.kernel.org; > linux-mmc@vger.kernel.org; Chris Morgan <macroalpha82@gmail.com>; Nitin Gupta <nitin.gupta981@gmail.com>; > Neo Jou <neojou@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; > Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Subject: [PATCH v2 RFC 2/9] wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets > > Add a sub-driver for SDIO based chipsets which implements the following > functionality: > - register accessors for 8, 16 and 32 bits for all states of the card > (including usage of 4x 8 bit access for one 32 bit buffer if the card > is not fully powered on yet - or if it's fully powered on then 1x 32 > bit access is used) > - checking whether there's space in the TX FIFO queue to transmit data > - transfers from the host to the device for actual network traffic, > reserved pages (for firmware download) and H2C (host-to-card) > transfers > - receiving data from the device > - deep power saving state > > The transmit path is optimized so DMA-capable SDIO host controllers can > directly use the buffers provided because the buffer's physical > addresses are 8 byte aligned. > > The receive path is prepared to support RX aggregation where the > chipset combines multiple MAC frames into one bigger buffer to reduce > SDIO transfer overhead. > > Co-developed-by: Jernej Skrabec <jernej.skrabec@gmail.com> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > Changes since v1: > - fixed size_t printk format in rtw_sdio_{read,write}_port as reported > by the Intel kernel test robot > - return -EINVAL from the 11n wcpu case in rtw_sdio_check_free_txpg to > fix an uninitialized variable (pages_free) warning as reported by > the Intel kernel test robot > - rename all int *ret to int *err_ret for better consistency with the > sdio_readX functions as suggested by Ping-Ke > - fix typos to use "if (!*err_ret ..." (to read the error code) > instead of "if (!err_ret ..." (which just checks if a non-null > pointer was passed) in rtw_sdio_read_indirect{8,32}) > - use a u8 tmp variable for reading the indirect status (BIT(4)) in > rtw_sdio_read_indirect32 > - change buf[0] to buf[i] in rtw_sdio_read_indirect_bytes > - remove stray semicolon after rtw_sdio_get_tx_qsel > - add proper BIT_RXDMA_AGG_PG_TH, BIT_DMA_AGG_TO_V1, BIT_HCI_SUS_REQ, > BIT_HCI_RESUME_RDY and BIT_SDIO_PAD_E5 macros as suggested by > Ping-Ke (thanks for sharing these names!) > - use /* ... */ style for copyright comments > - don't infinitely loop in rtw_sdio_process_tx_queue and limit the > number of skbs to process per queue to 1000 in rtw_sdio_tx_handler > - add bus_claim check to rtw_sdio_read_port() so it works similar to > rtw_sdio_write_port() (meaning it can be used from interrupt and > non interrupt context) > - enable RX aggregation on all chips except RTL8822CS (where it hurts > RX performance) > - use rtw_tx_fill_txdesc_checksum() helper instead of open-coding it > - re-use RTW_FLAG_POWERON instead of a new .power_switch callback > - added Ulf's Reviewed-by (who had a look at the SDIO specific bits, > thank you!) > > > drivers/net/wireless/realtek/rtw88/Kconfig | 3 + > drivers/net/wireless/realtek/rtw88/Makefile | 3 + > drivers/net/wireless/realtek/rtw88/debug.h | 1 + > drivers/net/wireless/realtek/rtw88/mac.h | 1 - > drivers/net/wireless/realtek/rtw88/reg.h | 12 + > drivers/net/wireless/realtek/rtw88/sdio.c | 1251 +++++++++++++++++++ > drivers/net/wireless/realtek/rtw88/sdio.h | 175 +++ > 7 files changed, 1445 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.c > create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.h > [...] > diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c > new file mode 100644 > index 000000000000..915d641d9226 > --- /dev/null > +++ b/drivers/net/wireless/realtek/rtw88/sdio.c > @@ -0,0 +1,1251 @@ > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause > +/* Copyright (C) 2021 Martin Blumenstingl <martin.blumenstingl@googlemail.com> > + * Copyright (C) 2021 Jernej Skrabec <jernej.skrabec@gmail.com> > + * > + * Based on rtw88/pci.c: > + * Copyright(c) 2018-2019 Realtek Corporation > + */ > + > +#include <linux/module.h> > +#include <linux/mmc/host.h> > +#include <linux/mmc/sdio_func.h> > +#include "sdio.h" > +#include "reg.h" > +#include "tx.h" > +#include "rx.h" > +#include "fw.h" > +#include "ps.h" > +#include "debug.h" How about making them in alphabetical order? [...] > +static void rtw_sdio_rx_skb(struct rtw_dev *rtwdev, struct sk_buff *skb, > + u32 pkt_offset, struct rtw_rx_pkt_stat *pkt_stat, > + struct ieee80211_rx_status *rx_status) > +{ > + memcpy(IEEE80211_SKB_RXCB(skb), rx_status, sizeof(*rx_status)); nit: IEEE80211_SKB_RXCB(skb) = *rx_status; Then, compiler can help to check the type. > + > + if (pkt_stat->is_c2h) { > + skb_put(skb, pkt_stat->pkt_len + pkt_offset); > + rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb); > + return; > + } > + > + skb_put(skb, pkt_stat->pkt_len); > + skb_reserve(skb, pkt_offset); > + > + rtw_rx_stats(rtwdev, pkt_stat->vif, skb); > + > + ieee80211_rx_irqsafe(rtwdev->hw, skb); > +} > + > +static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len) > +{ > + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv; > + const struct rtw_chip_info *chip = rtwdev->chip; > + u32 pkt_desc_sz = chip->rx_pkt_desc_sz; > + struct ieee80211_rx_status rx_status; > + struct rtw_rx_pkt_stat pkt_stat; > + struct sk_buff *skb, *split_skb; > + u32 pkt_offset, curr_pkt_len; > + size_t bufsz; > + u8 *rx_desc; > + int ret; > + > + bufsz = sdio_align_size(rtwsdio->sdio_func, rx_len); > + > + skb = dev_alloc_skb(bufsz); > + if (!skb) > + return; > + > + ret = rtw_sdio_read_port(rtwdev, skb->data, bufsz); > + if (ret) { > + dev_kfree_skb_any(skb); > + return; > + } > + > + while (true) { > + rx_desc = skb->data; > + chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, > + &rx_status); > + pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + > + pkt_stat.shift; > + > + curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len, > + RTW_SDIO_DATA_PTR_ALIGN); > + > + if ((curr_pkt_len + pkt_desc_sz) >= rx_len) { > + /* Use the original skb (with it's adjusted offset) > + * when processing the last (or even the only) entry to > + * have it's memory freed automatically. > + */ > + rtw_sdio_rx_skb(rtwdev, skb, pkt_offset, &pkt_stat, > + &rx_status); > + break; > + } > + > + split_skb = dev_alloc_skb(curr_pkt_len); > + if (!split_skb) { > + rtw_sdio_rx_skb(rtwdev, skb, pkt_offset, &pkt_stat, > + &rx_status); > + break; > + } > + > + skb_copy_header(split_skb, skb); > + memcpy(split_skb->data, skb->data, curr_pkt_len); > + > + rtw_sdio_rx_skb(rtwdev, split_skb, pkt_offset, &pkt_stat, > + &rx_status); > + > + /* Move to the start of the next RX descriptor */ > + skb_reserve(skb, curr_pkt_len); > + rx_len -= curr_pkt_len; > + } > +} > + > +static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev) > +{ > + u32 rx_len; > + > + while (true) { I forget if we have discussed this in v1, but it would be better to have a hard retry limit in driver, like 500. Will we miss to receive packets if break this loop early? > + if (rtw_chip_wcpu_11n(rtwdev)) > + rx_len = rtw_read16(rtwdev, REG_SDIO_RX0_REQ_LEN); > + else > + rx_len = rtw_read32(rtwdev, REG_SDIO_RX0_REQ_LEN); > + > + if (!rx_len) > + break; > + > + rtw_sdio_rxfifo_recv(rtwdev, rx_len); > + } > +} > + [...]
> -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Saturday, March 11, 2023 4:29 AM > To: linux-wireless@vger.kernel.org > Cc: Yan-Hsuan Chuang <tony0620emma@gmail.com>; Kalle Valo <kvalo@kernel.org>; Ulf Hansson > <ulf.hansson@linaro.org>; linux-kernel@vger.kernel.org; netdev@vger.kernel.org; > linux-mmc@vger.kernel.org; Chris Morgan <macroalpha82@gmail.com>; Nitin Gupta <nitin.gupta981@gmail.com>; > Neo Jou <neojou@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; > Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Subject: [PATCH v2 RFC 3/9] wifi: rtw88: mac: Support SDIO specific bits in the power on sequence > > Add the code specific to SDIO HCI in the MAC power on sequence. This is > based on the RTL8822BS and RTL8822CS vendor drivers. > > Co-developed-by: Jernej Skrabec <jernej.skrabec@gmail.com> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > Changes since v1: > - only access REG_SDIO_HIMR for RTW_HCI_TYPE_SDIO > - use proper BIT_HCI_SUS_REQ, BIT_HCI_RESUME_RDY and BIT_SDIO_PAD_E5 > macros as suggested by Ping-Ke > > > drivers/net/wireless/realtek/rtw88/mac.c | 46 +++++++++++++++++++++--- > 1 file changed, 42 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/mac.c b/drivers/net/wireless/realtek/rtw88/mac.c > index cfdfc8a2c836..17704394cca3 100644 > --- a/drivers/net/wireless/realtek/rtw88/mac.c > +++ b/drivers/net/wireless/realtek/rtw88/mac.c > @@ -7,6 +7,7 @@ > #include "reg.h" > #include "fw.h" > #include "debug.h" > +#include "sdio.h" > > void rtw_set_channel_mac(struct rtw_dev *rtwdev, u8 channel, u8 bw, > u8 primary_ch_idx) > @@ -60,6 +61,7 @@ EXPORT_SYMBOL(rtw_set_channel_mac); > > static int rtw_mac_pre_system_cfg(struct rtw_dev *rtwdev) > { > + unsigned int retry; > u32 value32; > u8 value8; > > @@ -77,6 +79,28 @@ static int rtw_mac_pre_system_cfg(struct rtw_dev *rtwdev) > case RTW_HCI_TYPE_PCIE: > rtw_write32_set(rtwdev, REG_HCI_OPT_CTRL, BIT_USB_SUS_DIS); > break; > + case RTW_HCI_TYPE_SDIO: > + rtw_write8_clr(rtwdev, REG_SDIO_HSUS_CTRL, BIT_HCI_SUS_REQ); > + > + for (retry = 0; retry < RTW_PWR_POLLING_CNT; retry++) { > + if (rtw_read8(rtwdev, REG_SDIO_HSUS_CTRL) & BIT_HCI_RESUME_RDY) > + break; > + > + usleep_range(10, 50); > + } > + > + if (retry == RTW_PWR_POLLING_CNT) { > + rtw_err(rtwdev, "failed to poll REG_SDIO_HSUS_CTRL[1]"); > + return -ETIMEDOUT; > + } > + > + if (rtw_sdio_is_sdio30_supported(rtwdev)) > + rtw_write8_set(rtwdev, REG_HCI_OPT_CTRL + 2, > + BIT_SDIO_PAD_E5 >> 16); > + else > + rtw_write8_clr(rtwdev, REG_HCI_OPT_CTRL + 2, > + BIT_SDIO_PAD_E5 >> 16); > + break; > case RTW_HCI_TYPE_USB: > break; > default: > @@ -248,6 +272,7 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on) > { > const struct rtw_chip_info *chip = rtwdev->chip; > const struct rtw_pwr_seq_cmd **pwr_seq; > + u32 imr; > u8 rpwm; > bool cur_pwr; > int ret; > @@ -273,18 +298,24 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on) > if (pwr_on == cur_pwr) > return -EALREADY; > > + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) { > + imr = rtw_read32(rtwdev, REG_SDIO_HIMR); > + rtw_write32(rtwdev, REG_SDIO_HIMR, 0); > + } > + > if (!pwr_on) > clear_bit(RTW_FLAG_POWERON, rtwdev->flags); > > pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq; > ret = rtw_pwr_seq_parser(rtwdev, pwr_seq); > - if (ret) > - return ret; > + > + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) > + rtw_write32(rtwdev, REG_SDIO_HIMR, imr); > > if (pwr_on) > set_bit(RTW_FLAG_POWERON, rtwdev->flags); If failed to power on, it still set RTW_FLAG_POWERON. Is it reasonable? Did you meet real problem here? Maybe, here can be if (pwr_on && !ret) set_bit(RTW_FLAG_POWERON, rtwdev->flags); > > - return 0; > + return ret; > } > > static int __rtw_mac_init_system_cfg(struct rtw_dev *rtwdev) > @@ -455,6 +486,9 @@ static void download_firmware_reg_backup(struct rtw_dev *rtwdev, > rtw_write16(rtwdev, REG_FIFOPAGE_INFO_1, 0x200); > rtw_write32(rtwdev, REG_RQPN_CTRL_2, bckp[bckp_idx - 1].val); > > + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) > + rtw_read32(rtwdev, REG_SDIO_FREE_TXPG); > + > /* Disable beacon related functions */ > tmp = rtw_read8(rtwdev, REG_BCN_CTRL); > bckp[bckp_idx].len = 1; > @@ -1067,8 +1101,12 @@ static int txdma_queue_mapping(struct rtw_dev *rtwdev) > if (rtw_chip_wcpu_11ac(rtwdev)) > rtw_write32(rtwdev, REG_H2CQ_CSR, BIT_H2CQ_FULL); > > - if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB) > + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) { > + rtw_read32(rtwdev, REG_SDIO_FREE_TXPG); > + rtw_write32(rtwdev, REG_SDIO_TX_CTRL, 0); > + } else if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB) { > rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_ARBBW_EN); > + } > > return 0; > } > -- > 2.39.2
> -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Saturday, March 11, 2023 4:29 AM > To: linux-wireless@vger.kernel.org > Cc: Yan-Hsuan Chuang <tony0620emma@gmail.com>; Kalle Valo <kvalo@kernel.org>; Ulf Hansson > <ulf.hansson@linaro.org>; linux-kernel@vger.kernel.org; netdev@vger.kernel.org; > linux-mmc@vger.kernel.org; Chris Morgan <macroalpha82@gmail.com>; Nitin Gupta <nitin.gupta981@gmail.com>; > Neo Jou <neojou@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; > Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Subject: [PATCH v2 RFC 9/9] wifi: rtw88: Add support for the SDIO based RTL8821CS chipset > > Wire up RTL8821CS chipset support using the new rtw88 SDIO HCI code as > well as the existing RTL8821C chipset code. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > Changes since v1: > - use /* ... */ style for copyright comments > > > drivers/net/wireless/realtek/rtw88/Kconfig | 11 ++++++ > drivers/net/wireless/realtek/rtw88/Makefile | 3 ++ > .../net/wireless/realtek/rtw88/rtw8821cs.c | 35 +++++++++++++++++++ > 3 files changed, 49 insertions(+) > create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8821cs.c > [...] > diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821cs.c > b/drivers/net/wireless/realtek/rtw88/rtw8821cs.c > new file mode 100644 > index 000000000000..7ad7c13ac9e6 > --- /dev/null > +++ b/drivers/net/wireless/realtek/rtw88/rtw8821cs.c > @@ -0,0 +1,35 @@ > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause > +/* Copyright(c) Martin Blumenstingl <martin.blumenstingl@googlemail.com> > + */ > + > +#include <linux/mmc/sdio_func.h> > +#include <linux/mmc/sdio_ids.h> > +#include <linux/module.h> > +#include "sdio.h" > +#include "rtw8821c.h" nit: alphabetic order I run sparse/smatch with this patchset, and smatch warns: 1. drivers/net/wireless/realtek/rtw88/mac.c:313 rtw_mac_power_switch() error: uninitialized symbol 'imr'. This should be a false-alarm, but just initialize imr to 0 to avoid this. 2. drivers/net/wireless/realtek/rtw88/sdio.c:136 rtw_sdio_read_indirect_bytes() error: uninitialized symbol 'ret'. This should be a false-alarm too. I guess it considers 'count = 0' is possible. Ping-Ke
Hello Ping-Ke, On Mon, Mar 13, 2023 at 10:05 AM Ping-Ke Shih <pkshih@realtek.com> wrote: [...] > > pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq; > > ret = rtw_pwr_seq_parser(rtwdev, pwr_seq); > > - if (ret) > > - return ret; > > + > > + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) > > + rtw_write32(rtwdev, REG_SDIO_HIMR, imr); > > > > if (pwr_on) > > set_bit(RTW_FLAG_POWERON, rtwdev->flags); > > If failed to power on, it still set RTW_FLAG_POWERON. Is it reasonable? That sounds very reasonable to me! > Did you meet real problem here? > > Maybe, here can be > > if (pwr_on && !ret) > set_bit(RTW_FLAG_POWERON, rtwdev->flags); I can't remember any issue that I've seen. I'll verify this at the end of the week (until then I am pretty busy with my daytime job) and then go with your suggestion. Thanks again as always - your feedback is really appreciated! Also thank you for commenting on the other patches. I'll take a closer look at your feedback at the end of the week and send another version of this series. Best regards, Martin
> -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Tuesday, March 14, 2023 4:12 AM > To: Ping-Ke Shih <pkshih@realtek.com> > Cc: linux-wireless@vger.kernel.org; Yan-Hsuan Chuang <tony0620emma@gmail.com>; Kalle Valo > <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; linux-mmc@vger.kernel.org; Chris Morgan <macroalpha82@gmail.com>; Nitin Gupta > <nitin.gupta981@gmail.com>; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com> > Subject: Re: [PATCH v2 RFC 3/9] wifi: rtw88: mac: Support SDIO specific bits in the power on sequence > > Hello Ping-Ke, > > On Mon, Mar 13, 2023 at 10:05 AM Ping-Ke Shih <pkshih@realtek.com> wrote: > [...] > > > pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq; > > > ret = rtw_pwr_seq_parser(rtwdev, pwr_seq); > > > - if (ret) > > > - return ret; > > > + > > > + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) > > > + rtw_write32(rtwdev, REG_SDIO_HIMR, imr); > > > > > > if (pwr_on) > > > set_bit(RTW_FLAG_POWERON, rtwdev->flags); > > > > If failed to power on, it still set RTW_FLAG_POWERON. Is it reasonable? > That sounds very reasonable to me! Let me clear here more. Consider a use case: 1. Initially, it enters this function with pwr_on = true, and RTW_FLAG_POWERON is unset. 2. rtw_pwr_seq_parser() return error, so power state is uncertain. 3. Unconditionally, set RTW_FLAG_POWERON. rtw_mac_power_on() will try to power off/on once again if it fails, so in step 3 setting RTW_FLAG_POWERON only if rtw_pwr_seq_parser() returns 0 can have the same values as step 1, when it retries to power on. Honestly, we don't have perfect error handle for error of rtw_pwr_seq_parser(), but still want to make things easier understand. > > > Did you meet real problem here? > > > > Maybe, here can be > > > > if (pwr_on && !ret) > > set_bit(RTW_FLAG_POWERON, rtwdev->flags); > I can't remember any issue that I've seen. I'll verify this at the end > of the week (until then I am pretty busy with my daytime job) and then > go with your suggestion. Thanks. Wait for you. > Thanks again as always - your feedback is really appreciated! > > Also thank you for commenting on the other patches. I'll take a closer > look at your feedback at the end of the week and send another version > of this series. > I also thank you for cooking these patches voluntarily for people who can use their own wifi happily. :-) Ping-Ke
Recently the rtw88 driver has gained locking support for the "slow" bus types (USB, SDIO) as part of USB support. Thanks to everyone who helped make this happen! Based on the USB work (especially the locking part and various bugfixes) this series adds support for SDIO based cards. It's the result of a collaboration between Jernej and myself. Neither of us has access to the rtw88 datasheets. All of our work is based on studying the RTL8822BS and RTL8822CS vendor drivers and trial and error. Jernej and myself have tested this with RTL8822BS and RTL8822CS cards. Other users have confirmed that RTL8821CS support is working as well. RTL8723DS may also work (we tried our best to handle rtw_chip_wcpu_11n where needed) but has not been tested at this point. Jernej's results with a RTL8822BS: - Main functionality works - Had a case where no traffic got across the link until he issued a scan My results with a RTL8822CS: - 2.4GHz and 5GHz bands are both working - TX throughput on a 5GHz network is between 50 Mbit/s and 90 Mbit/s - RX throughput on a 5GHz network is at 19 Mbit/s (this seems to be an combination of the location of my board and the cheap antenna which are both hurting RX performance) A user shared his results on his own RTL8822CS off-list with me: - 50Mbit/s throughput in both directions A user shared his results on RTL8821CS off-list with me: - 50Mbps down and 25Mbps on a 5GHz network Why is this an RFC? - I think it's worth to get another round of feedback from the rtw88 maintainers - As with most patches: testing is very welcome. If things are working fine then a Tested-by is appreciated (with some details about the card, throughput, ...). If something doesn't work for you: please still report back so we can investigate that problem! Changes since v1 at [0]: - removed patches 1-8 as they have been submitted and separately (they were indepdent and this helped cutting down the size of this series) - dropped patch "rtw88: ps: Increase LEAVE_LPS_TRY_CNT for SDIO based chipsets" as the underlying issue has been fixed - most likely with upstream commit 823092a53556eb ("wifi: rtw88: fix race condition when doing H2C command") - rework the code so we don't need a new HCI specific power_switch callback by utilizing the RTW_FLAG_POWERON flag which was recently introduced - various patches include the feedback from reviewers and build testing robots (see the individual patches for details) [0] https://lore.kernel.org/lkml/a2449a2d1e664bcc8962af4667aa1290@realtek.com/T/ Jernej Skrabec (1): wifi: rtw88: Add support for the SDIO based RTL8822BS chipset Martin Blumenstingl (8): wifi: rtw88: Clear RTW_FLAG_POWERON early in rtw_mac_power_switch() wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets wifi: rtw88: mac: Support SDIO specific bits in the power on sequence wifi: rtw88: main: Add the {cpwm,rpwm}_addr for SDIO based chipsets wifi: rtw88: main: Reserve 8 bytes of extra TX headroom for SDIO cards mmc: sdio: add Realtek SDIO vendor ID and various wifi device IDs wifi: rtw88: Add support for the SDIO based RTL8822CS chipset wifi: rtw88: Add support for the SDIO based RTL8821CS chipset drivers/net/wireless/realtek/rtw88/Kconfig | 36 + drivers/net/wireless/realtek/rtw88/Makefile | 12 + drivers/net/wireless/realtek/rtw88/debug.h | 1 + drivers/net/wireless/realtek/rtw88/mac.c | 51 +- drivers/net/wireless/realtek/rtw88/mac.h | 1 - drivers/net/wireless/realtek/rtw88/main.c | 9 +- drivers/net/wireless/realtek/rtw88/reg.h | 12 + .../net/wireless/realtek/rtw88/rtw8821cs.c | 35 + .../net/wireless/realtek/rtw88/rtw8822bs.c | 35 + .../net/wireless/realtek/rtw88/rtw8822cs.c | 35 + drivers/net/wireless/realtek/rtw88/sdio.c | 1251 +++++++++++++++++ drivers/net/wireless/realtek/rtw88/sdio.h | 175 +++ include/linux/mmc/sdio_ids.h | 9 + 13 files changed, 1654 insertions(+), 8 deletions(-) create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8821cs.c create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822bs.c create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822cs.c create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.c create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.h