-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Change Cadence i2c and Zynq GPIO from modules to built-in drivers #3821
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the kernel configuration for an ARM UEFI generic AArch64 board by changing the state of the Changes
Sequence DiagramsequenceDiagram
participant Kernel
participant I2C Cadence Driver
participant I2C Devices
Kernel->>I2C Cadence Driver: Load Driver
I2C Cadence Driver-->>Kernel: Driver Initialized
Kernel->>I2C Devices: Enable Communication
The sequence diagram illustrates the basic flow of the I2C Cadence driver initialization, showing how the kernel loads the driver and enables communication with I2C devices. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
buildroot-external/board/arm-uefi/generic-aarch64/kernel.config (1)
72-74
: Consider adding a comment explaining the built-in requirement.To help future maintainers understand why this driver must be built-in rather than a module, consider adding a brief comment.
# i2c support +# Must be built-in for early boot I2C access (required for USB hub reset) CONFIG_I2C_CADENCE=y
📜 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)
72-74
: LGTM! The change correctly enables built-in I2C Cadence driver support.The modification to build the I2C Cadence driver into the kernel rather than as a module is the right approach, as it ensures I2C functionality is available during early boot when the root filesystem is being mounted.
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.
I will consider pulling it in to final 14.2 release, however, have you verified this is really all it's needed for that platform? It'd be pointless without that.
I have removed reset part inside usb DWC3 node and try to boot that system and things booted properly. |
I think there's not an easy way to only build the kernel without building the toolchain, even if you wanted just to generate the kernel's dotconfig in the same way as Buildroot does. On the other hand, the build environment is self-contained by the helper scripts, so to build the kernel, run:
Alternatively, I can run an on-demand build here in the CI, or send you any of the files built/generated locally on my machine (which would be a bit faster). |
On Kria KD240 slg7xl45106 device is handling reset for USB hub which is providing access to SD card (USB/SD converter). Access to this device is done via i2c which needs to be also enabled in the kernel as built-in driver not as module when rootfs is mounted. Also change ZYNQ_GPIO to be built-in driver because i2c is using gpio for bus recovery that's why it should be also enabled to probe i2c driver properly. v6.6 kernel doesn't have support for usb5744 driver that's why disable it but add TODO to enable it once v6.12 upgrade is done.
First of all thank you for providing steps how to do it. Current v6.6 configuration without this patch is working with DT modification on kd240 (https://www.amd.com/en/products/system-on-modules/kria/k24/kd240-drives-starter-kit.html) and without any DT modification on kv260 (https://www.amd.com/en/products/system-on-modules/kria/k26/kv260-vision-starter-kit.html). To get it working without DT hack on kd240 But that being said in v6.6 usb5744 driver is not supported and likely would be better (but doesn't really matter) to ignore CONFIG_USB_ONBOARD_HUB completely and enable it in v6.12 where hub is enabled via I also merged #3767 to test v6.12 and validated that USB hub configuration is working properly when above config options are enabled. That being said I have updated my patch to reflect what described above and it is fine for me if you want to pull it to v6.6 kernel or wait for v6.12 or add it after 14.2 release. |
Just in case if you are interested how that bootlog looks like on v6.6 (with origin kernel modules on rootfs and v6.12 (without kernel modules on rootfs). |
On Kria KD240 slg7xl45106 device is handling reset for USB hub which is providing access to SD card (USB/SD converter). Access to this device is done via i2c which needs to be also enabled in the kernel as built-in driver not as module when rootfs is mounted.
I have tested 14.2.rc2 version and this one is also missing. I pointed via grub to DT without usb hub configuration and it boots properly.
Summary by CodeRabbit