Skip to content

Commit

Permalink
Merge #1973
Browse files Browse the repository at this point in the history 
1973: Remove reborrows? r=phil-levis a=ppannuto

### Pull Request Overview

We use a re-borrow idiom everywhere we access registers. Once upon a lifetime, when MMIO looked very different, I think this was necessary as we changed types to something that let us access the hardware. After the introduction of `StaticRef`, however, I don't think we actually need this anymore and we can simplify all of the register accesses.

There ~300 more instances of this `&*` register pattern in the kernel, so I wanted to take a temperature check on this before doing more of them.

### Testing Strategy

Compiling.

### TODO or Help Wanted

Is there are a reason to keep all of these `&*` around?

### Documentation Updated

- [ ] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [ ] Ran `make prepush`.


Co-authored-by: Pat Pannuto <pat.pannuto@gmail.com>
  • Loading branch information
@bors @ppannuto
bors[bot] and ppannuto committed Sep 20, 2020
2 parents cb748be + 06e0d69 commit 024738f
Show file tree
Hide file tree
Showing 28 changed files with 590 additions and 728 deletions.
18 changes: 7 additions & 11 deletions arch/cortex-m3/src/mpu.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,27 +364,25 @@ impl kernel::mpu::MPU for MPU {
type MpuConfig = CortexMConfig;

fn clear_mpu(&self) {
let regs = &*self.registers;
regs.ctrl.write(Control::ENABLE::CLEAR);
self.registers.ctrl.write(Control::ENABLE::CLEAR);
}

fn enable_app_mpu(&self) {
let regs = &*self.registers;

// Enable the MPU, disable it during HardFault/NMI handlers, and allow
// privileged code access to all unprotected memory.
regs.ctrl
self.registers
.ctrl
.write(Control::ENABLE::SET + Control::HFNMIENA::CLEAR + Control::PRIVDEFENA::SET);
}

fn disable_app_mpu(&self) {
// The MPU is not enabled for privileged mode, so we don't have to do
// anything
self.registers.ctrl.write(Control::ENABLE::CLEAR);
}

fn number_total_regions(&self) -> usize {
let regs = &*self.registers;
regs.mpu_type.read(Type::DREGION) as usize
self.registers.mpu_type.read(Type::DREGION) as usize
}

fn allocate_region(
Expand Down Expand Up @@ -685,12 +683,10 @@ impl kernel::mpu::MPU for MPU {
// If the hardware is already configured for this app and the app's MPU
// configuration has not changed, then skip the hardware update.
if !self.hardware_is_configured_for.contains(app_id) || config.is_dirty.get() {
let regs = &*self.registers;

// Set MPU regions
for region in config.regions.iter() {
regs.rbar.write(region.base_address());
regs.rasr.write(region.attributes());
self.registers.rbar.write(region.base_address());
self.registers.rasr.write(region.attributes());
}
self.hardware_is_configured_for.set(*app_id);
config.is_dirty.set(false);
Expand Down
90 changes: 40 additions & 50 deletions chips/apollo3/src/ble.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,85 +281,79 @@ impl<'a> Ble<'a> {
}

pub fn setup_clocks(&self) {
let regs = self.registers;

regs.clkcfg.write(CLKCFG::CLK32KEN::SET);
regs.bledbg.write(BLEDBG::DBGDATA.val(1 << 14));
self.registers.clkcfg.write(CLKCFG::CLK32KEN::SET);
self.registers.bledbg.write(BLEDBG::DBGDATA.val(1 << 14));
}

pub fn power_up(&self) {
let regs = self.registers;

regs.blecfg.write(BLECFG::PWRSMEN::SET);
self.registers.blecfg.write(BLECFG::PWRSMEN::SET);

while regs.bstatus.read(BSTATUS::PWRST) != 3 {}
while self.registers.bstatus.read(BSTATUS::PWRST) != 3 {}
}

pub fn ble_initialise(&self) {
let regs = self.registers;

// Configure the SPI
regs.mspicfg.write(
self.registers.mspicfg.write(
MSPICFG::SPOL::SET
+ MSPICFG::SPHA::SET
+ MSPICFG::RDFC::CLEAR
+ MSPICFG::WTFC::CLEAR
+ MSPICFG::WTFCPOL::SET,
);
regs.fifothr
self.registers
.fifothr
.write(FIFOTHR::FIFOWTHR.val(16) + FIFOTHR::FIFORTHR.val(16));
regs.fifoctrl.modify(FIFOCTRL::POPWR::SET);
self.registers.fifoctrl.modify(FIFOCTRL::POPWR::SET);

// Clock config
regs.clkcfg
self.registers
.clkcfg
.write(CLKCFG::FSEL.val(0x04) + CLKCFG::IOCLKEN::SET + CLKCFG::CLK32KEN::SET);

// Disable command queue
regs.cqcfg.modify(CQCFG::CQEN::CLEAR);
self.registers.cqcfg.modify(CQCFG::CQEN::CLEAR);

// TODO: Apply the BLE patch
}

fn reset_fifo(&self) {
let regs = self.registers;

regs.fifoctrl.modify(FIFOCTRL::FIFORSTN::CLEAR);
regs.fifoctrl.modify(FIFOCTRL::FIFORSTN::SET);
self.registers.fifoctrl.modify(FIFOCTRL::FIFORSTN::CLEAR);
self.registers.fifoctrl.modify(FIFOCTRL::FIFORSTN::SET);
}

fn send_data(&self) {
let regs = &*self.registers;

// Set the FIFO levels
regs.fifothr
self.registers
.fifothr
.write(FIFOTHR::FIFORTHR.val(16) + FIFOTHR::FIFOWTHR.val(16));

// Disable the FIFO
//regs.fifothr.write(FIFOTHR::FIFORTHR::CLEAR + FIFOTHR::FIFOWTHR::CLEAR);
//self.registers.fifothr.write(FIFOTHR::FIFORTHR::CLEAR + FIFOTHR::FIFOWTHR::CLEAR);

// Setup the DMA
unsafe {
regs.dmatargaddr.set(PAYLOAD.as_ptr() as u32);
self.registers.dmatargaddr.set(PAYLOAD.as_ptr() as u32);
}
regs.dmatocount.set(self.write_len.get() as u32);
regs.dmatrigen.write(DMATRIGEN::DTHREN::SET);
regs.dmacfg
self.registers.dmatocount.set(self.write_len.get() as u32);
self.registers.dmatrigen.write(DMATRIGEN::DTHREN::SET);
self.registers
.dmacfg
.write(DMACFG::DMADIR.val(1) + DMACFG::DMAPRI.val(1));

// Setup the operation
regs.cmd
self.registers
.cmd
.modify(CMD::TSIZE.val(self.write_len.get() as u32) + CMD::CMD::WRITE);

// Enable DMA
regs.dmacfg.modify(DMACFG::DMAEN::SET);
self.registers.dmacfg.modify(DMACFG::DMAEN::SET);

// Set the wake low
regs.blecfg.modify(BLECFG::WAKEUPCTL::OFF);
self.registers.blecfg.modify(BLECFG::WAKEUPCTL::OFF);
}

pub fn handle_interrupt(&self) {
let regs = self.registers;
let irqs = regs.intstat.extract();
let irqs = self.registers.intstat.extract();

// Disable and clear interrupts
self.disable_interrupts();
Expand All @@ -368,11 +362,11 @@ impl<'a> Ble<'a> {
// Enable interrupts
self.enable_interrupts();

if regs.bstatus.is_set(BSTATUS::BLEIRQ) {
if self.registers.bstatus.is_set(BSTATUS::BLEIRQ) {
panic!("Read requested while trying to write");
}

if !regs.bstatus.is_set(BSTATUS::SPISTATUS) {
if !self.registers.bstatus.is_set(BSTATUS::SPISTATUS) {
panic!("SPI not ready");
}

Expand All @@ -385,10 +379,10 @@ impl<'a> Ble<'a> {

if irqs.is_set(INT::DCMP) {
// Disable and clear DMA
regs.dmacfg.set(0x00000000);
self.registers.dmacfg.set(0x00000000);

// Disable the wake controller
regs.blecfg.modify(BLECFG::WAKEUPCTL::OFF);
self.registers.blecfg.modify(BLECFG::WAKEUPCTL::OFF);

// Reset FIFOs
self.reset_fifo();
Expand All @@ -404,13 +398,15 @@ impl<'a> Ble<'a> {

if irqs.is_set(INT::BLECIRQ) {
self.rx_client.map(|client| {
regs.cmd.modify(CMD::TSIZE.val(0) + CMD::CMD::READ);
self.registers
.cmd
.modify(CMD::TSIZE.val(0) + CMD::CMD::READ);

unsafe {
let mut i = 0;

while regs.fifoptr.read(FIFOPTR::FIFO1SIZ) > 0 && i < 40 {
let temp = regs.fifopop.get().to_ne_bytes();
while self.registers.fifoptr.read(FIFOPTR::FIFO1SIZ) > 0 && i < 40 {
let temp = self.registers.fifopop.get().to_ne_bytes();

PAYLOAD[i + 0] = temp[0];
PAYLOAD[i + 1] = temp[1];
Expand All @@ -427,16 +423,12 @@ impl<'a> Ble<'a> {
}

pub fn enable_interrupts(&self) {
let regs = &*self.registers;

regs.inten.set(0x18381);
self.registers.inten.set(0x18381);
}

pub fn disable_interrupts(&self) {
let regs = &*self.registers;

regs.intclr.set(0xFFFF_FFFF);
regs.inten.set(0x00);
self.registers.intclr.set(0xFFFF_FFFF);
self.registers.inten.set(0x00);
}

fn replace_radio_buffer(&self, buf: &'static mut [u8]) -> &'static mut [u8] {
Expand All @@ -452,8 +444,6 @@ impl<'a> Ble<'a> {

impl<'a> ble_advertising::BleAdvertisementDriver<'a> for Ble<'a> {
fn transmit_advertisement(&self, buf: &'static mut [u8], len: usize, _channel: RadioChannel) {
let regs = &*self.registers;

let res = self.replace_radio_buffer(buf);

// Setup all of the buffers
Expand All @@ -466,10 +456,10 @@ impl<'a> ble_advertising::BleAdvertisementDriver<'a> for Ble<'a> {
self.enable_interrupts();

// Wakeup BLE
regs.blecfg.modify(BLECFG::WAKEUPCTL::ON);
self.registers.blecfg.modify(BLECFG::WAKEUPCTL::ON);

// See if we can send the data
if regs.bstatus.is_set(BSTATUS::SPISTATUS) {
if self.registers.bstatus.is_set(BSTATUS::SPISTATUS) {
self.send_data();
}
}
Expand Down
48 changes: 23 additions & 25 deletions chips/nrf52/src/acomp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,52 +228,53 @@ impl<'a> Comparator<'a> {
/// Uses differential mode, with no hysteresis, and normal speed and power
/// VIN+ = AIN5 and VIN- = AIN0
fn enable(&self) {
let regs = &*self.registers;
// Checks if it's already enabled
// Assumes no one else is writing to comp regs directly
if regs.enable.matches_any(Enable::ENABLE::Enabled) {
// Assumes no one else is writing to comp registers directly
if self.registers.enable.matches_any(Enable::ENABLE::Enabled) {
return;
}

// Set mode to Differential
// Differential and single ended are pretty much the same,
// except single-ended gives more options for input and
// uses a ref ladder for hysteresis instead of a set voltage
regs.mode
self.registers
.mode
.write(Mode::OperatingMode::Differential + Mode::SpeedAndPower::Normal);
// VIN+ = Pin 0
regs.psel.write(PinSelect::PinSelect::AnalogInput5);
self.registers
.psel
.write(PinSelect::PinSelect::AnalogInput5);
// VIN- = Pin 1
regs.extrefsel
self.registers
.extrefsel
.write(ExternalRefSelect::ExternalRefSelect::AnalogRef0);
// Disable hysteresis
regs.hyst.write(Hysteresis::Hysteresis::CLEAR);
self.registers.hyst.write(Hysteresis::Hysteresis::CLEAR);

regs.enable.write(Enable::ENABLE::Enabled);
self.registers.enable.write(Enable::ENABLE::Enabled);
// start comparator
regs.events_ready.set(0);
regs.tasks_start.set(1);
self.registers.events_ready.set(0);
self.registers.tasks_start.set(1);
// wait for comparator to be ready
// delay is on order of 3 microseconds so spin wait is OK
while regs.events_ready.get() == 0 {}
while self.registers.events_ready.get() == 0 {}
}

fn disable(&self) {
let regs = &*self.registers;
// stop comparator
regs.tasks_stop.set(1);
self.registers.tasks_stop.set(1);
// completely turn comparator off
regs.enable.write(Enable::ENABLE::Disabled);
self.registers.enable.write(Enable::ENABLE::Disabled);
}

/// Handles upward crossing events (when VIN+ becomes greater than VIN-)
pub fn handle_interrupt(&self) {
// HIL only cares about upward crossing interrupts
let regs = &*self.registers;
// VIN+ crossed VIN-
if regs.events_up.get() == 1 {
if self.registers.events_up.get() == 1 {
// Clear event
regs.events_up.set(0);
self.registers.events_up.set(0);
self.client.map(|client| {
// Only one channel (0)
client.fired(0);
Expand All @@ -288,22 +289,20 @@ impl<'a> analog_comparator::AnalogComparator<'a> for Comparator<'a> {
/// Starts comparison on only channel
/// This enables comparator and interrupts
fn start_comparing(&self, _: &Self::Channel) -> ReturnCode {
let regs = &*self.registers;
self.enable();

// Enable only up interrupt (If VIN+ crosses VIN-)
regs.inten.write(InterruptEnable::UP::SET);
self.registers.inten.write(InterruptEnable::UP::SET);

ReturnCode::SUCCESS
}

/// Stops comparing and disables comparator
fn stop_comparing(&self, _: &Self::Channel) -> ReturnCode {
let regs = &*self.registers;
// Disables interrupts
regs.inten.set(0);
self.registers.inten.set(0);
// Stops comparison
regs.tasks_stop.set(1);
self.registers.tasks_stop.set(1);

self.disable();
ReturnCode::SUCCESS
Expand All @@ -313,14 +312,13 @@ impl<'a> analog_comparator::AnalogComparator<'a> for Comparator<'a> {
/// Returns true if vin+ > vin-
/// Enables comparator if not enabled, to disable call stop comparing
fn comparison(&self, _: &Self::Channel) -> bool {
let regs = &*self.registers;
self.enable();

// Signals to update Result register
regs.tasks_sample.set(1);
self.registers.tasks_sample.set(1);

// Returns 1 (true) if vin+ > vin-
regs.result.get() == 1
self.registers.result.get() == 1
}

fn set_client(&self, client: &'a dyn analog_comparator::Client) {
Expand Down

0 comments on commit 024738f

Please sign in to comment.