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

Allow updating single fields of InterrupterRegisterSet #142

Merged
merged 4 commits into from
Aug 23, 2022

Conversation

Demindiro
Copy link
Member

This is necessary to be able to update ERDP without confusing the controller (in QEMU, at least).

For example, this code works:

		reg.erdp
			.update_volatile(|c| c.set_event_ring_dequeue_pointer(phys));
xhci: reset while running!
xhci_ep_nuke_xfers(1, 1)
xhci_ep_nuke_xfers(1, 3)
xhci: event ring[0]:1129000 [32]
xhci_process_commands()
xhci_process_commands()
xhci: input context at 122a000

But this doesn't (which is what the old API effectively did):

		reg.erdp
			.update_volatile(|c| c.set_event_ring_dequeue_pointer(phys));
		reg.erstba.update_volatile(|c| c.set(self.buf.as_phys()));
xhci: reset while running!
xhci_ep_nuke_xfers(1, 1)
xhci_ep_nuke_xfers(1, 3)
xhci: event ring[0]:112a000 [32]
xhci: event ring[0]:112a000 [32]
xhci_process_commands()
xhci: ER 0 full, drop event

Since this is an API break I also took the liberty to rename InterruptRegisterSet to the technically correct InterrupterRegisterSet. I added a type alias to reduce confusion.

Fixes #115

@toku-sa-n
Copy link
Member

Thank you for the PR. I'm going to go on a trip for three days from tomorrow. I'll check this PR after that.

@toku-sa-n
Copy link
Member

toku-sa-n commented Aug 15, 2022

I appreciate your patience. I plan to release two versions:

  • 0.8.7: Rename the struct and add a type alias (which is marked as deprecated).
  • 0.9.0: Remove the type alias (and fix the structure of InterrupterRegisterSet which I haven't checked yet).

Could you split the two things and send another PR for the renaming?

@toku-sa-n
Copy link
Member

Thank you for #143. Could you rebase this PR?

@Demindiro Demindiro force-pushed the fix-erdp-update branch 2 times, most recently from d595ee5 to 9f41a78 Compare August 19, 2022 15:40
This is necessary to be able to update ERDP without confusing the
controller (in QEMU, at least).
@toku-sa-n
Copy link
Member

Thank you. The code looks good. I'll check this PR with my OS later. Meanwhile, could you apply rustfmt?

@toku-sa-n
Copy link
Member

Thank you. There are some Clippy warnings: https://github.com/rust-osdev/xhci/runs/7940871377?check_suite_focus=true
Could you fix them?

@toku-sa-n
Copy link
Member

Thanks for the fix. The code looks good to me. I'm still trying to try this PR with my OS, but (un)fortunately, this PR revealed a defect in my OS. I'm struggling to fix it, but I'll merge this PR anyway if I can't fix it shortly.
Just a confirmation, but does your OS work well with this PR?


By the way, I've figured out why the old API confused QEMU. The description of ERSTBA which is on page 428 of xHCI specification 1.2 says:

Writing this register sets the Event Ring State Machine:EREP Advancement to the Start state.

Thus, writing to any register in an interrupter register set made an event ring reset.

@Demindiro
Copy link
Member Author

Just a confirmation, but does your OS work well with this PR?

It does work with USB keyboard, though I haven't tried other devices yet.

@toku-sa-n
Copy link
Member

Thank you. Sorry for the inconvenience, but please wait one or two days.

Copy link
Member

@toku-sa-n toku-sa-n left a comment

Choose a reason for hiding this comment

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

I confirmed this PR with my OS, and it works! Thank you for your contributions and patience!
I'm sorry, but I left a comment. After resolving it, I'll release 0.9.0.

src/registers/runtime.rs Outdated Show resolved Hide resolved
@toku-sa-n toku-sa-n merged commit 3af7a25 into rust-osdev:main Aug 23, 2022
@toku-sa-n
Copy link
Member

Thanks! I'll release 0.9.0 soon.

@toku-sa-n
Copy link
Member

Published as 0.9.0.

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

Successfully merging this pull request may close these issues.

Should InterruptRegisterSet have Single members?
2 participants