[permalink] [raw]

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

Hello Ping-Ke,

On Thu, Mar 23, 2023 at 3:23 AM Ping-Ke Shih <[email protected]> wrote:
[...]
> > + if (direct) {
> > + addr = rtw_sdio_to_bus_offset(rtwdev, addr);
> > + val = rtw_sdio_readl(rtwdev, addr, &ret);
> > + } else if (addr & 3) {
>
> else if (IS_ALIGNED(addr, 4) {
I'll add these IS_ALIGNED in v4
Also I found an issue with RTW_WCPU_11N devices where indirect read
works differently (those can't use
REG_SDIO_INDIRECT_REG_CFG/REG_SDIO_INDIRECT_REG_DATA but need to go
through the normal path with WLAN_IOREG_OFFSET instead). I'll also
include that fix in v4

[...]
> > + ret = rtw_register_hw(rtwdev, hw);
> > + if (ret) {
> > + rtw_err(rtwdev, "failed to register hw");
> > + goto err_destroy_txwq;
> > + }
> > +
>
> Today, people reported there is race condition between register netdev and NAPI
> in rtw89 driver. I wonder if there will be in register netdev and request IRQ.
>
> You can add a msleep(10 * 100) here, and then do 'ifconfig up' and 'iw scan'
> quickly right after SDIO probe to see if it can work well. Otherwise, switching
> the order of rtw_register_hw() and rtw_sdio_request_irq() could be a possible
> solution.
I tried with 1 second and 10 second delays here and could not find any problems.
That said, I am still going to swap the order because a) it seems to
be what most drivers do (ath9k for example) and b) SDIO is a slow bus
and especially slow on my Amlogic SM1 SoC which has some SDIO DMA
errata.
Also testing this made me find another bug during module unregister (I
thought I caught all of them by now) and will include this fix in v4
as well.

As always: thank you for all your inputs!

Best regards,
Martin

2023-03-23 19:07:07

by Martin Blumenstingl

[permalink] [raw]

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

Hello Ping-Ke,

On Thu, Mar 23, 2023 at 12:16 PM Martin Blumenstingl
<[email protected]> wrote:
[...]
> > > + if (direct) {
> > > + addr = rtw_sdio_to_bus_offset(rtwdev, addr);
> > > + val = rtw_sdio_readl(rtwdev, addr, &ret);
> > > + } else if (addr & 3) {
> >
> > else if (IS_ALIGNED(addr, 4) {
> I'll add these IS_ALIGNED in v4
> Also I found an issue with RTW_WCPU_11N devices where indirect read
> works differently (those can't use
> REG_SDIO_INDIRECT_REG_CFG/REG_SDIO_INDIRECT_REG_DATA but need to go
> through the normal path with WLAN_IOREG_OFFSET instead). I'll also
> include that fix in v4
I have a question about the "indirect" handling.
Let me start with what I know:
- REG_SDIO_INDIRECT_REG_CFG and REG_SDIO_INDIRECT_REG_DATA are only
present on RTW_WCPU_11AC based chips (older RTW_WCPU_11N chips don't
have these registers)
- the name of REG_SDIO_INDIRECT_REG_CFG[20] is not known but we're
polling that bit to check if REG_SDIO_INDIRECT_REG_DATA is ready to be
read or has data from REG_SDIO_INDIRECT_REG_DATA has been written
- REG_SDIO_INDIRECT_REG_CFG[19] configures a read operation
- REG_SDIO_INDIRECT_REG_CFG[18] configures a write operation
- REG_SDIO_INDIRECT_REG_CFG[17] indicates that a DWORD (32-bit) are
written to REG_SDIO_INDIRECT_REG_DATA (+ the following 3), this bit
seems irrelevant for read mode
- REG_SDIO_INDIRECT_REG_CFG[16] indicates that a DWORD (16-bit) are
written to REG_SDIO_INDIRECT_REG_DATA (+ the following 3), this bit
seems irrelevant for read mode
- RTW_WCPU_11N chips are simply using "addr | WLAN_IOREG_OFFSET" for
accesses that would usually be "indirect" reads/writes on
RTW_WCPU_11AC chips

While fixing the issue for the RTW_WCPU_11N chips I discovered that
the "old" approach for indirect register access (without
REG_SDIO_INDIRECT_REG_CFG and REG_SDIO_INDIRECT_REG_DATA) also works
on RTW_WCPU_11AC chips.
(I'm calling it the "old" approach because it's what the RTL8723DS an
RTL8723BS vendor drivers use)
In fact, this series is using the "old" approach for writes, but the
new (REG_SDIO_INDIRECT_REG_CFG and REG_SDIO_INDIRECT_REG_DATA based)
approach for reads.
Naturally I'm curious as to why two different approaches achieve the
same goal. Using the "old" approach (addr | WLAN_IOREG_OFFSET) means a
lot of code could be deleted/simplified.

Now my question:
Do you have any explanation (either from internal documentation or
from the hardware/firmware teams) if and when the
REG_SDIO_INDIRECT_REG_CFG and REG_SDIO_INDIRECT_REG_DATA registers
should be used on RTW_WCPU_11AC chips?

Thank you and best regards,
Martin