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

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

[David Summers]

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 {

[PATCH v3 3/3] arm: dts: rk3288-tinker.dtsi: Add outline Bluetooth support

[David Summers]

This patch is an outline for bluetooth on the ASUS Tinker Board (S).

Bluetooth is provided my RTL8723BS on these machine, and this is wired
into UART0.

Here this is performed as a SerDev device.

Now as presented, this does not give functioning bluetooth. The
updates needed to the device driver:

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

Haven't yet been accepted. So this device tree hook doesn't yet have
"realtek,rtl8723bs-bt" compatible flag.

It is however still submitted as a patch, as it sets the UART up
correctly. And so basic communciation can be had with the device. This
has been confirmed by Tony McKahan @ Armbian, in particular that the
CTS/RTS is needed.

When Vasily patch is accepted, this will be added as a minor patch on
top of this patch; as this patch gives the pins needed.

The one oddity, is the RESET pin, it is labeled this way on the
schematic. However the hci_h5.c driver calls this ENABLE. Here we have
stuck with the schematic name.

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 0ffab1b7c940..d29f5b34d7e6 100644
--- a/arch/arm/boot/dts/rk3288-tinker.dtsi
+++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
@@ -486,6 +486,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_HIGH>;
+		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

Re: [PATCH v3 3/3] arm: dts: rk3288-tinker.dtsi: Add outline Bluetooth support

[Marcel Holtmann]

Hi David,

> This patch is an outline for bluetooth on the ASUS Tinker Board (S).
> 
> Bluetooth is provided my RTL8723BS on these machine, and this is wired
> into UART0.
> 
> Here this is performed as a SerDev device.
> 
> Now as presented, this does not give functioning bluetooth. The
> updates needed to the device driver:
> 
> https://www.spinics.net/lists/linux-bluetooth/msg78661.html
> 
> Haven't yet been accepted. So this device tree hook doesn't yet have
> "realtek,rtl8723bs-bt" compatible flag.
> 
> It is however still submitted as a patch, as it sets the UART up
> correctly. And so basic communciation can be had with the device. This
> has been confirmed by Tony McKahan @ Armbian, in particular that the
> CTS/RTS is needed.
> 
> When Vasily patch is accepted, this will be added as a minor patch on
> top of this patch; as this patch gives the pins needed.
> 
> The one oddity, is the RESET pin, it is labeled this way on the
> schematic. However the hci_h5.c driver calls this ENABLE. Here we have
> stuck with the schematic name.
> 
> 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 0ffab1b7c940..d29f5b34d7e6 100644
> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
> @@ -486,6 +486,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_HIGH>;
> +		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>;
> +	};
> };

I need an ACK from Rob for this and also you might want to send the whole series for me to review in full context.

Regards

Marcel

Re: [PATCH v3 3/3] arm: dts: rk3288-tinker.dtsi: Add outline Bluetooth support

[David Summers]

Hi Marcel,

On 11/03/2019 16:41, Marcel Holtmann wrote:
> Hi David,
>
>> This patch is an outline for bluetooth on the ASUS Tinker Board (S).
>>
>> Bluetooth is provided my RTL8723BS on these machine, and this is wired
>> into UART0.
>>
>> Here this is performed as a SerDev device.
>>
>> Now as presented, this does not give functioning bluetooth. The
>> updates needed to the device driver:
>>
>> https://www.spinics.net/lists/linux-bluetooth/msg78661.html
>>
>> Haven't yet been accepted. So this device tree hook doesn't yet have
>> "realtek,rtl8723bs-bt" compatible flag.
>>
>> It is however still submitted as a patch, as it sets the UART up
>> correctly. And so basic communciation can be had with the device. This
>> has been confirmed by Tony McKahan @ Armbian, in particular that the
>> CTS/RTS is needed.
>>
>> When Vasily patch is accepted, this will be added as a minor patch on
>> top of this patch; as this patch gives the pins needed.
>>
>> The one oddity, is the RESET pin, it is labeled this way on the
>> schematic. However the hci_h5.c driver calls this ENABLE. Here we have
>> stuck with the schematic name.
>>
>> 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 0ffab1b7c940..d29f5b34d7e6 100644
>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
>> @@ -486,6 +486,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_HIGH>;
>> +		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>;
>> +	};
>> };
> I need an ACK from Rob for this and also you might want to send the whole series for me to review in full context.
>
> Regards
>
> Marcel
>
Yes happy to wait for ACK from Rob. Of the 3 patches done this is the 
least urgent, as Bluetooth is currently non functioning on these 
machines, so delay in this doesn't change the status quo. Patch 3 is 
self standing, e.g. not applying it doesn't affect the other two. 
However patch 3 probably has to be applied sequentially after patches 
1&2 as all are on the single file.

Following on from Robs comments on last patch sent adding bluetooth, 
I've made this one FAR more detailed. That said though the drivers for 
bluetooth in this case doesn't use clock or supply ASAIK. Most things 
are brought up by the wifi part of the device tree which was patch 2. 
However adding to the bluetooth device tree as well, at least leaves the 
option open to a user just loading bluetooth, and not wifi.

The one thing missing from this bluetooth device tree, is explaining 
which pins need to be pulled to bring up the card. This is part be 
design (the tinker board and rt8723bs both have bad public documentation 
- so exact operation is hard to guess ...); but also because I couldn't 
see a way of getting a SerDev device to pull pins on loading, unlike 
sdio (used for wifi).

Full series:

Part 1 - device tree changes for sd card
Part 2 - adding wifi to the device tree
Part 3 - adding outline bluetooth

So are each fairly self standing. However if interested in parts 1 and 2 
they can be found:

http://lists.infradead.org/pipermail/linux-rockchip/2019-March/023330.html
http://lists.infradead.org/pipermail/linux-rockchip/2019-March/023331.html

Regards, and yes happy to wait and see what Rob has to say.

David.

Re: [PATCH v3 3/3] arm: dts: rk3288-tinker.dtsi: Add outline Bluetooth support

[Heiko Stübner]

Hi David,

Am Samstag, 9. März 2019, 16:39:23 CET schrieb David Summers:
> This patch is an outline for bluetooth on the ASUS Tinker Board (S).
> 
> Bluetooth is provided my RTL8723BS on these machine, and this is wired
> into UART0.
> 
> Here this is performed as a SerDev device.
> 
> Now as presented, this does not give functioning bluetooth. The
> updates needed to the device driver:
> 
> https://www.spinics.net/lists/linux-bluetooth/msg78661.html
> 
> Haven't yet been accepted. So this device tree hook doesn't yet have
> "realtek,rtl8723bs-bt" compatible flag.
> 
> It is however still submitted as a patch, as it sets the UART up
> correctly. And so basic communciation can be had with the device. This
> has been confirmed by Tony McKahan @ Armbian, in particular that the
> CTS/RTS is needed.
> 
> When Vasily patch is accepted, this will be added as a minor patch on
> top of this patch; as this patch gives the pins needed.
> 
> The one oddity, is the RESET pin, it is labeled this way on the
> schematic. However the hci_h5.c driver calls this ENABLE. Here we have
> stuck with the schematic name.
> 
> 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 0ffab1b7c940..d29f5b34d7e6
> 100644
> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
> @@ -486,6 +486,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_HIGH>;
> +		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>;
> +	};

As this is supposed to work as serial attached device it should follow a
dt-binding, so first of all definitly needs a compatible and a binding
specifying all the properties declared above.

See for example Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
for a similar device.

Heiko

Re: [PATCH v3 3/3] arm: dts: rk3288-tinker.dtsi: Add outline Bluetooth support

[David Summers]

On 13/03/2019 10:56, Heiko Stübner wrote:
> Hi David,
>
> Am Samstag, 9. März 2019, 16:39:23 CET schrieb David Summers:
>> This patch is an outline for bluetooth on the ASUS Tinker Board (S).
>>
>> Bluetooth is provided my RTL8723BS on these machine, and this is wired
>> into UART0.
>>
>> Here this is performed as a SerDev device.
>>
>> Now as presented, this does not give functioning bluetooth. The
>> updates needed to the device driver:
>>
>> https://www.spinics.net/lists/linux-bluetooth/msg78661.html
>>
>> Haven't yet been accepted. So this device tree hook doesn't yet have
>> "realtek,rtl8723bs-bt" compatible flag.
>>
>> It is however still submitted as a patch, as it sets the UART up
>> correctly. And so basic communciation can be had with the device. This
>> has been confirmed by Tony McKahan @ Armbian, in particular that the
>> CTS/RTS is needed.
>>
>> When Vasily patch is accepted, this will be added as a minor patch on
>> top of this patch; as this patch gives the pins needed.
>>
>> The one oddity, is the RESET pin, it is labeled this way on the
>> schematic. However the hci_h5.c driver calls this ENABLE. Here we have
>> stuck with the schematic name.
>>
>> 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 0ffab1b7c940..d29f5b34d7e6
>> 100644
>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi
>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi
>> @@ -486,6 +486,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_HIGH>;
>> +		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>;
>> +	};
> As this is supposed to work as serial attached device it should follow a
> dt-binding, so first of all definitly needs a compatible and a binding
> specifying all the properties declared above.
>
> See for example Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> for a similar device.
>
> Heiko
>
>
Heiko,

Yes I understand. Means I'll need to take care not to loose these email, 
as getting all the pins right took some time ...

I'll probably follow 
https://www.spinics.net/lists/linux-bluetooth/msg78654.html if it gets 
accepted. That is the main problem here, at the moment there is no 
compatible that can be used, as none yet accepted.

Rob H, Question for you, before you said [1]:

> There's no interrupts, GPIO control lines, power supplies, etc. for
> these chips? The binding should be complete even if your platform
> doesn't need these.
Now its clear that so far the driver for bluetooth doesn't know about 
supply, and clocks etc.

So should these be included in the dts, as a correct description as how 
its wired; even if not used in the current driver, they may be used in 
future.

Am happy to do whatever best.

Regards,

David.

[1] https://www.spinics.net/lists/linux-bluetooth/msg78545.html