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 OrangeCrab board. #76

Merged
merged 1 commit into from Jul 13, 2020
Merged

Conversation

tdaede
Copy link
Contributor

@tdaede tdaede commented Jul 11, 2020

I have not tested this beyond the basic blinky/button test.

@miek
Copy link
Contributor

miek commented Jul 11, 2020

There are also a bunch of r0.1 boards in the wild and it'd be nice to support those later, so maybe this should get a version number in the name before merging?

@tdaede
Copy link
Contributor Author

tdaede commented Jul 11, 2020

Realized that LUNA has an OrangeCrab platform. I'll see if I can make the nmigen platform match. https://github.com/greatscottgadgets/luna/blob/master/luna/gateware/platform/orangecrab.py

@tdaede
Copy link
Contributor Author

tdaede commented Jul 11, 2020

Updated to better match LUNA, but there are still some differences:

  • RGB LEDs, button, and SPIFlash use dedicated resource type rather than generic Resource.
  • DDR3 interface uses ecp5-versa names rather than LUNA names. Don't know which one is better.
  • SD card uses SD interface names rather than SPI.
  • User IO is exposed only via a Connector.

@tdaede tdaede force-pushed the orangecrab branch 3 times, most recently from b898069 to 90eb127 Compare Jul 11, 2020
nmigen_boards/orangecrab_r0d2.py Outdated Show resolved Hide resolved
nmigen_boards/orangecrab_r0d2.py Outdated Show resolved Hide resolved
nmigen_boards/orangecrab_r0d2.py Outdated Show resolved Hide resolved
nmigen_boards/orangecrab_r0d2.py Outdated Show resolved Hide resolved
nmigen_boards/orangecrab_r0d2.py Outdated Show resolved Hide resolved
@tdaede tdaede force-pushed the orangecrab branch 2 times, most recently from 15231a8 to 1f0ccb2 Compare Jul 13, 2020
@gregdavill
Copy link

gregdavill commented Jul 13, 2020

Would it be worth passing the version in as a default parameter, and call it the OrangeCrab platform, instead of embedding the revision number in the class name?

This is how we're handling it on litex: https://github.com/litex-hub/litex-boards/blob/master/litex_boards/platforms/orangecrab.py

@tdaede
Copy link
Contributor Author

tdaede commented Jul 13, 2020

I looked at the schematics and they seemed to be quite different. Indeed looking at the litex platform, there's almost no shared code, so it's mostly a style choice.

@whitequark
Copy link
Member

whitequark commented Jul 13, 2020

Would it be worth passing the version in as a default parameter, and call it the OrangeCrab platform, instead of embedding the revision number in the class name?

In nmigen-boards the convention is to have different classes for materially different platforms; parameters are reserved for things such as jumpers that change IO voltage and so on.

nmigen_boards/orangecrab_r0_2.py Outdated Show resolved Hide resolved
nmigen_boards/orangecrab_r0_2.py Outdated Show resolved Hide resolved
nmigen_boards/orangecrab_r0_2.py Outdated Show resolved Hide resolved
return super().toolchain_prepare(fragment, name, **overrides, **kwargs)

def toolchain_program(self, products, name):
dfu_suffix = os.environ.get("DFU_SUFFIX", "dfu-suffix")
Copy link
Member

@whitequark whitequark Jul 13, 2020

Choose a reason for hiding this comment

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

Arguably the dfu-suffix step should be a part of the normal build process, but this is a complex change I wouldn't insist you make here.

Copy link
Contributor Author

@tdaede tdaede Jul 13, 2020

Choose a reason for hiding this comment

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

I have the option of running dfu-util on the un-suffixed binary as well by manually passing vid/pid, which I considered doing, though it doesn't yield a distributable .dfu which is slightly annoying.

@whitequark
Copy link
Member

whitequark commented Jul 13, 2020

Meanwhile can you submit the RGB LED changes as a separate PR?

@tdaede tdaede force-pushed the orangecrab branch 3 times, most recently from 78671da to c372d92 Compare Jul 13, 2020
nmigen_boards/orangecrab_r0_2.py Outdated Show resolved Hide resolved
@tdaede tdaede force-pushed the orangecrab branch 2 times, most recently from f9afba2 to a0377c7 Compare Jul 13, 2020
nmigen_boards/orangecrab_r0_2.py Outdated Show resolved Hide resolved
@whitequark
Copy link
Member

whitequark commented Jul 13, 2020

@tdaede Okay, I think the last thing that's left is dfu-suffix. Can you do something similar to this:

def required_tools(self):
    super.required_tools + [
        "dfu-suffix"
    ]

def command_templates(self):
    super.command_templates + [
        r"""
        {{invoke_tool("dfu-suffix")}}
            -v 1209 -p 5af0 -a {{name}}.bit
        """,
    ]

and check that it works?

PS: I pushed to this branch, careful

@whitequark
Copy link
Member

whitequark commented Jul 13, 2020

It's kind of weird to just have the DFU suffix on the bare bitstream file, but provided that (a) dfu-util is the primary method of programming this board and (b) this shouldn't inherently break other uses, we can just merge it and fix any breakage in the future should it arise.

@tdaede
Copy link
Contributor Author

tdaede commented Jul 13, 2020

Annoyingly dfu-suffix has no -o option, and I don't want to put a dep on cp. There is still the bare bitstream into dfu-util option.

The main reason I could see this breaking in the future is if any sort of dependency tracking is added, as dfu-util will reject putting multiple DFU suffixes on.

@whitequark
Copy link
Member

whitequark commented Jul 13, 2020

The main reason I could see this breaking in the future is if any sort of dependency tracking is added, as dfu-util will reject putting multiple DFU suffixes on.

Definitely not, the approach used by edalize was considered and rejected a long time ago.

@whitequark whitequark merged commit 856f712 into amaranth-lang:master Jul 13, 2020
@whitequark
Copy link
Member

whitequark commented Jul 13, 2020

Thanks!

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.

None yet

5 participants