Si5351C refactoring - #1747
Conversation
15ef8db to
9fa07ca
Compare
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. |
|
Found the problem on r9 - just a dumb mistake in the first commit, when updating the logic controlling |
I did some testing and found that some Si5351Cs installed on H1s have those low bits set to 0 at start-up. I also found that changing those bits has no effect on clock frequency. |
mossmann
left a comment
mossmann
left a comment
There was a problem hiding this comment.
Great overall! I found one small bug.
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.
|
Hi @martinling and @mossmann, first of all, congratulations on your great work across the GSG projects and on the excellent documentation. In relation to this PR, I would like to share some investigation results from the Mayhem/PortaPack side, after observing a HackRF USB mode regression on one out of four HackRF devices I tested. Apologies for the long post, but I wanted to include the experimental details and hardware results. While investigating a HackRF USB mode regression affecting one particular HackRF One r6 + PortaPack H2 (WM8731), we eventually traced the issue back to the Si5351C changes introduced by this PR. The affected unit works correctly as a PortaPack, but in HackRF USB mode The investigation originally started as this Mayhem issue: portapack-mayhem/mayhem-firmware#3268 After extensive hardware testing and manual bisecting, we narrowed the on the affected device. The corresponding HackRF submodule revisions are: After isolating the HackRF changes in that interval, the regression was from this PR. More specifically, we were eventually able to isolate the physical Before this change, HackRF programmed REG183 deterministically as: The refactored driver first snapshots the Si5351 register map and uses During the investigation we noticed the earlier comment in this PR about
and, in particular, the observation that some Si5351Cs installed on H1s Our hardware testing has produced a different and reproducible result on Skyworks AN619 Rev. 0.8 specifies REG183 as: and states that D5:D0 should be written as: Therefore, with the existing 8 pF XTAL_CL setting: giving: This also matches what we observe in current HackRF firmware: However, on the affected r6/H2 device, REG183=0x92 reproducibly causes We isolated this experimentally while retaining the new register Historical isolated tests on the affected device: The 0x80 and 0x92 test candidates used the same helper/cache/dirty/commit We then repeated the experiment on current Mayhem next / current HackRF. Current upstream base: Current next is BAD on the affected device and reports: We then made a one-line diagnostic change in the current Si5351 driver: This keeps the current register cache/dirty/commit implementation intact No I2C timing, clock source, PLL, multisynth, startup sequence or other The corresponding current-next test revisions are: The firmware was built using the normal Mayhem Docker toolchain. SHA256 of the tested HackRF One firmware: SHA256 of the tested HackRF Pro firmware: On the affected device this immediately changes the result from: We then tested the same current-next 0x80 candidate across four different In all four cases the programmed value was also verified directly using: So the current-next validation result is 4/4 GOOD with deterministic For comparison, REG183=0x92 is not generally broken: it works correctly Therefore we are NOT suggesting that AN619 is generally wrong, nor that What we have found is at least one reproducible hardware case where: while USB communication and IQ streaming themselves remain operational. This particular affected HackRF One r6/H2 is an older third-party unit, For reference, here is a photo of the Si5351 device fitted on the affected HackRF One r6/H2 PCB. The package marking is visible in the photo. Given the earlier testing mentioned in this PR, we would be very Could there be a known Si5351C silicon/BOM/compatible-device variation, We can provide the individual test commits / firmware hashes and further |
|
One possible difference comes to mind: during the chip shortages a few years ago some third-party vendors decided to substitute in Si5351A instead of Si5351C on the boards they produced. Unfortunately there's no way to tell from the package marking which variant is fitted (the 'A' is for the chip revision not the model variant). However, since the Si5351A has no CLKIN support, they also added an extra footprint breaking the CLKIN PCB trace, so you could check to see if that is present. There are some pictures of that on issue #1099 . I do notice your board has the same marking & date code as the one in that issue, so I think it's pretty likely it's one of those boards. |
|
Thanks @miek, that's a very interesting observation. 
I would prefer not to reopen it at the moment because this old board has some damaged solder-mask areas that I have had to insulate carefully. Your observation about the identical package marking/date code is particularly interesting. This could indeed mean that the affected r6 board belongs to that batch of third-party boards with a possible Si5351A substitution. One additional data point is that my Device 3 is a HackRF One r9, therefore using the Si5351A design, and it works correctly with both REG183=0x92 and our 0x80 test. So Si5351A alone would not seem sufficient to explain the REG183 sensitivity, but perhaps the combination of the substituted device and this particular r6/third-party board design is relevant. 
From REG0, this gives:
I understand that Also, Device 3 (known Si5351A design) and Device 4 (Si5351C) both return exactly |
|
We don't know what the reserved bits of register 183 are. However, in general the Si5351 register map is very logical and related settings are placed together, so I suspect that those bits also relate to the crystal oscillator circuit somehow. They could be adjusting drive strength of the output for the driven side of the crystal, for instance. It would be interesting to probe pin 2 (XB) of the Si5351 on the troublesome board to see how the crystal waveform differs between the 0x80 and 0x92 settings. |
|
Thanks @martinling , that is a very interesting possibility. The idea that REG183 D5:D0 may control some undocumented analogue parameter of the oscillator block would fit the reproducible 0x80 / 0x92 behaviour we are seeing. I will try tomorrow to probe XB (pin 2) on the affected board and compare the waveform with REG183=0x80 and REG183=0x92. One detail worth mentioning is that Device 1 appears to use a passive 4-pad 25 MHz crystal, so I will first verify the exact XA/XB connection before interpreting the measurement. In any case, experimentally we have already established that this particular Device 1 cannot operate correctly with external SDR programs (sdr++, gqrx ,....) with REG183=0x92, while restoring the previous REG183=0x80 value makes it work correctly and reproducibly. Once we have the oscilloscope measurements, I would therefore appreciate considering whether some form of backward-compatibility detection could be appropriate for these older/special hardware variants, rather than changing the current behaviour for all devices. As a preliminary example only, one possibility might be to use the Si5351 REVID value: the affected device is the only one in my four-device sample reporting REVID=0, while all three unaffected devices report REVID=1. This could potentially allow something like 0x80 for REVID=0 devices and the current AN619 0x92 behaviour for REVID>=1. Of course, at this point we only have one REVID=0 device, so I would not assume that this is the correct detection criterion without further evidence. I mention it only as an example of the kind of backward-compatible solution that might be possible. |
|
It's worth noting that those REVID=0 parts are very old. Even this photo of a Jawbreaker board from 2012 already shows an Si5351 with the The Si5351 family appear to have been first marketed in late 2010, so it may be that REVID=0 are just the parts from the first year or two of production - but it's also possible that Rev. 0 was for pre-production samples, in which case the advice in AN619 might not even be intended to apply to them. |
|
Yeah, some of our users (including some of us maintainers) are having old devices. I don't see a nice way to tell it, but we are trying to keep compatibility even with old devices. |
|
I've opened PR #1811, which implements the proposal to zero the reserved bits of register 183 for rev 0 parts, whilst following the advice in AN619 otherwise. It's possible that the reason this helps is not related to the Si5351 revision, but rather to something about the crystal used on that specific clone board. However, even if that's the case, this could still be the right solution. Since the rev 0 chips predate the entire HackRF project, it's quite possible that this one batch of chip-shortage-era clones, with Si5351A and broken CLKIN, are the only HackRF boards that will ever exist with rev 0 chips on them. What's still not clear is whether 0x80 is the best choice of register value for those boards - perhaps the crystal has a different load capacitance, or perhaps zero is not the best value for the mystery bits. You could investigate that if you want to, but 0x80 seems to work, so if you're happy with that we can go with it. |
5 participants
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.