Now that ACPI and DT devices are both enumerated as serdevs, we can
remove platform_device support and the bcm_device_list lookup hack.
This also removes any races between suspend/resume and hci-uart binding,
also making the suspend/resume code a lot simpler.
This commit leaves manually binding to an uart using btattach supported
(without irq/gpio and thus suspend/resume support, as before).
Cc: Lukas Wunner <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 260 +++++---------------------------------------
1 file changed, 28 insertions(+), 232 deletions(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 64800cd2796c..61f73cc4c05e 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -30,7 +30,6 @@
#include <linux/of.h>
#include <linux/property.h>
#include <linux/platform_data/x86/apple.h>
-#include <linux/platform_device.h>
#include <linux/clk.h>
#include <linux/gpio/consumer.h>
#include <linux/tty.h>
@@ -80,9 +79,6 @@
* set to 0 if @init_speed is already the preferred baudrate
* @irq: interrupt triggered by HOST_WAKE_BT pin
* @irq_active_low: whether @irq is active low
- * @hu: pointer to HCI UART controller struct,
- * used to disable flow control during runtime suspend and system sleep
- * @is_suspended: whether flow control is currently disabled
*/
struct bcm_device {
/* Must be the first member, hci_serdev.c expects this. */
@@ -107,25 +103,14 @@ struct bcm_device {
u32 oper_speed;
int irq;
bool irq_active_low;
-
-#ifdef CONFIG_PM
- struct hci_uart *hu;
- bool is_suspended;
-#endif
};
/* generic bcm uart resources */
struct bcm_data {
struct sk_buff *rx_skb;
struct sk_buff_head txq;
-
- struct bcm_device *dev;
};
-/* List of BCM BT UART devices */
-static DEFINE_MUTEX(bcm_device_lock);
-static LIST_HEAD(bcm_device_list);
-
static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
{
if (hu->serdev)
@@ -183,27 +168,6 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
return 0;
}
-/* bcm_device_exists should be protected by bcm_device_lock */
-static bool bcm_device_exists(struct bcm_device *device)
-{
- struct list_head *p;
-
-#ifdef CONFIG_PM
- /* Devices using serdev always exist */
- if (device && device->hu && device->hu->serdev)
- return true;
-#endif
-
- list_for_each(p, &bcm_device_list) {
- struct bcm_device *dev = list_entry(p, struct bcm_device, list);
-
- if (device == dev)
- return true;
- }
-
- return false;
-}
-
static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
{
int err;
@@ -249,21 +213,17 @@ static irqreturn_t bcm_host_wake(int irq, void *data)
return IRQ_HANDLED;
}
-static int bcm_request_irq(struct bcm_data *bcm)
+static int bcm_request_irq(struct hci_uart *hu)
{
- struct bcm_device *bdev = bcm->dev;
+ struct bcm_device *bdev;
int err;
- mutex_lock(&bcm_device_lock);
- if (!bcm_device_exists(bdev)) {
- err = -ENODEV;
- goto unlock;
- }
+ if (!hu->serdev)
+ return -ENODEV;
- if (bdev->irq <= 0) {
- err = -EOPNOTSUPP;
- goto unlock;
- }
+ bdev = serdev_device_get_drvdata(hu->serdev);
+ if (bdev->irq <= 0)
+ return -EOPNOTSUPP;
err = devm_request_irq(bdev->dev, bdev->irq, bcm_host_wake,
bdev->irq_active_low ? IRQF_TRIGGER_FALLING :
@@ -271,7 +231,7 @@ static int bcm_request_irq(struct bcm_data *bcm)
"host_wake", bdev);
if (err) {
bdev->irq = err;
- goto unlock;
+ return err;
}
device_init_wakeup(bdev->dev, true);
@@ -282,10 +242,7 @@ static int bcm_request_irq(struct bcm_data *bcm)
pm_runtime_set_active(bdev->dev);
pm_runtime_enable(bdev->dev);
-unlock:
- mutex_unlock(&bcm_device_lock);
-
- return err;
+ return 0;
}
static const struct bcm_set_sleep_mode default_sleep_params = {
@@ -306,11 +263,11 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
static int bcm_setup_sleep(struct hci_uart *hu)
{
- struct bcm_data *bcm = hu->priv;
+ struct bcm_device *bdev = serdev_device_get_drvdata(hu->serdev);
struct sk_buff *skb;
struct bcm_set_sleep_mode sleep_params = default_sleep_params;
- sleep_params.host_wake_active = !bcm->dev->irq_active_low;
+ sleep_params.host_wake_active = !bdev->irq_active_low;
skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_params),
&sleep_params, HCI_INIT_TIMEOUT);
@@ -326,7 +283,7 @@ static int bcm_setup_sleep(struct hci_uart *hu)
return 0;
}
#else
-static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
+static inline int bcm_request_irq(struct hci_uart *hu) { return 0; }
static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
#endif
@@ -356,7 +313,6 @@ static int bcm_set_diag(struct hci_dev *hdev, bool enable)
static int bcm_open(struct hci_uart *hu)
{
struct bcm_data *bcm;
- struct list_head *p;
int err;
bt_dev_dbg(hu->hdev, "hu %p", hu);
@@ -369,54 +325,23 @@ static int bcm_open(struct hci_uart *hu)
hu->priv = bcm;
- mutex_lock(&bcm_device_lock);
-
if (hu->serdev) {
+ struct bcm_device *bdev = serdev_device_get_drvdata(hu->serdev);
+
err = serdev_device_open(hu->serdev);
if (err)
goto err_free;
- bcm->dev = serdev_device_get_drvdata(hu->serdev);
- goto out;
- }
-
- if (!hu->tty->dev)
- goto out;
-
- list_for_each(p, &bcm_device_list) {
- struct bcm_device *dev = list_entry(p, struct bcm_device, list);
-
- /* Retrieve saved bcm_device based on parent of the
- * platform device (saved during device probe) and
- * parent of tty device used by hci_uart
- */
- if (hu->tty->dev->parent == dev->dev->parent) {
- bcm->dev = dev;
-#ifdef CONFIG_PM
- dev->hu = hu;
-#endif
- break;
- }
- }
-
-out:
- if (bcm->dev) {
- hu->init_speed = bcm->dev->init_speed;
- hu->oper_speed = bcm->dev->oper_speed;
- err = bcm_gpio_set_power(bcm->dev, true);
+ hu->init_speed = bdev->init_speed;
+ hu->oper_speed = bdev->oper_speed;
+ err = bcm_gpio_set_power(bdev, true);
if (err)
- goto err_unset_hu;
+ goto err_free;
}
- mutex_unlock(&bcm_device_lock);
return 0;
-err_unset_hu:
-#ifdef CONFIG_PM
- bcm->dev->hu = NULL;
-#endif
err_free:
- mutex_unlock(&bcm_device_lock);
hu->priv = NULL;
kfree(bcm);
return err;
@@ -430,20 +355,10 @@ static int bcm_close(struct hci_uart *hu)
bt_dev_dbg(hu->hdev, "hu %p", hu);
- /* Protect bcm->dev against removal of the device or driver */
- mutex_lock(&bcm_device_lock);
-
if (hu->serdev) {
serdev_device_close(hu->serdev);
bdev = serdev_device_get_drvdata(hu->serdev);
- } else if (bcm_device_exists(bcm->dev)) {
- bdev = bcm->dev;
-#ifdef CONFIG_PM
- bdev->hu = NULL;
-#endif
- }
- if (bdev) {
if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
devm_free_irq(bdev->dev, bdev->irq, bdev);
device_init_wakeup(bdev->dev, false);
@@ -456,7 +371,6 @@ static int bcm_close(struct hci_uart *hu)
else
pm_runtime_set_suspended(bdev->dev);
}
- mutex_unlock(&bcm_device_lock);
skb_queue_purge(&bcm->txq);
kfree_skb(bcm->rx_skb);
@@ -479,7 +393,6 @@ static int bcm_flush(struct hci_uart *hu)
static int bcm_setup(struct hci_uart *hu)
{
- struct bcm_data *bcm = hu->priv;
char fw_name[64];
const struct firmware *fw;
unsigned int speed;
@@ -538,7 +451,7 @@ static int bcm_setup(struct hci_uart *hu)
if (err)
return err;
- if (!bcm_request_irq(bcm))
+ if (!bcm_request_irq(hu))
err = bcm_setup_sleep(hu);
return err;
@@ -580,12 +493,9 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
bt_dev_err(hu->hdev, "Frame reassembly failed (%d)", err);
bcm->rx_skb = NULL;
return err;
- } else if (!bcm->rx_skb) {
+ } else if (!bcm->rx_skb && hu->serdev) {
/* Delay auto-suspend when receiving completed packet */
- mutex_lock(&bcm_device_lock);
- if (bcm->dev && bcm_device_exists(bcm->dev))
- pm_request_resume(bcm->dev->dev);
- mutex_unlock(&bcm_device_lock);
+ pm_request_resume(&hu->serdev->dev);
}
return count;
@@ -610,10 +520,8 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
struct sk_buff *skb = NULL;
struct bcm_device *bdev = NULL;
- mutex_lock(&bcm_device_lock);
-
- if (bcm_device_exists(bcm->dev)) {
- bdev = bcm->dev;
+ if (hu->serdev) {
+ bdev = serdev_device_get_drvdata(hu->serdev);
pm_runtime_get_sync(bdev->dev);
/* Shall be resumed here */
}
@@ -625,8 +533,6 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
pm_runtime_put_autosuspend(bdev->dev);
}
- mutex_unlock(&bcm_device_lock);
-
return skb;
}
@@ -638,20 +544,12 @@ static int bcm_suspend_device(struct device *dev)
bt_dev_dbg(bdev, "");
- if (!bdev->is_suspended && bdev->hu) {
- hci_uart_set_flow_control(bdev->hu, true);
-
- /* Once this returns, driver suspends BT via GPIO */
- bdev->is_suspended = true;
- }
+ hci_uart_set_flow_control(&bdev->serdev_hu, true);
/* Suspend the device */
err = bdev->set_device_wakeup(bdev, false);
if (err) {
- if (bdev->is_suspended && bdev->hu) {
- bdev->is_suspended = false;
- hci_uart_set_flow_control(bdev->hu, false);
- }
+ hci_uart_set_flow_control(&bdev->serdev_hu, false);
return -EBUSY;
}
@@ -678,11 +576,7 @@ static int bcm_resume_device(struct device *dev)
msleep(15);
/* When this executes, the device has woken up already */
- if (bdev->is_suspended && bdev->hu) {
- bdev->is_suspended = false;
-
- hci_uart_set_flow_control(bdev->hu, false);
- }
+ hci_uart_set_flow_control(&bdev->serdev_hu, false);
return 0;
}
@@ -695,18 +589,7 @@ static int bcm_suspend(struct device *dev)
struct bcm_device *bdev = dev_get_drvdata(dev);
int error;
- bt_dev_dbg(bdev, "suspend: is_suspended %d", bdev->is_suspended);
-
- /*
- * When used with a device instantiated as platform_device, bcm_suspend
- * can be called at any time as long as the platform device is bound,
- * so it should use bcm_device_lock to protect access to hci_uart
- * and device_wake-up GPIO.
- */
- mutex_lock(&bcm_device_lock);
-
- if (!bdev->hu)
- goto unlock;
+ bt_dev_dbg(bdev, "");
if (pm_runtime_active(dev))
bcm_suspend_device(dev);
@@ -717,9 +600,6 @@ static int bcm_suspend(struct device *dev)
bt_dev_dbg(bdev, "BCM irq: enabled");
}
-unlock:
- mutex_unlock(&bcm_device_lock);
-
return 0;
}
@@ -729,18 +609,7 @@ static int bcm_resume(struct device *dev)
struct bcm_device *bdev = dev_get_drvdata(dev);
int err = 0;
- bt_dev_dbg(bdev, "resume: is_suspended %d", bdev->is_suspended);
-
- /*
- * When used with a device instantiated as platform_device, bcm_resume
- * can be called at any time as long as platform device is bound,
- * so it should use bcm_device_lock to protect access to hci_uart
- * and device_wake-up GPIO.
- */
- mutex_lock(&bcm_device_lock);
-
- if (!bdev->hu)
- goto unlock;
+ bt_dev_dbg(bdev, "");
if (device_may_wakeup(dev) && bdev->irq > 0) {
disable_irq_wake(bdev->irq);
@@ -748,10 +617,6 @@ static int bcm_resume(struct device *dev)
}
err = bcm_resume_device(dev);
-
-unlock:
- mutex_unlock(&bcm_device_lock);
-
if (!err) {
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
@@ -1002,57 +867,6 @@ static int bcm_of_probe(struct bcm_device *bdev)
return 0;
}
-static int bcm_probe(struct platform_device *pdev)
-{
- struct bcm_device *dev;
- int ret;
-
- dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
- if (!dev)
- return -ENOMEM;
-
- dev->dev = &pdev->dev;
- dev->irq = platform_get_irq(pdev, 0);
-
- if (has_acpi_companion(&pdev->dev)) {
- ret = bcm_acpi_probe(dev);
- if (ret)
- return ret;
- }
-
- ret = bcm_get_resources(dev);
- if (ret)
- return ret;
-
- platform_set_drvdata(pdev, dev);
-
- dev_info(&pdev->dev, "%s device registered.\n", dev->name);
-
- /* Place this instance on the device list */
- mutex_lock(&bcm_device_lock);
- list_add_tail(&dev->list, &bcm_device_list);
- mutex_unlock(&bcm_device_lock);
-
- ret = bcm_gpio_set_power(dev, false);
- if (ret)
- dev_err(&pdev->dev, "Failed to power down\n");
-
- return 0;
-}
-
-static int bcm_remove(struct platform_device *pdev)
-{
- struct bcm_device *dev = platform_get_drvdata(pdev);
-
- mutex_lock(&bcm_device_lock);
- list_del(&dev->list);
- mutex_unlock(&bcm_device_lock);
-
- dev_info(&pdev->dev, "%s device unregistered.\n", dev->name);
-
- return 0;
-}
-
static const struct hci_uart_proto bcm_proto = {
.id = HCI_UART_BCM,
.name = "Broadcom",
@@ -1100,16 +914,6 @@ static const struct dev_pm_ops bcm_pm_ops = {
SET_RUNTIME_PM_OPS(bcm_suspend_device, bcm_resume_device, NULL)
};
-static struct platform_driver bcm_driver = {
- .probe = bcm_probe,
- .remove = bcm_remove,
- .driver = {
- .name = "hci_bcm",
- .acpi_match_table = ACPI_PTR(bcm_acpi_match),
- .pm = &bcm_pm_ops,
- },
-};
-
static int bcm_serdev_probe(struct serdev_device *serdev)
{
struct bcm_device *bcmdev;
@@ -1120,9 +924,6 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
return -ENOMEM;
bcmdev->dev = &serdev->dev;
-#ifdef CONFIG_PM
- bcmdev->hu = &bcmdev->serdev_hu;
-#endif
bcmdev->serdev_hu.serdev = serdev;
serdev_device_set_drvdata(serdev, bcmdev);
@@ -1172,10 +973,6 @@ static struct serdev_device_driver bcm_serdev_driver = {
int __init bcm_init(void)
{
- /* For now, we need to keep both platform device
- * driver (ACPI generated) and serdev driver (DT).
- */
- platform_driver_register(&bcm_driver);
serdev_device_driver_register(&bcm_serdev_driver);
return hci_uart_register_proto(&bcm_proto);
@@ -1183,7 +980,6 @@ int __init bcm_init(void)
int __exit bcm_deinit(void)
{
- platform_driver_unregister(&bcm_driver);
serdev_device_driver_unregister(&bcm_serdev_driver);
return hci_uart_unregister_proto(&bcm_proto);
--
2.14.3
Op maandag 22 januari 2018 13:15:21 CET schreef Andy Shevchenko:
> On Mon, 2018-01-22 at 13:01 +0100, Marcel Holtmann wrote:
> > > > > Anyways it looks like this will be really hard to do without
> > > > > access
> > > > > to the hardware.
> > > >
> > > > I can do a BAT.
> > >
> > > Right, sorry I was not clear, when I was talking about hardware
> > > access I was not (not only) referring to testing.
> > >
> > > The problem is that to figure out how to hook all this together
> > > will require poking around on the hardware, looking in sysfs,
> > > finding
> > > some id we can use in a pci_serdev_register_devices() to add to
> > > drivers/tty/serdev/core.c so that it only turns the serial port on
> > > the Edison into a serdev and not somewhere else, etc.
> > >
> > > But thinking about this more, I too have higher priority items on
> > > my TODO list, so as much as I would like to see this cleaned up,
> > > lets shelf this for now until you have time to look into this.
> > >
> > > When you do find time, if you've any questions I'm happy to help.
> >
> > so in theory the Edison board has SFI. Problem is just that I am not
> > sure the SFI on the board is fully correct and contains enough
> > information to identify the serial port correctly.
>
> Unfortunately SFI was so badly designed that board (hard coded) data is
> needed.
>
> > If no one is willing to help port this over to serdev, then one option
> > is just to rip it out and see who complains.
>
> Me in the first place. + I guess at least couple of users as of my
> knowledge. + Undefined amount of swindled users of Edison who is trying
> or already did a switch to vanilla kernel.
>
> > Or include a version that is trying its best and see who reports
> >
> > issues and who helps to fix it. I am now regretting that I accepted
> > the Edison support in the first place, but sadly serdev was not yet
> > fully baked at that point.
>
> +Cc: Ferry, who might be interested in the helping with it.
I would be glad to help. Keep in mind I'm not a kernel developer and need
someone to hold my hand. All this in my free time of course, but can
spend a few evenings per week.
Nevertheless I have an Edison dual booting 4.15.0-rc8 with poky pyro user
space. So I can easily apply patches and test.
--
Ferry Toth
Hi,
On 22-01-18 20:57, Marcel Holtmann wrote:
> Hi Hans,
>
>>>>>> Andy, I see that you added support for bcm bluetooth over a tty
>>>>>> using
>>>>>> platform_data instead of ACPI enumeration. Can you change the code
>>>>>> instantiating the device to instead instantiate a serdev, so that
>>>>>> we
>>>>>> kill the platform device support in hci_bcm.c and so that users
>>>>>> don't
>>>>>> need to do a btattach, but instead the kernel will do the attach
>>>>>> itself
>>>>>> and things will just work ?
>>>>>
>>>>> I'm sorry, I can't do this soon, other more priority tasks in a
>>>>> pocket.
>>>>>
>>>>> The instantiation of the driver is happened in
>>>>> arch/x86/platform/intel-
>>>>> mid/device_libs/platform_bt.c
>>>>>
>>>>> I would help with review of any patches till I would able to look at
>>>>> it
>>>>> myself.
>>>>
>>>> If I manage to come up with patches do you have hardware and time to
>>>> test?
>>> Yes and I would find half an hour for sure.
>>
>> That is great, thank you, but ...
>>
>>>> First point of order to get this working as serdev I think is to
>>>> modify drivers/tty/serdev/core.c a and then the
>>>> serdev_controller_add()
>>>> function to somehow recognize the serial port in question, so
>>>> something akin to the of_serdev_register_devices(ctrl) /
>>>> acpi_serdev_register_devices(ctrl) functions for platform_devs,
>>>> assuming
>>>> the tty-parent-dev on the Edison SOM is a platform_dev ?
>>> tty parent is PCI device there.
>>>> Anyways it looks like this will be really hard to do without access
>>>> to the hardware.
>>> I can do a BAT.
>>
>> Right, sorry I was not clear, when I was talking about hardware
>> access I was not (not only) referring to testing.
>>
>> The problem is that to figure out how to hook all this together
>> will require poking around on the hardware, looking in sysfs, finding
>> some id we can use in a pci_serdev_register_devices() to add to
>> drivers/tty/serdev/core.c so that it only turns the serial port on
>> the Edison into a serdev and not somewhere else, etc.
>>
>> But thinking about this more, I too have higher priority items on
>> my TODO list, so as much as I would like to see this cleaned up,
>> lets shelf this for now until you have time to look into this.
>>
>> When you do find time, if you've any questions I'm happy to help.
>
> so the serial port is driven by drivers/tty/serial/8250/8250_mid.c and using PCI_DEVICE_ID_INTEL_TNG_UART 0x1191 to match the driver. So it is exposed as PCI device. While of course more than one UART port uses 0x1191. This one seems to be mapped to pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(4, 1)) actually.
>
> What I think is that we need to do is some sort of x86_match_cpu like arch/x86/platform/intel-mid/device_libs/platform_bt.c does. But then again, that is also what the Apple Broadcom support does to choose the different ACPI procedures.
>
> I have an Edison board on my desk and can quickly boot any patch you want me to test. Or grab data from sysfs or dmesg.
I'm sorry, I started looking at removing the platform code because
I already spend a lot of time looking at the hci_bcm code to make
the power-management work with serdev devices instantiated from
ACPI, when writing the patch-set for that I added removing the
remaining cruft to my todo list...
But I don't have time / don't want to make time given other priorities
to also look into the Edison stuff, which seems non-trivial to fix.
Regards,
Hans
Hi Hans,
>>>>> Andy, I see that you added support for bcm bluetooth over a tty
>>>>> using
>>>>> platform_data instead of ACPI enumeration. Can you change the code
>>>>> instantiating the device to instead instantiate a serdev, so that
>>>>> we
>>>>> kill the platform device support in hci_bcm.c and so that users
>>>>> don't
>>>>> need to do a btattach, but instead the kernel will do the attach
>>>>> itself
>>>>> and things will just work ?
>>>>
>>>> I'm sorry, I can't do this soon, other more priority tasks in a
>>>> pocket.
>>>>
>>>> The instantiation of the driver is happened in
>>>> arch/x86/platform/intel-
>>>> mid/device_libs/platform_bt.c
>>>>
>>>> I would help with review of any patches till I would able to look at
>>>> it
>>>> myself.
>>>
>>> If I manage to come up with patches do you have hardware and time to
>>> test?
>> Yes and I would find half an hour for sure.
>
> That is great, thank you, but ...
>
>>> First point of order to get this working as serdev I think is to
>>> modify drivers/tty/serdev/core.c a and then the
>>> serdev_controller_add()
>>> function to somehow recognize the serial port in question, so
>>> something akin to the of_serdev_register_devices(ctrl) /
>>> acpi_serdev_register_devices(ctrl) functions for platform_devs,
>>> assuming
>>> the tty-parent-dev on the Edison SOM is a platform_dev ?
>> tty parent is PCI device there.
>>> Anyways it looks like this will be really hard to do without access
>>> to the hardware.
>> I can do a BAT.
>
> Right, sorry I was not clear, when I was talking about hardware
> access I was not (not only) referring to testing.
>
> The problem is that to figure out how to hook all this together
> will require poking around on the hardware, looking in sysfs, finding
> some id we can use in a pci_serdev_register_devices() to add to
> drivers/tty/serdev/core.c so that it only turns the serial port on
> the Edison into a serdev and not somewhere else, etc.
>
> But thinking about this more, I too have higher priority items on
> my TODO list, so as much as I would like to see this cleaned up,
> lets shelf this for now until you have time to look into this.
>
> When you do find time, if you've any questions I'm happy to help.
so the serial port is driven by drivers/tty/serial/8250/8250_mid.c and using PCI_DEVICE_ID_INTEL_TNG_UART 0x1191 to match the driver. So it is exposed as PCI device. While of course more than one UART port uses 0x1191. This one seems to be mapped to pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(4, 1)) actually.
What I think is that we need to do is some sort of x86_match_cpu like arch/x86/platform/intel-mid/device_libs/platform_bt.c does. But then again, that is also what the Apple Broadcom support does to choose the different ACPI procedures.
I have an Edison board on my desk and can quickly boot any patch you want me to test. Or grab data from sysfs or dmesg.
Regards
Marcel
On Mon, 2018-01-22 at 13:01 +0100, Marcel Holtmann wrote:
> > > > Anyways it looks like this will be really hard to do without
> > > > access
> > > > to the hardware.
> > >
> > > I can do a BAT.
> >
> > Right, sorry I was not clear, when I was talking about hardware
> > access I was not (not only) referring to testing.
> >
> > The problem is that to figure out how to hook all this together
> > will require poking around on the hardware, looking in sysfs,
> > finding
> > some id we can use in a pci_serdev_register_devices() to add to
> > drivers/tty/serdev/core.c so that it only turns the serial port on
> > the Edison into a serdev and not somewhere else, etc.
> >
> > But thinking about this more, I too have higher priority items on
> > my TODO list, so as much as I would like to see this cleaned up,
> > lets shelf this for now until you have time to look into this.
> >
> > When you do find time, if you've any questions I'm happy to help.
>
> so in theory the Edison board has SFI. Problem is just that I am not
> sure the SFI on the board is fully correct and contains enough
> information to identify the serial port correctly.
Unfortunately SFI was so badly designed that board (hard coded) data is
needed.
> If no one is willing to help port this over to serdev, then one option
> is just to rip it out and see who complains.
Me in the first place. + I guess at least couple of users as of my
knowledge. + Undefined amount of swindled users of Edison who is trying
or already did a switch to vanilla kernel.
> Or include a version that is trying its best and see who reports
> issues and who helps to fix it. I am now regretting that I accepted
> the Edison support in the first place, but sadly serdev was not yet
> fully baked at that point.
+Cc: Ferry, who might be interested in the helping with it.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
Hi Hans,
>>>>> Andy, I see that you added support for bcm bluetooth over a tty
>>>>> using
>>>>> platform_data instead of ACPI enumeration. Can you change the code
>>>>> instantiating the device to instead instantiate a serdev, so that
>>>>> we
>>>>> kill the platform device support in hci_bcm.c and so that users
>>>>> don't
>>>>> need to do a btattach, but instead the kernel will do the attach
>>>>> itself
>>>>> and things will just work ?
>>>>
>>>> I'm sorry, I can't do this soon, other more priority tasks in a
>>>> pocket.
>>>>
>>>> The instantiation of the driver is happened in
>>>> arch/x86/platform/intel-
>>>> mid/device_libs/platform_bt.c
>>>>
>>>> I would help with review of any patches till I would able to look at
>>>> it
>>>> myself.
>>>
>>> If I manage to come up with patches do you have hardware and time to
>>> test?
>> Yes and I would find half an hour for sure.
>
> That is great, thank you, but ...
>
>>> First point of order to get this working as serdev I think is to
>>> modify drivers/tty/serdev/core.c a and then the
>>> serdev_controller_add()
>>> function to somehow recognize the serial port in question, so
>>> something akin to the of_serdev_register_devices(ctrl) /
>>> acpi_serdev_register_devices(ctrl) functions for platform_devs,
>>> assuming
>>> the tty-parent-dev on the Edison SOM is a platform_dev ?
>> tty parent is PCI device there.
>>> Anyways it looks like this will be really hard to do without access
>>> to the hardware.
>> I can do a BAT.
>
> Right, sorry I was not clear, when I was talking about hardware
> access I was not (not only) referring to testing.
>
> The problem is that to figure out how to hook all this together
> will require poking around on the hardware, looking in sysfs, finding
> some id we can use in a pci_serdev_register_devices() to add to
> drivers/tty/serdev/core.c so that it only turns the serial port on
> the Edison into a serdev and not somewhere else, etc.
>
> But thinking about this more, I too have higher priority items on
> my TODO list, so as much as I would like to see this cleaned up,
> lets shelf this for now until you have time to look into this.
>
> When you do find time, if you've any questions I'm happy to help.
so in theory the Edison board has SFI. Problem is just that I am not sure the SFI on the board is fully correct and contains enough information to identify the serial port correctly.
If no one is willing to help port this over to serdev, then one option is just to rip it out and see who complains. Or include a version that is trying its best and see who reports issues and who helps to fix it. I am now regretting that I accepted the Edison support in the first place, but sadly serdev was not yet fully baked at that point.
Regards
Marcel
Hi,
On 22-01-18 12:33, Andy Shevchenko wrote:
> On Mon, 2018-01-22 at 12:27 +0100, Hans de Goede wrote:
>> On 22-01-18 10:42, Andy Shevchenko wrote:
>>> On Mon, 2018-01-22 at 09:23 +0100, Hans de Goede wrote:
>>>> On 22-01-18 03:24, Marcel Holtmann wrote:
>
>>>> Andy, I see that you added support for bcm bluetooth over a tty
>>>> using
>>>> platform_data instead of ACPI enumeration. Can you change the code
>>>> instantiating the device to instead instantiate a serdev, so that
>>>> we
>>>> kill the platform device support in hci_bcm.c and so that users
>>>> don't
>>>> need to do a btattach, but instead the kernel will do the attach
>>>> itself
>>>> and things will just work ?
>>>
>>> I'm sorry, I can't do this soon, other more priority tasks in a
>>> pocket.
>>>
>>> The instantiation of the driver is happened in
>>> arch/x86/platform/intel-
>>> mid/device_libs/platform_bt.c
>>>
>>> I would help with review of any patches till I would able to look at
>>> it
>>> myself.
>>
>> If I manage to come up with patches do you have hardware and time to
>> test?
>
> Yes and I would find half an hour for sure.
That is great, thank you, but ...
>> First point of order to get this working as serdev I think is to
>> modify drivers/tty/serdev/core.c a and then the
>> serdev_controller_add()
>> function to somehow recognize the serial port in question, so
>> something akin to the of_serdev_register_devices(ctrl) /
>> acpi_serdev_register_devices(ctrl) functions for platform_devs,
>> assuming
>> the tty-parent-dev on the Edison SOM is a platform_dev ?
>
> tty parent is PCI device there.
>
>> Anyways it looks like this will be really hard to do without access
>> to the hardware.
>
> I can do a BAT.
Right, sorry I was not clear, when I was talking about hardware
access I was not (not only) referring to testing.
The problem is that to figure out how to hook all this together
will require poking around on the hardware, looking in sysfs, finding
some id we can use in a pci_serdev_register_devices() to add to
drivers/tty/serdev/core.c so that it only turns the serial port on
the Edison into a serdev and not somewhere else, etc.
But thinking about this more, I too have higher priority items on
my TODO list, so as much as I would like to see this cleaned up,
lets shelf this for now until you have time to look into this.
When you do find time, if you've any questions I'm happy to help.
Regards,
Hans
p.s.
I will send out a v2 of the patch-set with the patches swapped
as Marcel requested, but this patch (2/2 in the new set) is really
just for future reference until this Edison issue is resolved.
On Mon, 2018-01-22 at 12:27 +0100, Hans de Goede wrote:
> On 22-01-18 10:42, Andy Shevchenko wrote:
> > On Mon, 2018-01-22 at 09:23 +0100, Hans de Goede wrote:
> > > On 22-01-18 03:24, Marcel Holtmann wrote:
> > > Andy, I see that you added support for bcm bluetooth over a tty
> > > using
> > > platform_data instead of ACPI enumeration. Can you change the code
> > > instantiating the device to instead instantiate a serdev, so that
> > > we
> > > kill the platform device support in hci_bcm.c and so that users
> > > don't
> > > need to do a btattach, but instead the kernel will do the attach
> > > itself
> > > and things will just work ?
> >
> > I'm sorry, I can't do this soon, other more priority tasks in a
> > pocket.
> >
> > The instantiation of the driver is happened in
> > arch/x86/platform/intel-
> > mid/device_libs/platform_bt.c
> >
> > I would help with review of any patches till I would able to look at
> > it
> > myself.
>
> If I manage to come up with patches do you have hardware and time to
> test?
Yes and I would find half an hour for sure.
> First point of order to get this working as serdev I think is to
> modify drivers/tty/serdev/core.c a and then the
> serdev_controller_add()
> function to somehow recognize the serial port in question, so
> something akin to the of_serdev_register_devices(ctrl) /
> acpi_serdev_register_devices(ctrl) functions for platform_devs,
> assuming
> the tty-parent-dev on the Edison SOM is a platform_dev ?
tty parent is PCI device there.
> Anyways it looks like this will be really hard to do without access
> to the hardware.
I can do a BAT.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
Hi,
On 22-01-18 10:42, Andy Shevchenko wrote:
> On Mon, 2018-01-22 at 09:23 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 22-01-18 03:24, Marcel Holtmann wrote:
>>> Hi Hans,
>>>
>>>> Now that ACPI and DT devices are both enumerated as serdevs, we
>>>> can
>>>> remove platform_device support and the bcm_device_list lookup
>>>> hack.
>>>>
>>>> This also removes any races between suspend/resume and hci-uart
>>>> binding,
>>>> also making the suspend/resume code a lot simpler.
>>>>
>>>> This commit leaves manually binding to an uart using btattach
>>>> supported
>>>> (without irq/gpio and thus suspend/resume support, as before).
>>>>
>>>> Cc: Lukas Wunner <[email protected]>
>>>> Signed-off-by: Hans de Goede <[email protected]>
>>>> ---
>>>> drivers/bluetooth/hci_bcm.c | 260 +++++---------------------------
>>>> ------------
>>>> 1 file changed, 28 insertions(+), 232 deletions(-)
>>>
>>> so I was under the assumption platforms like Intel Edison still only
>>> do platform data. See commit
>>> 212d71833315c65644efc46223db61dee7b3c68e. Has that changed?
>
> Yes and no.
>
> So, we need that support to satisfy users with classical Edison
> firmware.
>
>> Ugh, I was not aware of that and the whole code to match the tty with
>> the platform_device on btattach is such a mess and I was actually
>> quite
>> happy to be able to delete this.
>
> Good idea.
>
>> Andy, I see that you added support for bcm bluetooth over a tty using
>> platform_data instead of ACPI enumeration. Can you change the code
>> instantiating the device to instead instantiate a serdev, so that we
>> kill the platform device support in hci_bcm.c and so that users don't
>> need to do a btattach, but instead the kernel will do the attach
>> itself
>> and things will just work ?
>
> I'm sorry, I can't do this soon, other more priority tasks in a pocket.
>
> The instantiation of the driver is happened in arch/x86/platform/intel-
> mid/device_libs/platform_bt.c
>
> I would help with review of any patches till I would able to look at it
> myself.
If I manage to come up with patches do you have hardware and time to
test?
First point of order to get this working as serdev I think is to
modify drivers/tty/serdev/core.c a and then the serdev_controller_add()
function to somehow recognize the serial port in question, so
something akin to the of_serdev_register_devices(ctrl) /
acpi_serdev_register_devices(ctrl) functions for platform_devs, assuming
the tty-parent-dev on the Edison SOM is a platform_dev ?
Anyways it looks like this will be really hard to do without access
to the hardware.
Regards,
Hans
On Mon, 2018-01-22 at 09:23 +0100, Hans de Goede wrote:
> Hi,
>
> On 22-01-18 03:24, Marcel Holtmann wrote:
> > Hi Hans,
> >
> > > Now that ACPI and DT devices are both enumerated as serdevs, we
> > > can
> > > remove platform_device support and the bcm_device_list lookup
> > > hack.
> > >
> > > This also removes any races between suspend/resume and hci-uart
> > > binding,
> > > also making the suspend/resume code a lot simpler.
> > >
> > > This commit leaves manually binding to an uart using btattach
> > > supported
> > > (without irq/gpio and thus suspend/resume support, as before).
> > >
> > > Cc: Lukas Wunner <[email protected]>
> > > Signed-off-by: Hans de Goede <[email protected]>
> > > ---
> > > drivers/bluetooth/hci_bcm.c | 260 +++++---------------------------
> > > ------------
> > > 1 file changed, 28 insertions(+), 232 deletions(-)
> >
> > so I was under the assumption platforms like Intel Edison still only
> > do platform data. See commit
> > 212d71833315c65644efc46223db61dee7b3c68e. Has that changed?
Yes and no.
So, we need that support to satisfy users with classical Edison
firmware.
> Ugh, I was not aware of that and the whole code to match the tty with
> the platform_device on btattach is such a mess and I was actually
> quite
> happy to be able to delete this.
Good idea.
> Andy, I see that you added support for bcm bluetooth over a tty using
> platform_data instead of ACPI enumeration. Can you change the code
> instantiating the device to instead instantiate a serdev, so that we
> kill the platform device support in hci_bcm.c and so that users don't
> need to do a btattach, but instead the kernel will do the attach
> itself
> and things will just work ?
I'm sorry, I can't do this soon, other more priority tasks in a pocket.
The instantiation of the driver is happened in arch/x86/platform/intel-
mid/device_libs/platform_bt.c
I would help with review of any patches till I would able to look at it
myself.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
Hi,
On 22-01-18 03:24, Marcel Holtmann wrote:
> Hi Hans,
>
>> Now that ACPI and DT devices are both enumerated as serdevs, we can
>> remove platform_device support and the bcm_device_list lookup hack.
>>
>> This also removes any races between suspend/resume and hci-uart binding,
>> also making the suspend/resume code a lot simpler.
>>
>> This commit leaves manually binding to an uart using btattach supported
>> (without irq/gpio and thus suspend/resume support, as before).
>>
>> Cc: Lukas Wunner <[email protected]>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> drivers/bluetooth/hci_bcm.c | 260 +++++---------------------------------------
>> 1 file changed, 28 insertions(+), 232 deletions(-)
>
> so I was under the assumption platforms like Intel Edison still only do platform data. See commit 212d71833315c65644efc46223db61dee7b3c68e. Has that changed?
Ugh, I was not aware of that and the whole code to match the tty with
the platform_device on btattach is such a mess and I was actually quite
happy to be able to delete this.
Andy, I see that you added support for bcm bluetooth over a tty using
platform_data instead of ACPI enumeration. Can you change the code
instantiating the device to instead instantiate a serdev, so that we
kill the platform device support in hci_bcm.c and so that users don't
need to do a btattach, but instead the kernel will do the attach itself
and things will just work ?
Regards,
Hans
Hi Hans,
> Now that ACPI and DT devices are both enumerated as serdevs, we can
> remove platform_device support and the bcm_device_list lookup hack.
>
> This also removes any races between suspend/resume and hci-uart binding,
> also making the suspend/resume code a lot simpler.
>
> This commit leaves manually binding to an uart using btattach supported
> (without irq/gpio and thus suspend/resume support, as before).
>
> Cc: Lukas Wunner <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> drivers/bluetooth/hci_bcm.c | 260 +++++---------------------------------------
> 1 file changed, 28 insertions(+), 232 deletions(-)
so I was under the assumption platforms like Intel Edison still only do platform data. See commit 212d71833315c65644efc46223db61dee7b3c68e. Has that changed?
Regards
Marcel
Hi Hans,
> Commit 8bfa7e1e03ac ("Bluetooth: hci_bcm: Handle errors properly")
> introduced error checking for the bcm_gpio_set_power() call in bcm_open()
> but the error-path it introduces does not properly call
> serdev_device_close() to undo the earlier serdev_device_open(), this
> commit fixes this.
>
> Cc: Lukas Wunner <[email protected]>
> Fixes: 8bfa7e1e03ac ("Bluetooth: hci_bcm: Handle errors properly")
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> drivers/bluetooth/hci_bcm.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
can I get this as 1/2 so I can apply it right away.
Regards
Marcel
Commit 8bfa7e1e03ac ("Bluetooth: hci_bcm: Handle errors properly")
introduced error checking for the bcm_gpio_set_power() call in bcm_open()
but the error-path it introduces does not properly call
serdev_device_close() to undo the earlier serdev_device_open(), this
commit fixes this.
Cc: Lukas Wunner <[email protected]>
Fixes: 8bfa7e1e03ac ("Bluetooth: hci_bcm: Handle errors properly")
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 61f73cc4c05e..da4736f2e913 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -336,11 +336,13 @@ static int bcm_open(struct hci_uart *hu)
hu->oper_speed = bdev->oper_speed;
err = bcm_gpio_set_power(bdev, true);
if (err)
- goto err_free;
+ goto err_close_serdev;
}
return 0;
+err_close_serdev:
+ serdev_device_close(hu->serdev);
err_free:
hu->priv = NULL;
kfree(bcm);
--
2.14.3