nxp: imx8ulp: add NXP HAL binding support #70219
Conversation
|
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
added
manifest
manifest-hal_nxp
DNM
LaurentiuM1234
force-pushed
the
ulp_adsp_patches
branch
from
203901c
to
fc8aea5
Compare
zephyrbot
requested review from
danieldegrasse,
dbaluta,
decsny,
dleach02,
iuliana-prodan and
yvanderv
LaurentiuM1234
force-pushed
the
ulp_adsp_patches
branch
from
fc8aea5
to
fe23d1d
Compare
LaurentiuM1234
force-pushed
the
ulp_adsp_patches
branch
from
fe23d1d
to
e02e659
Compare
soc/nxp/imx/imx8ulp/Kconfig.soc
Outdated
| @@ -13,7 +13,7 @@ config SOC_MIMX8ULP | |||
| select SOC_SERIES_IMX8ULP | |||
|
|
|||
| config SOC | |||
| default "imx8ulp" if SOC_MIMX8ULP | |||
| default "mimx8ud7" if SOC_MIMX8ULP | |||
There was a problem hiding this comment.
| default "mimx8ud7" if SOC_MIMX8ULP | |
| default "mimx8ud7" if SOC_MIMX8UD7 |
We need to rename SOC_MIMX8ULP to SOC_MIMX8UD7 elsewhere as well
There was a problem hiding this comment.
@danieldegrasse Is this mandatory? I believe not.
We can name the CONFIG_SOC_XYZ as suggestive as possible, and SOC_MIMX8ULP is more appropriate then SOC_MIMX8UD7, IMO. (8ULP is the board name).
I agree we have to have the right value set for it - in this case mimx8ud7 (set correctly here), to link it with our NXP_HAL.
Is this CONFIG_SOC_XYZ somewhere parsed and the "XYZ" is used?
IIUC only the value of CONFIG_SOC_XYZ is used.
PS: We did the same for 8MPlus - https://github.com/zephyrproject-rtos/zephyr/blob/main/soc/nxp/imx/imx8m/Kconfig.soc#L27 or others (8QXP or 8QM).
There was a problem hiding this comment.
I'm going off (my understanding) of the guidance for HWMv2, that the CONFIG_SOC_xxx suffix needs to match the SOC name in the YAML- @nordicjm, is this correct?
There was a problem hiding this comment.
My 2 cents here: switching to SOC-based (e.g: for ULP use MIMX8UD7 and so on) naming scheme should be addressed in another patch series (if required, of course). This is because:
- We have multiple SoCs that don't abide by this rule (there's also MP, QM, and QXP as @iuliana-prodan pointed out). Doing this in a separate patch with its only scope being fixing such irregularities should be more "clean" and easy to follow.
- Doing this change also implies making even more changes on SOF side, which is not ideal. We should strive to make such changes incrementally to minimize the chance of breaking stuff. We're also in the process of transitioning to Zephyr drivers with a couple of patches that are still in review due to some Zephyr patches still being in review and having to also take the renaming into account would just make the process more prone to errors.
- The scope of this PR is to just add the HAL binding. Having to modify the SOC name is just a side effect of this, not the main goal. The old
imx8ulpname has just been a "placeholder" since we didn't need the HAL binding up until now so this was long overdue.
There was a problem hiding this comment.
Keys and values for SOC, SOC_SERIES and SOC_FAMILY need to match (SOC can additionally contain the CPU cluster). Whilst this is not enforced by CI at the present time, it needs to be followed as in future lots of these Kconfig options will be generated automatically by the build system and at that point mismatches will suddenly cause entire SoCs to just break.
There was a problem hiding this comment.
@nordicjm @danieldegrasse can we leave this as is now (as @LaurentiuM1234 suggested here) and I'll follow up with a PR next week?
@nordicjm I want to understand what does it have to match exactly?
If my SOC value is mimx8ml8, the key for SOC_IMX8MP should be SOC_MIMX8ML8?
Why is this mandatory? Since the SOC_IMX8MP is just a bool - is defined or not.
Also, what should match in the SOC_SERIES or SOC_FAMILY?
What is wrong here, for example?
config SOC_SERIES
default "imx8m" if SOC_SERIES_IMX8M
or
config SOC_FAMILY
default "nxp_imx" if SOC_FAMILY_NXP_IMX
Thanks!
There was a problem hiding this comment.
No it needs to be updated here.
For:
config SOC_SERIES
default "imx8m" if SOC_SERIES_IMX8M
then the imx8m part must match in the format SOC_SERIES_<part>. For:
config SOC_FAMILY
default "nxp_imx" if SOC_FAMILY_NXP_IMX
then the nxp_imx part must match in the format SOC_FAMILY_<family>. For:
config SOC
default "mimx8ud7" if SOC_MIMX8ULP
then the mimx8ud7 part must match in the format SOC_FAMILY_<soc> or SOC_FAMILY_<soc>_<cpucluster>, so this is invalid and needs fixing. And all of these must match the text entries in the soc.yml file
soc/nxp/imx/imx8ulp/Kconfig.soc
Outdated
| @@ -13,7 +13,7 @@ config SOC_MIMX8ULP | |||
| select SOC_SERIES_IMX8ULP | |||
|
|
|||
| config SOC | |||
| default "imx8ulp" if SOC_MIMX8ULP | |||
| default "mimx8ud7" if SOC_MIMX8ULP | |||
There was a problem hiding this comment.
No it needs to be updated here.
For:
config SOC_SERIES
default "imx8m" if SOC_SERIES_IMX8M
then the imx8m part must match in the format SOC_SERIES_<part>. For:
config SOC_FAMILY
default "nxp_imx" if SOC_FAMILY_NXP_IMX
then the nxp_imx part must match in the format SOC_FAMILY_<family>. For:
config SOC
default "mimx8ud7" if SOC_MIMX8ULP
then the mimx8ud7 part must match in the format SOC_FAMILY_<soc> or SOC_FAMILY_<soc>_<cpucluster>, so this is invalid and needs fixing. And all of these must match the text entries in the soc.yml file
LaurentiuM1234
force-pushed
the
ulp_adsp_patches
branch
from
e02e659
to
36cc8e8
Compare
|
V2 changes
|
LaurentiuM1234
force-pushed
the
ulp_adsp_patches
branch
from
36cc8e8
to
ddc83f0
Compare
submanifests/optional.yaml
Outdated
There was a problem hiding this comment.
I think this manifest update needs to be in the same commit where you rename the SOC to preserve bisectability
There was a problem hiding this comment.
yep, you're right. Will update once zephyrproject-rtos/sof#44 is merged.
LaurentiuM1234
force-pushed
the
ulp_adsp_patches
branch
from
ddc83f0
to
5895fd0
Compare
|
Dependencies merged and CI is now green. @danieldegrasse @nordicjm can you please have another look? |
|
@LaurentiuM1234 needs to fix the conflicts and we are ready to merge. |
LaurentiuM1234
dismissed stale reviews from dbaluta, iuliana-prodan, nordicjm, and danieldegrasse
via
0989cfa
LaurentiuM1234
force-pushed
the
ulp_adsp_patches
branch
from
5895fd0
to
0989cfa
Compare
|
Removed NXP HAL hash bump. The changes should be pulled in by the hash bump performed in #70392. |
The SOC name `imx8ulp` has been just a placeholder until support for the SOC's ADSP (since this is the only core that's supported in Zephyr) could be added to the NXP HAL. Now that the support has been added, to make use of it, the SOC name `imx8ulp` has to be changed to `mimx8ud7`. As such, this commit does the following: 1) Introduces SOC part number configuration - needed by some HAL headers. 2) Replaces all occurrences of `imx8ulp` (as the SOC name) with `mimx8ud7`. 3) Enables `CONFIG_HAS_MCUX`. 4) Aligns all `CONFIG_SOC_` configurations with the new SOC name. 5) Updates SOF hash. This is needed to fix build issues caused by this name change. This is not done in a separate commit to preserve bisectability. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
The SOC name for the `imx8ulp_evk` board has been changed to `mimx8ud7`. As such, update `deprecated.cmake` to reflect this change. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
No description provided.