Skip to content

Conversation

@karstenkoenig
Copy link
Contributor

For nrf54h20 a range of combinations exist to configure the test and debug domains data sources and sinks. Expose them in DTS to allow configuring them. Also drop the previous style which was too rigid to extend to cover all cases cleanly. The old style was only used in a single sample application so far.

Signed-off-by: Karsten Koenig [email protected]

Upstream PR: zephyrproject-rtos/zephyr#78831

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

a42e290 is not the same as 8fdcf132716136935c990c3496e232a9bec8ce20, a fromlist or fromtree must cleanly apply without any modifications

@karstenkoenig
Copy link
Contributor Author

karstenkoenig commented Sep 24, 2024

Hi @anangl, you are listed as codeowner. There is a commit upstream that can't apply cleanly downstream because the file diverged downstream. I tried aligning up- and downstream when resolving the conflict in the cherry-pick, so afterwards they will be both aligned.

upstream commit: zephyrproject-rtos/zephyr@8fdcf13
downstream commit: a42e290

The downstream solution of using EXACT version for nrf-regtool seems to be the correct choice, the previous commit to upstream even says 'enforce exact version match'
zephyrproject-rtos/zephyr@9c23eff

Given you are the codeowner, can you help me figure out how to best proceed?

@anangl
Copy link
Contributor

anangl commented Sep 24, 2024

The downstream solution of using EXACT version for nrf-regtool seems to be the correct choice, the previous commit to upstream even says 'enforce exact version match' zephyrproject-rtos/zephyr@9c23eff

Not really. See zephyrproject-rtos/zephyr#69633 (comment). EXACT was intentionally not added. And I don't think we need it downstream (468d06d does not really explain why it was added). I think you should add a noup commit that removes EXACT downstream and modify the upstream commit to not add this modifier. Then it will apply cleanly.

@karstenkoenig
Copy link
Contributor Author

@anangl uh good catch, thanks for that. Given this cmake check fails upstream due to the outdated nrf-regtool version in the upstream docker image I'll just drop the version change upstream and only do the suggested noup here to drop the exact.
Then we can bump this cmake check in a separate PR after the upstream docker image has the new nrf-regtool version

@karstenkoenig karstenkoenig force-pushed the tddconf_binding_update_ncs branch from a42e290 to c68c0f7 Compare September 24, 2024 13:35
@karstenkoenig
Copy link
Contributor Author

@nordicjm Could you please have another look if this solution works for you?

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Build system change is OK

For nrf54h20 a range of combinations exist to configure the test and
debug domains data sources and sinks. Expose them in DTS to allow
configuring them. Also drop the previous style which was too rigid to
extend to cover all cases cleanly. The old style was only used in a
single sample application so far.

Signed-off-by: Karsten Koenig <[email protected]>
(cherry picked from commit 43f9488)
We don't want an exact version match, a minimum one is enough. This
aligns it also with the upstream copy of this file.

Signed-off-by: Karsten Koenig <[email protected]>
@karstenkoenig karstenkoenig force-pushed the tddconf_binding_update_ncs branch from c68c0f7 to 504140f Compare September 25, 2024 11:42
@karstenkoenig karstenkoenig changed the title [nrf fromlist] dts: bindings: arm: nordic: Add TDDCONF sources [nrf fromtree] dts: bindings: arm: nordic: Add TDDCONF sources Sep 25, 2024
@nordicjm nordicjm merged commit 3d01dcc into nrfconnect:main Sep 25, 2024
16 checks passed
@nordic-krch
Copy link
Contributor

Because we dropped EXACT STM logging does not work silently and user does not know that it because regtool update is needed. I think we should bring back EXACT.

@karstenkoenig
Copy link
Contributor Author

Is it not rather that 6.0.0 is needed now and not checked via CMake yet? So nrf-regtool 6.0.0 REQUIRED?

I tried kicking off the change upstream
zephyrproject-rtos/docker-image@eb419f1
zephyrproject-rtos/zephyr#79244

But am a bit lost with the upstream docker image, the zephyr builds requiring (at least) nrf-regtool 6.0.0 still fail with 6.0.0 not present in the image.

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.

7 participants