[v2,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>
---
v1->v2: Only enable this on MMIO gmux's
drivers/platform/x86/apple-gmux.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
Comments
Hi Orlando,
Thank you for the new version patches 1 + 2 look good,
one small remark on this one.
On 2/16/23 13:23, 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.
>
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
> v1->v2: Only enable this on MMIO gmux's
> drivers/platform/x86/apple-gmux.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 36208e93d745..12a93fc49c36 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -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,
> @@ -536,7 +560,11 @@ 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 (status) {
> + gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> + if (gmux_data->config->use_acpi_gmsp)
> + gmux_call_acpi_gmsp(gmux_data, 0);
> + }
This changes the behavior on the existing supported models to
only write back status when it is non 0. This is likely fine
but given that we seem to lack testers for the old models
I would prefer to not change the behavior there.
So how about:
gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
if (status && gmux_data->config->use_acpi_gmsp)
gmux_call_acpi_gmsp(gmux_data, 0);
?
The 0 write to what presumably is a register with
write 1 to clear bits should be harmless.
You can test that it is harmless on the new MMIO models
and this way we don't change the behavior on the older models.
Regards,
Hans
On Thu, 16 Feb 2023 14:07:53 +0100
Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Orlando,
>
> Thank you for the new version patches 1 + 2 look good,
> one small remark on this one.
>
> On 2/16/23 13:23, 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.
> >
> > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > ---
> > v1->v2: Only enable this on MMIO gmux's
> > drivers/platform/x86/apple-gmux.c | 30
> > +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1
> > deletion(-)
> >
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c index
> > 36208e93d745..12a93fc49c36 100644 ---
> > a/drivers/platform/x86/apple-gmux.c +++
> > b/drivers/platform/x86/apple-gmux.c @@ -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,
> > @@ -536,7 +560,11 @@ 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 (status) {
> > + gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS,
> > status);
> > + if (gmux_data->config->use_acpi_gmsp)
> > + gmux_call_acpi_gmsp(gmux_data, 0);
> > + }
>
> This changes the behavior on the existing supported models to
> only write back status when it is non 0. This is likely fine
> but given that we seem to lack testers for the old models
> I would prefer to not change the behavior there.
Oops I didn't realise I had changed that.
>
> So how about:
>
> gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> if (status && gmux_data->config->use_acpi_gmsp)
> gmux_call_acpi_gmsp(gmux_data, 0);
>
> ?
>
> The 0 write to what presumably is a register with
> write 1 to clear bits should be harmless.
>
> You can test that it is harmless on the new MMIO models
> and this way we don't change the behavior on the older models.
Your suggested code works fine for me.
I've realised the check for status != 0 probably isn't needed so I'll
take that out too, as using GMSP means we don't get spam of interrupts
with status = 0 on MMIO gmux's.
>
> Regards,
>
> Hans
>
@@ -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,
@@ -536,7 +560,11 @@ 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 (status) {
+ 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)