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.

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

nRF52 spi: fix get_rate #1799

Merged
merged 1 commit into from May 8, 2020
Merged

Conversation

lschuermann
Copy link
Member

@lschuermann lschuermann commented Apr 25, 2020

Pull Request Overview

The nRF52 SPI implementation's get_rate() method has been returning the actual register content instead of the converted SPI operating frequency.

This PR fixes this issue specifically. However, because the Frequency enum relates to two u32s, namely the register content and the actual frequency, using From<> traits are confusing which is likely how the bug was introduced originally. Those traits are replaced with methods indicating their purpose.

The back-conversion of the register content to a frequency may fail if the register contains an invalid value. As the reset value is valid and all other values written should be valid too, this should work. Nonetheless, a .expect() must be introduced.

Testing Strategy

Tested on nRF52DK.

    debug!(
        "Old rate: {:?}",
        hil::spi::SpiMaster::get_rate(&nrf52::spi::SPIM0)
    );
    debug!(
        "Set rate: {:?}",
        hil::spi::SpiMaster::set_rate(&nrf52::spi::SPIM0, 5_000_000)
    );
    debug!(
        "New rate: {:?}",
        hil::spi::SpiMaster::get_rate(&nrf52::spi::SPIM0)
    );

Output on latest master:

NRF52 HW INFO: Variant: AAE0, Part: N52832, Package: QF, Ram: K64, Flash: K512
Old rate: 67108864
Set rate: 4000000
New rate: 1073741824

Output with this PR:

NRF52 HW INFO: Variant: AAE0, Part: N52832, Package: QF, Ram: K64, Flash: K512
Old rate: 250000
Set rate: 4000000
New rate: 4000000

Verifying the actual SPI rate will need to wait for me to get my logic analyzer.

TODO or Help Wanted

  • The nRF52840 also supports 16 and 32 Mbps SPI rates. Those should probably be implemented using compile features on the chip crate. Can we do it in this PR as well, or should I open a new one?

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.

The nRF52 SPI implementation's get_rate() method has been returning
the actual register content instead of the converted SPI operating
frequency.
0x10000000 => Some(Frequency::M1),
0x20000000 => Some(Frequency::M2),
0x40000000 => Some(Frequency::M4),
0x80000000 => Some(Frequency::M8),
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Shouldn't this have more fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I've stated this in my todos. However the highest frequencies only apply to the nRF52840. I can fix that in this PR too, or create another one for that.

Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

I don't like adding a panic to the chip, but we haven't been consistently avoiding that so I won't block on this. If the hardware is broken then it seems like returning any value is fine, but 🤷

@bradjc bradjc added the last-call Final review period for a pull request. label May 7, 2020
@bradjc bradjc merged commit fdbb031 into tock:master May 8, 2020
@lschuermann lschuermann deleted the dev/nrf52-spi-rate-fix branch December 17, 2020 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call Final review period for a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants