Skip to content

Conversation

@ioannis-karachalios
Copy link
Contributor

@ioannis-karachalios ioannis-karachalios commented Oct 7, 2024

This commit deals with fixing various issues that prevents the APS6404 MSPI device from being built. In specific:

  1. Fix default timing macro definitions to build with an MSPI controller, other than AMBIG. Add macro definition for MSPI_PORT.
  2. Timing settings should be applied only when MSPI_TIMING is defined. Otherwise, the APS6404 initialization routine should fail with -EIO.
  3. Similarly, use MSPI_XIP and MSPI_SCRAMBLE to apply XIP and SCRAMBLE device settings, respectively (optimization).
  4. Based on current MEMC_INIT_PRIORITY (0), APS6404 device should be initialized before the underlying MSPI bus is enabled, and so the APS6404 initialization routine should fail during controller's bus check. Change init. priority to a value higher than MSPI_INIT_PRIORITY (70).

Fixes #81151

@zephyrbot zephyrbot requested a review from swift-tk October 7, 2024 15:46
@ioannis-karachalios ioannis-karachalios added the bug The issue is a bug, or the PR is fixing a bug label Oct 7, 2024
@ioannis-karachalios ioannis-karachalios force-pushed the driver-mspi-aps6404-bug-fixes branch from 6b6afaa to 8ac0290 Compare October 7, 2024 15:50
@ioannis-karachalios ioannis-karachalios changed the title drivers: memc: Fix various APS6404 device issues drivers: memc: Fix various APS6404 device driver issues Oct 7, 2024
@swift-tk
Copy link
Contributor

swift-tk commented Oct 8, 2024

@ioannis-karachalios Thanks for filing the PR, I was going to fix those things but apparently you’ve beat me to it😄.

@ioannis-karachalios
Copy link
Contributor Author

@ioannis-karachalios Thanks for filing the PR, I was going to fix those things but apparently you’ve beat me to it😄.

@swift-tk, you're welcome. I am working on a new family of device that is going to support the MSPI driver and I came across these errors.

ydamigos
ydamigos previously approved these changes Oct 8, 2024
@swift-tk
Copy link
Contributor

@ioannis-karachalios Please note that you also have to fix the CI checks before a merge.

@ioannis-karachalios ioannis-karachalios force-pushed the driver-mspi-aps6404-bug-fixes branch from 09cb552 to 4da4fce Compare October 23, 2024 10:25
@RichardSWheatley
Copy link
Contributor

@ioannis-karachalios Thanks for filing the PR, I was going to fix those things but apparently you’ve beat me to it😄.

@swift-tk, you're welcome. I am working on a new family of device that is going to support the MSPI driver and I came across these errors.

I am following as we are creating another device driver for memc. Same family, higher density.

@ioannis-karachalios ioannis-karachalios force-pushed the driver-mspi-aps6404-bug-fixes branch from 4da4fce to b43f46c Compare October 31, 2024 18:56
@ioannis-karachalios
Copy link
Contributor Author

ioannis-karachalios commented Oct 31, 2024

"Please note that you also have to fix the CI checks before a merge."
@swift-tk, to my understanding, the CI errors should be irrelevant to the underlying changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these ifdef's included of the enable is already there?

To save code space?

Copy link
Contributor

@swift-tk swift-tk Nov 4, 2024

Choose a reason for hiding this comment

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

When XIP is not supported by a specific hardware, CONFIG_MSPI_XIP=n is used to indicate that. This also means that xip_config may not be implemented, so calling it in device driver may result in error. Leaving it out of compile will not only save code space but also ensure the device driver is still functional without optional features.
Additionally, one can also choose to disable XIP even when it is supported.

@swift-tk
Copy link
Contributor

swift-tk commented Nov 4, 2024

"Please note that you also have to fix the CI checks before a merge." @swift-tk, to my understanding, the CI errors should be irrelevant to the underlying changes.

Could you try rebase to latest main and try again with the CI. If that don't work, I will ask help in discord.

@ioannis-karachalios ioannis-karachalios force-pushed the driver-mspi-aps6404-bug-fixes branch from b43f46c to f19906e Compare November 4, 2024 18:07
@ioannis-karachalios
Copy link
Contributor Author

Could you try rebase to latest main and try again with the CI. If that don't work, I will ask help in discord.
I re-based to the latest main, but CI reports the same twister errors, again. I'd appreciate if we seek for assistance in discord as my schedule is tight at the moment to further investigate it.

Copy link
Contributor

@swift-tk swift-tk left a comment

Choose a reason for hiding this comment

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

I did not notice the default 0 was gone.

This commit deals with fixing various issues that prevents
the device from being built. In specific:

1. Fix default timing macro definitions to build with
an MSPI controller, other than AMBIG.
Add macro definition for MSPI_PORT.
2. Timing settings should be applied only when MSPI_TIMING
is defined. Otherwise, the APS6404 initialization routine
will fail with -EIO.
3. Similarly, use MSPI_XIP and MSPI_SCRAMBLE to apply XIP
and SCRAMBLE device settings, respectively (optimization).
4. MEMC_INIT_PRIORITY is assigned higher priority than
MSPI_INIT_PRIORITY which results in compiler error as
APS6404 device initialization depends on its underlying
MSPI bus controller.
5. The 'acquire' subroutine should be compiled when PM_DEVICE
is used (suppress compiler warning).

Signed-off-by: Ioannis Karachalios <[email protected]>
@ioannis-karachalios
Copy link
Contributor Author

I did not notice the default 0 was gone.

@swift-tk, I was under the impression that a zero value should be given if not defined, explicitly. Thanks for your support.

@mmahadevan108
Copy link
Contributor

@ioannis-karachalios , we are only merging PR that have a Zephyr Issue linked to it. If this is needed for the 4.0 release, then please add a Zephyr issue with details about the problem seen and link it to this PR.

@ioannis-karachalios
Copy link
Contributor Author

ioannis-karachalios commented Nov 8, 2024

@mmahadevan108, I opened #81151 bug report incident linked to this PR.

@mmahadevan108 mmahadevan108 added this to the v4.0.0 milestone Nov 8, 2024
@mmahadevan108 mmahadevan108 merged commit 7c5c459 into zephyrproject-rtos:main Nov 8, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: MEMC area: MSPI bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple APS6404 MSPI Device Issues

6 participants