Date:   Sun, 19 Feb 2023 23:17:37 +0100
From:   Lukas Wunner <lukas@wunner.de>
To:     Orlando Chamberlain <orlandoch.dev@gmail.com>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        Seth Forshee <sforshee@kernel.org>,
        Aditya Garg <gargaditya08@live.com>,
        Aun-Ali Zaidi <admin@kodeit.net>,
        Kerem Karabay <kekrby@gmail.com>
Subject: Re: [PATCH v3 3/5] apple-gmux: Use GMSP acpi method for interrupt
 clear
Message-ID: <20230219221737.GA17355@wunner.de>
References: <20230218132007.3350-1-orlandoch.dev@gmail.com>
 <20230218132007.3350-4-orlandoch.dev@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230218132007.3350-4-orlandoch.dev@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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