Messages in this thread Patch in this message | | | Subject | Re: [PATCH] serdev: Add ACPI devices by ResourceSource field | From | Hans de Goede <> | Date | Fri, 20 Sep 2019 17:00:29 +0200 |
| |
Hi,
On 9/19/19 9:56 PM, Maximilian Luz wrote: > When registering a serdev controller, ACPI needs to be checked for > devices attached to it. Currently, all immediate children of the ACPI > node of the controller are assumed to be UART client devices for this > controller. Furthermore, these devices are not searched elsewhere. > > This is incorrect: Similar to SPI and I2C devices, the UART client > device definition (via UARTSerialBusV2) can reside anywhere in the ACPI > namespace as resource definition inside the _CRS method and points to > the controller via its ResourceSource field. This field may either > contain a fully qualified or relative path, indicating the controller > device. To address this, we need to walk over the whole ACPI namespace, > looking at each resource definition, and match the client device to the > controller via this field. > > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
So as promised I've given this patch a try, unfortunately it breaks existing users of ACPI serdev device instantation.
After adding this patch "ls /sys/bus/serial/devices" is empty, where as before it gives:
[root@dhcp-45-50 ~]# ls -l /sys/bus/serial/devices/ total 0 lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0 -> ../../../devices/pci0000:00/8086228A:00/serial0 lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0-0 -> ../../../devices/pci0000:00/8086228A:00/serial0/serial0-0
And since the serdev is missing bluetooth does not work.
(ACPI instantiated serdev is used for UART attached Blueooth HCI-s on many Cherry Trail devices).
I haven't looked why your patch is breakig things, I have a large backlog so I do not have time for that.
But if you can provide me with a version of the patch with a bunch of debug printk-s added I'm happy to run that for you.
I'll also send you the DSDT of the device I tested on off-list.
Regards,
Hans
> --- > This patch is similar to the the implementations in drivers/spi/spi.c > (see commit 4c3c59544f33e97cf8557f27e05a9904ead16363) and > drivers/i2c/i2c-core-acpi.c. However, I think that there may be an > issues with these two implementations: Both walk over the whole ACPI > namespace, but only match the first SPI or I2C resource (respectively), > so I think there may be problems when multiple SPI or I2C resources are > defined under the same ACPI device node (as in second or third SPI/I2C > resource definitions being ignored). Please note, however, that I am by > no means qualified with regards to this, and this might be totally fine. > Nevertheless I'd appreciate if anyone with more knowledge on the subject > could have a look at it. This patch would avoid this problem (for UART) > by simply walking all resource definitions via acpi_walk_resources. > > There is a further issue in the serdev ACPI implementation that this > patch does not address: ACPI UART resource definitions contain things > like the initial baud-rate, parity, flow-control, etc. As far as I know, > these things can currently only be set once the device is opened. > Furthermore, some option values, such as ParityTypeMark, are not (yet) > supported. I'd be willing to try and implement setting the currently > supported values based on ACPI for a future patch, if anyone can provide > me with some pointers on how to do that. > > I have personally tested this patch on a Microsoft Surface Book 2, which > like all newer MS Surface devices has a UART EC, and it has been in use > (in some form or another) for a couple of months on other Surface > devices via a patched kernel [1, 2, 3]. I can, however, not speak for > any non-Microsoft devices or potential Apple ACPI quirks. > > [1]: https://github.com/jakeday/linux-surface/ > [2]: https://github.com/qzed/linux-surface/ > [3]: https://github.com/qzed/linux-surfacegen5-acpi/ > > drivers/tty/serdev/core.c | 64 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 56 insertions(+), 8 deletions(-) > > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c > index a0ac16ee6575..1c8360deea77 100644 > --- a/drivers/tty/serdev/core.c > +++ b/drivers/tty/serdev/core.c > @@ -582,18 +582,64 @@ static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl, > return AE_OK; > } > > -static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level, > - void *data, void **return_value) > +struct acpi_serdev_resource_context { > + struct serdev_controller *controller; > + struct acpi_device *device; > +}; > + > +static acpi_status > +acpi_serdev_add_device_from_resource(struct acpi_resource *resource, void *data) > { > - struct serdev_controller *ctrl = data; > - struct acpi_device *adev; > + struct acpi_serdev_resource_context *ctx > + = (struct acpi_serdev_resource_context *)data; > + struct acpi_resource_source *ctrl_name; > + acpi_handle ctrl_handle; > + > + if (resource->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) > + return AE_OK; > > - if (acpi_bus_get_device(handle, &adev)) > + if (resource->data.common_serial_bus.type > + != ACPI_RESOURCE_SERIAL_TYPE_UART) > return AE_OK; > > - return acpi_serdev_register_device(ctrl, adev); > + ctrl_name = &resource->data.common_serial_bus.resource_source; > + if (ctrl_name->string_length == 0 || !ctrl_name->string_ptr) > + return AE_OK; > + > + if (acpi_get_handle(ctx->device->handle, ctrl_name->string_ptr, > + &ctrl_handle)) > + return AE_OK; > + > + if (ctrl_handle == ACPI_HANDLE(ctx->controller->dev.parent)) > + return acpi_serdev_register_device(ctx->controller, > + ctx->device); > + > + return AE_OK; > } > > +static acpi_status > +acpi_serdev_add_devices_from_resources(acpi_handle handle, u32 level, > + void *data, void **return_value) > +{ > + struct acpi_serdev_resource_context ctx; > + acpi_status status; > + > + ctx.controller = (struct serdev_controller *)data; > + status = acpi_bus_get_device(handle, &ctx.device); > + if (status) > + return AE_OK; // ignore device if not present > + > + status = acpi_walk_resources(handle, METHOD_NAME__CRS, > + acpi_serdev_add_device_from_resource, > + &ctx); > + if (status == AE_NOT_FOUND) > + return AE_OK; // ignore if _CRS is not found > + else > + return status; > +} > + > +#define SERDEV_ACPI_ENUMERATE_MAX_DEPTH 32 > + > static int acpi_serdev_register_devices(struct serdev_controller *ctrl) > { > acpi_status status; > @@ -603,8 +649,10 @@ static int acpi_serdev_register_devices(struct serdev_controller *ctrl) > if (!handle) > return -ENODEV; > > - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > - acpi_serdev_add_device, NULL, ctrl, NULL); > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > + SERDEV_ACPI_ENUMERATE_MAX_DEPTH, > + acpi_serdev_add_devices_from_resources, > + NULL, ctrl, NULL); > if (ACPI_FAILURE(status)) > dev_dbg(&ctrl->dev, "failed to enumerate serdev slaves\n"); > >
| |