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

UARTResource flow control signals direction mismatch for ICEStickPlatform #63

Closed
igrr opened this issue Jun 8, 2020 · 8 comments · Fixed by #65
Closed

UARTResource flow control signals direction mismatch for ICEStickPlatform #63

igrr opened this issue Jun 8, 2020 · 8 comments · Fixed by #65
Labels

Comments

@igrr
Copy link
Contributor

igrr commented Jun 8, 2020

I've noticed that dtr and rts signals of UARTResource are defined as outputs, but on the IceStick they are actually inputs, according to the schematic.

I see two options for fixing this:

  1. Add role argument (dce/dte) to UARTResource, similar to SPIResource; set the direction of the flow control pins based on the role; set correct role for ICEStickPlatform.
  2. Assume that UARTResource is only going to be used with role=="dce", and reverse the direction of flow control pins. This might (?) break some designs.

I can submit a PR for either of the options, would appreciate some guidance which one to choose.

Thanks for maintaining this project!

@whitequark
Copy link
Member

Option 1 seems ideal. Just to confirm, there is electrically no way to use the UART as currently defined on the icestick, right?

@whitequark whitequark added the bug label Jun 8, 2020
@whitequark
Copy link
Member

Oh, and please submit the change to UARTResource and to the board as two separate PRs.

@igrr
Copy link
Contributor Author

igrr commented Jun 8, 2020

Just to confirm, there is electrically no way to use the UART as currently defined on the icestick, right?

This could be possible by storing custom FT2232H pin configuration into the EEPROM, but I haven't tried.

Is it possible to define two overlapping resources ("uart" and "uart_dte"), and let the user choose which one to claim? One resource would use dce role, the other one dte.

@whitequark
Copy link
Member

Is it possible to define two overlapping resources ("uart" and "uart_dte"), and let the user choose which one to claim? One resource would use dce role, the other one dte.

Yes, but if this requires reflashing the devboard, it's probably best to leave this up to add to whoever uses the platform.

Speaking of which: do you think you could take a look at the other board and figure out if DTR/RTS are swapped on them, too? I think most FPGA boards are configured as DCE (correct me if I'm wrong), and you might actually be the first to care about the direction of those signals, so it's quite possible that our default is wrong and should be switched.

@igrr
Copy link
Contributor Author

igrr commented Jun 8, 2020

Sure, I can. There are only 3 boards so far which use the flow control signals. Will check the schematics and include the changes into the 2nd PR.

@igrr
Copy link
Contributor Author

igrr commented Jun 8, 2020

Turns out things get a bit complicated with some existing boards.

For example, nexys4ddr: the platform file defines rts="D3", cts="E5". This doesn't match the schematic, though (https://reference.digilentinc.com/_media/nexys4-ddr:nexys_4_ddr_sch.pdf — RTS is E5, CTS is D3). So the pins have likely been swapped in the platform definition to make it useable with signal directions defined in UARTResource. If i now restore the signal names used in the schematic, and set role="dce" in UARTResource, this would be "correct", but would break existing projects based on that board. This repository is pre-1.0, so I suppose some breaking changes are allowed, the question is, how much breakage is acceptable?

@igrr
Copy link
Contributor Author

igrr commented Jun 8, 2020

Same story with ice40_hx8k_b_evn: PR m-labs/nmigen-boards#41 has swapped DTR/RTS as a fix for incorrect direction. But the problem is, the direction is still incorrect for the remaining signals (like rts/cts). And for some signals (like ri), there is nothing to swap them with!

@whitequark
Copy link
Member

This repository is pre-1.0, so I suppose some breaking changes are allowed, the question is, how much breakage is acceptable?

This is a clear screwup that we shouldn't be stuck forever. Let's just tear this bandage off early and do things properly; it's not that many boards.

Once we get nmigen-boards on PyPI I'm going to use package versions to indicate breaking changes so that people get a bit more visible notice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants