[PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[maxin.john]

From: Andy Duan <fugang.duan@nxp.com>

>From the log in .bcm43xx_load_firmware():
        /* Wait 50ms to let the firmware placed in download mode */
        nanosleep(&tm_mode, NULL);

But timespec tm_mode is real is 50us. Correct the delayed timer count.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Maxin B. John <maxin.john@intel.com>
---
 tools/hciattach_bcm43xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/hciattach_bcm43xx.c b/tools/hciattach_bcm43xx.c
index 81f38cb..ac1b3c1 100644
--- a/tools/hciattach_bcm43xx.c
+++ b/tools/hciattach_bcm43xx.c
@@ -228,8 +228,8 @@ static int bcm43xx_set_speed(int fd, struct termios *ti, uint32_t speed)
 static int bcm43xx_load_firmware(int fd, const char *fw)
 {
 	unsigned char cmd[] = { HCI_COMMAND_PKT, 0x2e, 0xfc, 0x00 };
-	struct timespec tm_mode = { 0, 50000 };
-	struct timespec tm_ready = { 0, 2000000 };
+	struct timespec tm_mode = { 0, 50000000 };
+	struct timespec tm_ready = { 0, 200000000 };
 	unsigned char resp[CC_MIN_SIZE];
 	unsigned char tx_buf[1024];
 	int len, fd_fw, n;
-- 
2.4.0

Re: [PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[Marcel Holtmann]

Hi Maxin,

> From the log in .bcm43xx_load_firmware():
>        /* Wait 50ms to let the firmware placed in download mode */
>        nanosleep(&tm_mode, NULL);
> 
> But timespec tm_mode is real is 50us. Correct the delayed timer count.
> 
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> Signed-off-by: Maxin B. John <maxin.john@intel.com>

we are not using signed offs for user space code. So I fixed that up and applied the patch.

However I would prefer if we stop using hciattach and focus on hci_bcm.c support with btattach and serdev.

Regards

Marcel

Re: [PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[Ian Molton]

On 20/07/17 19:23, Marcel Holtmann wrote:
> 
> However I would prefer if we stop using hciattach and focus on hci_bcm.c support with btattach and serdev.

I can resubmit my serdev driver if you like? It needs a little cleanup
but its been solid here over the last week or two. Mostly its missing
some runtime PM support (I cant test that as my device is crippled and
has no host-wakeup or device-suspend lines).

I cant see a nice way to integrate it with the existing driver due to
the fact that it differs markedly in the routines that need pointers to
the serio structures.

The non-serio version uses a linked list that it traverses during
startup to reconcile the port with the tty. Its a much uglier way to do
it, but kinda by necessity.

Is there any particular issue with having both drivers in-tree?

A lot of legacy hardware will *never* use the serio method as it doesn't
have the information available from firmware as to which serio device it
should attach to. I guess some of the ACPI versions might be adaptable,
but that would require some kind of ACPI->serio mapping that I'm not
sure exists at present? ICBW though?

-Ian

Re: [PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[Marcel Holtmann]

Hi Ian,

>> However I would prefer if we stop using hciattach and focus on hci_bcm.c support with btattach and serdev.
> 
> I can resubmit my serdev driver if you like? It needs a little cleanup
> but its been solid here over the last week or two. Mostly its missing
> some runtime PM support (I cant test that as my device is crippled and
> has no host-wakeup or device-suspend lines).

I am not going to assign a new N_HCI UART type to the same hardware. It all needs to go through the same type. So I still think for now it should be in the same hci_bcm.c file.

> I cant see a nice way to integrate it with the existing driver due to
> the fact that it differs markedly in the routines that need pointers to
> the serio structures.
> 
> The non-serio version uses a linked list that it traverses during
> startup to reconcile the port with the tty. Its a much uglier way to do
> it, but kinda by necessity.
> 
> Is there any particular issue with having both drivers in-tree?
> 
> A lot of legacy hardware will *never* use the serio method as it doesn't
> have the information available from firmware as to which serio device it
> should attach to. I guess some of the ACPI versions might be adaptable,
> but that would require some kind of ACPI->serio mapping that I'm not
> sure exists at present? ICBW though?

We want to actually turn all legacy N_HCI drivers into serdev drivers. So that the only think you write are serdev drivers. Rob is trying to work out how that can be done if you do not have DT as backing information. And more importantly do that through N_HCI where the TTY is initially exposed to user space.

So that is really the way to go here. Convert everything into serdev drivers. Have a look at his current work.

https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=serdev-ldisc

Regards

Marcel

Re: [PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[Ian Molton]

On 21/07/17 07:40, Marcel Holtmann wrote:
> Hi Ian,
> 
>>> However I would prefer if we stop using hciattach and focus on
>>> hci_bcm.c support with btattach and serdev.
>> 
>> I can resubmit my serdev driver if you like? It needs a little
>> cleanup but its been solid here over the last week or two. Mostly
>> its missing some runtime PM support (I cant test that as my device
>> is crippled and has no host-wakeup or device-suspend lines).
> 
> I am not going to assign a new N_HCI UART type to the same hardware.
> It all needs to go through the same type. So I still think for now it
> should be in the same hci_bcm.c file.

If I have it share the UART type, that should be no problem. Its highly
unlikely for anyone to have both drivers coexist on a OF platform.

I could set it up so that you can select one *or* the other in Kconfig.

>> I cant see a nice way to integrate it with the existing driver due
>> to the fact that it differs markedly in the routines that need
>> pointers to the serio structures.
> 
> We want to actually turn all legacy N_HCI drivers into serdev
> drivers. So that the only think you write are serdev drivers.

I cannot see a nice way to do this.

> So that is really the way to go here. Convert everything into serdev
> drivers. Have a look at his current work.

It really isnt.

serdev drivers are for platforms that have the port -> subdevice
mappings available at probe time.

> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=serdev-ldisc

I hate to sound peevish, but that really does not look like a good idea.

Part of the point of serdev drivers is to keep the ttydev out of the
hands of userspace. This code explicitly overrides that.

This already looks a lot more complex than my serdev driver.

A far better approach would be to teach ACPI platforms how to chreate
their own serdev devices.

Platforms with zero hardware info available should NOT be using serdev.

-Ian

Re: [PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[Marcel Holtmann]

Hi Ian,

>>>> However I would prefer if we stop using hciattach and focus on
>>>> hci_bcm.c support with btattach and serdev.
>>> 
>>> I can resubmit my serdev driver if you like? It needs a little
>>> cleanup but its been solid here over the last week or two. Mostly
>>> its missing some runtime PM support (I cant test that as my device
>>> is crippled and has no host-wakeup or device-suspend lines).
>> 
>> I am not going to assign a new N_HCI UART type to the same hardware.
>> It all needs to go through the same type. So I still think for now it
>> should be in the same hci_bcm.c file.
> 
> If I have it share the UART type, that should be no problem. Its highly
> unlikely for anyone to have both drivers coexist on a OF platform.
> 
> I could set it up so that you can select one *or* the other in Kconfig.
> 
>>> I cant see a nice way to integrate it with the existing driver due
>>> to the fact that it differs markedly in the routines that need
>>> pointers to the serio structures.
>> 
>> We want to actually turn all legacy N_HCI drivers into serdev
>> drivers. So that the only think you write are serdev drivers.
> 
> I cannot see a nice way to do this.
> 
>> So that is really the way to go here. Convert everything into serdev
>> drivers. Have a look at his current work.
> 
> It really isnt.
> 
> serdev drivers are for platforms that have the port -> subdevice
> mappings available at probe time.
> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=serdev-ldisc
> 
> I hate to sound peevish, but that really does not look like a good idea.
> 
> Part of the point of serdev drivers is to keep the ttydev out of the
> hands of userspace. This code explicitly overrides that.
> 
> This already looks a lot more complex than my serdev driver.
> 
> A far better approach would be to teach ACPI platforms how to chreate
> their own serdev devices.

I agree that ACPI and even pure platform devices should create serdev nodes.

> Platforms with zero hardware info available should NOT be using serdev.

Actually there will be development hardware that comes via USB-TTY converters and we need to make this work somehow. I am not going to write two drivers for each hardware in the future. And we are using development boards often for hardware testing and bringup.

Regards

Marcel

Re: [PATCH] hciattach: bcm43xx: fix the delay timer for firmware download

[Ian Molton]

On 21/07/17 12:27, Marcel Holtmann wrote:

>> A far better approach would be to teach ACPI platforms how to chreate
>> their own serdev devices.
> 
> I agree that ACPI and even pure platform devices should create serdev nodes.

Then my driver will work as is (if the irq/sleep dt entries are
implemented. I can do a respin with these added, and it can use the same
DT labels as the Nokia driver does. No need for modifying the old driver
at all.

>> Platforms with zero hardware info available should NOT be using serdev.
> 
> Actually there will be development hardware that comes via USB-TTY converters
> and we need to make this work somehow. I am not going to write two drivers for
> each hardware in the future. And we are using development boards often for
> hardware testing and bringup.

That should work.

I havent looked, but it seems to me that all we need to do is allow
serial port drivers to register a DT structure for the BT subdevice
attached to them when they probe, if such a function doesnt already exist.

I dont see what other glue we would need - perhaps the USB ones might
need a callback to allow them to send USB packets to control the wakeup
lines etc.

-Ian