On 10/23/24 7:54 PM, Ajay.Kathat@xxxxxxxxxxxxx wrote:
Hello Marek,
Hi,
What I am trying to say is this:
With current code, this can happen, which is not good, because transfers
from multiple threads can be interleaved and interfere with each other:
Did you observe any bus failure in your test setup with SDIO. Is there any
configure to recreate it.
I am observing sporadic command and data CRC errors on STM32MP157F
system with SDIO WILC3000.
Does this patch help to resolve the CRC errors?
No
Do you observe the CRC errors during the bulk data transfer(iPerf) or does it
even happen during the basic WiFi operation like(scan/connect or basic ping
operation).
I can trigger them during this test:
$ while true ; ifconfig wlan0 up ; ifconfig wlan0 down ; done
But they happen sporadically and seemingly at random.
I already stuffed the driver with trace_printk()s through and through,
but the trace log does not indicate anything that would be off in any way.
Is power-save enabled during the test. With PS enabled, The SDIO commands may
fail momentarily but it should recover.
It seems it gets enabled after first ifconfig up, that's a good hint,
I'll try to disable it and see if that makes them errors go away. Thanks!
Do you have any details on WHY would such sporadic errors occur and how
to make those go away even with PS enabled ?
The SDIO transfer may appear to be split into into multiple transaction but
these calls should not impact each other since most of them are updating the
different registers except WILC_SDIO_FBR_CSA_REG register, which is used for
CSA read/write. If needed, wilc_sdio_set_func0_csa_address() can be modified
to club the 3x CMD52 and 1x CMD53 into a single transfer API.
In my opinion, If sdio_claim_host() is moved to acquire_bus() API then the
SDIO bus will be claimed longer than required especially in
wilc_wlan_handle_txq() API. Ideally, calling sdio_claim_host() call just
before the transfer is enough but now the SDIO I/O bus would be continuously
blocked for multiple independent SDIO transactions that is not necessary.
Why would that pose a problem ?
wilc_wlan_handle_txq() performs many operations on different registers which
are not related. It will block the SDIO bux for longer. Otherwise those
registers are allowed to update separately from the WILC SD side.
And is that blocking of SD bus actually a problem ?
I am more concerned that ksdioirqd can insert a command transfer right
in the middle of CMD52/CMD53 register read composite transfer, because
while ksdioirqd does use proper sdio_claim/release_host, this driver
does it per-SDIO-command instead of per the whole e.g. register read
"transaction".
I think, using sdio_claim/release for each-SDIO command should suffice because
the CMD52/CMD53 modify the specific registers that are unrelated. Each
CMD52/53 should work properly and independently provided they are protected
with sdio_claim/release.
Additionally, there is no WILC SD module limitation requiring a strict order
for CMD52/CMD53, except for Card Storage Area (CSA) read/write operations.
Can multiple register IO operations like that be interleaved with
ksdioirqd access to SDIO_CCCR_INTx too ?
Take e.g. wilc_wlan_handle_txq(), is the following order legal?
thread1 | thread2
ret = func->hif_write_reg(wilc, |
WILC_CORTUS_INTERRUPT_BASE, |
1); |
| ksdioirqd {
| ret = mmc_io_rw_direct(card, 0,
| 0, SDIO_CCCR_INTx, 0, pending);
| }
ret = func->hif_read_reg(wilc, |
WILC_CORTUS_INTERRUPT_BASE, |
®); |
For CSA read/write operations, which are necessary to read/write any specific
address from the card, multiple CMD52 commands are used to pass the desired
address to be read/written using registers (0x10c, 0x10d, 0x10e). Then, CMD53
is used to read/write to address 0x10f (CSA data window register). This
complete command sequence is required for a single CSA address read/write.
Based on this requirement, CSA read/write operations can cause issues if they
run in parallel with another CSA read/write operation, but not with other
CMD52 and CMD53 commands.
Unrelated to this, but ... is it possible to send the entire 3-Byte
register CSA address using a single command instead of 3 separate commands ?
Therefore, one approach to resolve this issue is to add sdio_claim/release
around wilc_sdio_set_func0_csa_address(). This way, this function will be
treated as a single operation and it will only modify the required command flow.
I can do the serialization per-command, but please see above.
