All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maxime Ripard <maxime.ripard@free-electrons.com>
To: Chen-Yu Tsai <wens@csie.org>
Cc: Icenowy Zheng <icenowy@aosc.xyz>,
	Hans de Goede <hdegoede@redhat.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH] ARM: dts: sun8i-q8-common: enable bluetooth on SDIO Wi-Fi
Date: Tue, 20 Dec 2016 14:50:11 +0100	[thread overview]
Message-ID: <20161220135011.2rk7v6tjujhivcy6@lukather> (raw)
In-Reply-To: <CAGb2v67wU+Hw-6oa9FxzB33sZY8Up-59HZ_mneFBdcRhXbU1pA@mail.gmail.com>

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

On Mon, Dec 19, 2016 at 10:24:44PM +0800, Chen-Yu Tsai wrote:
> On Mon, Dec 19, 2016 at 10:08 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote:
> >
> >
> > 19.12.2016, 18:09, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >> On Fri, Dec 16, 2016 at 10:40:00PM +0800, Icenowy Zheng wrote:
> >>>  >>  > >  &r_pio {
> >>>  >>  > >  wifi_pwrseq_pin_q8: wifi_pwrseq_pin@0 {
> >>>  >>  > > - pins = "PL6", "PL7", "PL11";
> >>>  >>  > > + pins = "PL6", "PL7", "PL8", "PL11";
> >>>  >>  > >  function = "gpio_in";
> >>>  >>  > >  bias-pull-up;
> >>>  >>  > >  };
> >>>  >>  >
> >>>  >>  > There's several things wrong here. The first one is that you rely
> >>>  >>  > solely on the pinctrl state to maintain a reset line. This is very
> >>>  >>  > fragile (especially since the GPIO pinctrl state are likely to go away
> >>>  >>  > at some point), but it also means that if your driver wants to recover
> >>>  >>  > from that situation at some point, it won't work.
> >>>  >>  >
> >>>  >>  > The other one is that the bluetooth and wifi chips are two devices in
> >>>  >>  > linux, and you assign that pin to the wrong device (wifi).
> >>>  >>  >
> >>>  >>  > rfkill-gpio is made just for that, so please use it.
> >>>  >>
> >>>  >>  The GPIO is not for the radio, but for the full Bluetooth part.
> >>>  >
> >>>  > I know.
> >>>  >
> >>>  >>  If it's set to 0, then the bluetooth part will reset, and the
> >>>  >>  hciattach will fail.
> >>>  >
> >>>  > Both rfkill-gpio and rfkill-regulator will shutdown when called
> >>>  > (either by poking the reset pin or shutting down the regulator), so
> >>>  > that definitely seems like an expected behavior to put the device in
> >>>  > reset.
> >>>  >
> >>>  >>  The BSP uses this as a rfkill, and the result is that the bluetooth
> >>>  >>  on/off switch do not work properly.
> >>>  >
> >>>  > Then rfkill needs fixing, but working around it by hoping that the
> >>>  > core will probe an entirely different device, and enforcing a default
> >>>  > that the rest of the kernel might or might not change is both fragile
> >>>  > and wrong.
> >>>
> >>>  I think a rfkill-gpio here works just like the BSP rfkill...
> >>>
> >>>  The real problem is that the Realtek UART bluetooth driver is a userspace
> >>>  program (a modified hciattach), which is not capable of the GPIO reset...
> >>
> >> Can't you run rfkill before attaching? What is the problem exactly?
> >> It's not in reset for long enough?
> >>
> >> This seems more and more like an issue in the BT stack you're
> >> using. We might consider workarounds in the kernel, but they have to
> >> be correct.
> >
> > One more rfkill interface will be generated for hci0 after hciattach, which can
> > be safely toggled block and unblock.
> >
> > However, if the GPIO is toggled down, the hciattach program will die.
> >
> > The bluetooth stack I used is fd.o's BlueZ.
> 
> I think the bigger issue is that the tty/serial subsystem does not have
> power sequencing support. Here we're trying to use rfkill to do that,
> but that doesn't seem to be what it was intended for. It might work with
> standalone USB bluetooth controllers that don't need any special setup,
> since the device will just appear and get registered. But it might not
> work so well with UART based adapters that need userspace fiddling with
> firmware and hciattach.

Then you can also have a look at the generic power sequence patches
that are floating around.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Maxime Ripard <maxime.ripard@free-electrons.com>
To: Chen-Yu Tsai <wens@csie.org>
Cc: Hans de Goede <hdegoede@redhat.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Icenowy Zheng <icenowy@aosc.xyz>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ARM: dts: sun8i-q8-common: enable bluetooth on SDIO Wi-Fi
Date: Tue, 20 Dec 2016 14:50:11 +0100	[thread overview]
Message-ID: <20161220135011.2rk7v6tjujhivcy6@lukather> (raw)
In-Reply-To: <CAGb2v67wU+Hw-6oa9FxzB33sZY8Up-59HZ_mneFBdcRhXbU1pA@mail.gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 3618 bytes --]

On Mon, Dec 19, 2016 at 10:24:44PM +0800, Chen-Yu Tsai wrote:
> On Mon, Dec 19, 2016 at 10:08 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote:
> >
> >
> > 19.12.2016, 18:09, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >> On Fri, Dec 16, 2016 at 10:40:00PM +0800, Icenowy Zheng wrote:
> >>>  >>  > >  &r_pio {
> >>>  >>  > >  wifi_pwrseq_pin_q8: wifi_pwrseq_pin@0 {
> >>>  >>  > > - pins = "PL6", "PL7", "PL11";
> >>>  >>  > > + pins = "PL6", "PL7", "PL8", "PL11";
> >>>  >>  > >  function = "gpio_in";
> >>>  >>  > >  bias-pull-up;
> >>>  >>  > >  };
> >>>  >>  >
> >>>  >>  > There's several things wrong here. The first one is that you rely
> >>>  >>  > solely on the pinctrl state to maintain a reset line. This is very
> >>>  >>  > fragile (especially since the GPIO pinctrl state are likely to go away
> >>>  >>  > at some point), but it also means that if your driver wants to recover
> >>>  >>  > from that situation at some point, it won't work.
> >>>  >>  >
> >>>  >>  > The other one is that the bluetooth and wifi chips are two devices in
> >>>  >>  > linux, and you assign that pin to the wrong device (wifi).
> >>>  >>  >
> >>>  >>  > rfkill-gpio is made just for that, so please use it.
> >>>  >>
> >>>  >>  The GPIO is not for the radio, but for the full Bluetooth part.
> >>>  >
> >>>  > I know.
> >>>  >
> >>>  >>  If it's set to 0, then the bluetooth part will reset, and the
> >>>  >>  hciattach will fail.
> >>>  >
> >>>  > Both rfkill-gpio and rfkill-regulator will shutdown when called
> >>>  > (either by poking the reset pin or shutting down the regulator), so
> >>>  > that definitely seems like an expected behavior to put the device in
> >>>  > reset.
> >>>  >
> >>>  >>  The BSP uses this as a rfkill, and the result is that the bluetooth
> >>>  >>  on/off switch do not work properly.
> >>>  >
> >>>  > Then rfkill needs fixing, but working around it by hoping that the
> >>>  > core will probe an entirely different device, and enforcing a default
> >>>  > that the rest of the kernel might or might not change is both fragile
> >>>  > and wrong.
> >>>
> >>>  I think a rfkill-gpio here works just like the BSP rfkill...
> >>>
> >>>  The real problem is that the Realtek UART bluetooth driver is a userspace
> >>>  program (a modified hciattach), which is not capable of the GPIO reset...
> >>
> >> Can't you run rfkill before attaching? What is the problem exactly?
> >> It's not in reset for long enough?
> >>
> >> This seems more and more like an issue in the BT stack you're
> >> using. We might consider workarounds in the kernel, but they have to
> >> be correct.
> >
> > One more rfkill interface will be generated for hci0 after hciattach, which can
> > be safely toggled block and unblock.
> >
> > However, if the GPIO is toggled down, the hciattach program will die.
> >
> > The bluetooth stack I used is fd.o's BlueZ.
> 
> I think the bigger issue is that the tty/serial subsystem does not have
> power sequencing support. Here we're trying to use rfkill to do that,
> but that doesn't seem to be what it was intended for. It might work with
> standalone USB bluetooth controllers that don't need any special setup,
> since the device will just appear and get registered. But it might not
> work so well with UART based adapters that need userspace fiddling with
> firmware and hciattach.

Then you can also have a look at the generic power sequence patches
that are floating around.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: maxime.ripard@free-electrons.com (Maxime Ripard)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] ARM: dts: sun8i-q8-common: enable bluetooth on SDIO Wi-Fi
Date: Tue, 20 Dec 2016 14:50:11 +0100	[thread overview]
Message-ID: <20161220135011.2rk7v6tjujhivcy6@lukather> (raw)
In-Reply-To: <CAGb2v67wU+Hw-6oa9FxzB33sZY8Up-59HZ_mneFBdcRhXbU1pA@mail.gmail.com>

On Mon, Dec 19, 2016 at 10:24:44PM +0800, Chen-Yu Tsai wrote:
> On Mon, Dec 19, 2016 at 10:08 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote:
> >
> >
> > 19.12.2016, 18:09, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >> On Fri, Dec 16, 2016 at 10:40:00PM +0800, Icenowy Zheng wrote:
> >>>  >>  > >  &r_pio {
> >>>  >>  > >  wifi_pwrseq_pin_q8: wifi_pwrseq_pin at 0 {
> >>>  >>  > > - pins = "PL6", "PL7", "PL11";
> >>>  >>  > > + pins = "PL6", "PL7", "PL8", "PL11";
> >>>  >>  > >  function = "gpio_in";
> >>>  >>  > >  bias-pull-up;
> >>>  >>  > >  };
> >>>  >>  >
> >>>  >>  > There's several things wrong here. The first one is that you rely
> >>>  >>  > solely on the pinctrl state to maintain a reset line. This is very
> >>>  >>  > fragile (especially since the GPIO pinctrl state are likely to go away
> >>>  >>  > at some point), but it also means that if your driver wants to recover
> >>>  >>  > from that situation at some point, it won't work.
> >>>  >>  >
> >>>  >>  > The other one is that the bluetooth and wifi chips are two devices in
> >>>  >>  > linux, and you assign that pin to the wrong device (wifi).
> >>>  >>  >
> >>>  >>  > rfkill-gpio is made just for that, so please use it.
> >>>  >>
> >>>  >>  The GPIO is not for the radio, but for the full Bluetooth part.
> >>>  >
> >>>  > I know.
> >>>  >
> >>>  >>  If it's set to 0, then the bluetooth part will reset, and the
> >>>  >>  hciattach will fail.
> >>>  >
> >>>  > Both rfkill-gpio and rfkill-regulator will shutdown when called
> >>>  > (either by poking the reset pin or shutting down the regulator), so
> >>>  > that definitely seems like an expected behavior to put the device in
> >>>  > reset.
> >>>  >
> >>>  >>  The BSP uses this as a rfkill, and the result is that the bluetooth
> >>>  >>  on/off switch do not work properly.
> >>>  >
> >>>  > Then rfkill needs fixing, but working around it by hoping that the
> >>>  > core will probe an entirely different device, and enforcing a default
> >>>  > that the rest of the kernel might or might not change is both fragile
> >>>  > and wrong.
> >>>
> >>>  I think a rfkill-gpio here works just like the BSP rfkill...
> >>>
> >>>  The real problem is that the Realtek UART bluetooth driver is a userspace
> >>>  program (a modified hciattach), which is not capable of the GPIO reset...
> >>
> >> Can't you run rfkill before attaching? What is the problem exactly?
> >> It's not in reset for long enough?
> >>
> >> This seems more and more like an issue in the BT stack you're
> >> using. We might consider workarounds in the kernel, but they have to
> >> be correct.
> >
> > One more rfkill interface will be generated for hci0 after hciattach, which can
> > be safely toggled block and unblock.
> >
> > However, if the GPIO is toggled down, the hciattach program will die.
> >
> > The bluetooth stack I used is fd.o's BlueZ.
> 
> I think the bigger issue is that the tty/serial subsystem does not have
> power sequencing support. Here we're trying to use rfkill to do that,
> but that doesn't seem to be what it was intended for. It might work with
> standalone USB bluetooth controllers that don't need any special setup,
> since the device will just appear and get registered. But it might not
> work so well with UART based adapters that need userspace fiddling with
> firmware and hciattach.

Then you can also have a look at the generic power sequence patches
that are floating around.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161220/4ffe5077/attachment.sig>

  reply	other threads:[~2016-12-20 13:50 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20161213233658.atGuNCNY@smtp1h.mail.yandex.net>
2016-12-16 12:47 ` [PATCH] ARM: dts: sun8i-q8-common: enable bluetooth on SDIO Wi-Fi Maxime Ripard
2016-12-16 12:47   ` Maxime Ripard
2016-12-16 12:47   ` Maxime Ripard
2016-12-16 14:40   ` Icenowy Zheng
2016-12-16 14:40     ` Icenowy Zheng
2016-12-19 10:09     ` Maxime Ripard
2016-12-19 10:09       ` Maxime Ripard
2016-12-19 10:09       ` Maxime Ripard
2016-12-19 14:08       ` Icenowy Zheng
2016-12-19 14:08         ` Icenowy Zheng
2016-12-19 14:24         ` Chen-Yu Tsai
2016-12-19 14:24           ` Chen-Yu Tsai
2016-12-19 14:24           ` Chen-Yu Tsai
2016-12-20 13:50           ` Maxime Ripard [this message]
2016-12-20 13:50             ` Maxime Ripard
2016-12-20 13:50             ` Maxime Ripard
2016-12-06  8:08 Icenowy Zheng
2016-12-06  8:08 ` Icenowy Zheng
2016-12-09  8:07 ` Maxime Ripard
2016-12-09  8:07   ` Maxime Ripard
2016-12-09  8:07   ` Maxime Ripard

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=20161220135011.2rk7v6tjujhivcy6@lukather \
    --to=maxime.ripard@free-electrons.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hdegoede@redhat.com \
    --cc=icenowy@aosc.xyz \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=wens@csie.org \
    /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.