search

probe-rs / rusty-probe-firmware

41 stars 15 forks source link

Fix swd sequence and swj pins #29

Closed korken89 closed 2 weeks ago

korken89 commented 2 weeks ago

In collision with https://github.com/probe-rs/rusty-probe-firmware/pull/27 . @jonathanherbstgrapple, feel free to fix your PR or ping me to use this one.

jonathanherbstgrapple commented 2 weeks ago

@korken89 Looks good to me, we can use this PR. I just didn't know how you wanted to handle the swj pins issue. Tried this one out this morning and it works fine for me.

korken89 commented 2 weeks ago

Yeah I see what you mean. The DAP_SWJ_Pins spec is not that clear to me what they mean, however the I found the following implementation in CMSIS: https://github.com/ARM-software/CMSIS_5/blob/master/CMSIS/DAP/Firmware/Source/DAP.c#L302-L397

I think following their example makes sense. I'll have a look at what they are doing an relate it to the DAP_SWJ_Pins documentation.

korken89 commented 2 weeks ago

@jonathanherbstgrapple I think I fixed it now, could you give it a try? This should be in-line with the documentation and the C implementation.

jonathanherbstgrapple commented 2 weeks ago

@korken89 The DAP_SWJ_Pins function doesn't make sense to me. It never sets the pin direction, even though the library brings a function to set the pin direction for users to implement. So I guess it just attempts to do this in whatever pin direction the pins are in when the function is called. I took a look at the raspberry-pi debug probe source and all their pin function implementations either return 0 or do nothing. https://github.com/raspberrypi/debugprobe/blob/master/include/DAP_config.h#L339

jonathanherbstgrapple commented 2 weeks ago

I think what you've implemented is correct, It doesn't panic anymore, but I don't know how to completely test it since I don't think openocd does anything with the response except print it out. Info : SWCLK/TCK = 1 SWDIO/TMS = 1 TDI = 0 TDO = 0 nTRST = 0 nRESET = 0

The MCU-Link example has a more complete example of this. https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/DAP/Firmware/Examples/MCU-LINK/DAP_config.h

The only note I have is that I think _is_sethigh tests what the output bit is set to and not what the actual level on the pin is, which is different from the MCU-Link implementation, but I don't think it will make a difference especially if you have current limiting resistors.

I also learned that all the pins except TDO are output pins (SWDIO should return to output after a read operation), and the _DAP_SWJPins function skips outputting TDO, but you probably already knew that.

korken89 commented 2 weeks ago

Yeah, this hardware does not have support to see if a pin in output mode is shorted as the logic level translator does the job. It seems to work for me as well, so I'll merge it and if we find issues we can investigate again!