Enable AMD 1-wire AXI IP and Zynq GPIO drivers #3795
Conversation
There was a problem hiding this comment.
Hi @michalsimek
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
📝 WalkthroughWalkthroughThe pull request introduces two new kernel configuration options in the Changes
Note: Since the changes are straightforward kernel configuration modifications, a sequence diagram is not applicable in this scenario. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
IIUC the features are to be used in the generic-aarch64 image. In that case I would suggest to apply them in buildroot-external/board/arm-uefi/generic-aarch64/kernel.config. Definitely the CONFIG_GPIO_ZYNQ needs to be moved there, as it depends on CONFIG_ARCH_ZYNQ || CONFIG_ARCH_ZYNQMP and could end up disabled in boards that include device-support.config but do not have that architecture enabled. That would trigger our kernel dotconfig checker in CI. The other option does not have a hard dependency but seems to be ARM-specific as well.
Also, with the options moved to the other fragment, they will be still applied once the Linux 6.12 is merged, without any further changes needed.
|
Yes. Kria is ARM based system. https://www.amd.com/en/products/system-on-modules/kria.html |
|
Looks better now, thanks. I'm just confused by the comments, how is the 1-wire option related to CAN bus? 🤔 |
Zynq GPIO driver is used on AMD/Xilinx Kria platform for ETH phy reset. Macb and PHY drivers are already enabled. 1 wire IP can be used for reading sensors via PMOD connector.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buildroot-external/board/arm-uefi/generic-aarch64/kernel.config(1 hunks)
🔇 Additional comments (1)
buildroot-external/board/arm-uefi/generic-aarch64/kernel.config (1)
64-65: Verify 1-wire dependencies and add documentation.The AMD AXI 1-wire driver is correctly configured as a module. However, please verify the following:
Consider adding documentation to clarify the intended use:
# 1-wire +# CONFIG_W1_MASTER_AMD_AXI: Required for reading sensors via PMOD connector on AMD/Xilinx Kria platform CONFIG_W1_MASTER_AMD_AXI=mAdditionally, given the confusion in PR comments about CAN bus relation, please clarify in the PR description that this 1-wire support is independent of CAN functionality.
✅ Verification successful
Dependencies are correctly configured
The AMD AXI 1-wire driver is properly configured as a module, with core 1-wire support (CONFIG_W1) provided by the base device-support.config. The original documentation suggestion remains valid for clarity:
# 1-wire +# CONFIG_W1_MASTER_AMD_AXI: Required for reading sensors via PMOD connector on AMD/Xilinx Kria platform CONFIG_W1_MASTER_AMD_AXI=m🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for required 1-wire dependencies in kernel config # Expected: CONFIG_W1 should be enabled, CONFIG_W1_SLAVE_THERM might be needed for temperature sensors # Search for W1 core config echo "Checking W1 core configuration:" rg -l 'CONFIG_W1=y' || echo "CONFIG_W1 not found" # Search for W1 slave temp sensor config (if needed) echo "Checking W1 temperature sensor configuration:" rg -l 'CONFIG_W1_SLAVE_THERM=m' || echo "CONFIG_W1_SLAVE_THERM not found"Length of output: 502
Script:
#!/bin/bash # Check if device-support.config is included and search for all W1 configs echo "Checking generic-aarch64 kernel config contents:" cat buildroot-external/board/arm-uefi/generic-aarch64/kernel.config echo -e "\nChecking for W1 configs in generic-aarch64:" rg "CONFIG_W1" buildroot-external/board/arm-uefi/generic-aarch64/kernel.config || echo "No W1 configs found"Length of output: 1586
| # GPIO support | ||
| CONFIG_GPIO_ZYNQ=m |
There was a problem hiding this comment.
💡 Codebase verification
Missing critical dependencies for Zynq GPIO driver
The Zynq GPIO driver (CONFIG_GPIO_ZYNQ) requires the following dependencies which are not configured:
- CONFIG_GPIO_CORE: Required for basic GPIO subsystem support
- CONFIG_OF: Required for device tree support
Please add these configurations to ensure the Zynq GPIO driver functions correctly for Ethernet PHY reset on the Kria platform:
+# GPIO core support
+CONFIG_GPIO_CORE=y
+# Device Tree support
+CONFIG_OF=y
# GPIO support
# CONFIG_GPIO_ZYNQ: Required for Ethernet PHY reset on AMD/Xilinx Kria platform
CONFIG_GPIO_ZYNQ=m🔗 Analysis chain
Verify GPIO core dependencies and add documentation.
The Zynq GPIO driver is correctly configured as a module. However, please verify the following:
Consider adding a comment explaining the Kria platform requirement:
# GPIO support
+# CONFIG_GPIO_ZYNQ: Required for Ethernet PHY reset on AMD/Xilinx Kria platform
CONFIG_GPIO_ZYNQ=m🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for required GPIO dependencies in kernel config
# Expected: CONFIG_GPIO_CORE and CONFIG_OF should be enabled
# Search for GPIO core config
echo "Checking GPIO core configuration:"
rg -l 'CONFIG_GPIO_CORE=y' || echo "CONFIG_GPIO_CORE not found"
# Search for OF (Open Firmware) config
echo "Checking OF configuration:"
rg -l 'CONFIG_OF=y' || echo "CONFIG_OF not found"
Length of output: 298
Script:
#!/bin/bash
# Broader search for GPIO and OF configurations
echo "Checking for any GPIO core settings:"
rg -l 'CONFIG_GPIO_CORE[= ]' || echo "No GPIO core config found"
echo -e "\nChecking for any OF settings:"
rg -l 'CONFIG_OF[= ]' || echo "No OF config found"
echo -e "\nChecking for related GPIO configs:"
rg 'CONFIG_GPIO' buildroot-external/board/arm-uefi/generic-aarch64/kernel.config
echo -e "\nChecking for config includes:"
rg '^source|^\.include' buildroot-external/board/arm-uefi/generic-aarch64/kernel.config
Length of output: 670
There was a problem hiding this comment.
@sairon Do you want me to do suggested changes by bot? CONFIG_OF is already enabled and CONFIG_GPIO_CORE doesn't exist in v6.6 or v6.12.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@michalsimek No need, unfortunately, Coderabbit is not particularly good at knowing Linux in detail. Sometimes it helps but here it's really hallucinating, just ignore it. The dependencies of the added options are met, so LGTM.
Zynq GPIO driver is used on AMD/Xilinx Kria platform for ETH phy reset.
Macb and PHY drivers are already enabled.
1 wire IP can be used for reading sensors via PMOD connector.
Would be also good to enable these drivers in upcoming 6.12 kernel.
Summary by CodeRabbit