e320: Ensure consistent sequencing when powering on/off GPSDO #756
Conversation
When powering up the GPSDO, ensure the GPS_3V3 rail is up before taking the GPSDO out of reset. When powering down the GPSDO, ensure I/O signals are driven low to ensure GPSDO isn't backfed power via its I/O input pins.
There was a problem hiding this comment.
This is very nice, thanks @draeman-synoptic. We haven't seen any issues around this, but this is definitely safer.
| mask = 0xFFFFFFFF ^ (0b1 << self.MB_GPS_CTRL_PWR_EN) | ||
| pwr_en_mask = (0b1 << self.MB_GPS_CTRL_PWR_EN) | ||
| rstn_mask = (0b1 << self.MB_GPS_CTRL_RST_N) | ||
| enabled_mask = pwr_en_mask | rstn_mask |
There was a problem hiding this comment.
@draeman-synoptic Actually, this flips the logic of INITSURV_N (bit 2). Before, it would be HIGH, now its LOW. Was that intentional?
There was a problem hiding this comment.
@mbr0wn Thanks for the careful review! There is a difference in handling the INITSURV_N bit. Please let me know what you think about this:
The E320 bitstream initializes the GPS control register to 0x3 (gps_ctrl <= 32'h3), so INITSURV_N is low (asserted) upon booting. In the original logic quoted here, INITSURV_N is included in the mask value, so the current value of INITSURV_N is preserved. Nothing in the E320 MPM seems to manipulate that bit, and it was low upon boot, so it will just always be low.
The PR logic ignores the current value, so INITSURV_N is always forced low by the new logic.
I think there are two ways to consider what to do:
- When powering the GPS on, restore the boot-time value for INITSURV_N. Presumably the Jackson Labs part has lost state while power cycled, so if the system initiated survey mode upon boot, we should do the same upon GPS power cycle. This is the PR behavior, but it was done sloppy. The better approach would be to latch the INITSURV_N value when booted, and then use that original bit value in this on/off logic. That way it behaves consistently even if a future FPGA change defaults INITSURV_N to high.
- Alternatively, perhaps INITSURV_N should be preserved unchanged through the power cycle. This is what the prior logic would've done. This makes sense for a use case where the user application manually did something to set INITSURV_N high, overriding the default behavior (perhaps with a custom-patched MPM). This approach would avoid unintentional breakage.
I'm happy to amend this PR either way.
There was a problem hiding this comment.
You're right, I'm wrong: I thought we were setting INITSURV_N to high. But we don't, and this is no change in behaviour. I think a comment regarding this pin is in order, but I can add that.
When powering up the GPSDO, ensure the GPS_3V3 rail is up before taking the GPSDO out of reset. When powering down the GPS_3V3 rail, ensure I/O signals to GPSDO are driven low so it isn't backfed power through control pins.
Description
It's worth noting that this PR doesn't address a known problem, but rather is an improvement in behavior I noted when I was troubleshooting an unrelated GPSDO issue.
Which devices/areas does this affect?
Affects MPM control of GPS power state on USRP E320 radios.
Testing Done
Confirmed that E320 generates GPS fixes and data, confirmed that "e320_bist gpsdo" runs as expected on the radio, confirmed that GPSDO can be powered on and powered off as expected.
Checklist
MPM compat, noc_shell, specific RFNoC block, ...)