by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [PATCH v2 RFC 1/9] wifi: rtw88: Clear RTW_FLAG_POWERON early in rtw_mac_power_switch()

2023-03-13 09:04:05

by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [PATCH v2 RFC 2/9] wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Saturday, March 11, 2023 4:29 AM
> To: [email protected]
> Cc: Yan-Hsuan Chuang <[email protected]>; Kalle Valo <[email protected]>; Ulf Hansson
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Chris Morgan <[email protected]>; Nitin Gupta <[email protected]>;
> Neo Jou <[email protected]>; Ping-Ke Shih <[email protected]>; Jernej Skrabec <[email protected]>;
> Martin Blumenstingl <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Jernej Skrabec <[email protected]>
> Reviewed-by: Ulf Hansson <[email protected]>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> 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 <[email protected]>
> + * Copyright (C) 2021 Jernej Skrabec <[email protected]>
> + *
> + * 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);
> + }
> +}
> +

[...]

2023-03-13 09:08:16

by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [PATCH v2 RFC 3/9] wifi: rtw88: mac: Support SDIO specific bits in the power on sequence

2023-03-13 09:11:33

by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [PATCH v2 RFC 4/9] wifi: rtw88: main: Add the {cpwm,rpwm}_addr for SDIO based chipsets

2023-03-13 09:22:38

by Ping-Ke Shih

[permalink] [raw]

Hi Chris,

On Thu, Mar 16, 2023 at 8:59 PM Chris Morgan <[email protected]> wrote:
[...]
> > +MODULE_AUTHOR("Martin Blumenstingl <[email protected]>");
> > +MODULE_DESCRIPTION("Realtek 802.11ac wireless 8821cs driver");
> > +MODULE_LICENSE("Dual BSD/GPL");
> > --
> > 2.39.2
> >
>
> Overall it works well for me, but when I resume from suspend I get the
> following filling up my dmesg:
>
> rtw_8821cs mmc3:0001:1: sdio read8 failed (0x86): -110
>
> So suspend/resume seems to be an issue, but otherwise it works well
> for me.
Thanks for reporting this issue! I have a fix in my local tree and I'm
testing it currently. If you want to try it for yourself (before I
send an updated series) you can just replace one function in sdio.c:
static int __maybe_unused rtw_sdio_suspend(struct device *dev)
{
struct sdio_func *func = dev_to_sdio_func(dev);
struct ieee80211_hw *hw = dev_get_drvdata(dev);
struct rtw_dev *rtwdev = hw->priv;
int ret;

ret = sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
if (ret)
rtw_err(rtwdev, "Failed to host PM flag MMC_PM_KEEP_POWER");

return ret;
}

Best regards,
Martin