All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sebastian Reichel <sre@kernel.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: "Marcel Holtmann" <marcel@holtmann.org>,
	"Pali Rohár" <pali.rohar@gmail.com>,
	"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: Fri, 12 Dec 2014 02:15:04 +0100	[thread overview]
Message-ID: <20141212011504.GA16599@earth.universe> (raw)
In-Reply-To: <20141211221306.GA2905@amd>

[-- Attachment #1: Type: text/plain, Size: 3686 bytes --]

Hi,

On Thu, Dec 11, 2014 at 11:13:07PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > 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.
> 
> IIRC driver does two speed changes, so it looks to me like someone
> already tried that (and it did not work).

maybe. maybe not. Should be easy to test it (again) and add a
comment, shouldn't it?

> > >> 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.
> > 
> 
> I can provide setup() callback and load firmware from there.
> 
> I have created provisional device tree binding, and the driver still
> works.

I don't have time to look at the code now, but I have some comments
regarding the binding.

> Some time ago you mentioned that with the "big" issues fixed, you'd be
> willing to take it into the tree. What way forward do you see? Would
> it make sense to re-enable the driver in staging, so that "big"
> changes could be applied, followed by renames?
> 
> Thanks,
> 								Pavel
> 
> diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
> index 9e0e5a2..201f21b 100644
> --- a/arch/arm/boot/dts/omap3-n900.dts
> +++ b/arch/arm/boot/dts/omap3-n900.dts
> @@ -790,9 +776,21 @@
>  };
>  
>  &uart2 {
> +        compatible = "brcm,uart,bcm2048";

This does not look correct. The uart should not be overwritten. The
current h4p driver indeed implements a driver for the serial port,
but that's a) linux specific and does not belong in the DT and b)
should probably be changed in the mainline kernel.

>  	interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&uart2_pins>;
> +	device {
> +		  compatible = "brcm,bcm2048";
> +		  uart = <&uart2>;

You don't need a phandle to the parent device.

> +		  reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* want 91 */
> +		  host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* want 101 */

The host-wakeup should be mapped as irq, gpio2irq conversion
will happen ;)

> +		  bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* want 37 */

To be consistent with the n900 DTS file you should probably drop
"want " from the comments.

> +		  chip-type = <3>;

This should be set in the driver based on the compatible
value and not via DT data.

> +		  clocks = <&uart2_fck>, <&uart2_ick>;
> +		  clock-names = "fck", "ick";

These clocks you defined belong to the uart device and not to the
uart slave (bluetooth) device.

> +		  bt-sysclk = <2>;

I think this should be mapped cleanly in DT by adding a new clock
to the DTS file:

vctcxo_clock: clock  {
    compatible = "fixed-clock";
    #clock-cells = <0>;
    clock-frequency = <38400000>;
};

Then the bluetooth device can reference its clock device:

clocks = <&vctcxo_clock>;

The same clock reference should be added to the wl1251 DT node :)

> ... [code] ...

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: sre@kernel.org (Sebastian Reichel)
To: linux-arm-kernel@lists.infradead.org
Subject: __hci_cmd_sync() not suitable for nokia h4p
Date: Fri, 12 Dec 2014 02:15:04 +0100	[thread overview]
Message-ID: <20141212011504.GA16599@earth.universe> (raw)
In-Reply-To: <20141211221306.GA2905@amd>

Hi,

On Thu, Dec 11, 2014 at 11:13:07PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > 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.
> 
> IIRC driver does two speed changes, so it looks to me like someone
> already tried that (and it did not work).

maybe. maybe not. Should be easy to test it (again) and add a
comment, shouldn't it?

> > >> 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.
> > 
> 
> I can provide setup() callback and load firmware from there.
> 
> I have created provisional device tree binding, and the driver still
> works.

I don't have time to look at the code now, but I have some comments
regarding the binding.

> Some time ago you mentioned that with the "big" issues fixed, you'd be
> willing to take it into the tree. What way forward do you see? Would
> it make sense to re-enable the driver in staging, so that "big"
> changes could be applied, followed by renames?
> 
> Thanks,
> 								Pavel
> 
> diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
> index 9e0e5a2..201f21b 100644
> --- a/arch/arm/boot/dts/omap3-n900.dts
> +++ b/arch/arm/boot/dts/omap3-n900.dts
> @@ -790,9 +776,21 @@
>  };
>  
>  &uart2 {
> +        compatible = "brcm,uart,bcm2048";

This does not look correct. The uart should not be overwritten. The
current h4p driver indeed implements a driver for the serial port,
but that's a) linux specific and does not belong in the DT and b)
should probably be changed in the mainline kernel.

>  	interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&uart2_pins>;
> +	device {
> +		  compatible = "brcm,bcm2048";
> +		  uart = <&uart2>;

You don't need a phandle to the parent device.

> +		  reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* want 91 */
> +		  host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* want 101 */

The host-wakeup should be mapped as irq, gpio2irq conversion
will happen ;)

> +		  bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* want 37 */

To be consistent with the n900 DTS file you should probably drop
"want " from the comments.

> +		  chip-type = <3>;

This should be set in the driver based on the compatible
value and not via DT data.

> +		  clocks = <&uart2_fck>, <&uart2_ick>;
> +		  clock-names = "fck", "ick";

These clocks you defined belong to the uart device and not to the
uart slave (bluetooth) device.

> +		  bt-sysclk = <2>;

I think this should be mapped cleanly in DT by adding a new clock
to the DTS file:

vctcxo_clock: clock  {
    compatible = "fixed-clock";
    #clock-cells = <0>;
    clock-frequency = <38400000>;
};

Then the bluetooth device can reference its clock device:

clocks = <&vctcxo_clock>;

The same clock reference should be added to the wl1251 DT node :)

> ... [code] ...

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141212/e0496be9/attachment.sig>

  parent reply	other threads:[~2014-12-12  1:15 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
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 [this message]
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=20141212011504.GA16599@earth.universe \
    --to=sre@kernel.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=marcel@holtmann.org \
    --cc=pali.rohar@gmail.com \
    --cc=pavel@ucw.cz \
    --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.