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_slave to reflect this.
As this is only tested on T100TA with Broadcom BCM2E39, I moved this to a
specific acpi_match_table.
Frédéric Danis (3):
serdev: Add ACPI support
ACPI / scan: Fix enumeration for special UART devices
Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39
drivers/acpi/scan.c | 29 ++++++-------
drivers/bluetooth/hci_bcm.c | 10 ++++-
drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++---
include/acpi/acpi_bus.h | 2 +-
4 files changed, 116 insertions(+), 24 deletions(-)
--
2.7.4
Hi Marcel,
Le 29/09/2017 à 14:00, Marcel Holtmann a écrit :
> Hi Fred,
>
>>>>> +#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;
>>>>> +}
>>>> how are we ensuring that we only take UART devices into account here?
>>> 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.
>> sounds good to me. Can you respin this series with the other comments addressed and maybe add some extra comments in the code or the commit message to make this clear.
> any updates on this?
While working on it, I tried to add the PM support
(bcm_serdev_driver.driver.pm = &bcm_pm_ops) but it ends up in kernel
freeze during suspend to ram test.
I may have found the problem but do not have access to T100 now to test
it (this may take some times as I work on it on spare time).
Regards,
Fred
Hi Fred,
>>>> +#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;
>>>> +}
>>> how are we ensuring that we only take UART devices into account here?
>>
>> 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.
>
> sounds good to me. Can you respin this series with the other comments addressed and maybe add some extra comments in the code or the commit message to make this clear.
any updates on this?
Regards
Marcel
Hi Fred,
>>> +#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;
>>> +}
>> how are we ensuring that we only take UART devices into account here?
>
> 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.
sounds good to me. Can you respin this series with the other comments addressed and maybe add some extra comments in the code or the commit message to make this clear.
Regards
Marcel
Hi Lukas,
>>>> --- 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_slave:1;
>>>> u32 reserved:19;
>>>> };
>>> I am not an ACPI expert, but wouldn't we better have a serial_bus_slave
>>> here. And the serial_bus_slave can be either UART or I2C? Or have a
>>> pretty good commit message explaining why this is serial_slave only.
>>
>> I will rename it.
>> serial_bus_slave can be either SPI, I2C or UART
>> (cf. http://elixir.free-electrons.com/linux/latest/source/include/acpi/acrestyp.h#L446).
>
> Another idea would be to describe the *effect*, rather than the devices
> affected, i.e. something like:
>
> - u32 spi_i2c_slave:1;
> + u32 parent_enumerated:1;
>
> such that it could be extended to slaves on a non-serial bus in the
> future.
that might work as well. However right now I would be fine with serial_bus_slave here. Since that makes it clear that this is more than just UART devices. It is generic serial bus based devices.
Regards
Marcel
On Sat, Sep 09, 2017 at 03:46:07PM +0200, Fr?d?ric Danis wrote:
> Le 07/09/2017 ? 19:25, Marcel Holtmann a ?crit :
> > >--- 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_slave:1;
> > > u32 reserved:19;
> > >};
> > I am not an ACPI expert, but wouldn't we better have a serial_bus_slave
> > here. And the serial_bus_slave can be either UART or I2C? Or have a
> > pretty good commit message explaining why this is serial_slave only.
>
> I will rename it.
> serial_bus_slave can be either SPI, I2C or UART
> (cf. http://elixir.free-electrons.com/linux/latest/source/include/acpi/acrestyp.h#L446).
Another idea would be to describe the *effect*, rather than the devices
affected, i.e. something like:
- u32 spi_i2c_slave:1;
+ u32 parent_enumerated:1;
such that it could be extended to slaves on a non-serial bus in the
future.
Thanks,
Lukas
Hi Marcel,
Le 07/09/2017 à 19:21, Marcel Holtmann a écrit :
<snip>
>> +#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;
>> +}
> how are we ensuring that we only take UART devices into account here?
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.
Regards,
Fred
Hi Marcel,
Le 07/09/2017 à 19:25, Marcel Holtmann a écrit :
> Hi Fred,
>
>> UART devices is expected to be enumerated by SerDev subsystem.
>> Rename spi_i2c_slave to serial_slave as this is no more specific to spi or
>> i2c buses.
>>
>> Signed-off-by: Frédéric Danis <[email protected]>
>> ---
>> drivers/acpi/scan.c | 29 ++++++++++++-----------------
>> include/acpi/acpi_bus.h | 2 +-
>> 2 files changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 70fd550..04c5ecc 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -1429,35 +1429,30 @@ 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_slave(struct acpi_resource *ares, void *data)
>> {
>> - bool *is_spi_i2c_slave_p = data;
>> + bool *is_serial_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_slave_p = true;
>>
>> /* no need to do more checking */
>> return -1;
>> }
>>
> isn’t this disabled the I2C support? I mean serial bus doesn’t always mean UART. There are other serial buses and even USB is actually a serial bus.
This will not disable I2C or SPI support but will add UART slave devices
to the devices which are not enumerated during ACPI scan, allowing them
do be enumerated by their respective parents, SerDev in case of UART.
>
>> -static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
>> +static bool acpi_is_serial_slave(struct acpi_device *device)
>> {
>> struct list_head resource_list;
>> - bool is_spi_i2c_slave = false;
>> + bool is_serial_slave = false;
>>
>> 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_slave,
>> + &is_serial_slave);
>> acpi_dev_free_resource_list(&resource_list);
>>
>> - return is_spi_i2c_slave;
>> + return is_serial_slave;
>> }
>>
>> void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
>> @@ -1476,7 +1471,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_slave = acpi_is_serial_slave(device);
>> acpi_device_clear_enumerated(device);
>> device_initialize(&device->dev);
>> dev_set_uevent_suppress(&device->dev, true);
>> @@ -1763,7 +1758,7 @@ static void acpi_default_enumeration(struct acpi_device *device)
>> * Do not enumerate SPI/I2C slaves as they will be enumerated by their
>> * respective parents.
>> */
>> - if (!device->flags.spi_i2c_slave) {
>> + if (!device->flags.serial_slave) {
>> acpi_create_platform_device(device, NULL);
>> acpi_device_set_enumerated(device);
>> } else {
>> @@ -1860,7 +1855,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_slave) {
>> acpi_device_set_enumerated(device);
>> goto ok;
>> }
>> @@ -1869,7 +1864,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_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 68bc6be..49a82f8 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_slave:1;
>> u32 reserved:19;
>> };
> I am not an ACPI expert, but wouldn’t we better have a serial_bus_slave here. And the serial_bus_slave can be either UART or I2C? Or have a pretty good commit message explaining why this is serial_slave only.
I will rename it.
serial_bus_slave can be either SPI, I2C or UART (cf.
http://elixir.free-electrons.com/linux/latest/source/include/acpi/acrestyp.h#L446).
Regards,
Fred
On Thu, Sep 07, 2017 at 02:10:13PM +0200, Fr?d?ric Danis wrote:
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> -static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
> +static bool acpi_is_serial_slave(struct acpi_device *device)
> {
> struct list_head resource_list;
> - bool is_spi_i2c_slave = false;
> + bool is_serial_slave = false;
>
> 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_slave,
> + &is_serial_slave);
> acpi_dev_free_resource_list(&resource_list);
>
> - return is_spi_i2c_slave;
> + return is_serial_slave;
> }
Commit ca9ef3ab68d3 ("ACPI / scan: Recognize Apple SPI and I2C slaves")
which landed in Linus' tree a few days ago changes the function above
to check for Apple device properties if running on an x86 Mac.
When rebasing, please amend the function to check for:
fwnode_property_present(&device->fwnode, "baud")
On Macs an empty ResourceTemplate is returned for uart slaves.
Instead the device properties "baud", "parity", "dataBits", "stopBits"
are provided. An excerpt of the DSDT on a MacBook8,1 is below.
An experimental patch for hci_bcm.c to take advantage of the baud
property is here, though I'm told it doesn't work yet (I don't have
such a machine myself to test):
https://github.com/l1k/linux/commit/8883d6225a92
Discussion on this patch is here:
https://github.com/Dunedan/mbp-2016-linux/issues/29
Thanks,
Lukas
-- cut here --
Scope (\_SB.PCI0.URT0)
{
Device (BLTH)
{
Name (_HID, EisaId ("BCM2E7C")) // _HID: Hardware ID
Name (_CID, "apple-uart-blth") // _CID: Compatible ID
Name (_UID, 0x01) // _UID: Unique ID
Name (_ADR, 0x00) // _ADR: Address
Method (_STA, 0, NotSerialized) // _STA: Status
{
Return (0x0F)
}
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
Name (UBUF, ResourceTemplate ()
{
UartSerialBus (0x0001C200, DataBitsEight, StopBitsOne,
0xC0, LittleEndian, ParityTypeNone, FlowControlHardware,
0x0020, 0x0020, "\\_SB.PCI0.URT0",
0x00, ResourceProducer, ,
)
})
Name (ABUF, ResourceTemplate ()
{
})
If (LNot (OSDW ()))
{
Return (UBUF) /* \_SB_.PCI0.URT0.BLTH._CRS.UBUF */
}
Return (ABUF) /* \_SB_.PCI0.URT0.BLTH._CRS.ABUF */
}
Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
{
If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
{
Store (Package (0x08)
{
"baud",
Buffer (0x08)
{
0xC0, 0xC6, 0x2D, 0x00, 0x00, 0x00, 0x00, 0x00 /* ..-..... */
},
"parity",
Buffer (0x08)
{
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
},
"dataBits",
Buffer (0x08)
{
0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
},
"stopBits",
Buffer (0x08)
{
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* ........ */
}
}, Local0)
DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
Return (Local0)
}
Return (0x00)
}
On Thu, Sep 7, 2017 at 12:57 PM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Rob,
>
>>>> Signed-off-by: Fr=C3=A9d=C3=A9ric Danis <[email protected]>
>>>> ---
>>>> drivers/tty/serdev/core.c | 99 +++++++++++++++++++++++++++++++++++++++=
+++++---
>>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>
>> [...]
>>
>>>> @@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controlle=
r *ctrl)
>>>> if (ret)
>>>> return ret;
>>>>
>>>> - ret =3D of_serdev_register_devices(ctrl);
>>>> - if (ret)
>>>> + ret_of =3D of_serdev_register_devices(ctrl);
>>>> + ret_acpi =3D 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 =3D -ENODEV;
>>>> goto out_dev_del;
>>>> + }
>>>>
>>>> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>>>> ctrl->nr, &ctrl->dev);
>>>
>>> Shouldn=E2=80=99t we just consider to always register the controller? E=
ven if there are no devices attached to it.
>>
>> You argued for the opposite at least in regards to a serdev ldisc. :)
>> The problem is we use the success or failure here to decide if we
>> create a tty char dev or not. I guess we could move that decision out
>> of the core and let the tty code check for devices and decide.
>
> we never got the serdev ldisc working. Is there still an attempt to get t=
his working. I frankly don=E2=80=99t know what is best here.
I did get it somewhat working, but haven't been working on it more.
Here's what I said in my last mail to you:
> Okay, I've got this kind of working. I can run "ldattach HCI
> /dev/ttyblah" to start the LL driver (nothing attached, just tries
> firmware loading), then kill ldattach and remove the serdev device.
> And it actually works a 2nd time. I've updated my serdev-ldisc branch
> with the fixes. The real question is what have I broken in the tty
> device. At least "echo rob > /dev/ttyS1" still works (in qemu) after
> attaching and de-attaching.
I pushed out my work to serdev-ldisc-v2 branch on my k.org tree.
Rob
Hi Rob,
>>> Signed-off-by: Frédéric Danis <[email protected]>
>>> ---
>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>
> [...]
>
>>> @@ -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);
>>
>> Shouldn’t we just consider to always register the controller? Even if there are no devices attached to it.
>
> You argued for the opposite at least in regards to a serdev ldisc. :)
> The problem is we use the success or failure here to decide if we
> create a tty char dev or not. I guess we could move that decision out
> of the core and let the tty code check for devices and decide.
we never got the serdev ldisc working. Is there still an attempt to get this working. I frankly don’t know what is best here.
Regards
Marcel
On Thu, Sep 7, 2017 at 12:21 PM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Fred,
>
>> Signed-off-by: Fr=C3=A9d=C3=A9ric Danis <[email protected]>
>> ---
>> drivers/tty/serdev/core.c | 99 +++++++++++++++++++++++++++++++++++++++++=
+++---
>> 1 file changed, 94 insertions(+), 5 deletions(-)
[...]
>> @@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller =
*ctrl)
>> if (ret)
>> return ret;
>>
>> - ret =3D of_serdev_register_devices(ctrl);
>> - if (ret)
>> + ret_of =3D of_serdev_register_devices(ctrl);
>> + ret_acpi =3D 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 =3D -ENODEV;
>> goto out_dev_del;
>> + }
>>
>> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>> ctrl->nr, &ctrl->dev);
>
> Shouldn=E2=80=99t we just consider to always register the controller? Eve=
n if there are no devices attached to it.
You argued for the opposite at least in regards to a serdev ldisc. :)
The problem is we use the success or failure here to decide if we
create a tty char dev or not. I guess we could move that decision out
of the core and let the tty code check for devices and decide.
Rob
Hi Fred,
> Signed-off-by: Frédéric Danis <[email protected]>
> ---
> drivers/bluetooth/hci_bcm.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 2e358cc..b1cf07e 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -922,7 +922,6 @@ static const struct hci_uart_proto bcm_proto = {
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id bcm_acpi_match[] = {
> { "BCM2E1A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> - { "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> { "BCM2E3A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> { "BCM2E3D", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> { "BCM2E3F", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> @@ -942,6 +941,14 @@ static const struct acpi_device_id bcm_acpi_match[] = {
> MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
> #endif
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id bcm_serdev_acpi_match[] = {
> + { "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, bcm_serdev_acpi_match);
> +#endif
> +
> /* Platform suspend and resume callbacks */
> static const struct dev_pm_ops bcm_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
> @@ -999,6 +1006,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
> .driver = {
> .name = "hci_uart_bcm",
> .of_match_table = of_match_ptr(bcm_bluetooth_of_match),
> + .acpi_match_table = ACPI_PTR(bcm_serdev_acpi_match),
> },
> };
I think doing this one device at a time is actually fine. However please add a proper commit message for it explaining it.
Regards
Marcel
Hi Fred,
> UART devices is expected to be enumerated by SerDev subsystem.
> Rename spi_i2c_slave to serial_slave as this is no more specific to spi or
> i2c buses.
>
> Signed-off-by: Frédéric Danis <[email protected]>
> ---
> drivers/acpi/scan.c | 29 ++++++++++++-----------------
> include/acpi/acpi_bus.h | 2 +-
> 2 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 70fd550..04c5ecc 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1429,35 +1429,30 @@ 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_slave(struct acpi_resource *ares, void *data)
> {
> - bool *is_spi_i2c_slave_p = data;
> + bool *is_serial_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_slave_p = true;
>
> /* no need to do more checking */
> return -1;
> }
>
isn’t this disabled the I2C support? I mean serial bus doesn’t always mean UART. There are other serial buses and even USB is actually a serial bus.
> -static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
> +static bool acpi_is_serial_slave(struct acpi_device *device)
> {
> struct list_head resource_list;
> - bool is_spi_i2c_slave = false;
> + bool is_serial_slave = false;
>
> 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_slave,
> + &is_serial_slave);
> acpi_dev_free_resource_list(&resource_list);
>
> - return is_spi_i2c_slave;
> + return is_serial_slave;
> }
>
> void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
> @@ -1476,7 +1471,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_slave = acpi_is_serial_slave(device);
> acpi_device_clear_enumerated(device);
> device_initialize(&device->dev);
> dev_set_uevent_suppress(&device->dev, true);
> @@ -1763,7 +1758,7 @@ static void acpi_default_enumeration(struct acpi_device *device)
> * Do not enumerate SPI/I2C slaves as they will be enumerated by their
> * respective parents.
> */
> - if (!device->flags.spi_i2c_slave) {
> + if (!device->flags.serial_slave) {
> acpi_create_platform_device(device, NULL);
> acpi_device_set_enumerated(device);
> } else {
> @@ -1860,7 +1855,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_slave) {
> acpi_device_set_enumerated(device);
> goto ok;
> }
> @@ -1869,7 +1864,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_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 68bc6be..49a82f8 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_slave:1;
> u32 reserved:19;
> };
I am not an ACPI expert, but wouldn’t we better have a serial_bus_slave here. And the serial_bus_slave can be either UART or I2C? Or have a pretty good commit message explaining why this is serial_slave only.
Regards
Marcel
Hi Fred,
> Signed-off-by: Frédéric Danis <[email protected]>
> ---
> 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 ae1aaa0..923dd4ad 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;
> +}
how are we ensuring that we only take UART devices into account here?
> +#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);
Shouldn’t we just consider to always register the controller? Even if there are no devices attached to it.
Regards
Marcel
Hi Greg,
>> Signed-off-by: Frédéric Danis <[email protected]>
>> ---
>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 94 insertions(+), 5 deletions(-)
>
> I can not take patches without any changelog text, and you should feel
> bad about trying to get me to do so :)
I wouldn’t expect you to take RFC patches :)
Regards
Marcel
On Thu, Sep 07, 2017 at 02:10:12PM +0200, Fr?d?ric Danis wrote:
> Signed-off-by: Fr?d?ric Danis <[email protected]>
> ---
> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 94 insertions(+), 5 deletions(-)
I can not take patches without any changelog text, and you should feel
bad about trying to get me to do so :)
thanks,
greg k-h
Signed-off-by: Frédéric Danis <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 2e358cc..b1cf07e 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -922,7 +922,6 @@ static const struct hci_uart_proto bcm_proto = {
#ifdef CONFIG_ACPI
static const struct acpi_device_id bcm_acpi_match[] = {
{ "BCM2E1A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
- { "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
{ "BCM2E3A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
{ "BCM2E3D", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
{ "BCM2E3F", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
@@ -942,6 +941,14 @@ static const struct acpi_device_id bcm_acpi_match[] = {
MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
#endif
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id bcm_serdev_acpi_match[] = {
+ { "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, bcm_serdev_acpi_match);
+#endif
+
/* Platform suspend and resume callbacks */
static const struct dev_pm_ops bcm_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
@@ -999,6 +1006,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
.driver = {
.name = "hci_uart_bcm",
.of_match_table = of_match_ptr(bcm_bluetooth_of_match),
+ .acpi_match_table = ACPI_PTR(bcm_serdev_acpi_match),
},
};
--
2.7.4
UART devices is expected to be enumerated by SerDev subsystem.
Rename spi_i2c_slave to serial_slave as this is no more specific to spi or
i2c buses.
Signed-off-by: Frédéric Danis <[email protected]>
---
drivers/acpi/scan.c | 29 ++++++++++++-----------------
include/acpi/acpi_bus.h | 2 +-
2 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 70fd550..04c5ecc 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1429,35 +1429,30 @@ 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_slave(struct acpi_resource *ares, void *data)
{
- bool *is_spi_i2c_slave_p = data;
+ bool *is_serial_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_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_slave(struct acpi_device *device)
{
struct list_head resource_list;
- bool is_spi_i2c_slave = false;
+ bool is_serial_slave = false;
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_slave,
+ &is_serial_slave);
acpi_dev_free_resource_list(&resource_list);
- return is_spi_i2c_slave;
+ return is_serial_slave;
}
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
@@ -1476,7 +1471,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_slave = acpi_is_serial_slave(device);
acpi_device_clear_enumerated(device);
device_initialize(&device->dev);
dev_set_uevent_suppress(&device->dev, true);
@@ -1763,7 +1758,7 @@ static void acpi_default_enumeration(struct acpi_device *device)
* Do not enumerate SPI/I2C slaves as they will be enumerated by their
* respective parents.
*/
- if (!device->flags.spi_i2c_slave) {
+ if (!device->flags.serial_slave) {
acpi_create_platform_device(device, NULL);
acpi_device_set_enumerated(device);
} else {
@@ -1860,7 +1855,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_slave) {
acpi_device_set_enumerated(device);
goto ok;
}
@@ -1869,7 +1864,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_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 68bc6be..49a82f8 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_slave:1;
u32 reserved:19;
};
--
2.7.4
Signed-off-by: Frédéric Danis <[email protected]>
---
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 ae1aaa0..923dd4ad 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
Hi,
On 02-10-17 09:07, Hans de Goede wrote:
> Hi,
>
> First of all Frédéric, thank you very much for working on this,
> this is a very much needed patch series.
>
> I've been testing this on a GPD pocket which has a BCM2E7E
> ACPI device for the BT which is part of the BCM43562A wifi/bt
> combo on this device.
>
> 2 remarks:
>
> 1) The following errors show up with this series:
>
> [ 87.059091] synth uevent: /devices/pci0000:00/8086228A:00/serial1: failed to send uevent
> [ 87.059097] serial serial1: uevent: failed to send synthetic uevent
> [ 87.059178] synth uevent: /devices/pci0000:00/8086228A:01/serial2: failed to send uevent
> [ 87.059180] serial serial2: uevent: failed to send synthetic uevent
> [ 87.062665] synth uevent: /devices/pnp0/00:01/serial0: failed to send uevent
> [ 87.062671] serial serial0: uevent: failed to send synthetic uevent
>
> This needs to be fixed :) I haven't looked into this yet.
>
> 2) As I already suspected when reading the patches, we cannot
> move the ACPI ids over 1 by 1. The first 2 patches in the series
> cause all BCM2E* ACPI devices to no longer get enumerated as
> platform devices, instead they now get enumerated as serdevs.
>
> So adding only the known to work ACPI ids to the acpi_match_table
> for the bcm_serdev_driver has the undesirable result that it makes
> sure that the other ACPI-ids will not work, as there are not
> platform devices instantiated for the platform driver to bind to.
>
> I started with simplifying your patch to this:
>
> From f8728f5440c85f7e5584ac1eafa1b090b2042276 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Danis?= <[email protected]>
> Date: Thu, 7 Sep 2017 14:10:14 +0200
> Subject: [PATCH] Bluetooth: hci_bcm: Make ACPI enumerated HCIs use serdev
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Now that the ACPI subsys instantiates ACPI enumerated HCIs as serdevs,
> the ACPI ids for them should be part of the bcm_serdev_driver's
> acpi_match_table.
>
> Signed-off-by: Frédéric Danis <[email protected]>
> [hdegoede: Move all ACPI ids over]
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> drivers/bluetooth/hci_bcm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 2285ca673ae3..4db777bb0049 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -942,7 +942,6 @@ static struct platform_driver bcm_driver = {
> .remove = bcm_remove,
> .driver = {
> .name = "hci_bcm",
> - .acpi_match_table = ACPI_PTR(bcm_acpi_match),
> .pm = &bcm_pm_ops,
> },
> };
> @@ -988,6 +987,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
> .driver = {
> .name = "hci_uart_bcm",
> .of_match_table = of_match_ptr(bcm_bluetooth_of_match),
> + .acpi_match_table = ACPI_PTR(bcm_acpi_match),
> },
> };
>
> --
> 2.14.2
>
> Unfortunately that is not good enough. The platform driver code also
> has (runtime) pm support using gpios and a host-wake interrupt, which
> the serdev code all lacks and without driving the shutdown gpio to not
> shutdown the BT device connected to the UART we not only are lacking
> pm support, but BT does not work at all.
>
> So yesterday evening I did a patch to merge all that into the serdev hci_bcm
> code and throw away the platform code as I don't think any users will be left
> after this. With this functionality for the BCM2E7E ACPI device is restored
> again, without needing to do a btattach from userspace, which is great :)
>
> I've attached the resulting patch. In the mean time I've realized that
> this approach is going to make merging hard. So I'm going to redo the changes
> so that we can have both a complete (with runtime-pm, gpios, etc.) serdev
> driver and keep the platform driver for now. Then we can first merge the
> hci_bcm changes to make the serdev driver side complete and once those are
> merged merge the matching ACPI subsys changes without having a window in
> between where things will not work.
Ok, I've just finished and send out v1 of a patch-series adding (runtime)pm,
GPIO and IRQ support to the serdev based code paths in hci_bcm.c .
Regards,
Hans
Hi,
First of all Frédéric, thank you very much for working on this,
this is a very much needed patch series.
I've been testing this on a GPD pocket which has a BCM2E7E
ACPI device for the BT which is part of the BCM43562A wifi/bt
combo on this device.
2 remarks:
1) The following errors show up with this series:
[ 87.059091] synth uevent: /devices/pci0000:00/8086228A:00/serial1: failed to send uevent
[ 87.059097] serial serial1: uevent: failed to send synthetic uevent
[ 87.059178] synth uevent: /devices/pci0000:00/8086228A:01/serial2: failed to send uevent
[ 87.059180] serial serial2: uevent: failed to send synthetic uevent
[ 87.062665] synth uevent: /devices/pnp0/00:01/serial0: failed to send uevent
[ 87.062671] serial serial0: uevent: failed to send synthetic uevent
This needs to be fixed :) I haven't looked into this yet.
2) As I already suspected when reading the patches, we cannot
move the ACPI ids over 1 by 1. The first 2 patches in the series
cause all BCM2E* ACPI devices to no longer get enumerated as
platform devices, instead they now get enumerated as serdevs.
So adding only the known to work ACPI ids to the acpi_match_table
for the bcm_serdev_driver has the undesirable result that it makes
sure that the other ACPI-ids will not work, as there are not
platform devices instantiated for the platform driver to bind to.
I started with simplifying your patch to this:
From f8728f5440c85f7e5584ac1eafa1b090b2042276 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Danis?= <[email protected]>
Date: Thu, 7 Sep 2017 14:10:14 +0200
Subject: [PATCH] Bluetooth: hci_bcm: Make ACPI enumerated HCIs use serdev
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Now that the ACPI subsys instantiates ACPI enumerated HCIs as serdevs,
the ACPI ids for them should be part of the bcm_serdev_driver's
acpi_match_table.
Signed-off-by: Frédéric Danis <[email protected]>
[hdegoede: Move all ACPI ids over]
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 2285ca673ae3..4db777bb0049 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -942,7 +942,6 @@ static struct platform_driver bcm_driver = {
.remove = bcm_remove,
.driver = {
.name = "hci_bcm",
- .acpi_match_table = ACPI_PTR(bcm_acpi_match),
.pm = &bcm_pm_ops,
},
};
@@ -988,6 +987,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
.driver = {
.name = "hci_uart_bcm",
.of_match_table = of_match_ptr(bcm_bluetooth_of_match),
+ .acpi_match_table = ACPI_PTR(bcm_acpi_match),
},
};
--
2.14.2
Unfortunately that is not good enough. The platform driver code also
has (runtime) pm support using gpios and a host-wake interrupt, which
the serdev code all lacks and without driving the shutdown gpio to not
shutdown the BT device connected to the UART we not only are lacking
pm support, but BT does not work at all.
So yesterday evening I did a patch to merge all that into the serdev hci_bcm
code and throw away the platform code as I don't think any users will be left
after this. With this functionality for the BCM2E7E ACPI device is restored
again, without needing to do a btattach from userspace, which is great :)
I've attached the resulting patch. In the mean time I've realized that
this approach is going to make merging hard. So I'm going to redo the changes
so that we can have both a complete (with runtime-pm, gpios, etc.) serdev
driver and keep the platform driver for now. Then we can first merge the
hci_bcm changes to make the serdev driver side complete and once those are
merged merge the matching ACPI subsys changes without having a window in
between where things will not work.
###
On a related note this series does expose a module load ordering issue.
With this series and with the dw_dmac driver built as module I get:
[ 80.239270] ttyS4 - failed to request DMA
[ 80.316338] dw_dmac INTL9C60:00: DesignWare DMA Controller, 8 channels
[ 80.353639] dw_dmac INTL9C60:01: DesignWare DMA Controller, 8 channels
Building in dw_dmac is a workaround for this, anyone has any idea how to
fix this ?
Regards,
Hans
On 07-09-17 14:10, Frédéric Danis wrote:
> Signed-off-by: Frédéric Danis <[email protected]>
> ---
> drivers/bluetooth/hci_bcm.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 2e358cc..b1cf07e 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -922,7 +922,6 @@ static const struct hci_uart_proto bcm_proto = {
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id bcm_acpi_match[] = {
> { "BCM2E1A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> - { "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> { "BCM2E3A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> { "BCM2E3D", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> { "BCM2E3F", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> @@ -942,6 +941,14 @@ static const struct acpi_device_id bcm_acpi_match[] = {
> MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
> #endif
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id bcm_serdev_acpi_match[] = {
> + { "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, bcm_serdev_acpi_match);
> +#endif
> +
> /* Platform suspend and resume callbacks */
> static const struct dev_pm_ops bcm_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
> @@ -999,6 +1006,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
> .driver = {
> .name = "hci_uart_bcm",
> .of_match_table = of_match_ptr(bcm_bluetooth_of_match),
> + .acpi_match_table = ACPI_PTR(bcm_serdev_acpi_match),
> },
> };
>
>