Skip to content
This repository was archived by the owner on Nov 7, 2022. It is now read-only.

Define EL1 cache management registers #41

Merged
merged 5 commits into from
Jun 17, 2022

Conversation

vbe0201
Copy link
Contributor

@vbe0201 vbe0201 commented Jun 15, 2022

As per title, this adds definitions for the CSSELR_EL1, CLIDR_EL1, and CCSIDR_EL1 registers.

The CCSIDR_EL1 register changes its bit layout depending on the availability of a particular CPU feature. I tried to separate that into two distinct register types to reduce ambiguity. Let me know if a different approach should be preferred.

@andre-richter
Copy link
Member

The CCSIDR_EL1 register changes its bit layout depending on the availability of a particular CPU feature. I tried to separate that into two distinct register types to reduce ambiguity. Let me know if a different approach should be preferred.

Hmm yeah, I was wondering in the past already what best to do once we get a register that does that.

Do you think it is worth exploring to somehow dynamically dispatch this in the library? That is, have the two register variants defined privately (Does tock-registers allow overlapping fields? Then we might be able to save some code by having only one definition that can share common fields) and somehow implement the tock traits on a third struct that just dispatches to one or the other register definition depending on the feature being set?
Not sure if this is feasible, writing this from the phone and can't check the sources properly at the moment.

@vbe0201
Copy link
Contributor Author

vbe0201 commented Jun 15, 2022

Does tock-registers allow overlapping fields?

It does, so we could merge both bitfields together and have something like this then:

impl Reg {
    pub fn get_num_sets(&self) -> u64 {
        match () {
            #[cfg(target_arch = "aarch64")]
            () => match has_feature_ccidx() {
                true => self.read(CCSIDR_EL1::NumSetsWithCcidx),
                false => self.read(CCSIDR_EL1::NumSetsWithoutCcidx),
            },
            #[cfg(not(target_arch = "aarch64"))]
            () => unimplemented!(),
        }
    }

    // ...
}

We should still expose the bitfields for manual access though because often users find themselves writing code for one particular target where the availability of the feature is already known statically.

@andre-richter
Copy link
Member

I think that’s a better design 👍

Allows us to expose only one register, and the helper member functions will be useful to many users as well.

@vbe0201
Copy link
Contributor Author

vbe0201 commented Jun 15, 2022

I implemented the suggested strategy using the ID_AA64MMFR2_EL1 register. I didn't deem the static target_arch = "aarch64" checks necessary in the end because the TTBRx_ELx getters/setters don't do it either. 😅

@andre-richter
Copy link
Member

Thanks! I'll try to do remaining review and a new release the coming days when I get to it. Stay tuned 👍

@andre-richter
Copy link
Member

I didn't deem the static target_arch = "aarch64" checks necessary in the end because the TTBRx_ELx getters/setters don't do it either. 😅

I think that should be fine, since the code would eventually stop at the unimplemented!() in the R/W functions of the tock interface.

@andre-richter
Copy link
Member

Thanks!

Disclaimer: I only did a coarse-grained review due to volume of changes and lack of time. Should something be off, we'll do bugfix releases.

@andre-richter andre-richter merged commit c6f8932 into rust-embedded:master Jun 17, 2022
@andre-richter
Copy link
Member

Released as 7.4.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants