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_hic: clock enhancements #925

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Conversation

groleo
Copy link

@groleo groleo commented Jan 7, 2022

  • Remove lpc43xx_cgu

@mathias-arm mathias-arm changed the title Remove lpc43xx_cgu lpc4322_hic: clock enhancements Jan 7, 2022
@mathias-arm
Copy link
Collaborator

Can you fold #924 in this PR?

@groleo
Copy link
Author

groleo commented Jan 7, 2022

Can you fold #924 in this PR?

done.

@groleo
Copy link
Author

groleo commented Jan 8, 2022

i'll upload a new version. the new code from board should sit in system (already defines the xtal freq and has a waitUS function too)

@groleo
Copy link
Author

groleo commented Jan 8, 2022

it looks to me that system_LPC43xx.c::SetClock() does the same thing as board_LPC43xx.c::sdk_init().
am I missing some init order or some other bits?

@mbrossard
Copy link
Contributor

I have always felt lpc43xx_cgu.c was duplicating things already done by system_LPC43xx.c, but when I tried to remove the former, I ended up breaking UART functionality.

I have tested this branch with gcc_arm and it was working. I was hopeful that it would fix armclang but there was a regression compared to v0257-beta2. It's a strange thing that happens with max32625 HIC as well: flashing will somehow corrupt bootloader... 🤦

@groleo
Copy link
Author

groleo commented Jan 8, 2022

From what I see, PLL1 MSEL (multiplier) is different between system_LPC43xx.c (19) and lpc43xx_cgu.c(9);
system_LPC43xx.c is setting PLL1 to 20x12MHz and lpc43xx_cgu.c is setting it to 10x12Mhz.

I'll modify the system_LPC43xx.c's MSEL to match what I'm setting in lpc43xx_cgu.c and see how it goes

@mbrossard
Copy link
Contributor

The comments in system_LPC43xx.c say PLL1 is 120 Mhz.
If you want to try getting the CPU and PLL1 up to 180 MHz you can revert [ea6013d].
It seems the CMSIS UART driver (source/hic_hal/nxp/lpc4322/RTE_Driver/USART_LPC43xx.c) is connecting the BASE_UARTx_CLK to PLL1 and uses GetClockFreq() in system_LPC43xx.c in its computations for baud rate.

Hopefully soon, I would like move from the current uart.c to using the CMSIS drivers for USART0.

@groleo
Copy link
Author

groleo commented Jan 8, 2022

The comments in system_LPC43xx.c say PLL1 is 120 Mhz.
i've tried to have PLL1 at 120MHz, but I hit the error #error "PLL1 Fcco frequency out of range! (156MHz >= Fcco <= 320MHz)".
I checked the manual and for PLL1 says: 156 MHz to 320 MHz Current Controlled Oscillator (CCO) frequency.

Another difference I've noticed is that the previous defines in system_LPC43xx.c were not disabling the post-divider of PLL1.

If you want to try getting the CPU and PLL1 up to 180 MHz you can revert [ea6013d]. It seems the CMSIS UART driver (source/hic_hal/nxp/lpc4322/RTE_Driver/USART_LPC43xx.c) is connecting the BASE_UARTx_CLK to PLL1 and uses GetClockFreq() in system_LPC43xx.c in its computations for baud rate.

There is something fishy with the SystemCoreClock.
Right now, with CPU at 180MHz and the SystemCoreClock variable set at 120000000, I get consistently 350 kB/sec transfer rate (no other patches).
If I set SystemCoreClock to 180000000, the transfer rate starts varying between 190 kB/sec to 240 kB/sec.
I reckon there are some protocol errors showing up.

@mbrossard
Copy link
Contributor

Ok this is strange, this issue in GitHub shows 2 commits (3bad93a and 76f20ba), but if I go to the branch it shows one commit (b8ec269).

The latter has changes to system_LPC43xx.c that are too invasive for my taste.

I used your commit 3bad93a, in the mbrossard/feature/lpc4322-cmsis-uart branch. Using the standard 180 MHz version of system_LPC43xx.c seems to work with CMSIS-DAP drivers with two caveats:

  • There are issues with the CMSIS UART drivers adapter, with an optimization that seems to fail an assertion under stress.
  • The lpc4322_bl seems to fail if clocked at 180 Mhz so I have re-used your code to clock it down to 96 MHz (120 MHz could be an option). I am considering using the two distinct versions of system_LPC43xx.c, 120 MHz and 180 MHz, for bootloader and interface respectively).

If we want to increase to 180 Mhz, I would rather we use system_LPC43xx.c as a basis, using its set-up (PLL0USB, PLL1, BASE_M4_CLK, FLASHCFG_FLASHTIM, etc.). I would prefer to see additional clock configuration done in sdk_init() (but it seems currently gpio.c, uart.c, and elsewhere swdp-sgpio.c are all doing their clock initialization for their thing), unless there is something that is very wrong with system_LPC43xx.c. RTE_Driver/USART_LPC43xx.c seems to work out of GetClockFreq() in system_LPC43xx.c. I know it sounds a bit constraining, but I am concerned about the maintenance costs and compiler compatibility.

@mbrossard mbrossard requested a review from flit January 9, 2022 16:56
@groleo
Copy link
Author

groleo commented Jan 9, 2022

If we want to increase to 180 Mhz, I would rather we use system_LPC43xx.c as a basis
using its set-up (PLL0USB, PLL1, BASE_M4_CLK, FLASHCFG_FLASHTIM, etc.).
That's how already done in this change: the clock setup done in board is dropped,
the mdec, pdec, ndec, etc are extracted as functions,
where possible, magic numbers are replaced with defines,
direct PLL mode is used instead.
I can split it into smaller chunks to make it easier to digest.
One other thing is that if CPU clock is done in system*, then OS_CLOCK from yaml
is half-right, since the true freq is given by the msel defines found in system file.

I would prefer to see additional clock configuration done in sdk_init() (but it seems currently gpio.c, uart.c,
and elsewhere swdp-sgpio.c are all doing their clock initialization for their thing)
I don't know which component clock setup is done when.
I tried to replace SysteCoreClock with a function that get the clock from the reg and the system behaved
differently.
Maybe a per-component {component}_clock_init() would clarify things a bit; these would be called by sdk_init().

@groleo groleo force-pushed the rm-cgu branch 2 times, most recently from f8c1edf to c8f3e9e Compare January 9, 2022 19:21
@groleo
Copy link
Author

groleo commented Feb 15, 2022

I've dropped the commit in which PLL1 was put into direct mode and the CPUFREQ was set to 180 MHz, so
no functionality is changed now.

Added new change to reuse lpc55xx/uart.c; so far it's the same file but can be shared

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.

3 participants