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

[David Summers <beagleboard@davidjohnsummers.uk>]

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?


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