EttusResearch / uhd

The USRP™ Hardware Driver Repository
http://uhd.ettus.com
Other
942 stars 645 forks source link

Possible Typo for db_control Registers in RFNOC Radio Block #677

Closed johnwstanford closed 1 year ago

johnwstanford commented 1 year ago

https://github.com/EttusResearch/uhd/blob/6181693e87cc8c25733a9fd65570126b71241ae9/host/lib/usrp/x300/x300_radio_control.cpp#L91

When you look at fpga/usrp3/lib/control/db_control.v, it looks like SR_MISC_OUTS and SR_SPI are supposed to be right next to each other. However, I don't think that's what we're getting here. It doesn't seem like we want a multiplication, unless db_control somehow measures addresses in bits while they're measured in bytes everywhere else, which seems unlikely. I don't see PERIPH_REG_OFFSET anywhere in the HDL. It seems like we want to replace:

static constexpr uint32_t SR_MISC_OUTS = PERIPH_BASE + 160 * PERIPH_REG_OFFSET;

with just:

static constexpr uint32_t SR_MISC_OUTS = PERIPH_BASE + 160;

and so on for the rest of them.

mbr0wn commented 1 year ago

Oh yikes, are you saying none of our X310s have been working since UHD 4.0? :smile:

Just kidding. Also, thanks for your continued scrutiny of UHD.

The code is correct. The thing that you're missing is that db_control.v is part of "legacy" (pre-4.0) code that uses a settings bus. To drive a settings bus in UHD 4.0 RFNoC, we go through this module:

https://github.com/EttusResearch/uhddev/blob/8a6709588b2dd8fee5bcd33a851406e64ef6d609/fpga/usrp3/lib/rfnoc/utils/ctrlport_to_settings_bus.v#L26-L35

...which explains the multiply-by-8 thing.

johnwstanford commented 1 year ago

Haha, yeah I've got several X310s and they definitely work! The only way it would have made sense as a typo is if there was somehow an equal and opposite typo somewhere else (a divide-by-8). I was totally stumped but I'm not as fluent with Verilog yet. Thanks for the explanation! I love your devices!

mbr0wn commented 1 year ago

To be fair this is not obvious at all. The question was fair.