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

Added gatemate_a1_evb board #249

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TarikHamedovic
Copy link

@TarikHamedovic TarikHamedovic commented Jul 23, 2024

Hello

I opened up a pull request also on amaranth-lang for GateMate, I opened it up here because the gatemate_a1_evb.py file is present in amaranth-boards folder.

I used openFPGALoader for programming and added the programming in the gatemate_a1_evb.py file.

The constraint file is written by me, I went through the documentation and placed the appropriate pins with their names. There could be some mistakes, I checked it many times.

@TarikHamedovic
Copy link
Author

The Pull Request failed because it doesn't recognize 'GateMatePlatform' import because I made a separate Pull Request for that in 'amaranth-lang'

@whitequark
Copy link
Member

The Pull Request failed because it doesn't recognize 'GateMatePlatform' import because I made a separate Pull Request for that in 'amaranth-lang'

This is normal. We don't really have a process for introducing a new platform since it's just going to keep failing until a new Amaranth release is made.

Comment on lines 275 to 276
overrides = dict(yosys_opts="-p 'synth_gatemate -top {} -nomx8 -vlog'".format(name),
pr_opts="-v -ccP",
Copy link
Member

@whitequark whitequark Jul 23, 2024

Choose a reason for hiding this comment

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

While these could and should be valid overrides in principle, they currently aren't: you are not doing anything with yosys_opts or pr_opts in your Amaranth PR. In addition, basic options like this should not be in a board file at all.

Copy link
Author

Choose a reason for hiding this comment

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

Should I remove toolchain_prepare ?

Copy link
Member

Choose a reason for hiding this comment

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

I can't see a reason why one would be needed here.

@property
def required_tools(self):
return super().required_tools + [
"openFPGALoader"
Copy link
Member

Choose a reason for hiding this comment

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

Although it's not currently documented, required_tools is only for the tools required to produce the bitstream. Programming tools should not included and it will cause issues if they are.

Copy link
Author

Choose a reason for hiding this comment

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

I will remove that, but just to point out that ulx3s.py has the same line aswell.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's also wrong (I missed it during review previously).

def toolchain_prepare(self, fragment, name, **kwargs):
overrides = dict(yosys_opts="-p 'synth_gatemate -top {} -nomx8 -vlog'".format(name),
pr_opts="-v -ccP",
openfpgaloader_opts="-b gatemate_evb_jtag --cable dirtyJtag")
Copy link
Member

Choose a reason for hiding this comment

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

Since overrides only affect the build process, not the programming process, this option will never be used.

pr_opts="-v -ccP",
openfpgaloader_opts="-b gatemate_evb_jtag --cable dirtyJtag")
overrides.update(kwargs)
return super().toolchain_prepare(fragment, name, **overrides)
Copy link
Member

Choose a reason for hiding this comment

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

(In this particular case I don't think you need any toolchain overrides.)

amaranth_boards/gatemate_a1_evb.py Outdated Show resolved Hide resolved
Subsignal("cs", Pins("IO_EA_B3", dir ="o" )),
),

Resource("pmod", 0,
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely a Connector.

Copy link
Author

Choose a reason for hiding this comment

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

Changed from Resource to Connector



# Mostly used for RP2040
Resource("gpio", 0,
Copy link
Member

Choose a reason for hiding this comment

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

This also looks like a Connector.

Copy link
Author

Choose a reason for hiding this comment

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

This is used for RP2040, and there are no external connections to these pins here. Are they still connectors?

Copy link

Choose a reason for hiding this comment

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

The point of a Connector isn't necessarily that it's a physical connector, but that it's an extension point where a downstream user of this platform can attach project specific resources. A project using SPI to communicate with the RP2040 could e.g. want to attach a SPIResource to some of those signals.

amaranth_boards/gatemate_a1_evb.py Show resolved Hide resolved
Comment on lines 287 to 288
class GateMate_A1_EVB(_GateMate_A1_EVB):
name = "Olimex GateMateA1-EVB"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class GateMate_A1_EVB(_GateMate_A1_EVB):
name = "Olimex GateMateA1-EVB"

amaranth_boards/gatemate_a1_evb.py Outdated Show resolved Hide resolved
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.

3 participants