lpc4322_hic: clock enhancements

[groleo]

• Remove lpc43xx_cgu

[mathias-arm]

Can you fold #924 in this PR?

[groleo]

Can you fold #924 in this PR?

done.

[groleo]

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]

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]

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]

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]

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]

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]

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.

[groleo]

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]

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