Custom write implementation for specific registers

[G33KatWork]

Hi,

I am currently writing a HAL for some ATtiny 1-series AVRs. These newer AVRs have a few registers that are deemed critical and are protected against erroneous writes. These registers are mostly clock configuration and flash-writing related.

To write to these registers, you need to write a special unlocking register with a magic value after which you have four instructions to write into the protected registers. After either the fourth instruction or the write to the register you need to unlock it again.

Doing this manually by issuing two consecutive writes using the generated pac isn't always working, because the compiler emits more than four instruction between the unlock and the actual register write. Sometimes it works, but as soon as the writes become more complex and you are maybe using struct variants it stops working.

This means you essentially have to perform the unlock and the write using inline assembly. There is no way around this to make this work reliably, I think. So I started hacking an extension trait in my HAL to add another method to a writable register that would perform the unlock and write the register by using an inline assembly block. The idea is to use the same Writer to turn all fields into bits and then instead of using the VolatileCell to write them to register, just pass them to an asm!-block. Right now the extension trait would be valid for all Writable registers. This needs to be restricted further to only applicable registers with a marker trait or something in the future, but I wanted to get it working first.

There is a problem though: The API that is generated from svd2rust is "too safe". I cannot instantiate a Writer and I also cannot access the bits field, because it's private. Even moving this code to the device crate that is generated by svd2rust doesn't help, because _reg in the writer is a private field.

This is what I came up with so far:

use core::marker::PhantomData;
use avr_device::generic::{Reg, W, Writable, Resettable};

pub trait ProtectedWritable<REG>
where
    REG: Writable
{
    fn write_protected<F>(&self, f: F)
    where
       F: FnOnce(&mut REG::Writer) -> &mut W<REG>;
}

impl<REG> ProtectedWritable<REG> for Reg<REG>
where
    REG: Writable + Resettable
{
    fn write_protected<F>(&self, f: F)
    where
        F: FnOnce(&mut REG::Writer) -> &mut W<REG>
    {
        unsafe {
            let ptr = self.as_ptr();
            let val = f(&mut REG::Writer::from(W {
               bits: REG::RESET_VALUE & !REG::ONE_TO_MODIFY_FIELDS_BITMAP
                   | REG::ZERO_TO_MODIFY_FIELDS_BITMAP,
               _reg: PhantomData,
            })).bits;

            let ccp_ptr = unsafe { &(*CPU::ptr()).ccp.as_ptr() };
            let unlock_magic = 0xD8u8;

            // insert asm! block for unlock and write here...
            // *ccp_ptr = unlock_magic
            // *ptr = val
        }

    }
}

The CPU.ccp register is the magic unlock register.

Any idea how I could implement something like this? My last resort is to patch svd2rust itself to generate this code somehow, but I wanted to avoid this until now.

edit & update:
After trying a lot more things, it boils down to the fact that this doesn't seem to be implementable outside of svd2rust, but then again, this feels like the right place to implement this.

I found #427 and #520 which deal with similar issues that I have and I decided to try and apply the same approach that was done with the atomic operations on MSP430 for the locked AVR registers. Adding the write_protected function becomes trivial, because it's the same crate, even the same module and all the private field issues are just gone and I can use the code I already wrote.

There is still a catch: Not all registers are write protected and not all registers are unlocked in the same way. There needs to be some marker trait that can be implemented for RegisterSpecs which mark these registers as protected and also adds a generic constant which contains the magic unlock value. The question is, who should implement that? The right place is the PAC, but neither the AVR ATDF, nor the SVD that is generated from the ATDF contain the information which registers are protected. I could still patch the generated SVD where necessary to add this information in hindsight using the YAML patches, but after checking out the SVD spec, there is no concept of a locked register that needs to be unlocked before a write can occur. There are protected registers, but that is related to ARM's Trustzone and I don't want to abuse this feature to mark protected registers.
That is a problem. I can't define the trait in the PAC and then implement the trait in the HAL for the appropriate register specs because of orphan rules. So, I guess I'll have to manually implement this marker trait for every register in every applicable controller in some additional file in the pac crate.

Any better ideas?

Also, now that I think of it, getting the pointer to the CCP register is going to be problematic as well. I can't take it for granted that it exists under the same name on all controllers and just statically use it like I do in my code snippet above. This would definitely needed to be passed to the protected_write function which would make it unsafe. Or I need to play with another trait that gets implemented only for the CCP register which defines it as a global unlock register for the configuration change protection on this controller which can then be passed into a protected_write function.