Skip to content

Conversation

@RichardSWheatley
Copy link
Member

@RichardSWheatley RichardSWheatley commented Aug 29, 2025

Use the Security CRC in HW

@github-actions
Copy link

github-actions bot commented Aug 29, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
ambiqhal_ambiq AmbiqMicro/ambiqhal_ambiq@e3a37c2 AmbiqMicro/ambiqhal_ambiq@6777fad (apollo5-udpate-for-security) AmbiqMicro/[email protected]

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

This comment was marked as outdated.

@RichardSWheatley RichardSWheatley force-pushed the ambiq-crc-driver branch 2 times, most recently from 45dcbb9 to faa639b Compare August 31, 2025 16:20
@RichardSWheatley
Copy link
Member Author

@AlessandroLuo
Can you request Copilot again?

I addressed the things it mentioned.

@AlessandroLuo AlessandroLuo requested a review from Copilot August 31, 2025 16:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR creates an Ambiq CRC driver that utilizes the Security CRC hardware module for CRC32 calculations. It provides hardware-accelerated CRC functionality for Ambiq Apollo4P and Apollo510 SoCs.

  • Adds new CRC driver API and Ambiq-specific hardware implementation
  • Creates comprehensive sample application demonstrating CRC driver usage
  • Updates device trees and board configurations to enable CRC32 module

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
west.yml Updates Ambiq HAL revision to support CRC functionality
samples/drivers/crc/* Complete sample application with documentation and configuration
include/zephyr/drivers/crc/crc.h New CRC driver API header
dts/bindings/crc/ambiq,hw-crc32.yaml Device tree binding for Ambiq CRC32 hardware
dts/arm/ambiq/*.dtsi Device tree definitions for CRC32 module across Ambiq SoCs
drivers/crc/* Ambiq CRC driver implementation and build configuration
drivers/Kconfig, drivers/CMakeLists.txt Integration of CRC subsystem into driver framework
boards/ambiq/*/**.dts Board-specific CRC module enablement
Comments suppressed due to low confidence (1)

boards/ambiq/apollo4p_evb/apollo4p_evb-pinctrl.dtsi:1

  • This PWM configuration appears to be unrelated to the CRC driver changes and seems like an accidental inclusion in this PR. It should be removed or moved to a separate PWM-related PR.
/*

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@RichardSWheatley RichardSWheatley force-pushed the ambiq-crc-driver branch 2 times, most recently from 36341c6 to 9395ccf Compare August 31, 2025 16:31
Use the Security CRC in HW

Signed-off-by: Richard Wheatley <[email protected]>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

};

DEVICE_DT_INST_DEFINE(0, crc_ambiq_init, NULL, NULL, NULL, POST_KERNEL,
CONFIG_KERNEL_INIT_PRIORITY_DEFAULT,
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device definition uses hardcoded instance 0 and CONFIG_KERNEL_INIT_PRIORITY_DEFAULT. Consider using CONFIG_CRC_INIT_PRIORITY defined in drivers/crc/Kconfig for consistency with other driver subsystems.

Suggested change
CONFIG_KERNEL_INIT_PRIORITY_DEFAULT,
CONFIG_CRC_INIT_PRIORITY,

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants