Re: [PATCH v4 2/9] wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets

[Simon Horman]

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/

...