All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marcel Holtmann <marcel@holtmann.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: "Pali Rohár" <pali.rohar@gmail.com>,
	"Sebastian Reichel" <sre@debian.org>,
	"Sebastian Reichel" <sre@ring0.de>,
	"kernel list" <linux-kernel@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	"Linux OMAP Mailing List" <linux-omap@vger.kernel.org>,
	"Tony Lindgren" <tony@atomide.com>,
	khilman@kernel.org, "Aaro Koskinen" <aaro.koskinen@iki.fi>,
	"Ивайло Димитров" <ivo.g.dimitrov.75@gmail.com>,
	"Gustavo F. Padovan" <gustavo@padovan.org>,
	"Johan Hedberg" <johan.hedberg@gmail.com>,
	linux-bluetooth@vger.kernel.org
Subject: Re: __hci_cmd_sync() not suitable for nokia h4p
Date: Thu, 11 Dec 2014 13:58:09 +0100	[thread overview]
Message-ID: <EE1A4B9D-30D5-4BD9-9A43-532619494A0C@holtmann.org> (raw)
In-Reply-To: <20141210131519.GA14748@amd>

Hi Pavel,

>>> The TODO file says:
>>> 
>>> # > +
>>> # > +     skb_queue_tail(&info->txq, fw_skb);
>>> # > +     spin_lock_irqsave(&info->lock, flags);
>>> # > +     hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
>>> # > +                     UART_IER_THRI);
>>> # > +     spin_unlock_irqrestore(&info->lock, flags);
>>> # > +}
>>> # 
>>> # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a
>>> # +hdev->setup callback for loading firmware and doing other setup details. You can just
>>> # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the
>>> # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the
>>> # +case with the Nokia firmware files.
>>> 
>>> ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is
>>> not suitable after all?
>> 
>> __hci_cmd_sync is to be used in hdev->setup where you load the firmware. However when hdev->setup is run, we expect that the HCI transport is fully up and running and that the driver takes care of the transport. That is done via hdev->send and hci_recv_frame.
>> 
> 
> h4p changes uart speed again after load of the firmware, but I guess
> that's ok.

if you can do it the other way around it would result in a faster init. Depending on how many patches are actually required to be loaded.

>>> But I don't understand what you want me to do at this point. I guess
>>> skb_queue_tail+hci_h4p_outb should be moved to a helper function
>>> (that's easy), and I already moved initialization to hci_setup().
>>> 
>>> nokia_core.c uses test_bit(HCI_RUNNING, &info->hdev->flags) to tell
>>> between initialization and data traffic, but I guess that's fine?
>> 
>> I have no idea on how much more I can explain this. There should be code in the driver that handles the HCI transport. That means init of the transport and sending and receiving HCI frames. And then there is the piece to load the firmware etc. These are two independent things.
>> 
> 
> Ok, it looks like __hci_cmd_sync() is indeed good match for the
> firmware load.
> 
>> 
>> What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard.
>> 
> 
> Speed changes at the end of firmware load, but I guess that's detail?
> Anyway, patch would look like this.

You should really look into providing hdev->setup() callback. That is normally the callback where you want to load the firmware.

Regards

Marcel


WARNING: multiple messages have this Message-ID (diff)
From: marcel@holtmann.org (Marcel Holtmann)
To: linux-arm-kernel@lists.infradead.org
Subject: __hci_cmd_sync() not suitable for nokia h4p
Date: Thu, 11 Dec 2014 13:58:09 +0100	[thread overview]
Message-ID: <EE1A4B9D-30D5-4BD9-9A43-532619494A0C@holtmann.org> (raw)
In-Reply-To: <20141210131519.GA14748@amd>

Hi Pavel,

>>> The TODO file says:
>>> 
>>> # > +
>>> # > +     skb_queue_tail(&info->txq, fw_skb);
>>> # > +     spin_lock_irqsave(&info->lock, flags);
>>> # > +     hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
>>> # > +                     UART_IER_THRI);
>>> # > +     spin_unlock_irqrestore(&info->lock, flags);
>>> # > +}
>>> # 
>>> # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a
>>> # +hdev->setup callback for loading firmware and doing other setup details. You can just
>>> # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the
>>> # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the
>>> # +case with the Nokia firmware files.
>>> 
>>> ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is
>>> not suitable after all?
>> 
>> __hci_cmd_sync is to be used in hdev->setup where you load the firmware. However when hdev->setup is run, we expect that the HCI transport is fully up and running and that the driver takes care of the transport. That is done via hdev->send and hci_recv_frame.
>> 
> 
> h4p changes uart speed again after load of the firmware, but I guess
> that's ok.

if you can do it the other way around it would result in a faster init. Depending on how many patches are actually required to be loaded.

>>> But I don't understand what you want me to do at this point. I guess
>>> skb_queue_tail+hci_h4p_outb should be moved to a helper function
>>> (that's easy), and I already moved initialization to hci_setup().
>>> 
>>> nokia_core.c uses test_bit(HCI_RUNNING, &info->hdev->flags) to tell
>>> between initialization and data traffic, but I guess that's fine?
>> 
>> I have no idea on how much more I can explain this. There should be code in the driver that handles the HCI transport. That means init of the transport and sending and receiving HCI frames. And then there is the piece to load the firmware etc. These are two independent things.
>> 
> 
> Ok, it looks like __hci_cmd_sync() is indeed good match for the
> firmware load.
> 
>> 
>> What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard.
>> 
> 
> Speed changes at the end of firmware load, but I guess that's detail?
> Anyway, patch would look like this.

You should really look into providing hdev->setup() callback. That is normally the callback where you want to load the firmware.

Regards

Marcel

  reply	other threads:[~2014-12-11 12:58 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-09 19:02 __hci_cmd_sync() not suitable for nokia h4p Pavel Machek
2014-12-09 19:02 ` Pavel Machek
2014-12-09 19:07 ` Marcel Holtmann
2014-12-09 19:07   ` Marcel Holtmann
2014-12-09 20:13   ` Pavel Machek
2014-12-09 20:13     ` Pavel Machek
2014-12-09 21:19     ` Marcel Holtmann
2014-12-09 21:19       ` Marcel Holtmann
2014-12-10 13:15       ` Pavel Machek
2014-12-10 13:15         ` Pavel Machek
2014-12-11 12:58         ` Marcel Holtmann [this message]
2014-12-11 12:58           ` Marcel Holtmann
2014-12-11 22:13           ` Pavel Machek
2014-12-11 22:13             ` Pavel Machek
2014-12-11 22:25             ` Marcel Holtmann
2014-12-11 22:25               ` Marcel Holtmann
2014-12-12  9:51               ` Pavel Machek
2014-12-12  9:51                 ` Pavel Machek
2014-12-12 12:28                 ` Marcel Holtmann
2014-12-12 12:28                   ` Marcel Holtmann
2014-12-12  1:15             ` Sebastian Reichel
2014-12-12  1:15               ` Sebastian Reichel
2014-12-12 12:14               ` Pavel Machek
2014-12-12 12:14                 ` Pavel Machek
2014-12-13 17:35                 ` Sebastian Reichel
2014-12-13 17:35                   ` Sebastian Reichel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=EE1A4B9D-30D5-4BD9-9A43-532619494A0C@holtmann.org \
    --to=marcel@holtmann.org \
    --cc=aaro.koskinen@iki.fi \
    --cc=gustavo@padovan.org \
    --cc=ivo.g.dimitrov.75@gmail.com \
    --cc=johan.hedberg@gmail.com \
    --cc=khilman@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=pali.rohar@gmail.com \
    --cc=pavel@ucw.cz \
    --cc=sre@debian.org \
    --cc=sre@ring0.de \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.