8000 Fix the bus library to support all the possible address widths by inesmaria08 · Pull Request #4099 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix the bus library to support all the possible address widths #4099

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
Merged
merged 4 commits into from
Oct 13, 2024
Merged

Fix the bus library to support all the possible address widths #4099

merged 4 commits into from
Oct 13, 2024

Conversation

inesmaria08
Copy link
Contributor
@inesmaria08 inesmaria08 commented Jul 12, 2024 •
edited by alevy
Loading

Pull Request Overview

This pull request fixes the bus library to support all the possible address types. Before this change, the bus library:

  • would error if the address with was larger than 16 bits
  • would possible not be able to store 64 bit addresses as the value of the address was encoded into a usize that may be only 32 bits wide (this is the case for all the boards now)
  • the bus8080 library would accept usize address types even though it only supports 16 bit addresses

Testing Strategy

This pull request was tested on STM32F4 Discovery board and on Raspberry Pi Pico (connected to Pico Explorer Base).

TODO or Help Wanted

This pull request depends on #4098.

Documentation Updated

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

Formatting

  • Ran make prepush.

@github-actions github-actions bot added HIL This affects a Tock HIL interface. stm32 Change pertains to the stm32 family of MCUSs labels Jul 12, 2024
@bradjc
Copy link
Contributor
bradjc commented Jul 12, 2024

Do buses need to change width dynamically, or should we template pub trait Bus on the width?

@alexandruradovici
Copy link
Contributor

Addresses should always be fixed. I thought about templating it, but I am not sure about the benefit of it. The only thing that changes is the address with and endianness. Do you have any template idea?

lschuermann
lschuermann reviewed Jul 12, 2024
View reviewed changes
@bradjc
Copy link
Contributor
bradjc commented Jul 12, 2024

Using u64 on a 32 bit platform always seems like a big hammer to me (particularly for a 16 bit bus). If it is easy to use the appropriate type then I think we should. If not, then it's not a huge deal.

@inesmaria08
Copy link
Contributor Author

Using u64 on a 32 bit platform always seems like a big hammer to me (particularly for a 16 bit bus). If it is easy to use the appropriate type then I think we should. If not, then it's not a huge deal.

I tested both versions (the previous one that is now in master and this one) and there seems to be no code size impact on ARM architecture. The call stack size might be larger though due to the u64 argument,.

bradjc
bradjc reviewed Jul 17, 2024
View reviewed changes
@alexandruradovici
Copy link
Contributor

Is there anything else that we need for this PR?

@alevy
Copy link
Member
alevy commented Aug 30, 2024

We need approvals. @alexandruradovici maybe can start with you?

@lschuermann
Copy link
Member

@alevy I don't think this has addressed the concerns from @bradjc and myself in this thread yet: #4099 (comment)

@alevy
Copy link
Member
alevy commented Aug 30, 2024

See #4099 (comment) for current feedback state

@inesmaria08 inesmaria08 force-pushed the support_for_up_to_64bit_buffer branch from 6a611e5 to fac8199 Compare September 6, 2024 15:25
lschuermann
lschuermann requested changes Sep 8, 2024
View reviewed changes
Copy link
Member
@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

I think this is good to go modulo the mismatch between documentation and implementation. I think you should copy (and adjust) that large document over DataWidth into the HIL and explain why the various widths & endianness selectors are required.

alexandruradovici
alexandruradovici requested changes Sep 9, 2024
View reviewed changes
alexandruradovici
alexandruradovici previously approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor
@alexandruradovici alexandruradovici left a comment

Choose a reason for hiding this comment

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

LGTM

bradjc
bradjc reviewed Sep 10, 2024
View reviewed changes
@inesmaria08
Copy link
Contributor Author

I would like to move this forward. Is there anything you’d like me to adjust, or is it ready to go?
@lschuermann @bradjc

bradjc
bradjc reviewed Oct 2, 2024
View reviewed changes
@alexandruradovici alexandruradovici mentioned this pull request Oct 9, 2024
Merged
2 tasks
@inesmaria08 inesmaria08 force-pushed the support_for_up_to_64bit_buffer branch from 1a057ff to 80b5e11 Compare October 10, 2024 13:30
@inesmaria08
Copy link
Contributor Author

@bradjc @lschuermann Is there anything else that needs to be added, or is this ready to be merged?

alexandruradovici
alexandruradovici reviewed Oct 10, 2024
View reviewed changes
bradjc
bradjc previously approved these changes Oct 10, 2024
View reviewed changes
@inesmaria08 @bradjc
Update kernel/src/hil/bus8080.rs
9ac4749 
Co-authored-by: Brad Campbell <bradjc5@gmail.com>
@lschuermann lschuermann added the last-call Final review period for a pull request. label Oct 12, 2024
@lschuermann lschuermann added this pull request to the merge queue Oct 13, 2024
Merged via the queue into tock:master with commit f30976d Oct 13, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradjc bradjc bradjc approved these changes

@alexandruradovici alexandruradovici alexandruradovici approved these changes

@lschuermann lschuermann lschuermann approved these changes

Assignees

@alexandruradovici alexandruradovici

Labels
component HIL This affects a Tock HIL interface. last-call Final review period for a pull request. stm32 Change pertains to the stm32 family of MCUSs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@inesmaria08 @bradjc @alexandruradovici @alevy @lschuermann
0