[v3,3/5] apple-gmux: Use GMSP acpi method for interrupt clear
Commit Message
This is needed for interrupts to be cleared correctly on MMIO based
gmux's. It is untested if this helps/hinders other gmux types, so
currently this is only enabled for the MMIO gmux's.
There is also a "GMLV" acpi method, and the "GMSP" method can be called
with 1 as its argument, but the purposes of these aren't known and they
don't seem to be needed.
Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
v2->v3: remove status != 0 check
drivers/platform/x86/apple-gmux.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
Comments
On Sun, Feb 19, 2023 at 12:20:05AM +1100, Orlando Chamberlain wrote:
> This is needed for interrupts to be cleared correctly on MMIO based
> gmux's. It is untested if this helps/hinders other gmux types, so
> currently this is only enabled for the MMIO gmux's.
>
> There is also a "GMLV" acpi method, and the "GMSP" method can be called
> with 1 as its argument, but the purposes of these aren't known and they
> don't seem to be needed.
GMLV and GMSP access a GPIO on the PCH which is connected to the
GMUX_INT pin of the gmux microcontroller. I've just verified that
in the schematics of my MBP9,1.
GMLV reads the value of the GPIO ("level").
GMSP likely sets the value ("set polarity").
On my MBP9,1 (indexed gmux), if the gmux controller signals an interrupt,
the platform signals a notification:
Scope (\_GPE)
{
Method (_L16, 0, NotSerialized) // _Lxx: Level-Triggered GPE
{
Notify (\_SB.PCI0.LPCB.GMUX, 0x80) // Status Change
}
}
Comparing this to the MBP13,3 and MBP16,1, the GPE method differentiates
between the OS type: On Darwin, only a notification is signaled,
whereas on other OSes, the GPIO's value is read and then inverted:
Scope (\_GPE)
{
Method (_L15, 0, NotSerialized) // _Lxx: Level-Triggered GPE, xx=0x00-0xFF
{
If (OSDW ())
{
Notify (\_SB.PCI0.LPCB.GPUC, 0x80) // Status Change
}
ElseIf ((\_SB.GGII (0x03000015) == One))
{
\_SB.SGII (0x03000015, Zero)
}
Else
{
\_SB.SGII (0x03000015, One)
}
}
}
Linux masquerades as Darwin, so ends up in the notification-only
code path.
Does macOS execute the GMSP method as well? Have you disassembled
the gmux driver? All vital information that belongs in the commit
message and/or a code comment.
> +static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data, int arg)
> +{
> + acpi_status status = AE_OK;
> + union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> + struct acpi_object_list arg_list = { 1, &arg0 };
> +
> + arg0.integer.value = arg;
> +
> + status = acpi_evaluate_object(gmux_data->dhandle, "GMSP", &arg_list, NULL);
Can this be simplified by using acpi_execute_simple_method() or
one of the other helpers provided by drivers/acpi/utils.c?
> @@ -537,6 +561,8 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
> /* to clear interrupts write back current status */
> status = gmux_interrupt_get_status(gmux_data);
> gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> + if (gmux_data->config->use_acpi_gmsp)
> + gmux_call_acpi_gmsp(gmux_data, 0);
> }
I think it would be clearer to check the gmux type directly here,
so that a casual reader understands that invoking the method is
necessary on MMIO-accessed GMUXes, but not any of the other types.
By contrast, with the use_acpi_gmsp one has to look up first which
of the gmux types sets this to true.
What happens if GMSP is not executed? Needs to be documented in the
commit message and/or a code comment!
Thanks,
Lukas
On Sun, 19 Feb 2023 23:17:37 +0100
Lukas Wunner <lukas@wunner.de> wrote:
> On Sun, Feb 19, 2023 at 12:20:05AM +1100, Orlando Chamberlain wrote:
> > This is needed for interrupts to be cleared correctly on MMIO based
> > gmux's. It is untested if this helps/hinders other gmux types, so
> > currently this is only enabled for the MMIO gmux's.
> >
> > There is also a "GMLV" acpi method, and the "GMSP" method can be
> > called with 1 as its argument, but the purposes of these aren't
> > known and they don't seem to be needed.
>
> GMLV and GMSP access a GPIO on the PCH which is connected to the
> GMUX_INT pin of the gmux microcontroller. I've just verified that
> in the schematics of my MBP9,1.
>
> GMLV reads the value of the GPIO ("level").
> GMSP likely sets the value ("set polarity").
>
> On my MBP9,1 (indexed gmux), if the gmux controller signals an
> interrupt, the platform signals a notification:
>
> Scope (\_GPE)
> {
> Method (_L16, 0, NotSerialized) // _Lxx: Level-Triggered GPE
> {
> Notify (\_SB.PCI0.LPCB.GMUX, 0x80) // Status Change
> }
> }
>
> Comparing this to the MBP13,3 and MBP16,1, the GPE method
> differentiates between the OS type: On Darwin, only a notification
> is signaled, whereas on other OSes, the GPIO's value is read and then
> inverted:
>
> Scope (\_GPE)
> {
> Method (_L15, 0, NotSerialized) // _Lxx: Level-Triggered GPE,
> xx=0x00-0xFF {
> If (OSDW ())
> {
> Notify (\_SB.PCI0.LPCB.GPUC, 0x80) // Status Change
> }
> ElseIf ((\_SB.GGII (0x03000015) == One))
> {
> \_SB.SGII (0x03000015, Zero)
> }
> Else
> {
> \_SB.SGII (0x03000015, One)
> }
> }
> }
>
> Linux masquerades as Darwin, so ends up in the notification-only
> code path.
>
> Does macOS execute the GMSP method as well? Have you disassembled
> the gmux driver? All vital information that belongs in the commit
> message and/or a code comment.
I think it does, if certain based bits in "HWFeatureMask" (which shows
up in `ioreg -l`) are set, but I'm not very good at RE so I don't know
exactly how macOS uses it. The kext is AppleMuxControl2.kext.
>
>
> > +static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data,
> > int arg) +{
> > + acpi_status status = AE_OK;
> > + union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > + struct acpi_object_list arg_list = { 1, &arg0 };
> > +
> > + arg0.integer.value = arg;
> > +
> > + status = acpi_evaluate_object(gmux_data->dhandle, "GMSP",
> > &arg_list, NULL);
>
> Can this be simplified by using acpi_execute_simple_method() or
> one of the other helpers provided by drivers/acpi/utils.c?
>
Yes it can thanks!
>
> > @@ -537,6 +561,8 @@ static void gmux_clear_interrupts(struct
> > apple_gmux_data *gmux_data) /* to clear interrupts write back
> > current status */ status = gmux_interrupt_get_status(gmux_data);
> > gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> > + if (gmux_data->config->use_acpi_gmsp)
> > + gmux_call_acpi_gmsp(gmux_data, 0);
> > }
>
> I think it would be clearer to check the gmux type directly here,
> so that a casual reader understands that invoking the method is
> necessary on MMIO-accessed GMUXes, but not any of the other types.
> By contrast, with the use_acpi_gmsp one has to look up first which
> of the gmux types sets this to true.
I can do it like that next version.
>
> What happens if GMSP is not executed? Needs to be documented in the
> commit message and/or a code comment!
>
It gets a flood of status=0 interrupts, I'll add that as a comment.
> Thanks,
>
> Lukas
@@ -76,6 +76,7 @@ struct apple_gmux_config {
enum vga_switcheroo_handler_flags_t handler_flags;
unsigned long resource_type;
bool read_version_as_u32;
+ bool use_acpi_gmsp;
char *name;
};
@@ -488,6 +489,7 @@ static const struct apple_gmux_config apple_gmux_pio = {
.handler_flags = VGA_SWITCHEROO_CAN_SWITCH_DDC,
.resource_type = IORESOURCE_IO,
.read_version_as_u32 = false,
+ .use_acpi_gmsp = false,
.name = "classic"
};
@@ -500,6 +502,7 @@ static const struct apple_gmux_config apple_gmux_index = {
.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
.resource_type = IORESOURCE_IO,
.read_version_as_u32 = true,
+ .use_acpi_gmsp = false,
.name = "indexed"
};
@@ -511,8 +514,29 @@ static const struct apple_gmux_config apple_gmux_index = {
* MCP79, on all following generations it's GPIO pin 6 of the Intel PCH.
* The GPE merely signals that an interrupt occurred, the actual type of event
* is identified by reading a gmux register.
+ *
+ * On MMIO gmux's, we also need to call the acpi method GMSP to properly clear
+ * interrupts.
*/
+static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data, int arg)
+{
+ acpi_status status = AE_OK;
+ union acpi_object arg0 = { ACPI_TYPE_INTEGER };
+ struct acpi_object_list arg_list = { 1, &arg0 };
+
+ arg0.integer.value = arg;
+
+ status = acpi_evaluate_object(gmux_data->dhandle, "GMSP", &arg_list, NULL);
+ if (ACPI_FAILURE(status)) {
+ pr_err("GMSP call failed: %s\n",
+ acpi_format_exception(status));
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
{
gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_ENABLE,
@@ -537,6 +561,8 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
/* to clear interrupts write back current status */
status = gmux_interrupt_get_status(gmux_data);
gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
+ if (gmux_data->config->use_acpi_gmsp)
+ gmux_call_acpi_gmsp(gmux_data, 0);
}
static void gmux_notify_handler(acpi_handle device, u32 value, void *context)