Skip to content
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

Update SDK to 2.14 #270

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

mmahadevan108
Copy link
Collaborator

@mmahadevan108
Copy link
Collaborator Author

cc @iuliana-prodan @dbaluta

@manuargue
Copy link
Member

Changes LGTM, we will run validation with this branch for the S32 devices to see if there's any issue.

@JiafeiPan
Copy link
Contributor

Hi, @mmahadevan108, may I know whether this PR covers CMSIS update from https://github.com/nxp-mcuxpresso/CMSIS_5/tree/mcux_develop? thanks.

@dbaluta
Copy link
Collaborator

dbaluta commented Sep 7, 2023

@mmahadevan108 Can you please also integrate this PR: nxp-mcuxpresso/mcux-sdk#135 into this SDK update?

Later edit:

I noticed that the changes are merged with SDK update (so big thanks!!)

image

We need to open a discussion (maybe in another thread) about the process of updating the SDK. Ideally the changes should be cherry-picked preserving the commit message that we have in the original mcu-sdk repo.

I assume that this is not possible for some reason. Then at the least the commit subjects should be added in the commit updating the SDK.

Something like this:

 mcux-sdk: Update to SDK 2.14

This pulls in following commits from mcu-sdk:
34bbf137  fsl_common: Add support for xt-clang compiler
6868378c Incorrect Fifo status on Message Lost

@@ -222,6 +222,9 @@ enum
/*! @brief Type used for all status and error return values. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally there should be no difference between SDK repo and this repo modulo some specific files that are only used in hal_nxp.

Not sure why this can't be merged to SDK repo? It shouldn't bother anyone but I think you know better so I don't have anything against this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will check with the SDK team if this can be merged.

@mmahadevan108
Copy link
Collaborator Author

@mmahadevan108 Can you please also integrate this PR: nxp-mcuxpresso/mcux-sdk#135 into this SDK update?

Later edit:

I noticed that the changes are merged with SDK update (so big thanks!!)

image

We need to open a discussion (maybe in another thread) about the process of updating the SDK. Ideally the changes should be cherry-picked preserving the commit message that we have in the original mcu-sdk repo.

I assume that this is not possible for some reason. Then at the least the commit subjects should be added in the commit updating the SDK.

Something like this:

 mcux-sdk: Update to SDK 2.14

This pulls in following commits from mcu-sdk:
34bbf137  fsl_common: Add support for xt-clang compiler
6868378c Incorrect Fifo status on Message Los

@mmahadevan108 Can you please also integrate this PR: nxp-mcuxpresso/mcux-sdk#135 into this SDK update?

Later edit:

I noticed that the changes are merged with SDK update (so big thanks!!)

image

We need to open a discussion (maybe in another thread) about the process of updating the SDK. Ideally the changes should be cherry-picked preserving the commit message that we have in the original mcu-sdk repo.

I assume that this is not possible for some reason. Then at the least the commit subjects should be added in the commit updating the SDK.

Something like this:

 mcux-sdk: Update to SDK 2.14

This pulls in following commits from mcu-sdk:
34bbf137  fsl_common: Add support for xt-clang compiler
6868378c Incorrect Fifo status on Message Lost

Ideally you should have seen it when you did a git log inside the mcux-sdk folder. However I did a manual merge of a few commits as I was about to push this PR when I got information about this change, hence you do not see it.

@mmahadevan108
Copy link
Collaborator Author

@hakehuang, can you run a full test cycle on the Zephyr PR to update to SDK 2.14

@mmahadevan108
Copy link
Collaborator Author

Hi, @mmahadevan108, may I know whether this PR covers CMSIS update from https://github.com/nxp-mcuxpresso/CMSIS_5/tree/mcux_develop? thanks.

No, I have not updated the CMSIS package. Can you help update the CMSIS pack we maintain for the A series

@JiafeiPan
Copy link
Contributor

Hi, @mmahadevan108, may I know whether this PR covers CMSIS update from https://github.com/nxp-mcuxpresso/CMSIS_5/tree/mcux_develop? thanks.

No, I have not updated the CMSIS package. Can you help update the CMSIS pack we maintain for the A series

Sure, will create PR for that.

@manuargue
Copy link
Member

Changes LGTM, we will run validation with this branch for the S32 devices to see if there's any issue.

we have some SPI tests failing with this branch that we need to further investigate, will feedback soon.

@hakehuang
Copy link
Collaborator

@hakehuang, can you run a full test cycle on the Zephyr PR to update to SDK 2.14

there are many build fails, need fix, I will forward you details

@JiafeiPan
Copy link
Contributor

Hi, @mmahadevan108, may I know whether this PR covers CMSIS update from https://github.com/nxp-mcuxpresso/CMSIS_5/tree/mcux_develop? thanks.

No, I have not updated the CMSIS package. Can you help update the CMSIS pack we maintain for the A series

Done with PR: #272 please review, thanks.

@Dat-NguyenDuy
Copy link
Collaborator

Hi @mmahadevan108, could you please also integrate this PR: nxp-mcuxpresso/mcux-sdk#132 into the update, thanks.

@mmahadevan108
Copy link
Collaborator Author

Hi @mmahadevan108, could you please also integrate this PR: nxp-mcuxpresso/mcux-sdk#132 into the update, thanks.

Done

@mmahadevan108
Copy link
Collaborator Author

@manuargue Kindly send me a patch for the SPI issue.

@mmahadevan108
Copy link
Collaborator Author

cc @LaurentiuM1234

@mmahadevan108 mmahadevan108 force-pushed the Update_SDK_2_14 branch 2 times, most recently from 37585ee to ec0347c Compare September 15, 2023 11:37
@hakehuang
Copy link
Collaborator

@mmahadevan108 all tset done. now all issues fixed

Copy link
Member

@manuargue manuargue left a comment

Choose a reason for hiding this comment

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

@manuargue Kindly send me a patch for the SPI issue.

@mmahadevan108 I've sent you the patch from MCUX SDK team. Let me know if further support is needed. I'll remove the -1 once the patch is pushed.

Update to SDK 2.14

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
This will not get merged into the SDK repo and will have to
be maintained here.

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
Move used job descriptors in the CAAM driver from the stack to
noncacheable section.
This change has not been accepted by the SDK team and needs to
be maintained here.

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
Delete patches that have been added to SDK 2.14 release

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
The flexcomm and flexio driver structure has changed to
include sub folder for the individual component drivers.

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
@mmahadevan108
Copy link
Collaborator Author

@manuargue the patch has been added to this PR. Kindly check.

Copy link
Member

@manuargue manuargue left a comment

Choose a reason for hiding this comment

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

All my comments addressed

@mmahadevan108 mmahadevan108 merged commit a87e6a8 into zephyrproject-rtos:master Sep 19, 2023
@mmahadevan108 mmahadevan108 deleted the Update_SDK_2_14 branch September 20, 2023 15:41
@marc-hb
Copy link

marc-hb commented Dec 15, 2023

Congratulations, you broke west :-)

@butok
Copy link

butok commented Dec 15, 2023

Congratulations, you broke west :-)

Could you be more specific what & how is broken,?

@marc-hb
Copy link

marc-hb commented Dec 16, 2023

Did you open the link you quoted?

@hakehuang
Copy link
Collaborator

Could you be more specific what & how is broken,?

the original code is as below °C is the problem one. @SusanSu I think we need enforce some encode check for source code.

#define FXPQ3115_TEMPERATURE_CONV_FACTOR (256) /* Will give °C */

@mcuxsusan
Copy link

I see the linked improvement with west tool at zephyrproject-rtos/west#700, does this issue finally relate to the improvement to west tool? What I mean is that our code comment which has encode issue hit the hole in the west tool.

@hakehuang
Copy link
Collaborator

I see the linked improvement with west tool at zephyrproject-rtos/west#700, does this issue finally relate to the improvement to west tool? What I mean is that our code comment which has encode issue hit the hole in the west tool.

per my understand, the fix goes to tool is better. In the meantime, we need somehow standard our hal with charset encodecing.

@marc-hb
Copy link

marc-hb commented Dec 30, 2023

BTW @dbaluta does it really make sense to dump an entire SDK in a "HAL" module? hal_nxp is currently the largest zephyr/modules/ by far (git checkout = 1.2G) and this almost entirely caused by this sort of SDK dump.

There is a constant stream on Discord and Github of users complaining that west update is too big and too slow and there are regular optimization efforts to solve this problem. Such efforts are easily destroyed by PRs like this one.

See

+ a lot more from that link.

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Dec 30, 2023

Oh wow, yeah at a quick glance there's a lot that does not really belong to here, besides 3000+ png images, 2000+ html files and 13 pdf files there's also a 7MB sqlite database and 3 ELF binaries, which I imagine are not used but should probably not be redistributed under the org.

@nashif @carlescufi

We should probably think about making the west diff a bit smarter and detect binary files or unreasonably big deltas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants