| Message ID | 1507107090-15992-3-git-send-email-frederic.danis.oss@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 >
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? Johan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; };
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(-)