Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221227233020.284266-1-martin.blumenstingl@googlemail.com>
 <20221227233020.284266-13-martin.blumenstingl@googlemail.com> <2a9e671ef17444238fee3e7e6f14484b@realtek.com>
In-Reply-To: <2a9e671ef17444238fee3e7e6f14484b@realtek.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Wed, 28 Dec 2022 12:59:42 +0100
Message-ID: <CAFBinCDVq6o0c6OLSD0PhQKFPrXohjhdJeXk=5wuDEWMKwufrA@mail.gmail.com>
Subject: Re: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO
 based chipsets
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     "linux-wireless@vger.kernel.org" <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" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-mmc@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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Ping-Ke,

as always: thank you so much for taking time to go through this!

On Wed, Dec 28, 2022 at 10:39 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
[...]
>
> > +
> > +static void rtw_sdio_writel(struct rtw_sdio *rtwsdio, u32 val,
> > +                         u32 addr, int *ret)
> > +{
> > +     u8 buf[4];
> > +     int i;
> > +
> > +     if (!(addr & 3) && rtwsdio->is_powered_on) {
> > +             sdio_writel(rtwsdio->sdio_func, val, addr, ret);
> > +             return;
> > +     }
> > +
> > +     *(__le32 *)buf = cpu_to_le32(val);
> > +
> > +     for (i = 0; i < 4; i++) {
> > +             sdio_writeb(rtwsdio->sdio_func, buf[i], addr + i, ret);
> > +             if (*ret)
>
> Do you need some messages to know something wrong?
It's not obvious but we're already logging that something went wrong.
The messages are logged in rtw_sdio_{read,write}{8,16,32}.
We do this because there's multiple ways to access data (direct,
indirect, ...) and some of them require multiple register operations.
So we print one message in the end.

[...]
> > +static u8 rtw_sdio_read_indirect8(struct rtw_dev *rtwdev, u32 addr, int *ret)
> > +{
> > +     struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > +     u32 reg_cfg, reg_data;
> > +     int retry;
> > +     u8 tmp;
> > +
> > +     reg_cfg = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_CFG);
> > +     reg_data = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_DATA);
> > +
> > +     rtw_sdio_writel(rtwsdio, BIT(19) | addr, reg_cfg, ret);
> > +     if (*ret)
> > +             return 0;
> > +
> > +     for (retry = 0; retry < RTW_SDIO_INDIRECT_RW_RETRIES; retry++) {
> > +             tmp = sdio_readb(rtwsdio->sdio_func, reg_cfg + 2, ret);
> > +             if (!ret && tmp & BIT(4))
>
> 'ret' is pointer, do you need '*' ?
Well spotted - thank you!

[...]
> As I look into sdio_readb(), it use 'int *err_ret' as arugment.
> Would you like to change ' int *ret' to 'int *err_ret'?
> It could help to misunderstand.
Sure, I'll do that

[...]
> > +             rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, size |
> > +                         (timeout << BIT_SHIFT_DMA_AGG_TO_V1));
>
> BIT_RXDMA_AGG_PG_TH GENMASK(7, 0)       // for size
> BIT_DMA_AGG_TO_V1 GENMASK(15, 8)        // for timeout
Thanks, I'll use these

[...]
> > +static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
> > +{
> > +     u32 rx_len;
> > +
> > +     while (true) {
>
> add a limit to prevent infinite loop.
Do you have any recommendations on how many packets to pull in one go?
My thinking is: pulling to little data at once can hurt performance

[...]
>
> > +
> > +static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> > +                                   enum rtw_tx_queue_type queue)
> > +{
> > +     struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > +     struct sk_buff *skb;
> > +     int ret;
> > +
> > +     while (true) {
>
> Can we have a limit?
Similar to the question above: do you have any recommendations on how
many packets (per queue) to send in one go?


Best regards,
Martin