Skip to content

Conversation

@swift-tk
Copy link
Contributor

@swift-tk swift-tk commented Apr 21, 2025

  1. Add APS Z8 hex PSRAM driver (APS256xxN, APS256xxB, APS512xxB) APS51216B
  2. Add IS25xx0xx driver (IS25WX064)
  3. Add Ambiq MSPI timing scan utility

More device variants support is dropped from APS Z8 driver due to #91166

@swift-tk swift-tk self-assigned this Apr 21, 2025
@swift-tk swift-tk requested a review from AlessandroLuo April 21, 2025 10:42
@github-actions
Copy link

github-actions bot commented Apr 21, 2025

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

Name Old Revision New Revision Diff

All manifest checks OK

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

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 adds MSPI support to the Apollo510 SoC while also addressing various driver fixes and enhancements. Key changes include:

  • Updates to MSPI driver code across memory controllers, flash, and GPIO modules with improved dummy handling and priority settings.
  • Introduction of new macro definitions and board support files for the Apollo510 EVB.
  • Minor adjustments in driver implementations (e.g. replacing bit-shift vendor ID extraction with memcpy and adding a brief sleep in busy-wait loops) to improve robustness.

Reviewed Changes

Copilot reviewed 68 out of 81 changed files in this pull request and generated no comments.

Show a summary per file
File Description
drivers/memc/memc_mspi_aps6404l.c Added dummy transfer assignments and updated Ambiq timing macros
drivers/gpio/gpio_ambiq.c Added gpio_ambiq header include and implemented a new pin-number helper
drivers/flash/flash_mspi_emul_device.c Updated transfer priority definitions for MSPI flash transactions
drivers/flash/flash_mspi_atxp032.c Updated dummy handling, vendor ID extraction using memcpy, added sleep, and refined macro definitions
boards/ambiq/apollo510_evb/* Added new board support files (YAML and C) for the Apollo510 SoC EVB
Files not reviewed (13)
  • boards/ambiq/apollo510_evb/CMakeLists.txt: Language not supported
  • boards/ambiq/apollo510_evb/Kconfig: Language not supported
  • boards/ambiq/apollo510_evb/Kconfig.apollo510_evb: Language not supported
  • boards/ambiq/apollo510_evb/Kconfig.defconfig: Language not supported
  • boards/ambiq/apollo510_evb/apollo510_evb-pinctrl.dtsi: Language not supported
  • boards/ambiq/apollo510_evb/apollo510_evb.dts: Language not supported
  • boards/ambiq/apollo510_evb/apollo510_evb_defconfig: Language not supported
  • boards/ambiq/apollo510_evb/board.cmake: Language not supported
  • boards/ambiq/apollo510_evb/doc/index.rst: Language not supported
  • drivers/flash/CMakeLists.txt: Language not supported
  • drivers/flash/Kconfig.mspi: Language not supported
  • drivers/memc/CMakeLists.txt: Language not supported
  • drivers/memc/Kconfig.mspi: Language not supported
Comments suppressed due to low confidence (3)

drivers/flash/flash_mspi_atxp032.c:266

  • Replacing the previous bit-shift assignment with memcpy may affect the intended byte ordering of the JEDEC ID. Please verify that this change correctly preserves the intended endianness.
memcpy(&data->jedec_id, buffer + 7, 3);

drivers/flash/flash_mspi_atxp032.c:381

  • The introduction of a sleep in the busy-wait loop might introduce unnecessary delays in time-critical operations. Confirm that a 1ms delay is acceptable for the required performance.
k_sleep(K_MSEC(1));

drivers/gpio/gpio_ambiq.c:576

  • Please verify that the calculation for adjusting the pin number using an offset shift (for non-Apollo3X series) correctly maps to the hardware configuration. Double-check that this behavior aligns with expected GPIO pin numbering for the target devices.
gpio_pin_t ambiq_gpio_get_pinnum(const struct device *dev, gpio_pin_t pin)

@swift-tk swift-tk force-pushed the apollo510-mspi-merge branch 9 times, most recently from bbdca1a to 78f1d28 Compare April 22, 2025 07:46
@decsny
Copy link
Member

decsny commented Jun 3, 2025

Okay, I understand now from the meeting that the issue here is that the second compatible doesn't export it's binding to the node. I was not getting this during the meeting for a while.

In that case then I think that the correct way to handle this here is to remove the timing parameters from DT, and put them in source code. I do not think that an intermediate "timing port" node should be created because it sounds like the SPI device node should be directly on the bus of the MSPI controller in DT to accurately describe the hardware, and introducing vendor specific intermediate nodes might complicate future generic MSPI support such as any kind of like "DT spec" type of macros.

I will also point out that the way NXP describes the parameters needed to configure our flexspi to talk to a particular flash or ram chip right now is by hardcoding those parameters in a board specific C file. It sounds like this could be the same for now but specified in a format to be given for the mspi API.

Then the issue of having two compatibles can be moot, since there would only be one and the main compatible can be the proper one for the device.

I also doubt that you are choosing these timing parameters for the controller randomly. It feels like there must be some way to describe the device properly in a generic way in DT that you can use to derive the correct controller timing parameters. What properties of the devices are causing you to choose those timing parameters? Perhaps consider describing those traits in DT instead of describing your controller's reaction to them, and then code programmatically how to configure the controller to communicate to a device with those properties.

@swift-tk
Copy link
Contributor Author

swift-tk commented Jun 3, 2025

In response to @danieldegrasse proposal, I agree that it can be another way to get around. But I have some concerns.

  • Lifting the timing params at the controller level makes its relationship to the device implicit (like the ce-gpios) as the timing params is constrained by a number of other things(such as rx-dummy) in the device’s dts as well as the frequency of device.
  • This would change the behavior of MSPI controllers reacting to the call of mspi_timing_config API, as it would need to recognize whether it is called during runtime or initialization. (Which is not explicitly supported yet)
  • What if there are more than 1 timing param set for the device. Say write in quad-1-1-4 requires one set and then read in quad-1-4-4 requires another.
  • The suggested method would trigger massive rework. Though I’m fine with it.

I’m not sure about the other proposal for creating another node inside of the device that represents the variant? Am I understanding it correctly?

@swift-tk
Copy link
Contributor Author

swift-tk commented Jun 3, 2025

I also doubt that you are choosing these timing parameters for the controller randomly. It feels like there must be some way to describe the device properly in a generic way in DT that you can use to derive the correct controller timing parameters. What properties of the devices are causing you to choose those timing parameters? Perhaps consider describing those traits in DT instead of describing your controller's reaction to them, and then code programmatically how to configure the controller to communicate to a device with those properties.

The important part of controller timing parameters are not specific to any of its device settings. They are obtained from actual timing scans if you look into the ambiq_timing_scan.c, so the timing param can be runtime configurable.
If you look at my reply earlier, there is also the problem of multiple timing params for a single device.

@nashif
Copy link
Member

nashif commented Jun 3, 2025

My claim is that there are always going to be special cases and limitations in the way we do things and in it is not documented as a hard rule and is rather some guideance helping to decide when to use Kcofnig or DTS. If there is away to deal with this based on the various suggestions above, that's great, if not, then we should document the limitation/gap somewhere, maybe in a bug/enhancement, put this in a comment and get this unblocked and revisit when a proper solution that is fully DTS based.

@swift-tk swift-tk force-pushed the apollo510-mspi-merge branch from 6454fc8 to 650ea0f Compare June 6, 2025 03:37
@swift-tk
Copy link
Contributor Author

swift-tk commented Jun 6, 2025

I've removed the kconfig in dispute so the driver would just support only one variant that is on our PCB for now...
I have opened another issue to discuss how should we handle vendor specific timing for MSPI.

@swift-tk swift-tk requested a review from nordicjm June 6, 2025 03:41
@swift-tk swift-tk removed DNM This PR should not be merged (Do Not Merge) Architecture Review Discussion in the Architecture WG required labels Jun 6, 2025
@swift-tk swift-tk force-pushed the apollo510-mspi-merge branch 5 times, most recently from 367be68 to 380662d Compare June 6, 2025 07:12
@swift-tk
Copy link
Contributor Author

swift-tk commented Jun 6, 2025

Proposed solution from arch meeting:

&mspi0 {
        ce-gpios = <&gpio0 0>, /* maps to psram0 */
                   <&gpio0 1>; /* maps to psram1 */

        /* maps to psram0 */
        timings-0 = {
                compatible = "ambiq,timings";
		ambiq,timing-config-mask = <0x62>;
		ambiq,timing-config = <8 7 1 0 0 3 10>;
        };

        /* maps to psram1 */
        timings-1 = {
                compatible = "ambiq,timings";
		ambiq,timing-config-mask = <0x62>;
		ambiq,timing-config = <8 7 1 0 0 3 10>;
        };

 	psram0: aps51216ba@0 {
		compatible = "mspi-aps-z8"; /* actual hardware device here, no soc specific timings */
                ....
        };

 	psram1: aps51216ba@1 {
		compatible = "mspi-aps-z8"; /* actual hardware device here, no soc specific timings */
                ....
        };
};

There are many ways to do this, but the important thing is timings are moved out from the device node and instead "mapped" to it like the CE GPIOs

A hint would be the child-binding property used by pinctrl and gpio-hogs for example :)

child-binding:
description: |
Optional GPIO hog configuration.
Each GPIO controller may contain GPIO hog definitions. GPIO hogging is a mechanism for
providing automatic GPIO configuration during driver initialization when Kconfig
CONFIG_GPIO_HOGS is enabled.
Each GPIO hog is represented as a child node of the GPIO controller.
Example:
&gpio1 {
mux-hog {
gpio-hog;
gpios = <10 GPIO_ACTIVE_HIGH>, <11 GPIO_ACTIVE_HIGH>;
output-high;
};
};
&gpio2 {
test-hog {
gpio-hog;
gpios = <26 GPIO_ACTIVE_HIGH>;
output-low;
line-name = "test";
};
};
properties:
gpio-hog:
type: boolean
required: true
description: |
Conveys this node is a GPIO hog.
gpios:
type: array
required: true
description: |
This is an array of GPIO specifiers (e.g. pin, flags) to be hogged. The number of array
entries must be an integer multiple of the number of GPIO specifier cells for the parent
GPIO controller.
input:
type: boolean
description: |
If this property is set, the GPIO is configured as an input. This property takes
precedence over the output-low and output-high properties.
output-low:
type: boolean
description: |
If this property is set, the GPIO is configured as an output set to logical low. This
property takes precedence over the output-high property.
output-high:
type: boolean
description: |
If this property is set, the GPIO is configured as an output set to logical high.
line-name:
type: string
description: |
Optional GPIO line name.

I just found out that the proprosal from @bjarki-andreasen has a critical issue i.e. it creates more childs for the controller and it would mess up DT_CHILD_NUM and DT_CHILD_NUM_STATUS_OKAY which mspi uses to count the max and active number of mspi peripheral devices to perform indexing, and also checking it against the reg value of each peripheral devices.

@bjarki-andreasen
Copy link
Contributor

Proposed solution from arch meeting:

&mspi0 {
        ce-gpios = <&gpio0 0>, /* maps to psram0 */
                   <&gpio0 1>; /* maps to psram1 */

        /* maps to psram0 */
        timings-0 = {
                compatible = "ambiq,timings";
		ambiq,timing-config-mask = <0x62>;
		ambiq,timing-config = <8 7 1 0 0 3 10>;
        };

        /* maps to psram1 */
        timings-1 = {
                compatible = "ambiq,timings";
		ambiq,timing-config-mask = <0x62>;
		ambiq,timing-config = <8 7 1 0 0 3 10>;
        };

 	psram0: aps51216ba@0 {
		compatible = "mspi-aps-z8"; /* actual hardware device here, no soc specific timings */
                ....
        };

 	psram1: aps51216ba@1 {
		compatible = "mspi-aps-z8"; /* actual hardware device here, no soc specific timings */
                ....
        };
};

There are many ways to do this, but the important thing is timings are moved out from the device node and instead "mapped" to it like the CE GPIOs
A hint would be the child-binding property used by pinctrl and gpio-hogs for example :)

child-binding:
description: |
Optional GPIO hog configuration.
Each GPIO controller may contain GPIO hog definitions. GPIO hogging is a mechanism for
providing automatic GPIO configuration during driver initialization when Kconfig
CONFIG_GPIO_HOGS is enabled.
Each GPIO hog is represented as a child node of the GPIO controller.
Example:
&gpio1 {
mux-hog {
gpio-hog;
gpios = <10 GPIO_ACTIVE_HIGH>, <11 GPIO_ACTIVE_HIGH>;
output-high;
};
};
&gpio2 {
test-hog {
gpio-hog;
gpios = <26 GPIO_ACTIVE_HIGH>;
output-low;
line-name = "test";
};
};
properties:
gpio-hog:
type: boolean
required: true
description: |
Conveys this node is a GPIO hog.
gpios:
type: array
required: true
description: |
This is an array of GPIO specifiers (e.g. pin, flags) to be hogged. The number of array
entries must be an integer multiple of the number of GPIO specifier cells for the parent
GPIO controller.
input:
type: boolean
description: |
If this property is set, the GPIO is configured as an input. This property takes
precedence over the output-low and output-high properties.
output-low:
type: boolean
description: |
If this property is set, the GPIO is configured as an output set to logical low. This
property takes precedence over the output-high property.
output-high:
type: boolean
description: |
If this property is set, the GPIO is configured as an output set to logical high.
line-name:
type: string
description: |
Optional GPIO line name.

I just found out that the proprosal from @bjarki-andreasen has a critical issue i.e. it creates more childs for the controller and it would mess up DT_CHILD_NUM and DT_CHILD_NUM_STATUS_OKAY which mspi uses to count the max and active number of mspi peripheral devices to perform indexing, and also checking it against the reg value of each peripheral devices.

See the response in the RFC you created :)

@nordicjm nordicjm dismissed their stale review June 9, 2025 07:38

conflict part removed from PR

@carlescufi carlescufi moved this from Todo to In Progress in Architecture Review Jun 10, 2025
swift-tk added 3 commits June 17, 2025 13:33
The APS Z8 driver would just support APS51216BA for now.

Signed-off-by: Swift Tian <[email protected]>
This device driver supports ISSI is25w/lx032/64 series flash.
Only extended SPI mode(1s-1s-1s, 1s-8s-8s, 1s-1s-8s) is implemented.

Signed-off-by: Swift Tian <[email protected]>
The utility may be used during development stage to get
ambiq platform specific timing parameters for mspi devices.

Signed-off-by: Swift Tian <[email protected]>
@swift-tk swift-tk force-pushed the apollo510-mspi-merge branch from 380662d to 9e1b042 Compare June 17, 2025 05:35
@swift-tk
Copy link
Contributor Author

rebased as the workaround for #90777 is merged.

@sonarqubecloud
Copy link

@kartben kartben merged commit 69c14e3 into zephyrproject-rtos:main Jun 18, 2025
27 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Architecture Review Jun 18, 2025
@swift-tk swift-tk deleted the apollo510-mspi-merge branch August 1, 2025 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

9 participants