[7/7] ARM: sun7i: cubietruck: enable bluetooth module
diff mbox

Message ID 1397544101-18135-8-git-send-email-wens@csie.org
State New, archived
Headers show

Commit Message

Chen-Yu Tsai (Moxa) April 15, 2014, 6:41 a.m. UTC
The CubieTruck has an AMPAK AP6210 WiFi+Bluetooth module. The Bluetooth
part is a BCM20710 device connected to UART2 in the A20 SoC.

The IC requires a 32.768 KHz low power clock input for proper
auto-detection of the main clock, and an enable signal via GPIO.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Maxime Ripard April 15, 2014, 2:42 p.m. UTC | #1
On Tue, Apr 15, 2014 at 02:41:41PM +0800, Chen-Yu Tsai wrote:
> The CubieTruck has an AMPAK AP6210 WiFi+Bluetooth module. The Bluetooth
> part is a BCM20710 device connected to UART2 in the A20 SoC.
> 
> The IC requires a 32.768 KHz low power clock input for proper
> auto-detection of the main clock, and an enable signal via GPIO.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> index cb25d3c..767c8e1 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> @@ -61,6 +61,13 @@
>  				allwinner,drive = <0>;
>  				allwinner,pull = <0>;
>  			};
> +
> +			bt_pwr_pin_cubietruck: bt_pwr_pin@0 {
> +				allwinner,pins = "PH18";
> +				allwinner,function = "gpio_out";
> +				allwinner,drive = <0>;
> +				allwinner,pull = <0>;
> +			};
>  		};
>  
>  		uart0: serial@01c28000 {
> @@ -69,6 +76,12 @@
>  			status = "okay";
>  		};
>  
> +		uart2: serial@01c28800 {
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&uart2_pins_a>;
> +			status = "okay";
> +		};
> +

Please make this a separate patch.

>  		i2c0: i2c@01c2ac00 {
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&i2c0_pins_a>;
> @@ -139,4 +152,16 @@
>  	reg_usb2_vbus: usb2-vbus {
>  		status = "okay";
>  	};
> +
> +	rfkill_bt {
> +		compatible = "rfkill-gpio";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> +		clocks = <&clk_out_a>;
> +		clock-frequency = <32768>;
> +		gpios = <&pio 7 18 0>; /* PH18 */
> +		gpio-names = "reset";
> +		rfkill-name = "bt";
> +		rfkill-type = <2>;
> +	};

Hmmm, I don't think that's actually right.

If you have such a device, then I'd expect it to be represented as a
full device in the DT, probably with one part for the WiFi, one part
for the Bluetooth, and here the definition of the rfkill device that
controls it.

But tying parts of the device to the rfkill that controls it, such as
the clocks, or the frequency it runs at seems just wrong.

Maxime
Chen-Yu Tsai (Moxa) April 15, 2014, 4:06 p.m. UTC | #2
On Tue, Apr 15, 2014 at 10:42 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Apr 15, 2014 at 02:41:41PM +0800, Chen-Yu Tsai wrote:
>> The CubieTruck has an AMPAK AP6210 WiFi+Bluetooth module. The Bluetooth
>> part is a BCM20710 device connected to UART2 in the A20 SoC.
>>
>> The IC requires a 32.768 KHz low power clock input for proper
>> auto-detection of the main clock, and an enable signal via GPIO.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> index cb25d3c..767c8e1 100644
>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> @@ -61,6 +61,13 @@
>>                               allwinner,drive = <0>;
>>                               allwinner,pull = <0>;
>>                       };
>> +
>> +                     bt_pwr_pin_cubietruck: bt_pwr_pin@0 {
>> +                             allwinner,pins = "PH18";
>> +                             allwinner,function = "gpio_out";
>> +                             allwinner,drive = <0>;
>> +                             allwinner,pull = <0>;
>> +                     };
>>               };
>>
>>               uart0: serial@01c28000 {
>> @@ -69,6 +76,12 @@
>>                       status = "okay";
>>               };
>>
>> +             uart2: serial@01c28800 {
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&uart2_pins_a>;
>> +                     status = "okay";
>> +             };
>> +
>
> Please make this a separate patch.

Will do.

>>               i2c0: i2c@01c2ac00 {
>>                       pinctrl-names = "default";
>>                       pinctrl-0 = <&i2c0_pins_a>;
>> @@ -139,4 +152,16 @@
>>       reg_usb2_vbus: usb2-vbus {
>>               status = "okay";
>>       };
>> +
>> +     rfkill_bt {
>> +             compatible = "rfkill-gpio";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>> +             clocks = <&clk_out_a>;
>> +             clock-frequency = <32768>;
>> +             gpios = <&pio 7 18 0>; /* PH18 */
>> +             gpio-names = "reset";
>> +             rfkill-name = "bt";
>> +             rfkill-type = <2>;
>> +     };
>
> Hmmm, I don't think that's actually right.
>
> If you have such a device, then I'd expect it to be represented as a
> full device in the DT, probably with one part for the WiFi, one part
> for the Bluetooth, and here the definition of the rfkill device that
> controls it.

The AP6210 is not one device, but 2 separate chips in one module. Each
chip has its own controls and interface. They just so happen to share
the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
and interfaces. The WiFi side is most likely connected via SDIO, while
the Bluetooth side is connected to a UART, and optionally I2S for sound.

> But tying parts of the device to the rfkill that controls it, such as
> the clocks, or the frequency it runs at seems just wrong.

I understand where you're coming from. For devices on buses that require
drivers (such as USB, SDIO) these properties probably should be tied to
the device node.

For our use case here, which is a bluetooth chip connected on the UART,
there is no in kernel representation or driver to tie them to. Same goes
for UART based GPS chips. They just so happen to require toggling a GPIO,
and maybe enabling a specific clock, to get it running. Afterwards,
accessing it is done solely from userspace. For our Broadcom chips, the
user has to upload its firmware first, then designate the tty as a Bluetooth
HCI using hciattach.

We are using the rfkill device as a on-off switch.

Hope this explains the situation.


Cheers
ChenYu
Alan Cox April 15, 2014, 4:18 p.m. UTC | #3
> For our use case here, which is a bluetooth chip connected on the UART,
> there is no in kernel representation or driver to tie them to. Same goes
> for UART based GPS chips. They just so happen to require toggling a GPIO,
> and maybe enabling a specific clock, to get it running. Afterwards,
> accessing it is done solely from userspace. For our Broadcom chips, the
> user has to upload its firmware first, then designate the tty as a Bluetooth
> HCI using hciattach.

Somewhat similar on some ordinary PC systems. ACPI describes the
properties there but the same things apply - its a device mostly
controlled by a blob of userspace and having a bunch of GPIO lines that
do stuff like turn it on and off.

Is there any reason for not describing it properly and letting the
userspace code fish it out of the devicetree ?

There's no fundamental reason that it's magically different because of
the location of the driver. Given in a few cases we have a choice of
kernel or user space devices for the same hardware thats even more true ?

Failing that we have a long standing proposal never implemented for
adding GPIO awareness to the tty layer. There are a few other use
cases for it including gpio widgets with serial and either some of the
modem lines hacked in via GPIO or with additional control lines for
stuff like smartcard reading interfaces.

https://lkml.org/lkml/2012/5/16/288

In hindsight I'd do it slightly differently and make each "gpio" a pair
of values (purpose,number).

Alan
Maxime Ripard April 16, 2014, 9:44 a.m. UTC | #4
Hi,

Please try to keep me in CC, even though the ML doesn't make it easy..

On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
> >> @@ -139,4 +152,16 @@
> >>       reg_usb2_vbus: usb2-vbus {
> >>               status = "okay";
> >>       };
> >> +
> >> +     rfkill_bt {
> >> +             compatible = "rfkill-gpio";
> >> +             pinctrl-names = "default";
> >> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> >> +             clocks = <&clk_out_a>;
> >> +             clock-frequency = <32768>;
> >> +             gpios = <&pio 7 18 0>; /* PH18 */
> >> +             gpio-names = "reset";
> >> +             rfkill-name = "bt";
> >> +             rfkill-type = <2>;
> >> +     };
> >
> > Hmmm, I don't think that's actually right.
> >
> > If you have such a device, then I'd expect it to be represented as a
> > full device in the DT, probably with one part for the WiFi, one part
> > for the Bluetooth, and here the definition of the rfkill device that
> > controls it.
> 
> The AP6210 is not one device, but 2 separate chips in one module. Each
> chip has its own controls and interface. They just so happen to share
> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
> and interfaces. The WiFi side is most likely connected via SDIO, while
> the Bluetooth side is connected to a UART, and optionally I2S for sound.

It's even easier to represent then.

> > But tying parts of the device to the rfkill that controls it, such as
> > the clocks, or the frequency it runs at seems just wrong.
> 
> I understand where you're coming from. For devices on buses that require
> drivers (such as USB, SDIO) these properties probably should be tied to
> the device node.
> 
> For our use case here, which is a bluetooth chip connected on the UART,
> there is no in kernel representation or driver to tie them to. Same goes
> for UART based GPS chips. They just so happen to require toggling a GPIO,
> and maybe enabling a specific clock, to get it running. Afterwards,
> accessing it is done solely from userspace. For our Broadcom chips, the
> user has to upload its firmware first, then designate the tty as a Bluetooth
> HCI using hciattach.
> 
> We are using the rfkill device as a on-off switch.

I understand your point, but the fact that it's implemented in
user-space, or that UART is not a bus (which probably should be), is
only a Linux specific story, and how it's implemented in Linux (even
if the whole rfkill node is another one, but let's stay on topic).

This is a huge abstraction leak.

Let's say you need the I2S stream you mentionned for some
reason. Would you tie the audio stream to the rfkill node as well?
I'm sorry, but from an hardware description perspective, it makes no
sense.

What's the feeling of the DT maintainers?
Chen-Yu Tsai (Moxa) April 16, 2014, 10:39 a.m. UTC | #5
Hi,

On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> Please try to keep me in CC, even though the ML doesn't make it easy..

Sorry about that.

> On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
>> >> @@ -139,4 +152,16 @@
>> >>       reg_usb2_vbus: usb2-vbus {
>> >>               status = "okay";
>> >>       };
>> >> +
>> >> +     rfkill_bt {
>> >> +             compatible = "rfkill-gpio";
>> >> +             pinctrl-names = "default";
>> >> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>> >> +             clocks = <&clk_out_a>;
>> >> +             clock-frequency = <32768>;
>> >> +             gpios = <&pio 7 18 0>; /* PH18 */
>> >> +             gpio-names = "reset";
>> >> +             rfkill-name = "bt";
>> >> +             rfkill-type = <2>;
>> >> +     };
>> >
>> > Hmmm, I don't think that's actually right.
>> >
>> > If you have such a device, then I'd expect it to be represented as a
>> > full device in the DT, probably with one part for the WiFi, one part
>> > for the Bluetooth, and here the definition of the rfkill device that
>> > controls it.
>>
>> The AP6210 is not one device, but 2 separate chips in one module. Each
>> chip has its own controls and interface. They just so happen to share
>> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
>> and interfaces. The WiFi side is most likely connected via SDIO, while
>> the Bluetooth side is connected to a UART, and optionally I2S for sound.
>
> It's even easier to represent then.
>
>> > But tying parts of the device to the rfkill that controls it, such as
>> > the clocks, or the frequency it runs at seems just wrong.
>>
>> I understand where you're coming from. For devices on buses that require
>> drivers (such as USB, SDIO) these properties probably should be tied to
>> the device node.
>>
>> For our use case here, which is a bluetooth chip connected on the UART,
>> there is no in kernel representation or driver to tie them to. Same goes
>> for UART based GPS chips. They just so happen to require toggling a GPIO,
>> and maybe enabling a specific clock, to get it running. Afterwards,
>> accessing it is done solely from userspace. For our Broadcom chips, the
>> user has to upload its firmware first, then designate the tty as a Bluetooth
>> HCI using hciattach.
>>
>> We are using the rfkill device as a on-off switch.
>
> I understand your point, but the fact that it's implemented in
> user-space, or that UART is not a bus (which probably should be), is
> only a Linux specific story, and how it's implemented in Linux (even
> if the whole rfkill node is another one, but let's stay on topic).

I gave it some thought last night. You are right. My whole approach
is wrong. But let's try to make it right.

So considering the fact that it's primarily connected to a UART,
maybe I should make it a sub-node to the UART node it's actually
connected to? Something like:

        uart2: serial@01c28800 {
                pinctrl-names = "default";
                pinctrl-0 = <&uart2_pins_a>;
                status = "okay";

                bt: bt_hci {
                        compatible = "brcm,bcm20710";
                        /* maybe add some generic compatible */
                        pinctrl-names = "default";
                        pinctrl-0 = <&clk_out_a_pins_a>,
<&bt_pwr_pin_cubietruck>;
                        clocks = <&clk_out_a>;
                        clock-frequency = <32768>;
                        gpios = <&pio 7 18 0>; /* PH18 */
                };
        };

And let the uart core handle power sequencing for sub-nodes.

The rfkill node would still have the gpios and clocks, but not the
clock-frequency property. It's sole purpose would be to toggle the
controls. But I think the placement is still odd. Perhaps these
virtual devices shouldn't live in the DT at all.

> This is a huge abstraction leak.
>
> Let's say you need the I2S stream you mentionned for some
> reason. Would you tie the audio stream to the rfkill node as well?
> I'm sorry, but from an hardware description perspective, it makes no
> sense.

The above revision should be better, from a hardware perspective. I'm
not sure how to tie in the I2S stream, and there I haven't found any
examples in the DT tree.

> What's the feeling of the DT maintainers?


Cheers

ChenYu
Hans de Goede April 16, 2014, 1:08 p.m. UTC | #6
Hi,

On 04/16/2014 11:44 AM, Maxime Ripard wrote:
> Hi,
> 
> Please try to keep me in CC, even though the ML doesn't make it easy..
> 
> On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
>>>> @@ -139,4 +152,16 @@
>>>>       reg_usb2_vbus: usb2-vbus {
>>>>               status = "okay";
>>>>       };
>>>> +
>>>> +     rfkill_bt {
>>>> +             compatible = "rfkill-gpio";
>>>> +             pinctrl-names = "default";
>>>> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>>>> +             clocks = <&clk_out_a>;
>>>> +             clock-frequency = <32768>;
>>>> +             gpios = <&pio 7 18 0>; /* PH18 */
>>>> +             gpio-names = "reset";
>>>> +             rfkill-name = "bt";
>>>> +             rfkill-type = <2>;
>>>> +     };
>>>
>>> Hmmm, I don't think that's actually right.
>>>
>>> If you have such a device, then I'd expect it to be represented as a
>>> full device in the DT, probably with one part for the WiFi, one part
>>> for the Bluetooth, and here the definition of the rfkill device that
>>> controls it.
>>
>> The AP6210 is not one device, but 2 separate chips in one module. Each
>> chip has its own controls and interface. They just so happen to share
>> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
>> and interfaces. The WiFi side is most likely connected via SDIO, while
>> the Bluetooth side is connected to a UART, and optionally I2S for sound.
> 
> It's even easier to represent then.
> 
>>> But tying parts of the device to the rfkill that controls it, such as
>>> the clocks, or the frequency it runs at seems just wrong.
>>
>> I understand where you're coming from. For devices on buses that require
>> drivers (such as USB, SDIO) these properties probably should be tied to
>> the device node.
>>
>> For our use case here, which is a bluetooth chip connected on the UART,
>> there is no in kernel representation or driver to tie them to. Same goes
>> for UART based GPS chips. They just so happen to require toggling a GPIO,
>> and maybe enabling a specific clock, to get it running. Afterwards,
>> accessing it is done solely from userspace. For our Broadcom chips, the
>> user has to upload its firmware first, then designate the tty as a Bluetooth
>> HCI using hciattach.
>>
>> We are using the rfkill device as a on-off switch.
> 
> I understand your point, but the fact that it's implemented in
> user-space, or that UART is not a bus (which probably should be), is
> only a Linux specific story, and how it's implemented in Linux (even
> if the whole rfkill node is another one, but let's stay on topic).
> 
> This is a huge abstraction leak.
> 
> Let's say you need the I2S stream you mentionned for some
> reason. Would you tie the audio stream to the rfkill node as well?
> I'm sorry, but from an hardware description perspective, it makes no
> sense.

Sorry wens, but I have to agree with Maxime here. What I really would
like to see is the bluetooth part of the AP6210 be a child node of the
uart node like how i2c slaves are child nodes of the i2c master node.

With my distro maintainer had on, I say that even under Linux we will
need this. Sure we can get things to work from userspace manually,
by running the necessary commands. But we want things to work automatically,
so we will need to know about the attached bluetooth device in userspace,
and the proper way to do that is to put the info in devicetree. I've not
yet thought about how userspace can then extract and react on this info,
but for things to work plug and play with a generic distro rootfs we
will need to eventually have some udev rules doing the firmware loading
and hciattach. Which starts with being able to identify that the uart
has a bcm hci bluetooth adapter attached.

Regards,

Hans
Hans de Goede April 16, 2014, 1:09 p.m. UTC | #7
Hi,

On 04/16/2014 12:39 PM, Chen-Yu Tsai wrote:
> Hi,
> 
> On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> Hi,
>>
>> Please try to keep me in CC, even though the ML doesn't make it easy..
> 
> Sorry about that.
> 
>> On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
>>>>> @@ -139,4 +152,16 @@
>>>>>       reg_usb2_vbus: usb2-vbus {
>>>>>               status = "okay";
>>>>>       };
>>>>> +
>>>>> +     rfkill_bt {
>>>>> +             compatible = "rfkill-gpio";
>>>>> +             pinctrl-names = "default";
>>>>> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>>>>> +             clocks = <&clk_out_a>;
>>>>> +             clock-frequency = <32768>;
>>>>> +             gpios = <&pio 7 18 0>; /* PH18 */
>>>>> +             gpio-names = "reset";
>>>>> +             rfkill-name = "bt";
>>>>> +             rfkill-type = <2>;
>>>>> +     };
>>>>
>>>> Hmmm, I don't think that's actually right.
>>>>
>>>> If you have such a device, then I'd expect it to be represented as a
>>>> full device in the DT, probably with one part for the WiFi, one part
>>>> for the Bluetooth, and here the definition of the rfkill device that
>>>> controls it.
>>>
>>> The AP6210 is not one device, but 2 separate chips in one module. Each
>>> chip has its own controls and interface. They just so happen to share
>>> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
>>> and interfaces. The WiFi side is most likely connected via SDIO, while
>>> the Bluetooth side is connected to a UART, and optionally I2S for sound.
>>
>> It's even easier to represent then.
>>
>>>> But tying parts of the device to the rfkill that controls it, such as
>>>> the clocks, or the frequency it runs at seems just wrong.
>>>
>>> I understand where you're coming from. For devices on buses that require
>>> drivers (such as USB, SDIO) these properties probably should be tied to
>>> the device node.
>>>
>>> For our use case here, which is a bluetooth chip connected on the UART,
>>> there is no in kernel representation or driver to tie them to. Same goes
>>> for UART based GPS chips. They just so happen to require toggling a GPIO,
>>> and maybe enabling a specific clock, to get it running. Afterwards,
>>> accessing it is done solely from userspace. For our Broadcom chips, the
>>> user has to upload its firmware first, then designate the tty as a Bluetooth
>>> HCI using hciattach.
>>>
>>> We are using the rfkill device as a on-off switch.
>>
>> I understand your point, but the fact that it's implemented in
>> user-space, or that UART is not a bus (which probably should be), is
>> only a Linux specific story, and how it's implemented in Linux (even
>> if the whole rfkill node is another one, but let's stay on topic).
> 
> I gave it some thought last night. You are right. My whole approach
> is wrong. But let's try to make it right.
> 
> So considering the fact that it's primarily connected to a UART,
> maybe I should make it a sub-node to the UART node it's actually
> connected to? Something like:
> 
>         uart2: serial@01c28800 {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&uart2_pins_a>;
>                 status = "okay";
> 
>                 bt: bt_hci {
>                         compatible = "brcm,bcm20710";
>                         /* maybe add some generic compatible */
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&clk_out_a_pins_a>,
> <&bt_pwr_pin_cubietruck>;
>                         clocks = <&clk_out_a>;
>                         clock-frequency = <32768>;
>                         gpios = <&pio 7 18 0>; /* PH18 */
>                 };
>         };
> 
> And let the uart core handle power sequencing for sub-nodes.

Great, I missed this reply when I typed my mail I send a few minutes
ago. I agree that this approach is how thing should be.

Regards,

Hans
Arend van Spriel April 17, 2014, 7:43 a.m. UTC | #8
+ linux-serial@vger.kernel.org

On 16/04/14 15:09, Hans de Goede wrote:
> Hi,
>
> On 04/16/2014 12:39 PM, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>>> Hi,
>>>
>>> Please try to keep me in CC, even though the ML doesn't make it easy..
>>
>> Sorry about that.
>>
>>> On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
>>>>>> @@ -139,4 +152,16 @@
>>>>>>        reg_usb2_vbus: usb2-vbus {
>>>>>>                status = "okay";
>>>>>>        };
>>>>>> +
>>>>>> +     rfkill_bt {
>>>>>> +             compatible = "rfkill-gpio";
>>>>>> +             pinctrl-names = "default";
>>>>>> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>>>>>> +             clocks = <&clk_out_a>;
>>>>>> +             clock-frequency = <32768>;
>>>>>> +             gpios = <&pio 7 18 0>; /* PH18 */
>>>>>> +             gpio-names = "reset";
>>>>>> +             rfkill-name = "bt";
>>>>>> +             rfkill-type = <2>;
>>>>>> +     };
>>>>>
>>>>> Hmmm, I don't think that's actually right.
>>>>>
>>>>> If you have such a device, then I'd expect it to be represented as a
>>>>> full device in the DT, probably with one part for the WiFi, one part
>>>>> for the Bluetooth, and here the definition of the rfkill device that
>>>>> controls it.
>>>>
>>>> The AP6210 is not one device, but 2 separate chips in one module. Each
>>>> chip has its own controls and interface. They just so happen to share
>>>> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
>>>> and interfaces. The WiFi side is most likely connected via SDIO, while
>>>> the Bluetooth side is connected to a UART, and optionally I2S for sound.
>>>
>>> It's even easier to represent then.
>>>
>>>>> But tying parts of the device to the rfkill that controls it, such as
>>>>> the clocks, or the frequency it runs at seems just wrong.
>>>>
>>>> I understand where you're coming from. For devices on buses that require
>>>> drivers (such as USB, SDIO) these properties probably should be tied to
>>>> the device node.
>>>>
>>>> For our use case here, which is a bluetooth chip connected on the UART,
>>>> there is no in kernel representation or driver to tie them to. Same goes
>>>> for UART based GPS chips. They just so happen to require toggling a GPIO,
>>>> and maybe enabling a specific clock, to get it running. Afterwards,
>>>> accessing it is done solely from userspace. For our Broadcom chips, the
>>>> user has to upload its firmware first, then designate the tty as a Bluetooth
>>>> HCI using hciattach.
>>>>
>>>> We are using the rfkill device as a on-off switch.
>>>
>>> I understand your point, but the fact that it's implemented in
>>> user-space, or that UART is not a bus (which probably should be), is
>>> only a Linux specific story, and how it's implemented in Linux (even
>>> if the whole rfkill node is another one, but let's stay on topic).
>>
>> I gave it some thought last night. You are right. My whole approach
>> is wrong. But let's try to make it right.
>>
>> So considering the fact that it's primarily connected to a UART,
>> maybe I should make it a sub-node to the UART node it's actually
>> connected to? Something like:
>>
>>          uart2: serial@01c28800 {
>>                  pinctrl-names = "default";
>>                  pinctrl-0 = <&uart2_pins_a>;
>>                  status = "okay";
>>
>>                  bt: bt_hci {
>>                          compatible = "brcm,bcm20710";
>>                          /* maybe add some generic compatible */
>>                          pinctrl-names = "default";
>>                          pinctrl-0 = <&clk_out_a_pins_a>,
>> <&bt_pwr_pin_cubietruck>;
>>                          clocks = <&clk_out_a>;
>>                          clock-frequency = <32768>;
>>                          gpios = <&pio 7 18 0>; /* PH18 */
>>                  };
>>          };
>>
>> And let the uart core handle power sequencing for sub-nodes.
>
> Great, I missed this reply when I typed my mail I send a few minutes
> ago. I agree that this approach is how thing should be.

Regarding the device tree hierarchy this seems right, but powering the 
sub-nodes seems outside the realm of uart core.

> Regards,
>
> Hans
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Maxime Ripard April 18, 2014, 5:47 p.m. UTC | #9
On Wed, Apr 16, 2014 at 06:39:28PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > Please try to keep me in CC, even though the ML doesn't make it easy..
> 
> Sorry about that.
> 
> > On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
> >> >> @@ -139,4 +152,16 @@
> >> >>       reg_usb2_vbus: usb2-vbus {
> >> >>               status = "okay";
> >> >>       };
> >> >> +
> >> >> +     rfkill_bt {
> >> >> +             compatible = "rfkill-gpio";
> >> >> +             pinctrl-names = "default";
> >> >> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> >> >> +             clocks = <&clk_out_a>;
> >> >> +             clock-frequency = <32768>;
> >> >> +             gpios = <&pio 7 18 0>; /* PH18 */
> >> >> +             gpio-names = "reset";
> >> >> +             rfkill-name = "bt";
> >> >> +             rfkill-type = <2>;
> >> >> +     };
> >> >
> >> > Hmmm, I don't think that's actually right.
> >> >
> >> > If you have such a device, then I'd expect it to be represented as a
> >> > full device in the DT, probably with one part for the WiFi, one part
> >> > for the Bluetooth, and here the definition of the rfkill device that
> >> > controls it.
> >>
> >> The AP6210 is not one device, but 2 separate chips in one module. Each
> >> chip has its own controls and interface. They just so happen to share
> >> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
> >> and interfaces. The WiFi side is most likely connected via SDIO, while
> >> the Bluetooth side is connected to a UART, and optionally I2S for sound.
> >
> > It's even easier to represent then.
> >
> >> > But tying parts of the device to the rfkill that controls it, such as
> >> > the clocks, or the frequency it runs at seems just wrong.
> >>
> >> I understand where you're coming from. For devices on buses that require
> >> drivers (such as USB, SDIO) these properties probably should be tied to
> >> the device node.
> >>
> >> For our use case here, which is a bluetooth chip connected on the UART,
> >> there is no in kernel representation or driver to tie them to. Same goes
> >> for UART based GPS chips. They just so happen to require toggling a GPIO,
> >> and maybe enabling a specific clock, to get it running. Afterwards,
> >> accessing it is done solely from userspace. For our Broadcom chips, the
> >> user has to upload its firmware first, then designate the tty as a Bluetooth
> >> HCI using hciattach.
> >>
> >> We are using the rfkill device as a on-off switch.
> >
> > I understand your point, but the fact that it's implemented in
> > user-space, or that UART is not a bus (which probably should be), is
> > only a Linux specific story, and how it's implemented in Linux (even
> > if the whole rfkill node is another one, but let's stay on topic).
> 
> I gave it some thought last night. You are right. My whole approach
> is wrong. But let's try to make it right.
> 
> So considering the fact that it's primarily connected to a UART,
> maybe I should make it a sub-node to the UART node it's actually
> connected to? Something like:
> 
>         uart2: serial@01c28800 {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&uart2_pins_a>;
>                 status = "okay";
> 
>                 bt: bt_hci {
>                         compatible = "brcm,bcm20710";
>                         /* maybe add some generic compatible */
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&clk_out_a_pins_a>,
> <&bt_pwr_pin_cubietruck>;
>                         clocks = <&clk_out_a>;
>                         clock-frequency = <32768>;
>                         gpios = <&pio 7 18 0>; /* PH18 */
>                 };
>         };
> 
> And let the uart core handle power sequencing for sub-nodes.
> 
> The rfkill node would still have the gpios and clocks, but not the
> clock-frequency property. It's sole purpose would be to toggle the
> controls. But I think the placement is still odd. Perhaps these
> virtual devices shouldn't live in the DT at all.

Yes, it looks much better.

I agree with you on the virtual devices things, and we have a lot of
other examples unfortunately.

However, since the rfkill nodes are in the DT, I think you'd still
have a rfkill node to handle the gpio, and a reference to the killed
device of some sort.

As far as the clock is concerned, I don't know if it makes sense to
have the BT clock in the RF kill node.

You probably want to use it to gate it whenever the device is killed,
but if the device is setting the frequency, it will more likely hold a
reference to that clock, so calling the disable in rfkill won't do
much.

Is rfkill sending any notification of some sort that the device is
being killed?

> > This is a huge abstraction leak.
> >
> > Let's say you need the I2S stream you mentionned for some
> > reason. Would you tie the audio stream to the rfkill node as well?
> > I'm sorry, but from an hardware description perspective, it makes no
> > sense.
> 
> The above revision should be better, from a hardware perspective. I'm
> not sure how to tie in the I2S stream, and there I haven't found any
> examples in the DT tree.

If it acts like an I2S "consumer", you can use ASoC's simple-card, and
you have a few examples in the other DTs.

Thanks!
Maxime
Maxime Ripard April 18, 2014, 5:49 p.m. UTC | #10
On Thu, Apr 17, 2014 at 09:43:40AM +0200, Arend van Spriel wrote:
> + linux-serial@vger.kernel.org
> 
> On 16/04/14 15:09, Hans de Goede wrote:
> >Hi,
> >
> >On 04/16/2014 12:39 PM, Chen-Yu Tsai wrote:
> >>Hi,
> >>
> >>On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
> >><maxime.ripard@free-electrons.com> wrote:
> >>>Hi,
> >>>
> >>>Please try to keep me in CC, even though the ML doesn't make it easy..
> >>
> >>Sorry about that.
> >>
> >>>On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
> >>>>>>@@ -139,4 +152,16 @@
> >>>>>>       reg_usb2_vbus: usb2-vbus {
> >>>>>>               status = "okay";
> >>>>>>       };
> >>>>>>+
> >>>>>>+     rfkill_bt {
> >>>>>>+             compatible = "rfkill-gpio";
> >>>>>>+             pinctrl-names = "default";
> >>>>>>+             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> >>>>>>+             clocks = <&clk_out_a>;
> >>>>>>+             clock-frequency = <32768>;
> >>>>>>+             gpios = <&pio 7 18 0>; /* PH18 */
> >>>>>>+             gpio-names = "reset";
> >>>>>>+             rfkill-name = "bt";
> >>>>>>+             rfkill-type = <2>;
> >>>>>>+     };
> >>>>>
> >>>>>Hmmm, I don't think that's actually right.
> >>>>>
> >>>>>If you have such a device, then I'd expect it to be represented as a
> >>>>>full device in the DT, probably with one part for the WiFi, one part
> >>>>>for the Bluetooth, and here the definition of the rfkill device that
> >>>>>controls it.
> >>>>
> >>>>The AP6210 is not one device, but 2 separate chips in one module. Each
> >>>>chip has its own controls and interface. They just so happen to share
> >>>>the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
> >>>>and interfaces. The WiFi side is most likely connected via SDIO, while
> >>>>the Bluetooth side is connected to a UART, and optionally I2S for sound.
> >>>
> >>>It's even easier to represent then.
> >>>
> >>>>>But tying parts of the device to the rfkill that controls it, such as
> >>>>>the clocks, or the frequency it runs at seems just wrong.
> >>>>
> >>>>I understand where you're coming from. For devices on buses that require
> >>>>drivers (such as USB, SDIO) these properties probably should be tied to
> >>>>the device node.
> >>>>
> >>>>For our use case here, which is a bluetooth chip connected on the UART,
> >>>>there is no in kernel representation or driver to tie them to. Same goes
> >>>>for UART based GPS chips. They just so happen to require toggling a GPIO,
> >>>>and maybe enabling a specific clock, to get it running. Afterwards,
> >>>>accessing it is done solely from userspace. For our Broadcom chips, the
> >>>>user has to upload its firmware first, then designate the tty as a Bluetooth
> >>>>HCI using hciattach.
> >>>>
> >>>>We are using the rfkill device as a on-off switch.
> >>>
> >>>I understand your point, but the fact that it's implemented in
> >>>user-space, or that UART is not a bus (which probably should be), is
> >>>only a Linux specific story, and how it's implemented in Linux (even
> >>>if the whole rfkill node is another one, but let's stay on topic).
> >>
> >>I gave it some thought last night. You are right. My whole approach
> >>is wrong. But let's try to make it right.
> >>
> >>So considering the fact that it's primarily connected to a UART,
> >>maybe I should make it a sub-node to the UART node it's actually
> >>connected to? Something like:
> >>
> >>         uart2: serial@01c28800 {
> >>                 pinctrl-names = "default";
> >>                 pinctrl-0 = <&uart2_pins_a>;
> >>                 status = "okay";
> >>
> >>                 bt: bt_hci {
> >>                         compatible = "brcm,bcm20710";
> >>                         /* maybe add some generic compatible */
> >>                         pinctrl-names = "default";
> >>                         pinctrl-0 = <&clk_out_a_pins_a>,
> >><&bt_pwr_pin_cubietruck>;
> >>                         clocks = <&clk_out_a>;
> >>                         clock-frequency = <32768>;
> >>                         gpios = <&pio 7 18 0>; /* PH18 */
> >>                 };
> >>         };
> >>
> >>And let the uart core handle power sequencing for sub-nodes.
> >
> >Great, I missed this reply when I typed my mail I send a few minutes
> >ago. I agree that this approach is how thing should be.
> 
> Regarding the device tree hierarchy this seems right, but powering
> the sub-nodes seems outside the realm of uart core.

Yet, a lot of devices are connected to an UART: GPS, BT chips, GSM
modems, even some odd PMICs, so UART acting like a real bus might make
sense.

Patch
diff mbox

diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index cb25d3c..767c8e1 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -61,6 +61,13 @@ 
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
+			bt_pwr_pin_cubietruck: bt_pwr_pin@0 {
+				allwinner,pins = "PH18";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
 		};
 
 		uart0: serial@01c28000 {
@@ -69,6 +76,12 @@ 
 			status = "okay";
 		};
 
+		uart2: serial@01c28800 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&uart2_pins_a>;
+			status = "okay";
+		};
+
 		i2c0: i2c@01c2ac00 {
 			pinctrl-names = "default";
 			pinctrl-0 = <&i2c0_pins_a>;
@@ -139,4 +152,16 @@ 
 	reg_usb2_vbus: usb2-vbus {
 		status = "okay";
 	};
+
+	rfkill_bt {
+		compatible = "rfkill-gpio";
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
+		clocks = <&clk_out_a>;
+		clock-frequency = <32768>;
+		gpios = <&pio 7 18 0>; /* PH18 */
+		gpio-names = "reset";
+		rfkill-name = "bt";
+		rfkill-type = <2>;
+	};
 };