USB HUB release 2.0 - #17
Conversation
|
There was a problem hiding this comment.
🟡 Changes recommended
Stale build integration, ineffective descriptor writes, invalid defaults, and misrouted DMA skip interrupts are blocking correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the composite USB hub for release 2.0 with flip-flop descriptor storage, per-device configuration, and ULPI/timing fixes.
Changes:
- Replaces external Hub descriptor RAM with an internal writable descriptor table.
- Adds per-device RAM, endpoint, buffering, and DMA configuration.
- Updates ULPI receive handling, simulation timers, interfaces, and obsolete sources.
File summaries
| File | Description |
|---|---|
RTL/STRUCTURE/ip_xxx_3511_hs_mem_compound_structure.a.vhdl |
Integrates descriptor storage and per-device DMA routing. |
RTL/STRUCTURE/ip_xxx_3511_hs_mem_compound_structure_cfg.c.vhdl |
Updates component bindings. |
RTL/STRUCTURE/ip_xxx_3511_hs_mem_compound_structure_c090_cfg.c.vhdl |
Removes obsolete configuration. |
RTL/RTL/usb_upstreamled.m.vhdl |
Removes unused LED logic. |
RTL/RTL/usb_reg_if.m.vhdl |
Bounds skip-endpoint accesses. |
RTL/RTL/usb_pie.m.vhdl |
Adds ULPI and simulation-timer changes. |
RTL/RTL/usb_mux.m.vhdl |
Renames the generic USB mux. |
RTL/RTL/usb_ep0_hub_descr.m.vhdl |
Adds flip-flop Hub descriptor storage. |
RTL/RTL/usb_ep0_handler.m.vhdl |
Updates descriptor addressing and widths. |
RTL/RTL/usb_dma.m.vhdl |
Adds multi-device skip scanning. |
RTL/RTL/usb_clock_gate_rtl.m.vhdl |
Removes obsolete clock gating. |
RTL/RTL/usb_clock_divider.m.vhdl |
Removes obsolete clock division. |
RTL/RTL/usb_ahb_slave.m.vhdl |
Generalizes register address width. |
RTL/RTL/ahb_dma_slave.m.vhdl |
Restricts and updates RAM/AHB widths. |
RTL/INTERFACE/usb_fs_emb_dev_pkg.p.vhdl |
Removes legacy embedded-device definitions. |
RTL/INTERFACE/usb_ep_config_hub_pkg.p.vhdl |
Updates Hub endpoint and link layout. |
RTL/INTERFACE/ip_xxx_3511_hs_mem_compound.e.vhdl |
Exposes per-device generics and embedded Hub FIFO. |
RTL/INTERFACE/ip_xxx_3511_hs_mem_compound_cmp_pkg.p.vhdl |
Updates the component declaration. |
Review details
Suppressed comments (2)
src/usb_hub_composite_device/RTL/STRUCTURE/ip_xxx_3511_hs_mem_compound_structure.a.vhdl:1791
- The same skip-list routing problem affects Device 1: when
dma_clear_skipidentifies Device 1 butsync_pie_dev_selecteddoes not, the correspondingdma_set_intpulse never reaches this register interface. Select bydma_skipdev_selectedfor skip completions and by PIE selection for normal transfers.
dev1_dma_set_int <= dma_set_int when sync_pie_dev_selected = C_NBDEV + 1
else '0';
src/usb_hub_composite_device/RTL/RTL/usb_ep0_hub_descr.m.vhdl:399
- The 64-bit read is aligned down to an even word above, but this test compares the unaligned request against odd word indices 17 and 65. Descriptor transfers start at words 16 and 64, so the self-powered bit is not overlaid in the returned upper word. Compare the aligned upper-word address instead.
if (usb_self_powered = '1') and ((to_integer(unsigned(ep0_mem_addr)) = C_ADDR_SP1) or (to_integer(unsigned(ep0_mem_addr)) = C_ADDR_SP2)) then
- Files reviewed: 18/18 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| C_HUB_FIFO_SIZE : integer := 172; --Number of 32-bit words in HUB DESCRIPTOR FIFO. Maximum allowed value is 8096. | ||
| C_DEV0_RAM_ADDRWIDTH : integer := 15; | ||
| C_DEV1_RAM_ADDRWIDTH : integer := 15; | ||
| C_DEV0_NBPHYSEP : integer := 14; | ||
| C_DEV1_NBPHYSEP : integer := 12; |
| use usb_lib.usb_ep_config_pkg.all; | ||
|
|
||
| entity usb_ep0_hub_descr is | ||
| generic(C_NWORDS : integer := 128; --Number of 32 bits words |
| PROC_ROM : process(ep0_mem_req, ep0_mem_addr, usb_self_powered ) | ||
| variable var_mem_addr_integer : integer range 0 to C_NWORDS-1; | ||
| begin | ||
| if ep0_mem_req = '1' then --This is required to prevent range constraint violations (as ep0_mem_addr is also used for data transfer) | ||
| var_mem_addr_integer := to_integer(unsigned(ep0_mem_addr(log2(C_NWORDS)-1 downto 1)&'0')); | ||
| ep0_mem_rdata(31 downto 0) <= C_EP0_ROM(var_mem_addr_integer); | ||
| var_mem_addr_integer := to_integer(unsigned(ep0_mem_addr(log2(C_NWORDS)-1 downto 1)&'1')); | ||
| ep0_mem_rdata(63 downto 32) <= C_EP0_ROM(var_mem_addr_integer); |
| dev0_dma_set_int <= dma_set_int when sync_pie_dev_selected = C_NBDEV | ||
| else '0'; |
| for usb_ep0_hub_descr_1 : usb_ep0_hub_descr | ||
| use entity usb_lib.usb_ep0_hub_descr(rtl); | ||
| end for; | ||
| for usb_mux_1 : usb_mux | ||
| use entity usb_lib.usb_mux(rtl); |
| C_HUB_FIFO_SIZE : integer := 128; | ||
| C_DEV0_RAM_ADDRWIDTH : integer := 15; | ||
| C_DEV1_RAM_ADDRWIDTH : integer := 15; | ||
| C_DEV0_NBPHYSEP : integer := 14; | ||
| C_DEV1_NBPHYSEP : integer := 14; |
| C_EPUB : integer := 32; | ||
| C_DAUB : integer := 32; --Requirement : C_DAUB > C_DALB | ||
| C_DALB : integer := 17; --maximum allowed value is 17 | ||
| C_HUB_FIFO_SIZE : integer := 172; --Number of 32-bit words in HUB DESCRIPTOR FIFO. Maximum allowed value is 8096. |
There was a problem hiding this comment.
This comment states we support up to 8096 but I think there might be a problem. ep0_mem_addr is only 12 bits. and this connection selects 13 bits when C_HUB_FIFO_SIZE exceeds 4096.
Should we limit to 4096 or widen the bus to support 8096?
Replaced HUB RAM with flipflops
Fixed ULPI issue
Separate generics for each device