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

lpc4322 cleanups for GCC #813

Closed
wants to merge 16 commits into from
Closed

Conversation

groleo
Copy link

@groleo groleo commented May 21, 2021

No description provided.

Adrian Negreanu added 16 commits May 21, 2021 09:12
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
It provides GetClockFreq() which is needed by USART_LPC43xx.
this provides definitions needed by GetClockFreq()
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
This way, one can set SWO_UART from the target's yaml.

Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
The stack and heap have to reside in m_data.

The largest symbols (.bss.TraceBuf, .bss.sector_buf, .bss.DAP_Cmd_queue)
are explicitly placed in m_data_2.

Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
Signed-off-by: Adrian Negreanu <adrian.negreanu@nxp.com>
@groleo
Copy link
Author

groleo commented May 21, 2021

@mathias-arm

@mathias-arm
Copy link
Collaborator

I have tentatively re-worked this PR in mathias-arm:feature/lpc4322_enhancements:

  • Your updated system_LPC43xx.c already has a lot of clock setup code. There was a version in the latest CMSIS pack pre-configured for 120 MHz. We should be able to remove lpc43xx_cgu.c and lpc43xx_cgu.h once the last use in uart.c is removed.
  • With GCC, VTOR is already set in gcc/startup_LPC43xx.S, so I replaced [93e724a] with a different fix.
  • I made a few changes to be more consistent with the lpc5xx HIC: copying the CMSIS headers, moving RTE_Device.h up to the HIC base directory, other fixes in the lpc4322.yaml.

I have done some light testing with lpc4322_lpc55s69xpresso_if.

I do have a few additional questions which might be addressed at a later date:

  • Core clock: the standard system_LPC43xx.c uses 180 Mhz, and you have just bumped from 96 Mhz to 120 Mhz, is there any specific reason you are aware of not to use 180 MHz (160 MHz is also documented).
  • The lpc55xx HIC has bumped SWO_BUFFER_SIZE to 8 KB, and with your m_data_2 change it is possible to do the same here (or even 16 KB).
  • You have set SWO_STREAM to be configurable, but it seems to be lacking some additional changes.

@mathias-arm mathias-arm requested a review from flit May 22, 2021 21:03
@mathias-arm
Copy link
Collaborator

mathias-arm commented May 22, 2021

The removal of lpc43xx_cgu.c and lpc43xx_cgu.h has been attempted in f384a76.

@mathias-arm
Copy link
Collaborator

@groleo: If you are okay with my changes can you force-push them to this PR?

@groleo
Copy link
Author

groleo commented May 27, 2021

I have tentatively re-worked this PR in mathias-arm:feature/lpc4322_enhancements:

* Core clock: the standard `system_LPC43xx.c` uses 180 Mhz, and you have just bumped from 96 Mhz to 120 Mhz, is there any specific reason you are aware of not to use 180 MHz (160 MHz is also documented).

I don't see why not. I'll try to test 180 MHz sometime this week.

* The lpc55xx HIC has bumped `SWO_BUFFER_SIZE` to 8 KB, and with your `m_data_2` change it is possible to do the same here (or even 16 KB).

On LPC4322 the bottleneck is not the buffer-size, but the bit-banging SWD implementation.
LPC55xx has a DMA based SWD (if I remember correctly);
My guess is that increasing the buffer size won't bring benefits.
I'm working on a SGPIO implementation of SWD; once that is done, the TraceBuf size may be increased.

* You have set `SWO_STREAM` to be configurable, but it seems to be lacking some additional changes.

additional changes are in the rt1170 patches.

@mathias-arm
Copy link
Collaborator

I have tentatively re-worked this PR in mathias-arm:feature/lpc4322_enhancements:

  • Core clock: the standard system_LPC43xx.c uses 180 Mhz, and you have just bumped from 96 Mhz to 120 Mhz, is there any specific reason you are aware of not to use 180 MHz (160 MHz is also documented).

I don't see why not. I'll try to test 180 MHz sometime this week.

My question came out wrong. There might be good reasons to use a lower frequency (lower power, no performance, increased stability, etc.), I was wondering if you had some insight on the trade-offs for the LPC4322.

  • You have set SWO_STREAM to be configurable, but it seems to be lacking some additional changes.

additional changes are in the rt1170 patches.

This define comes from CMSIS-DAP, but DAPLink is missing some of bits that are needed to make it work (e.g. start SWO_Thread, add the extra USB Endpoint, implement the function to send the data). I did not see those in your previous PR.

@groleo
Copy link
Author

groleo commented May 28, 2021

This define comes from CMSIS-DAP, but DAPLink is missing some of bits that are needed to make it work (e.g. start SWO_Thread, add the extra USB Endpoint, implement the function to send the data). I did not see those in your previous PR.
Ah, yes, I remember now (in fact, defining SWO_STREAM breaks the build).
That's a direction I'm investigating to see if there are any speed benefits.
I haven't seen any hic to use it

@mathias-arm
Copy link
Collaborator

I have tentatively re-worked this PR in mathias-arm:feature/lpc4322_enhancements

I'll try to test 180 MHz sometime this week.

If you can validate my branch and push it to this PR (with possible amendments) I will merge it.

  • The lpc55xx HIC has bumped SWO_BUFFER_SIZE to 8 KB, and with your m_data_2 change it is possible to do the same here (or even 16 KB).

On LPC4322 the bottleneck is not the buffer-size, but the bit-banging SWD implementation.

There might be a better use for that RAM then. In [23fed18] I needed to run a few functions from RAM, and it is quite easy to do. We should probably have a macro definition and conditionally put it on the most performance sensitive functions.

LPC55xx has a DMA based SWD (if I remember correctly);

@flit has taken a stab at it but it does not work yet.

I'm working on a SGPIO implementation of SWD; once that is done, the TraceBuf size may be increased.

Nice.

additional changes are in the rt1170 patches.

You should create a separate PR for the MIMXRT1170-EVK to replace #712.

@mathias-arm
Copy link
Collaborator

The removal of lpc43xx_cgu.c and lpc43xx_cgu.h has been attempted in ...

I pushed this change in a branch instead: mathias-arm:feature/lpc4322_cgu

I rebased both branches.

@mathias-arm
Copy link
Collaborator

@groleo: I rebased mathias-arm:feature/lpc4322_enhancements on develop and added a fix to re-add SystemReset() otherwise the interface did not support restarting to bootloader. If you can validate this branch and push it to this PR, I will merge it. I did some tests with lpc4322_lpc55s69xpresso_if using in mbrossard:fix/run-tests to run some quick tests.

Currently the additional commit in mathias-arm:feature/lpc4322_cgu breaks CDC functionality. So we can postpone this.

@mathias-arm
Copy link
Collaborator

Superseded by #904

@mathias-arm mathias-arm closed this Dec 6, 2021
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.

2 participants