-
Notifications
You must be signed in to change notification settings - Fork 10
fix aic8800 WiFi & BT on lichee-pi-4a #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR refactors RFKill GPIO handling to use the DT-friendly of_get_named_gpio interface with updated property names, strengthens cleanup logic, switches the WLAN/BT drivers to build as loadable modules, enables UART1/3 in the Lichee-Pi-4A device tree, and bumps the AIC8800 Wi-Fi driver to v4.0. ER diagram for device tree UART additionserDiagram
SOC ||--o{ UART1 : enables
SOC ||--o{ UART3 : enables
SOC {
string name
}
UART1 {
int id
string status
}
UART3 {
int id
string status
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nekorouter - I've reviewed your changes - here's some feedback:
- The PR description mentions adding UART1 & UART3 entries, but I don’t see any DTS updates for those in the diff—please include them or remove that reference to avoid confusion.
- Since you’re converting rfkill-wlan and rfkill-bt into loadable modules, please verify that Kconfig/module dependencies are updated so they load in the correct order under CONFIG_RFKILL_GPIO.
- Consider switching to the gpiod descriptor API (e.g. devm_gpiod_get_optional()) for GPIO handling to simplify resource management instead of manual of_get_named_gpio()/gpio_free() calls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PR description mentions adding UART1 & UART3 entries, but I don’t see any DTS updates for those in the diff—please include them or remove that reference to avoid confusion.
- Since you’re converting rfkill-wlan and rfkill-bt into loadable modules, please verify that Kconfig/module dependencies are updated so they load in the correct order under CONFIG_RFKILL_GPIO.
- Consider switching to the gpiod descriptor API (e.g. devm_gpiod_get_optional()) for GPIO handling to simplify resource management instead of manual of_get_named_gpio()/gpio_free() calls.
## Individual Comments
### Comment 1
<location> `net/rfkill/rfkill-wlan.c:198` </location>
<code_context>
struct rfkill_wlan_data *rfkill = platform_get_drvdata(pdev);
- if (gpio_is_valid(rfkill->pdata->power_n.io))
+// if (gpio_is_valid(rfkill->pdata->power_n.io))
+ if (rfkill && rfkill->pdata && gpio_is_valid(rfkill->pdata->power_n.io))
gpio_free(rfkill->pdata->power_n.io);
-
</code_context>
<issue_to_address>
Added null checks for rfkill and rfkill->pdata before gpio_is_valid.
If rfkill or rfkill->pdata is null, it may signal a logic issue elsewhere. Consider adding logging or explicit handling for these cases.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// if (gpio_is_valid(rfkill->pdata->power_n.io))
if (rfkill && rfkill->pdata && gpio_is_valid(rfkill->pdata->power_n.io))
gpio_free(rfkill->pdata->power_n.io);
=======
if (!rfkill) {
pr_err("%s: rfkill is NULL in %s\n", __func__, __FILE__);
} else if (!rfkill->pdata) {
pr_err("%s: rfkill->pdata is NULL in %s\n", __func__, __FILE__);
} else if (gpio_is_valid(rfkill->pdata->power_n.io)) {
gpio_free(rfkill->pdata->power_n.io);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `net/rfkill/rfkill-wlan.c:201` </location>
<code_context>
gpio_free(rfkill->pdata->power_n.io);
-
- kfree(rfkill);
+ if (rfkill)
+ kfree(rfkill);
g_rfkill = NULL;
</code_context>
<issue_to_address>
Redundant null check before kfree.
Since kfree handles NULL pointers safely, you can remove the explicit null check to simplify the code.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
net/rfkill/rfkill-wlan.c
Outdated
// if (gpio_is_valid(rfkill->pdata->power_n.io)) | ||
if (rfkill && rfkill->pdata && gpio_is_valid(rfkill->pdata->power_n.io)) | ||
gpio_free(rfkill->pdata->power_n.io); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Added null checks for rfkill and rfkill->pdata before gpio_is_valid.
If rfkill or rfkill->pdata is null, it may signal a logic issue elsewhere. Consider adding logging or explicit handling for these cases.
// if (gpio_is_valid(rfkill->pdata->power_n.io)) | |
if (rfkill && rfkill->pdata && gpio_is_valid(rfkill->pdata->power_n.io)) | |
gpio_free(rfkill->pdata->power_n.io); | |
if (!rfkill) { | |
pr_err("%s: rfkill is NULL in %s\n", __func__, __FILE__); | |
} else if (!rfkill->pdata) { | |
pr_err("%s: rfkill->pdata is NULL in %s\n", __func__, __FILE__); | |
} else if (gpio_is_valid(rfkill->pdata->power_n.io)) { | |
gpio_free(rfkill->pdata->power_n.io); | |
} |
if (rfkill) | ||
kfree(rfkill); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Redundant null check before kfree.
Since kfree handles NULL pointers safely, you can remove the explicit null check to simplify the code.
734ffae
to
657df0c
Compare
Fix functions in rfkill-wlan / rfkill-bt for getting gpio name from devicetree. Fix null pointer error in rfkill-wlan. Signed-off-by: KamijoToma <[email protected]> Signed-off-by: NekoRouter <[email protected]>
Add definition for uart1 and uart3. Signed-off-by: NekoRouter <[email protected]>
While working with the T-Head 1520 LicheePi4A SoC, certain conditions arose that allowed me to reproduce a race issue in the sdhci code. To reproduce the bug, you need to enable the sdio1 controller in the device tree file `arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi` as follows: &sdio1 { bus-width = <4>; max-frequency = <100000000>; no-sd; no-mmc; broken-cd; cap-sd-highspeed; post-power-on-delay-ms = <50>; status = "okay"; wakeup-source; keep-power-in-suspend; }; When resetting the SoC using the reset button, the following messages appear in the dmesg log: [ 8.164898] mmc2: Got command interrupt 0x00000001 even though no command operation was in progress. [ 8.174054] mmc2: sdhci: ============ SDHCI REGISTER DUMP =========== [ 8.180503] mmc2: sdhci: Sys addr: 0x00000000 | Version: 0x00000005 [ 8.186950] mmc2: sdhci: Blk size: 0x00000000 | Blk cnt: 0x00000000 [ 8.193395] mmc2: sdhci: Argument: 0x00000000 | Trn mode: 0x00000000 [ 8.199841] mmc2: sdhci: Present: 0x03da0000 | Host ctl: 0x00000000 [ 8.206287] mmc2: sdhci: Power: 0x0000000f | Blk gap: 0x00000000 [ 8.212733] mmc2: sdhci: Wake-up: 0x00000000 | Clock: 0x0000decf [ 8.219178] mmc2: sdhci: Timeout: 0x00000000 | Int stat: 0x00000000 [ 8.225622] mmc2: sdhci: Int enab: 0x00ff1003 | Sig enab: 0x00ff1003 [ 8.232068] mmc2: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000 [ 8.238513] mmc2: sdhci: Caps: 0x3f69c881 | Caps_1: 0x08008177 [ 8.244959] mmc2: sdhci: Cmd: 0x00000502 | Max curr: 0x00191919 [ 8.254115] mmc2: sdhci: Resp[0]: 0x00001009 | Resp[1]: 0x00000000 [ 8.260561] mmc2: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000 [ 8.267005] mmc2: sdhci: Host ctl2: 0x00001000 [ 8.271453] mmc2: sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000 [ 8.278594] mmc2: sdhci: ============================================ I also enabled some traces to better understand the problem: kworker/3:1-62 [003] ..... 8.163538: mmc_request_start: mmc2: start struct mmc_request[000000000d30cc0c]: cmd_opcode=5 cmd_arg=0x0 cmd_flags=0x2e1 cmd_retries=0 stop_opcode=0 stop_arg=0x0 stop_flags=0x0 stop_retries=0 sbc_opcode=0 sbc_arg=0x0 sbc_flags=0x0 sbc_retires=0 blocks=0 block_size=0 blk_addr=0 data_flags=0x0 tag=0 can_retune=0 doing_retune=0 retune_now=0 need_retune=0 hold_retune=1 retune_period=0 <idle>-0 [000] d.h2. 8.164816: sdhci_cmd_irq: hw_name=ffe70a0000.mmc quirks=0x2008008 quirks2=0x8 intmask=0x10000 intmask_p=0x18000 irq/24-mmc2-96 [000] ..... 8.164840: sdhci_thread_irq: msg= irq/24-mmc2-96 [000] d.h2. 8.164896: sdhci_cmd_irq: hw_name=ffe70a0000.mmc quirks=0x2008008 quirks2=0x8 intmask=0x1 intmask_p=0x1 irq/24-mmc2-96 [000] ..... 8.285142: mmc_request_done: mmc2: end struct mmc_request[000000000d30cc0c]: cmd_opcode=5 cmd_err=-110 cmd_resp=0x0 0x0 0x0 0x0 cmd_retries=0 stop_opcode=0 stop_err=0 stop_resp=0x0 0x0 0x0 0x0 stop_retries=0 sbc_opcode=0 sbc_err=0 sbc_resp=0x0 0x0 0x0 0x0 sbc_retries=0 bytes_xfered=0 data_err=0 tag=0 can_retune=0 doing_retune=0 retune_now=0 need_retune=0 hold_retune=1 retune_period=0 Here's what happens: the __mmc_start_request function is called with opcode 5. Since the power to the Wi-Fi card, which resides on this SDIO bus, is initially off after the reset, an interrupt SDHCI_INT_TIMEOUT is triggered. Immediately after that, a second interrupt SDHCI_INT_RESPONSE is triggered. Depending on the exact timing, these conditions can trigger the following race problem: 1) The sdhci_cmd_irq top half handles the command as an error. It sets host->cmd to NULL and host->pending_reset to true. 2) The sdhci_thread_irq bottom half is scheduled next and executes faster than the second interrupt handler for SDHCI_INT_RESPONSE. It clears host->pending_reset before the SDHCI_INT_RESPONSE handler runs. 3) The pending interrupt SDHCI_INT_RESPONSE handler gets called, triggering a code path that prints: "mmc2: Got command interrupt 0x00000001 even though no command operation was in progress." To solve this issue, we need to clear pending interrupts when resetting host->pending_reset. This ensures that after sdhci_threaded_irq restores interrupts, there are no pending stale interrupts. The behavior observed here is non-compliant with the SDHCI standard. Place the code in the sdhci-of-dwcmshc driver to account for a hardware-specific quirk instead of the core SDHCI code. Signed-off-by: Michal Wilczynski <[email protected]> Acked-by: Adrian Hunter <[email protected]> Fixes: 43658a5 ("mmc: sdhci-of-dwcmshc: Add support for T-Head TH1520") Cc: [email protected] Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Ulf Hansson <[email protected]> Signed-off-by: NekoRouter <[email protected]>
657df0c
to
30dd959
Compare
TH1520 has 6 serial port, expand 8250 runtime uart port number up to 6 to match with hardware. Signed-off-by: NekoRouter <[email protected]>
For rtl8723/aic8800 different hardware compatibility, using systemd to manually attach HCI port; This definition can conflict with aic8800, so disable it for now. Signed-off-by: NekoRouter <[email protected]>
Update aic8800 driver to 20241119 (aic8800d_linux_sdk_V4.0_2024_1119_06da8476). Signed-off-by: NekoRouter <[email protected]>
fix aic8800 WiFi & BT on lichee-pi-4a
Summary by Sourcery
Fix GPIO-based RF kill handling and module configuration for the AIC8800 wireless on the Lichee-Pi-4A.
Enhancements:
Build: