[PATCH 0/2] ACPI serdev support

[PATCH 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.

For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
support to the serdev drv" patches to work.

Tested on T100TA with Broadcom BCM2E39.

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 | 99 ++++++++++++++++++++++++++++++++++++++++++++---
 include/acpi/acpi_bus.h   |  2 +-
 3 files changed, 112 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH 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.

Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c68fb3a..104777d 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,74 @@ 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 Serial 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 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_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+#else
+static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_ACPI */
+
 /**
  * serdev_controller_add() - Add an serdev controller
  * @ctrl:	controller to be registered.
@@ -394,7 +478,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 +488,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, "serdev%d no devices registered: of:%d acpi:%d\n",
+			ctrl->nr, 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

[PATCH 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>
---
 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 0/2] ACPI serdev support

[Marcel Holtmann]

Hi Fred,

> 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.
> 
> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
> support to the serdev drv" patches to work.
> 
> Tested on T100TA with Broadcom BCM2E39.
> 
> 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

I already applied Hans’ patch series to bluetooth-next tree. And I have no objections or comments for these changes. They should be reviewed and acked by the appropriate maintainers as well, but from my side this looks good.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

What we should discuss on how we get them upstream since besides the Bluetooth subsystem, they cover ACPI and TTY subsystems.

Regards

Marcel

Re: [PATCH 0/2] ACPI serdev support

[Ian W MORRISON]

On 10/6/17 2:21 AM, Marcel Holtmann wrote:
> Hi Fred,
> 
>> 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.
>>
>> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
>> support to the serdev drv" patches to work.
>>
>> Tested on T100TA with Broadcom BCM2E39.
>>
>> 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
> 
> I already applied Hans’ patch series to bluetooth-next tree. And I have no objections or comments for these changes. They should be reviewed and acked by the appropriate maintainers as well, but from my side this looks good.
> 
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> 
> What we should discuss on how we get them upstream since besides the Bluetooth subsystem, they cover ACPI and TTY subsystems.
> 
> Regards
> 
> Marcel
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Hi Marcel,

I've tested these two patches along with the nine patches (plus revert patch) from Hans's series but cannot get bluetooth to work on some of my devices using these two patches as is, specifically on MINIX Z83-4 based PCs. 

In order for bluetooth to function on these devices I submitted the patch 'Bluetooth: BCM: Add support for MINIX Z83-4 based devices' as they require the trigger type IRQF_TRIGGER_FALLING. With Hans's new patch series I've had to modify the patch which I've resubmitted as version 2. However when I apply the second patch from Fred's series 'ACPI / scan: Fix enumeration for special UART devices' I have found the bluetooth device (BCM2EA4) is no longer enumerated. Specifically I've found that the removal of the check 'if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)' causes this failure and if I re-add the 'if' statement then the device is loaded. However a 'btattach' is still required for functioning bluetooth.

In terms on applying the patches I have some questions. 

Like Hans I too would like one of my patches to be applied to v4.14. A lot of people have been waiting to get bluetooth working on the MINIX Z83-4 family of devices so is it possible to get my original (resend) patch for bluetooth on MINIX Z83-4 based devices applied to v4.14? If so and assuming the nine patches from Hans go into v4.15 I can then send an additional patch to extend his second patch (Bluetooth: hci_bcm: Fix setting of irq trigger type) to delete the extra line '.driver_data = &acpi_active_low,'. 

If the original (resend) version of my patch for MINIX Z83-4 cannot be applied to v4.14 then I would like to request that my second version of my patch for MINIX Z83-4 based devices be applied to bluetooth-next tree so that they are included in v4.15.

Finally I would like to request some help in addressing the issue caused by Fred's 'Fix enumeration for special UART devices' patch so as to prevent a regression.

Regards,
Ian

Re: [PATCH 0/2] ACPI serdev support

[Frédéric Danis]

Hi Ian,

Le 06/10/2017 à 09:33, Ian W MORRISON a écrit :
> On 10/6/17 2:21 AM, Marcel Holtmann wrote:
>> Hi Fred,
>>
>>> 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.
>>>
>>> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
>>> support to the serdev drv" patches to work.
>>>
>>> Tested on T100TA with Broadcom BCM2E39.
>>>
>>> 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
>> I already applied Hans’ patch series to bluetooth-next tree. And I have no objections or comments for these changes. They should be reviewed and acked by the appropriate maintainers as well, but from my side this looks good.
>>
>> Acked-by: Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
>>
>> What we should discuss on how we get them upstream since besides the Bluetooth subsystem, they cover ACPI and TTY subsystems.
>>
>> Regards
>>
>> Marcel
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> Hi Marcel,
>
> I've tested these two patches along with the nine patches (plus revert patch) from Hans's series but cannot get bluetooth to work on some of my devices using these two patches as is, specifically on MINIX Z83-4 based PCs.
>
> In order for bluetooth to function on these devices I submitted the patch 'Bluetooth: BCM: Add support for MINIX Z83-4 based devices' as they require the trigger type IRQF_TRIGGER_FALLING. With Hans's new patch series I've had to modify the patch which I've resubmitted as version 2. However when I apply the second patch from Fred's series 'ACPI / scan: Fix enumeration for special UART devices' I have found the bluetooth device (BCM2EA4) is no longer enumerated. Specifically I've found that the removal of the check 'if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)' causes this failure and if I re-add the 'if' statement then the device is loaded. However a 'btattach' is still required for functioning bluetooth.

It seems normal to me that BCM2EA4 is no more enumerated at ACPI level 
as this is moved to serdev.
When removing 'if (ares->data.common_serial_bus.type != 
ACPI_RESOURCE_SERIAL_TYPE_UART)' you stop the serdev module finding the 
Serial UART information. In this case it will not register the device 
and it will fall back to previous behavior needing to use btattach to 
setup Bluetooth.

Can you share:
- btattach you are currently using,
- dmesg with with dynamic debug enabled for serdev and hci_uart modules 
during boot (with Hans's patches, your MINIX Z83-4 patches and mine 
patches).

Regards,

Fred

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

[Rob Herring]

On Wed, Oct 4, 2017 at 3:51 AM, Frédéric Danis
<frederic.danis.oss@gmail.com> 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.
>
> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> ---
>  drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 94 insertions(+), 5 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/2] ACPI serdev support

[Frédéric Danis]

Hi Ian,

Le 06/10/2017 à 16:47, Ian W MORRISON a écrit :
> <snip>
>> It seems normal to me that BCM2EA4 is no more enumerated at ACPI level as
>> this is moved to serdev.
>> When removing 'if (ares->data.common_serial_bus.type !=
>> ACPI_RESOURCE_SERIAL_TYPE_UART)' you stop the serdev module finding the
>> Serial UART information. In this case it will not register the device and it
>> will fall back to previous behavior needing to use btattach to setup
>> Bluetooth.
>>
>> Can you share:
>> - btattach you are currently using,
>> - dmesg with with dynamic debug enabled for serdev and hci_uart modules
>> during boot (with Hans's patches, your MINIX Z83-4 patches and mine
>> patches).
>>
>> Regards,
>>
>> Fred
> Hi Fred,
>
> I've attached four (text) files:
>
> 1. btattach.txt - Details of the 'bluez' package that contains the
> 'btattach' I'm using.
> 2. dmesg.txt - 'dmesg' with with dynamic debug enabled for serdev and
> hci_uart modules. This doesn't seem to show much so have I provided
> what you wanted?
> 3. gitlog.txt - First few commits from the git log showing the kernel
> patches used to build the kernel (sent just for clarity).
> 4. working.txt - An extract from 'dmesg' when BCM2EA4 is enumerated
> from a kernel patched with the 'if' statement refered to above.
>
> Regards,
> Ian

Which tty is used for btattach?
Is this tty existing in /dev?

I took a look at dmesg.txt and I did not find any trace related to serdev.
On the T100, where ttyS4 is used for Bluetooth, I can see the following 
traces:
   [   11.732347] serial serial0: allocated controller 
0xffff880036229000 id 0
   [   11.732470] serial serial0-0: device serial0-0 registered
   [   11.732475] serial serial0: serdev0 registered: dev:ffff880036229000
   [   11.732478] serial serial0: tty port ttyS4 registered

If serdev registration failed you should at least get something like:
     serdev0 no devices registered: of:<> acpi:<>

So, just to be sure, is SERIAL_DEV_BUS and SERIAL_DEV_CTRL_TTYPORT 
enabled in your kernel?

Regards,

Fred

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

[Marcel Holtmann]

Hi Rob,

>> 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.
>> 
>> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 94 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

so how do we get these changes upstream? If the serdev changes alone do not cause any harm, I am almost proposing taking them through bluetooth-next tree and only leave only the ACPI change to the ACPI maintainers.

Greg, any thoughts?

Regards

Marcel

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

[Rafael J. Wysocki]

On Fri, Oct 6, 2017 at 8:32 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Rob,
>
>>> 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.
>>>
>>> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
>>> ---
>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>
> so how do we get these changes upstream? If the serdev changes alone do not cause any harm, I am almost proposing taking them through bluetooth-next tree and only leave only the ACPI change to the ACPI maintainers.

That would be fine by me.  I can take the serdev patch too, though.

Thanks,
Rafael

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

[Marcel Holtmann]

Hi Rafael,

>>>> 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.
>>>> 
>>>> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>> 
>>> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> 
>> so how do we get these changes upstream? If the serdev changes alone do not cause any harm, I am almost proposing taking them through bluetooth-next tree and only leave only the ACPI change to the ACPI maintainers.
> 
> That would be fine by me.  I can take the serdev patch too, though.

having both patches go via ACPI tree might be simplest. Greg, any objections from you?

Regards

Marcel

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

[Greg Kroah-Hartman]

On Sat, Oct 07, 2017 at 02:31:05AM +0200, Marcel Holtmann wrote:
> Hi Rafael,
> 
> >>>> 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.
> >>>> 
> >>>> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>> ---
> >>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> >>>> 1 file changed, 94 insertions(+), 5 deletions(-)
> >>> 
> >>> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> 
> >> so how do we get these changes upstream? If the serdev changes alone do not cause any harm, I am almost proposing taking them through bluetooth-next tree and only leave only the ACPI change to the ACPI maintainers.
> > 
> > That would be fine by me.  I can take the serdev patch too, though.
> 
> having both patches go via ACPI tree might be simplest. Greg, any objections from you?

None from me, let me go ack the serdev patch...

thanks,

greg k-h

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

[Greg KH]

On Wed, Oct 04, 2017 at 10:51:29AM +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.
> 
> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5241 bytes --]

Hi,

On Wed, Oct 04, 2017 at 10:51:29AM +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.
> 
> 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>

-- Sebastian

> ---
>  drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 94 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index c68fb3a..104777d 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,74 @@ 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 Serial 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 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_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +#else
> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
> +{
> +	return -ENODEV;
> +}
> +#endif /* CONFIG_ACPI */
> +
>  /**
>   * serdev_controller_add() - Add an serdev controller
>   * @ctrl:	controller to be registered.
> @@ -394,7 +478,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 +488,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, "serdev%d no devices registered: of:%d acpi:%d\n",
> +			ctrl->nr, 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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5191 bytes --]

Hi,

On Wed, Oct 04, 2017 at 10:51:30AM +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().
> 
> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Johan Hovold]

On Wed, Oct 04, 2017 at 10:51:29AM +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.
> 
> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> ---
>  drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 94 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index c68fb3a..104777d 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c

> @@ -385,6 +401,74 @@ 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 Serial device for %s\n",

s/Serial/serdev/

> +			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 device. status %d\n", err);

s/ACPI/ACPI serdev/?

> +		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_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");

s/Serial/serdev/

> +		return -ENODEV;
> +	}
> +
> +	return 0;

What if there are no slaves defined? I'm not very familiar with the ACPI
helpers, but from a quick look it seems you'd then end up returning zero
here which would cause the serdev controller to be registered instead of
the tty-class device.

[ And if I'm mistaken, you do want to suppress that error message for
when there are no slaves defined. ]

> +}
> +#else
> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)

s/ctlr/ctrl/

> +{
> +	return -ENODEV;
> +}
> +#endif /* CONFIG_ACPI */
> +
>  /**
>   * serdev_controller_add() - Add an serdev controller
>   * @ctrl:	controller to be registered.
> @@ -394,7 +478,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 +488,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, "serdev%d no devices registered: of:%d acpi:%d\n",

"serdev%d" is redundant here as you're using dev_dbg (which will print
the device name).

> +			ctrl->nr, ret_of, ret_acpi);
> +		ret = -ENODEV;
>  		goto out_dev_del;
> +	}
>  
>  	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>  		ctrl->nr, &ctrl->dev);

Hmm, I see it's already used here. No need to follow that example
though.

Johan

Re: [PATCH 0/2] ACPI serdev support

[Johan Hovold]

On Fri, Oct 06, 2017 at 07:36:27PM +0200, Frédéric Danis wrote:

> I took a look at dmesg.txt and I did not find any trace related to serdev.
> On the T100, where ttyS4 is used for Bluetooth, I can see the following 
> traces:
>    [   11.732347] serial serial0: allocated controller 
> 0xffff880036229000 id 0
>    [   11.732470] serial serial0-0: device serial0-0 registered
>    [   11.732475] serial serial0: serdev0 registered: dev:ffff880036229000
>    [   11.732478] serial serial0: tty port ttyS4 registered
> 
> If serdev registration failed you should at least get something like:
>      serdev0 no devices registered: of:<> acpi:<>

Only with debugging enabled (it's a dev_dbg).

Johan

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

[Johan Hovold]

On Wed, Oct 04, 2017 at 10:51:30AM +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().
> 
> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

So just to reiterate what I just mentioned in a comment to one of Hans's
hci_bcm patches:

This one would silently break PM for such devices on any system which
does not have serdev enabled (as the corresponding platform devices
would no longer be registered). And with serdev enabled, hciattach
(btattach) would start failing as the tty device would no longer be
registered (but I assume everyone is aware of that, and fine with it, by
now).

Perhaps the hci_bcm driver should start depending on
SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?

Johan

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

Hi,

On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> On Wed, Oct 04, 2017 at 10:51:30AM +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().
> > 
> > Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> 
> So just to reiterate what I just mentioned in a comment to one of Hans's
> hci_bcm patches:
> 
> This one would silently break PM for such devices on any system which
> does not have serdev enabled (as the corresponding platform devices
> would no longer be registered). And with serdev enabled, hciattach
> (btattach) would start failing as the tty device would no longer be
> registered (but I assume everyone is aware of that, and fine with it, by
> now).
> 
> Perhaps the hci_bcm driver should start depending on
> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?

ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
for serdev. If any other controller is implemented that one could
also be used.

I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
it together with SERDEV. I suspect that we won't see any other
controller (it would be a UART device, that is not registered as
tty device) in the next few years and the extra option seems to
confuse people.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marcel Holtmann]

Hi Sebastian,

>>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> 
>> So just to reiterate what I just mentioned in a comment to one of Hans's
>> hci_bcm patches:
>> 
>> This one would silently break PM for such devices on any system which
>> does not have serdev enabled (as the corresponding platform devices
>> would no longer be registered). And with serdev enabled, hciattach
>> (btattach) would start failing as the tty device would no longer be
>> registered (but I assume everyone is aware of that, and fine with it, by
>> now).
>> 
>> Perhaps the hci_bcm driver should start depending on
>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> 
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> for serdev. If any other controller is implemented that one could
> also be used.
> 
> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> it together with SERDEV. I suspect that we won't see any other
> controller (it would be a UART device, that is not registered as
> tty device) in the next few years and the extra option seems to
> confuse people.

I wonder if we should just default SERIAL_DEV_CTRL_TTYPORT=y when DT or ACPI is enabled. Then no driver would have to select or depend on it.

Regards

Marcel

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

[Johan Hovold]

On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > On Wed, Oct 04, 2017 at 10:51:30AM +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().
> > > 
> > > Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> > 
> > So just to reiterate what I just mentioned in a comment to one of Hans's
> > hci_bcm patches:
> > 
> > This one would silently break PM for such devices on any system which
> > does not have serdev enabled (as the corresponding platform devices
> > would no longer be registered). And with serdev enabled, hciattach
> > (btattach) would start failing as the tty device would no longer be
> > registered (but I assume everyone is aware of that, and fine with it, by
> > now).
> > 
> > Perhaps the hci_bcm driver should start depending on
> > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> 
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> for serdev. If any other controller is implemented that one could
> also be used.

Not for hci_bcm, right? This particular driver specifically depends on
SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
serdev controller (or currently working systems soon breaks silently).

I don't think the same is true for the DT case where we do not already
have child nodes defined in firmware (and in fact, this driver did not
really support DT before serdev).

> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> it together with SERDEV. I suspect that we won't see any other
> controller (it would be a UART device, that is not registered as
> tty device) in the next few years and the extra option seems to
> confuse people.

I agree that it is somewhat confusing. But now that we have both,
perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
might need to be amended as well (e.g. if only to mention that you need
to select a controller as well).

And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.

Johan

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4062 bytes --]

Hi,

On Mon, Oct 09, 2017 at 09:35:26AM +0200, Johan Hovold wrote:
> On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> > On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > > On Wed, Oct 04, 2017 at 10:51:30AM +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().
> > > > 
> > > > Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> > > 
> > > So just to reiterate what I just mentioned in a comment to one of Hans's
> > > hci_bcm patches:
> > > 
> > > This one would silently break PM for such devices on any system which
> > > does not have serdev enabled (as the corresponding platform devices
> > > would no longer be registered). And with serdev enabled, hciattach
> > > (btattach) would start failing as the tty device would no longer be
> > > registered (but I assume everyone is aware of that, and fine with it, by
> > > now).
> > > 
> > > Perhaps the hci_bcm driver should start depending on
> > > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> > 
> > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > for serdev. If any other controller is implemented that one could
> > also be used.
> 
> Not for hci_bcm, right? This particular driver specifically depends on
> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> serdev controller (or currently working systems soon breaks silently).
> 
> I don't think the same is true for the DT case where we do not already
> have child nodes defined in firmware (and in fact, this driver did not
> really support DT before serdev).

The serdev ACPI support has been added to the core and not to
the ttyport and the hci_bcm driver only uses functions from the
core. As far as I can see the ACPI part would also work fine with
a different serdev controller.

Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
since it's the only serdev controller implementation. Also it
covers most use cases. When SERIAL_DEV_BUS is selected it's
very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.

> > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > it together with SERDEV. I suspect that we won't see any other
> > controller (it would be a UART device, that is not registered as
> > tty device) in the next few years and the extra option seems to
> > confuse people.
> 
> I agree that it is somewhat confusing. But now that we have both,
> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> might need to be amended as well (e.g. if only to mention that you
> need to select a controller as well).

I think we should at least add a default "y" if SERIAL_DEV_BUS.

> And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.

Yes and that's the correct dependency. They only need the serdev
core and controller. The only reason they do not work without
SERIAL_DEV_CTRL_TTYPORT is, that there won't be any serdev
controller.

Note, that the default "y" if SERIAL_DEV_BUS in SERIAL_DEV_CTRL_TTYPORT's
config entry is only a partial fix. There is still the problem,
that SERIAL_DEV_CTRL_TTYPORT can only be enabled if SERIAL_DEV_BUS
is configured builtin. This is a limitation of the ttyport
implementation, that hooks into builtin TTY core code.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

Hi,

On Sun, Oct 08, 2017 at 10:51:52AM +0200, Marcel Holtmann wrote:
> Hi Sebastian,
> 
> >>> 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>
> >> 
> >> So just to reiterate what I just mentioned in a comment to one of Hans's
> >> hci_bcm patches:
> >> 
> >> This one would silently break PM for such devices on any system which
> >> does not have serdev enabled (as the corresponding platform devices
> >> would no longer be registered). And with serdev enabled, hciattach
> >> (btattach) would start failing as the tty device would no longer be
> >> registered (but I assume everyone is aware of that, and fine with it, by
> >> now).
> >> 
> >> Perhaps the hci_bcm driver should start depending on
> >> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> > 
> > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > for serdev. If any other controller is implemented that one could
> > also be used.
> > 
> > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > it together with SERDEV. I suspect that we won't see any other
> > controller (it would be a UART device, that is not registered as
> > tty device) in the next few years and the extra option seems to
> > confuse people.
> 
> I wonder if we should just default SERIAL_DEV_CTRL_TTYPORT=y when
> DT or ACPI is enabled. Then no driver would have to select or
> depend on it.

This is not related to DT/ACPI. SERIAL_DEV_CTRL_TTYPORT is a serdev
controller driver, that registers serdev controller devices for each
tty. It will also be used by clients, that are registered via board
code.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Johan Hovold]

On Mon, Oct 09, 2017 at 10:55:34AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Oct 09, 2017 at 09:35:26AM +0200, Johan Hovold wrote:
> > On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> > > On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > > > On Wed, Oct 04, 2017 at 10:51:30AM +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().
> > > > > 
> > > > > Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> > > > 
> > > > So just to reiterate what I just mentioned in a comment to one of Hans's
> > > > hci_bcm patches:
> > > > 
> > > > This one would silently break PM for such devices on any system which
> > > > does not have serdev enabled (as the corresponding platform devices
> > > > would no longer be registered). And with serdev enabled, hciattach
> > > > (btattach) would start failing as the tty device would no longer be
> > > > registered (but I assume everyone is aware of that, and fine with it, by
> > > > now).
> > > > 
> > > > Perhaps the hci_bcm driver should start depending on
> > > > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> > > 
> > > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > > for serdev. If any other controller is implemented that one could
> > > also be used.
> > 
> > Not for hci_bcm, right? This particular driver specifically depends on
> > SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> > serdev controller (or currently working systems soon breaks silently).
> > 
> > I don't think the same is true for the DT case where we do not already
> > have child nodes defined in firmware (and in fact, this driver did not
> > really support DT before serdev).
> 
> The serdev ACPI support has been added to the core and not to
> the ttyport and the hci_bcm driver only uses functions from the
> core. As far as I can see the ACPI part would also work fine with
> a different serdev controller.

Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
current ACPI setups which breaks when this patch is applied (as these
devices all hang off of common serial ports managed by serial core).

> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
> since it's the only serdev controller implementation. Also it
> covers most use cases. When SERIAL_DEV_BUS is selected it's
> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.

> > > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > > it together with SERDEV. I suspect that we won't see any other
> > > controller (it would be a UART device, that is not registered as
> > > tty device) in the next few years and the extra option seems to
> > > confuse people.
> > 
> > I agree that it is somewhat confusing. But now that we have both,
> > perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> > SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> > might need to be amended as well (e.g. if only to mention that you
> > need to select a controller as well).
> 
> I think we should at least add a default "y" if SERIAL_DEV_BUS.

I'm preparing a patch.

> > And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.
> 
> Yes and that's the correct dependency. They only need the serdev
> core and controller. The only reason they do not work without
> SERIAL_DEV_CTRL_TTYPORT is, that there won't be any serdev
> controller.

In general, yes. Again, the only exception would be hci_bcm to avoid
breaking current setups without people noticing.

> Note, that the default "y" if SERIAL_DEV_BUS in SERIAL_DEV_CTRL_TTYPORT's
> config entry is only a partial fix. There is still the problem,
> that SERIAL_DEV_CTRL_TTYPORT can only be enabled if SERIAL_DEV_BUS
> is configured builtin. This is a limitation of the ttyport
> implementation, that hooks into builtin TTY core code.

I'm not saying it's a fix, but it is a sane default. I'm preparing a
patch also amending the Kconfig entries, and we can take it from there.

Johan

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

[Marcel Holtmann]

Hi Johan,

>>>>>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> 
>>>>> So just to reiterate what I just mentioned in a comment to one of Hans's
>>>>> hci_bcm patches:
>>>>> 
>>>>> This one would silently break PM for such devices on any system which
>>>>> does not have serdev enabled (as the corresponding platform devices
>>>>> would no longer be registered). And with serdev enabled, hciattach
>>>>> (btattach) would start failing as the tty device would no longer be
>>>>> registered (but I assume everyone is aware of that, and fine with it, by
>>>>> now).
>>>>> 
>>>>> Perhaps the hci_bcm driver should start depending on
>>>>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
>>>> 
>>>> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
>>>> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
>>>> for serdev. If any other controller is implemented that one could
>>>> also be used.
>>> 
>>> Not for hci_bcm, right? This particular driver specifically depends on
>>> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
>>> serdev controller (or currently working systems soon breaks silently).
>>> 
>>> I don't think the same is true for the DT case where we do not already
>>> have child nodes defined in firmware (and in fact, this driver did not
>>> really support DT before serdev).
>> 
>> The serdev ACPI support has been added to the core and not to
>> the ttyport and the hci_bcm driver only uses functions from the
>> core. As far as I can see the ACPI part would also work fine with
>> a different serdev controller.
> 
> Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
> current ACPI setups which breaks when this patch is applied (as these
> devices all hang off of common serial ports managed by serial core).
> 
>> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
>> since it's the only serdev controller implementation. Also it
>> covers most use cases. When SERIAL_DEV_BUS is selected it's
>> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> 
>>>> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
>>>> it together with SERDEV. I suspect that we won't see any other
>>>> controller (it would be a UART device, that is not registered as
>>>> tty device) in the next few years and the extra option seems to
>>>> confuse people.
>>> 
>>> I agree that it is somewhat confusing. But now that we have both,
>>> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
>>> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
>>> might need to be amended as well (e.g. if only to mention that you
>>> need to select a controller as well).
>> 
>> I think we should at least add a default "y" if SERIAL_DEV_BUS.
> 
> I'm preparing a patch.

yes, please prepare a patch since the discussion spans multiple email threads now. Lets get this back on track and find a patch that we are all happy with.

Regards

Marcel

Re: [PATCH 0/2] ACPI serdev support

[Rafael J. Wysocki]

On Wed, Oct 4, 2017 at 10:51 AM, Frédéric Danis
<frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.
>
> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
> support to the serdev drv" patches to work.
>
> Tested on T100TA with Broadcom BCM2E39.
>
> 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

Some review comments you received on this series should be addressed
IMO, so I'm expecting an update of it.

When sending the next version, please add all of the tags you've
already received on it.

Thanks,
Rafael

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

[Johan Hovold]

On Mon, Oct 09, 2017 at 08:09:19PM +0200, Marcel Holtmann wrote:
> Hi Johan,
> 
> >>>>>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>>> 
> >>>>> So just to reiterate what I just mentioned in a comment to one of Hans's
> >>>>> hci_bcm patches:
> >>>>> 
> >>>>> This one would silently break PM for such devices on any system which
> >>>>> does not have serdev enabled (as the corresponding platform devices
> >>>>> would no longer be registered). And with serdev enabled, hciattach
> >>>>> (btattach) would start failing as the tty device would no longer be
> >>>>> registered (but I assume everyone is aware of that, and fine with it, by
> >>>>> now).
> >>>>> 
> >>>>> Perhaps the hci_bcm driver should start depending on
> >>>>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> >>>> 
> >>>> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> >>>> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> >>>> for serdev. If any other controller is implemented that one could
> >>>> also be used.
> >>> 
> >>> Not for hci_bcm, right? This particular driver specifically depends on
> >>> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> >>> serdev controller (or currently working systems soon breaks silently).
> >>> 
> >>> I don't think the same is true for the DT case where we do not already
> >>> have child nodes defined in firmware (and in fact, this driver did not
> >>> really support DT before serdev).
> >> 
> >> The serdev ACPI support has been added to the core and not to
> >> the ttyport and the hci_bcm driver only uses functions from the
> >> core. As far as I can see the ACPI part would also work fine with
> >> a different serdev controller.
> > 
> > Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
> > current ACPI setups which breaks when this patch is applied (as these
> > devices all hang off of common serial ports managed by serial core).
> > 
> >> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
> >> since it's the only serdev controller implementation. Also it
> >> covers most use cases. When SERIAL_DEV_BUS is selected it's
> >> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> > 
> >>>> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> >>>> it together with SERDEV. I suspect that we won't see any other
> >>>> controller (it would be a UART device, that is not registered as
> >>>> tty device) in the next few years and the extra option seems to
> >>>> confuse people.
> >>> 
> >>> I agree that it is somewhat confusing. But now that we have both,
> >>> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> >>> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> >>> might need to be amended as well (e.g. if only to mention that you
> >>> need to select a controller as well).
> >> 
> >> I think we should at least add a default "y" if SERIAL_DEV_BUS.
> > 
> > I'm preparing a patch.
> 
> yes, please prepare a patch since the discussion spans multiple email
> threads now. Lets get this back on track and find a patch that we are
> all happy with.

I submitted a patch amending the serdev Kconfig entries and making
SERIAL_DEV_CTRL_TTYPORT default to Y when serdev support has been
chosen.

While that hopefully reduces the confusion regarding the Kconfig options
somewhat, it's not really a fix for the potential silent hci_bcm
regression that could result from this patch.

[ The problem being that hci-attach would still succeed, but PM would be
broken, when SERIAL_DEV_CTRL_TTYPORT is not set. ]

I mentioned that making HCI_BCM depend on SERIAL_DEV_CTRL_TTYPORT might
be used to avoid this situation, although it is on some level is
conceptually wrong to describe runtime dependencies in Kconfig (cf.
having a USB device driver, depend on a particular host controller
rather than just USB support).

I'll write something like that up in a patch for Bluetooth and you can
decide if you want to apply it to remedy this particular situation,
though.

Johan

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

[Marcel Holtmann]

Hi Johan,

>> 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.
>> 
>> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
>> ---
>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 94 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>> index c68fb3a..104777d 100644
>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
> 
>> @@ -385,6 +401,74 @@ 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 Serial device for %s\n",
> 
> s/Serial/serdev/
> 
>> +			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 device. status %d\n", err);
> 
> s/ACPI/ACPI serdev/?
> 
>> +		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_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
> 
> s/Serial/serdev/
> 
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
> 
> What if there are no slaves defined? I'm not very familiar with the ACPI
> helpers, but from a quick look it seems you'd then end up returning zero
> here which would cause the serdev controller to be registered instead of
> the tty-class device.
> 
> [ And if I'm mistaken, you do want to suppress that error message for
> when there are no slaves defined. ]
> 
>> +}
>> +#else
>> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
> 
> s/ctlr/ctrl/
> 
>> +{
>> +	return -ENODEV;
>> +}
>> +#endif /* CONFIG_ACPI */
>> +
>> /**
>>  * serdev_controller_add() - Add an serdev controller
>>  * @ctrl:	controller to be registered.
>> @@ -394,7 +478,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 +488,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, "serdev%d no devices registered: of:%d acpi:%d\n",
> 
> "serdev%d" is redundant here as you're using dev_dbg (which will print
> the device name).
> 
>> +			ctrl->nr, ret_of, ret_acpi);
>> +		ret = -ENODEV;
>> 		goto out_dev_del;
>> +	}
>> 
>> 	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>> 		ctrl->nr, &ctrl->dev);
> 
> Hmm, I see it's already used here. No need to follow that example
> though.

lets have an extra patch on top of it that fixes these.

Regards

Marcel

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

[Johan Hovold]

On Tue, Oct 10, 2017 at 10:10:58AM +0200, Marcel Holtmann wrote:
> Hi Johan,
> 
> >> 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.
> >> 
> >> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> >> ---
> >> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 94 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >> index c68fb3a..104777d 100644
> >> --- a/drivers/tty/serdev/core.c
> >> +++ b/drivers/tty/serdev/core.c

> >> +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_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
> > 
> > s/Serial/serdev/
> > 
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	return 0;
> > 
> > What if there are no slaves defined? I'm not very familiar with the ACPI
> > helpers, but from a quick look it seems you'd then end up returning zero
> > here which would cause the serdev controller to be registered instead of
> > the tty-class device.
> > 
> > [ And if I'm mistaken, you do want to suppress that error message for
> > when there are no slaves defined. ]

> >> -	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, "serdev%d no devices registered: of:%d acpi:%d\n",
> > 
> > "serdev%d" is redundant here as you're using dev_dbg (which will print
> > the device name).
> > 
> >> +			ctrl->nr, ret_of, ret_acpi);
> >> +		ret = -ENODEV;
> >> 		goto out_dev_del;
> >> +	}
> >> 
> >> 	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
> >> 		ctrl->nr, &ctrl->dev);
> > 
> > Hmm, I see it's already used here. No need to follow that example
> > though.
> 
> lets have an extra patch on top of it that fixes these.

Some of the above are minor nits that can be addressed by a follow-up
patch indeed, but the handling of nodes with no child devices needs to
be correct (or you could end up breaking normal serial-port support).

Johan

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

[Marcel Holtmann]

Hi Johan,

>>>> 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.
>>>> 
>>>> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
>>>> ---
>>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>>>> index c68fb3a..104777d 100644
>>>> --- a/drivers/tty/serdev/core.c
>>>> +++ b/drivers/tty/serdev/core.c
> 
>>>> +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_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
>>> 
>>> s/Serial/serdev/
>>> 
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return 0;
>>> 
>>> What if there are no slaves defined? I'm not very familiar with the ACPI
>>> helpers, but from a quick look it seems you'd then end up returning zero
>>> here which would cause the serdev controller to be registered instead of
>>> the tty-class device.
>>> 
>>> [ And if I'm mistaken, you do want to suppress that error message for
>>> when there are no slaves defined. ]
> 
>>>> -	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, "serdev%d no devices registered: of:%d acpi:%d\n",
>>> 
>>> "serdev%d" is redundant here as you're using dev_dbg (which will print
>>> the device name).
>>> 
>>>> +			ctrl->nr, ret_of, ret_acpi);
>>>> +		ret = -ENODEV;
>>>> 		goto out_dev_del;
>>>> +	}
>>>> 
>>>> 	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>>>> 		ctrl->nr, &ctrl->dev);
>>> 
>>> Hmm, I see it's already used here. No need to follow that example
>>> though.
>> 
>> lets have an extra patch on top of it that fixes these.
> 
> Some of the above are minor nits that can be addressed by a follow-up
> patch indeed, but the handling of nodes with no child devices needs to
> be correct (or you could end up breaking normal serial-port support).

that sounds good to me as well. Lets get this feature into the ACPI tree since I am going to push Hans’ patches into bluetooth-next as soon as he re-submits the missing ID change. That hopefully gives us then fully serdev support for ACPI and DT when it comes to Broadcom Bluetooth controllers.

Next step is to convert the Intel driver to also use ACPI based serdev :)

And for the brave, I think there are Realtek based systems using ACPI as well.

What I was wondering the other day is if we need a lsserdev tool or some integration in lshw to be able to debug what serdev devices and ID are present. The lsusb and /sys/kernel/debug/usb/devices is just super powerful and easy when it comes to figuring out what people have in their system. Maybe /sys/kernel/debug/serdev/devices could be helpful as well. Just thinking out loud here.

Regards

Marcel

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

[Johan Hovold]

On Tue, Oct 10, 2017 at 10:22:19AM +0200, Marcel Holtmann wrote:

> > Some of the above are minor nits that can be addressed by a follow-up
> > patch indeed, but the handling of nodes with no child devices needs to
> > be correct (or you could end up breaking normal serial-port support).
> 
> that sounds good to me as well. Lets get this feature into the ACPI
> tree since I am going to push Hans’ patches into bluetooth-next as
> soon as he re-submits the missing ID change. That hopefully gives us
> then fully serdev support for ACPI and DT when it comes to Broadcom
> Bluetooth controllers.

But with Hans's work these devices should continue to function using the
platform-device hacks until the ACPI patch eventually lands in Linus's
tree (via the acpi tree).

> Next step is to convert the Intel driver to also use ACPI based serdev
> :)
> 
> And for the brave, I think there are Realtek based systems using ACPI
> as well.
> 
> What I was wondering the other day is if we need a lsserdev tool or
> some integration in lshw to be able to debug what serdev devices and
> ID are present. The lsusb and /sys/kernel/debug/usb/devices is just
> super powerful and easy when it comes to figuring out what people have
> in their system. Maybe /sys/kernel/debug/serdev/devices could be
> helpful as well. Just thinking out loud here.

Yeah, maybe. Since you'd typically only have small number of serdev
devices (say, max 4), using /sys/bus/serial/devices directly should not
be too bad meanwhile. Not that much common information we can expose
either, at least not in comparison to USB. But I'll keep it mind. :)

Johan

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

[Ian W MORRISON]

On 11 October 2017 at 03:36, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Oct 10, 2017 at 10:22:19AM +0200, Marcel Holtmann wrote:
>>
>> What I was wondering the other day is if we need a lsserdev tool or
>> some integration in lshw to be able to debug what serdev devices and
>> ID are present. The lsusb and /sys/kernel/debug/usb/devices is just
>> super powerful and easy when it comes to figuring out what people have
>> in their system. Maybe /sys/kernel/debug/serdev/devices could be
>> helpful as well. Just thinking out loud here.
>
> Yeah, maybe. Since you'd typically only have small number of serdev
> devices (say, max 4), using /sys/bus/serial/devices directly should not
> be too bad meanwhile. Not that much common information we can expose
> either, at least not in comparison to USB. But I'll keep it mind. :)
>

Yes in the interim, if you have 'tree' installed then for example

$ alias lsserdev='tree /sys/bus/serial/devices/*-0'
$ lsserdev
/sys/bus/serial/devices/serial0-0
├── bluetooth
│   └── hci0
│       ├── device -> ../../../serial0-0
│       ├── power
│       │   ├── async
│       │   ├── autosuspend_delay_ms
│       │   ├── control
│       │   ├── runtime_active_kids
│       │   ├── runtime_active_time
│       │   ├── runtime_enabled
│       │   ├── runtime_status
│       │   ├── runtime_suspended_time
│       │   └── runtime_usage
│       ├── rfkill0
│       │   ├── device -> ../../hci0
│       │   ├── hard
│       │   ├── index
│       │   ├── name
│       │   ├── persistent
│       │   ├── power
│       │   │   ├── async
│       │   │   ├── autosuspend_delay_ms
│       │   │   ├── control
│       │   │   ├── runtime_active_kids
│       │   │   ├── runtime_active_time
│       │   │   ├── runtime_enabled
│       │   │   ├── runtime_status
│       │   │   ├── runtime_suspended_time
│       │   │   └── runtime_usage
│       │   ├── soft
│       │   ├── state
│       │   ├── subsystem -> ../../../../../../../../class/rfkill
│       │   ├── type
│       │   └── uevent
│       ├── subsystem -> ../../../../../../../class/bluetooth
│       └── uevent
├── driver -> ../../../../../bus/serial/drivers/hci_uart_bcm
├── firmware_node ->
../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/8086228A:00/BCM2EA4:00
├── modalias
├── power
│   ├── async
│   ├── autosuspend_delay_ms
│   ├── control
│   ├── runtime_active_kids
│   ├── runtime_active_time
│   ├── runtime_enabled
│   ├── runtime_status
│   ├── runtime_suspended_time
│   └── runtime_usage
├── subsystem -> ../../../../../bus/serial
└── uevent

13 directories, 38 files
$

It is a bit shabby when there is nothing to report

$ lsserdev
/sys/bus/serial/devices/*-0 [error opening dir]

0 directories, 0 files
$