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

boards: add Colorlight 5a-75b 7.0 #75

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

@FFY00
Copy link

@FFY00 FFY00 commented Jul 9, 2020

It would be great if we were able to have a generic class and pass the revision as an argument but it doesn't seem to be possible due to the use of abstractproperty. I could get around this using a metaclass but I don't think it's something we want to do, right?

I based this on
https://github.com/litex-hub/litex-boards/blob/master/litex_boards/platforms/colorlight_5a_75b.py

There are a few things that I was not able to figure out. How do I define the spi flash? The clock is required. What is the nmigen equivalent of Misc("SLEWRATE=FAST")?

I did not test all components.

Signed-off-by: Filipe Laíns <lains@archlinux.org>
@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

How do I define the spi flash?

The one that uses MCLK? Good question! I'd say write the definition similar to what SPIFlashResources would generate, omit the clock, and call it something like cfg_spiflash to mark it as a special flash resource that wouldn't be usable with an SPI flash core without some modifications.

In the future we might add a special pin type for this case, since it seems to be fairly common; feel free to open an issue on nmigen/nmigen so we can discuss the best way to handle this.

What is the nmigen equivalent of Misc("SLEWRATE=FAST")?

Attrs(SLEWRATE="FAST") should do it.

It would be great if we were able to have a generic class and pass the revision as an argument but it doesn't seem to be possible due to the use of abstractproperty.

Can you explain this in more detail? I'm not married to abstractproperty, but I'd like to understand the problem better first.

@FFY00
Copy link
Author

@FFY00 FFY00 commented Jul 9, 2020

I had prototyped this only then to find out it's not possible:

from nmigen.build import *
from nmigen.vendor.lattice_ecp5 import *
from nmigen_boards.resources import *


__all__ = ['Colorlight5a75bPlatform']


class Colorlight5a75bPlatform(LatticeECP5Platform):
    _package = {
        '6.1': 'BG381',
        '7.0': 'BG256',
        '7.1': 'BG256',
    }

    _resources = {
        '7.0': [
            Resource('clk25', 0, Pins('P6'), Clock(25e6), Attrs(GLOBAL=True, IO_STANDARD='LVCMOS33')),

            *LEDResources(pins='P11', attrs=Attrs(IO_STANDARD='LVCMOS33')),
            Resource('user_led', 0, PinsN('P11', dir='o'), Attrs(IO_STANDARD='LVCMOS33')),

            *ButtonResources(pins='M13', attrs=Attrs(IO_STANDARD='LVCMOS33')),
            Resource('user_btn', 0, PinsN('M13'), Attrs(IO_STANDARD='LVCMOS33')),

            # available in the J19 connector (rx = btn, tx=led)
            UARTResource(0,
                rx='P11', tx='M13',
                attrs=Attrs(IO_STANDARD='LVCMOS33')
            ),

            # W25Q32JV
            #SPIResource(0,
            #    cs='N8', mosi='T8', miso='T7',  # clk driven through USRMCLK
            #    attrs=Attrs(IO_STANDARD='LVCMOS33')
            #),

            # M12616161A
            Resource('sdram_clock', 0, Pins('C6'), Attrs(IO_STANDARD='LVCMOS33')),
            Resource('sdram', 0,
                Subsignal('a', Pins('A9 E10 B12 D13 C12 D11 D10 E9 D9 B7 C8')),
                Subsignal('dq', Pins(
                    'B13 C11 C10 A11 C9 E8  B6  B9  '
                    'A6  B5  A5  B4  B3 C3  A2  B2  '
                    'E2  D3  A4  E4  D4 C4  E5  D5  '
                    'E6  D6  D8  A8  B8 B10 B11 E11  '
                )),
                Subsignal('we_n',  Pins('C7')),
                Subsignal('ras_n', Pins('D7')),
                Subsignal('cas_n', Pins('E7')),
                Subsignal('ba',    Pins('A7')),
                Attrs(IO_STANDARD='LVCMOS33')
            ),

            # B50612D
            Resource('eth_clocks', 0,
                Subsignal('tx', Pins('M2')),
                Subsignal('rx', Pins('M1')),
                Attrs(IO_STANDARD='LVCMOS33')
            ),
            Resource('eth', 0,
                Subsignal('rst_n',   Pins('P5')),
                Subsignal('mdio',    Pins('T2')),
                Subsignal('mdc',     Pins('P3')),
                Subsignal('rx_ctl',  Pins('N6')),
                Subsignal('rx_data', Pins('N1 M5 N5 M6')),
                Subsignal('tx_ctl',  Pins('M3')),
                Subsignal('tx_data', Pins('L1 L3 P2 L4')),
                Attrs(IO_STANDARD='LVCMOS33')
            ),
            Resource('eth_clocks', 1,
                Subsignal('tx', Pins('M12')),
                Subsignal('rx', Pins('M16')),
                Attrs(IO_STANDARD='LVCMOS33')
            ),
            Resource('eth', 1,
                Subsignal('rst_n',   Pins('P5')),
                Subsignal('mdio',    Pins('T2')),
                Subsignal('mdc',     Pins('P3')),
                Subsignal('rx_ctl',  Pins('L15')),
                Subsignal('rx_data', Pins('P13 N13 P14 M15')),
                Subsignal('tx_ctl',  Pins('R15')),
                Subsignal('tx_data', Pins('T14 R12 R13 R14')),
                Attrs(IO_STANDARD='LVCMOS33')
            ),

            Resource('usb', 0,
                Subsignal('d_p',    Pins('M8')),
                Subsignal('d_n',    Pins('R2')),
                Subsignal('pullup', Pins('P4')),
                Attrs(IO_STANDARD='LVCMOS33')
            ),
        ],
    }

    _connectors = {
        '7.0': [
            Connector('j', 1, 'F3  F1  G3  - G2  H3  H5  F15 L2 K1 J5 K2 B16 J14 F12 -'),
            Connector('j', 2, 'J4  K3  G1  - K4  C2  E3  F15 L2 K1 J5 K2 B16 J14 F12 -'),
            Connector('j', 3, 'H4  K5  P1  - R1  L5  F2  F15 L2 K1 J5 K2 B16 J14 F12 -'),
            Connector('j', 4, 'P4  R2  M8  - M9  T6  R6  F15 L2 K1 J5 K2 B16 J14 F12 -'),
            Connector('j', 5, 'M11 N11 P12 - K15 N12 L16 F15 L2 K1 J5 K2 B16 J14 F12 -'),
            Connector('j', 6, 'K16 J15 J16 - J12 H15 G16 F15 L2 K1 J5 K2 B16 J14 F12 -'),
            Connector('j', 7, 'H13 J13 H12 - G14 H14 G15 F15 L2 K1 J5 K2 B16 J14 F12 -'),
            Connector('j', 8, 'A15 F16 A14 - E13 B14 A13 F15 L2 K1 J5 K2 B16 J14 F12 -'),
        ],
    }

    def __init__(self, revision='7.0', *args, **kwargs):
        assert revision in {'7.0'}  # not all revisions are supported yet

        self.device = 'LFE5U-25F'
        self.package = self._package[revision]
        self.default_clk = 'clk25'
        self.speed = 8

        self.resources = self._resources[revision] or []
        self.connectors = self._connectors[revision] or []

        super().__init__(*args, **kwargs)


if __name__ == "__main__":
    from nmigen_boards.test.blinky import Blinky
    for revision in {'7.0'}:
        Colorlight5a75bPlatform(revision).build(Blinky(), do_program=True)

I'd say to add a hook after the child init to verify the existence of the attributes, alternatively this could be in the super init but the child can choose not to call it. This should be backward compatible and more pythonic.

Actually, I believe the current approach has a design flaw. The modules and connectors are mutable objects (they're lists), by making them class attributes as opposed to instance attributes, they are shared between all objects. If an object decides to change them, it will leak to the other ones, which is undesirable.

Example:

class BasePlatform():
    part = 'some_super_special_part'
    modules = []

    def build(self):
        print(f'{self.__class__.__name__} modules = {self.modules}')


class MySlightlyModifiedPlatform(BasePlatform):
    def __init__(self):
        super().__init__()
        self.modules.append('something custom')


MySlightlyModifiedPlatform().build()
BasePlatform().build()
MySlightlyModifiedPlatform modules = ['something custom']
BasePlatform modules = ['something custom']

I know it's not usual to be building for multiple platforms in the same session, but AFAIK it is a supported use-case, right?

By making them instance variables we get rid of this behavior.

class BasePlatform():
    def __init__(self):
        self.part = 'some_super_special_part'
        self.modules = []

    def build(self):
        print(f'{self.__class__.__name__} modules = {self.modules}')


class MySlightlyModifiedPlatform(BasePlatform):
    def __init__(self):
        super().__init__()
        self.modules.append('something custom')


MySlightlyModifiedPlatform().build()
BasePlatform().build()
MySlightlyModifiedPlatform modules = ['something custom']
BasePlatform modules = []

I usually just use class attributes to hold constants or variables I actually need to share between instances.

So, my proposal is to do something like this.

class PlatformValidator(type):
    def __call__(cls, *args, **kwargs):
        instance = super().__call__(*args, **kwargs)
        cls.validate(instance)
        return instance

    def validate(instance):
        pass
class CustomPlatformValidator(PlatformValidator):
    def validate(instance):
        assert hasattr(instance, 'special_attribute')

class CustomPlatform(metaclass=CustomPlatformValidator):
   def __init__(self):
       special_attribute = 'something'

This is just an example, the validation interface could be modified. If you have any concerns or dislikes about the design, just let me know and I'll help you find something suitable.

You can always just opt to add the checks in the super init or something similar, just be sure to document the behavior.

Btw, the abstractproperty implementation is similar to this 😁.

@FFY00
Copy link
Author

@FFY00 FFY00 commented Jul 9, 2020

Just to explain what is happening there, a metaclass allows us to override the call protocol (the use of () in an object, eg. MyClass()), so what I do override it to perform the call and validate the object.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

I had prototyped this only then to find out it's not possible:

Ah yeah right, so we only use constructor arguments for choosing options on a single board. Most boards wouldn't have them at all, and in the the few which do, you would select things such as jumpered IO bank voltage with the arguments.

In case there are different but similar boards, we do one of the following:

  • Just copy the code (TinyFPGA AX1, AX2),
  • Use inheritance (Versa ECP5, ECPIX-5).

I'm not sure what's more appropriate here but likely the latter.

Actually, I believe the current approach has a design flaw. The modules and connectors are mutable objects (they're lists), by making them class attributes as opposed to instance attributes, they are shared between all objects. If an object decides to change them, it will leak to the other ones, which is undesirable.

Yeah, I'm aware of this quirk of Python. The reason they aren't tuples is that lists are, generally speaking, used with homogeneous elements, and tuples with heterogenous elements; a tuple would behave exactly the same as a hypothetical frozenlist, but it doesn't communicate the same thing to a person. When inheriting boards, we work around this issue with:

resources = ParentPlatform.resources + [
    ...,
]

and once you instantiate a platform, the problem doesn't exist anymore because the resources instance attribute replaces the resources class attribute. (Which, now that I give it a fresh look, is kind of badly implemented because their types are different...)

I know it's not usual to be building for multiple platforms in the same session, but AFAIK it is a supported use-case, right?

Yes, that's a supported use case; however as described above this problem is not currently acutely causing issues, it's more of a gotcha. Which is still bad, of course!


At the moment I don't have the bandwidth to redesign the build system DSL, so let's not do it for now. In long term we likely have to redesign it anyway for a simple reason: the current DSL is actively hostile to every autoformatter I found, and unlike with the normal nMigen codebase (where I don't use an autoformatter and it's mostly fine), the nMigen boards codebase really, really, really needs to be autoformattable. Whatever the replacement is, it would have to fix this issue as well.

@FFY00
Copy link
Author

@FFY00 FFY00 commented Jul 9, 2020

Yes, that's a supported use case; however as described above this problem is not currently acutely causing issues, it's more of a gotcha. Which is still bad, of course!

Of course, my main worry is that it is a really hard issue to find out and that it has the potential to damage hardware.

At the moment I don't have the bandwidth to redesign the build system DSL, so let's not do it for now.

I am more than happy to help out with this if you want.

In long term we likely have to redesign it anyway for a simple reason: the current DSL is actively hostile to every autoformatter I found, and unlike with the normal nMigen codebase (where I don't use an autoformatter and it's mostly fine), the nMigen boards codebase really, really, really needs to be autoformattable. Whatever the replacement is, it would have to fix this issue as well.

Can you elaborate a little bit more on this? I gave it a try with black, increasing the line length to 127 (same size of the github editor) and it looks fairly decent. If you want more choice, there is yapf which is pretty configurable. If you want, I can help out with this too, just let me know what you want to achieve.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

I am more than happy to help out with this if you want.

I'm not going to have time to either (co-)design a new DSL or to review the code introducing it and the migration path for the next several months at least. I don't even have enough time to review all of the things that are more important, much less something that doesn't present an immediate problem.

I gave it a try with black, increasing the line length to 127 (same size of the github editor) and it looks fairly decent. If you want more choice, there is yapf which is pretty configurable.

I recall trying both of these, discovering their output gets really ugly with some boards/peripherals, and deciding to implement my own on top of one of the existing frameworks that would have the domain-specific knowledge. IIRC the main problem was that they either tended to blow up some cases horizontally or vertically, depending on the exact pattern.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

Oh, to add to this, the problem with formatting in nmigen-boards is acute enough I'm interested to find a solution for it relatively soon, so we could certainly collaborate on that!

@FFY00
Copy link
Author

@FFY00 FFY00 commented Jul 9, 2020

I'm not going to have time to either (co-)design a new DSL or to review the code introducing it and the migration path for the next several months at least. I don't even have enough time to review all of the things that are more important, much less something that doesn't present an immediate problem.

Alright, ping me when you do 😁.

I recall trying both of these, discovering their output gets really ugly with some boards/peripherals, and deciding to implement my own on top of one of the existing frameworks that would have the domain-specific knowledge. IIRC the main problem was that they either tended to blow up some cases horizontally or vertically, depending on the exact pattern.

Yes, that is true. Our issue with board/peripheal definitions is that we use too much functions in declarations. The code formatters are not designed for the functions to be declarative, they are assumed to be procedural, usually the data you pass varies a lot. I really wish this code formatters could detect clusters of similar declarations in function arguments and treat them as a block (eg. our use of Subsignal).

Implementing a code formatter would be a lot of work, especially for a configurable one, I imagine 😕.

I have recently adopted yapf in large codebase, so I have played quite a bit with the configurations. I have some proposals for nmigen and nmigen-boards, they are far from perfect but are okay-ish.

nmigen: https://github.com/FFY00/nmigen/tree/yapf
nmigen-boards: https://github.com/FFY00/nmigen-boards/tree/yapf

Let me know what you think.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

Our issue with board/peripheal definitions is that we use too much functions in declarations. The code formatters are not designed for the functions to be declarative, they are assumed to be procedural, usually the data you pass varies a lot.

That's exactly the reason I think the API should be redesigned.

nmigen: https://github.com/FFY00/nmigen/tree/yapf

I strongly dislike this and I am against adopting any autoformatter whatsoever in nmigen itself. It doesn't matter that the barrier to contributions is slightly lower if each time I open the editor I resent what I see. The other reason is that git still doesn't have a good way to exclude certain commits from appearing in blame.

nmigen-boards: https://github.com/FFY00/nmigen-boards/tree/yapf

This is objectively a mess. The original code was in a nice tabular form with a header. After the autoformatter is done, all structure is lost.

Screenshot_20200709_205954

I think nmigen-boards needs an autoformatter but it will either have to be an off-the-shelf autoformatter running against a redesigned board definition DSL, or a custom autoformatter running against the existing DSL.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

Implementing a code formatter would be a lot of work, especially for a configurable one, I imagine .

IIRC, I looked into the internals of yapf and I liked their approach. So if I would design an autoformatter for the existing nmigen-boards DSL, I would start with yapf and customize it for the use case. I'm not sure how much work that'd actually take. I might still do it some day.

@FFY00
Copy link
Author

@FFY00 FFY00 commented Jul 9, 2020

This is objectively a mess. The original code was in a nice tabular form with a header. After the autoformatter is done, all structure is lost.

Oh, I didn't notice this particular piece of code. This is awful, ouch.

I think nmigen-boards needs an autoformatter but it will either have to be an off-the-shelf autoformatter running against a redesigned board definition DSL, or a custom autoformatter running against the existing DSL.

Yes. Redesigning the board definition DSL seems easier to me. I am not particularly keen on the current desgin anyway, it's not very pythonic.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

I am not particularly keen on the current desgin anyway, it's not very pythonic.

Neither am I to be honest. Like many (perhaps most) of the things borrowed more or less exactly from Migen, it is more of a liability in the long term.

Resource('clk25', 0, Pins('P6'), Clock(25e6), Attrs(GLOBAL=True, IO_STANDARD='LVCMOS33')),

*LEDResources(pins='P11', attrs=Attrs(IO_STANDARD='LVCMOS33')),
Resource('user_led', 0, PinsN('P11', dir='o'), Attrs(IO_STANDARD='LVCMOS33')),
Copy link
Member

@whitequark whitequark Nov 24, 2020

Choose a reason for hiding this comment

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

Please use "led", like in LEDResources.

Resource('user_led', 0, PinsN('P11', dir='o'), Attrs(IO_STANDARD='LVCMOS33')),

*ButtonResources(pins='M13', attrs=Attrs(IO_STANDARD='LVCMOS33')),
Resource('user_btn', 0, PinsN('M13'), Attrs(IO_STANDARD='LVCMOS33')),
Copy link
Member

@whitequark whitequark Nov 24, 2020

Choose a reason for hiding this comment

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

Please use "button", like in ButtonResources.

#),

# M12616161A
Resource('sdram_clock', 0, Pins('C6'), Attrs(IO_STANDARD='LVCMOS33')),
Copy link
Member

@whitequark whitequark Nov 24, 2020

Choose a reason for hiding this comment

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

Please use SDRAMResource.

),

# W25Q32JV
#SPIResource(0,
Copy link
Member

@whitequark whitequark Nov 24, 2020

Choose a reason for hiding this comment

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

Please update the definition to be up-to-date (use COPI etc) with a comment about USRMCLK. At the moment it is not possible to request this resource but this will be fixed.

Attrs(IO_STANDARD='LVCMOS33')
),

Resource('usb', 0,
Copy link
Member

@whitequark whitequark Nov 24, 2020

Choose a reason for hiding this comment

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

Please use DirectUSBResource.

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

Successfully merging this pull request may close these issues.

None yet

2 participants