[PATCH v3 0/2] ACPI serdev support

[PATCH v3 0/2] ACPI serdev support

[Frédéric Danis]

Add ACPI support for serial attached devices.

Currently, serial devices are not set as enumerated during ACPI scan for SPI
or i2c buses (but not for UART). This should also be done for UART serial
devices.
I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.

This needs Johan Hovold's "serdev: fix registration of second slave" patch.

Tested on T100TA with Broadcom BCM2E39.

Since v2:
  - Remove ctrl->serdev set to NULL in acpi_serdev_register_device() in favor
    of Johan's patch
  - Fallback to ctrl->serdev check when acpi_walk_namespace() returns an error
    to prevent memory leak
  - Remove a change in dev_dbg() call in serdev_controller_add(), this will
    be done in separate patch
Since v1:
  - Check if a serdev device as been allocated during acpi_walk_namespace() to
    prevent serdev controller registration instead of the tty-class device.
  - Reword dev_dbg() strings replacing Serial by serdev
  - Removing redundant "serdev%d" in dev_dbg() calls in serdev_controller_add()
Since RFC:
  - Add or reword commit messages
  - Rename *serial_slave* to *serial_bus_slave*
  - Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
    Lukas Wunner
  - Update comment in acpi_default_enumeration()
  - Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39"
    in favor of patches from Hans de Goede

Frédéric Danis (2):
  serdev: Add ACPI support
  ACPI / scan: Fix enumeration for special UART devices

 drivers/acpi/scan.c       |  37 ++++++++---------
 drivers/tty/serdev/core.c | 100 +++++++++++++++++++++++++++++++++++++++++++---
 include/acpi/acpi_bus.h   |   2 +-
 3 files changed, 113 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] serdev: Add ACPI support

[Frédéric Danis]

This patch allows SerDev module to manage serial devices declared as
attached to an UART in ACPI table.

acpi_serdev_add_device() callback will only take into account entries
without enumerated flag set. This flags is set for all entries during
ACPI scan, except for SPI and I2C serial devices, and for UART with
2nd patch in the series.

Check if a serdev device as been allocated during acpi_walk_namespace()
to prevent serdev controller registration instead of the tty-class device.

Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
---
 drivers/tty/serdev/core.c | 100 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 95 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c68fb3a..ec113e3 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -14,6 +14,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/errno.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
@@ -49,13 +50,22 @@ static const struct device_type serdev_ctrl_type = {
 
 static int serdev_device_match(struct device *dev, struct device_driver *drv)
 {
-	/* TODO: ACPI and platform matching */
+	/* TODO: platform matching */
+	if (acpi_driver_match_device(dev, drv))
+		return 1;
+
 	return of_driver_match_device(dev, drv);
 }
 
 static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	/* TODO: ACPI and platform modalias */
+	int rc;
+
+	/* TODO: platform modalias */
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
+
 	return of_device_uevent_modalias(dev, env);
 }
 
@@ -260,6 +270,12 @@ static int serdev_drv_remove(struct device *dev)
 static ssize_t modalias_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
+	if (len != -ENODEV)
+		return len;
+
 	return of_device_modalias(dev, buf, PAGE_SIZE);
 }
 DEVICE_ATTR_RO(modalias);
@@ -385,6 +401,75 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+					    struct acpi_device *adev)
+{
+	struct serdev_device *serdev = NULL;
+	int err;
+
+	if (acpi_bus_get_status(adev) || !adev->status.present ||
+	    acpi_device_enumerated(adev))
+		return AE_OK;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev) {
+		dev_err(&ctrl->dev, "failed to allocate serdev device for %s\n",
+			dev_name(&adev->dev));
+		return AE_NO_MEMORY;
+	}
+
+	ACPI_COMPANION_SET(&serdev->dev, adev);
+	acpi_device_set_enumerated(adev);
+
+	err = serdev_device_add(serdev);
+	if (err) {
+		dev_err(&serdev->dev,
+			"failure adding ACPI serdev device. status %d\n", err);
+		serdev_device_put(serdev);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+				       void *data, void **return_value)
+{
+	struct serdev_controller *ctrl = data;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		return AE_OK;
+
+	return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	acpi_status status;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(ctrl->dev.parent);
+	if (!handle)
+		return -ENODEV;
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+				     acpi_serdev_add_device, NULL, ctrl, NULL);
+	if (ACPI_FAILURE(status))
+		dev_dbg(&ctrl->dev, "failed to enumerate serdev slaves\n");
+
+	if (!ctrl->serdev)
+		return -ENODEV;
+
+	return 0;
+}
+#else
+static inline int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_ACPI */
+
 /**
  * serdev_controller_add() - Add an serdev controller
  * @ctrl:	controller to be registered.
@@ -394,7 +479,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
  */
 int serdev_controller_add(struct serdev_controller *ctrl)
 {
-	int ret;
+	int ret_of, ret_acpi, ret;
 
 	/* Can't register until after driver model init */
 	if (WARN_ON(!is_registered))
@@ -404,9 +489,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
 	if (ret)
 		return ret;
 
-	ret = of_serdev_register_devices(ctrl);
-	if (ret)
+	ret_of = of_serdev_register_devices(ctrl);
+	ret_acpi = acpi_serdev_register_devices(ctrl);
+	if (ret_of && ret_acpi) {
+		dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
+			ret_of, ret_acpi);
+		ret = -ENODEV;
 		goto out_dev_del;
+	}
 
 	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
 		ctrl->nr, &ctrl->dev);
-- 
2.7.4

Re: [PATCH v3 1/2] serdev: Add ACPI support

[Johan Hovold]

On Wed, Oct 11, 2017 at 10:32:13AM +0200, Frédéric Danis wrote:
> This patch allows SerDev module to manage serial devices declared as
> attached to an UART in ACPI table.
> 
> acpi_serdev_add_device() callback will only take into account entries
> without enumerated flag set. This flags is set for all entries during
> ACPI scan, except for SPI and I2C serial devices, and for UART with
> 2nd patch in the series.
> 
> Check if a serdev device as been allocated during acpi_walk_namespace()
> to prevent serdev controller registration instead of the tty-class device.
> 
> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reviewed-by: Johan Hovold <johan@kernel.org>

Thanks for doing this work.

Johan

[PATCH v3 2/2] ACPI / scan: Fix enumeration for special UART devices

[Frédéric Danis]

UART devices is expected to be enumerated by SerDev subsystem.

During ACPI scan, serial devices behind SPI, I2C or UART buses are not
enumerated, allowing them to be enumerated by their respective parents.

Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
devices on serial buses (SPI, I2C or UART).

On Macs an empty ResourceTemplate is returned for uart slaves.
Instead the device properties "baud", "parity", "dataBits", "stopBits" are
provided. Add a check for "baud" in acpi_is_serial_bus_slave().

Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/acpi/scan.c     | 37 +++++++++++++++++--------------------
 include/acpi/acpi_bus.h |  2 +-
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 602f8ff..860b698 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1505,41 +1505,38 @@ static void acpi_init_coherency(struct acpi_device *adev)
 	adev->flags.coherent_dma = cca;
 }
 
-static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
+static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
 {
-	bool *is_spi_i2c_slave_p = data;
+	bool *is_serial_bus_slave_p = data;
 
 	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
 		return 1;
 
-	/*
-	 * devices that are connected to UART still need to be enumerated to
-	 * platform bus
-	 */
-	if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
-		*is_spi_i2c_slave_p = true;
+	*is_serial_bus_slave_p = true;
 
 	 /* no need to do more checking */
 	return -1;
 }
 
-static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
+static bool acpi_is_serial_bus_slave(struct acpi_device *device)
 {
 	struct list_head resource_list;
-	bool is_spi_i2c_slave = false;
+	bool is_serial_bus_slave = false;
 
 	/* Macs use device properties in lieu of _CRS resources */
 	if (x86_apple_machine &&
 	    (fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
-	     fwnode_property_present(&device->fwnode, "i2cAddress")))
+	     fwnode_property_present(&device->fwnode, "i2cAddress") ||
+	     fwnode_property_present(&device->fwnode, "baud")))
 		return true;
 
 	INIT_LIST_HEAD(&resource_list);
-	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
-			       &is_spi_i2c_slave);
+	acpi_dev_get_resources(device, &resource_list,
+			       acpi_check_serial_bus_slave,
+			       &is_serial_bus_slave);
 	acpi_dev_free_resource_list(&resource_list);
 
-	return is_spi_i2c_slave;
+	return is_serial_bus_slave;
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
@@ -1557,7 +1554,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
 	device->flags.initialized = true;
-	device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
+	device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
 	acpi_device_clear_enumerated(device);
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
@@ -1841,10 +1838,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
 static void acpi_default_enumeration(struct acpi_device *device)
 {
 	/*
-	 * Do not enumerate SPI/I2C slaves as they will be enumerated by their
-	 * respective parents.
+	 * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
+	 * their respective parents.
 	 */
-	if (!device->flags.spi_i2c_slave) {
+	if (!device->flags.serial_bus_slave) {
 		acpi_create_platform_device(device, NULL);
 		acpi_device_set_enumerated(device);
 	} else {
@@ -1941,7 +1938,7 @@ static void acpi_bus_attach(struct acpi_device *device)
 		return;
 
 	device->flags.match_driver = true;
-	if (ret > 0 && !device->flags.spi_i2c_slave) {
+	if (ret > 0 && !device->flags.serial_bus_slave) {
 		acpi_device_set_enumerated(device);
 		goto ok;
 	}
@@ -1950,7 +1947,7 @@ static void acpi_bus_attach(struct acpi_device *device)
 	if (ret < 0)
 		return;
 
-	if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
+	if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
 		acpi_device_set_enumerated(device);
 	else
 		acpi_default_enumeration(device);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index fa15052..f849be2 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -211,7 +211,7 @@ struct acpi_device_flags {
 	u32 of_compatible_ok:1;
 	u32 coherent_dma:1;
 	u32 cca_seen:1;
-	u32 spi_i2c_slave:1;
+	u32 serial_bus_slave:1;
 	u32 reserved:19;
 };
 
-- 
2.7.4

Re: [PATCH v3 2/2] ACPI / scan: Fix enumeration for special UART devices

[Lukas Wunner]

On Wed, Oct 11, 2017 at 10:32:14AM +0200, Frédéric Danis wrote:
> UART devices is expected to be enumerated by SerDev subsystem.
> 
> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> enumerated, allowing them to be enumerated by their respective parents.
> 
> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> devices on serial buses (SPI, I2C or UART).
> 
> On Macs an empty ResourceTemplate is returned for uart slaves.
> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> provided. Add a check for "baud" in acpi_is_serial_bus_slave().

Tested-by: Ronald Tschalär <ronald@innovation.ch>
Tested-by: Peter Y. Chuang <peteryuchuang@gmail.com>

Ronald and Peter both report success for the above-mentioned Mac-specific
change on GitHub: https://github.com/l1k/linux/pull/1#issuecomment-336126330

Thanks,

Lukas

Re: [v3,2/2] ACPI / scan: Fix enumeration for special UART devices

[Graeme Gregory]

On Wed, Oct 11, 2017 at 10:32:14AM +0200, Frédéric Danis wrote:
> UART devices is expected to be enumerated by SerDev subsystem.
> 
> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> enumerated, allowing them to be enumerated by their respective parents.
> 
> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> devices on serial buses (SPI, I2C or UART).
> 
> On Macs an empty ResourceTemplate is returned for uart slaves.
> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> 

This patch appears to break UART probing in ACPI on xgene based
plaforms.

The appropriate chunks of DSDT.

    Device (_SB.AHBC)
    {
        OperationRegion (SRST, SystemMemory, 0x1F2AC000, 0x04)
        OperationRegion (CLKE, SystemMemory, 0x1F2AC004, 0x04)
        OperationRegion (SRRM, SystemMemory, 0x1F2AD070, 0x04)
        OperationRegion (RD2F, SystemMemory, 0x1F2AE014, 0x04)

    ...

        Device (UAR0)
        {
            Name (_HID, "APMC0D08")  // _HID: Hardware ID
            Name (_DDN, "UAR0")  // _DDN: DOS Device Name
            Name (_UID, "UAR0")  // _UID: Unique ID
            Name (_STR, Unicode ("APM88xxxx UART0 Controller"))  // _STR: Description String
            Name (_ADR, 0x1C021000)  // _ADR: Address
            Name (_CID, "NS16550A")  // _CID: Compatible ID

    ...

            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                Memory32Fixed (ReadWrite,
                    0x1C021000,         // Address Base
                    0x00000100,         // Address Length
                    )
                UartSerialBusV2 (0x00002580, DataBitsEight, StopBitsOne,
                    0x00, LittleEndian, ParityTypeNone, FlowControlHardware,
                    0x0010, 0x0010, "UAR0",
                    0x00, ResourceConsumer, , Exclusive,
                    )
                Interrupt (ResourceProducer, Level, ActiveHigh, Exclusive, ,, )
                {
                    0x0000006D,
                }
            })

Thanks

Graeme

> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> Tested-by: Ronald Tschalär <ronald-Cgq6lnktLNMTaf21n8AfGQ@public.gmane.org>
> Tested-by: Peter Y. Chuang <peteryuchuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/acpi/scan.c     | 37 +++++++++++++++++--------------------
>  include/acpi/acpi_bus.h |  2 +-
>  2 files changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 602f8ff..860b698 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1505,41 +1505,38 @@ static void acpi_init_coherency(struct acpi_device *adev)
>  	adev->flags.coherent_dma = cca;
>  }
>  
> -static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
> +static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
>  {
> -	bool *is_spi_i2c_slave_p = data;
> +	bool *is_serial_bus_slave_p = data;
>  
>  	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
>  		return 1;
>  
> -	/*
> -	 * devices that are connected to UART still need to be enumerated to
> -	 * platform bus
> -	 */
> -	if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
> -		*is_spi_i2c_slave_p = true;
> +	*is_serial_bus_slave_p = true;
>  
>  	 /* no need to do more checking */
>  	return -1;
>  }
>  
> -static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
> +static bool acpi_is_serial_bus_slave(struct acpi_device *device)
>  {
>  	struct list_head resource_list;
> -	bool is_spi_i2c_slave = false;
> +	bool is_serial_bus_slave = false;
>  
>  	/* Macs use device properties in lieu of _CRS resources */
>  	if (x86_apple_machine &&
>  	    (fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
> -	     fwnode_property_present(&device->fwnode, "i2cAddress")))
> +	     fwnode_property_present(&device->fwnode, "i2cAddress") ||
> +	     fwnode_property_present(&device->fwnode, "baud")))
>  		return true;
>  
>  	INIT_LIST_HEAD(&resource_list);
> -	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
> -			       &is_spi_i2c_slave);
> +	acpi_dev_get_resources(device, &resource_list,
> +			       acpi_check_serial_bus_slave,
> +			       &is_serial_bus_slave);
>  	acpi_dev_free_resource_list(&resource_list);
>  
> -	return is_spi_i2c_slave;
> +	return is_serial_bus_slave;
>  }
>  
>  void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
> @@ -1557,7 +1554,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
>  	acpi_bus_get_flags(device);
>  	device->flags.match_driver = false;
>  	device->flags.initialized = true;
> -	device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
> +	device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
>  	acpi_device_clear_enumerated(device);
>  	device_initialize(&device->dev);
>  	dev_set_uevent_suppress(&device->dev, true);
> @@ -1841,10 +1838,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
>  static void acpi_default_enumeration(struct acpi_device *device)
>  {
>  	/*
> -	 * Do not enumerate SPI/I2C slaves as they will be enumerated by their
> -	 * respective parents.
> +	 * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
> +	 * their respective parents.
>  	 */
> -	if (!device->flags.spi_i2c_slave) {
> +	if (!device->flags.serial_bus_slave) {
>  		acpi_create_platform_device(device, NULL);
>  		acpi_device_set_enumerated(device);
>  	} else {
> @@ -1941,7 +1938,7 @@ static void acpi_bus_attach(struct acpi_device *device)
>  		return;
>  
>  	device->flags.match_driver = true;
> -	if (ret > 0 && !device->flags.spi_i2c_slave) {
> +	if (ret > 0 && !device->flags.serial_bus_slave) {
>  		acpi_device_set_enumerated(device);
>  		goto ok;
>  	}
> @@ -1950,7 +1947,7 @@ static void acpi_bus_attach(struct acpi_device *device)
>  	if (ret < 0)
>  		return;
>  
> -	if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
> +	if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
>  		acpi_device_set_enumerated(device);
>  	else
>  		acpi_default_enumeration(device);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index fa15052..f849be2 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -211,7 +211,7 @@ struct acpi_device_flags {
>  	u32 of_compatible_ok:1;
>  	u32 coherent_dma:1;
>  	u32 cca_seen:1;
> -	u32 spi_i2c_slave:1;
> +	u32 serial_bus_slave:1;
>  	u32 reserved:19;
>  };
>  

Re: [v3,2/2] ACPI / scan: Fix enumeration for special UART devices

[Frédéric Danis]

Hi Graeme,

Le 31/01/2018 à 15:21, Graeme Gregory a écrit :
> On Wed, Oct 11, 2017 at 10:32:14AM +0200, Frédéric Danis wrote:
>> UART devices is expected to be enumerated by SerDev subsystem.
>>
>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
>> enumerated, allowing them to be enumerated by their respective parents.
>>
>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
>> devices on serial buses (SPI, I2C or UART).
>>
>> On Macs an empty ResourceTemplate is returned for uart slaves.
>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>>
> This patch appears to break UART probing in ACPI on xgene based
> plaforms.
>
> The appropriate chunks of DSDT.
>
>      Device (_SB.AHBC)
>      {
>          OperationRegion (SRST, SystemMemory, 0x1F2AC000, 0x04)
>          OperationRegion (CLKE, SystemMemory, 0x1F2AC004, 0x04)
>          OperationRegion (SRRM, SystemMemory, 0x1F2AD070, 0x04)
>          OperationRegion (RD2F, SystemMemory, 0x1F2AE014, 0x04)
>
>      ...
>
>          Device (UAR0)
>          {
>              Name (_HID, "APMC0D08")  // _HID: Hardware ID
>              Name (_DDN, "UAR0")  // _DDN: DOS Device Name
>              Name (_UID, "UAR0")  // _UID: Unique ID
>              Name (_STR, Unicode ("APM88xxxx UART0 Controller"))  // _STR: Description String
>              Name (_ADR, 0x1C021000)  // _ADR: Address
>              Name (_CID, "NS16550A")  // _CID: Compatible ID
>
>      ...
>
>              Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
>              {
>                  Memory32Fixed (ReadWrite,
>                      0x1C021000,         // Address Base
>                      0x00000100,         // Address Length
>                      )
>                  UartSerialBusV2 (0x00002580, DataBitsEight, StopBitsOne,
>                      0x00, LittleEndian, ParityTypeNone, FlowControlHardware,
>                      0x0010, 0x0010, "UAR0",
>                      0x00, ResourceConsumer, , Exclusive,
>                      )
>                  Interrupt (ResourceProducer, Level, ActiveHigh, Exclusive, ,, )
>                  {
>                      0x0000006D,
>                  }
>              })

This seems to be related to 
https://bugzilla.redhat.com/show_bug.cgi?id=1531140
Am I correct?

The SerDev support should allow UART to appear as tty device if not used 
by an underlying component (cf. tty_port_register_device_attr() in 
drivers/tty/tty_port.c).

AFAIU, there is no internal device attached to this serial port.
Is it possible to get complete ACPI DSDT?

Is SerDev enabled on this device?
Boot logs with SerDev debug traces enabled can be useful to understand 
what happens.

Regards,

Fred

-- 
Frédéric Danis                       Embedded Linux Consultant
frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [PATCH v3 0/2] ACPI serdev support

[Johan Hovold]

On Wed, Oct 11, 2017 at 10:32:12AM +0200, Frédéric Danis wrote:
> Add ACPI support for serial attached devices.
> 
> Currently, serial devices are not set as enumerated during ACPI scan for SPI
> or i2c buses (but not for UART). This should also be done for UART serial
> devices.
> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
> 
> This needs Johan Hovold's "serdev: fix registration of second slave" patch.

In theory this series could go in through the acpi-tree without my
fix. It would only affect an error case where an unlikely failure to
register an ACPI serdev device, would prevent the tty-class device from
being registered instead of the controller. That is, something we can
live with until this all converges in 4.15-rc1 if needed.

That said, I think we should consider taking all serdev changes, and
therefore also the ACPI patch, through the tty tree instead in order to
avoid merge conflicts. Rafael?

Johan

Re: [PATCH v3 0/2] ACPI serdev support

[Rafael J. Wysocki]

On Wed, Oct 11, 2017 at 11:03 AM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Oct 11, 2017 at 10:32:12AM +0200, Frédéric Danis wrote:
>> Add ACPI support for serial attached devices.
>>
>> Currently, serial devices are not set as enumerated during ACPI scan for SPI
>> or i2c buses (but not for UART). This should also be done for UART serial
>> devices.
>> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>
>> This needs Johan Hovold's "serdev: fix registration of second slave" patch.
>
> In theory this series could go in through the acpi-tree without my
> fix. It would only affect an error case where an unlikely failure to
> register an ACPI serdev device, would prevent the tty-class device from
> being registered instead of the controller. That is, something we can
> live with until this all converges in 4.15-rc1 if needed.
>
> That said, I think we should consider taking all serdev changes, and
> therefore also the ACPI patch, through the tty tree instead in order to
> avoid merge conflicts. Rafael?

OK

Please feel free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

to the ACPI core change.

And I will assume that this series will go in via the tty tree.

Thanks,
Rafael

Re: [PATCH v3 0/2] ACPI serdev support

[Marcel Holtmann]

Hi Greg,

>>> Add ACPI support for serial attached devices.
>>> 
>>> Currently, serial devices are not set as enumerated during ACPI scan for SPI
>>> or i2c buses (but not for UART). This should also be done for UART serial
>>> devices.
>>> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>> 
>>> This needs Johan Hovold's "serdev: fix registration of second slave" patch.
>> 
>> In theory this series could go in through the acpi-tree without my
>> fix. It would only affect an error case where an unlikely failure to
>> register an ACPI serdev device, would prevent the tty-class device from
>> being registered instead of the controller. That is, something we can
>> live with until this all converges in 4.15-rc1 if needed.
>> 
>> That said, I think we should consider taking all serdev changes, and
>> therefore also the ACPI patch, through the tty tree instead in order to
>> avoid merge conflicts. Rafael?
> 
> OK
> 
> Please feel free to add
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> to the ACPI core change.
> 
> And I will assume that this series will go in via the tty tree.

you have to take these two patches now via the TTY tree now. In case you already marked them as someone else problem ;)

Regards

Marcel

Re: [PATCH v3 0/2] ACPI serdev support

[Frédéric Danis]

Hi Greg, Rafael, Marcel,

Le 11/10/2017 à 20:32, Marcel Holtmann a écrit :
> Hi Greg,
>
>>>> Add ACPI support for serial attached devices.
>>>>
>>>> Currently, serial devices are not set as enumerated during ACPI scan for SPI
>>>> or i2c buses (but not for UART). This should also be done for UART serial
>>>> devices.
>>>> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>>>
>>>> This needs Johan Hovold's "serdev: fix registration of second slave" patch.
>>> In theory this series could go in through the acpi-tree without my
>>> fix. It would only affect an error case where an unlikely failure to
>>> register an ACPI serdev device, would prevent the tty-class device from
>>> being registered instead of the controller. That is, something we can
>>> live with until this all converges in 4.15-rc1 if needed.
>>>
>>> That said, I think we should consider taking all serdev changes, and
>>> therefore also the ACPI patch, through the tty tree instead in order to
>>> avoid merge conflicts. Rafael?
>> OK
>>
>> Please feel free to add
>>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> to the ACPI core change.
>>
>> And I will assume that this series will go in via the tty tree.
> you have to take these two patches now via the TTY tree now. In case you already marked them as someone else problem ;)
>
> Regards
>
> Marcel

Is there any problem I missed with those patches?
Do I have to re-send them?

Regards,

Fred

Re: [PATCH v3 0/2] ACPI serdev support

[Greg Kroah-Hartman]

On Wed, Oct 18, 2017 at 04:46:05PM +0200, Frédéric Danis wrote:
> Hi Greg, Rafael, Marcel,
> 
> Le 11/10/2017 à 20:32, Marcel Holtmann a écrit :
> > Hi Greg,
> > 
> > > > > Add ACPI support for serial attached devices.
> > > > > 
> > > > > Currently, serial devices are not set as enumerated during ACPI scan for SPI
> > > > > or i2c buses (but not for UART). This should also be done for UART serial
> > > > > devices.
> > > > > I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
> > > > > 
> > > > > This needs Johan Hovold's "serdev: fix registration of second slave" patch.
> > > > In theory this series could go in through the acpi-tree without my
> > > > fix. It would only affect an error case where an unlikely failure to
> > > > register an ACPI serdev device, would prevent the tty-class device from
> > > > being registered instead of the controller. That is, something we can
> > > > live with until this all converges in 4.15-rc1 if needed.
> > > > 
> > > > That said, I think we should consider taking all serdev changes, and
> > > > therefore also the ACPI patch, through the tty tree instead in order to
> > > > avoid merge conflicts. Rafael?
> > > OK
> > > 
> > > Please feel free to add
> > > 
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > to the ACPI core change.
> > > 
> > > And I will assume that this series will go in via the tty tree.
> > you have to take these two patches now via the TTY tree now. In case you already marked them as someone else problem ;)
> > 
> > Regards
> > 
> > Marcel
> 
> Is there any problem I missed with those patches?
> Do I have to re-send them?

No, they are in my queue, still catching up...

thanks,

greg k-h

Re: [PATCH v3 0/2] ACPI serdev support

[Johan Hovold]

On Wed, Oct 18, 2017 at 04:56:08PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 18, 2017 at 04:46:05PM +0200, Frédéric Danis wrote:

> > Le 11/10/2017 à 20:32, Marcel Holtmann a écrit :

> > > > > > Add ACPI support for serial attached devices.
> > > > > > 
> > > > > > Currently, serial devices are not set as enumerated during
> > > > > > ACPI scan for SPI or i2c buses (but not for UART). This
> > > > > > should also be done for UART serial devices.  I renamed
> > > > > > *spi_i2c_slave* to *serial_bus_slave* to reflect this.
> > > > > > 
> > > > > > This needs Johan Hovold's "serdev: fix registration of
> > > > > > second slave" patch.
> > > > > In theory this series could go in through the acpi-tree
> > > > > without my fix. It would only affect an error case where an
> > > > > unlikely failure to register an ACPI serdev device, would
> > > > > prevent the tty-class device from being registered instead of
> > > > > the controller. That is, something we can live with until this
> > > > > all converges in 4.15-rc1 if needed.
> > > > > 
> > > > > That said, I think we should consider taking all serdev
> > > > > changes, and therefore also the ACPI patch, through the tty
> > > > > tree instead in order to avoid merge conflicts. Rafael?
> > > > OK
> > > > 
> > > > Please feel free to add
> > > > 
> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > 
> > > > to the ACPI core change.
> > > > 
> > > > And I will assume that this series will go in via the tty tree.
> > > you have to take these two patches now via the TTY tree now. In
> > > case you already marked them as someone else problem ;)

> > Is there any problem I missed with those patches?
> > Do I have to re-send them?
> 
> No, they are in my queue, still catching up...

I just realised that we cannot merge this series (the second acpi patch)
until the hci_intel driver gains serdev support or otherwise PM will
break for those devices.

Specifically, the hci_intel driver uses similar hacks as the hci_bcm
driver does for PM, so we need something like Hans's hci_bcm series also
for hci_intel before we can do the switch.

Hopefully there are no more of these...

Johan

Re: [PATCH v3 0/2] ACPI serdev support

[Hans de Goede]

Hi,

On 19-10-17 16:23, Johan Hovold wrote:
> On Wed, Oct 18, 2017 at 04:56:08PM +0200, Greg Kroah-Hartman wrote:
>> On Wed, Oct 18, 2017 at 04:46:05PM +0200, Frédéric Danis wrote:
> 
>>> Le 11/10/2017 à 20:32, Marcel Holtmann a écrit :
> 
>>>>>>> Add ACPI support for serial attached devices.
>>>>>>>
>>>>>>> Currently, serial devices are not set as enumerated during
>>>>>>> ACPI scan for SPI or i2c buses (but not for UART). This
>>>>>>> should also be done for UART serial devices.  I renamed
>>>>>>> *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>>>>>>
>>>>>>> This needs Johan Hovold's "serdev: fix registration of
>>>>>>> second slave" patch.
>>>>>> In theory this series could go in through the acpi-tree
>>>>>> without my fix. It would only affect an error case where an
>>>>>> unlikely failure to register an ACPI serdev device, would
>>>>>> prevent the tty-class device from being registered instead of
>>>>>> the controller. That is, something we can live with until this
>>>>>> all converges in 4.15-rc1 if needed.
>>>>>>
>>>>>> That said, I think we should consider taking all serdev
>>>>>> changes, and therefore also the ACPI patch, through the tty
>>>>>> tree instead in order to avoid merge conflicts. Rafael?
>>>>> OK
>>>>>
>>>>> Please feel free to add
>>>>>
>>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> to the ACPI core change.
>>>>>
>>>>> And I will assume that this series will go in via the tty tree.
>>>> you have to take these two patches now via the TTY tree now. In
>>>> case you already marked them as someone else problem ;)
> 
>>> Is there any problem I missed with those patches?
>>> Do I have to re-send them?
>>
>> No, they are in my queue, still catching up...
> 
> I just realised that we cannot merge this series (the second acpi patch)
> until the hci_intel driver gains serdev support or otherwise PM will
> break for those devices.
> 
> Specifically, the hci_intel driver uses similar hacks as the hci_bcm
> driver does for PM, so we need something like Hans's hci_bcm series also
> for hci_intel before we can do the switch.

Hmm, I've never actually seen any hardware use an intel BT HCI connected
to a serdev, but I guess people did not write that code for fun, so those
do exist ?

Regards,

Hans

Re: [PATCH v3 0/2] ACPI serdev support

[Marcel Holtmann]

Hi Hans,

>>>>>>>> Add ACPI support for serial attached devices.
>>>>>>>> 
>>>>>>>> Currently, serial devices are not set as enumerated during
>>>>>>>> ACPI scan for SPI or i2c buses (but not for UART). This
>>>>>>>> should also be done for UART serial devices.  I renamed
>>>>>>>> *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>>>>>>> 
>>>>>>>> This needs Johan Hovold's "serdev: fix registration of
>>>>>>>> second slave" patch.
>>>>>>> In theory this series could go in through the acpi-tree
>>>>>>> without my fix. It would only affect an error case where an
>>>>>>> unlikely failure to register an ACPI serdev device, would
>>>>>>> prevent the tty-class device from being registered instead of
>>>>>>> the controller. That is, something we can live with until this
>>>>>>> all converges in 4.15-rc1 if needed.
>>>>>>> 
>>>>>>> That said, I think we should consider taking all serdev
>>>>>>> changes, and therefore also the ACPI patch, through the tty
>>>>>>> tree instead in order to avoid merge conflicts. Rafael?
>>>>>> OK
>>>>>> 
>>>>>> Please feel free to add
>>>>>> 
>>>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>> 
>>>>>> to the ACPI core change.
>>>>>> 
>>>>>> And I will assume that this series will go in via the tty tree.
>>>>> you have to take these two patches now via the TTY tree now. In
>>>>> case you already marked them as someone else problem ;)
>>>> Is there any problem I missed with those patches?
>>>> Do I have to re-send them?
>>> 
>>> No, they are in my queue, still catching up...
>> I just realised that we cannot merge this series (the second acpi patch)
>> until the hci_intel driver gains serdev support or otherwise PM will
>> break for those devices.
>> Specifically, the hci_intel driver uses similar hacks as the hci_bcm
>> driver does for PM, so we need something like Hans's hci_bcm series also
>> for hci_intel before we can do the switch.
> 
> Hmm, I've never actually seen any hardware use an intel BT HCI connected
> to a serdev, but I guess people did not write that code for fun, so those
> do exist ?

they are all ACPI based and could now start using serdev. Previously they were all driven by btattach.

Regards

Marcel

Re: [PATCH v3 0/2] ACPI serdev support

[Hans de Goede]

Hi,

On 19-10-17 16:32, Marcel Holtmann wrote:
> Hi Hans,
> 
>>>>>>>>> Add ACPI support for serial attached devices.
>>>>>>>>>
>>>>>>>>> Currently, serial devices are not set as enumerated during
>>>>>>>>> ACPI scan for SPI or i2c buses (but not for UART). This
>>>>>>>>> should also be done for UART serial devices.  I renamed
>>>>>>>>> *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>>>>>>>>
>>>>>>>>> This needs Johan Hovold's "serdev: fix registration of
>>>>>>>>> second slave" patch.
>>>>>>>> In theory this series could go in through the acpi-tree
>>>>>>>> without my fix. It would only affect an error case where an
>>>>>>>> unlikely failure to register an ACPI serdev device, would
>>>>>>>> prevent the tty-class device from being registered instead of
>>>>>>>> the controller. That is, something we can live with until this
>>>>>>>> all converges in 4.15-rc1 if needed.
>>>>>>>>
>>>>>>>> That said, I think we should consider taking all serdev
>>>>>>>> changes, and therefore also the ACPI patch, through the tty
>>>>>>>> tree instead in order to avoid merge conflicts. Rafael?
>>>>>>> OK
>>>>>>>
>>>>>>> Please feel free to add
>>>>>>>
>>>>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>>>
>>>>>>> to the ACPI core change.
>>>>>>>
>>>>>>> And I will assume that this series will go in via the tty tree.
>>>>>> you have to take these two patches now via the TTY tree now. In
>>>>>> case you already marked them as someone else problem ;)
>>>>> Is there any problem I missed with those patches?
>>>>> Do I have to re-send them?
>>>>
>>>> No, they are in my queue, still catching up...
>>> I just realised that we cannot merge this series (the second acpi patch)
>>> until the hci_intel driver gains serdev support or otherwise PM will
>>> break for those devices.
>>> Specifically, the hci_intel driver uses similar hacks as the hci_bcm
>>> driver does for PM, so we need something like Hans's hci_bcm series also
>>> for hci_intel before we can do the switch.
>>
>> Hmm, I've never actually seen any hardware use an intel BT HCI connected
>> to a serdev, but I guess people did not write that code for fun, so those
>> do exist ?
> 
> they are all ACPI based and could now start using serdev. Previously they were all driven by btattach.

I understand, I was just wondering if anyone is aware of any hardware
actually using Intel BT devices in this manner, because it is going
to be tricky to do a similar series if we cannot test it.

Regards,

Hans

Re: [PATCH v3 0/2] ACPI serdev support

[Hans de Goede]

Hi,

On 19-10-17 18:15, Loic Poulain wrote:
> Hi
> 
> 
>             Hmm, I've never actually seen any hardware use an intel BT HCI connected
>             to a serdev, but I guess people did not write that code for fun, so those
>             do exist ?
> 
> 
>         they are all ACPI based and could now start using serdev. Previously they were all driven by btattach.
> 
> 
>     I understand, I was just wondering if anyone is aware of any hardware
>     actually using Intel BT devices in this manner, because it is going
>     to be tricky to do a similar series if we cannot test it.
> 
> 
> Yeah this driver supports Lightning-Peak iBT 3.0 UART controller.
> I remember this controller comes (at least) with Atom x3-c3440 / x3-c3445 SoCs.
> However I don't have the hardware anymore.

Ah, those are the Rockchip manufactured Atom based SoCs which
come with Mali graphics, I don't think anyone is trying to run
mainline on those and not a whole lot of those have shipped to
begin with (the line has been canceled).

I've an Apollo Lake based laptop with Intel Bluetooth, I just
checked and that is simply using USB.

So I don't think we are actually going to break anything by
just moving forward as planned. This reminds me of the axp288
driver where Intel's "embedded" engineering team upstreamed
parts of the axp288 PMIC code, but in a way where the driver
could not work as it would not load with platform_data which
was never provided by the mainline kernel.

As such it might actually be good to break this and if no-one
complains we can just remove the hci_intel.c code altogether.

The alternative would be to revert 2 if the 3 patches of
my last series for making the BT on the GPD win / pocket
(and likely other devices) work in uart mode. Then someone
(me?) would need to do a completely untested series for
hci_intel.c, and get that + reverts of the reverts into 4.16,
which is not the best way forward IMHO.

Regards,

Hans

Re: [PATCH v3 0/2] ACPI serdev support

[Marcel Holtmann]

Hi Hans,

>>            Hmm, I've never actually seen any hardware use an intel BT HCI connected
>>            to a serdev, but I guess people did not write that code for fun, so those
>>            do exist ?
>>        they are all ACPI based and could now start using serdev. Previously they were all driven by btattach.
>>    I understand, I was just wondering if anyone is aware of any hardware
>>    actually using Intel BT devices in this manner, because it is going
>>    to be tricky to do a similar series if we cannot test it.
>> Yeah this driver supports Lightning-Peak iBT 3.0 UART controller.
>> I remember this controller comes (at least) with Atom x3-c3440 / x3-c3445 SoCs.
>> However I don't have the hardware anymore.
> 
> Ah, those are the Rockchip manufactured Atom based SoCs which
> come with Mali graphics, I don't think anyone is trying to run
> mainline on those and not a whole lot of those have shipped to
> begin with (the line has been canceled).
> 
> I've an Apollo Lake based laptop with Intel Bluetooth, I just
> checked and that is simply using USB.
> 
> So I don't think we are actually going to break anything by
> just moving forward as planned. This reminds me of the axp288
> driver where Intel's "embedded" engineering team upstreamed
> parts of the axp288 PMIC code, but in a way where the driver
> could not work as it would not load with platform_data which
> was never provided by the mainline kernel.
> 
> As such it might actually be good to break this and if no-one
> complains we can just remove the hci_intel.c code altogether.

I think we can go forward with what we have. I am not going to remove hci_intel.c since the firmware loading code etc. is pretty generic to all Intel UART based chips. However we just take the PM pieces out or maybe someone is going to fix them.

> The alternative would be to revert 2 if the 3 patches of
> my last series for making the BT on the GPD win / pocket
> (and likely other devices) work in uart mode. Then someone
> (me?) would need to do a completely untested series for
> hci_intel.c, and get that + reverts of the reverts into 4.16,
> which is not the best way forward IMHO.

I am not doing that. We have to make progress towards serdev. So if things really turn out badly, then we have to deal with it, but that should not hold up getting things in the direction this needs to go.

Regards

Marcel

Re: [PATCH v3 0/2] ACPI serdev support

[Hans de Goede]

Hi,

On 19-10-17 21:00, Marcel Holtmann wrote:
> Hi Hans,
> 
>>>             Hmm, I've never actually seen any hardware use an intel BT HCI connected
>>>             to a serdev, but I guess people did not write that code for fun, so those
>>>             do exist ?
>>>         they are all ACPI based and could now start using serdev. Previously they were all driven by btattach.
>>>     I understand, I was just wondering if anyone is aware of any hardware
>>>     actually using Intel BT devices in this manner, because it is going
>>>     to be tricky to do a similar series if we cannot test it.
>>> Yeah this driver supports Lightning-Peak iBT 3.0 UART controller.
>>> I remember this controller comes (at least) with Atom x3-c3440 / x3-c3445 SoCs.
>>> However I don't have the hardware anymore.
>>
>> Ah, those are the Rockchip manufactured Atom based SoCs which
>> come with Mali graphics, I don't think anyone is trying to run
>> mainline on those and not a whole lot of those have shipped to
>> begin with (the line has been canceled).
>>
>> I've an Apollo Lake based laptop with Intel Bluetooth, I just
>> checked and that is simply using USB.
>>
>> So I don't think we are actually going to break anything by
>> just moving forward as planned. This reminds me of the axp288
>> driver where Intel's "embedded" engineering team upstreamed
>> parts of the axp288 PMIC code, but in a way where the driver
>> could not work as it would not load with platform_data which
>> was never provided by the mainline kernel.
>>
>> As such it might actually be good to break this and if no-one
>> complains we can just remove the hci_intel.c code altogether.
> 
> I think we can go forward with what we have. I am not going to remove hci_intel.c since the firmware loading code etc. is pretty generic to all Intel UART based chips. However we just take the PM pieces out or maybe someone is going to fix them.

Ok.

>> The alternative would be to revert 2 if the 3 patches of
>> my last series for making the BT on the GPD win / pocket
>> (and likely other devices) work in uart mode. Then someone
>> (me?) would need to do a completely untested series for
>> hci_intel.c, and get that + reverts of the reverts into 4.16,
>> which is not the best way forward IMHO.
> 
> I am not doing that. We have to make progress towards serdev. So if things really turn out badly, then we have to deal with it, but that should not hold up getting things in the direction this needs to go.

Sounds good to me.

Regards,

Hans

Re: [PATCH v3 0/2] ACPI serdev support

[Johan Hovold]

On Thu, Oct 19, 2017 at 09:00:25PM +0200, Marcel Holtmann wrote:
> Hi Hans,
> 
> >>            Hmm, I've never actually seen any hardware use an intel BT HCI connected
> >>            to a serdev, but I guess people did not write that code for fun, so those
> >>            do exist ?
> >>        they are all ACPI based and could now start using serdev. Previously they were all driven by btattach.
> >>    I understand, I was just wondering if anyone is aware of any hardware
> >>    actually using Intel BT devices in this manner, because it is going
> >>    to be tricky to do a similar series if we cannot test it.
> >> Yeah this driver supports Lightning-Peak iBT 3.0 UART controller.
> >> I remember this controller comes (at least) with Atom x3-c3440 / x3-c3445 SoCs.
> >> However I don't have the hardware anymore.
> > 
> > Ah, those are the Rockchip manufactured Atom based SoCs which
> > come with Mali graphics, I don't think anyone is trying to run
> > mainline on those and not a whole lot of those have shipped to
> > begin with (the line has been canceled).
> > 
> > I've an Apollo Lake based laptop with Intel Bluetooth, I just
> > checked and that is simply using USB.
> > 
> > So I don't think we are actually going to break anything by
> > just moving forward as planned. This reminds me of the axp288
> > driver where Intel's "embedded" engineering team upstreamed
> > parts of the axp288 PMIC code, but in a way where the driver
> > could not work as it would not load with platform_data which
> > was never provided by the mainline kernel.
> > 
> > As such it might actually be good to break this and if no-one
> > complains we can just remove the hci_intel.c code altogether.
> 
> I think we can go forward with what we have. I am not going to remove
> hci_intel.c since the firmware loading code etc. is pretty generic to
> all Intel UART based chips. However we just take the PM pieces out or
> maybe someone is going to fix them.

Note that hci_intel used the platform hack for the reset line and host
wake-up irq. And as the reset line appears to be required when using
ACPI, we'd definitely find out eventually if anyone is using such
devices as they would fail to even power on.

> > The alternative would be to revert 2 if the 3 patches of
> > my last series for making the BT on the GPD win / pocket
> > (and likely other devices) work in uart mode. Then someone
> > (me?) would need to do a completely untested series for
> > hci_intel.c, and get that + reverts of the reverts into 4.16,
> > which is not the best way forward IMHO.
> 
> I am not doing that. We have to make progress towards serdev. So if
> things really turn out badly, then we have to deal with it, but that
> should not hold up getting things in the direction this needs to go.

As long as we know what we are breaking.

Johan

Re: [PATCH v3 0/2] ACPI serdev support

[Johan Hovold]

On Thu, Oct 19, 2017 at 04:26:25PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 19-10-17 16:23, Johan Hovold wrote:
> > On Wed, Oct 18, 2017 at 04:56:08PM +0200, Greg Kroah-Hartman wrote:
> >> On Wed, Oct 18, 2017 at 04:46:05PM +0200, Frédéric Danis wrote:
> > 
> >>> Le 11/10/2017 à 20:32, Marcel Holtmann a écrit :
> > 
> >>>>>>> Add ACPI support for serial attached devices.
> >>>>>>>
> >>>>>>> Currently, serial devices are not set as enumerated during
> >>>>>>> ACPI scan for SPI or i2c buses (but not for UART). This
> >>>>>>> should also be done for UART serial devices.  I renamed
> >>>>>>> *spi_i2c_slave* to *serial_bus_slave* to reflect this.

> > I just realised that we cannot merge this series (the second acpi patch)
> > until the hci_intel driver gains serdev support or otherwise PM will
> > break for those devices.
> > 
> > Specifically, the hci_intel driver uses similar hacks as the hci_bcm
> > driver does for PM, so we need something like Hans's hci_bcm series also
> > for hci_intel before we can do the switch.
> 
> Hmm, I've never actually seen any hardware use an intel BT HCI connected
> to a serdev, but I guess people did not write that code for fun, so those
> do exist ?

At least that's what it looks like. 

It was added by Loic Poulain in commit 1ab1f239bf17 ("Bluetooth:
hci_intel: Add support for platform driver") two years ago and the
ACPI-match table has an entry for "INT33E1".

Johan