| Message ID | 20241111181807.13211-4-tszucs@linux.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: rock-3b TF + M2E updates | expand |
Hi Tamás, On 2024-11-11 19:17, Tamás Szűcs wrote: > Enable UART lines on Radxa ROCK 3 Model B M.2 Key E. > > Signed-off-by: Tamás Szűcs <tszucs@linux.com> > --- > arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts > index b7527ba418f7..61d4ba2d312a 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts > @@ -732,7 +732,7 @@ &uart8 { > pinctrl-names = "default"; > pinctrl-0 = <&uart8m0_xfer &uart8m0_ctsn &uart8m0_rtsn>; > uart-has-rtscts; > - status = "disabled"; > + status = "okay"; This should probably be enabled using an dt-overlay, there is no UART device embedded on the board and the reason I left it disabled in original board DT submission. On second thought maybe they should be enabled, think PCIe and USB lines on the M.2 Key E is already enabled by default. I probably only tested with a pcie/usb wifi/bt card and not a sido/uart wifi/bt card. Regards, Jonas > }; > > &usb_host0_ehci {
Hi Jonas, I agree; it's not possible to tell if the user will use a PCIe/USB, PCIe/UART, SDIO/UART, perhaps USB/UART device, or any other HIF combination. The way I see it is UART8 is hardwired to the M2E, so there is a reasonable expectation that it should work too if need be. Kind regards, Tamas Tamás Szűcs tszucs@linux.com On Mon, Nov 11, 2024 at 8:12 PM Jonas Karlman <jonas@kwiboo.se> wrote: > > Hi Tamás, > > On 2024-11-11 19:17, Tamás Szűcs wrote: > > Enable UART lines on Radxa ROCK 3 Model B M.2 Key E. > > > > Signed-off-by: Tamás Szűcs <tszucs@linux.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts > > index b7527ba418f7..61d4ba2d312a 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts > > @@ -732,7 +732,7 @@ &uart8 { > > pinctrl-names = "default"; > > pinctrl-0 = <&uart8m0_xfer &uart8m0_ctsn &uart8m0_rtsn>; > > uart-has-rtscts; > > - status = "disabled"; > > + status = "okay"; > > This should probably be enabled using an dt-overlay, there is no UART > device embedded on the board and the reason I left it disabled in > original board DT submission. > > On second thought maybe they should be enabled, think PCIe and USB lines > on the M.2 Key E is already enabled by default. I probably only tested > with a pcie/usb wifi/bt card and not a sido/uart wifi/bt card. > > Regards, > Jonas > > > }; > > > > &usb_host0_ehci { >
Hello Tamas, On 2024-11-12 15:35, Tamás Szűcs wrote: > I agree; it's not possible to tell if the user will use a PCIe/USB, > PCIe/UART, SDIO/UART, perhaps USB/UART device, or any other HIF > combination. The way I see it is UART8 is hardwired to the M2E, so > there is a reasonable expectation that it should work too if need be. Please correct me if I'm wrong, but isn't this UART supposed to be used for the Bluetooth part of an SDIO WiFi + Bluetooth module, in form of a non-standard M.2 module that Radxa sells? With that in mind, I see very little sense in just enabling the UART, without defining the entire Bluetooth interface, which AFAIK produces nasty looking error messages in the kernel log when there's actually nothing connected to the UART. As a side note, please use inline replying. [*] [*] https://en.wikipedia.org/wiki/Posting_style > On Mon, Nov 11, 2024 at 8:12 PM Jonas Karlman <jonas@kwiboo.se> wrote: >> >> Hi Tamás, >> >> On 2024-11-11 19:17, Tamás Szűcs wrote: >> > Enable UART lines on Radxa ROCK 3 Model B M.2 Key E. >> > >> > Signed-off-by: Tamás Szűcs <tszucs@linux.com> >> > --- >> > arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts >> > index b7527ba418f7..61d4ba2d312a 100644 >> > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts >> > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts >> > @@ -732,7 +732,7 @@ &uart8 { >> > pinctrl-names = "default"; >> > pinctrl-0 = <&uart8m0_xfer &uart8m0_ctsn &uart8m0_rtsn>; >> > uart-has-rtscts; >> > - status = "disabled"; >> > + status = "okay"; >> >> This should probably be enabled using an dt-overlay, there is no UART >> device embedded on the board and the reason I left it disabled in >> original board DT submission. >> >> On second thought maybe they should be enabled, think PCIe and USB >> lines >> on the M.2 Key E is already enabled by default. I probably only tested >> with a pcie/usb wifi/bt card and not a sido/uart wifi/bt card. >> >> > }; >> > >> > &usb_host0_ehci {
Hi Dragan, On Tue, Nov 12, 2024 at 4:07 PM Dragan Simic <dsimic@manjaro.org> wrote: > Please correct me if I'm wrong, but isn't this UART supposed to be > used for the Bluetooth part of an SDIO WiFi + Bluetooth module, in > form of a non-standard M.2 module that Radxa sells? UART8 is supposed to be used for any radio module connected to the M2E connector. It will typically be responsible for Bluetooth or BLE but it could be 802.15.4 or whatever. In any case, all wanting to use it will need the uart8 node enabled. > > With that in mind, I see very little sense in just enabling the UART, > without defining the entire Bluetooth interface, which AFAIK produces Defining a bluetooth node would hardwire idiosyncrasies of a given radio module's Bluetooth core. Sure you could add a sleep clock, all kind of sideband signals for wakeups, reset, power down, etc. But hey, some will use them, some won't. I think it's undesirable and unnecessary. You can hciattach from here and most will work just like that. Tighter integration or anything special, module specific on top should be handled individially, on a case-by-case basis. This is a dev board after all. I say trick of all trades. > nasty looking error messages in the kernel log when there's actually > nothing connected to the UART. My dmesg is clean as a whistle root@rock-3b:~# dmesg | grep -E 'fe6c0000|ttyS0' [ 0.344818] fe6c0000.serial: ttyS0 at MMIO 0xfe6c0000 (irq = 26, base_baud = 1500000) is a 16550A What kind of nasty errors do you recall? Kind regards, Tamas
Hi Tamás, On 2024-11-12 22:04, Tamás Szűcs wrote: > Hi Dragan, > > On Tue, Nov 12, 2024 at 4:07 PM Dragan Simic <dsimic@manjaro.org> wrote: >> Please correct me if I'm wrong, but isn't this UART supposed to be >> used for the Bluetooth part of an SDIO WiFi + Bluetooth module, in >> form of a non-standard M.2 module that Radxa sells? > > UART8 is supposed to be used for any radio module connected to the M2E > connector. > It will typically be responsible for Bluetooth or BLE but it could be > 802.15.4 or whatever. In any case, all wanting to use it will need the > uart8 node enabled. Do you have any specific sdio+uart module you are testing these changes with? The pinout for sdio+uart on Radxa's M.2 Key E slot is their own, pinout for pcie and usb should be closer to a common standard. https://dl.radxa.com/accessories/wireless-module/ROCKPi_M2_Wireless_Module_Pinout_v10.xlsx > >> >> With that in mind, I see very little sense in just enabling the UART, >> without defining the entire Bluetooth interface, which AFAIK produces > > Defining a bluetooth node would hardwire idiosyncrasies of a given > radio module's Bluetooth core. Sure you could add a sleep clock, all > kind of sideband signals for wakeups, reset, power down, etc. But hey, > some will use them, some won't. I think it's undesirable and > unnecessary. You can hciattach from here and most will work just like > that. Tighter integration or anything special, module specific on top > should be handled individially, on a case-by-case basis. This is a dev > board after all. I say trick of all trades. Changing to status=okay for sdmmc2 and uart8 should be fine, it does not cause any issue for my pcie wifi module testing with a Radxa A8 module. Testing with a Radxa A2 module (sdio+uart), the sdio/wifi part is automatically discovered, however bluetooth require a DT overlay for automatic probe. Something like this seem to work: diff --git a/rk3568-rock-3b-radxa-a2.dtso b/rk3568-rock-3b-radxa-a2.dtso new file mode 100644 index 000000000000..746b04e601af --- /dev/null +++ b/dts/rk3568-rock-3b-radxa-a2.dtso @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * DT-overlay for Radxa ROCK Pi Wireless Module A2. + */ + +/dts-v1/; +/plugin/; + +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/interrupt-controller/irq.h> +#include <dt-bindings/pinctrl/rockchip.h> + +&sdmmc2 { + #address-cells = <1>; + #size-cells = <0>; + status = "okay"; + + wifi@1 { + compatible = "brcm,bcm43456-fmac", "brcm,bcm4329-fmac"; + reg = <1>; + interrupt-parent = <&gpio0>; + interrupts = <RK_PD6 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "host-wake"; + pinctrl-names = "default"; + pinctrl-0 = <&wifi_wake_host_h>; + }; +}; + +&uart8 { + status = "okay"; + + bluetooth { + compatible = "brcm,bcm4345c5"; + clocks = <&rk809 1>; + clock-names = "lpo"; + interrupt-parent = <&gpio4>; + interrupts = <RK_PB5 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "host-wakeup"; + device-wakeup-gpios = <&gpio4 RK_PB4 GPIO_ACTIVE_HIGH>; + shutdown-gpios = <&gpio4 RK_PB2 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&bt_reg_on_h &bt_wake_host_h &host_wake_bt_h>; + vbat-supply = <&vcc3v3_sys2>; + vddio-supply = <&vcc_1v8>; + }; +}; With that applied wifi and bt module is detected and firmware loaded during startup: [ 4.684687] mmc_host mmc2: Bus speed (slot 0) = 150000000Hz (slot req 150000000Hz, actual 150000000HZ div = 0) [ 4.699412] dwmmc_rockchip fe000000.mmc: Successfully tuned phase to 360 [ 4.707429] mmc2: new ultra high speed SDR104 SDIO card at address 0001 [ 4.717034] brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43456-sdio for chip BCM4345/9 [ 4.760907] Bluetooth: hci0: BCM: chip id 130 [ 4.763736] Bluetooth: hci0: BCM: features 0x0f [ 4.787714] Bluetooth: hci0: BCM4345C5 [ 4.788482] Bluetooth: hci0: BCM4345C5 (003.006.006) build 0000 [ 11.417553] Bluetooth: hci0: BCM: features 0x0f [ 11.441621] Bluetooth: hci0: BCM4345C5 Ampak_CL1 UART 37.4 MHz BT 5.2 [Version: 1039.1086] [ 11.442423] Bluetooth: hci0: BCM4345C5 (003.006.006) build 1086 Regards, Jonas > >> nasty looking error messages in the kernel log when there's actually >> nothing connected to the UART. > > My dmesg is clean as a whistle > root@rock-3b:~# dmesg | grep -E 'fe6c0000|ttyS0' > [ 0.344818] fe6c0000.serial: ttyS0 at MMIO 0xfe6c0000 (irq = 26, > base_baud = 1500000) is a 16550A > What kind of nasty errors do you recall? > > Kind regards, > Tamas
Hello Tamas, On 2024-11-12 22:04, Tamás Szűcs wrote: > On Tue, Nov 12, 2024 at 4:07 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> Please correct me if I'm wrong, but isn't this UART supposed to be >> used for the Bluetooth part of an SDIO WiFi + Bluetooth module, in >> form of a non-standard M.2 module that Radxa sells? > > UART8 is supposed to be used for any radio module connected to the M2E > connector. > It will typically be responsible for Bluetooth or BLE but it could be > 802.15.4 or whatever. In any case, all wanting to use it will need the > uart8 node enabled. I see, but I'm still guessing what's the actual use of enabling the UART8 when it will remain pretty much useless without the additional DT configuration, such as in the WiFi+Bluetooth DT overlay that Jonas sent a bit earlier? I think that the UART8 should be enabled together with something that actually makes use of it, which in this case unfortunately cannot be automatically detected and configured, so it belongs to a DT overlay. I'll get back to this in my next response. >> With that in mind, I see very little sense in just enabling the UART, >> without defining the entire Bluetooth interface, which AFAIK produces > > Defining a bluetooth node would hardwire idiosyncrasies of a given > radio module's Bluetooth core. Sure you could add a sleep clock, all > kind of sideband signals for wakeups, reset, power down, etc. But hey, > some will use them, some won't. I think it's undesirable and > unnecessary. You can hciattach from here and most will work just like > that. Tighter integration or anything special, module specific on top > should be handled individially, on a case-by-case basis. This is a dev > board after all. I say trick of all trades. > >> nasty looking error messages in the kernel log when there's actually >> nothing connected to the UART. > > My dmesg is clean as a whistle > root@rock-3b:~# dmesg | grep -E 'fe6c0000|ttyS0' > [ 0.344818] fe6c0000.serial: ttyS0 at MMIO 0xfe6c0000 (irq = 26, > base_baud = 1500000) is a 16550A > What kind of nasty errors do you recall? Those would be the kernel error messages produced with the Bluetooth DT configuration in place, but with no SDIO module installed.
Hi Dragan, On Wed, Nov 13, 2024 at 12:25 AM Dragan Simic <dsimic@manjaro.org> wrote: > > Hello Tamas, > > On 2024-11-12 22:04, Tamás Szűcs wrote: > > On Tue, Nov 12, 2024 at 4:07 PM Dragan Simic <dsimic@manjaro.org> > > wrote: > >> Please correct me if I'm wrong, but isn't this UART supposed to be > >> used for the Bluetooth part of an SDIO WiFi + Bluetooth module, in > >> form of a non-standard M.2 module that Radxa sells? > > > > UART8 is supposed to be used for any radio module connected to the M2E > > connector. > > It will typically be responsible for Bluetooth or BLE but it could be > > 802.15.4 or whatever. In any case, all wanting to use it will need the > > uart8 node enabled. > > I see, but I'm still guessing what's the actual use of enabling the > UART8 when it will remain pretty much useless without the additional > DT configuration, such as in the WiFi+Bluetooth DT overlay that Jonas > sent a bit earlier? The actual use is device enablement. > > I think that the UART8 should be enabled together with something that > actually makes use of it, which in this case unfortunately cannot be > automatically detected and configured, so it belongs to a DT overlay. > I'll get back to this in my next response. I agree, bluetooth blocks dedicated to specific modules should belong to DT overlays. > > >> With that in mind, I see very little sense in just enabling the UART, > >> without defining the entire Bluetooth interface, which AFAIK produces > > > > Defining a bluetooth node would hardwire idiosyncrasies of a given > > radio module's Bluetooth core. Sure you could add a sleep clock, all > > kind of sideband signals for wakeups, reset, power down, etc. But hey, > > some will use them, some won't. I think it's undesirable and > > unnecessary. You can hciattach from here and most will work just like > > that. Tighter integration or anything special, module specific on top > > should be handled individially, on a case-by-case basis. This is a dev > > board after all. I say trick of all trades. > > > >> nasty looking error messages in the kernel log when there's actually > >> nothing connected to the UART. > > > > My dmesg is clean as a whistle > > root@rock-3b:~# dmesg | grep -E 'fe6c0000|ttyS0' > > [ 0.344818] fe6c0000.serial: ttyS0 at MMIO 0xfe6c0000 (irq = 26, > > base_baud = 1500000) is a 16550A > > What kind of nasty errors do you recall? > > Those would be the kernel error messages produced with the Bluetooth > DT configuration in place, but with no SDIO module installed. These are the kernel messages related to UART8 with the uart8 DT node enabled and an SDIO module installed.
Hello Tamas, On 2024-11-13 11:24, Tamás Szűcs wrote: > On Wed, Nov 13, 2024 at 12:25 AM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-11-12 22:04, Tamás Szűcs wrote: >> > On Tue, Nov 12, 2024 at 4:07 PM Dragan Simic <dsimic@manjaro.org> >> > wrote: >> >> Please correct me if I'm wrong, but isn't this UART supposed to be >> >> used for the Bluetooth part of an SDIO WiFi + Bluetooth module, in >> >> form of a non-standard M.2 module that Radxa sells? >> > >> > UART8 is supposed to be used for any radio module connected to the M2E >> > connector. >> > It will typically be responsible for Bluetooth or BLE but it could be >> > 802.15.4 or whatever. In any case, all wanting to use it will need the >> > uart8 node enabled. >> >> I see, but I'm still guessing what's the actual use of enabling the >> UART8 when it will remain pretty much useless without the additional >> DT configuration, such as in the WiFi+Bluetooth DT overlay that Jonas >> sent a bit earlier? > > The actual use is device enablement. Hmm, I'll need to think more about how it fits together. >> I think that the UART8 should be enabled together with something that >> actually makes use of it, which in this case unfortunately cannot be >> automatically detected and configured, so it belongs to a DT overlay. >> I'll get back to this in my next response. > > I agree, bluetooth blocks dedicated to specific modules should belong > to DT overlays. > >> >> With that in mind, I see very little sense in just enabling the UART, >> >> without defining the entire Bluetooth interface, which AFAIK produces >> > >> > Defining a bluetooth node would hardwire idiosyncrasies of a given >> > radio module's Bluetooth core. Sure you could add a sleep clock, all >> > kind of sideband signals for wakeups, reset, power down, etc. But hey, >> > some will use them, some won't. I think it's undesirable and >> > unnecessary. You can hciattach from here and most will work just like >> > that. Tighter integration or anything special, module specific on top >> > should be handled individially, on a case-by-case basis. This is a dev >> > board after all. I say trick of all trades. >> > >> >> nasty looking error messages in the kernel log when there's actually >> >> nothing connected to the UART. >> > >> > My dmesg is clean as a whistle >> > root@rock-3b:~# dmesg | grep -E 'fe6c0000|ttyS0' >> > [ 0.344818] fe6c0000.serial: ttyS0 at MMIO 0xfe6c0000 (irq = 26, >> > base_baud = 1500000) is a 16550A >> > What kind of nasty errors do you recall? >> >> Those would be the kernel error messages produced with the Bluetooth >> DT configuration in place, but with no SDIO module installed. > > These are the kernel messages related to UART8 with the uart8 DT node > enabled and an SDIO module installed. Out of curiosity, what M.2 module are you testing it with?
Hi Dragan, On Wed, Nov 13, 2024 at 11:38 AM Dragan Simic <dsimic@manjaro.org> wrote: > > Hello Tamas, > > On 2024-11-13 11:24, Tamás Szűcs wrote: > > On Wed, Nov 13, 2024 at 12:25 AM Dragan Simic <dsimic@manjaro.org> > > wrote: > >> On 2024-11-12 22:04, Tamás Szűcs wrote: > >> > On Tue, Nov 12, 2024 at 4:07 PM Dragan Simic <dsimic@manjaro.org> > >> > wrote: > >> >> Please correct me if I'm wrong, but isn't this UART supposed to be > >> >> used for the Bluetooth part of an SDIO WiFi + Bluetooth module, in > >> >> form of a non-standard M.2 module that Radxa sells? > >> > > >> > UART8 is supposed to be used for any radio module connected to the M2E > >> > connector. > >> > It will typically be responsible for Bluetooth or BLE but it could be > >> > 802.15.4 or whatever. In any case, all wanting to use it will need the > >> > uart8 node enabled. > >> > >> I see, but I'm still guessing what's the actual use of enabling the > >> UART8 when it will remain pretty much useless without the additional > >> DT configuration, such as in the WiFi+Bluetooth DT overlay that Jonas > >> sent a bit earlier? > > > > The actual use is device enablement. > > Hmm, I'll need to think more about how it fits together. > > >> I think that the UART8 should be enabled together with something that > >> actually makes use of it, which in this case unfortunately cannot be > >> automatically detected and configured, so it belongs to a DT overlay. > >> I'll get back to this in my next response. > > > > I agree, bluetooth blocks dedicated to specific modules should belong > > to DT overlays. > > > >> >> With that in mind, I see very little sense in just enabling the UART, > >> >> without defining the entire Bluetooth interface, which AFAIK produces > >> > > >> > Defining a bluetooth node would hardwire idiosyncrasies of a given > >> > radio module's Bluetooth core. Sure you could add a sleep clock, all > >> > kind of sideband signals for wakeups, reset, power down, etc. But hey, > >> > some will use them, some won't. I think it's undesirable and > >> > unnecessary. You can hciattach from here and most will work just like > >> > that. Tighter integration or anything special, module specific on top > >> > should be handled individially, on a case-by-case basis. This is a dev > >> > board after all. I say trick of all trades. > >> > > >> >> nasty looking error messages in the kernel log when there's actually > >> >> nothing connected to the UART. > >> > > >> > My dmesg is clean as a whistle > >> > root@rock-3b:~# dmesg | grep -E 'fe6c0000|ttyS0' > >> > [ 0.344818] fe6c0000.serial: ttyS0 at MMIO 0xfe6c0000 (irq = 26, > >> > base_baud = 1500000) is a 16550A > >> > What kind of nasty errors do you recall? > >> > >> Those would be the kernel error messages produced with the Bluetooth > >> DT configuration in place, but with no SDIO module installed. > > > > These are the kernel messages related to UART8 with the uart8 DT node > > enabled and an SDIO module installed. > > Out of curiosity, what M.2 module are you testing it with? https://www.u-blox.com/en/short-range-radio-modules#Host-based and some more you don't see here. Kind regards, Tamas
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts index b7527ba418f7..61d4ba2d312a 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts @@ -732,7 +732,7 @@ &uart8 { pinctrl-names = "default"; pinctrl-0 = <&uart8m0_xfer &uart8m0_ctsn &uart8m0_rtsn>; uart-has-rtscts; - status = "disabled"; + status = "okay"; }; &usb_host0_ehci {
Enable UART lines on Radxa ROCK 3 Model B M.2 Key E. Signed-off-by: Tamás Szűcs <tszucs@linux.com> --- arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)