Si5351C refactoring#1747
Conversation
martinling
force-pushed
the
si5351c-defs
branch
2 times, most recently
from
15ef8db to
9fa07ca
Compare
martinling
force-pushed
the
si5351c-defs
branch
5 times, most recently
from
d98a294 to
4d8e4ff
Compare
|
This has been tested on HackRF One and Praline in the HITL CI, and also verified locally by taking diffs of There is one remaining difference in my register dumps: Previously, we were writing 
The new implementation does this, because it reads the reserved bits during setup and retains them when updating the |
|
There is another difference in register dumps after this change, which happens specifically on Praline: This corresponds to This is because of what I assume is a bug in the /* CLK3: CLKOUT */
clkout_ctrl = SI5351C_CLK_INT_MODE | SI5351C_CLK_PLL_SRC(SI5351C_PLL_B) |
SI5351C_CLK_SRC(SI5351C_CLK_SRC_MULTISYNTH_SELF) |
SI5351C_CLK_IDRV(SI5351C_CLK_IDRV_8MA);...but never copied the new value of I have assumed that the intent was to use PLL B for CLKOUT on Praline, so I now have: /* CLK3: CLKOUT */
clkout.pll = SI5351C_PLL_B;
clk[3] = clkout; |
|
Something is still wrong with this on HackRF One r9; achieved sample rate appears to be around 14MHz (with XTAL) or 15MHz (with CLKIN) when it should be 10MHz. After startup and a Register 0 is the status register, and is indicating loss of lock on both PLLs, which would certainly explain the wrong sample rate, but I don't know why. Registers 19-21 should be irrelevant; these are the clock control registers for the CLK3-CLK5 outputs, which don't exist on the Si5351A, and are also flagged as powered down. Registers 222-223 are undocumented, and listed as reserved in AN619, so I've no idea what that change means. |
There are multiple similar but different things called a "source" or "clock source", or that are set in a register called "SRC", in the context of the Si5351 and its usage in HackRF. One place we can be unambiguous is that there are only two inputs to the Si5351C itself: XTAL and CLKIN. Let's have a type for that alone, and name it as such when we use it. There should be no functional changes in this commit.
We're going to want a type that just identifies one PLL, and is consistent with how the PLLs are numbered in register settings. The current si5351c_pll_t is a bitmask which allows referring to both PLLs. Let's make that a different type, and name things more clearly when using it.
|
Found the problem on r9 - just a dumb mistake in the first commit, when updating the logic controlling |
1 participant
This PR refactors the Si5351C driver code. The goal is to make it more maintainable and easier to add new features, by depending less on hand-written bit wrangling.
Although this PR is primarily an internal refactor, I've made a couple of changes to the external API used by
clock_gen.cin order to better name and distinguish different concepts and types. These can be made independently of the refactoring work so are placed first.Next, we add a new
si5351c_regs.deffile with register definitions and helper functions. This is similar to our otherdeffiles for other register-based parts, but with additional macros that simplify access to fields which are split across multiple registers, and for indexed access to fields which are duplicated for each multisynth, or for each PLL.Finally, the implementation in
si5351.cis rewritten to take advantage of the new helpers, while retaining the same structure and external API. As with other register-based parts, we now maintain a cached copy of the registers and aregs_dirtybitmask, and flush changes in asi5351c_regs_commit()function. The commit implementation will group writes to sequences of adjacent registers, so performance should match the previous hand-written code.