Date:   Mon, 20 Feb 2023 23:02:15 +1100
From:   Orlando Chamberlain <orlandoch.dev@gmail.com>
To:     Lukas Wunner <lukas@wunner.de>
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: <20230220230215.6e7e09cf@redecorated-mbp>
In-Reply-To: <20230219221737.GA17355@wunner.de>
References: <20230218132007.3350-1-orlandoch.dev@gmail.com>
        <20230218132007.3350-4-orlandoch.dev@gmail.com>
        <20230219221737.GA17355@wunner.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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