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

usb: disable tps6598x interrupts #72

Merged
merged 4 commits into from
Jun 18, 2021
Merged

Conversation

jannau
Copy link
Member

@jannau jannau commented Jun 9, 2021

Restore the interrupt masks on transition to the next state. The masks
are not restored on the USB-C port used by the hypervisor. This prevents
an interrupt storm in the guest when the other USB-C port is exposed to
the guest. The tps6598x interrupt line is shared amoung both ports.

@jannau jannau mentioned this pull request Jun 9, 2021
@marcan
Copy link
Member

marcan commented Jun 9, 2021

Can we just unconditionally do this on init? If m1n1 isn't using/handling these IRQs it should just disable them by default.

@jannau
Copy link
Member Author

jannau commented Jun 9, 2021

Mac OS seems to rely on the default config or iboot for the pd controller's interrupt config. When the interrupts are disabled in m1n1 the port is dead.
We could declare that HV needs port 0 and only disable interrupts it.

@marcan
Copy link
Member

marcan commented Jun 10, 2021

We've run into things like this in the past, so I think I'm going to rework how we do subsystem shutdowns a bit. I'll discriminate between "shutdown for kboot" and "shutdown for chainload"; the latter tries to restore state to whatever iBoot leaves to make macOS (or another m1n1) happy, while the former leaves things in the sanest state for Linux. Let me think for a bit how I want to do this :)

@jannau jannau changed the title hv: add proxy command to disable interrupts of the USB PD controller usb: disable tps6598x interrupts Jun 12, 2021
@jannau
Copy link
Member Author

jannau commented Jun 12, 2021

I think the sanest state for chainload and linux boot does not differ in this case. Leaving the interrupts in the default config looks sane to me. The Problem is the hv guest. It wouldn't be a problem there either if we were not using one port for the hv or if both ports were not sharing the same interrupt line.

Kept now mostly within m1n1. The only required proxy change is that the other USB iodev gets disabled in hv.init() so m1n1 knows in hv_init for which port it should restore the interrupt mask.

Also in this PR some minor python hv fixes and removal of the hiding of the second usb-c port.

@jannau jannau marked this pull request as draft June 13, 2021 20:28
@@ -677,7 +683,7 @@ def rh(base, off, width):
print(f"R {base:x}+{off:x}:{width} = 0x{data:x} -> 0x{ret:x}")
return ret

for addr in (0x23b700420, 0x23d280098, 0x23d280088, 0x23d280090):
for addr in (0x23b700420, 0x23d280098, 0x23d280088):
Copy link
Member

Choose a reason for hiding this comment

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

Does this cover the case where the HV is using both ports? I've only tested with port 0 so far, I imagine the problem addresses are different for each.

Copy link
Member Author

@jannau jannau Jun 15, 2021

Choose a reason for hiding this comment

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

I haven't noticed a difference after removing 0x23d280090 and still removing both ports from the guest's ADT.

It is however required when running running Mac OS as guest using HV on port 1. In that case it's necessary to hook (0x23b700448, 0x23d2800a0, 0x23d280090) though. I'll add that to this PR.

Is there use case where the HV would use both USB-C ports? Hiding only the used USB-C port from the guest looks like the sane default when there are devices with only two USB ports.

@marcan
Copy link
Member

marcan commented Jun 15, 2021

I don't like the idea of leaving IRQs enabled for Linux (the sane state for IRQ hardware is leaving stuff masked), but we can shave that yak later. Other than the comment this looks good, feel free to mark it ready.

Restore the interrupt masks on chainload or HV guest start. The
interrupt mask is not restored on the USB-C port used by the hypervisor.
This prevents an interrupt storm in the guest when the other USB-C port
is exposed to the guest. Both tps6598x share unfortunately an interrupt
line.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Allows running HV over each USB-C port of a Mac Mini with Mac OS as
guest.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
@jannau jannau marked this pull request as ready for review June 15, 2021 22:28
@jannau
Copy link
Member Author

jannau commented Jun 15, 2021

Interrupts are now only restored for the guest and for chainloading (not via chainload.py -c though)
Writes to the tps6598x were not reliable. I've both failures to clear IntMask and to restore it to the boot values. Apparently "solved" by adding udelay(120) after i2c writes.
HV works now on both USB-C ports and the other port is usable in Mac OS.

@pipcet
Copy link
Contributor

pipcet commented Aug 25, 2021

I don't like the idea of leaving IRQs enabled for Linux (the sane state for IRQ hardware is leaving stuff masked), but we can shave that yak later. Other than the comment this looks good, feel free to mark it ready.

I've experimented a little, and my theory is that on the MacBooks, the SMC (or the PMP, or any other coprocessor) snoops the PD chips' interrupt line to know when to kick the battery charging circuitry: reconnecting power with the interrupts masked, the PD chip will correctly configure 20V but no power is drawn and there are no SMC notifications. Unmasking interrupts fixes this.

(I don't think there is a sane way to deal with this as it's not a sane thing for Apple to be doing.)

@jannau jannau deleted the usb_hpm_irq branch March 5, 2022 16:04
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.

None yet

3 participants