[PATCH v2 0/5] apple-gmux: support MMIO gmux type on T2 Macs

[PATCH v2 0/5] apple-gmux: support MMIO gmux type on T2 Macs

[Orlando Chamberlain]

Hi All,

This patch series adds support for the MMIO based gmux present on these
Dual GPU Apple T2 Macs: MacBookPro15,1, MacBookPro15,3, MacBookPro16,1,
MacBookPro16,4 (although amdgpu isn't working on MacBookPro16,4 [1]).

Changes from v1/RFC[2]:

- Drop commits "use cpu_to_be32 instead of manual reorder" and
  "consolidate version reading" as the former was broken and they
  could introduce regressions.
- Only use the GMSP acpi method on MMIO gmux's.
- Use a debugfs interface instead of a sysfs interface.
- Document some of the chips used with MMIO gmux's.
- Move changes to amdgpu and snd_hda_intel out of this patchset.

# 1:

has a slight change in how the switch state is read: instead of checking
for x == 2, check !(x & 1)

# 2:

implements a system to support more than 2 gmux types

# 3:

start using the gmux's GMSP acpi method when handling interrupts on MMIO
gmux's. This is needed for the MMIO gmux's to clear interrupts.

# 4:

Adds support for the MMIO based gmux on T2 macs.

# 5:

Add a debugfs interface to apple-gmux so data from ports can be read
and written to from userspace.

This can be used for more easily researching what unknown ports do,
and switching gpus when vga_switcheroo isn't ready (e.g. when one gpu
is bound to vfio-pci and in use by a Windows VM, I can use this to
switch my internal display between Linux and Windows easily).

# Issues:

1. Switching gpus at runtime has the same issue as indexed gmux's: the
inactive gpu can't probe the DDC lines for eDP [3]

2. Powering on the amdgpu with vga_switcheroo doesn't work well. I'm
told on the MacBookPro15,1 it works sometimes, and adding delays helps,
but on my MacBookPro16,1 I haven't been able to get it to work at all:

amdgpu: switched off
amdgpu: switched on
amdgpu 0000:03:00.0:
    Unable to change power state from D3hot to D0, device inaccessible
amdgpu 0000:03:00.0:
    Unable to change power state from D3cold to D0, device inaccessible
[drm] PCIE GART of 512M enabled (table at 0x00000080FEE00000).
[drm] PSP is resuming...
[drm:psp_hw_start [amdgpu]] *ERROR* PSP create ring failed!
[drm:psp_resume [amdgpu]] *ERROR* PSP resume failed
[drm:amdgpu_device_fw_loading [amdgpu]]
    *ERROR* resume of IP block <psp> failed -62
amdgpu 0000:03:00.0: amdgpu: amdgpu_device_ip_resume failed (-62).
snd_hda_intel 0000:03:00.1: Enabling via vga_switcheroo
snd_hda_intel 0000:03:00.1:
    Unable to change power state from D3cold to D0, device inaccessible
snd_hda_intel 0000:03:00.1: CORB reset timeout#2, CORBRP = 65535
snd_hda_codec_hdmi hdaudioC0D0: Unable to sync register 0x2f0d00. -5

There are some acpi methods (PWRD, PWG1 [4, 5]) that macOS calls when
changing the amdgpu's power state, but we don't use them and that could be
a cause. Additionally unlike previous generation Macbooks which work
better, on MacBookPro16,1 the gpu is located behind 2 pci bridges:

01:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI]
    Navi 10 XL Upstream Port of PCI Express Switch (rev 43)
02:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI]
    Navi 10 XL Downstream Port of PCI Express Switch
03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI]
    Navi 14 [Radeon RX 5500/5500M / Pro 5500M] (rev 43)
03:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI]
    Navi 10 HDMI Audio

Upon attempting to power on the gpu with vga_switcheroo, all these
devices except 01:00.0 have their config space filled with 1s.
Rescanning pci makes the config space of all the devices go back to
normal, however amdgpu still fails to resume with the same logs as
above.

[1]: https://lore.kernel.org/all/3AFB9142-2BD0-46F9-AEA9-C9C5D13E68E6@live.com/
[2]: https://lore.kernel.org/platform-driver-x86/20230210044826.9834-1-orlandoch.dev@gmail.com/
[3]: https://lore.kernel.org/all/9eed8ede6f15a254ad578e783b050e1c585d5a15.1439288957.git.lukas@wunner.de/
[4]: https://gist.github.com/Redecorating/6c7136b7a4ac7ce3b77d8e41740dd87b
[5]: https://lore.kernel.org/all/20120710160555.GA31562@srcf.ucam.org/

Orlando Chamberlain (5):
  apple-gmux: use first bit to check switch state
  apple-gmux: refactor gmux types
  apple-gmux: Use GMSP acpi method for interrupt clear
  apple-gmux: support MMIO gmux on T2 Macs
  apple-gmux: add debugfs interface

 drivers/platform/x86/apple-gmux.c | 361 ++++++++++++++++++++++++++----
 include/linux/apple-gmux.h        |  52 +++--
 2 files changed, 349 insertions(+), 64 deletions(-)

-- 
2.39.1

[PATCH v2 1/5] apple-gmux: use first bit to check switch state

[Orlando Chamberlain]

On T2 Macs with MMIO gmux, when GMUX_PORT_SWITCH_DISPLAY is read, it can
have values of 2, 3, 4, and 5. Odd values correspond to the discrete gpu,
and even values correspond to the integrated gpu. The current logic is
that only 2 corresponds to IGD, but this doesn't work for T2 Macs.
Instead, check the first bit to determine the connected gpu.

As T2 Macs with gmux only can switch the internal display, it is
untested if this change (or a similar change) would be applicable
to GMUX_PORT_SWITCH_DDC and GMUX_PORT_SWITCH_EXTERNAL.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
v1->v2: no change
 drivers/platform/x86/apple-gmux.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 9333f82cfa8a..ec99e05e532c 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -346,10 +346,10 @@ static void gmux_read_switch_state(struct apple_gmux_data *gmux_data)
 	else
 		gmux_data->switch_state_ddc = VGA_SWITCHEROO_DIS;
 
-	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) == 2)
-		gmux_data->switch_state_display = VGA_SWITCHEROO_IGD;
-	else
+	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) & 1)
 		gmux_data->switch_state_display = VGA_SWITCHEROO_DIS;
+	else
+		gmux_data->switch_state_display = VGA_SWITCHEROO_IGD;
 
 	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL) == 2)
 		gmux_data->switch_state_external = VGA_SWITCHEROO_IGD;
-- 
2.39.1

[PATCH v2 2/5] apple-gmux: refactor gmux types

[Orlando Chamberlain]

Add apple_gmux_config struct containing operations and data specific to
each mux type.

This is in preparation for adding a third, MMIO based, gmux type.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
v1->v2: Handle the two ways of reading the version as part of this type
system (read_version_as_u32).
 drivers/platform/x86/apple-gmux.c | 93 ++++++++++++++++++++-----------
 include/linux/apple-gmux.h        | 18 ++++--
 2 files changed, 74 insertions(+), 37 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index ec99e05e532c..36208e93d745 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -5,6 +5,7 @@
  *  Copyright (C) Canonical Ltd. <seth.forshee@canonical.com>
  *  Copyright (C) 2010-2012 Andreas Heider <andreas@meetr.de>
  *  Copyright (C) 2015 Lukas Wunner <lukas@wunner.de>
+ *  Copyright (C) 2023 Orlando Chamberlain <orlandoch.dev@gmail.com>
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -43,10 +44,12 @@
  *     http://www.renesas.com/products/mpumcu/h8s/h8s2100/h8s2113/index.jsp
  */
 
+struct apple_gmux_config;
+
 struct apple_gmux_data {
 	unsigned long iostart;
 	unsigned long iolen;
-	bool indexed;
+	const struct apple_gmux_config *config;
 	struct mutex index_lock;
 
 	struct backlight_device *bdev;
@@ -64,6 +67,18 @@ struct apple_gmux_data {
 
 static struct apple_gmux_data *apple_gmux_data;
 
+struct apple_gmux_config {
+	u8 (*read8)(struct apple_gmux_data *gmux_data, int port);
+	void (*write8)(struct apple_gmux_data *gmux_data, int port, u8 val);
+	u32 (*read32)(struct apple_gmux_data *gmux_data, int port);
+	void (*write32)(struct apple_gmux_data *gmux_data, int port, u32 val);
+	const struct vga_switcheroo_handler *gmux_handler;
+	enum vga_switcheroo_handler_flags_t handler_flags;
+	unsigned long resource_type;
+	bool read_version_as_u32;
+	char *name;
+};
+
 #define GMUX_INTERRUPT_ENABLE		0xff
 #define GMUX_INTERRUPT_DISABLE		0x00
 
@@ -195,35 +210,23 @@ static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
 
 static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
 {
-	if (gmux_data->indexed)
-		return gmux_index_read8(gmux_data, port);
-	else
-		return gmux_pio_read8(gmux_data, port);
+	return gmux_data->config->read8(gmux_data, port);
 }
 
 static void gmux_write8(struct apple_gmux_data *gmux_data, int port, u8 val)
 {
-	if (gmux_data->indexed)
-		gmux_index_write8(gmux_data, port, val);
-	else
-		gmux_pio_write8(gmux_data, port, val);
+	return gmux_data->config->write8(gmux_data, port, val);
 }
 
 static u32 gmux_read32(struct apple_gmux_data *gmux_data, int port)
 {
-	if (gmux_data->indexed)
-		return gmux_index_read32(gmux_data, port);
-	else
-		return gmux_pio_read32(gmux_data, port);
+	return gmux_data->config->read32(gmux_data, port);
 }
 
 static void gmux_write32(struct apple_gmux_data *gmux_data, int port,
 			     u32 val)
 {
-	if (gmux_data->indexed)
-		gmux_index_write32(gmux_data, port, val);
-	else
-		gmux_pio_write32(gmux_data, port, val);
+	return gmux_data->config->write32(gmux_data, port, val);
 }
 
 /**
@@ -463,19 +466,43 @@ static enum vga_switcheroo_client_id gmux_get_client_id(struct pci_dev *pdev)
 		return VGA_SWITCHEROO_DIS;
 }
 
-static const struct vga_switcheroo_handler gmux_handler_indexed = {
+static const struct vga_switcheroo_handler gmux_handler_no_ddc = {
 	.switchto = gmux_switchto,
 	.power_state = gmux_set_power_state,
 	.get_client_id = gmux_get_client_id,
 };
 
-static const struct vga_switcheroo_handler gmux_handler_classic = {
+static const struct vga_switcheroo_handler gmux_handler_ddc = {
 	.switchto = gmux_switchto,
 	.switch_ddc = gmux_switch_ddc,
 	.power_state = gmux_set_power_state,
 	.get_client_id = gmux_get_client_id,
 };
 
+static const struct apple_gmux_config apple_gmux_pio = {
+	.read8 = &gmux_pio_read8,
+	.write8 = &gmux_pio_write8,
+	.read32 = &gmux_pio_read32,
+	.write32 = &gmux_pio_write32,
+	.gmux_handler = &gmux_handler_ddc,
+	.handler_flags = VGA_SWITCHEROO_CAN_SWITCH_DDC,
+	.resource_type = IORESOURCE_IO,
+	.read_version_as_u32 = false,
+	.name = "classic"
+};
+
+static const struct apple_gmux_config apple_gmux_index = {
+	.read8 = &gmux_index_read8,
+	.write8 = &gmux_index_write8,
+	.read32 = &gmux_index_read32,
+	.write32 = &gmux_index_write32,
+	.gmux_handler = &gmux_handler_no_ddc,
+	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
+	.resource_type = IORESOURCE_IO,
+	.read_version_as_u32 = true,
+	.name = "indexed"
+};
+
 /**
  * DOC: Interrupt
  *
@@ -565,13 +592,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	int ret = -ENXIO;
 	acpi_status status;
 	unsigned long long gpe;
-	bool indexed = false;
+	enum apple_gmux_type type;
 	u32 version;
 
 	if (apple_gmux_data)
 		return -EBUSY;
 
-	if (!apple_gmux_detect(pnp, &indexed)) {
+	if (!apple_gmux_detect(pnp, &type)) {
 		pr_info("gmux device not present\n");
 		return -ENODEV;
 	}
@@ -581,6 +608,16 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		return -ENOMEM;
 	pnp_set_drvdata(pnp, gmux_data);
 
+	switch (type) {
+	case APPLE_GMUX_TYPE_INDEXED:
+		gmux_data->config = &apple_gmux_index;
+		mutex_init(&gmux_data->index_lock);
+		break;
+	case APPLE_GMUX_TYPE_PIO:
+		gmux_data->config = &apple_gmux_pio;
+		break;
+	}
+
 	res = pnp_get_resource(pnp, IORESOURCE_IO, 0);
 	gmux_data->iostart = res->start;
 	gmux_data->iolen = resource_size(res);
@@ -591,9 +628,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		goto err_free;
 	}
 
-	if (indexed) {
-		mutex_init(&gmux_data->index_lock);
-		gmux_data->indexed = true;
+	if (gmux_data->config->read_version_as_u32) {
 		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
 		ver_major = (version >> 24) & 0xff;
 		ver_minor = (version >> 16) & 0xff;
@@ -604,7 +639,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE);
 	}
 	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
-		ver_release, (gmux_data->indexed ? "indexed" : "classic"));
+		ver_release, gmux_data->config->name);
 
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_PLATFORM;
@@ -694,12 +729,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	 *
 	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
 	 */
-	if (gmux_data->indexed)
-		ret = vga_switcheroo_register_handler(&gmux_handler_indexed,
-					      VGA_SWITCHEROO_NEEDS_EDP_CONFIG);
-	else
-		ret = vga_switcheroo_register_handler(&gmux_handler_classic,
-					      VGA_SWITCHEROO_CAN_SWITCH_DDC);
+	ret = vga_switcheroo_register_handler(gmux_data->config->gmux_handler,
+			gmux_data->config->handler_flags);
 	if (ret) {
 		pr_err("Failed to register vga_switcheroo handler\n");
 		goto err_register_handler;
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index 1f68b49bcd68..5f658439f7f8 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -36,6 +36,11 @@
 
 #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
 
+enum apple_gmux_type {
+	APPLE_GMUX_TYPE_PIO,
+	APPLE_GMUX_TYPE_INDEXED
+};
+
 #if IS_ENABLED(CONFIG_APPLE_GMUX)
 static inline bool apple_gmux_is_indexed(unsigned long iostart)
 {
@@ -65,13 +70,13 @@ static inline bool apple_gmux_is_indexed(unsigned long iostart)
  * Return: %true if a supported gmux ACPI device is detected and the kernel
  * was configured with CONFIG_APPLE_GMUX, %false otherwise.
  */
-static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
+static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, enum apple_gmux_type *type_ret)
 {
 	u8 ver_major, ver_minor, ver_release;
 	struct device *dev = NULL;
 	struct acpi_device *adev;
 	struct resource *res;
-	bool indexed = false;
+	enum apple_gmux_type type = APPLE_GMUX_TYPE_PIO;
 	bool ret = false;
 
 	if (!pnp_dev) {
@@ -99,13 +104,14 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
 	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
 	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
 	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
-		indexed = apple_gmux_is_indexed(res->start);
-		if (!indexed)
+		if (apple_gmux_is_indexed(res->start))
+			type = APPLE_GMUX_TYPE_INDEXED;
+		else
 			goto out;
 	}
 
-	if (indexed_ret)
-		*indexed_ret = indexed;
+	if (type_ret)
+		*type_ret = type;
 
 	ret = true;
 out:
-- 
2.39.1

[PATCH v2 3/5] apple-gmux: Use GMSP acpi method for interrupt clear

[Orlando Chamberlain]

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);
+	}
 }
 
 static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
-- 
2.39.1

[PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Orlando Chamberlain]

In some newer dual gpu MacBooks, gmux is controlled by the T2 security
chip, and acessed with MMIO. Add support for these gmux controllers

Interestingly, the ACPI table only allocates 8 bytes for GMUX, but we
actually need 16, and as such we request 16 with request_mem_region.

Reading and writing from ports:
    16 bytes from 0xfe0b0200 are used. 0x0 to 0x4 are where data
    to read appears, and where data to write goes. Writing to 0xe
    sets the gmux port being accessed, and writing to 0xf sends commands.

    These commands are 0x40 & data_length for write, and data_length for
    read, where data_length is 1, 2 or 4. Once byte base+0xf is 0, the
    command is done.

Issues:
    As with other retina models, we can't switch DDC lines so
    switching at runtime doesn't work if the inactive gpu driver
    already disabled eDP due to it not being connected when that
    driver loaded.

    Additionally, turning on the dgpu back on the MacBookPro16,1 does
    not work.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
v1->v2: Document some chips present, and clarify which chips aren't
present on MMIO gmux laptops.
 drivers/platform/x86/apple-gmux.c | 142 +++++++++++++++++++++++++++---
 include/linux/apple-gmux.h        |  40 ++++++---
 2 files changed, 158 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 12a93fc49c36..5bac6dcfada0 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -28,15 +28,17 @@
  * DOC: Overview
  *
  * gmux is a microcontroller built into the MacBook Pro to support dual GPUs:
- * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on retinas.
+ * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on pre-T2 retinas.
+ * The chip used on T2 Macs is not known.
  *
  * (The MacPro6,1 2013 also has a gmux, however it is unclear why since it has
  * dual GPUs but no built-in display.)
  *
  * gmux is connected to the LPC bus of the southbridge. Its I/O ports are
  * accessed differently depending on the microcontroller: Driver functions
- * to access a pre-retina gmux are infixed ``_pio_``, those for a retina gmux
- * are infixed ``_index_``.
+ * to access a pre-retina gmux are infixed ``_pio_``, those for a pre-T2
+ * retina gmux are infixed ``_index_``, and those on T2 Macs are infixed
+ * with ``_mmio_``.
  *
  * .. _Lattice XP2:
  *     http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
@@ -47,6 +49,7 @@
 struct apple_gmux_config;
 
 struct apple_gmux_data {
+	u8 *__iomem iomem_base;
 	unsigned long iostart;
 	unsigned long iolen;
 	const struct apple_gmux_config *config;
@@ -209,6 +212,79 @@ static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
 	mutex_unlock(&gmux_data->index_lock);
 }
 
+static int gmux_mmio_wait(struct apple_gmux_data *gmux_data)
+{
+	int i = 200;
+	u8 gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+
+	while (i && gwr) {
+		gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+		udelay(100);
+		i--;
+	}
+
+	return !!i;
+}
+
+static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
+{
+	u8 val;
+
+	mutex_lock(&gmux_data->index_lock);
+	gmux_mmio_wait(gmux_data);
+	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_READ | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+	gmux_mmio_wait(gmux_data);
+	val = ioread8(gmux_data->iomem_base);
+	mutex_unlock(&gmux_data->index_lock);
+
+	return val;
+}
+
+static void gmux_mmio_write8(struct apple_gmux_data *gmux_data, int port,
+			      u8 val)
+{
+	mutex_lock(&gmux_data->index_lock);
+	gmux_mmio_wait(gmux_data);
+	iowrite8(val, gmux_data->iomem_base);
+
+	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+
+	gmux_mmio_wait(gmux_data);
+	mutex_unlock(&gmux_data->index_lock);
+}
+
+static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
+{
+	u32 val;
+
+	mutex_lock(&gmux_data->index_lock);
+	gmux_mmio_wait(gmux_data);
+	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_READ | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+	gmux_mmio_wait(gmux_data);
+	val = be32_to_cpu(ioread32(gmux_data->iomem_base));
+	mutex_unlock(&gmux_data->index_lock);
+
+	return val;
+}
+
+static void gmux_mmio_write32(struct apple_gmux_data *gmux_data, int port,
+			       u32 val)
+{
+	mutex_lock(&gmux_data->index_lock);
+	iowrite32(cpu_to_be32(val), gmux_data->iomem_base);
+	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+	gmux_mmio_wait(gmux_data);
+	mutex_unlock(&gmux_data->index_lock);
+}
+
 static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
 {
 	return gmux_data->config->read8(gmux_data, port);
@@ -237,8 +313,8 @@ static void gmux_write32(struct apple_gmux_data *gmux_data, int port,
  * the GPU. On dual GPU MacBook Pros by contrast, either GPU may be suspended
  * to conserve energy. Hence the PWM signal needs to be generated by a separate
  * backlight driver which is controlled by gmux. The earliest generation
- * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. All newer models
- * use a `TI LP8545`_.
+ * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. Newer models
+ * use a `TI LP8545`_ or a TI LP8548.
  *
  * .. _TI LP8543: https://www.ti.com/lit/ds/symlink/lp8543.pdf
  * .. _TI LP8545: https://www.ti.com/lit/ds/symlink/lp8545.pdf
@@ -302,8 +378,8 @@ static const struct backlight_ops gmux_bl_ops = {
  * connecting it either to the discrete GPU or the Thunderbolt controller.
  * Oddly enough, while the full port is no longer switchable, AUX and HPD
  * are still switchable by way of an `NXP CBTL03062`_ (on pre-retinas
- * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on retinas) under the
- * control of gmux. Since the integrated GPU is missing the main link,
+ * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on pre-t2 retinas) under
+ * the control of gmux. Since the integrated GPU is missing the main link,
  * external displays appear to it as phantoms which fail to link-train.
  *
  * gmux receives the HPD signal of all display connectors and sends an
@@ -506,6 +582,20 @@ static const struct apple_gmux_config apple_gmux_index = {
 	.name = "indexed"
 };
 
+static const struct apple_gmux_config apple_gmux_mmio = {
+	.read8 = &gmux_mmio_read8,
+	.write8 = &gmux_mmio_write8,
+	.read32 = &gmux_mmio_read32,
+	.write32 = &gmux_mmio_write32,
+	.gmux_handler = &gmux_handler_no_ddc,
+	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
+	.resource_type = IORESOURCE_MEM,
+	.read_version_as_u32 = true,
+	.use_acpi_gmsp = true,
+	.name = "T2"
+};
+
+
 /**
  * DOC: Interrupt
  *
@@ -637,6 +727,25 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	pnp_set_drvdata(pnp, gmux_data);
 
 	switch (type) {
+	case APPLE_GMUX_TYPE_MMIO:
+		gmux_data->config = &apple_gmux_mmio;
+		mutex_init(&gmux_data->index_lock);
+
+		res = pnp_get_resource(pnp, IORESOURCE_MEM, 0);
+		gmux_data->iostart = res->start;
+		/* Although the ACPI table only allocates 8 bytes, we need 16. */
+		gmux_data->iolen = 16;
+		if (!request_mem_region(gmux_data->iostart, gmux_data->iolen,
+					"Apple gmux")) {
+			pr_err("gmux I/O already in use\n");
+			goto err_free;
+		}
+		gmux_data->iomem_base = ioremap(gmux_data->iostart, gmux_data->iolen);
+		if (!gmux_data->iomem_base) {
+			pr_err("couldn't remap gmux mmio region");
+			goto err_release;
+		}
+		goto get_version;
 	case APPLE_GMUX_TYPE_INDEXED:
 		gmux_data->config = &apple_gmux_index;
 		mutex_init(&gmux_data->index_lock);
@@ -656,6 +765,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		goto err_free;
 	}
 
+get_version:
 	if (gmux_data->config->read_version_as_u32) {
 		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
 		ver_major = (version >> 24) & 0xff;
@@ -686,7 +796,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 					 gmux_data, &gmux_bl_ops, &props);
 	if (IS_ERR(bdev)) {
 		ret = PTR_ERR(bdev);
-		goto err_release;
+		goto err_unmap;
 	}
 
 	gmux_data->bdev = bdev;
@@ -753,7 +863,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	/*
 	 * Retina MacBook Pros cannot switch the panel's AUX separately
 	 * and need eDP pre-calibration. They are distinguishable from
-	 * pre-retinas by having an "indexed" gmux.
+	 * pre-retinas by having an "indexed" or "T2" gmux.
 	 *
 	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
 	 */
@@ -778,8 +888,14 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 					   &gmux_notify_handler);
 err_notify:
 	backlight_device_unregister(bdev);
+err_unmap:
+	if (gmux_data->iomem_base)
+		iounmap(gmux_data->iomem_base);
 err_release:
-	release_region(gmux_data->iostart, gmux_data->iolen);
+	if (gmux_data->config->resource_type == IORESOURCE_MEM)
+		release_mem_region(gmux_data->iostart, gmux_data->iolen);
+	else
+		release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
 	kfree(gmux_data);
 	return ret;
@@ -800,7 +916,11 @@ static void gmux_remove(struct pnp_dev *pnp)
 
 	backlight_device_unregister(gmux_data->bdev);
 
-	release_region(gmux_data->iostart, gmux_data->iolen);
+	if (gmux_data->iomem_base) {
+		iounmap(gmux_data->iomem_base);
+		release_mem_region(gmux_data->iostart, gmux_data->iolen);
+	} else
+		release_region(gmux_data->iostart, gmux_data->iolen);
 	apple_gmux_data = NULL;
 	kfree(gmux_data);
 
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index 5f658439f7f8..b7532f26b756 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -34,11 +34,18 @@
 #define GMUX_PORT_READ			0xd0
 #define GMUX_PORT_WRITE			0xd4
 
+#define GMUX_MMIO_PORT_SELECT		0x0e
+#define GMUX_MMIO_COMMAND_SEND		0x0f
+
+#define GMUX_MMIO_READ			0x00
+#define GMUX_MMIO_WRITE			0x40
+
 #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
 
 enum apple_gmux_type {
 	APPLE_GMUX_TYPE_PIO,
-	APPLE_GMUX_TYPE_INDEXED
+	APPLE_GMUX_TYPE_INDEXED,
+	APPLE_GMUX_TYPE_MMIO
 };
 
 #if IS_ENABLED(CONFIG_APPLE_GMUX)
@@ -93,19 +100,24 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, enum apple_gmux_ty
 	}
 
 	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
-	if (!res || resource_size(res) < GMUX_MIN_IO_LEN)
-		goto out;
-
-	/*
-	 * Invalid version information may indicate either that the gmux
-	 * device isn't present or that it's a new one that uses indexed io.
-	 */
-	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
-	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
-	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
-	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
-		if (apple_gmux_is_indexed(res->start))
-			type = APPLE_GMUX_TYPE_INDEXED;
+	if (res && resource_size(res) >= GMUX_MIN_IO_LEN) {
+		/*
+		 * Invalid version information may indicate either that the gmux
+		 * device isn't present or that it's a new one that uses indexed io.
+		 */
+		ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
+		ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
+		ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
+		if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
+			if (apple_gmux_is_indexed(res->start))
+				type = APPLE_GMUX_TYPE_INDEXED;
+			else
+				goto out;
+		}
+	} else {
+		res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
+		if (res)
+			type = APPLE_GMUX_TYPE_MMIO;
 		else
 			goto out;
 	}
-- 
2.39.1

[PATCH v2 5/5] apple-gmux: add debugfs interface

[Orlando Chamberlain]

Allow reading and writing gmux ports from userspace.

For example:

echo 4 > /sys/kernel/debug/apple_gmux/selected_port
cat /sys/kernel/debug/apple_gmux/selected_port_data | xxd -p

Will show the gmux version information (00000005 in this case)

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
v1->v2: Use debugfs instead of sysfs.
 drivers/platform/x86/apple-gmux.c | 88 +++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 5bac6dcfada0..e8a35d98b113 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -22,6 +22,7 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/vga_switcheroo.h>
+#include <linux/debugfs.h>
 #include <asm/io.h>
 
 /**
@@ -66,6 +67,10 @@ struct apple_gmux_data {
 	enum vga_switcheroo_client_id switch_state_external;
 	enum vga_switcheroo_state power_state;
 	struct completion powerchange_done;
+
+	/* debugfs data */
+	u8 selected_port;
+	struct dentry *debug_dentry;
 };
 
 static struct apple_gmux_data *apple_gmux_data;
@@ -674,6 +679,87 @@ static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
 		complete(&gmux_data->powerchange_done);
 }
 
+/**
+ * DOC: Debugfs Interface
+ *
+ * gmux ports can be accessed from userspace as a debugfs interface. For example:
+ *
+ * # echo 4 > /sys/kernel/debug/apple_gmux/selected_port
+ * # cat /sys/kernel/debug/apple_gmux/selected_port_data | xxd -p
+ * 00000005
+ *
+ * Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
+ *
+ * 1 and 4 byte writes are also allowed.
+ */
+
+static ssize_t gmux_selected_port_data_write(struct file *file,
+		const char __user *userbuf, size_t count, loff_t *ppos)
+{
+	struct apple_gmux_data *gmux_data = file->private_data;
+	int ret;
+
+	if (*ppos)
+		return -EINVAL;
+
+	if (count == 1) {
+		u8 data;
+
+		ret = copy_from_user(&data, userbuf, 1);
+		if (ret)
+			return ret;
+		gmux_write8(gmux_data, gmux_data->selected_port, data);
+	} else if (count == 4) {
+		u32 data;
+
+		ret = copy_from_user(&data, userbuf, 4);
+		if (ret)
+			return ret;
+		gmux_write32(gmux_data, gmux_data->selected_port, data);
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t gmux_selected_port_data_read(struct file *file,
+		char __user *userbuf, size_t count, loff_t *ppos)
+{
+	struct apple_gmux_data *gmux_data = file->private_data;
+	u32 data;
+
+	data = gmux_read32(gmux_data, gmux_data->selected_port);
+
+	return simple_read_from_buffer(userbuf, count, ppos, &data, sizeof(data));
+}
+
+static const struct file_operations gmux_port_data_ops = {
+	.open = simple_open,
+	.write = gmux_selected_port_data_write,
+	.read = gmux_selected_port_data_read
+};
+
+static void gmux_init_debugfs(struct apple_gmux_data *gmux_data)
+{
+	struct dentry *debug_dentry;
+
+	debug_dentry = debugfs_create_dir(KBUILD_MODNAME, NULL);
+
+	if (IS_ERR(debug_dentry))
+		return;
+
+	gmux_data->debug_dentry = debug_dentry;
+
+	debugfs_create_u8("selected_port", 0644, debug_dentry, &gmux_data->selected_port);
+	debugfs_create_file("selected_port_data", 0644, debug_dentry,
+			gmux_data, &gmux_port_data_ops);
+}
+
+static void gmux_fini_debugfs(struct apple_gmux_data *gmux_data)
+{
+	debugfs_remove_recursive(gmux_data->debug_dentry);
+}
+
 static int gmux_suspend(struct device *dev)
 {
 	struct pnp_dev *pnp = to_pnp_dev(dev);
@@ -874,6 +960,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		goto err_register_handler;
 	}
 
+	gmux_init_debugfs(gmux_data);
 	return 0;
 
 err_register_handler:
@@ -905,6 +992,7 @@ static void gmux_remove(struct pnp_dev *pnp)
 {
 	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
 
+	gmux_fini_debugfs(gmux_data);
 	vga_switcheroo_unregister_handler();
 	gmux_disable_interrupts(gmux_data);
 	if (gmux_data->gpe >= 0) {
-- 
2.39.1

Re: [PATCH v2 3/5] apple-gmux: Use GMSP acpi method for interrupt clear

[Hans de Goede]

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

Re: [PATCH v2 2/5] apple-gmux: refactor gmux types

[Hans de Goede]

Hi,

One small nit below.

On 2/16/23 13:23, Orlando Chamberlain wrote:
> Add apple_gmux_config struct containing operations and data specific to
> each mux type.
> 
> This is in preparation for adding a third, MMIO based, gmux type.
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
> v1->v2: Handle the two ways of reading the version as part of this type
> system (read_version_as_u32).
>  drivers/platform/x86/apple-gmux.c | 93 ++++++++++++++++++++-----------
>  include/linux/apple-gmux.h        | 18 ++++--
>  2 files changed, 74 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index ec99e05e532c..36208e93d745 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -5,6 +5,7 @@
>   *  Copyright (C) Canonical Ltd. <seth.forshee@canonical.com>
>   *  Copyright (C) 2010-2012 Andreas Heider <andreas@meetr.de>
>   *  Copyright (C) 2015 Lukas Wunner <lukas@wunner.de>
> + *  Copyright (C) 2023 Orlando Chamberlain <orlandoch.dev@gmail.com>
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -43,10 +44,12 @@
>   *     http://www.renesas.com/products/mpumcu/h8s/h8s2100/h8s2113/index.jsp
>   */
>  
> +struct apple_gmux_config;
> +
>  struct apple_gmux_data {
>  	unsigned long iostart;
>  	unsigned long iolen;
> -	bool indexed;
> +	const struct apple_gmux_config *config;
>  	struct mutex index_lock;
>  
>  	struct backlight_device *bdev;
> @@ -64,6 +67,18 @@ struct apple_gmux_data {
>  
>  static struct apple_gmux_data *apple_gmux_data;
>  
> +struct apple_gmux_config {
> +	u8 (*read8)(struct apple_gmux_data *gmux_data, int port);
> +	void (*write8)(struct apple_gmux_data *gmux_data, int port, u8 val);
> +	u32 (*read32)(struct apple_gmux_data *gmux_data, int port);
> +	void (*write32)(struct apple_gmux_data *gmux_data, int port, u32 val);
> +	const struct vga_switcheroo_handler *gmux_handler;
> +	enum vga_switcheroo_handler_flags_t handler_flags;
> +	unsigned long resource_type;
> +	bool read_version_as_u32;
> +	char *name;
> +};
> +
>  #define GMUX_INTERRUPT_ENABLE		0xff
>  #define GMUX_INTERRUPT_DISABLE		0x00
>  
> @@ -195,35 +210,23 @@ static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
>  
>  static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
>  {
> -	if (gmux_data->indexed)
> -		return gmux_index_read8(gmux_data, port);
> -	else
> -		return gmux_pio_read8(gmux_data, port);
> +	return gmux_data->config->read8(gmux_data, port);
>  }
>  
>  static void gmux_write8(struct apple_gmux_data *gmux_data, int port, u8 val)
>  {
> -	if (gmux_data->indexed)
> -		gmux_index_write8(gmux_data, port, val);
> -	else
> -		gmux_pio_write8(gmux_data, port, val);
> +	return gmux_data->config->write8(gmux_data, port, val);
>  }
>  
>  static u32 gmux_read32(struct apple_gmux_data *gmux_data, int port)
>  {
> -	if (gmux_data->indexed)
> -		return gmux_index_read32(gmux_data, port);
> -	else
> -		return gmux_pio_read32(gmux_data, port);
> +	return gmux_data->config->read32(gmux_data, port);
>  }
>  
>  static void gmux_write32(struct apple_gmux_data *gmux_data, int port,
>  			     u32 val)
>  {
> -	if (gmux_data->indexed)
> -		gmux_index_write32(gmux_data, port, val);
> -	else
> -		gmux_pio_write32(gmux_data, port, val);
> +	return gmux_data->config->write32(gmux_data, port, val);
>  }
>  
>  /**
> @@ -463,19 +466,43 @@ static enum vga_switcheroo_client_id gmux_get_client_id(struct pci_dev *pdev)
>  		return VGA_SWITCHEROO_DIS;
>  }
>  
> -static const struct vga_switcheroo_handler gmux_handler_indexed = {
> +static const struct vga_switcheroo_handler gmux_handler_no_ddc = {
>  	.switchto = gmux_switchto,
>  	.power_state = gmux_set_power_state,
>  	.get_client_id = gmux_get_client_id,
>  };
>  
> -static const struct vga_switcheroo_handler gmux_handler_classic = {
> +static const struct vga_switcheroo_handler gmux_handler_ddc = {
>  	.switchto = gmux_switchto,
>  	.switch_ddc = gmux_switch_ddc,
>  	.power_state = gmux_set_power_state,
>  	.get_client_id = gmux_get_client_id,
>  };
>  
> +static const struct apple_gmux_config apple_gmux_pio = {
> +	.read8 = &gmux_pio_read8,
> +	.write8 = &gmux_pio_write8,
> +	.read32 = &gmux_pio_read32,
> +	.write32 = &gmux_pio_write32,
> +	.gmux_handler = &gmux_handler_ddc,
> +	.handler_flags = VGA_SWITCHEROO_CAN_SWITCH_DDC,
> +	.resource_type = IORESOURCE_IO,
> +	.read_version_as_u32 = false,
> +	.name = "classic"
> +};
> +
> +static const struct apple_gmux_config apple_gmux_index = {
> +	.read8 = &gmux_index_read8,
> +	.write8 = &gmux_index_write8,
> +	.read32 = &gmux_index_read32,
> +	.write32 = &gmux_index_write32,
> +	.gmux_handler = &gmux_handler_no_ddc,
> +	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
> +	.resource_type = IORESOURCE_IO,
> +	.read_version_as_u32 = true,
> +	.name = "indexed"
> +};
> +
>  /**
>   * DOC: Interrupt
>   *
> @@ -565,13 +592,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	int ret = -ENXIO;
>  	acpi_status status;
>  	unsigned long long gpe;
> -	bool indexed = false;
> +	enum apple_gmux_type type;
>  	u32 version;
>  
>  	if (apple_gmux_data)
>  		return -EBUSY;
>  
> -	if (!apple_gmux_detect(pnp, &indexed)) {
> +	if (!apple_gmux_detect(pnp, &type)) {
>  		pr_info("gmux device not present\n");
>  		return -ENODEV;
>  	}
> @@ -581,6 +608,16 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		return -ENOMEM;
>  	pnp_set_drvdata(pnp, gmux_data);
>  
> +	switch (type) {
> +	case APPLE_GMUX_TYPE_INDEXED:
> +		gmux_data->config = &apple_gmux_index;
> +		mutex_init(&gmux_data->index_lock);
> +		break;
> +	case APPLE_GMUX_TYPE_PIO:
> +		gmux_data->config = &apple_gmux_pio;
> +		break;
> +	}
> +
>  	res = pnp_get_resource(pnp, IORESOURCE_IO, 0);
>  	gmux_data->iostart = res->start;
>  	gmux_data->iolen = resource_size(res);
> @@ -591,9 +628,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		goto err_free;
>  	}
>  
> -	if (indexed) {
> -		mutex_init(&gmux_data->index_lock);
> -		gmux_data->indexed = true;
> +	if (gmux_data->config->read_version_as_u32) {
>  		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
>  		ver_major = (version >> 24) & 0xff;
>  		ver_minor = (version >> 16) & 0xff;
> @@ -604,7 +639,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE);
>  	}
>  	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
> -		ver_release, (gmux_data->indexed ? "indexed" : "classic"));
> +		ver_release, gmux_data->config->name);
>  
>  	memset(&props, 0, sizeof(props));
>  	props.type = BACKLIGHT_PLATFORM;
> @@ -694,12 +729,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	 *
>  	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
>  	 */
> -	if (gmux_data->indexed)
> -		ret = vga_switcheroo_register_handler(&gmux_handler_indexed,
> -					      VGA_SWITCHEROO_NEEDS_EDP_CONFIG);
> -	else
> -		ret = vga_switcheroo_register_handler(&gmux_handler_classic,
> -					      VGA_SWITCHEROO_CAN_SWITCH_DDC);
> +	ret = vga_switcheroo_register_handler(gmux_data->config->gmux_handler,
> +			gmux_data->config->handler_flags);
>  	if (ret) {
>  		pr_err("Failed to register vga_switcheroo handler\n");
>  		goto err_register_handler;
> diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> index 1f68b49bcd68..5f658439f7f8 100644
> --- a/include/linux/apple-gmux.h
> +++ b/include/linux/apple-gmux.h
> @@ -36,6 +36,11 @@
>  
>  #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
>  
> +enum apple_gmux_type {
> +	APPLE_GMUX_TYPE_PIO,
> +	APPLE_GMUX_TYPE_INDEXED

In the kernel with things like enum "values" or array initializers we
typically add a , at the end of the last entry, to avoid needless
churn when adding more entry.

The one exception is this when there is a special entry which
marks the end of the array / enum. But that is not the case here.


> +};
> +
>  #if IS_ENABLED(CONFIG_APPLE_GMUX)
>  static inline bool apple_gmux_is_indexed(unsigned long iostart)
>  {
> @@ -65,13 +70,13 @@ static inline bool apple_gmux_is_indexed(unsigned long iostart)
>   * Return: %true if a supported gmux ACPI device is detected and the kernel
>   * was configured with CONFIG_APPLE_GMUX, %false otherwise.
>   */
> -static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, enum apple_gmux_type *type_ret)
>  {
>  	u8 ver_major, ver_minor, ver_release;
>  	struct device *dev = NULL;
>  	struct acpi_device *adev;
>  	struct resource *res;
> -	bool indexed = false;
> +	enum apple_gmux_type type = APPLE_GMUX_TYPE_PIO;
>  	bool ret = false;
>  
>  	if (!pnp_dev) {
> @@ -99,13 +104,14 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
>  	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
>  	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
>  	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> -		indexed = apple_gmux_is_indexed(res->start);
> -		if (!indexed)
> +		if (apple_gmux_is_indexed(res->start))
> +			type = APPLE_GMUX_TYPE_INDEXED;
> +		else
>  			goto out;
>  	}
>  
> -	if (indexed_ret)
> -		*indexed_ret = indexed;
> +	if (type_ret)
> +		*type_ret = type;
>  
>  	ret = true;
>  out:

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Hans de Goede]

Hi,

On 2/16/23 13:23, Orlando Chamberlain wrote:
> In some newer dual gpu MacBooks, gmux is controlled by the T2 security
> chip, and acessed with MMIO. Add support for these gmux controllers
> 
> Interestingly, the ACPI table only allocates 8 bytes for GMUX, but we
> actually need 16, and as such we request 16 with request_mem_region.
> 
> Reading and writing from ports:
>     16 bytes from 0xfe0b0200 are used. 0x0 to 0x4 are where data
>     to read appears, and where data to write goes. Writing to 0xe
>     sets the gmux port being accessed, and writing to 0xf sends commands.
> 
>     These commands are 0x40 & data_length for write, and data_length for
>     read, where data_length is 1, 2 or 4. Once byte base+0xf is 0, the
>     command is done.
> 
> Issues:
>     As with other retina models, we can't switch DDC lines so
>     switching at runtime doesn't work if the inactive gpu driver
>     already disabled eDP due to it not being connected when that
>     driver loaded.
> 
>     Additionally, turning on the dgpu back on the MacBookPro16,1 does
>     not work.
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
> v1->v2: Document some chips present, and clarify which chips aren't
> present on MMIO gmux laptops.
>  drivers/platform/x86/apple-gmux.c | 142 +++++++++++++++++++++++++++---
>  include/linux/apple-gmux.h        |  40 ++++++---
>  2 files changed, 158 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 12a93fc49c36..5bac6dcfada0 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -28,15 +28,17 @@
>   * DOC: Overview
>   *
>   * gmux is a microcontroller built into the MacBook Pro to support dual GPUs:
> - * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on retinas.
> + * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on pre-T2 retinas.
> + * The chip used on T2 Macs is not known.
>   *
>   * (The MacPro6,1 2013 also has a gmux, however it is unclear why since it has
>   * dual GPUs but no built-in display.)
>   *
>   * gmux is connected to the LPC bus of the southbridge. Its I/O ports are
>   * accessed differently depending on the microcontroller: Driver functions
> - * to access a pre-retina gmux are infixed ``_pio_``, those for a retina gmux
> - * are infixed ``_index_``.
> + * to access a pre-retina gmux are infixed ``_pio_``, those for a pre-T2
> + * retina gmux are infixed ``_index_``, and those on T2 Macs are infixed
> + * with ``_mmio_``.
>   *
>   * .. _Lattice XP2:
>   *     http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
> @@ -47,6 +49,7 @@
>  struct apple_gmux_config;
>  
>  struct apple_gmux_data {
> +	u8 *__iomem iomem_base;
>  	unsigned long iostart;
>  	unsigned long iolen;
>  	const struct apple_gmux_config *config;
> @@ -209,6 +212,79 @@ static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
>  	mutex_unlock(&gmux_data->index_lock);
>  }
>  
> +static int gmux_mmio_wait(struct apple_gmux_data *gmux_data)
> +{
> +	int i = 200;
> +	u8 gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +
> +	while (i && gwr) {
> +		gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		udelay(100);
> +		i--;
> +	}
> +
> +	return !!i;
> +}
> +
> +static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
> +{
> +	u8 val;
> +
> +	mutex_lock(&gmux_data->index_lock);
> +	gmux_mmio_wait(gmux_data);
> +	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +	gmux_mmio_wait(gmux_data);
> +	val = ioread8(gmux_data->iomem_base);
> +	mutex_unlock(&gmux_data->index_lock);
> +
> +	return val;
> +}
> +
> +static void gmux_mmio_write8(struct apple_gmux_data *gmux_data, int port,
> +			      u8 val)
> +{
> +	mutex_lock(&gmux_data->index_lock);
> +	gmux_mmio_wait(gmux_data);
> +	iowrite8(val, gmux_data->iomem_base);
> +
> +	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +
> +	gmux_mmio_wait(gmux_data);
> +	mutex_unlock(&gmux_data->index_lock);
> +}
> +
> +static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
> +{
> +	u32 val;
> +
> +	mutex_lock(&gmux_data->index_lock);
> +	gmux_mmio_wait(gmux_data);
> +	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +	gmux_mmio_wait(gmux_data);
> +	val = be32_to_cpu(ioread32(gmux_data->iomem_base));
> +	mutex_unlock(&gmux_data->index_lock);
> +
> +	return val;
> +}
> +
> +static void gmux_mmio_write32(struct apple_gmux_data *gmux_data, int port,
> +			       u32 val)
> +{
> +	mutex_lock(&gmux_data->index_lock);
> +	iowrite32(cpu_to_be32(val), gmux_data->iomem_base);
> +	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +	gmux_mmio_wait(gmux_data);
> +	mutex_unlock(&gmux_data->index_lock);
> +}
> +
>  static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
>  {
>  	return gmux_data->config->read8(gmux_data, port);
> @@ -237,8 +313,8 @@ static void gmux_write32(struct apple_gmux_data *gmux_data, int port,
>   * the GPU. On dual GPU MacBook Pros by contrast, either GPU may be suspended
>   * to conserve energy. Hence the PWM signal needs to be generated by a separate
>   * backlight driver which is controlled by gmux. The earliest generation
> - * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. All newer models
> - * use a `TI LP8545`_.
> + * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. Newer models
> + * use a `TI LP8545`_ or a TI LP8548.
>   *
>   * .. _TI LP8543: https://www.ti.com/lit/ds/symlink/lp8543.pdf
>   * .. _TI LP8545: https://www.ti.com/lit/ds/symlink/lp8545.pdf
> @@ -302,8 +378,8 @@ static const struct backlight_ops gmux_bl_ops = {
>   * connecting it either to the discrete GPU or the Thunderbolt controller.
>   * Oddly enough, while the full port is no longer switchable, AUX and HPD
>   * are still switchable by way of an `NXP CBTL03062`_ (on pre-retinas
> - * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on retinas) under the
> - * control of gmux. Since the integrated GPU is missing the main link,
> + * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on pre-t2 retinas) under
> + * the control of gmux. Since the integrated GPU is missing the main link,
>   * external displays appear to it as phantoms which fail to link-train.
>   *
>   * gmux receives the HPD signal of all display connectors and sends an
> @@ -506,6 +582,20 @@ static const struct apple_gmux_config apple_gmux_index = {
>  	.name = "indexed"
>  };
>  
> +static const struct apple_gmux_config apple_gmux_mmio = {
> +	.read8 = &gmux_mmio_read8,
> +	.write8 = &gmux_mmio_write8,
> +	.read32 = &gmux_mmio_read32,
> +	.write32 = &gmux_mmio_write32,
> +	.gmux_handler = &gmux_handler_no_ddc,
> +	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
> +	.resource_type = IORESOURCE_MEM,
> +	.read_version_as_u32 = true,
> +	.use_acpi_gmsp = true,
> +	.name = "T2"
> +};
> +
> +
>  /**
>   * DOC: Interrupt
>   *
> @@ -637,6 +727,25 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	pnp_set_drvdata(pnp, gmux_data);
>  
>  	switch (type) {
> +	case APPLE_GMUX_TYPE_MMIO:
> +		gmux_data->config = &apple_gmux_mmio;
> +		mutex_init(&gmux_data->index_lock);
> +
> +		res = pnp_get_resource(pnp, IORESOURCE_MEM, 0);
> +		gmux_data->iostart = res->start;
> +		/* Although the ACPI table only allocates 8 bytes, we need 16. */
> +		gmux_data->iolen = 16;
> +		if (!request_mem_region(gmux_data->iostart, gmux_data->iolen,
> +					"Apple gmux")) {
> +			pr_err("gmux I/O already in use\n");
> +			goto err_free;
> +		}
> +		gmux_data->iomem_base = ioremap(gmux_data->iostart, gmux_data->iolen);
> +		if (!gmux_data->iomem_base) {
> +			pr_err("couldn't remap gmux mmio region");
> +			goto err_release;
> +		}
> +		goto get_version;
>  	case APPLE_GMUX_TYPE_INDEXED:
>  		gmux_data->config = &apple_gmux_index;
>  		mutex_init(&gmux_data->index_lock);
> @@ -656,6 +765,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		goto err_free;
>  	}
>  
> +get_version:
>  	if (gmux_data->config->read_version_as_u32) {
>  		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
>  		ver_major = (version >> 24) & 0xff;
> @@ -686,7 +796,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  					 gmux_data, &gmux_bl_ops, &props);
>  	if (IS_ERR(bdev)) {
>  		ret = PTR_ERR(bdev);
> -		goto err_release;
> +		goto err_unmap;
>  	}
>  
>  	gmux_data->bdev = bdev;
> @@ -753,7 +863,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	/*
>  	 * Retina MacBook Pros cannot switch the panel's AUX separately
>  	 * and need eDP pre-calibration. They are distinguishable from
> -	 * pre-retinas by having an "indexed" gmux.
> +	 * pre-retinas by having an "indexed" or "T2" gmux.
>  	 *
>  	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
>  	 */
> @@ -778,8 +888,14 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  					   &gmux_notify_handler);
>  err_notify:
>  	backlight_device_unregister(bdev);
> +err_unmap:
> +	if (gmux_data->iomem_base)
> +		iounmap(gmux_data->iomem_base);
>  err_release:
> -	release_region(gmux_data->iostart, gmux_data->iolen);
> +	if (gmux_data->config->resource_type == IORESOURCE_MEM)
> +		release_mem_region(gmux_data->iostart, gmux_data->iolen);
> +	else
> +		release_region(gmux_data->iostart, gmux_data->iolen);
>  err_free:
>  	kfree(gmux_data);
>  	return ret;
> @@ -800,7 +916,11 @@ static void gmux_remove(struct pnp_dev *pnp)
>  
>  	backlight_device_unregister(gmux_data->bdev);
>  
> -	release_region(gmux_data->iostart, gmux_data->iolen);
> +	if (gmux_data->iomem_base) {
> +		iounmap(gmux_data->iomem_base);
> +		release_mem_region(gmux_data->iostart, gmux_data->iolen);
> +	} else
> +		release_region(gmux_data->iostart, gmux_data->iolen);
>  	apple_gmux_data = NULL;
>  	kfree(gmux_data);
>  
> diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> index 5f658439f7f8..b7532f26b756 100644
> --- a/include/linux/apple-gmux.h
> +++ b/include/linux/apple-gmux.h
> @@ -34,11 +34,18 @@
>  #define GMUX_PORT_READ			0xd0
>  #define GMUX_PORT_WRITE			0xd4
>  
> +#define GMUX_MMIO_PORT_SELECT		0x0e
> +#define GMUX_MMIO_COMMAND_SEND		0x0f
> +
> +#define GMUX_MMIO_READ			0x00
> +#define GMUX_MMIO_WRITE			0x40
> +
>  #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
>  
>  enum apple_gmux_type {
>  	APPLE_GMUX_TYPE_PIO,
> -	APPLE_GMUX_TYPE_INDEXED
> +	APPLE_GMUX_TYPE_INDEXED,
> +	APPLE_GMUX_TYPE_MMIO
>  };

With my suggested change to patch 2/5 the - + for APPLE_GMUX_TYPE_INDEXED
will go away because the , is already there. Likewise please add a ,
after APPLE_GMUX_TYPE_MMIO in case we want to add more entries in
the future.

Otherwise this patch looks good to me.

Regards,

Hans


>  
>  #if IS_ENABLED(CONFIG_APPLE_GMUX)
> @@ -93,19 +100,24 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, enum apple_gmux_ty
>  	}
>  
>  	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
> -	if (!res || resource_size(res) < GMUX_MIN_IO_LEN)
> -		goto out;
> -
> -	/*
> -	 * Invalid version information may indicate either that the gmux
> -	 * device isn't present or that it's a new one that uses indexed io.
> -	 */
> -	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> -	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> -	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> -	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> -		if (apple_gmux_is_indexed(res->start))
> -			type = APPLE_GMUX_TYPE_INDEXED;
> +	if (res && resource_size(res) >= GMUX_MIN_IO_LEN) {
> +		/*
> +		 * Invalid version information may indicate either that the gmux
> +		 * device isn't present or that it's a new one that uses indexed io.
> +		 */
> +		ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> +		ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> +		ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> +		if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> +			if (apple_gmux_is_indexed(res->start))
> +				type = APPLE_GMUX_TYPE_INDEXED;
> +			else
> +				goto out;
> +		}
> +	} else {
> +		res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
> +		if (res)
> +			type = APPLE_GMUX_TYPE_MMIO;
>  		else
>  			goto out;
>  	}

Re: [PATCH v2 5/5] apple-gmux: add debugfs interface

[Hans de Goede]

Hi,

On 2/16/23 13:23, Orlando Chamberlain wrote:
> Allow reading and writing gmux ports from userspace.
> 
> For example:
> 
> echo 4 > /sys/kernel/debug/apple_gmux/selected_port
> cat /sys/kernel/debug/apple_gmux/selected_port_data | xxd -p
> 
> Will show the gmux version information (00000005 in this case)
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
> v1->v2: Use debugfs instead of sysfs.
>  drivers/platform/x86/apple-gmux.c | 88 +++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 5bac6dcfada0..e8a35d98b113 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -22,6 +22,7 @@
>  #include <linux/delay.h>
>  #include <linux/pci.h>
>  #include <linux/vga_switcheroo.h>
> +#include <linux/debugfs.h>
>  #include <asm/io.h>
>  
>  /**
> @@ -66,6 +67,10 @@ struct apple_gmux_data {
>  	enum vga_switcheroo_client_id switch_state_external;
>  	enum vga_switcheroo_state power_state;
>  	struct completion powerchange_done;
> +
> +	/* debugfs data */
> +	u8 selected_port;
> +	struct dentry *debug_dentry;
>  };
>  
>  static struct apple_gmux_data *apple_gmux_data;
> @@ -674,6 +679,87 @@ static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
>  		complete(&gmux_data->powerchange_done);
>  }
>  
> +/**
> + * DOC: Debugfs Interface
> + *
> + * gmux ports can be accessed from userspace as a debugfs interface. For example:
> + *
> + * # echo 4 > /sys/kernel/debug/apple_gmux/selected_port
> + * # cat /sys/kernel/debug/apple_gmux/selected_port_data | xxd -p
> + * 00000005
> + *
> + * Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
> + *
> + * 1 and 4 byte writes are also allowed.
> + */
> +
> +static ssize_t gmux_selected_port_data_write(struct file *file,
> +		const char __user *userbuf, size_t count, loff_t *ppos)
> +{
> +	struct apple_gmux_data *gmux_data = file->private_data;
> +	int ret;
> +
> +	if (*ppos)
> +		return -EINVAL;
> +
> +	if (count == 1) {
> +		u8 data;
> +
> +		ret = copy_from_user(&data, userbuf, 1);
> +		if (ret)
> +			return ret;
> +		gmux_write8(gmux_data, gmux_data->selected_port, data);
> +	} else if (count == 4) {
> +		u32 data;
> +
> +		ret = copy_from_user(&data, userbuf, 4);
> +		if (ret)
> +			return ret;
> +		gmux_write32(gmux_data, gmux_data->selected_port, data);
> +	} else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +static ssize_t gmux_selected_port_data_read(struct file *file,
> +		char __user *userbuf, size_t count, loff_t *ppos)
> +{
> +	struct apple_gmux_data *gmux_data = file->private_data;
> +	u32 data;
> +
> +	data = gmux_read32(gmux_data, gmux_data->selected_port);
> +
> +	return simple_read_from_buffer(userbuf, count, ppos, &data, sizeof(data));
> +}
> +
> +static const struct file_operations gmux_port_data_ops = {
> +	.open = simple_open,
> +	.write = gmux_selected_port_data_write,
> +	.read = gmux_selected_port_data_read
> +};
> +
> +static void gmux_init_debugfs(struct apple_gmux_data *gmux_data)
> +{
> +	struct dentry *debug_dentry;
> +
> +	debug_dentry = debugfs_create_dir(KBUILD_MODNAME, NULL);
> +
> +	if (IS_ERR(debug_dentry))
> +		return;

This error check is not necessary here. The debugfs_create_*
and debugfs_remove_recursive() functions will happily take
the ERR_PTR value and ignore it.

This is what I tried to say when I said that no error handling
is necessary with debugfs (by design).

Regards,

Hans


> +
> +	gmux_data->debug_dentry = debug_dentry;
> +
> +	debugfs_create_u8("selected_port", 0644, debug_dentry, &gmux_data->selected_port);
> +	debugfs_create_file("selected_port_data", 0644, debug_dentry,
> +			gmux_data, &gmux_port_data_ops);
> +}
> +
> +static void gmux_fini_debugfs(struct apple_gmux_data *gmux_data)
> +{
> +	debugfs_remove_recursive(gmux_data->debug_dentry);
> +}
> +
>  static int gmux_suspend(struct device *dev)
>  {
>  	struct pnp_dev *pnp = to_pnp_dev(dev);
> @@ -874,6 +960,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		goto err_register_handler;
>  	}
>  
> +	gmux_init_debugfs(gmux_data);
>  	return 0;
>  
>  err_register_handler:
> @@ -905,6 +992,7 @@ static void gmux_remove(struct pnp_dev *pnp)
>  {
>  	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
>  
> +	gmux_fini_debugfs(gmux_data);
>  	vga_switcheroo_unregister_handler();
>  	gmux_disable_interrupts(gmux_data);
>  	if (gmux_data->gpe >= 0) {

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Hans de Goede]

Hi,

On 2/16/23 13:23, Orlando Chamberlain wrote:
> In some newer dual gpu MacBooks, gmux is controlled by the T2 security
> chip, and acessed with MMIO. Add support for these gmux controllers
> 
> Interestingly, the ACPI table only allocates 8 bytes for GMUX, but we
> actually need 16, and as such we request 16 with request_mem_region.
> 
> Reading and writing from ports:
>     16 bytes from 0xfe0b0200 are used. 0x0 to 0x4 are where data
>     to read appears, and where data to write goes. Writing to 0xe
>     sets the gmux port being accessed, and writing to 0xf sends commands.
> 
>     These commands are 0x40 & data_length for write, and data_length for
>     read, where data_length is 1, 2 or 4. Once byte base+0xf is 0, the
>     command is done.
> 
> Issues:
>     As with other retina models, we can't switch DDC lines so
>     switching at runtime doesn't work if the inactive gpu driver
>     already disabled eDP due to it not being connected when that
>     driver loaded.
> 
>     Additionally, turning on the dgpu back on the MacBookPro16,1 does
>     not work.
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
> v1->v2: Document some chips present, and clarify which chips aren't
> present on MMIO gmux laptops.
>  drivers/platform/x86/apple-gmux.c | 142 +++++++++++++++++++++++++++---
>  include/linux/apple-gmux.h        |  40 ++++++---
>  2 files changed, 158 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 12a93fc49c36..5bac6dcfada0 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -28,15 +28,17 @@
>   * DOC: Overview
>   *
>   * gmux is a microcontroller built into the MacBook Pro to support dual GPUs:
> - * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on retinas.
> + * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on pre-T2 retinas.
> + * The chip used on T2 Macs is not known.
>   *
>   * (The MacPro6,1 2013 also has a gmux, however it is unclear why since it has
>   * dual GPUs but no built-in display.)
>   *
>   * gmux is connected to the LPC bus of the southbridge. Its I/O ports are
>   * accessed differently depending on the microcontroller: Driver functions
> - * to access a pre-retina gmux are infixed ``_pio_``, those for a retina gmux
> - * are infixed ``_index_``.
> + * to access a pre-retina gmux are infixed ``_pio_``, those for a pre-T2
> + * retina gmux are infixed ``_index_``, and those on T2 Macs are infixed
> + * with ``_mmio_``.
>   *
>   * .. _Lattice XP2:
>   *     http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
> @@ -47,6 +49,7 @@
>  struct apple_gmux_config;
>  
>  struct apple_gmux_data {
> +	u8 *__iomem iomem_base;
>  	unsigned long iostart;
>  	unsigned long iolen;
>  	const struct apple_gmux_config *config;
> @@ -209,6 +212,79 @@ static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
>  	mutex_unlock(&gmux_data->index_lock);
>  }
>  
> +static int gmux_mmio_wait(struct apple_gmux_data *gmux_data)
> +{
> +	int i = 200;
> +	u8 gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +
> +	while (i && gwr) {
> +		gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		udelay(100);
> +		i--;
> +	}
> +
> +	return !!i;
> +}
> +
> +static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
> +{
> +	u8 val;
> +
> +	mutex_lock(&gmux_data->index_lock);
> +	gmux_mmio_wait(gmux_data);
> +	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +	gmux_mmio_wait(gmux_data);
> +	val = ioread8(gmux_data->iomem_base);
> +	mutex_unlock(&gmux_data->index_lock);
> +
> +	return val;
> +}
> +
> +static void gmux_mmio_write8(struct apple_gmux_data *gmux_data, int port,
> +			      u8 val)
> +{
> +	mutex_lock(&gmux_data->index_lock);
> +	gmux_mmio_wait(gmux_data);
> +	iowrite8(val, gmux_data->iomem_base);
> +
> +	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +
> +	gmux_mmio_wait(gmux_data);
> +	mutex_unlock(&gmux_data->index_lock);
> +}
> +
> +static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
> +{
> +	u32 val;
> +
> +	mutex_lock(&gmux_data->index_lock);
> +	gmux_mmio_wait(gmux_data);
> +	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +	gmux_mmio_wait(gmux_data);
> +	val = be32_to_cpu(ioread32(gmux_data->iomem_base));
> +	mutex_unlock(&gmux_data->index_lock);
> +
> +	return val;
> +}
> +
> +static void gmux_mmio_write32(struct apple_gmux_data *gmux_data, int port,
> +			       u32 val)
> +{
> +	mutex_lock(&gmux_data->index_lock);
> +	iowrite32(cpu_to_be32(val), gmux_data->iomem_base);
> +	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +	gmux_mmio_wait(gmux_data);
> +	mutex_unlock(&gmux_data->index_lock);
> +}
> +
>  static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
>  {
>  	return gmux_data->config->read8(gmux_data, port);
> @@ -237,8 +313,8 @@ static void gmux_write32(struct apple_gmux_data *gmux_data, int port,
>   * the GPU. On dual GPU MacBook Pros by contrast, either GPU may be suspended
>   * to conserve energy. Hence the PWM signal needs to be generated by a separate
>   * backlight driver which is controlled by gmux. The earliest generation
> - * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. All newer models
> - * use a `TI LP8545`_.
> + * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. Newer models
> + * use a `TI LP8545`_ or a TI LP8548.
>   *
>   * .. _TI LP8543: https://www.ti.com/lit/ds/symlink/lp8543.pdf
>   * .. _TI LP8545: https://www.ti.com/lit/ds/symlink/lp8545.pdf
> @@ -302,8 +378,8 @@ static const struct backlight_ops gmux_bl_ops = {
>   * connecting it either to the discrete GPU or the Thunderbolt controller.
>   * Oddly enough, while the full port is no longer switchable, AUX and HPD
>   * are still switchable by way of an `NXP CBTL03062`_ (on pre-retinas
> - * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on retinas) under the
> - * control of gmux. Since the integrated GPU is missing the main link,
> + * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on pre-t2 retinas) under
> + * the control of gmux. Since the integrated GPU is missing the main link,
>   * external displays appear to it as phantoms which fail to link-train.
>   *
>   * gmux receives the HPD signal of all display connectors and sends an
> @@ -506,6 +582,20 @@ static const struct apple_gmux_config apple_gmux_index = {
>  	.name = "indexed"
>  };
>  
> +static const struct apple_gmux_config apple_gmux_mmio = {
> +	.read8 = &gmux_mmio_read8,
> +	.write8 = &gmux_mmio_write8,
> +	.read32 = &gmux_mmio_read32,
> +	.write32 = &gmux_mmio_write32,
> +	.gmux_handler = &gmux_handler_no_ddc,
> +	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
> +	.resource_type = IORESOURCE_MEM,
> +	.read_version_as_u32 = true,
> +	.use_acpi_gmsp = true,
> +	.name = "T2"
> +};
> +
> +
>  /**
>   * DOC: Interrupt
>   *
> @@ -637,6 +727,25 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	pnp_set_drvdata(pnp, gmux_data);
>  
>  	switch (type) {
> +	case APPLE_GMUX_TYPE_MMIO:
> +		gmux_data->config = &apple_gmux_mmio;
> +		mutex_init(&gmux_data->index_lock);
> +
> +		res = pnp_get_resource(pnp, IORESOURCE_MEM, 0);
> +		gmux_data->iostart = res->start;
> +		/* Although the ACPI table only allocates 8 bytes, we need 16. */
> +		gmux_data->iolen = 16;
> +		if (!request_mem_region(gmux_data->iostart, gmux_data->iolen,
> +					"Apple gmux")) {
> +			pr_err("gmux I/O already in use\n");
> +			goto err_free;
> +		}
> +		gmux_data->iomem_base = ioremap(gmux_data->iostart, gmux_data->iolen);
> +		if (!gmux_data->iomem_base) {
> +			pr_err("couldn't remap gmux mmio region");
> +			goto err_release;
> +		}
> +		goto get_version;
>  	case APPLE_GMUX_TYPE_INDEXED:
>  		gmux_data->config = &apple_gmux_index;
>  		mutex_init(&gmux_data->index_lock);
> @@ -656,6 +765,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		goto err_free;
>  	}
>  
> +get_version:
>  	if (gmux_data->config->read_version_as_u32) {
>  		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
>  		ver_major = (version >> 24) & 0xff;
> @@ -686,7 +796,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  					 gmux_data, &gmux_bl_ops, &props);
>  	if (IS_ERR(bdev)) {
>  		ret = PTR_ERR(bdev);
> -		goto err_release;
> +		goto err_unmap;
>  	}
>  
>  	gmux_data->bdev = bdev;
> @@ -753,7 +863,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	/*
>  	 * Retina MacBook Pros cannot switch the panel's AUX separately
>  	 * and need eDP pre-calibration. They are distinguishable from
> -	 * pre-retinas by having an "indexed" gmux.
> +	 * pre-retinas by having an "indexed" or "T2" gmux.
>  	 *
>  	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
>  	 */
> @@ -778,8 +888,14 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  					   &gmux_notify_handler);
>  err_notify:
>  	backlight_device_unregister(bdev);
> +err_unmap:
> +	if (gmux_data->iomem_base)
> +		iounmap(gmux_data->iomem_base);
>  err_release:
> -	release_region(gmux_data->iostart, gmux_data->iolen);
> +	if (gmux_data->config->resource_type == IORESOURCE_MEM)
> +		release_mem_region(gmux_data->iostart, gmux_data->iolen);
> +	else
> +		release_region(gmux_data->iostart, gmux_data->iolen);
>  err_free:
>  	kfree(gmux_data);
>  	return ret;
> @@ -800,7 +916,11 @@ static void gmux_remove(struct pnp_dev *pnp)
>  
>  	backlight_device_unregister(gmux_data->bdev);
>  
> -	release_region(gmux_data->iostart, gmux_data->iolen);
> +	if (gmux_data->iomem_base) {
> +		iounmap(gmux_data->iomem_base);
> +		release_mem_region(gmux_data->iostart, gmux_data->iolen);
> +	} else
> +		release_region(gmux_data->iostart, gmux_data->iolen);
>  	apple_gmux_data = NULL;
>  	kfree(gmux_data);
>  
> diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> index 5f658439f7f8..b7532f26b756 100644
> --- a/include/linux/apple-gmux.h
> +++ b/include/linux/apple-gmux.h
> @@ -34,11 +34,18 @@
>  #define GMUX_PORT_READ			0xd0
>  #define GMUX_PORT_WRITE			0xd4
>  
> +#define GMUX_MMIO_PORT_SELECT		0x0e
> +#define GMUX_MMIO_COMMAND_SEND		0x0f
> +
> +#define GMUX_MMIO_READ			0x00
> +#define GMUX_MMIO_WRITE			0x40
> +
>  #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
>  
>  enum apple_gmux_type {
>  	APPLE_GMUX_TYPE_PIO,
> -	APPLE_GMUX_TYPE_INDEXED
> +	APPLE_GMUX_TYPE_INDEXED,
> +	APPLE_GMUX_TYPE_MMIO
>  };
>  
>  #if IS_ENABLED(CONFIG_APPLE_GMUX)
> @@ -93,19 +100,24 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, enum apple_gmux_ty
>  	}
>  
>  	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
> -	if (!res || resource_size(res) < GMUX_MIN_IO_LEN)
> -		goto out;
> -
> -	/*
> -	 * Invalid version information may indicate either that the gmux
> -	 * device isn't present or that it's a new one that uses indexed io.
> -	 */
> -	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> -	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> -	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> -	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> -		if (apple_gmux_is_indexed(res->start))
> -			type = APPLE_GMUX_TYPE_INDEXED;
> +	if (res && resource_size(res) >= GMUX_MIN_IO_LEN) {
> +		/*
> +		 * Invalid version information may indicate either that the gmux
> +		 * device isn't present or that it's a new one that uses indexed io.
> +		 */
> +		ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> +		ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> +		ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> +		if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> +			if (apple_gmux_is_indexed(res->start))
> +				type = APPLE_GMUX_TYPE_INDEXED;
> +			else
> +				goto out;
> +		}
> +	} else {
> +		res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
> +		if (res)
> +			type = APPLE_GMUX_TYPE_MMIO;

Question are we not worried about MacBooks with an "APP000B"
ACPI device (with a value IORSOURCE_MEM entry) but which do not
actually have a gmux, because they are iGPU only ?

I have learned the hard way (through backlight control regressions
in 6.1) that at least some older model MacBooks with an IO resource
have an APP000B ACPI device without them actually having a gmux,
these get caught by the version check and then do not pass the
indexed check so that apple_gmux_detect() properly returns false.

Maybe make gmux_mmio_read32() a static inline inside
include/linux/apple-gmux.h and try to read the version here ?

Has this been tested on iGPU only T2 Macs?

Regards,

Hans

Re: [PATCH v2 2/5] apple-gmux: refactor gmux types

[Orlando Chamberlain]

On Thu, 16 Feb 2023 14:13:25 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> One small nit below.
> 
> On 2/16/23 13:23, Orlando Chamberlain wrote:
> > Add apple_gmux_config struct containing operations and data
> > specific to each mux type.
> > 
> > This is in preparation for adding a third, MMIO based, gmux type.
> > 
> > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > ---
> > v1->v2: Handle the two ways of reading the version as part of this
> > type system (read_version_as_u32).
> >  drivers/platform/x86/apple-gmux.c | 93
> > ++++++++++++++++++++----------- include/linux/apple-gmux.h        |
> > 18 ++++-- 2 files changed, 74 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c index
> > ec99e05e532c..36208e93d745 100644 ---
> > a/drivers/platform/x86/apple-gmux.c +++
> > b/drivers/platform/x86/apple-gmux.c @@ -5,6 +5,7 @@
> >   *  Copyright (C) Canonical Ltd. <seth.forshee@canonical.com>
> >   *  Copyright (C) 2010-2012 Andreas Heider <andreas@meetr.de>
> >   *  Copyright (C) 2015 Lukas Wunner <lukas@wunner.de>
> > + *  Copyright (C) 2023 Orlando Chamberlain
> > <orlandoch.dev@gmail.com> */
> >  
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > @@ -43,10 +44,12 @@
> >   *
> > http://www.renesas.com/products/mpumcu/h8s/h8s2100/h8s2113/index.jsp
> > */ 
> > +struct apple_gmux_config;
> > +
> >  struct apple_gmux_data {
> >  	unsigned long iostart;
> >  	unsigned long iolen;
> > -	bool indexed;
> > +	const struct apple_gmux_config *config;
> >  	struct mutex index_lock;
> >  
> >  	struct backlight_device *bdev;
> > @@ -64,6 +67,18 @@ struct apple_gmux_data {
> >  
> >  static struct apple_gmux_data *apple_gmux_data;
> >  
> > +struct apple_gmux_config {
> > +	u8 (*read8)(struct apple_gmux_data *gmux_data, int port);
> > +	void (*write8)(struct apple_gmux_data *gmux_data, int
> > port, u8 val);
> > +	u32 (*read32)(struct apple_gmux_data *gmux_data, int port);
> > +	void (*write32)(struct apple_gmux_data *gmux_data, int
> > port, u32 val);
> > +	const struct vga_switcheroo_handler *gmux_handler;
> > +	enum vga_switcheroo_handler_flags_t handler_flags;
> > +	unsigned long resource_type;
> > +	bool read_version_as_u32;
> > +	char *name;
> > +};
> > +
> >  #define GMUX_INTERRUPT_ENABLE		0xff
> >  #define GMUX_INTERRUPT_DISABLE		0x00
> >  
> > @@ -195,35 +210,23 @@ static void gmux_index_write32(struct
> > apple_gmux_data *gmux_data, int port, 
> >  static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
> >  {
> > -	if (gmux_data->indexed)
> > -		return gmux_index_read8(gmux_data, port);
> > -	else
> > -		return gmux_pio_read8(gmux_data, port);
> > +	return gmux_data->config->read8(gmux_data, port);
> >  }
> >  
> >  static void gmux_write8(struct apple_gmux_data *gmux_data, int
> > port, u8 val) {
> > -	if (gmux_data->indexed)
> > -		gmux_index_write8(gmux_data, port, val);
> > -	else
> > -		gmux_pio_write8(gmux_data, port, val);
> > +	return gmux_data->config->write8(gmux_data, port, val);
> >  }
> >  
> >  static u32 gmux_read32(struct apple_gmux_data *gmux_data, int port)
> >  {
> > -	if (gmux_data->indexed)
> > -		return gmux_index_read32(gmux_data, port);
> > -	else
> > -		return gmux_pio_read32(gmux_data, port);
> > +	return gmux_data->config->read32(gmux_data, port);
> >  }
> >  
> >  static void gmux_write32(struct apple_gmux_data *gmux_data, int
> > port, u32 val)
> >  {
> > -	if (gmux_data->indexed)
> > -		gmux_index_write32(gmux_data, port, val);
> > -	else
> > -		gmux_pio_write32(gmux_data, port, val);
> > +	return gmux_data->config->write32(gmux_data, port, val);
> >  }
> >  
> >  /**
> > @@ -463,19 +466,43 @@ static enum vga_switcheroo_client_id
> > gmux_get_client_id(struct pci_dev *pdev) return VGA_SWITCHEROO_DIS;
> >  }
> >  
> > -static const struct vga_switcheroo_handler gmux_handler_indexed = {
> > +static const struct vga_switcheroo_handler gmux_handler_no_ddc = {
> >  	.switchto = gmux_switchto,
> >  	.power_state = gmux_set_power_state,
> >  	.get_client_id = gmux_get_client_id,
> >  };
> >  
> > -static const struct vga_switcheroo_handler gmux_handler_classic = {
> > +static const struct vga_switcheroo_handler gmux_handler_ddc = {
> >  	.switchto = gmux_switchto,
> >  	.switch_ddc = gmux_switch_ddc,
> >  	.power_state = gmux_set_power_state,
> >  	.get_client_id = gmux_get_client_id,
> >  };
> >  
> > +static const struct apple_gmux_config apple_gmux_pio = {
> > +	.read8 = &gmux_pio_read8,
> > +	.write8 = &gmux_pio_write8,
> > +	.read32 = &gmux_pio_read32,
> > +	.write32 = &gmux_pio_write32,
> > +	.gmux_handler = &gmux_handler_ddc,
> > +	.handler_flags = VGA_SWITCHEROO_CAN_SWITCH_DDC,
> > +	.resource_type = IORESOURCE_IO,
> > +	.read_version_as_u32 = false,
> > +	.name = "classic"
> > +};
> > +
> > +static const struct apple_gmux_config apple_gmux_index = {
> > +	.read8 = &gmux_index_read8,
> > +	.write8 = &gmux_index_write8,
> > +	.read32 = &gmux_index_read32,
> > +	.write32 = &gmux_index_write32,
> > +	.gmux_handler = &gmux_handler_no_ddc,
> > +	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
> > +	.resource_type = IORESOURCE_IO,
> > +	.read_version_as_u32 = true,
> > +	.name = "indexed"
> > +};
> > +
> >  /**
> >   * DOC: Interrupt
> >   *
> > @@ -565,13 +592,13 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) int ret = -ENXIO;
> >  	acpi_status status;
> >  	unsigned long long gpe;
> > -	bool indexed = false;
> > +	enum apple_gmux_type type;
> >  	u32 version;
> >  
> >  	if (apple_gmux_data)
> >  		return -EBUSY;
> >  
> > -	if (!apple_gmux_detect(pnp, &indexed)) {
> > +	if (!apple_gmux_detect(pnp, &type)) {
> >  		pr_info("gmux device not present\n");
> >  		return -ENODEV;
> >  	}
> > @@ -581,6 +608,16 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) return -ENOMEM;
> >  	pnp_set_drvdata(pnp, gmux_data);
> >  
> > +	switch (type) {
> > +	case APPLE_GMUX_TYPE_INDEXED:
> > +		gmux_data->config = &apple_gmux_index;
> > +		mutex_init(&gmux_data->index_lock);
> > +		break;
> > +	case APPLE_GMUX_TYPE_PIO:
> > +		gmux_data->config = &apple_gmux_pio;
> > +		break;
> > +	}
> > +
> >  	res = pnp_get_resource(pnp, IORESOURCE_IO, 0);
> >  	gmux_data->iostart = res->start;
> >  	gmux_data->iolen = resource_size(res);
> > @@ -591,9 +628,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) goto err_free;
> >  	}
> >  
> > -	if (indexed) {
> > -		mutex_init(&gmux_data->index_lock);
> > -		gmux_data->indexed = true;
> > +	if (gmux_data->config->read_version_as_u32) {
> >  		version = gmux_read32(gmux_data,
> > GMUX_PORT_VERSION_MAJOR); ver_major = (version >> 24) & 0xff;
> >  		ver_minor = (version >> 16) & 0xff;
> > @@ -604,7 +639,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) ver_release = gmux_read8(gmux_data,
> > GMUX_PORT_VERSION_RELEASE); }
> >  	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major,
> > ver_minor,
> > -		ver_release, (gmux_data->indexed ? "indexed" :
> > "classic"));
> > +		ver_release, gmux_data->config->name);
> >  
> >  	memset(&props, 0, sizeof(props));
> >  	props.type = BACKLIGHT_PLATFORM;
> > @@ -694,12 +729,8 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) *
> >  	 * Pre-retina MacBook Pros can switch the panel's DDC
> > separately. */
> > -	if (gmux_data->indexed)
> > -		ret =
> > vga_switcheroo_register_handler(&gmux_handler_indexed,
> > -
> > VGA_SWITCHEROO_NEEDS_EDP_CONFIG);
> > -	else
> > -		ret =
> > vga_switcheroo_register_handler(&gmux_handler_classic,
> > -
> > VGA_SWITCHEROO_CAN_SWITCH_DDC);
> > +	ret =
> > vga_switcheroo_register_handler(gmux_data->config->gmux_handler,
> > +			gmux_data->config->handler_flags);
> >  	if (ret) {
> >  		pr_err("Failed to register vga_switcheroo
> > handler\n"); goto err_register_handler;
> > diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> > index 1f68b49bcd68..5f658439f7f8 100644
> > --- a/include/linux/apple-gmux.h
> > +++ b/include/linux/apple-gmux.h
> > @@ -36,6 +36,11 @@
> >  
> >  #define GMUX_MIN_IO_LEN
> > (GMUX_PORT_BRIGHTNESS + 4) 
> > +enum apple_gmux_type {
> > +	APPLE_GMUX_TYPE_PIO,
> > +	APPLE_GMUX_TYPE_INDEXED  
> 
> In the kernel with things like enum "values" or array initializers we
> typically add a , at the end of the last entry, to avoid needless
> churn when adding more entry.
> 
> The one exception is this when there is a special entry which
> marks the end of the array / enum. But that is not the case here.
> 

Thanks, I'll fix that up in v3.

> 
> > +};
> > +
> >  #if IS_ENABLED(CONFIG_APPLE_GMUX)
> >  static inline bool apple_gmux_is_indexed(unsigned long iostart)
> >  {
> > @@ -65,13 +70,13 @@ static inline bool
> > apple_gmux_is_indexed(unsigned long iostart)
> >   * Return: %true if a supported gmux ACPI device is detected and
> > the kernel
> >   * was configured with CONFIG_APPLE_GMUX, %false otherwise.
> >   */
> > -static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool
> > *indexed_ret) +static inline bool apple_gmux_detect(struct pnp_dev
> > *pnp_dev, enum apple_gmux_type *type_ret) {
> >  	u8 ver_major, ver_minor, ver_release;
> >  	struct device *dev = NULL;
> >  	struct acpi_device *adev;
> >  	struct resource *res;
> > -	bool indexed = false;
> > +	enum apple_gmux_type type = APPLE_GMUX_TYPE_PIO;
> >  	bool ret = false;
> >  
> >  	if (!pnp_dev) {
> > @@ -99,13 +104,14 @@ static inline bool apple_gmux_detect(struct
> > pnp_dev *pnp_dev, bool *indexed_ret) ver_minor = inb(res->start +
> > GMUX_PORT_VERSION_MINOR); ver_release = inb(res->start +
> > GMUX_PORT_VERSION_RELEASE); if (ver_major == 0xff && ver_minor ==
> > 0xff && ver_release == 0xff) {
> > -		indexed = apple_gmux_is_indexed(res->start);
> > -		if (!indexed)
> > +		if (apple_gmux_is_indexed(res->start))
> > +			type = APPLE_GMUX_TYPE_INDEXED;
> > +		else
> >  			goto out;
> >  	}
> >  
> > -	if (indexed_ret)
> > -		*indexed_ret = indexed;
> > +	if (type_ret)
> > +		*type_ret = type;
> >  
> >  	ret = true;
> >  out:  
> 

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Orlando Chamberlain]

On Thu, 16 Feb 2023 14:15:16 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/16/23 13:23, Orlando Chamberlain wrote:
> > In some newer dual gpu MacBooks, gmux is controlled by the T2
> > security chip, and acessed with MMIO. Add support for these gmux
> > controllers
> > 
> > Interestingly, the ACPI table only allocates 8 bytes for GMUX, but
> > we actually need 16, and as such we request 16 with
> > request_mem_region.
> > 
> > Reading and writing from ports:
> >     16 bytes from 0xfe0b0200 are used. 0x0 to 0x4 are where data
> >     to read appears, and where data to write goes. Writing to 0xe
> >     sets the gmux port being accessed, and writing to 0xf sends
> > commands.
> > 
> >     These commands are 0x40 & data_length for write, and
> > data_length for read, where data_length is 1, 2 or 4. Once byte
> > base+0xf is 0, the command is done.
> > 
> > Issues:
> >     As with other retina models, we can't switch DDC lines so
> >     switching at runtime doesn't work if the inactive gpu driver
> >     already disabled eDP due to it not being connected when that
> >     driver loaded.
> > 
> >     Additionally, turning on the dgpu back on the MacBookPro16,1
> > does not work.
> > 
> > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > ---
> > v1->v2: Document some chips present, and clarify which chips aren't
> > present on MMIO gmux laptops.
> >  drivers/platform/x86/apple-gmux.c | 142
> > +++++++++++++++++++++++++++--- include/linux/apple-gmux.h        |
> > 40 ++++++--- 2 files changed, 158 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c index
> > 12a93fc49c36..5bac6dcfada0 100644 ---
> > a/drivers/platform/x86/apple-gmux.c +++
> > b/drivers/platform/x86/apple-gmux.c @@ -28,15 +28,17 @@
> >   * DOC: Overview
> >   *
> >   * gmux is a microcontroller built into the MacBook Pro to support
> > dual GPUs:
> > - * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on
> > retinas.
> > + * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on pre-T2
> > retinas.
> > + * The chip used on T2 Macs is not known.
> >   *
> >   * (The MacPro6,1 2013 also has a gmux, however it is unclear why
> > since it has
> >   * dual GPUs but no built-in display.)
> >   *
> >   * gmux is connected to the LPC bus of the southbridge. Its I/O
> > ports are
> >   * accessed differently depending on the microcontroller: Driver
> > functions
> > - * to access a pre-retina gmux are infixed ``_pio_``, those for a
> > retina gmux
> > - * are infixed ``_index_``.
> > + * to access a pre-retina gmux are infixed ``_pio_``, those for a
> > pre-T2
> > + * retina gmux are infixed ``_index_``, and those on T2 Macs are
> > infixed
> > + * with ``_mmio_``.
> >   *
> >   * .. _Lattice XP2:
> >   *
> > http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
> > @@ -47,6 +49,7 @@ struct apple_gmux_config;
> >  
> >  struct apple_gmux_data {
> > +	u8 *__iomem iomem_base;
> >  	unsigned long iostart;
> >  	unsigned long iolen;
> >  	const struct apple_gmux_config *config;
> > @@ -209,6 +212,79 @@ static void gmux_index_write32(struct
> > apple_gmux_data *gmux_data, int port,
> > mutex_unlock(&gmux_data->index_lock); }
> >  
> > +static int gmux_mmio_wait(struct apple_gmux_data *gmux_data)
> > +{
> > +	int i = 200;
> > +	u8 gwr = ioread8(gmux_data->iomem_base +
> > GMUX_MMIO_COMMAND_SEND); +
> > +	while (i && gwr) {
> > +		gwr = ioread8(gmux_data->iomem_base +
> > GMUX_MMIO_COMMAND_SEND);
> > +		udelay(100);
> > +		i--;
> > +	}
> > +
> > +	return !!i;
> > +}
> > +
> > +static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int
> > port) +{
> > +	u8 val;
> > +
> > +	mutex_lock(&gmux_data->index_lock);
> > +	gmux_mmio_wait(gmux_data);
> > +	iowrite8((port & 0xff), gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +	gmux_mmio_wait(gmux_data);
> > +	val = ioread8(gmux_data->iomem_base);
> > +	mutex_unlock(&gmux_data->index_lock);
> > +
> > +	return val;
> > +}
> > +
> > +static void gmux_mmio_write8(struct apple_gmux_data *gmux_data,
> > int port,
> > +			      u8 val)
> > +{
> > +	mutex_lock(&gmux_data->index_lock);
> > +	gmux_mmio_wait(gmux_data);
> > +	iowrite8(val, gmux_data->iomem_base);
> > +
> > +	iowrite8(port & 0xff, gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +
> > +	gmux_mmio_wait(gmux_data);
> > +	mutex_unlock(&gmux_data->index_lock);
> > +}
> > +
> > +static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int
> > port) +{
> > +	u32 val;
> > +
> > +	mutex_lock(&gmux_data->index_lock);
> > +	gmux_mmio_wait(gmux_data);
> > +	iowrite8((port & 0xff), gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +	gmux_mmio_wait(gmux_data);
> > +	val = be32_to_cpu(ioread32(gmux_data->iomem_base));
> > +	mutex_unlock(&gmux_data->index_lock);
> > +
> > +	return val;
> > +}
> > +
> > +static void gmux_mmio_write32(struct apple_gmux_data *gmux_data,
> > int port,
> > +			       u32 val)
> > +{
> > +	mutex_lock(&gmux_data->index_lock);
> > +	iowrite32(cpu_to_be32(val), gmux_data->iomem_base);
> > +	iowrite8(port & 0xff, gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +	gmux_mmio_wait(gmux_data);
> > +	mutex_unlock(&gmux_data->index_lock);
> > +}
> > +
> >  static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
> >  {
> >  	return gmux_data->config->read8(gmux_data, port);
> > @@ -237,8 +313,8 @@ static void gmux_write32(struct apple_gmux_data
> > *gmux_data, int port,
> >   * the GPU. On dual GPU MacBook Pros by contrast, either GPU may
> > be suspended
> >   * to conserve energy. Hence the PWM signal needs to be generated
> > by a separate
> >   * backlight driver which is controlled by gmux. The earliest
> > generation
> > - * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. All newer
> > models
> > - * use a `TI LP8545`_.
> > + * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. Newer models
> > + * use a `TI LP8545`_ or a TI LP8548.
> >   *
> >   * .. _TI LP8543: https://www.ti.com/lit/ds/symlink/lp8543.pdf
> >   * .. _TI LP8545: https://www.ti.com/lit/ds/symlink/lp8545.pdf
> > @@ -302,8 +378,8 @@ static const struct backlight_ops gmux_bl_ops =
> > {
> >   * connecting it either to the discrete GPU or the Thunderbolt
> > controller.
> >   * Oddly enough, while the full port is no longer switchable, AUX
> > and HPD
> >   * are still switchable by way of an `NXP CBTL03062`_ (on
> > pre-retinas
> > - * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on retinas)
> > under the
> > - * control of gmux. Since the integrated GPU is missing the main
> > link,
> > + * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on pre-t2
> > retinas) under
> > + * the control of gmux. Since the integrated GPU is missing the
> > main link,
> >   * external displays appear to it as phantoms which fail to
> > link-train. *
> >   * gmux receives the HPD signal of all display connectors and
> > sends an @@ -506,6 +582,20 @@ static const struct apple_gmux_config
> > apple_gmux_index = { .name = "indexed"
> >  };
> >  
> > +static const struct apple_gmux_config apple_gmux_mmio = {
> > +	.read8 = &gmux_mmio_read8,
> > +	.write8 = &gmux_mmio_write8,
> > +	.read32 = &gmux_mmio_read32,
> > +	.write32 = &gmux_mmio_write32,
> > +	.gmux_handler = &gmux_handler_no_ddc,
> > +	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
> > +	.resource_type = IORESOURCE_MEM,
> > +	.read_version_as_u32 = true,
> > +	.use_acpi_gmsp = true,
> > +	.name = "T2"
> > +};
> > +
> > +
> >  /**
> >   * DOC: Interrupt
> >   *
> > @@ -637,6 +727,25 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) pnp_set_drvdata(pnp, gmux_data);
> >  
> >  	switch (type) {
> > +	case APPLE_GMUX_TYPE_MMIO:
> > +		gmux_data->config = &apple_gmux_mmio;
> > +		mutex_init(&gmux_data->index_lock);
> > +
> > +		res = pnp_get_resource(pnp, IORESOURCE_MEM, 0);
> > +		gmux_data->iostart = res->start;
> > +		/* Although the ACPI table only allocates 8 bytes,
> > we need 16. */
> > +		gmux_data->iolen = 16;
> > +		if (!request_mem_region(gmux_data->iostart,
> > gmux_data->iolen,
> > +					"Apple gmux")) {
> > +			pr_err("gmux I/O already in use\n");
> > +			goto err_free;
> > +		}
> > +		gmux_data->iomem_base =
> > ioremap(gmux_data->iostart, gmux_data->iolen);
> > +		if (!gmux_data->iomem_base) {
> > +			pr_err("couldn't remap gmux mmio region");
> > +			goto err_release;
> > +		}
> > +		goto get_version;
> >  	case APPLE_GMUX_TYPE_INDEXED:
> >  		gmux_data->config = &apple_gmux_index;
> >  		mutex_init(&gmux_data->index_lock);
> > @@ -656,6 +765,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) goto err_free;
> >  	}
> >  
> > +get_version:
> >  	if (gmux_data->config->read_version_as_u32) {
> >  		version = gmux_read32(gmux_data,
> > GMUX_PORT_VERSION_MAJOR); ver_major = (version >> 24) & 0xff;
> > @@ -686,7 +796,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) gmux_data, &gmux_bl_ops, &props);
> >  	if (IS_ERR(bdev)) {
> >  		ret = PTR_ERR(bdev);
> > -		goto err_release;
> > +		goto err_unmap;
> >  	}
> >  
> >  	gmux_data->bdev = bdev;
> > @@ -753,7 +863,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) /*
> >  	 * Retina MacBook Pros cannot switch the panel's AUX
> > separately
> >  	 * and need eDP pre-calibration. They are distinguishable
> > from
> > -	 * pre-retinas by having an "indexed" gmux.
> > +	 * pre-retinas by having an "indexed" or "T2" gmux.
> >  	 *
> >  	 * Pre-retina MacBook Pros can switch the panel's DDC
> > separately. */
> > @@ -778,8 +888,14 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) &gmux_notify_handler);
> >  err_notify:
> >  	backlight_device_unregister(bdev);
> > +err_unmap:
> > +	if (gmux_data->iomem_base)
> > +		iounmap(gmux_data->iomem_base);
> >  err_release:
> > -	release_region(gmux_data->iostart, gmux_data->iolen);
> > +	if (gmux_data->config->resource_type == IORESOURCE_MEM)
> > +		release_mem_region(gmux_data->iostart,
> > gmux_data->iolen);
> > +	else
> > +		release_region(gmux_data->iostart,
> > gmux_data->iolen); err_free:
> >  	kfree(gmux_data);
> >  	return ret;
> > @@ -800,7 +916,11 @@ static void gmux_remove(struct pnp_dev *pnp)
> >  
> >  	backlight_device_unregister(gmux_data->bdev);
> >  
> > -	release_region(gmux_data->iostart, gmux_data->iolen);
> > +	if (gmux_data->iomem_base) {
> > +		iounmap(gmux_data->iomem_base);
> > +		release_mem_region(gmux_data->iostart,
> > gmux_data->iolen);
> > +	} else
> > +		release_region(gmux_data->iostart,
> > gmux_data->iolen); apple_gmux_data = NULL;
> >  	kfree(gmux_data);
> >  
> > diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> > index 5f658439f7f8..b7532f26b756 100644
> > --- a/include/linux/apple-gmux.h
> > +++ b/include/linux/apple-gmux.h
> > @@ -34,11 +34,18 @@
> >  #define GMUX_PORT_READ			0xd0
> >  #define GMUX_PORT_WRITE			0xd4
> >  
> > +#define GMUX_MMIO_PORT_SELECT		0x0e
> > +#define GMUX_MMIO_COMMAND_SEND		0x0f
> > +
> > +#define GMUX_MMIO_READ			0x00
> > +#define GMUX_MMIO_WRITE			0x40
> > +
> >  #define GMUX_MIN_IO_LEN
> > (GMUX_PORT_BRIGHTNESS + 4) 
> >  enum apple_gmux_type {
> >  	APPLE_GMUX_TYPE_PIO,
> > -	APPLE_GMUX_TYPE_INDEXED
> > +	APPLE_GMUX_TYPE_INDEXED,
> > +	APPLE_GMUX_TYPE_MMIO
> >  };  
> 
> With my suggested change to patch 2/5 the - + for
> APPLE_GMUX_TYPE_INDEXED will go away because the , is already there.
> Likewise please add a , after APPLE_GMUX_TYPE_MMIO in case we want to
> add more entries in the future.

I've made those changes and will use them in v3.

> 
> Otherwise this patch looks good to me.
> 
> Regards,
> 
> Hans
> 
> 
> >  
> >  #if IS_ENABLED(CONFIG_APPLE_GMUX)
> > @@ -93,19 +100,24 @@ static inline bool apple_gmux_detect(struct
> > pnp_dev *pnp_dev, enum apple_gmux_ty }
> >  
> >  	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
> > -	if (!res || resource_size(res) < GMUX_MIN_IO_LEN)
> > -		goto out;
> > -
> > -	/*
> > -	 * Invalid version information may indicate either that
> > the gmux
> > -	 * device isn't present or that it's a new one that uses
> > indexed io.
> > -	 */
> > -	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> > -	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> > -	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> > -	if (ver_major == 0xff && ver_minor == 0xff && ver_release
> > == 0xff) {
> > -		if (apple_gmux_is_indexed(res->start))
> > -			type = APPLE_GMUX_TYPE_INDEXED;
> > +	if (res && resource_size(res) >= GMUX_MIN_IO_LEN) {
> > +		/*
> > +		 * Invalid version information may indicate either
> > that the gmux
> > +		 * device isn't present or that it's a new one
> > that uses indexed io.
> > +		 */
> > +		ver_major = inb(res->start +
> > GMUX_PORT_VERSION_MAJOR);
> > +		ver_minor = inb(res->start +
> > GMUX_PORT_VERSION_MINOR);
> > +		ver_release = inb(res->start +
> > GMUX_PORT_VERSION_RELEASE);
> > +		if (ver_major == 0xff && ver_minor == 0xff &&
> > ver_release == 0xff) {
> > +			if (apple_gmux_is_indexed(res->start))
> > +				type = APPLE_GMUX_TYPE_INDEXED;
> > +			else
> > +				goto out;
> > +		}
> > +	} else {
> > +		res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
> > +		if (res)
> > +			type = APPLE_GMUX_TYPE_MMIO;
> >  		else
> >  			goto out;
> >  	}  
> 

Re: [PATCH v2 5/5] apple-gmux: add debugfs interface

[Orlando Chamberlain]

On Thu, 16 Feb 2023 14:20:27 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/16/23 13:23, Orlando Chamberlain wrote:
> > Allow reading and writing gmux ports from userspace.
> > 
> > For example:
> > 
> > echo 4 > /sys/kernel/debug/apple_gmux/selected_port
> > cat /sys/kernel/debug/apple_gmux/selected_port_data | xxd -p
> > 
> > Will show the gmux version information (00000005 in this case)
> > 
> > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > ---
> > v1->v2: Use debugfs instead of sysfs.
> >  drivers/platform/x86/apple-gmux.c | 88
> > +++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c index
> > 5bac6dcfada0..e8a35d98b113 100644 ---
> > a/drivers/platform/x86/apple-gmux.c +++
> > b/drivers/platform/x86/apple-gmux.c @@ -22,6 +22,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/pci.h>
> >  #include <linux/vga_switcheroo.h>
> > +#include <linux/debugfs.h>
> >  #include <asm/io.h>
> >  
> >  /**
> > @@ -66,6 +67,10 @@ struct apple_gmux_data {
> >  	enum vga_switcheroo_client_id switch_state_external;
> >  	enum vga_switcheroo_state power_state;
> >  	struct completion powerchange_done;
> > +
> > +	/* debugfs data */
> > +	u8 selected_port;
> > +	struct dentry *debug_dentry;
> >  };
> >  
> >  static struct apple_gmux_data *apple_gmux_data;
> > @@ -674,6 +679,87 @@ static void gmux_notify_handler(acpi_handle
> > device, u32 value, void *context)
> > complete(&gmux_data->powerchange_done); }
> >  
> > +/**
> > + * DOC: Debugfs Interface
> > + *
> > + * gmux ports can be accessed from userspace as a debugfs
> > interface. For example:
> > + *
> > + * # echo 4 > /sys/kernel/debug/apple_gmux/selected_port
> > + * # cat /sys/kernel/debug/apple_gmux/selected_port_data | xxd -p
> > + * 00000005
> > + *
> > + * Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
> > + *
> > + * 1 and 4 byte writes are also allowed.
> > + */
> > +
> > +static ssize_t gmux_selected_port_data_write(struct file *file,
> > +		const char __user *userbuf, size_t count, loff_t
> > *ppos) +{
> > +	struct apple_gmux_data *gmux_data = file->private_data;
> > +	int ret;
> > +
> > +	if (*ppos)
> > +		return -EINVAL;
> > +
> > +	if (count == 1) {
> > +		u8 data;
> > +
> > +		ret = copy_from_user(&data, userbuf, 1);
> > +		if (ret)
> > +			return ret;
> > +		gmux_write8(gmux_data, gmux_data->selected_port,
> > data);
> > +	} else if (count == 4) {
> > +		u32 data;
> > +
> > +		ret = copy_from_user(&data, userbuf, 4);
> > +		if (ret)
> > +			return ret;
> > +		gmux_write32(gmux_data, gmux_data->selected_port,
> > data);
> > +	} else
> > +		return -EINVAL;
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t gmux_selected_port_data_read(struct file *file,
> > +		char __user *userbuf, size_t count, loff_t *ppos)
> > +{
> > +	struct apple_gmux_data *gmux_data = file->private_data;
> > +	u32 data;
> > +
> > +	data = gmux_read32(gmux_data, gmux_data->selected_port);
> > +
> > +	return simple_read_from_buffer(userbuf, count, ppos,
> > &data, sizeof(data)); +}
> > +
> > +static const struct file_operations gmux_port_data_ops = {
> > +	.open = simple_open,
> > +	.write = gmux_selected_port_data_write,
> > +	.read = gmux_selected_port_data_read
> > +};
> > +
> > +static void gmux_init_debugfs(struct apple_gmux_data *gmux_data)
> > +{
> > +	struct dentry *debug_dentry;
> > +
> > +	debug_dentry = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > +
> > +	if (IS_ERR(debug_dentry))
> > +		return;  
> 
> This error check is not necessary here. The debugfs_create_*
> and debugfs_remove_recursive() functions will happily take
> the ERR_PTR value and ignore it.
> 
> This is what I tried to say when I said that no error handling
> is necessary with debugfs (by design).
> 

I see, I'll simplify it to not check that in v3.

> Regards,
> 
> Hans
> 
> 
> > +
> > +	gmux_data->debug_dentry = debug_dentry;
> > +
> > +	debugfs_create_u8("selected_port", 0644, debug_dentry,
> > &gmux_data->selected_port);
> > +	debugfs_create_file("selected_port_data", 0644,
> > debug_dentry,
> > +			gmux_data, &gmux_port_data_ops);
> > +}
> > +
> > +static void gmux_fini_debugfs(struct apple_gmux_data *gmux_data)
> > +{
> > +	debugfs_remove_recursive(gmux_data->debug_dentry);
> > +}
> > +
> >  static int gmux_suspend(struct device *dev)
> >  {
> >  	struct pnp_dev *pnp = to_pnp_dev(dev);
> > @@ -874,6 +960,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) goto err_register_handler;
> >  	}
> >  
> > +	gmux_init_debugfs(gmux_data);
> >  	return 0;
> >  
> >  err_register_handler:
> > @@ -905,6 +992,7 @@ static void gmux_remove(struct pnp_dev *pnp)
> >  {
> >  	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
> >  
> > +	gmux_fini_debugfs(gmux_data);
> >  	vga_switcheroo_unregister_handler();
> >  	gmux_disable_interrupts(gmux_data);
> >  	if (gmux_data->gpe >= 0) {  
> 

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Orlando Chamberlain]

On Thu, 16 Feb 2023 14:27:13 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/16/23 13:23, Orlando Chamberlain wrote:
> > In some newer dual gpu MacBooks, gmux is controlled by the T2
> > security chip, and acessed with MMIO. Add support for these gmux
> > controllers
> > 
> > Interestingly, the ACPI table only allocates 8 bytes for GMUX, but
> > we actually need 16, and as such we request 16 with
> > request_mem_region.
> > 
> > Reading and writing from ports:
> >     16 bytes from 0xfe0b0200 are used. 0x0 to 0x4 are where data
> >     to read appears, and where data to write goes. Writing to 0xe
> >     sets the gmux port being accessed, and writing to 0xf sends
> > commands.
> > 
> >     These commands are 0x40 & data_length for write, and
> > data_length for read, where data_length is 1, 2 or 4. Once byte
> > base+0xf is 0, the command is done.
> > 
> > Issues:
> >     As with other retina models, we can't switch DDC lines so
> >     switching at runtime doesn't work if the inactive gpu driver
> >     already disabled eDP due to it not being connected when that
> >     driver loaded.
> > 
> >     Additionally, turning on the dgpu back on the MacBookPro16,1
> > does not work.
> > 
> > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > ---
> > v1->v2: Document some chips present, and clarify which chips aren't
> > present on MMIO gmux laptops.
> >  drivers/platform/x86/apple-gmux.c | 142
> > +++++++++++++++++++++++++++--- include/linux/apple-gmux.h        |
> > 40 ++++++--- 2 files changed, 158 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c index
> > 12a93fc49c36..5bac6dcfada0 100644 ---
> > a/drivers/platform/x86/apple-gmux.c +++
> > b/drivers/platform/x86/apple-gmux.c @@ -28,15 +28,17 @@
> >   * DOC: Overview
> >   *
> >   * gmux is a microcontroller built into the MacBook Pro to support
> > dual GPUs:
> > - * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on
> > retinas.
> > + * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on pre-T2
> > retinas.
> > + * The chip used on T2 Macs is not known.
> >   *
> >   * (The MacPro6,1 2013 also has a gmux, however it is unclear why
> > since it has
> >   * dual GPUs but no built-in display.)
> >   *
> >   * gmux is connected to the LPC bus of the southbridge. Its I/O
> > ports are
> >   * accessed differently depending on the microcontroller: Driver
> > functions
> > - * to access a pre-retina gmux are infixed ``_pio_``, those for a
> > retina gmux
> > - * are infixed ``_index_``.
> > + * to access a pre-retina gmux are infixed ``_pio_``, those for a
> > pre-T2
> > + * retina gmux are infixed ``_index_``, and those on T2 Macs are
> > infixed
> > + * with ``_mmio_``.
> >   *
> >   * .. _Lattice XP2:
> >   *
> > http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
> > @@ -47,6 +49,7 @@ struct apple_gmux_config;
> >  
> >  struct apple_gmux_data {
> > +	u8 *__iomem iomem_base;
> >  	unsigned long iostart;
> >  	unsigned long iolen;
> >  	const struct apple_gmux_config *config;
> > @@ -209,6 +212,79 @@ static void gmux_index_write32(struct
> > apple_gmux_data *gmux_data, int port,
> > mutex_unlock(&gmux_data->index_lock); }
> >  
> > +static int gmux_mmio_wait(struct apple_gmux_data *gmux_data)
> > +{
> > +	int i = 200;
> > +	u8 gwr = ioread8(gmux_data->iomem_base +
> > GMUX_MMIO_COMMAND_SEND); +
> > +	while (i && gwr) {
> > +		gwr = ioread8(gmux_data->iomem_base +
> > GMUX_MMIO_COMMAND_SEND);
> > +		udelay(100);
> > +		i--;
> > +	}
> > +
> > +	return !!i;
> > +}
> > +
> > +static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int
> > port) +{
> > +	u8 val;
> > +
> > +	mutex_lock(&gmux_data->index_lock);
> > +	gmux_mmio_wait(gmux_data);
> > +	iowrite8((port & 0xff), gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +	gmux_mmio_wait(gmux_data);
> > +	val = ioread8(gmux_data->iomem_base);
> > +	mutex_unlock(&gmux_data->index_lock);
> > +
> > +	return val;
> > +}
> > +
> > +static void gmux_mmio_write8(struct apple_gmux_data *gmux_data,
> > int port,
> > +			      u8 val)
> > +{
> > +	mutex_lock(&gmux_data->index_lock);
> > +	gmux_mmio_wait(gmux_data);
> > +	iowrite8(val, gmux_data->iomem_base);
> > +
> > +	iowrite8(port & 0xff, gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +
> > +	gmux_mmio_wait(gmux_data);
> > +	mutex_unlock(&gmux_data->index_lock);
> > +}
> > +
> > +static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int
> > port) +{
> > +	u32 val;
> > +
> > +	mutex_lock(&gmux_data->index_lock);
> > +	gmux_mmio_wait(gmux_data);
> > +	iowrite8((port & 0xff), gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_READ | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +	gmux_mmio_wait(gmux_data);
> > +	val = be32_to_cpu(ioread32(gmux_data->iomem_base));
> > +	mutex_unlock(&gmux_data->index_lock);
> > +
> > +	return val;
> > +}
> > +
> > +static void gmux_mmio_write32(struct apple_gmux_data *gmux_data,
> > int port,
> > +			       u32 val)
> > +{
> > +	mutex_lock(&gmux_data->index_lock);
> > +	iowrite32(cpu_to_be32(val), gmux_data->iomem_base);
> > +	iowrite8(port & 0xff, gmux_data->iomem_base +
> > GMUX_MMIO_PORT_SELECT);
> > +	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
> > +		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> > +	gmux_mmio_wait(gmux_data);
> > +	mutex_unlock(&gmux_data->index_lock);
> > +}
> > +
> >  static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
> >  {
> >  	return gmux_data->config->read8(gmux_data, port);
> > @@ -237,8 +313,8 @@ static void gmux_write32(struct apple_gmux_data
> > *gmux_data, int port,
> >   * the GPU. On dual GPU MacBook Pros by contrast, either GPU may
> > be suspended
> >   * to conserve energy. Hence the PWM signal needs to be generated
> > by a separate
> >   * backlight driver which is controlled by gmux. The earliest
> > generation
> > - * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. All newer
> > models
> > - * use a `TI LP8545`_.
> > + * MBP5 2008/09 uses a `TI LP8543`_ backlight driver. Newer models
> > + * use a `TI LP8545`_ or a TI LP8548.
> >   *
> >   * .. _TI LP8543: https://www.ti.com/lit/ds/symlink/lp8543.pdf
> >   * .. _TI LP8545: https://www.ti.com/lit/ds/symlink/lp8545.pdf
> > @@ -302,8 +378,8 @@ static const struct backlight_ops gmux_bl_ops =
> > {
> >   * connecting it either to the discrete GPU or the Thunderbolt
> > controller.
> >   * Oddly enough, while the full port is no longer switchable, AUX
> > and HPD
> >   * are still switchable by way of an `NXP CBTL03062`_ (on
> > pre-retinas
> > - * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on retinas)
> > under the
> > - * control of gmux. Since the integrated GPU is missing the main
> > link,
> > + * MBP8 2011 and MBP9 2012) or two `TI TS3DS10224`_ (on pre-t2
> > retinas) under
> > + * the control of gmux. Since the integrated GPU is missing the
> > main link,
> >   * external displays appear to it as phantoms which fail to
> > link-train. *
> >   * gmux receives the HPD signal of all display connectors and
> > sends an @@ -506,6 +582,20 @@ static const struct apple_gmux_config
> > apple_gmux_index = { .name = "indexed"
> >  };
> >  
> > +static const struct apple_gmux_config apple_gmux_mmio = {
> > +	.read8 = &gmux_mmio_read8,
> > +	.write8 = &gmux_mmio_write8,
> > +	.read32 = &gmux_mmio_read32,
> > +	.write32 = &gmux_mmio_write32,
> > +	.gmux_handler = &gmux_handler_no_ddc,
> > +	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
> > +	.resource_type = IORESOURCE_MEM,
> > +	.read_version_as_u32 = true,
> > +	.use_acpi_gmsp = true,
> > +	.name = "T2"
> > +};
> > +
> > +
> >  /**
> >   * DOC: Interrupt
> >   *
> > @@ -637,6 +727,25 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) pnp_set_drvdata(pnp, gmux_data);
> >  
> >  	switch (type) {
> > +	case APPLE_GMUX_TYPE_MMIO:
> > +		gmux_data->config = &apple_gmux_mmio;
> > +		mutex_init(&gmux_data->index_lock);
> > +
> > +		res = pnp_get_resource(pnp, IORESOURCE_MEM, 0);
> > +		gmux_data->iostart = res->start;
> > +		/* Although the ACPI table only allocates 8 bytes,
> > we need 16. */
> > +		gmux_data->iolen = 16;
> > +		if (!request_mem_region(gmux_data->iostart,
> > gmux_data->iolen,
> > +					"Apple gmux")) {
> > +			pr_err("gmux I/O already in use\n");
> > +			goto err_free;
> > +		}
> > +		gmux_data->iomem_base =
> > ioremap(gmux_data->iostart, gmux_data->iolen);
> > +		if (!gmux_data->iomem_base) {
> > +			pr_err("couldn't remap gmux mmio region");
> > +			goto err_release;
> > +		}
> > +		goto get_version;
> >  	case APPLE_GMUX_TYPE_INDEXED:
> >  		gmux_data->config = &apple_gmux_index;
> >  		mutex_init(&gmux_data->index_lock);
> > @@ -656,6 +765,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) goto err_free;
> >  	}
> >  
> > +get_version:
> >  	if (gmux_data->config->read_version_as_u32) {
> >  		version = gmux_read32(gmux_data,
> > GMUX_PORT_VERSION_MAJOR); ver_major = (version >> 24) & 0xff;
> > @@ -686,7 +796,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) gmux_data, &gmux_bl_ops, &props);
> >  	if (IS_ERR(bdev)) {
> >  		ret = PTR_ERR(bdev);
> > -		goto err_release;
> > +		goto err_unmap;
> >  	}
> >  
> >  	gmux_data->bdev = bdev;
> > @@ -753,7 +863,7 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) /*
> >  	 * Retina MacBook Pros cannot switch the panel's AUX
> > separately
> >  	 * and need eDP pre-calibration. They are distinguishable
> > from
> > -	 * pre-retinas by having an "indexed" gmux.
> > +	 * pre-retinas by having an "indexed" or "T2" gmux.
> >  	 *
> >  	 * Pre-retina MacBook Pros can switch the panel's DDC
> > separately. */
> > @@ -778,8 +888,14 @@ static int gmux_probe(struct pnp_dev *pnp,
> > const struct pnp_device_id *id) &gmux_notify_handler);
> >  err_notify:
> >  	backlight_device_unregister(bdev);
> > +err_unmap:
> > +	if (gmux_data->iomem_base)
> > +		iounmap(gmux_data->iomem_base);
> >  err_release:
> > -	release_region(gmux_data->iostart, gmux_data->iolen);
> > +	if (gmux_data->config->resource_type == IORESOURCE_MEM)
> > +		release_mem_region(gmux_data->iostart,
> > gmux_data->iolen);
> > +	else
> > +		release_region(gmux_data->iostart,
> > gmux_data->iolen); err_free:
> >  	kfree(gmux_data);
> >  	return ret;
> > @@ -800,7 +916,11 @@ static void gmux_remove(struct pnp_dev *pnp)
> >  
> >  	backlight_device_unregister(gmux_data->bdev);
> >  
> > -	release_region(gmux_data->iostart, gmux_data->iolen);
> > +	if (gmux_data->iomem_base) {
> > +		iounmap(gmux_data->iomem_base);
> > +		release_mem_region(gmux_data->iostart,
> > gmux_data->iolen);
> > +	} else
> > +		release_region(gmux_data->iostart,
> > gmux_data->iolen); apple_gmux_data = NULL;
> >  	kfree(gmux_data);
> >  
> > diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> > index 5f658439f7f8..b7532f26b756 100644
> > --- a/include/linux/apple-gmux.h
> > +++ b/include/linux/apple-gmux.h
> > @@ -34,11 +34,18 @@
> >  #define GMUX_PORT_READ			0xd0
> >  #define GMUX_PORT_WRITE			0xd4
> >  
> > +#define GMUX_MMIO_PORT_SELECT		0x0e
> > +#define GMUX_MMIO_COMMAND_SEND		0x0f
> > +
> > +#define GMUX_MMIO_READ			0x00
> > +#define GMUX_MMIO_WRITE			0x40
> > +
> >  #define GMUX_MIN_IO_LEN
> > (GMUX_PORT_BRIGHTNESS + 4) 
> >  enum apple_gmux_type {
> >  	APPLE_GMUX_TYPE_PIO,
> > -	APPLE_GMUX_TYPE_INDEXED
> > +	APPLE_GMUX_TYPE_INDEXED,
> > +	APPLE_GMUX_TYPE_MMIO
> >  };
> >  
> >  #if IS_ENABLED(CONFIG_APPLE_GMUX)
> > @@ -93,19 +100,24 @@ static inline bool apple_gmux_detect(struct
> > pnp_dev *pnp_dev, enum apple_gmux_ty }
> >  
> >  	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
> > -	if (!res || resource_size(res) < GMUX_MIN_IO_LEN)
> > -		goto out;
> > -
> > -	/*
> > -	 * Invalid version information may indicate either that
> > the gmux
> > -	 * device isn't present or that it's a new one that uses
> > indexed io.
> > -	 */
> > -	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> > -	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> > -	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> > -	if (ver_major == 0xff && ver_minor == 0xff && ver_release
> > == 0xff) {
> > -		if (apple_gmux_is_indexed(res->start))
> > -			type = APPLE_GMUX_TYPE_INDEXED;
> > +	if (res && resource_size(res) >= GMUX_MIN_IO_LEN) {
> > +		/*
> > +		 * Invalid version information may indicate either
> > that the gmux
> > +		 * device isn't present or that it's a new one
> > that uses indexed io.
> > +		 */
> > +		ver_major = inb(res->start +
> > GMUX_PORT_VERSION_MAJOR);
> > +		ver_minor = inb(res->start +
> > GMUX_PORT_VERSION_MINOR);
> > +		ver_release = inb(res->start +
> > GMUX_PORT_VERSION_RELEASE);
> > +		if (ver_major == 0xff && ver_minor == 0xff &&
> > ver_release == 0xff) {
> > +			if (apple_gmux_is_indexed(res->start))
> > +				type = APPLE_GMUX_TYPE_INDEXED;
> > +			else
> > +				goto out;
> > +		}
> > +	} else {
> > +		res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
> > +		if (res)
> > +			type = APPLE_GMUX_TYPE_MMIO;  
> 
> Question are we not worried about MacBooks with an "APP000B"
> ACPI device (with a value IORSOURCE_MEM entry) but which do not
> actually have a gmux, because they are iGPU only ?

It looks like iMac20,1, iMac20,2, and iMacPro1,1 have APP000B:

apple_gmux: Failed to find gmux I/O resource

iMac20,2: https://linux-hardware.org/?probe=ec2af584b3&log=dmesg
iMac20,1: https://linux-hardware.org/?probe=fee7644b9c&log=dmesg
iMacPro1,1: https://linux-hardware.org/?probe=6c26c9ff8c&log=dmesg

But I'm not sure if they actually have it or not. I'll see if I can get
people with those models to test if it's a real gmux. There does seem
to be a pattern in that those three all have AMD GPU's.

I've looked at dmesg or at least lsmod on all the models with the T2
chip and there wasn't evidence of any other models having that error or
having apple-gmux loaded on any models that shouldn't have a gmux,
other than the three mentioned above. Of course I don't know if its
possible for there to be firmware versions where this isn't the case.

> 
> I have learned the hard way (through backlight control regressions
> in 6.1) that at least some older model MacBooks with an IO resource
> have an APP000B ACPI device without them actually having a gmux,
> these get caught by the version check and then do not pass the
> indexed check so that apple_gmux_detect() properly returns false.
> 
> Maybe make gmux_mmio_read32() a static inline inside
> include/linux/apple-gmux.h and try to read the version here ?

For that would we need to ioremap() and iounmap()?
> 
> Has this been tested on iGPU only T2 Macs?

I don't think so. 

> 
> Regards,
> 
> Hans
> 
> 

Re: [PATCH v2 3/5] apple-gmux: Use GMSP acpi method for interrupt clear

[Orlando Chamberlain]

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
> 

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Orlando Chamberlain]

On Fri, 17 Feb 2023 11:05:31 +1100
Orlando Chamberlain <orlandoch.dev@gmail.com> wrote:
> > 
> > Question are we not worried about MacBooks with an "APP000B"
> > ACPI device (with a value IORSOURCE_MEM entry) but which do not
> > actually have a gmux, because they are iGPU only ?  
> 
> It looks like iMac20,1, iMac20,2, and iMacPro1,1 have APP000B:
> 
> apple_gmux: Failed to find gmux I/O resource
> 
> iMac20,2: https://linux-hardware.org/?probe=ec2af584b3&log=dmesg
> iMac20,1: https://linux-hardware.org/?probe=fee7644b9c&log=dmesg
> iMacPro1,1: https://linux-hardware.org/?probe=6c26c9ff8c&log=dmesg
> 
> But I'm not sure if they actually have it or not. I'll see if I can
> get people with those models to test if it's a real gmux. There does
> seem to be a pattern in that those three all have AMD GPU's.

Kerem Karabay managed to find the acpi tables and macOS's ioreg from and
iMacPro1,1:

https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/ACPI%20Tables/DSL/DSDT.dsl#L10423
https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/IORegistry/IOReg.txt#L5096

The DSDT table has the same APP000B device as MacBooks with actual gmux,
while the ioreg has no mention of Apple's driver AppleMuxControl2 being
used for that device.

I think that confirms Apple has not fixed the issue of putting
APP000B's where they don't need to.

Solutions to this I can think of are:

- Use DMI matching to ignore product_names "iMacPro1,1" "iMac20,1",
  "iMac20,2"
- Maybe check if the MMIO region for gmux is filled with 0xff*

*I don't know if this would work or not as I don't have a machine to
check with. On my machine everything surrounding the 16 bytes used for
gmux is 0xff:

# hexdump -n48 -C -s 0xfe0b01f0 /dev/mem
fe0b01f0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
fe0b0200  00 00 3e 4f 00 00 00 00  00 00 00 00 00 00 14 00  |..>O............|
fe0b0210  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|

so maybe on the iMacPro and iMac's, this would all be 0xff.

> 
> I've looked at dmesg or at least lsmod on all the models with the T2
> chip and there wasn't evidence of any other models having that error
> or having apple-gmux loaded on any models that shouldn't have a gmux,
> other than the three mentioned above. Of course I don't know if its
> possible for there to be firmware versions where this isn't the case.
> 
> > 
> > I have learned the hard way (through backlight control regressions
> > in 6.1) that at least some older model MacBooks with an IO resource
> > have an APP000B ACPI device without them actually having a gmux,
> > these get caught by the version check and then do not pass the
> > indexed check so that apple_gmux_detect() properly returns false.
> > 
> > Maybe make gmux_mmio_read32() a static inline inside
> > include/linux/apple-gmux.h and try to read the version here ?  
> 
> For that would we need to ioremap() and iounmap()?
> > 
> > Has this been tested on iGPU only T2 Macs?  
> 
> I don't think so. 
> 
> > 
> > Regards,
> > 
> > Hans
> > 
> >   
> 

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Hans de Goede]

Hi,

On 2/17/23 13:02, Orlando Chamberlain wrote:
> On Fri, 17 Feb 2023 11:05:31 +1100
> Orlando Chamberlain <orlandoch.dev@gmail.com> wrote:
>>>
>>> Question are we not worried about MacBooks with an "APP000B"
>>> ACPI device (with a value IORSOURCE_MEM entry) but which do not
>>> actually have a gmux, because they are iGPU only ?  
>>
>> It looks like iMac20,1, iMac20,2, and iMacPro1,1 have APP000B:
>>
>> apple_gmux: Failed to find gmux I/O resource
>>
>> iMac20,2: https://linux-hardware.org/?probe=ec2af584b3&log=dmesg
>> iMac20,1: https://linux-hardware.org/?probe=fee7644b9c&log=dmesg
>> iMacPro1,1: https://linux-hardware.org/?probe=6c26c9ff8c&log=dmesg
>>
>> But I'm not sure if they actually have it or not. I'll see if I can
>> get people with those models to test if it's a real gmux. There does
>> seem to be a pattern in that those three all have AMD GPU's.
> 
> Kerem Karabay managed to find the acpi tables and macOS's ioreg from and
> iMacPro1,1:
> 
> https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/ACPI%20Tables/DSL/DSDT.dsl#L10423
> https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/IORegistry/IOReg.txt#L5096
> 
> The DSDT table has the same APP000B device as MacBooks with actual gmux,
> while the ioreg has no mention of Apple's driver AppleMuxControl2 being
> used for that device.
> 
> I think that confirms Apple has not fixed the issue of putting
> APP000B's where they don't need to.
> 
> Solutions to this I can think of are:
> 
> - Use DMI matching to ignore product_names "iMacPro1,1" "iMac20,1",
>   "iMac20,2"
> - Maybe check if the MMIO region for gmux is filled with 0xff*
> 
> *I don't know if this would work or not as I don't have a machine to
> check with. On my machine everything surrounding the 16 bytes used for
> gmux is 0xff:
> 
> # hexdump -n48 -C -s 0xfe0b01f0 /dev/mem
> fe0b01f0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> fe0b0200  00 00 3e 4f 00 00 00 00  00 00 00 00 00 00 14 00  |..>O............|
> fe0b0210  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> 
> so maybe on the iMacPro and iMac's, this would all be 0xff.

Yes checking for a regular ioread32 returning 0xffffffff sounds
like it should work. Can you add a check for that in the next version
please ?  Note this means we still need to do an iomap + unmap as
you pointed out in another email, but I see no way around that.

Regards,

Hans




> 
>>
>> I've looked at dmesg or at least lsmod on all the models with the T2
>> chip and there wasn't evidence of any other models having that error
>> or having apple-gmux loaded on any models that shouldn't have a gmux,
>> other than the three mentioned above. Of course I don't know if its
>> possible for there to be firmware versions where this isn't the case.
>>
>>>
>>> I have learned the hard way (through backlight control regressions
>>> in 6.1) that at least some older model MacBooks with an IO resource
>>> have an APP000B ACPI device without them actually having a gmux,
>>> these get caught by the version check and then do not pass the
>>> indexed check so that apple_gmux_detect() properly returns false.
>>>
>>> Maybe make gmux_mmio_read32() a static inline inside
>>> include/linux/apple-gmux.h and try to read the version here ?  
>>
>> For that would we need to ioremap() and iounmap()?
>>>
>>> Has this been tested on iGPU only T2 Macs?  
>>
>> I don't think so. 
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>   
>>
> 

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Orlando Chamberlain]

On Sat, 18 Feb 2023 11:49:52 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/17/23 13:02, Orlando Chamberlain wrote:
> > On Fri, 17 Feb 2023 11:05:31 +1100
> > Orlando Chamberlain <orlandoch.dev@gmail.com> wrote:  
> >>>
> >>> Question are we not worried about MacBooks with an "APP000B"
> >>> ACPI device (with a value IORSOURCE_MEM entry) but which do not
> >>> actually have a gmux, because they are iGPU only ?    
> >>
> >> It looks like iMac20,1, iMac20,2, and iMacPro1,1 have APP000B:
> >>
> >> apple_gmux: Failed to find gmux I/O resource
> >>
> >> iMac20,2: https://linux-hardware.org/?probe=ec2af584b3&log=dmesg
> >> iMac20,1: https://linux-hardware.org/?probe=fee7644b9c&log=dmesg
> >> iMacPro1,1: https://linux-hardware.org/?probe=6c26c9ff8c&log=dmesg
> >>
> >> But I'm not sure if they actually have it or not. I'll see if I can
> >> get people with those models to test if it's a real gmux. There
> >> does seem to be a pattern in that those three all have AMD GPU's.  
> > 
> > Kerem Karabay managed to find the acpi tables and macOS's ioreg
> > from and iMacPro1,1:
> > 
> > https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/ACPI%20Tables/DSL/DSDT.dsl#L10423
> > https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/IORegistry/IOReg.txt#L5096
> > 
> > The DSDT table has the same APP000B device as MacBooks with actual
> > gmux, while the ioreg has no mention of Apple's driver
> > AppleMuxControl2 being used for that device.
> > 
> > I think that confirms Apple has not fixed the issue of putting
> > APP000B's where they don't need to.
> > 
> > Solutions to this I can think of are:
> > 
> > - Use DMI matching to ignore product_names "iMacPro1,1" "iMac20,1",
> >   "iMac20,2"
> > - Maybe check if the MMIO region for gmux is filled with 0xff*
> > 
> > *I don't know if this would work or not as I don't have a machine to
> > check with. On my machine everything surrounding the 16 bytes used
> > for gmux is 0xff:
> > 
> > # hexdump -n48 -C -s 0xfe0b01f0 /dev/mem
> > fe0b01f0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> > |................| fe0b0200  00 00 3e 4f 00 00 00 00  00 00 00 00
> > 00 00 14 00  |..>O............| fe0b0210  ff ff ff ff ff ff ff ff
> > ff ff ff ff ff ff ff ff  |................|
> > 
> > so maybe on the iMacPro and iMac's, this would all be 0xff.  
> 
> Yes checking for a regular ioread32 returning 0xffffffff sounds
> like it should work. Can you add a check for that in the next version
> please ?  Note this means we still need to do an iomap + unmap as
> you pointed out in another email, but I see no way around that.

I'll check that GMUX_MMIO_COMMAND_SEND (16th byte) is not 0xff, as if
the gmux is present it will reset that to 0x00, unless a command isn't
finished yet, in which case it will be one of 0x1, 0x4, 0x41, or 0x44.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> >   
> >>
> >> I've looked at dmesg or at least lsmod on all the models with the
> >> T2 chip and there wasn't evidence of any other models having that
> >> error or having apple-gmux loaded on any models that shouldn't
> >> have a gmux, other than the three mentioned above. Of course I
> >> don't know if its possible for there to be firmware versions where
> >> this isn't the case. 
> >>>
> >>> I have learned the hard way (through backlight control regressions
> >>> in 6.1) that at least some older model MacBooks with an IO
> >>> resource have an APP000B ACPI device without them actually having
> >>> a gmux, these get caught by the version check and then do not
> >>> pass the indexed check so that apple_gmux_detect() properly
> >>> returns false.
> >>>
> >>> Maybe make gmux_mmio_read32() a static inline inside
> >>> include/linux/apple-gmux.h and try to read the version here ?    
> >>
> >> For that would we need to ioremap() and iounmap()?  
> >>>
> >>> Has this been tested on iGPU only T2 Macs?    
> >>
> >> I don't think so. 
> >>  
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>>     
> >>  
> >   
> 

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Hans de Goede]

Hi,

On 2/18/23 13:52, Orlando Chamberlain wrote:
> On Sat, 18 Feb 2023 11:49:52 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 2/17/23 13:02, Orlando Chamberlain wrote:
>>> On Fri, 17 Feb 2023 11:05:31 +1100
>>> Orlando Chamberlain <orlandoch.dev@gmail.com> wrote:  
>>>>>
>>>>> Question are we not worried about MacBooks with an "APP000B"
>>>>> ACPI device (with a value IORSOURCE_MEM entry) but which do not
>>>>> actually have a gmux, because they are iGPU only ?    
>>>>
>>>> It looks like iMac20,1, iMac20,2, and iMacPro1,1 have APP000B:
>>>>
>>>> apple_gmux: Failed to find gmux I/O resource
>>>>
>>>> iMac20,2: https://linux-hardware.org/?probe=ec2af584b3&log=dmesg
>>>> iMac20,1: https://linux-hardware.org/?probe=fee7644b9c&log=dmesg
>>>> iMacPro1,1: https://linux-hardware.org/?probe=6c26c9ff8c&log=dmesg
>>>>
>>>> But I'm not sure if they actually have it or not. I'll see if I can
>>>> get people with those models to test if it's a real gmux. There
>>>> does seem to be a pattern in that those three all have AMD GPU's.  
>>>
>>> Kerem Karabay managed to find the acpi tables and macOS's ioreg
>>> from and iMacPro1,1:
>>>
>>> https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/ACPI%20Tables/DSL/DSDT.dsl#L10423
>>> https://github.com/khronokernel/DarwinDumped/blob/master/iMacPro/iMacPro1%2C1/Darwin%20Dumper/DarwinDumper_3.0.4_30.12_15.30.40_iMacPro1%2C1_Apple_X64_High%20Sierra_17C2120_apple/IORegistry/IOReg.txt#L5096
>>>
>>> The DSDT table has the same APP000B device as MacBooks with actual
>>> gmux, while the ioreg has no mention of Apple's driver
>>> AppleMuxControl2 being used for that device.
>>>
>>> I think that confirms Apple has not fixed the issue of putting
>>> APP000B's where they don't need to.
>>>
>>> Solutions to this I can think of are:
>>>
>>> - Use DMI matching to ignore product_names "iMacPro1,1" "iMac20,1",
>>>   "iMac20,2"
>>> - Maybe check if the MMIO region for gmux is filled with 0xff*
>>>
>>> *I don't know if this would work or not as I don't have a machine to
>>> check with. On my machine everything surrounding the 16 bytes used
>>> for gmux is 0xff:
>>>
>>> # hexdump -n48 -C -s 0xfe0b01f0 /dev/mem
>>> fe0b01f0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>>> |................| fe0b0200  00 00 3e 4f 00 00 00 00  00 00 00 00
>>> 00 00 14 00  |..>O............| fe0b0210  ff ff ff ff ff ff ff ff
>>> ff ff ff ff ff ff ff ff  |................|
>>>
>>> so maybe on the iMacPro and iMac's, this would all be 0xff.  
>>
>> Yes checking for a regular ioread32 returning 0xffffffff sounds
>> like it should work. Can you add a check for that in the next version
>> please ?  Note this means we still need to do an iomap + unmap as
>> you pointed out in another email, but I see no way around that.
> 
> I'll check that GMUX_MMIO_COMMAND_SEND (16th byte) is not 0xff, as if
> the gmux is present it will reset that to 0x00, unless a command isn't
> finished yet, in which case it will be one of 0x1, 0x4, 0x41, or 0x44.

Ok, this sounds good to me, thanks.

Regards,

Hans



>>>> I've looked at dmesg or at least lsmod on all the models with the
>>>> T2 chip and there wasn't evidence of any other models having that
>>>> error or having apple-gmux loaded on any models that shouldn't
>>>> have a gmux, other than the three mentioned above. Of course I
>>>> don't know if its possible for there to be firmware versions where
>>>> this isn't the case. 
>>>>>
>>>>> I have learned the hard way (through backlight control regressions
>>>>> in 6.1) that at least some older model MacBooks with an IO
>>>>> resource have an APP000B ACPI device without them actually having
>>>>> a gmux, these get caught by the version check and then do not
>>>>> pass the indexed check so that apple_gmux_detect() properly
>>>>> returns false.
>>>>>
>>>>> Maybe make gmux_mmio_read32() a static inline inside
>>>>> include/linux/apple-gmux.h and try to read the version here ?    
>>>>
>>>> For that would we need to ioremap() and iounmap()?  
>>>>>
>>>>> Has this been tested on iGPU only T2 Macs?    
>>>>
>>>> I don't think so. 
>>>>  
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>>
>>>>>     
>>>>  
>>>   
>>
> 

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Lukas Wunner]

On Fri, Feb 17, 2023 at 11:05:31AM +1100, Orlando Chamberlain wrote:
> On Thu, 16 Feb 2023 14:27:13 +0100 Hans de Goede <hdegoede@redhat.com> wrote:
> It looks like iMac20,1, iMac20,2, and iMacPro1,1 have APP000B:
> 
> apple_gmux: Failed to find gmux I/O resource
> 
> iMac20,2: https://linux-hardware.org/?probe=ec2af584b3&log=dmesg
> iMac20,1: https://linux-hardware.org/?probe=fee7644b9c&log=dmesg
> iMacPro1,1: https://linux-hardware.org/?probe=6c26c9ff8c&log=dmesg
> 
> But I'm not sure if they actually have it or not.

A number of iMacs support Target Display Mode, i.e. you can plug in
an external computer to the iMac's DisplayPort and use the iMac as
its screen.  Those iMac models do contain a gmux to mux the display
between internal GPU and external DisplayPort connection.  Linux
does not have support for this, sadly.  It would require generalizing
vga_switcheroo for use cases beyond dual GPU laptops.  Florian Echtler
has been looking into Target Display Mode but I'm not sure he got it
working:

https://lore.kernel.org/all/e6b44662-8b05-fb8d-660e-77153680d11b@butterbrot.org/
https://lore.kernel.org/all/71a48bf5-54b5-6d7b-d574-e0aa6c1acb80@butterbrot.org/
https://lore.kernel.org/all/1497603751-22360-1-git-send-email-floe@butterbrot.org/

I believe the Mac Pro (the trashcan one) also contains a gmux and
an APP000B device in DSDT.  I recall seeing a bug report due to a
splat in the gmux driver on that machine.  Back then I confirmed
in the schematics that it does contain a gmux, though I think it's
only used for brightness, not GPU switching.

Thanks,

Lukas

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Hans de Goede]

Hi,

On 2/19/23 14:39, Lukas Wunner wrote:
> On Fri, Feb 17, 2023 at 11:05:31AM +1100, Orlando Chamberlain wrote:
>> On Thu, 16 Feb 2023 14:27:13 +0100 Hans de Goede <hdegoede@redhat.com> wrote:
>> It looks like iMac20,1, iMac20,2, and iMacPro1,1 have APP000B:
>>
>> apple_gmux: Failed to find gmux I/O resource
>>
>> iMac20,2: https://linux-hardware.org/?probe=ec2af584b3&log=dmesg
>> iMac20,1: https://linux-hardware.org/?probe=fee7644b9c&log=dmesg
>> iMacPro1,1: https://linux-hardware.org/?probe=6c26c9ff8c&log=dmesg
>>
>> But I'm not sure if they actually have it or not.
> 
> A number of iMacs support Target Display Mode, i.e. you can plug in
> an external computer to the iMac's DisplayPort and use the iMac as
> its screen.  Those iMac models do contain a gmux to mux the display
> between internal GPU and external DisplayPort connection.  Linux
> does not have support for this, sadly.  It would require generalizing
> vga_switcheroo for use cases beyond dual GPU laptops.  Florian Echtler
> has been looking into Target Display Mode but I'm not sure he got it
> working:
> 
> https://lore.kernel.org/all/e6b44662-8b05-fb8d-660e-77153680d11b@butterbrot.org/
> https://lore.kernel.org/all/71a48bf5-54b5-6d7b-d574-e0aa6c1acb80@butterbrot.org/
> https://lore.kernel.org/all/1497603751-22360-1-git-send-email-floe@butterbrot.org/
> 
> I believe the Mac Pro (the trashcan one) also contains a gmux and
> an APP000B device in DSDT.  I recall seeing a bug report due to a
> splat in the gmux driver on that machine.  Back then I confirmed
> in the schematics that it does contain a gmux, though I think it's
> only used for brightness, not GPU switching.

Erm, the Mac Pro (the trashcan one) does not have an internal LCD
panel, right?  So I don't think the gmux will be used for brightness
control there ...

Regards,

Hans

Re: [PATCH v2 4/5] apple-gmux: support MMIO gmux on T2 Macs

[Lukas Wunner]

On Mon, Feb 20, 2023 at 09:44:54AM +0100, Hans de Goede wrote:
> On 2/19/23 14:39, Lukas Wunner wrote:
> > I believe the Mac Pro (the trashcan one) also contains a gmux and
> > an APP000B device in DSDT.  I recall seeing a bug report due to a
> > splat in the gmux driver on that machine.  Back then I confirmed
> > in the schematics that it does contain a gmux, though I think it's
> > only used for brightness, not GPU switching.
> 
> Erm, the Mac Pro (the trashcan one) does not have an internal LCD
> panel, right?  So I don't think the gmux will be used for brightness
> control there ...

Right.  I see now that I even added a comment to apple-gmux.c on that
Mac Pro, I couldn't figure out back then what the gmux was for:

 * (The MacPro6,1 2013 also has a gmux, however it is unclear why since it has
 * dual GPUs but no built-in display.)

Thanks,

Lukas