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

[RCFC] LPC4322 SWD-SGPIO. #907

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

Conversation

groleo
Copy link

@groleo groleo commented Dec 7, 2021

Requires #906.

This p-r is replacing the bit-banged SWD implementation with one which uses the lpc4322's SGPIO.
Transfer rate reported by IAR is around 380 kB/sec with DAP.nominal_clock @30mhz and SGPIO_CLK@120MHz.
Maximum I could get was 420 kB/sec with DAP.nominal_clock @40MHz and SGPIO_CLK @204MHz;

testing in progress.

@groleo
Copy link
Author

groleo commented Dec 7, 2021

v2:

  • fixed the lpc4322_bl target by defining SWDP_SGPIO per-board (records/board/mimxrt1170_evk_qspi.yaml)
  • use DAP_Data.nominal_clock instead of infering it from DAP_Data.clock_delay.

@groleo
Copy link
Author

groleo commented Dec 8, 2021

v3:

  • fix DAP_Info for debuggers not yet supporting cmsis-dap v2.1.

v2:

  • fixed the lpc4322_bl target by defining SWDP_SGPIO per-board (records/board/mimxrt1170_evk_qspi.yaml)
  • use DAP_Data.nominal_clock instead of infering it from DAP_Data.clock_delay.

// SHIFT_CLOCK = DAP_Data.nominal_clock
static uint32_t shift_clk_preset()
{
return SystemCoreClock/DAP_Data.nominal_clock - 1;
Copy link
Author

Choose a reason for hiding this comment

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

This should be using PLL1. This is how I've seen it used in the sgpio_uart NXP's Application Notes.
At the moment I'm trying to make CGU aware of CGU_PERIPHERAL_SGPIO, which is mentioned in the User Manual but
nowhere to be found in lpc43xx_cgu.c.

Copy link
Author

Choose a reason for hiding this comment

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

done. using PLL0AUDIO

////////////////////////////////////////
////////////////////////////////////////
// handle DAP_TRANSFER_FAULT
// check parity
Copy link
Author

Choose a reason for hiding this comment

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

Add parity check and return DAP_TRANSFER_ERROR on mismatch.

Copy link
Author

Choose a reason for hiding this comment

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

done.


// bits IN -> SLICE_H -> SLICE_P
static void sequence_read(uint32_t bits_sz, uint8_t *data)
{
Copy link
Author

Choose a reason for hiding this comment

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

Proper implementation is needed on this one.

Copy link
Author

Choose a reason for hiding this comment

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

done.

@mathias-arm
Copy link
Collaborator

@groleo: Can you put [8774a9e] in a separate PR?

@groleo
Copy link
Author

groleo commented Dec 9, 2021

@groleo: Can you put [8774a9e] in a separate PR?

Done

@groleo
Copy link
Author

groleo commented Jan 6, 2022

v4:
add sequence_read().
proper SGPIO_CLK initialization and setup.
add idle_cyles.
explicitly set the P1_17 mode so that it's visible it's a Hard-Drive pin (doesn't support slew-rate programming and it's driven @4ma)

Copy link
Contributor

@elfmimi elfmimi left a comment

Choose a reason for hiding this comment

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

Thank you @groleo this is awesome. I tried your code on LPC-Link2 (LPC4370) and it was fun to tinker around with SGPIO peripheral.
So far I've found 3 concerns. 2 of them are rather trivial. please see my suggested changes.

The real problem I'm experiencing is that I can't run it slowly, which is unexpected.
When SGPIO_CLK is at 120 MHz it runs no problem at SWD clock rate of 30 MHz. like you have explained. And it seems it's fine to lower the clock rate down to 3 MHz. But beyond that it starts to fail. Using non SGPIO version of DAPLink it can go just down to 1 kHz .

Comment on lines 561 to 585
SLICE_POS(SLICE_SWDIO_TXEN) = POS_POS(period) | POS_PRESET(0);
SLICE_REG(SLICE_SWDIO_TXEN) = 0b00000000000000000000000000000000;
SLICE_SHD(SLICE_SWDIO_TXEN) = 0;
SLICE_CNT(SLICE_SWDIO_TXEN) = 0;

SLICE_POS(SLICE_SWDIO_TMS_OE) = POS_POS(period) | POS_PRESET(0);
SLICE_REG(SLICE_SWDIO_TMS_OE) = 0b00000000000000000000000000000000;
SLICE_SHD(SLICE_SWDIO_TMS_OE) = 0;
SLICE_CNT(SLICE_SWDIO_TMS_OE) = 0;
SLICE_REG(SLICE_SWDIO_TMS_DOUT0) = 0;
SLICE_SHD(SLICE_SWDIO_TMS_DOUT0) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SLICE_POS(SLICE_SWDIO_TXEN) = POS_POS(period) | POS_PRESET(0);
SLICE_REG(SLICE_SWDIO_TXEN) = 0b00000000000000000000000000000000;
SLICE_SHD(SLICE_SWDIO_TXEN) = 0;
SLICE_CNT(SLICE_SWDIO_TXEN) = 0;
SLICE_POS(SLICE_SWDIO_TMS_OE) = POS_POS(period) | POS_PRESET(0);
SLICE_REG(SLICE_SWDIO_TMS_OE) = 0b00000000000000000000000000000000;
SLICE_SHD(SLICE_SWDIO_TMS_OE) = 0;
SLICE_CNT(SLICE_SWDIO_TMS_OE) = 0;
SLICE_REG(SLICE_SWDIO_TMS_DOUT0) = 0;
SLICE_SHD(SLICE_SWDIO_TMS_DOUT0) = 0;
// contents of registers are directly reflected to corresponding signals.
SLICE_REG(SLICE_SWDIO_TXEN) = 0b00000000000000000000000010000000;
SLICE_REG(SLICE_SWDIO_TMS_OE) = 0b00000000000000000000000000000001;
SLICE_REG(SLICE_SWDIO_TMS_DOUT0) = 0;

Comment on lines 1146 to 1150
if (info & SWD_SEQUENCE_DIN) {
sequence_read(bytes_nb*8, swdi);
} else {
sequence_send(bytes_nb*8, swdo);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (info & SWD_SEQUENCE_DIN) {
sequence_read(bytes_nb*8, swdi);
} else {
sequence_send(bytes_nb*8, swdo);
}
if (info & SWD_SEQUENCE_DIN) {
sequence_read(bytes_nb, swdi);
} else {
sequence_send(bytes_nb, swdo);
}

Copy link
Author

Choose a reason for hiding this comment

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

sequence_send() takes a bits_sz (bits size) as first parameter. I can replace the 8 to BITS_IN_CHAR to clarify the intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

info & SWD_SEQUENCE_CLK specifies clock cycles , so it is bits , not bytes . I could have renamed bytes_nb to bits_nb . anyway naming is up to you.

Copy link
Author

Choose a reason for hiding this comment

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

Now i get what you mean (thanks for taking a good look at this).
I'll change bytes_nb to bits_nb.

SLICE_SHD(SLICE_SWDIO_TXEN) = 0;

uint32_t bits_per_chunk = bits_sz<=64 ? bits_sz : 64;
SLICE_POS(SLICE_SWDIO_TMS_DIN1) = POS_POS(bits_per_chunk) | POS_PRESET(bits_per_chunk);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SLICE_POS(SLICE_SWDIO_TMS_DIN1) = POS_POS(bits_per_chunk) | POS_PRESET(bits_per_chunk);


if (bits_sz <= BYTES_PER_SLICE*8)
{
slices_used = SLICE_SWDIO_TMS_DIN0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
slices_used = SLICE_SWDIO_TMS_DIN0;
slices_used = SLICE_SWDIO_TMS_DIN0_MASK;

}
else
{
slices_used = SLICE_SWDIO_TMS_DIN0 | SLICE_SWDIO_TMS_DIN1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
slices_used = SLICE_SWDIO_TMS_DIN0 | SLICE_SWDIO_TMS_DIN1;
slices_used = SLICE_SWDIO_TMS_DIN0_MASK | SLICE_SWDIO_TMS_DIN1_MASK;

uint32_t bits_remaining = bits_sz - bits_it;
uint32_t bytes_remaining = (bits_remaining + 7) / 8;
uint32_t slice_it = 0;
uint32_t slices_used = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint32_t slices_used = 0;

// Disable TXEN.
SLICE_POS(SLICE_SWDIO_TXEN) = POS_POS(0) | POS_PRESET(0);
SLICE_CNT(SLICE_SWDIO_TXEN) = 0;
SLICE_REG(SLICE_SWDIO_TMS_OE) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SLICE_REG(SLICE_SWDIO_TMS_OE) = 0;
SLICE_REG(SLICE_SWDIO_TXEN) = 0;

@elfmimi
Copy link
Contributor

elfmimi commented Apr 9, 2022

I'm experimenting reconfiguration of PLL to match required SWD frequency. Based on your code. It can run at SWD clock speed ranging 50MHz down-to 1kHz .
https://github.com/elfmimi/DAPLink/tree/lpc4370_sgpio_pll_sequence_fix_if_wip

Signed-off-by: Adrian Negreanu <[email protected]>
@elfmimi
Copy link
Contributor

elfmimi commented Apr 24, 2022

Current status of this pull-request.

  • Build the interface firmware from pr's head commit 9f14137
    progen generate -p lpc4322_mimxrt1170_evk_qspi_if -t make_gcc_arm -b -o jobs=8
  • Install the interface firmware using the bootloader firmware lpc4322_bl
    Dev board I'm using has different target MCU. But it's OK for testing pyOCD access.
  • Results
    • pyOCD can connect to the onboard target LPC43S67 but only when frequency setting is high enough.
      python -mpyocd cmd -t cortex_m -f 3000000
    • pyOCD can connect to external targets. for example RaspberryPi Pico - RP2040 , but with the same frequency constraint.
      python -mpyocd cmd -t rp2040 -f 3000000
    • pyOCD can not connect to some external targets. for example nRF52840.

Copy link
Contributor

@elfmimi elfmimi left a comment

Choose a reason for hiding this comment

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

I wasn't sure until now if this change is needed or not.
Now I had a chance to test with and without this change, and I'm convinced that this change fixes stability when connecting with some target , ex nRF52840 .

Comment on lines +173 to +174
#define SGPIO_SWDIO_TXEN 15
#define SLICE_SWDIO_TXEN SLICE_M
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define SGPIO_SWDIO_TXEN 15
#define SLICE_SWDIO_TXEN SLICE_M
#define SGPIO_SWDIO_TXEN 15
#define SGPIO_SWDIO_TXEN_DOUTM8_BIT 7
#define SLICE_SWDIO_TXEN SLICE_M

uint32_t parity;

SLICE_POS(SLICE_SWDIO_TXEN) = POS_POS(8) | POS_PRESET(0);
SLICE_REG(SLICE_SWDIO_TXEN) = 0b00000000000000001111111111111111;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SLICE_REG(SLICE_SWDIO_TXEN) = 0b00000000000000001111111111111111;
SLICE_REG(SLICE_SWDIO_TXEN) = 0b11111111 << SGPIO_SWDIO_TXEN_DOUTM8_BIT;

uint32_t pkt[2];

SLICE_POS(SLICE_SWDIO_TXEN) = POS_POS(8) | POS_PRESET(0);
SLICE_REG(SLICE_SWDIO_TXEN) = 0b00000000000000001111111111111111;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SLICE_REG(SLICE_SWDIO_TXEN) = 0b00000000000000001111111111111111;
SLICE_REG(SLICE_SWDIO_TXEN) = 0b11111111 << SGPIO_SWDIO_TXEN_DOUTM8_BIT;

slice_pos_disable(SLICE_SWDIO_TMS_DOUT0_MASK | SLICE_SWDIO_TMS_DOUT1_MASK | SLICE_SWCLK_TCK_MASK | SLICE_SWDIO_TXEN_MASK | SLICE_SWDIO_TMS_OE_MASK);

SLICE_REG(SLICE_SWDIO_TMS_OE) = 1;
SLICE_REG(SLICE_SWDIO_TXEN) = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SLICE_REG(SLICE_SWDIO_TXEN) = 1;
SLICE_REG(SLICE_SWDIO_TXEN) = 1 << SGPIO_SWDIO_TXEN_DOUTM8_BIT;

slice_pos_disable(SLICE_SWDIO_TMS_DIN0_MASK | SLICE_SWDIO_TMS_DIN1_MASK | SLICE_SWCLK_TCK_MASK);

SLICE_REG(SLICE_SWDIO_TMS_OE) = 1;
SLICE_REG(SLICE_SWDIO_TXEN) = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SLICE_REG(SLICE_SWDIO_TXEN) = 1;
SLICE_REG(SLICE_SWDIO_TXEN) = 1 << SGPIO_SWDIO_TXEN_DOUTM8_BIT;

@elfmimi
Copy link
Contributor

elfmimi commented Aug 3, 2022

I want to see this going forward.

@groleo
Copy link
Author

groleo commented Aug 3, 2022

Hi @elfmimi ,

what testing infra do you use?

I've more or less used the on-board debug probe (on the rt1170-evk)
and many reboot/flash-daplink cycles but I would benefit from some sort
of logging/output.

I also have a OM13070 (which has a LPC4322)
if this can be used for test setup.

thx

@elfmimi
Copy link
Contributor

elfmimi commented Aug 3, 2022

Oh, I used OM13084 (interface: LPC4322 , target: LPC43S67) .
OM13070 (interface LPC4322 , target: LPC4337) should work just fine as well.

In case of OM13070 , LPCX4337_V3_SCHEMATIC_REVA3 , you can access SWD pins of LPC4322 through these test points.
Z11 = SWCLK
Z12 = GND
Z13 = SWDIO

For logging/output Segger RTT might be the best option.

@elfmimi
Copy link
Contributor

elfmimi commented Aug 4, 2022

Please have a look at this branch for how to do RTT. @groleo
https://github.com/elfmimi/DAPLink/tree/pull907-fix-and-rtt

After executing the above code, attach with pyocd to get the log.
pyocd rtt -t cortex-m -f 12000000 -a 0x10000000 -s 0x8000

@elfmimi
Copy link
Contributor

elfmimi commented Aug 6, 2022

You don't even need soldering to connect to SWD pins.
image

@groleo
Copy link
Author

groleo commented Aug 6, 2022

Cool.
And I should stick the SWD wires in a UART-over-USB ?

@elfmimi
Copy link
Contributor

elfmimi commented Aug 6, 2022

Yes, it's cool! No. SWD is SWD. bring in another CMSIS-DAP adapter.
RTT is a software solution which establishes logical pipe over SWD connection.

@groleo
Copy link
Author

groleo commented Aug 6, 2022

which probe should I get ?
Is the picoprobe ok ?

@elfmimi
Copy link
Contributor

elfmimi commented Aug 6, 2022

picoprobe may be is confirmed okay.
If you have RaspberryPi Pico to spare , try rust-dap .
pre-compiled binaries here. https://github.com/elfmimi/rust-dap/releases/tag/preview-0.1

@groleo
Copy link
Author

groleo commented Aug 6, 2022

that "may be ok" sounds worrisome (i'd like to avoid debugging the debugger).

Do you have a recommendation that's sure to work (or you use yourself) ?
thx

@elfmimi
Copy link
Contributor

elfmimi commented Aug 6, 2022

I suppose we should move this talk to daplink channel of pyOCD slack.
https://pyocd.slack.com/archives/C02GTJPENEN

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.

3 participants