diff mbox series

[3/3] ARM: dts: rk3288 Tinker Board (S) add bluetooth

Message ID 20190217121513.22965-4-beagleboard@davidjohnsummers.uk (mailing list archive)
State New, archived
Headers show
Series ARM: dts: rk3288 Tinker Board (S) updates | expand

Commit Message

David Summers Feb. 17, 2019, 12:15 p.m. UTC
This patch is to add Bluetooth to the ASUS Tinker Board (S) device
tree.

This patch is more contraversal - so probably view it as a request for
comments.

The reason behind this, is this patch does not currently set Bluetooth
working on the these boards. The problem is that the
bluetooth/hci_h5.c driver, that is used for Realtek serial devices,
depends on ACPI. Unfortantly ACPI can not be enabled on armv7 machines
such as the TB(S). And so the kernel module for the RTL8723BS
Bluetooth can not be loaded.

However this patch is believed to be an accurate description of the
Tinker Board (S) wiring. It is strongly based on the ASUS patch:

https://github.com/TinkerBoard/debian_kernel/commit/6a3128ade33f758887048578ada61a4b7ab8e678

As the Bluetooth is connected via a UART and this is wired into uart0,
this has been described as a SerDev.

The ASUS patch mentions the UART_CTS, but not the _RTS. Now as the
RTL8723BS has both CTS and RTS on the uart, it has been assumed both
are wired into uart0 on the cpu.

The Bluetooth connection has several wake up pins both device and host
and a reset pin. The wiring of these has been described; however how
the kernel driver will use these isn't clear. E.g. should HOST_WAKE be
an interupt?

The main reason for asking for comments, is that when it comes time to
make the changes to the bluetooth uart code, and add compatible flags,
robh has indicated that this will only be accepted with a valid
example. So the code below is that example. So I would like comments
that this approach is valid, which can be used when the later
submission comes. 

Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk>
---
 arch/arm/boot/dts/rk3288-tinker.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Tony McKahan Feb. 17, 2019, 1:31 p.m. UTC | #1
Hello Dave,

For the Bluetooth I can confirm _RTS is required, quite a lot of work
went into creating a userspace
solution for the Tinker Board bluetooth for Armbian by myself and by
Miouyouyou for a more
generic mainline patchset. Without RTS the rtk_hciattach handshake fails.

On Sun, Feb 17, 2019 at 7:17 AM David Summers
<beagleboard@davidjohnsummers.uk> wrote:
>
> This patch is to add Bluetooth to the ASUS Tinker Board (S) device
> tree.
>
> This patch is more contraversal - so probably view it as a request for
> comments.
>
> The reason behind this, is this patch does not currently set Bluetooth
> working on the these boards. The problem is that the
> bluetooth/hci_h5.c driver, that is used for Realtek serial devices,
> depends on ACPI. Unfortantly ACPI can not be enabled on armv7 machines
> such as the TB(S). And so the kernel module for the RTL8723BS
> Bluetooth can not be loaded.
>
> However this patch is believed to be an accurate description of the
> Tinker Board (S) wiring. It is strongly based on the ASUS patch:
>
> https://github.com/TinkerBoard/debian_kernel/commit/6a3128ade33f758887048578ada61a4b7ab8e678
>
> As the Bluetooth is connected via a UART and this is wired into uart0,
> this has been described as a SerDev.
>
> The ASUS patch mentions the UART_CTS, but not the _RTS. Now as the
> RTL8723BS has both CTS and RTS on the uart, it has been assumed both
> are wired into uart0 on the cpu.
>
> The Bluetooth connection has several wake up pins both device and host
> and a reset pin. The wiring of these has been described; however how
> the kernel driver will use these isn't clear. E.g. should HOST_WAKE be
> an interupt?
>
> The main reason for asking for comments, is that when it comes time to
> make the changes to the bluetooth uart code, and add compatible flags,
> robh has indicated that this will only be accepted with a valid
> example. So the code below is that example. So I would like comments
> that this approach is valid, which can be used when the later
> submission comes.
>
> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk>
> ---
>  arch/arm/boot/dts/rk3288-tinker.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi
> index e1796f340eef..931b4c652fdc 100644
> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
> @@ -481,6 +481,17 @@
>
>  &uart0 {
>         status = "okay";
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
> +
> +       bluetooth {
> +               clocks = <&rk808 RK808_CLKOUT1>;
> +               reset-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_LOW>;
> +               device-wake-gpios = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
> +               host-wake-gpios = <&gpio4 RK_PD7 GPIO_ACTIVE_HIGH>;
> +               vcc-18-supply = <&vcc_18>;
> +               vcc-io-supply = <&vcc_io>;
> +       };
>  };
>
>  &uart1 {
> --
> beagleboard@davidjohnsummers.uk
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Stefan Wahren Feb. 17, 2019, 9:05 p.m. UTC | #2
Hi David,

> David Summers <beagleboard@davidjohnsummers.uk> hat am 17. Februar 2019 um 13:15 geschrieben:
> 
> 
> This patch is to add Bluetooth to the ASUS Tinker Board (S) device
> tree.
> 
> This patch is more contraversal - so probably view it as a request for
> comments.
> 
> The reason behind this, is this patch does not currently set Bluetooth
> working on the these boards. The problem is that the
> bluetooth/hci_h5.c driver, that is used for Realtek serial devices,
> depends on ACPI. Unfortantly ACPI can not be enabled on armv7 machines
> such as the TB(S). And so the kernel module for the RTL8723BS
> Bluetooth can not be loaded.
> 
> However this patch is believed to be an accurate description of the
> Tinker Board (S) wiring. It is strongly based on the ASUS patch:
> 
> https://github.com/TinkerBoard/debian_kernel/commit/6a3128ade33f758887048578ada61a4b7ab8e678
> 
> As the Bluetooth is connected via a UART and this is wired into uart0,
> this has been described as a SerDev.
> 
> The ASUS patch mentions the UART_CTS, but not the _RTS. Now as the
> RTL8723BS has both CTS and RTS on the uart, it has been assumed both
> are wired into uart0 on the cpu.

which would be very strange, because we could use H4 instead of H5.

> 
> The Bluetooth connection has several wake up pins both device and host
> and a reset pin. The wiring of these has been described; however how
> the kernel driver will use these isn't clear. E.g. should HOST_WAKE be
> an interupt?
> 
> The main reason for asking for comments, is that when it comes time to
> make the changes to the bluetooth uart code, and add compatible flags,
> robh has indicated that this will only be accepted with a valid
> example. So the code below is that example. So I would like comments
> that this approach is valid, which can be used when the later
> submission comes. 

Asking for comment should be mark with RFC in the patch subject.

> 
> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk>
> ---
>  arch/arm/boot/dts/rk3288-tinker.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi
> index e1796f340eef..931b4c652fdc 100644
> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
> @@ -481,6 +481,17 @@
>  
>  &uart0 {
>  	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
> +
> +	bluetooth {
> +		clocks = <&rk808 RK808_CLKOUT1>;

As we already linked the RK808 clock in the sdio pwrseq, we shouldn't do this here again.

> +		reset-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_LOW>;

reset-gpios isn't compatible to hci_h5, please use my preliminary dt-binding

> +		device-wake-gpios = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
> +		host-wake-gpios = <&gpio4 RK_PD7 GPIO_ACTIVE_HIGH>;

AFAIK hci_h5 doesn't support host-wake yet. So if you want to keep for the future we should make a comment.

> +		vcc-18-supply = <&vcc_18>;
> +		vcc-io-supply = <&vcc_io>;

Please drop these as the driver doesn't use it and it's already linked in the sdio pwrseq.

Thanks Stefan

> +	};

[1] - https://gist.github.com/lategoodbye/79bac99d4f1158a719a48ea3c45eb7f1#file-0002-dt-bindings-add-support-for-realtek-bluetooth-serial-patch
David Summers Feb. 18, 2019, 8:47 p.m. UTC | #3
On 17/02/2019 21:05, Stefan Wahren wrote:
> Hi David,
>
>> David Summers <beagleboard@davidjohnsummers.uk> hat am 17. Februar 2019 um 13:15 geschrieben:
>>
>>
>> This patch is to add Bluetooth to the ASUS Tinker Board (S) device
>> tree.
>>
>> This patch is more contraversal - so probably view it as a request for
>> comments.
>>
>> The reason behind this, is this patch does not currently set Bluetooth
>> working on the these boards. The problem is that the
>> bluetooth/hci_h5.c driver, that is used for Realtek serial devices,
>> depends on ACPI. Unfortantly ACPI can not be enabled on armv7 machines
>> such as the TB(S). And so the kernel module for the RTL8723BS
>> Bluetooth can not be loaded.
>>
>> However this patch is believed to be an accurate description of the
>> Tinker Board (S) wiring. It is strongly based on the ASUS patch:
>>
>> https://github.com/TinkerBoard/debian_kernel/commit/6a3128ade33f758887048578ada61a4b7ab8e678
>>
>> As the Bluetooth is connected via a UART and this is wired into uart0,
>> this has been described as a SerDev.
>>
>> The ASUS patch mentions the UART_CTS, but not the _RTS. Now as the
>> RTL8723BS has both CTS and RTS on the uart, it has been assumed both
>> are wired into uart0 on the cpu.
> which would be very strange, because we could use H4 instead of H5.

Well the bluetooth maintainer said it was hci_h5.

The device is BT4.0 and that under Volume 4 HCI, Part D - 2 wire UART 
Transport Layer, in section 11.2 says that Hardware flow control may be 
used.

Also if you check what ASUS did:

https://github.com/TinkerBoard/debian_kernel/commit/5106d9486cf1c2bad4f701611f51a526191c56ad

They added H5 code to the hci_h4.c, seems strange but thats what they 
said they did.

Now maybe the RTL8723BS could work with the UART Transport Layer, but 
that hasn't been done yet. 3 wire has some overhead, but it doesn't look 
like much.

To my mind though it down to the bluetooth Maintainer - so CCing Marcel 
Holtmann in ...


>> The Bluetooth connection has several wake up pins both device and host
>> and a reset pin. The wiring of these has been described; however how
>> the kernel driver will use these isn't clear. E.g. should HOST_WAKE be
>> an interupt?
>>
>> The main reason for asking for comments, is that when it comes time to
>> make the changes to the bluetooth uart code, and add compatible flags,
>> robh has indicated that this will only be accepted with a valid
>> example. So the code below is that example. So I would like comments
>> that this approach is valid, which can be used when the later
>> submission comes.
> Asking for comment should be mark with RFC in the patch subject.
OOps my mistake.
>> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk>
>> ---
>>   arch/arm/boot/dts/rk3288-tinker.dtsi | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi
>> index e1796f340eef..931b4c652fdc 100644
>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
>> @@ -481,6 +481,17 @@
>>   
>>   &uart0 {
>>   	status = "okay";
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
>> +
>> +	bluetooth {
>> +		clocks = <&rk808 RK808_CLKOUT1>;
> As we already linked the RK808 clock in the sdio pwrseq, we shouldn't do this here again.
>
>> +		reset-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_LOW>;
> reset-gpios isn't compatible to hci_h5, please use my preliminary dt-binding
>
>> +		device-wake-gpios = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
>> +		host-wake-gpios = <&gpio4 RK_PD7 GPIO_ACTIVE_HIGH>;
> AFAIK hci_h5 doesn't support host-wake yet. So if you want to keep for the future we should make a comment.
>
>> +		vcc-18-supply = <&vcc_18>;
>> +		vcc-io-supply = <&vcc_io>;
> Please drop these as the driver doesn't use it and it's already linked in the sdio pwrseq.
>
> Thanks Stefan
>
>> +	};
> [1] - https://gist.github.com/lategoodbye/79bac99d4f1158a719a48ea3c45eb7f1#file-0002-dt-bindings-add-support-for-realtek-bluetooth-serial-patch

Sorry I disagree here.

Last time I posted an example patch like this robh said clearly that 
these should give clocks, power, etc:

https://www.spinics.net/lists/linux-bluetooth/msg78545.html
https://www.spinics.net/lists/linux-bluetooth/msg78585.html 
<https://www.spinics.net/lists/linux-bluetooth/msg78585.html>

and as he is maintainer he wins.

I can understand Robs position, device tree are for describing hardware, 
they aren't primarily for helping code to load. Yes that is a second 
effect, its so it knows what is in  the hardware. In this case what if 
someone wanted Bluetooth, but not WiFi - then the Bluetooth should know 
what power and clocks are needed. Now that the driver doesn't understand 
these things is secondary. Maybe that will be cured in future?
Stefan Wahren Feb. 18, 2019, 9:40 p.m. UTC | #4
Hi David,

> David Summers <beagleboard@davidjohnsummers.uk> hat am 18. Februar 2019 um 21:47 geschrieben:
> 
> 
> On 17/02/2019 21:05, Stefan Wahren wrote:
> > Hi David,
> >
> >> David Summers <beagleboard@davidjohnsummers.uk> hat am 17. Februar 2019 um 13:15 geschrieben:
> >>
> >> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
> >> @@ -481,6 +481,17 @@
> >>   
> >>   &uart0 {
> >>   	status = "okay";
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
> >> +
> >> +	bluetooth {
> >> +		clocks = <&rk808 RK808_CLKOUT1>;
> > As we already linked the RK808 clock in the sdio pwrseq, we shouldn't do this here again.
> >
> >> +		reset-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_LOW>;
> > reset-gpios isn't compatible to hci_h5, please use my preliminary dt-binding
> >
> >> +		device-wake-gpios = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
> >> +		host-wake-gpios = <&gpio4 RK_PD7 GPIO_ACTIVE_HIGH>;
> > AFAIK hci_h5 doesn't support host-wake yet. So if you want to keep for the future we should make a comment.
> >
> >> +		vcc-18-supply = <&vcc_18>;
> >> +		vcc-io-supply = <&vcc_io>;
> > Please drop these as the driver doesn't use it and it's already linked in the sdio pwrseq.
> >
> > Thanks Stefan
> >
> >> +	};
> > [1] - https://gist.github.com/lategoodbye/79bac99d4f1158a719a48ea3c45eb7f1#file-0002-dt-bindings-add-support-for-realtek-bluetooth-serial-patch
> 
> Sorry I disagree here.
> 
> Last time I posted an example patch like this robh said clearly that 
> these should give clocks, power, etc:
> 
> https://www.spinics.net/lists/linux-bluetooth/msg78545.html
> https://www.spinics.net/lists/linux-bluetooth/msg78585.html 
> <https://www.spinics.net/lists/linux-bluetooth/msg78585.html>
> 
> and as he is maintainer he wins.
> 
> I can understand Robs position, device tree are for describing hardware, 
> they aren't primarily for helping code to load. Yes that is a second 
> effect, its so it knows what is in  the hardware. In this case what if 
> someone wanted Bluetooth, but not WiFi - then the Bluetooth should know 
> what power and clocks are needed. Now that the driver doesn't understand 
> these things is secondary. Maybe that will be cured in future?
>

you need to consider to context of the discussion. The link pointed to a discussion about the dt-binding. A binding should describe all parts (even optional) of the hardware and that is the reason why i explained in Arch board that we need 3 parts for bluetooth (dt-binding, driver change and finally the dts change). Since you avoid that and send only the last part without compatibles causes confusion.

We usually don't change DTS properties because it's considered as an ABI, which means we should do it properly in the first run. So lets make the step one (test bluetooth) and not step four ...

Regards
Stefan
David Summers March 3, 2019, 8:15 p.m. UTC | #5
Finally on the patch to give an outline description to Bluetooth on the 
Tinker Boards.

Synthesis here is harder.

On the one hand, it is non functioning - because although it describes 
the hardware, it has no compatible flag, and there is no current driver 
in the kernel.

It should be said though there is a patch for a proposed driver:

https://www.spinics.net/lists/linux-bluetooth/msg78661.html

And if accepted this would give the compatible flag. So should this 
change wait until that is added?

An alternative approach though arises from what Tony McKahan of Armbian 
writes

"For the Bluetooth I can confirm _RTS is required"

So this device tree segment is useful - it correctly describes the uart 
portion of the interface, and that is hard to determine from the ASUS 
dts. So this is useful information to capture.

On the pinout information of the rtl8723. Two points:

1) information like clock, and voltage supplies, are formally hardware 
correct, but practically will probably never be applied via bluetooth. 
Instead its the wifi that brings this hardware up. So should these be 
supplied if unlikly to be used? I think they should as they formally 
describe the hardware.

2) Confusion over enable-gpios vs reset-gpios

The second appeared on the bluetooth thread:

https://www.spinics.net/lists/linux-bluetooth/msg78654.html

should this be written:

enable-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_LOW>;

or

reset-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_HIGH>;

The datasheet (as available as it is): 
http://cit.odessa.ua/media/pdf/Intel-Compute-Stick/FN-Link_F23BDSM25-W1.pdf

Says it is BT_RST - BT Reset IN

but the current kernel driver hci_h5 says:

https://github.com/torvalds/linux/blob/master/drivers/bluetooth/hci_h5.c#L989

     { "enable-gpios", &btrtl_enable_gpios, 1 },

And RobH clearly has a view:

https://www.spinics.net/lists/linux-bluetooth/msg79138.html

So there is still some confusion.

Guess its a question of it a partial hardware description, with possible 
faults, is better than no device tree description of the hardware?

Any views?



On 17/02/2019 12:15, David Summers wrote:
> This patch is to add Bluetooth to the ASUS Tinker Board (S) device
> tree.
>
> This patch is more contraversal - so probably view it as a request for
> comments.
>
> The reason behind this, is this patch does not currently set Bluetooth
> working on the these boards. The problem is that the
> bluetooth/hci_h5.c driver, that is used for Realtek serial devices,
> depends on ACPI. Unfortantly ACPI can not be enabled on armv7 machines
> such as the TB(S). And so the kernel module for the RTL8723BS
> Bluetooth can not be loaded.
>
> However this patch is believed to be an accurate description of the
> Tinker Board (S) wiring. It is strongly based on the ASUS patch:
>
> https://github.com/TinkerBoard/debian_kernel/commit/6a3128ade33f758887048578ada61a4b7ab8e678
>
> As the Bluetooth is connected via a UART and this is wired into uart0,
> this has been described as a SerDev.
>
> The ASUS patch mentions the UART_CTS, but not the _RTS. Now as the
> RTL8723BS has both CTS and RTS on the uart, it has been assumed both
> are wired into uart0 on the cpu.
>
> The Bluetooth connection has several wake up pins both device and host
> and a reset pin. The wiring of these has been described; however how
> the kernel driver will use these isn't clear. E.g. should HOST_WAKE be
> an interupt?
>
> The main reason for asking for comments, is that when it comes time to
> make the changes to the bluetooth uart code, and add compatible flags,
> robh has indicated that this will only be accepted with a valid
> example. So the code below is that example. So I would like comments
> that this approach is valid, which can be used when the later
> submission comes.
>
> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk>
> ---
>   arch/arm/boot/dts/rk3288-tinker.dtsi | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi
> index e1796f340eef..931b4c652fdc 100644
> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
> @@ -481,6 +481,17 @@
>   
>   &uart0 {
>   	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
> +
> +	bluetooth {
> +		clocks = <&rk808 RK808_CLKOUT1>;
> +		reset-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_LOW>;
> +		device-wake-gpios = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
> +		host-wake-gpios = <&gpio4 RK_PD7 GPIO_ACTIVE_HIGH>;
> +		vcc-18-supply = <&vcc_18>;
> +		vcc-io-supply = <&vcc_io>;
> +	};
>   };
>   
>   &uart1 {
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi
index e1796f340eef..931b4c652fdc 100644
--- a/arch/arm/boot/dts/rk3288-tinker.dtsi
+++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
@@ -481,6 +481,17 @@ 
 
 &uart0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
+
+	bluetooth {
+		clocks = <&rk808 RK808_CLKOUT1>;
+		reset-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_LOW>;
+		device-wake-gpios = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
+		host-wake-gpios = <&gpio4 RK_PD7 GPIO_ACTIVE_HIGH>;
+		vcc-18-supply = <&vcc_18>;
+		vcc-io-supply = <&vcc_io>;
+	};
 };
 
 &uart1 {