On Mon, Apr 03, 2023 at 10:24:33PM +0200, Martin Blumenstingl wrote:
> 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@xxxxxxxxx>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
Hi Martin,
some minor nits from my side that you may wish to consider
if you need to respin the series for some other reason.
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> new file mode 100644
> index 000000000000..038e209e6107
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -0,0 +1,1387 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/* Copyright (C) 2021 Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> + * Copyright (C) 2021 Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> + *
> + * 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 "main.h"
> +#include "debug.h"
> +#include "fw.h"
> +#include "ps.h"
> +#include "reg.h"
> +#include "rx.h"
> +#include "sdio.h"
> +#include "tx.h"
> +
> +#define RTW_SDIO_INDIRECT_RW_RETRIES 50
> +
> +static bool rtw_sdio_is_bus_addr(u32 addr)
> +{
> + return (addr & RTW_SDIO_BUS_MSK) != 0;
> +}
nit: this could be.
return !!(addr & RTW_SDIO_BUS_MSK)
...
> +static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
> +{
> + struct ieee80211_hw *hw = sdio_get_drvdata(sdio_func);
> + struct rtw_dev *rtwdev = hw->priv;
> + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> + u32 hisr;
nit: reverse xmas tree - longest line to shortest - for local variable
declarations.
So I guess (*completely untested*):
struct ieee80211_hw *hw = sdio_get_drvdata(sdio_func);
struct rtw_dev *rtwdev = hw->priv;
struct rtw_sdio *rtwsdio;
u32 hisr;
rtwsdio = (struct rtw_sdio *)rtwdev->priv;
...
> +static void rtw_sdio_tx_handler(struct work_struct *work)
> +{
> + struct rtw_sdio_work_data *work_data =
> + container_of(work, struct rtw_sdio_work_data, work);
> + struct rtw_dev *rtwdev = work_data->rtwdev;
> + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> + int limit, queue;
Reverse xmas tree again.
...
> +void rtw_sdio_shutdown(struct device *dev)
> +{
> + struct sdio_func *sdio_func = dev_to_sdio_func(dev);
> + struct ieee80211_hw *hw = sdio_get_drvdata(sdio_func);
> + const struct rtw_chip_info *chip;
> + struct rtw_dev *rtwdev;
Ditto.
...
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.h b/drivers/net/wireless/realtek/rtw88/sdio.h
...
> +/* Free Tx Page Sequence */
> +#define REG_SDIO_FREE_TXPG_SEQ (SDIO_LOCAL_OFFSET + 0x0028)
> +/* HTSF Informaion */
nit: s/Informaion/Information/
...
