Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

e320: Ensure consistent sequencing when powering on/off GPSDO #756

Closed
wants to merge 1 commit into from
Closed

e320: Ensure consistent sequencing when powering on/off GPSDO #756

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions mpm/python/usrp_mpm/periph_manager/e320_periphs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"""

import math
import time
from usrp_mpm.sys_utils.sysfs_gpio import SysFSGPIO, GPIOBank
from usrp_mpm.periph_manager.common import MboardRegsCommon

Expand Down Expand Up @@ -278,12 +279,23 @@ def enable_gps(self, enable):
self.log.trace("{} power to GPS".format(
"Enabling" if enable else "Disabling"
))
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@draeman-synoptic Actually, this flips the logic of INITSURV_N (bit 2). Before, it would be HIGH, now its LOW. Was that intentional?

Copy link
Contributor Author

@draeman-synoptic draeman-synoptic Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

with self.regs:
reg_val = self.peek32(self.MB_GPS_CTRL) & mask
reg_val = reg_val | (enable << self.MB_GPS_CTRL_PWR_EN)
self.log.trace("Writing MB_GPS_CTRL to 0x{:08X}".format(reg_val))
return self.poke32(self.MB_GPS_CTRL, reg_val)
cur_reg_val = self.peek32(self.MB_GPS_CTRL)
if enable and (cur_reg_val & enabled_mask != enabled_mask):
# First bring up supply, then bring out of reset
self.log.trace("Writing MB_GPS_CTRL to 0x{:08X}".format(pwr_en_mask))
self.poke32(self.MB_GPS_CTRL, pwr_en_mask)
time.sleep(0.001) # 1 ms

self.log.trace("Writing MB_GPS_CTRL to 0x{:08X}".format(enabled_mask))
return self.poke32(self.MB_GPS_CTRL, enabled_mask)
elif not enable:
# All controls pins low to avoid backfeeding I/O pins when unpowered
self.log.trace("Writing MB_GPS_CTRL to 0x{:08X}".format(0))
return self.poke32(self.MB_GPS_CTRL, 0)

def get_refclk_lock(self):
"""
Expand Down
Loading