Skip to content

Conversation

@zyp
Copy link
Contributor

@zyp zyp commented Nov 28, 2022

This is a draft of initial support for nRF9160.

Detailed description

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Closing issues

@dragonmux dragonmux added this to the v1.10 milestone Nov 28, 2022
@dragonmux dragonmux added the New Target New debug target label Nov 28, 2022
@mfiumara
Copy link

Hi @zyp, I've been looking for nrf91 support on the bmp for some time and it'd be great if this could be merged. Do you need help in testing it? I have a nrf91 thingy I could test this on.

@zyp
Copy link
Contributor Author

zyp commented Aug 26, 2023

This was originally blocked on a proper fix for handling the HNONSEC bit in CSW, which got merged in a82624c, so I think if I take out the workaround and rebase it to current main, it is mergeable.

@dragonmux Are there any other changes you'd like to see while I'm at it?

@mfiumara
Copy link

mfiumara commented Aug 26, 2023

For me this does not seem to build @zyp, maybe I'm missing something:

 make PROBE_HOST=native
  BUILD   lib/stm32/f1
make[2]: Nothing to be done for `all'.
  BUILD   lib/stm32/f4
make[2]: Nothing to be done for `all'.
  BUILD   lib/lm4f
make[2]: Nothing to be done for `all'.
  LD      blackmagic.elf
/Applications/ArmGNUToolchain/12.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/12.3.1/../../../../arm-none-eabi/bin/ld: blackmagic.elf section `.data' will not fit in region `rom'
/Applications/ArmGNUToolchain/12.3.rel1/arm-none-eabi/bin/../lib/gcc/arm-none-eabi/12.3.1/../../../../arm-none-eabi/bin/ld: region `rom' overflowed by 392 bytes
Memory region         Used Size  Region Size  %age Used
             rom:      131464 B       128 KB    100.30%
             ram:        3756 B        20 KB     18.34%
collect2: error: ld returned 1 exit status
make[1]: *** [blackmagic.elf] Error 1
make: *** [all] Error 2

After rebasing on main there seem to be some minor API changes that need to be updated:

target/nrf91.c: In function 'nrf91_probe':
target/nrf91.c:87:35: error: implicit declaration of function 'cortexm_ap'; did you mean 'cortex_ap'? [-Werror=implicit-function-declaration]
   87 |         adiv5_access_port_s *ap = cortexm_ap(t);
      |                                   ^~~~~~~~~~
      |                                   cortex_ap
target/nrf91.c:87:35: error: initialization of 'adiv5_access_port_s *' {aka 'struct adiv5_access_port *'} from 'int' makes pointer from integer without a cast [-Werror=int-conversio]
cc1: all warnings being treated as errors
make[1]: *** [nrf91.o] Error 1
make: *** [all] Error 2

@zyp zyp force-pushed the add_nrf91 branch 3 times, most recently from 2fe391b to ed71423 Compare August 26, 2023 13:34
@zyp zyp changed the title Draft of initial nRF91 support. Add initial nRF91 support. Aug 26, 2023
@zyp zyp marked this pull request as ready for review August 26, 2023 13:42
@zyp
Copy link
Contributor Author

zyp commented Aug 26, 2023

Removed workaround, rebased to current main, fixed the minor API thing and the formatting the linter complained about.

Have only tested that scanning still works with BMDA, so it could do with some more testing, but apart from that I think this is ready to be merged.

@mfiumara
Copy link

Removed workaround, rebased to current main, fixed the minor API thing and the formatting the linter complained about.

Have only tested that scanning still works with BMDA, so it could do with some more testing, but apart from that I think this is ready to be merged.

I can test this on my bmp v2 later today with an nrf91 thingy.

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

This all looks good save for a couple of naming items and some errant braces. With them fixed, we'll be happy to merge 👍🏼

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

LGTM, we'll merge this once the builds complete. If there's something broken, a new PR with fixes can always be opened.

@dragonmux dragonmux merged commit c7c209a into blackmagic-debug:main Aug 26, 2023
@zyp zyp deleted the add_nrf91 branch August 26, 2023 15:22
@dragonmux
Copy link
Member

@zyp just noticed this was taken without ever getting a license notice added to the top of file - what license was this intended to be contributed under? We'll add it in a branch we're currently building as a fixup to the file.

@zyp
Copy link
Contributor Author

zyp commented Sep 8, 2025

Anything is okay for me, I'm happy for all contributions I do to be considered non-copyleft (MIT and/or BSD). However, the code is partially based on nrf51.c which does have a GPL header, so if you need a single license for the whole file, GPL restrictions would have to apply.

This goes for #2009 as well.

@dragonmux
Copy link
Member

The code in the file is so different to the nRF51/2 code that even if that's where you started, we can't see a similarity now so we'll apply BSD-3-Clause for you. Many thanks for the response!

@dragonmux
Copy link
Member

The other question for you is.. attribution: Are you handing 1BitSquared the copyright for the code, or are you wishing to keep that? We will be attributing the file as written by you, however need to know what copyright line(s) to add.

@zyp
Copy link
Contributor Author

zyp commented Sep 8, 2025

As before, either is fine with me. If there is a desire to consolidate the copyright for new contributions then please do so. Maybe it would be a good idea to have a CLA in place?

@dragonmux
Copy link
Member

Right now the policy is "if the contributor hands us copyright, that's appreciated; if not then no worriess, no problem there" - something to ask Esden about..

We'll assign you copyright then for the original code, and copyright 1b² for our modifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Target New debug target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants