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

[RFC] Add InstanceResource to use Instance I/O as Platform I/O #525

Closed
Ravenslofty opened this issue Nov 3, 2020 · 9 comments
Closed

[RFC] Add InstanceResource to use Instance I/O as Platform I/O #525

Ravenslofty opened this issue Nov 3, 2020 · 9 comments
Labels

Comments

@Ravenslofty
Copy link
Contributor

This is an RFC for #308.

Some chips like the Intel Cyclone V SoC and Xilinx Zynq require Instances to access certain I/O functions not directly available through top-level module ports. Others, such as the Lattice iCE40, use Instances to source clocks.

Presently, the latter chip manually instantiates SB_HFOSC, but I think there should be a better way.

nMigen should provide an InstanceResource (or perhaps an InstanceConnector?) to treat I/O from an Instance as global I/O provided by platform.request.

@whitequark
Copy link
Member

Okay, this isn't really an RFC because this doesn't explain the exact mechanism through which this will happen. There isn't anything to comment on.

@anuejn
Copy link
Contributor

anuejn commented Nov 8, 2020

I propose, that we just allow to pass and Elaboratable object instead of a Pins object to Resource.
This way one could add arbitrary logic to the platform.
This, however, has one major downside: Since Instances dont expose their ports as attributes but rather as constructor arguments, every Instance first needs to be wrapped.

Usage Example for the proposal:

class ExamplePlatform(LatticeMachXO2Platform):
    device = "LCMXO2-2000HC"
    package = "TG100"
    speed = "6"
    default_clk = "clk_internal"

    class Osc(Elaboratable):
        def __init__(self, freq=2.08e6):
            self.freq = freq
            self.i = Signal()

        def elaborate(self, platform):
            m = Module()

            inst = m.submodules.inst = Instance("OSCH", o_OSC=self.i)
            inst.attrs["NOM_FREQ"] = str(self.freq / 1e6)

            return m
        
    resources = [
        Resource("clk_internal", 0, Osc())
    ]

@whitequark
Copy link
Member

I propose, that we just allow to pass and Elaboratable object instead of a Pins object to Resource.

I like this direction. I think wrapping Instances is reasonable: you will usually want to do error checking, change pin polarity, make sure everything is hooked to the right clocks and resets...

There are multiple unresolved questions:

  • What happens with resources that only partially use instances? E.g. with USRMCLK on ECP5, you would only drive the clock through the instance.
  • What happens if you refer twice to the same elaboratable?
  • Pin has a fairly rich interface: it has attributes width, dir, xdr, name. It is not very nice to require developers to reimplement all those (or risk opaque breakage).

@whitequark
Copy link
Member

whitequark commented Nov 25, 2020

Here is my counter-RFC.

Mechanism

We currently have a bunch of methods like:

def get_input_output(self, pin, port, attrs, invert):

Let's add two more, which connect IO of a requested resource to a particular port of a particular internal component. Both the name and the port in this case are purely symbolic--they are not interpreted other than by the get_internal function. You can still specify attrs (for auxiliary data, the need for which will no doubt arise) and invert (many common buses/instances do have active-low signals).

def get_internal_signal(self, signal, name, port, attrs, invert):
def get_internal_pin(self, pin, name, port, attrs, invert):

There would be two behaviors added to the DSL.

First, there would be a new Internal resource part with this signature:

class Internal:
    def __init__(self, name, port, *, dir, width=1, invert=False):

When requested, this Internal resource part produces a bare Signal--i.e. not a Pin. This resource part is convenient for internal buses, such as in PS7 or hps_system. The only possible XDR (as specified through request) is 0 and the only possible directions are i, o, and io.

Second, the existing Pins resource part would be altered to recognize Pins("@NAME.port[W]") similarly to Internal("NAME", "port", width=W). It would be possible to pass only one @-prefixed name passed to Pins, and its format must be strictly followed, except that the [W] part may be omitted for 1-bit pins.

When requested, this @-prefixed Pins resource part produces a Pin object that has the usual width, xdr, etc. This resource part is convenient for external buses driven through instances, such as in USRMCLK. (USRMCLK would have dir="oe", for example.) This feature is critically important because without it, it would not be possible to use USRMCLK with e.g. SPIFlashResources because this resource factory accepts as arguments pin names, not resource parts, and because I/O cores expect a Pin object and not a Signal.

Like other similar methods, get_internal_{signal,pin} would return an elaboratable (or None), which would then be injected into the toplevel fragment in the same way as normal I/O buffers.

Examples

Using ECP5 SPI Flash would look like:

        *SPIFlashResources(0,
            cs="R2", clk="@USRMCLK.MCLK", cipo="V2", copi="W2", wp="Y2", hold="W1",
            attrs=Attrs(IO_TYPE="LVCMOS33")
        ),

Using Cyclone-V HPS might look like:

        Resource("rst", 0, InternalN("hps_system", "hps_fpga_reset_reset_n", dir="i")),
        Resource("clk50", 0, Internal("hps_system", "clk_50_clk", dir="i"), Clock(50e6)),

        Resource("ddr3", 0,
            Subsignal("rst",     PinsN("@hps_system.memory_mem_reset_n", dir="o")),
            Subsignal("clk",     DiffPairs("@hps_system.memory_mem_reset", 
                                           "@hps_system.memory_mem_reset_n", dir="o")),
            Subsignal("clk_en",  Pins("@hps_system.memory_mem_cke", dir="o")),
            Subsignal("cs",      PinsN("@hps_system.memory_mem_cs_n", dir="o")),
            Subsignal("we",      PinsN("@hps_system.memory_mem_we_n", dir="o")),
            Subsignal("ras",     PinsN("@hps_system.memory_mem_ras_n", dir="o")),
            Subsignal("cas",     PinsN("@hps_system.memory_mem_cas_n", dir="o")),

I think Cyclone-V HPS looks like it might require the use of Internal and @-prefixed Pins resource parts at the same time. (I find the DDR3 interface somewhat baffling.) If it does in fact require instantiating e.g. differential buffers to use clk, then this is a strong argument in favor of the @-prefixed system with indirection.

Benefits

This mechanism has many desirable properties:

  • It is similar to the existing DSL and I/O buffer machinery and does not introduce a lot of surprises.
  • It can be transparently used with resource factories like SPIFlashResources.
  • Both internal ports and internal signals have their direction and width inherently specified, which means that it is even possible to implement get_internal_{signal,pin} in a way that transparently creates Instances it knows nothing about. It's not a good idea to do so because parameters, attributes, and the identity of the instance cannot be transparently managed. However, this property will ensure that hooking up large buses on SoC devices would not require boilerplate.
  • It affords get_internal_{signal,pin} complete implementation freedom so that it can do whatever makes sense for the platform. For example, most internal components (like USRMCLK) will be singletons, but in principle, an internal component may be instantiated many times, e.g. if the component is actually a plain nMigen module. (We might want to provide get_internal_{signal,pin} with the context, i.e. the top-level resource that is being instantiated, so it can keep track of such instances; note that get_internal_{signal,pin} is called once per pin/port.)
  • Its behavior can be easily changed or extended in board classes simply by overriding get_internal_{signal,pin}.

In particular, this would satisfy:

  • @Ravenslofty's needs (connecting to hps_system on Cyclone-V);
  • @rroohhh's needs (connecting to PS7 on Zynq);
  • @smunaut's needs (instantiating a prebuilt USB core).

It would also let us get rid of the weird custom code that instantiates SB_HFOSC and similar oscillator macros. (Some work will be required to plumb the clock constraints through.)

Drawbacks

While this mechanism is fairly simple to implement, it may be hard to understand because of the extra overloading and indirection. This particularly applies to the @-prefixed Pins resource parts.

This mechanism adds two ways to do almost the same thing: Internal and @-prefixed Pins.

Alternatives

  • Ditch Internal. In this case, the bus of e.g. PS7 will have subsignals i, o, etc. The get_internal_pin method would also have to do extra work to make sure no one is trying to request a PS7 port with XDR>0.

@smunaut
Copy link
Contributor

smunaut commented Nov 25, 2020

How would passing arguments / config to the internal/hard block work ? Because those config would/could be "global" and not per-signal/pin.

@whitequark
Copy link
Member

How would passing arguments / config to the internal/hard block work ?

Depends. Some hard blocks are singletons, but it is actually (counterintuitively) better to instantiate them as many times as requested. E.g. suppose we are exposing SB_HFOSC. We can implement get_internal_pin in such a way that it returns a new SB_HFOSC instance each time you call it. This will obviously blow up during placement if you request two of them, but that's ok. The flipside is that there needs to be no singleton logic, and the SB_HFOSC instance can be configured entirely from attributes (or maybe from the Clock(...). This only really works for instances that are a "single pin".

For the rest I think it is appropriate to configure it out of band. E.g. your USB case. I imagine most designs will have 1 and only 1 USB gadget in them--so you can just add an attribute to the platform. But if you have more than one, then I can imagime some sort of method like platform.add_usb_gadget("USB0", vid=, pid=) which would be requested by a Resource that has a bunch of Internals referring to USB0.

@rroohhh
Copy link
Contributor

rroohhh commented Nov 25, 2020

One thing that the RFC doesn't mention (but might be obvious) is, how the @-prefixed pins interact with the conn argument?

With the connector system there is already one way of indirection, I was wondering if there is a way to maybe expand it to use it for internal connections aswell.

@whitequark
Copy link
Member

One thing that the RFC doesn't mention (but might be obvious) is, how the @-prefixed pins interact with the conn argument?

Excellent question. I have actually spent a good while thinking about it and haven't decided anything.

Chiefly, the issue is that a normal physical pin name always refers to exactly 1 pin, but the @-prefixed pin name does not (and will often not). This works fine in Pins itself (because that one is multibit), but it cannot work in connectors (because they are single bit).

The closest thing to a solution I see is a compromise where @-prefixed pins can be looked up through connectors but only if they have width 1.

@whitequark
Copy link
Member

This proposal, or my counter-proposal, would have to go through our new RFC process to be accepted. In general, the current system of defining pins should probably be scrapped entirely and replaced with something better, so there is little point in defining things on top of it.

@whitequark whitequark closed this as not planned Won't fix, can't repro, duplicate, stale Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

5 participants