USB example, submodules.car, and no direct platform support

[YusufCelik]

Tang primer 20k has a working example of the usb3317 being used (https://github.com/sipeed/TangPrimer-20K-example/tree/main/USB).
This example has been generated via https://luna.readthedocs.io/en/latest/. Unfortunately the author did not include the original .py files that generated the verilog.
I have been trying to reconstruct for two weeks what the original file might have been, but no luck (since I do not want a serial device but a simple custom device for bulk transfers).
I generated a file that fails at the descriptor stage as far as I can tell from Windows (although the connection sound does play. In other words, the device does appear in the device manager.

I suspect there might be an issue with my customization (i.e., disabling submodules.car and platform.request and using a Record instead):

#!/usr/bin/env python3
#
# This file is part of LUNA.
#
# Copyright (c) 2020 Great Scott Gadgets <[email protected]>
# SPDX-License-Identifier: BSD-3-Clause

import os

from amaranth import Elaboratable, Module
from usb_protocol.emitters import DeviceDescriptorCollection

from luna import top_level_cli
from luna.gateware.platform import NullPin
from luna.gateware.usb.usb2.device import USBDevice

from amaranth import Elaboratable, Module
from amaranth.hdl.rec import Record, DIR_FANIN, DIR_FANOUT, DIR_NONE
from amaranth.back import verilog

from luna import top_level_cli


class USBDeviceExample(Elaboratable):
    """ Simple example of a USB device using the LUNA framework. """

    def __init__(self):
        self.ulpi = Record([
            ('data', [('i', 8, DIR_FANIN),
                      ('o', 8, DIR_FANOUT), ('oe', 1, DIR_FANOUT)]),
            ('clk', [('i', 8, DIR_FANIN)]),
            ('nxt', [('i', 8, DIR_FANIN)]),
            ('stp', [('o', 8, DIR_FANOUT)]),
            ('dir', [('i', 1, DIR_FANIN)]),
            ('rst', [('o', 8, DIR_FANOUT)])
        ])

    def create_descriptors(self):
        """ Create the descriptors we want to use for our device. """

        descriptors = DeviceDescriptorCollection()

        #
        # We'll add the major components of the descriptors we we want.
        # The collection we build here will be necessary to create a standard endpoint.
        #

        # We'll need a device descriptor...
        with descriptors.DeviceDescriptor() as d:
            d.idVendor = 0x16d0
            d.idProduct = 0xf3b

            d.iManufacturer = "LUNA"
            d.iProduct = "Test Device"
            d.iSerialNumber = "1234"

            d.bNumConfigurations = 1

        # ... and a description of the USB configuration we'll provide.
        with descriptors.ConfigurationDescriptor() as c:

            with c.InterfaceDescriptor() as i:
                i.bInterfaceNumber = 0

                with i.EndpointDescriptor() as e:
                    e.bEndpointAddress = 0x01
                    e.wMaxPacketSize = 64

                with i.EndpointDescriptor() as e:
                    e.bEndpointAddress = 0x81
                    e.wMaxPacketSize = 64

        return descriptors

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

        # Generate our domain clocks/resets.
        # m.submodules.car = platform.clock_domain_generator()

        # Create our USB device interface...
        # bus = platform.request(platform.default_usb_connection)
        m.submodules.usb = usb = USBDevice(bus=bus)

        # Add our standard control endpoint to the device.
        descriptors = self.create_descriptors()
        usb.add_standard_control_endpoint(descriptors)

        # Connect our device as a high speed device by default.
        m.d.comb += [
            usb.connect          .eq(1),
            usb.full_speed_only  .eq(1 if os.getenv('LUNA_FULL_ONLY') else 0),
        ]

        # ... and for now, attach our LEDs to our most recent control request.
        m.d.comb += [
            platform.request_optional(
                'led', 0, default=NullPin()).o  .eq(usb.tx_activity_led),
            platform.request_optional(
                'led', 1, default=NullPin()).o  .eq(usb.rx_activity_led),
            platform.request_optional(
                'led', 2, default=NullPin()).o  .eq(usb.suspended),
        ]

        return m

    def ports(self):
        return [
            self.ulpi.data.o,
            self.ulpi.data.oe,
            self.ulpi.data.i,
            self.ulpi.clk.i,
            self.ulpi.nxt.i,
            self.ulpi.stp.o,
            self.ulpi.dir.i,
            self.ulpi.rst.o
        ]


if __name__ == "__main__":
    top_level_cli(USBDeviceExample)

The .car submodule seems to be addressed(?) in the Gowin project as follows (I assume):

assign ulpi_rst  = 1'b1;
assign ulpi_data = ulpi_data_oe ? ulpi_data_o : 8'hz;

wire int_clk = ~ulpi_clk;

reg [4:0] rst_cnt;

always @(posedge int_clk or negedge rst_n)begin
    if(rst_n == 1'b0)begin
        rst_cnt <=5'd0;
    end else begin
        if (rst_cnt[4] == 1'b0) rst_cnt <= rst_cnt + 5'd1;
    end
end

What am I overlooking here? Did I do something counter to the Luna framework?

[miek]

One thing to note is that the connection noise on Windows can be a little misleading, all it takes is detecting a pull-up on one of the data lines to generate that noise & an entry in device manager. It doesn't necessarily mean that any actual communication is taking place. Since it fails in the descriptors, there probably isn't any communication happening.

Disabling the clock-and-reset (car) submodule is a problem, because that generates all the required clocks for everything to work. Unless you're creating those clocks yourself in some other code outside of what you've posted, the USB gateware won't be running. You can see an example of the implementation for ECP5 here:

luna/luna/gateware/architecture/car.py

Lines 192 to 380 in be05684

	class LunaECP5DomainGenerator(LunaDomainGenerator):
	""" ECP5 clock domain generator for LUNA. Assumes a 60MHz input clock. """
	# For debugging, we'll allow the ECP5's onboard clock to generate a 62MHz
	# oscillator signal. This won't work for USB, but it'll at least allow
	# running some basic self-tests. The clock is 310 MHz by default, so
	# dividing by 5 will yield 62MHz.
	OSCG_DIV = 5
	# Quick configuration selection
	DEFAULT_CLOCK_FREQUENCIES_MHZ = {
	"fast": 240,
	"sync": 120,
	"usb": 60
	}
	def __init__(self, *, clock_frequencies=None, clock_signal_name=None, clock_signal_frequency=None):
	"""
	Parameters:
	clock_frequencies -- A dictionary mapping 'fast', 'sync', and 'usb' to the clock
	frequencies for those domains, in MHz. Valid choices for each
	domain are 60, 120, and 240. If not provided, fast will be
	assumed to be 240, sync will assumed to be 120, and usb will
	be assumed to be a standard 60.
	"""
	super().__init__(clock_signal_name=clock_signal_name, clock_signal_frequency=clock_signal_frequency)
	self.clock_frequencies = clock_frequencies
	def create_submodules(self, m, platform):
	self._pll_lock = Signal()
	# Figure out our platform's clock frequencies -- grab the platform's
	# defaults, and then override any with our local, caller-provided copies.
	new_clock_frequencies = platform.DEFAULT_CLOCK_FREQUENCIES_MHZ.copy()
	if self.clock_frequencies:
	new_clock_frequencies.update(self.clock_frequencies)
	self.clock_frequencies = new_clock_frequencies
	# Use the provided clock name and frequency for our input; or the default clock
	# if no name was provided.
	clock_name = self.clock_name if self.clock_name else platform.default_clk
	clock_frequency = self.clock_frequency if self.clock_name else platform.default_clk_frequency
	# Create absolute-frequency copies of our PLL outputs.
	# We'll use the generate_ methods below to select which domains
	# apply to which components.
	self._clk_240MHz = Signal()
	self._clk_120MHz = Signal()
	self._clk_60MHz = Signal()
	self._clock_options = {
	60: self._clk_60MHz,
	120: self._clk_120MHz,
	240: self._clk_240MHz
	}
	pll_params = {}
	# Grab our input clock
	# For debugging: if our clock name is "OSCG", allow using the internal
	# oscillator. This is mostly useful for debugging.
	if clock_name == "OSCG":
	logging.warning("Using FPGA-internal oscillator for an approximately 62MHz.")
	logging.warning("USB communication won't work for f_OSC != 60MHz.")
	input_clock = Signal()
	m.submodules += Instance("OSCG", p_DIV=self.OSCG_DIV, o_OSC=input_clock)
	pll_params["CLKFB_DIV"] = 4
	else:
	input_clock = platform.request(clock_name).i
	divisor = 240e6 / clock_frequency
	if not divisor.is_integer():
	raise ValueError("Unsupported clock frequency {} MHz, must be an integer divisor of 240 MHz"
	.format(clock_frequency/1e6))
	pll_params["CLKFB_DIV"] = int(divisor)
	# Instantiate the ECP5 PLL.
	# These constants generated by Clarity Designer; which will
	# ideally be replaced by an open-source component.
	# (see https://github.com/SymbiFlow/prjtrellis/issues/34.)
	m.submodules.pll = Instance("EHXPLLL",
	# Clock in.
	i_CLKI=input_clock,
	# Generated clock outputs.
	o_CLKOP=self._clk_240MHz,
	o_CLKOS=self._clk_120MHz,
	o_CLKOS2=self._clk_60MHz,
	# Status.
	o_LOCK=self._pll_lock,
	# PLL parameters...
	p_PLLRST_ENA="DISABLED",
	p_INTFB_WAKE="DISABLED",
	p_STDBY_ENABLE="DISABLED",
	p_DPHASE_SOURCE="DISABLED",
	p_CLKOS3_FPHASE=0,
	p_CLKOS3_CPHASE=0,
	p_CLKOS2_FPHASE=0,
	p_CLKOS2_CPHASE=7,
	p_CLKOS_FPHASE=0,
	p_CLKOS_CPHASE=3,
	p_CLKOP_FPHASE=0,
	p_CLKOP_CPHASE=1,
	p_PLL_LOCK_MODE=0,
	p_CLKOS_TRIM_DELAY="0",
	p_CLKOS_TRIM_POL="FALLING",
	p_CLKOP_TRIM_DELAY="0",
	p_CLKOP_TRIM_POL="FALLING",
	p_OUTDIVIDER_MUXD="DIVD",
	p_CLKOS3_ENABLE="DISABLED",
	p_OUTDIVIDER_MUXC="DIVC",
	p_CLKOS2_ENABLE="ENABLED",
	p_OUTDIVIDER_MUXB="DIVB",
	p_CLKOS_ENABLE="ENABLED",
	p_OUTDIVIDER_MUXA="DIVA",
	p_CLKOP_ENABLE="ENABLED",
	p_CLKOS3_DIV=1,
	p_CLKOS2_DIV=8,
	p_CLKOS_DIV=4,
	p_CLKOP_DIV=2,
	p_CLKFB_DIV=pll_params["CLKFB_DIV"],
	p_CLKI_DIV=1,
	p_FEEDBK_PATH="CLKOP",
	# Internal feedback.
	i_CLKFB=self._clk_240MHz,
	# Control signals.
	i_RST=0,
	i_PHASESEL0=0,
	i_PHASESEL1=0,
	i_PHASEDIR=0,
	i_PHASESTEP=0,
	i_PHASELOADREG=0,
	i_STDBY=0,
	i_PLLWAKESYNC=0,
	# Output Enables.
	i_ENCLKOP=0,
	i_ENCLKOS=0,
	i_ENCLKOS2=0,
	i_ENCLKOS3=0,
	# Synthesis attributes.
	a_FREQUENCY_PIN_CLKI="60.000000",
	a_FREQUENCY_PIN_CLKOS2="60.000000",
	a_FREQUENCY_PIN_CLKOS="120.000000",
	a_FREQUENCY_PIN_CLKOP="240.000000",
	a_ICP_CURRENT="9",
	a_LPF_RESISTOR="8"
	)
	# Set up our global resets so the system is kept fully in reset until
	# our core PLL is fully stable. This prevents us from internally clock
	# glitching ourselves before our PLL is locked. :)
	m.d.comb += [
	ResetSignal("sync").eq(~self._pll_lock),
	ResetSignal("fast").eq(~self._pll_lock),
	]
	def generate_usb_clock(self, m, platform):
	return self._clock_options[self.clock_frequencies['usb']]
	def generate_sync_clock(self, m, platform):
	return self._clock_options[self.clock_frequencies['sync']]
	def generate_fast_clock(self, m, platform):
	return self._clock_options[self.clock_frequencies['fast']]
	def stretch_sync_strobe_to_usb(self, m, strobe, output=None, allow_delay=False):
	"""
	Helper that stretches a strobe from the `sync` domain to communicate with the `usn` domain.
	Works for any chosen frequency in which f(usb) < f(sync).
	"""
	# TODO: replace with Amaranth's pulsesynchronizer?
	to_cycles = self.clock_frequencies['sync'] // self.clock_frequencies['usb']
	return stretch_strobe_signal(m, strobe, output=output, to_cycles=to_cycles, allow_delay=allow_delay)

Usually the way to support a new board is to create a platform class that defines details of the FPGA, how to setup the clocks, and all the relevant resources/pins available. Some examples are here: https://github.com/greatscottgadgets/cynthion/tree/main/cynthion/python/src/gateware/platform and here: https://github.com/greatscottgadgets/luna-boards/tree/main/luna_boards

[YusufCelik]

Dear @miek, thanks for responding--appreciate it! From what I can tell, the working example verilog (generated by Luna), is based on acm_serial.py. I cannot find any references in the verilog to clocks other than the ulpi_clk (60hz). Neither do I find a reference to "car" anywhere. See screenshot, my generation of acm_serial.py is a carbon copy of the working example verilog (except for some dynamic naming changes). I copy the working project and only overwrite my version of the verilog from acm_serial.py. Even the modules hierarchy is exactly the same! Despite that, it just does not work...
The only clock/rst related code is the one I shared above.
What am I missing here? An older luna version? Perhaps car is initialized in the example verilog, but obfuscated? In other words, does the working verilog still call for some clocks or domains I am not aware of? For a simple device to enumerate, should a ulpi clock signal not suffice? Why the need for 120mhz and 240mhz?

[miek]

As well as the ulpi_clock, you also need to provide the usb_clock signal which is a 120 MHz clock. This is where most of the logic runs and its just down to how it's designed that it needs a faster clock to keep up with running the ULPI bus. The fast (240 MHz) clock should only be required for HyperRAM, so you shouldn't need that.

One other thing is that you may be hitting #280, so it's worth trying an older version before that issue was introduced.

[YusufCelik]

As well as the ulpi_clock, you also need to provide the usb_clock signal which is a 120 MHz clock. This is where most of the logic runs and its just down to how it's designed that it needs a faster clock to keep up with running the ULPI bus. The fast (240 MHz) clock should only be required for HyperRAM, so you shouldn't need that.

One other thing is that you may be hitting #280, so it's worth trying an older version before that issue was introduced.

Dear Mike (@miek), thanks for responding. It was quite frustrating, but once or twice I had the device actually enumerate as 'Test Device' (basically based on the usb example). (IThe problem cannot be with the actual fpga or usb3317 chip, one would assume, because the pre-generated Gowin verilog example I mentioned earlier enumerates consistently; what does not work is my code). Going back to an earlier commit (the one to which you alluded), did not really help in this case.

Interestingly, coming back to your point on the ulpi and usb clocks. Obviously I can feed in a 120 MHz clock. However, I think it was in the usb translator module, there is an assign usb_clk = ulpi_clock. Hence, that creates two interesting problems. First of all, I cannot drive the usb_clk in the top module via a 120mhz_clk_input. Moreover, the ulpi clock is 60hz.

On a final note, per your advice I looked at the luna boards and I made my own board file. I played around with a variety of board examples, but this one I find the most noteworthy: https://github.com/greatscottgadgets/luna-boards/blob/main/luna_boards/openvizsla.py
It, seems, to completely rely on only the 60hz ulpi clock:

            ClockSignal("sync")  .eq(ClockSignal("usb")),
            ClockSignal("fast")  .eq(ClockSignal("usb"))
        ]

It's clock domain generator is quite simple (no special instance being used, no plls being called, and so forth). If that is all it takes...one would assume I could easily replicate it and make my code finally work. (Alas it did not though).