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

  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(-)
  

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;
>  	}
  

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
  

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;
> >  	}  
>
  

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
> 
>
  

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
> > 
> >   
>
  

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
>>>
>>>   
>>
>
  

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
> >>>
> >>>     
> >>  
> >   
>
  

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
>>>>>
>>>>>     
>>>>  
>>>   
>>
>
  

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
  

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
  

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