On Thu, 16 Feb 2023 14:07:53 +0100
Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
>
