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

Add ULX3S. #90

Merged
merged 2 commits into from
Jul 27, 2020
Merged

Add ULX3S. #90

merged 2 commits into from
Jul 27, 2020

Conversation

ktemkin
Copy link
Contributor

@ktemkin ktemkin commented Jul 26, 2020

Some interesting things about this platform that might be questionable here:

  • They use a homebrew microcontroller-based programming method that seems to be supported by a few obscure tools. I've included a toolchain_program that uses the tool that seems to be most widely in repos.
  • Seems to come with every size of FPGA, so we have four platforms in the same file. This seems cleaner than having four files inheriting from a "default", but this isn't how it's done elsewhere (Versa).
  • The header labeling on the side headers is a little odd on the ULX3S. The left and right headers are treated as one, with consecutive pin numbering; and the pins are labeled deferentially (e.g. 0+, 0-, 1+, 1-).

@ktemkin
Copy link
Contributor Author

ktemkin commented Jul 27, 2020

Note: failing check is elsewhere (machxo3_sk).

@whitequark
Copy link
Member

  • They use a homebrew microcontroller-based programming method that seems to be supported by a few obscure tools. I've included a toolchain_program that uses the tool that seems to be most widely in repos.

That's indeed what we do. Also, right now no toolchain_program implementation provides several options, but that could change in future.

  • Seems to come with every size of FPGA, so we have four platforms in the same file. This seems cleaner than having four files inheriting from a "default", but this isn't how it's done elsewhere (Versa).

We don't currently do this consistently, #89.

  • The header labeling on the side headers is a little odd on the ULX3S. The left and right headers are treated as one, with consecutive pin numbering; and the pins are labeled deferentially (e.g. 0+, 0-, 1+, 1-).

I would actually use a single connector with 0+, 0-, etc as pin names. Do you think that'd be more convenient?

@ktemkin
Copy link
Contributor Author

ktemkin commented Jul 27, 2020

I would actually use a single connector with 0+, 0-, etc as pin names. Do you think that'd be more convenient?

Something like this?

    connectors = [
        Connector("gpio", 0, {
            "0+": "B11",  "0-":  "C11", "1+":  "A10", "1-":  "A11",
            "2+": "A9",   "2-":  "B10", "3+":  "B9",  "3-":  "C10",
            "4+": "A7",   "4-":  "A8",  "5+":  "C8",  "5-":  "B8",
            "6+": "C6",   "6-":  "C7",  "7+":  "A6",  "7-":  "B6",
            "8+": "A4",   "8-":  "A5",  "9+":  "A2",  "9-":  "B1",
            "10-": "C4",  "10-": "B4",  "11-": "F4",  "11-": "E3",
            "12-": "G3",  "12-": "F3",  "13-": "H4",  "13-": "G5",
            "14-": "U18", "14-": "U17", "15-": "N17", "15-": "P16",
            "16-": "N16", "16-": "M17", "17-": "L16", "17-": "L17",
            "18-": "H18", "18-": "H17", "19-": "F17", "19-": "G18",
            "20-": "D18", "20-": "E17", "21-": "C18", "21-": "D17",
            "22-": "B15", "22-": "C15", "23-": "B17", "23-": "C17",
            "24-": "C16", "24-": "D16", "25-": "D14", "25-": "E14",
            "26-": "B13", "26-": "C13", "27-": "D13", "27-": "E13",
        })
    ]

I'm honestly not sure what's really convenient, here, so I'm entirely ambivalent.

@ktemkin
Copy link
Contributor Author

ktemkin commented Jul 27, 2020

That's indeed what we do. Also, right now no toolchain_program implementation provides several options, but that could change in future.

I considered ways to do this, and decided toolchain_program seemed mostly like a "null hypothesis" programming method; and like users wanting to use a specific tool wouldn't have that hard a time running it themselves.

That said, if we had a nice pattern of saying "if this tool is here, do this" until one succeeds, that might be a good thing to establish.

We don't currently do this consistently, #89.

Yep. Read through 89; and I don't think this is any of the patterns that have been used thus far, since I'm using a single file for four platforms. One downside of what I'm doing here is that the __main__ implementation picks a specific board, and so there's not an immediate "run the module, get a test".

On the other hand, it seems like creating a file for each inherited subtype can easily create lots of namespace sprawl -- (imagine an ECP5 board that's sometimes populated with e.g. 12F, 25F, 45F, 85F, the SerDes variants, the 5G SerDes variants, etc).

@whitequark
Copy link
Member

whitequark commented Jul 27, 2020

Something like this?

Something like this, yeah.

That said, if we had a nice pattern of saying "if this tool is here, do this" until one succeeds, that might be a good thing to establish.

I was thinking that it would need explicit selection, since there could be confusing issues otherwise: maybe you have openocd but it can't program the board because it's outdated, and you need Vivado for that.

One downside of what I'm doing here is that the __main__ implementation picks a specific board, and so there's not an immediate "run the module, get a test".

Perhaps use argparse to choose one of the boards?

@ktemkin
Copy link
Contributor Author

ktemkin commented Jul 27, 2020

Perhaps use argparse to choose one of the boards?

Done; though we should probably think standardizing some of this and/or adding in some helper infrastructure, since I imagine that this boilerplate might be repeated several times in boards with different variants:

if __name__ == "__main__":
    from .test.blinky import *
    
    variants = {
        '12F': ULX3S_12F_Platform,
        '25F': ULX3S_25F_Platform,
        '45F': ULX3S_45F_Platform,
        '85F': ULX3S_85F_Platform
    }
    
    # Figure out which FPGA variant we want to target...
    parser = argparse.ArgumentParser()
    parser.add_argument('variant', choices=variants.keys())
    args = parser.parse_args()

    # ... and run Blinky on it.
    platform = variants[args.variant]
    platform().build(Blinky(), do_program=True)

Want me to summarize this commentary in #89, or open a new 'improvement'?

@whitequark
Copy link
Member

I'm generally fine with the amount of boilerplate in nmigen-boards at the moment and I feel like there's no urgent need to factor out this sort of infrastructure, so I think for now a summary in #89 should be enough.

@ktemkin
Copy link
Contributor Author

ktemkin commented Jul 27, 2020

Done. Is this ready to merge, or do we want to give a day or so in case any good feedback gathers in #89?

@whitequark whitequark merged commit 479ae79 into amaranth-lang:master Jul 27, 2020
@whitequark
Copy link
Member

(It was ready to merge.)

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

Successfully merging this pull request may close these issues.

2 participants