Device-tree improvements

[lategoodbye]

Hi,
thanks for your explanations. I started with Wifi because it's much easier.

Does Tinker board and Tinker board S have a (maybe the same) wifi/bt module?
Can anyone test my patch with a recent mainline kernel (4.20/5.0)?

[summers]

Yes the Tinker Board and Tinker Board S are effectively the same, the S has eMMC on board on the sdio for eMMC.

Your patch is very similar to the ASUS patch. When I submitted that patch, mainline wasn't happy with it; but you've addressed many of the issues. Anyway mainline asked if it could be checked exactly what is needed - as the patch goes well beyond the sdio interface.

Oh yes, as TB and TBS both have the same wifi/bt chip, this patch should probably go in rk3288-tinker.dtsi.

TheSaint - are you interested in trying this out?

Have to wonder if I could use mmc-pwrseq to get power for the bluetooth as well ...

and should the pinctrl be acting on sdio or on sdio0 ?

[TheSaint]

I'd try out, within a couple of days.
@ lategoodbye
How can you fix a dtf without the interested piece of hardware ?

EDIT
$this->bbcode_second_pass_code('', '@AsusG75-debian_kernel> ls arch/arm/boot/dts/rk3288-tinker-s.dts
ls: cannot access 'arch/arm/boot/dts/rk3288-tinker-s.dts': No such file or directory')
So probably I'm not knowing which kernel I should get. I wonder how I got to move 3 Gb for a silly file of few Kb. If you patched you might have that file.
It's mostly I took the wrong path.
Anyway I'm trying to clone the kernel repository.
p$this->bbcode_second_pass_code('', 'atch -p1 <0001-ARM-dts-rk3288-tinker-s-Enable-wifi.patch
patching file arch/arm/boot/dts/rk3288-tinker-s.dts
Hunk #2 succeeded at 72 with fuzz 1 (offset 30 lines).
patch unexpectedly ends in middle of line
============================================
dtc -I dts -O dtb arch/arm/boot/dts/rk3288-tinker-s.dts -o /tmp/rk3288-tinker-s.dtb
Error: arch/arm/boot/dts/rk3288-tinker-s.dts:8.1-9 syntax error
FATAL ERROR: Unable to parse input tree')
I think I need a review

[summers]

Ah yes -regulators are how to do power. So following : https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/regulator/regulator.txt the bluetooth becomes
$this->bbcode_second_pass_code('', '&uart0 {
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
uart-has-rtscts;
bluetooth {
compatible = "realtek,rtl8723bs-bt";
reset-gpios = <&gpio4 29 GPIO_ACTIVE_HIGH>;
wake-gpios = <&gpio4 26 GPIO_ACTIVE_HIGH>;
wake-host-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>;
vcc-18-supply = <&vcc_18>;
vcc-io-supply = <&vcc_io>;
};
};')
Have to wonder why we can't specify power like that in the WiFi? The above assumes that the above 3.3 and 1.8 lines from the rk808 are wired to the wifi/bluetooth chip - but as taken from the asus patch makes sense.

Guess now have to work out how to pull pins, and guess how they are wired.

Also wonder why the pwrseq is needed ...

TheSaint - the patch will only apply against a 4.20 or later kernel - the tinker-s dts only appeared then. If you can't get it to work, and lategoodbye can't compile - I'll see if I get time this weekend ...

[lategoodbye]

@TheSaint

Sorry the patch got corrupted during copy paste. I think this should be fixed now in gist. Please test against a recent mainline kernel everything else isn't helpful for upstreaming. git provides the ability to make a shallow copy which should a bit faster (doesn't work for branches).

@summers

To be honest i mostly ignored your upstreaming efforts and took a different approach. Since the RK3288 is very popular i grep for a mainline board which uses the sdio interface (bingo RK3288 Firefly Reload) and used that as an template for the Tinker Board S. After that all i need to fixed were the GPIO, regulator stuff (Firefly uses a different PMIC) until the devicetree compiler was happy.

Adding the regulators like this doesn't have any benefit because the BT driver ignores them. Only the MMC core reads out the supported voltage from the regulators (vmmc-supply / vqmmc-supply). vbat_wl is only a fake regulator since i don't have schematics, but vmmc-supply doesn't need to be connect to the real PMIC for proper function.
https://elixir.bootlin.com/linux/latest ... etooth.txt
https://elixir.bootlin.com/linux/latest ... mc/mmc.txt

The power sequence is a standardized way to init the GPIOs and the clock (and has nothing to do with power supply):
https://elixir.bootlin.com/linux/latest ... simple.txt

[TheSaint]

$this->bbcode_second_pass_quote('summers', 'T')heSaint - the patch will only apply against a 4.20 or later kernel

[TheSaint]

$this->bbcode_second_pass_quote('lategoodbye', '@')TheSaint

Sorry the patch got corrupted during copy paste.

[summers]

@lategoodbye

Yes its the interdependencies that made me abandon my mainlining. Alas I don't understand well enough how the kernel and device tree work to tie it down. If you take my bluetooth dts, and expand it it becomes:
$this->bbcode_second_pass_code('', ' &uart0 {
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
uart-has-rtscts;
bluetooth {
compatible = "realtek,rtl8723bs-bt";
reset-gpios = <&gpio4 29 GPIO_ACTIVE_HIGH>;
wake-gpios = <&gpio4 26 GPIO_ACTIVE_HIGH>;
wake-host-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>;
vcc-18-supply = {
regulator-always-on;
regulator-boot-on;
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
regulator-name = "vcc_18";
regulator-state-mem {
regulator-on-in-suspend;
regulator-suspend-microvolt = <1800000>;
};
};
vcc-io-supply = <&vcc_io>;
};
};')
The question is how does the kernel handle the sub tree. Now for the uart this is known from: http://events17.linuxfoundation.org/sites/events/files/slides/serdev-elce-2017-2.pdf - serdev drivers (which we have in this case) know that this means start up a uart on the pins needed, but pass the data onto the driver required (in this case bluetooth).

Now the hci_h5 bluetooth driver that we are using doesn't know about either -gpios or -supply. But mainline would like these in the example code in documentation: https://www.spinics.net/lists/linux-bluetooth/msg78545.html.

So have to wonder, does the kernel know about the endings -gpios and -supply; and know when it sees these that it has to start the module responsible for that (so for power, talk to the rk808 over i2c).

Alas I don't know well enough how the kernel works.

@TheSaint

I don't think I can see how the wi-fi driver would mess up the 80211 module, I'd have though that the 80211 would sit higher in the steam, e.g. wi-fi gets the data and goes via 80211.

The pinout for the realtek device makes it sound like there are control pins separate for wi-fi and bluetooth. However this isn't really documented. So yes these pins quite possibly need pulling to bring the device up. This said though we don't know for sue how the pins are wired to the rk3288 cpu, its not in the schematic, so we have to hope we can get it from the asus miniarm device tree ... actually reading the two data sheets for the device, on has the line called BT_DIS# the other BT_RST_N. Now given the miniarm dts - my guess is that its the Reset. In which case bluetooth doesn't have an additional pin that needs pulling to enable the device.

[lategoodbye]

Bluetooth would be more complicated because we need 3 patches:
1) a new DT binding for the realtek chip - this should be easy because this chip looks like a copy of the Broadcom chip on the RPi with the same pins
2) extend a hci driver regarding the realtek chip - this would be the hardest part
3) add the devicetree node for the realtek chip - this is the part where we should start in order to know what could be tested with the tinker board

Here is my suggestion which is copy of the Broadcom binding (ignoring the hardflow control at this point):
$this->bbcode_second_pass_code('', '
&uart0 {
pinctrl-names = "default";
pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart_rts>;
status = "okay";

bluetooth {
compatible = "realtek,rtl8723bs-bt";
enable-gpios = <&gpio4 29 GPIO_ACTIVE_LOW>; // BT_DIS#
device-wakeup-gpios = <&gpio4 26 GPIO_ACTIVE_HIGH>; // BT_WAKE
host-wakeup-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>; // BT_HOST_WAKE
};
};
')

As i said regulator stuff isn't necessary and makes implemention more easier.

Edit: Sorry, i didn't noticed that hci_h5 already an ACPI binding, so we should keep the names.

[TheSaint]

Sorry it's getting too techy for me.... Whenever you need some testing I'll fully support it.