[PATCH 0/3] arm64: dts: rockchip: rock-3b TF + M2E updates

[PATCH 0/3] arm64: dts: rockchip: rock-3b TF + M2E updates

[Tamás Szűcs]

Hi,

Changes have been tested on an RK3568J, with an emphasis on DDR50 and SDR104 at 200 MHz.

Kind regards,
Tamas

Tamás Szűcs (3):
  arm64: dts: rockchip: Add supported UHS-I rates to sdmmc0 on rock-3b
  arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO
    devices
  arm64: dts: rockchip: Enable UART8 on rock-3b

 arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH 1/3] arm64: dts: rockchip: Add supported UHS-I rates to sdmmc0 on rock-3b

[Tamás Szűcs]

Add all supported UHS-I rates to sdmmc0 and allow 200 MHz maximum clock to
benefit modern SD cards.

Signed-off-by: Tamás Szűcs <tszucs@linux.com>
---
 arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
index 3d0c1ccfaa79..242af5337cdf 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
@@ -670,8 +670,14 @@ &sdmmc0 {
 	bus-width = <4>;
 	cap-sd-highspeed;
 	disable-wp;
+	max-frequency = <200000000>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
+	sd-uhs-sdr12;
+	sd-uhs-sdr25;
+	sd-uhs-sdr50;
+	sd-uhs-sdr104;
+	sd-uhs-ddr50;
 	vmmc-supply = <&vcc3v3_sd>;
 	vqmmc-supply = <&vccio_sd>;
 	status = "okay";
-- 
2.45.2

[PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Tamás Szűcs]

Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I rates and
enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.

Signed-off-by: Tamás Szűcs <tszucs@linux.com>
---
 arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
 1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
@@ -688,14 +688,20 @@ &sdmmc2 {
 	cap-sd-highspeed;
 	cap-sdio-irq;
 	keep-power-in-suspend;
+	max-frequency = <200000000>;
 	mmc-pwrseq = <&sdio_pwrseq>;
 	non-removable;
 	pinctrl-names = "default";
 	pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
+	sd-uhs-sdr12;
+	sd-uhs-sdr25;
+	sd-uhs-sdr50;
 	sd-uhs-sdr104;
+	sd-uhs-ddr50;
 	vmmc-supply = <&vcc3v3_sys2>;
 	vqmmc-supply = <&vcc_1v8>;
-	status = "disabled";
+	wakeup-source;
+	status = "okay";
 };
 
 &sfc {
-- 
2.45.2

[PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Tamás Szűcs]

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";
 };
 
 &usb_host0_ehci {
-- 
2.45.2

Re: [PATCH 1/3] arm64: dts: rockchip: Add supported UHS-I rates to sdmmc0 on rock-3b

[Jonas Karlman]

Hi Tamás,

On 2024-11-11 19:17, Tamás Szűcs wrote:
> Add all supported UHS-I rates to sdmmc0 and allow 200 MHz maximum clock to
> benefit modern SD cards.
> 
> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> index 3d0c1ccfaa79..242af5337cdf 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> @@ -670,8 +670,14 @@ &sdmmc0 {
>  	bus-width = <4>;
>  	cap-sd-highspeed;
>  	disable-wp;
> +	max-frequency = <200000000>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
> +	sd-uhs-sdr12;
> +	sd-uhs-sdr25;
> +	sd-uhs-sdr50;
> +	sd-uhs-sdr104;
> +	sd-uhs-ddr50;

There is an issue with io-domain driver not always being probed before
mmc driver, this typically result in io-domain being configured wrong,
and mmc tuning happen before io-domain is correctly configured.

You can usually observe this by looking at the tuning value during boot
and comparing it to the tuning value after removing and re-insering a
sd-card.

Because of this uhs modes was left out from initial DT submission, some
cards will work others wont, sd-uhs-sdr50 is known to be working with
most cards even with the probe order issue.

Also I thought that lower speeds where implied?

Regards,
Jonas

>  	vmmc-supply = <&vcc3v3_sd>;
>  	vqmmc-supply = <&vccio_sd>;
>  	status = "okay";

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Jonas Karlman]

Hi Tamás,

On 2024-11-11 19:17, Tamás Szűcs wrote:
> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I rates and
> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
> 
> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> @@ -688,14 +688,20 @@ &sdmmc2 {
>  	cap-sd-highspeed;
>  	cap-sdio-irq;
>  	keep-power-in-suspend;
> +	max-frequency = <200000000>;
>  	mmc-pwrseq = <&sdio_pwrseq>;
>  	non-removable;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
> +	sd-uhs-sdr12;
> +	sd-uhs-sdr25;
> +	sd-uhs-sdr50;

I thought that lower speeds was implied by uhs-sdr104?

>  	sd-uhs-sdr104;
> +	sd-uhs-ddr50;
>  	vmmc-supply = <&vcc3v3_sys2>;
>  	vqmmc-supply = <&vcc_1v8>;
> -	status = "disabled";
> +	wakeup-source;
> +	status = "okay";

This should probably be enabled using an dt-overlay, there is no SDIO
device embedded on the board and the reason I left it disabled in
original board DT submission.

Regards,
Jonas

>  };
>  
>  &sfc {

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Jonas Karlman]

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 {

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Dragan Simic]

Hello Jonas and Tamas,

On 2024-11-11 20:06, Jonas Karlman wrote:
> On 2024-11-11 19:17, Tamás Szűcs wrote:
>> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I 
>> rates and
>> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
>> 
>> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
>>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> @@ -688,14 +688,20 @@ &sdmmc2 {
>>  	cap-sd-highspeed;
>>  	cap-sdio-irq;
>>  	keep-power-in-suspend;
>> +	max-frequency = <200000000>;
>>  	mmc-pwrseq = <&sdio_pwrseq>;
>>  	non-removable;
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
>> +	sd-uhs-sdr12;
>> +	sd-uhs-sdr25;
>> +	sd-uhs-sdr50;
> 
> I thought that lower speeds was implied by uhs-sdr104?

Last time I went through the MMC drivers, they were implied.  IIRC,
such backward mode compatibility is actually a requirement made by
the MMC specification.

>>  	sd-uhs-sdr104;
>> +	sd-uhs-ddr50;
>>  	vmmc-supply = <&vcc3v3_sys2>;
>>  	vqmmc-supply = <&vcc_1v8>;
>> -	status = "disabled";
>> +	wakeup-source;
>> +	status = "okay";
> 
> This should probably be enabled using an dt-overlay, there is no
> SDIO device embedded on the board and the reason I left it disabled
> in original board DT submission.

Just went through the ROCK 3B schematic, version 1.51, and I think
there should be no need for a separate overlay, because sdmmc2 goes
to the M.2 slot on the board, which any user can plug an M.2 module
into, and the SDIO interface is kind-of self-discoverable.

Of course, all that unless there are some horribly looking :) error
messages emitted to the kernel log when nothing is actually found,
in which case the SDIO/MMC driers should be fixed first.  Also, I'm
not sure what do we do with the possible SDIO-related power timing
requirements?

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Tamás Szűcs]

Hi Jonas, Dragan,

I think it was totally fine to disable sdmmc2 at first, especially if
it couldn’t be tested or wasn’t needed right away. From what I’ve
seen, this board works great even at higher clock speeds than what
rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
and there don’t seem to be any limits mentioned in the TRM either.
Overall, this board is doing just fine as it is.

Regarding device tree overlays, they would be ideal for implementing
secondary functions, such as PCIe endpoint mode for users with
specific requirements. However, the primary functions for PCIe on the
M2E will be root complex mode, along with SDIO host, etc. In my view,
the hardware is well-designed and interconnected. Users have a
reasonable expectation that these primary functions should work
seamlessly without additional configuration, right out of the box.

Dragan, what did you mean by SDIO related power timing requirements?

Kind regards,
Tamas



Tamás Szűcs
tszucs@linux.com

On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Jonas and Tamas,
>
> On 2024-11-11 20:06, Jonas Karlman wrote:
> > On 2024-11-11 19:17, Tamás Szűcs wrote:
> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
> >> rates and
> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
> >>
> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> >> ---
> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> @@ -688,14 +688,20 @@ &sdmmc2 {
> >>      cap-sd-highspeed;
> >>      cap-sdio-irq;
> >>      keep-power-in-suspend;
> >> +    max-frequency = <200000000>;
> >>      mmc-pwrseq = <&sdio_pwrseq>;
> >>      non-removable;
> >>      pinctrl-names = "default";
> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
> >> +    sd-uhs-sdr12;
> >> +    sd-uhs-sdr25;
> >> +    sd-uhs-sdr50;
> >
> > I thought that lower speeds was implied by uhs-sdr104?
>
> Last time I went through the MMC drivers, they were implied.  IIRC,
> such backward mode compatibility is actually a requirement made by
> the MMC specification.
>
> >>      sd-uhs-sdr104;
> >> +    sd-uhs-ddr50;
> >>      vmmc-supply = <&vcc3v3_sys2>;
> >>      vqmmc-supply = <&vcc_1v8>;
> >> -    status = "disabled";
> >> +    wakeup-source;
> >> +    status = "okay";
> >
> > This should probably be enabled using an dt-overlay, there is no
> > SDIO device embedded on the board and the reason I left it disabled
> > in original board DT submission.
>
> Just went through the ROCK 3B schematic, version 1.51, and I think
> there should be no need for a separate overlay, because sdmmc2 goes
> to the M.2 slot on the board, which any user can plug an M.2 module
> into, and the SDIO interface is kind-of self-discoverable.
>
> Of course, all that unless there are some horribly looking :) error
> messages emitted to the kernel log when nothing is actually found,
> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
> not sure what do we do with the possible SDIO-related power timing
> requirements?

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Tamás Szűcs]

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 {
>

Re: [PATCH 1/3] arm64: dts: rockchip: Add supported UHS-I rates to sdmmc0 on rock-3b

[Tamás Szűcs]

Hi Jonas,

Thank you for pointing this out! I haven't noticed this before. I've
done some testing and I believe I am able to reproduce the issue you
described, although I cannot confirm the reason.
The only occasion I encounter any problems is when a UHS SD card or
SDIO device is connected to sdmmc0 during bootup. Sometimes the device
is recognized as HS only. Obviously no tuning value reported. Also,
sdmmc2 cuts out completely. I'm booting from eMMC and when the SD card
is removed in this state I lose my rootfs. Certainly, this needs more
attention but it seems to be unrelated to the changes here.

I need more time to check but are you sure this SD card during bootup
issue is gone with UHS-I disabled?

Also, in every other case, when you connect any device to sdmmc0 after
bootup, performance and stability is perfect.
Interestingly I also don't experience this behavior with an eMMC
device and / or an SDIO device connected to sdmmc2 during bootup. Only
sdmmc0 is problematic and only during bootup.

Any more thoughts on this are very welcome.

Kind regards,
Tamas



Tamás Szűcs
tszucs@linux.com

On Mon, Nov 11, 2024 at 8:00 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Tamás,
>
> On 2024-11-11 19:17, Tamás Szűcs wrote:
> > Add all supported UHS-I rates to sdmmc0 and allow 200 MHz maximum clock to
> > benefit modern SD cards.
> >
> > Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> > index 3d0c1ccfaa79..242af5337cdf 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> > @@ -670,8 +670,14 @@ &sdmmc0 {
> >       bus-width = <4>;
> >       cap-sd-highspeed;
> >       disable-wp;
> > +     max-frequency = <200000000>;
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
> > +     sd-uhs-sdr12;
> > +     sd-uhs-sdr25;
> > +     sd-uhs-sdr50;
> > +     sd-uhs-sdr104;
> > +     sd-uhs-ddr50;
>
> There is an issue with io-domain driver not always being probed before
> mmc driver, this typically result in io-domain being configured wrong,
> and mmc tuning happen before io-domain is correctly configured.
>
> You can usually observe this by looking at the tuning value during boot
> and comparing it to the tuning value after removing and re-insering a
> sd-card.
>
> Because of this uhs modes was left out from initial DT submission, some
> cards will work others wont, sd-uhs-sdr50 is known to be working with
> most cards even with the probe order issue.
>
> Also I thought that lower speeds where implied?
>
> Regards,
> Jonas
>
> >       vmmc-supply = <&vcc3v3_sd>;
> >       vqmmc-supply = <&vccio_sd>;
> >       status = "okay";
>

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Dragan Simic]

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 {

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Dragan Simic]

Hello Tamas,

On 2024-11-12 15:35, Tamás Szűcs wrote:
> I think it was totally fine to disable sdmmc2 at first, especially if
> it couldn’t be tested or wasn’t needed right away. From what I’ve
> seen, this board works great even at higher clock speeds than what
> rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
> and there don’t seem to be any limits mentioned in the TRM either.
> Overall, this board is doing just fine as it is.

Sorry, I'm missing the point of mentioning some clock speeds?  Any
chances, please, to clarify that a bit?

> Regarding device tree overlays, they would be ideal for implementing
> secondary functions, such as PCIe endpoint mode for users with
> specific requirements. However, the primary functions for PCIe on the
> M2E will be root complex mode, along with SDIO host, etc. In my view,
> the hardware is well-designed and interconnected. Users have a
> reasonable expectation that these primary functions should work
> seamlessly without additional configuration, right out of the box.

That's basically what I referred to in my earlier response, and in my
previous response regarding the UART.  Users would expect the Bluetooth
part to work as well, but the error messages I mentioned look nasty, so
perhaps something should be done about that first.

> Dragan, what did you mean by SDIO related power timing requirements?

Whenever there's an SDIO module, there's usually some required timing
of the power rails.  Though, I don't know what's that like with the
non-standard M.2 SDIO modules that Radxa sells, which are intended to
be used on Radxa boards with "hybrid" M.2 slots.

Once again, please use inline replying. [*]

[*] https://en.wikipedia.org/wiki/Posting_style

> On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> 
>> Hello Jonas and Tamas,
>> 
>> On 2024-11-11 20:06, Jonas Karlman wrote:
>> > On 2024-11-11 19:17, Tamás Szűcs wrote:
>> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
>> >> rates and
>> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
>> >>
>> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
>> >> ---
>> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
>> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
>> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> @@ -688,14 +688,20 @@ &sdmmc2 {
>> >>      cap-sd-highspeed;
>> >>      cap-sdio-irq;
>> >>      keep-power-in-suspend;
>> >> +    max-frequency = <200000000>;
>> >>      mmc-pwrseq = <&sdio_pwrseq>;
>> >>      non-removable;
>> >>      pinctrl-names = "default";
>> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
>> >> +    sd-uhs-sdr12;
>> >> +    sd-uhs-sdr25;
>> >> +    sd-uhs-sdr50;
>> >
>> > I thought that lower speeds was implied by uhs-sdr104?
>> 
>> Last time I went through the MMC drivers, they were implied.  IIRC,
>> such backward mode compatibility is actually a requirement made by
>> the MMC specification.
>> 
>> >>      sd-uhs-sdr104;
>> >> +    sd-uhs-ddr50;
>> >>      vmmc-supply = <&vcc3v3_sys2>;
>> >>      vqmmc-supply = <&vcc_1v8>;
>> >> -    status = "disabled";
>> >> +    wakeup-source;
>> >> +    status = "okay";
>> >
>> > This should probably be enabled using an dt-overlay, there is no
>> > SDIO device embedded on the board and the reason I left it disabled
>> > in original board DT submission.
>> 
>> Just went through the ROCK 3B schematic, version 1.51, and I think
>> there should be no need for a separate overlay, because sdmmc2 goes
>> to the M.2 slot on the board, which any user can plug an M.2 module
>> into, and the SDIO interface is kind-of self-discoverable.
>> 
>> Of course, all that unless there are some horribly looking :) error
>> messages emitted to the kernel log when nothing is actually found,
>> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
>> not sure what do we do with the possible SDIO-related power timing
>> requirements?

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Tamás Szűcs]

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

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Tamás Szűcs]

Hi Dragan,

On Tue, Nov 12, 2024 at 4:16 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Tamas,
>
> On 2024-11-12 15:35, Tamás Szűcs wrote:
> > I think it was totally fine to disable sdmmc2 at first, especially if
> > it couldn’t be tested or wasn’t needed right away. From what I’ve
> > seen, this board works great even at higher clock speeds than what
> > rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
> > and there don’t seem to be any limits mentioned in the TRM either.
> > Overall, this board is doing just fine as it is.
>
> Sorry, I'm missing the point of mentioning some clock speeds?  Any
> chances, please, to clarify that a bit?

It's all about stress scenarios, right. Sustained transfer at maximum
clock, multiple SD/MMC blocks used concurrently. That kind of thing.
Different data rates forced. I hope that answers your question.

>
> > Regarding device tree overlays, they would be ideal for implementing
> > secondary functions, such as PCIe endpoint mode for users with
> > specific requirements. However, the primary functions for PCIe on the
> > M2E will be root complex mode, along with SDIO host, etc. In my view,
> > the hardware is well-designed and interconnected. Users have a
> > reasonable expectation that these primary functions should work
> > seamlessly without additional configuration, right out of the box.
>
> That's basically what I referred to in my earlier response, and in my
> previous response regarding the UART.  Users would expect the Bluetooth
> part to work as well, but the error messages I mentioned look nasty, so
> perhaps something should be done about that first.

I'm not aware of any nasty error messages especially related to UART.
Well, MMC core will acknowledge when the platform part fails to
enumerate a device on sdmmc2, but there's nothing wrong with this.
It's not even an error -- certainly not a nasty one.

[    1.799703] mmc_host mmc2: card is non-removable.
[    1.935011] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
400000Hz, actual 375000HZ div = 0)
[    7.195009] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
375000Hz, actual 375000HZ div = 0)
[   13.029540] mmc2: Failed to initialize a non-removable card

>
> > Dragan, what did you mean by SDIO related power timing requirements?
>
> Whenever there's an SDIO module, there's usually some required timing
> of the power rails.  Though, I don't know what's that like with the
> non-standard M.2 SDIO modules that Radxa sells, which are intended to
> be used on Radxa boards with "hybrid" M.2 slots.

Ok, I see. Not always. I can't comment on Radxa's SDIO module but I'm
sure it's reasonably standard. And so is the M.2 Key E on this board.
Actually, part of the appeal is that all standard buses are very
nicely wired up. I want everybody to be able to use them.


>
> Once again, please use inline replying. [*]
>
> [*] https://en.wikipedia.org/wiki/Posting_style
>
> > On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >>
> >> Hello Jonas and Tamas,
> >>
> >> On 2024-11-11 20:06, Jonas Karlman wrote:
> >> > On 2024-11-11 19:17, Tamás Szűcs wrote:
> >> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
> >> >> rates and
> >> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
> >> >>
> >> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> >> >> ---
> >> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
> >> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
> >> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> >> @@ -688,14 +688,20 @@ &sdmmc2 {
> >> >>      cap-sd-highspeed;
> >> >>      cap-sdio-irq;
> >> >>      keep-power-in-suspend;
> >> >> +    max-frequency = <200000000>;
> >> >>      mmc-pwrseq = <&sdio_pwrseq>;
> >> >>      non-removable;
> >> >>      pinctrl-names = "default";
> >> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
> >> >> +    sd-uhs-sdr12;
> >> >> +    sd-uhs-sdr25;
> >> >> +    sd-uhs-sdr50;
> >> >
> >> > I thought that lower speeds was implied by uhs-sdr104?
> >>
> >> Last time I went through the MMC drivers, they were implied.  IIRC,
> >> such backward mode compatibility is actually a requirement made by
> >> the MMC specification.
> >>
> >> >>      sd-uhs-sdr104;
> >> >> +    sd-uhs-ddr50;
> >> >>      vmmc-supply = <&vcc3v3_sys2>;
> >> >>      vqmmc-supply = <&vcc_1v8>;
> >> >> -    status = "disabled";
> >> >> +    wakeup-source;
> >> >> +    status = "okay";
> >> >
> >> > This should probably be enabled using an dt-overlay, there is no
> >> > SDIO device embedded on the board and the reason I left it disabled
> >> > in original board DT submission.
> >>
> >> Just went through the ROCK 3B schematic, version 1.51, and I think
> >> there should be no need for a separate overlay, because sdmmc2 goes
> >> to the M.2 slot on the board, which any user can plug an M.2 module
> >> into, and the SDIO interface is kind-of self-discoverable.
> >>
> >> Of course, all that unless there are some horribly looking :) error
> >> messages emitted to the kernel log when nothing is actually found,
> >> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
> >> not sure what do we do with the possible SDIO-related power timing
> >> requirements?

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Jonas Karlman]

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

Re: [PATCH 1/3] arm64: dts: rockchip: Add supported UHS-I rates to sdmmc0 on rock-3b

[Jonas Karlman]

Hi Tamás,

On 2024-11-12 15:36, Tamás Szűcs wrote:
> Hi Jonas,
> 
> Thank you for pointing this out! I haven't noticed this before. I've
> done some testing and I believe I am able to reproduce the issue you
> described, although I cannot confirm the reason.
> The only occasion I encounter any problems is when a UHS SD card or
> SDIO device is connected to sdmmc0 during bootup. Sometimes the device
> is recognized as HS only. Obviously no tuning value reported. Also,
> sdmmc2 cuts out completely. I'm booting from eMMC and when the SD card
> is removed in this state I lose my rootfs. Certainly, this needs more
> attention but it seems to be unrelated to the changes here.
> 
> I need more time to check but are you sure this SD card during bootup
> issue is gone with UHS-I disabled?

Yes, the issue is that the io voltage domain must be configured to match
the io signal voltage used, and to use uhs the voltage changes from 3v3
to 1v8. Causing a miss-match between io voltage domain config and the
regulator voltage used during initial probe, unless io-domain driver
happens to be fully loaded before mmc devices are probed.

> 
> Also, in every other case, when you connect any device to sdmmc0 after
> bootup, performance and stability is perfect.
> Interestingly I also don't experience this behavior with an eMMC
> device and / or an SDIO device connected to sdmmc2 during bootup. Only
> sdmmc0 is problematic and only during bootup.

Yes, as you have discovered, inserting the sd-card after system has
booted and io-domain driver has been loaded, everything can work as
expected with uhs speeds.

Until this probe race condition has been solved booting with a sd-card
inserted may or may not result in wrong tuning or other related issues.

Because of this I advice not to enable uhs mode for sdmmc0 at this time.

Regards,
Jonas

> 
> Any more thoughts on this are very welcome.
> 
> Kind regards,
> Tamas
> 
> 
> 
> Tamás Szűcs
> tszucs@linux.com
> 
> On Mon, Nov 11, 2024 at 8:00 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> Hi Tamás,
>>
>> On 2024-11-11 19:17, Tamás Szűcs wrote:
>>> Add all supported UHS-I rates to sdmmc0 and allow 200 MHz maximum clock to
>>> benefit modern SD cards.
>>>
>>> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>>> index 3d0c1ccfaa79..242af5337cdf 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>>> @@ -670,8 +670,14 @@ &sdmmc0 {
>>>       bus-width = <4>;
>>>       cap-sd-highspeed;
>>>       disable-wp;
>>> +     max-frequency = <200000000>;
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
>>> +     sd-uhs-sdr12;
>>> +     sd-uhs-sdr25;
>>> +     sd-uhs-sdr50;
>>> +     sd-uhs-sdr104;
>>> +     sd-uhs-ddr50;
>>
>> There is an issue with io-domain driver not always being probed before
>> mmc driver, this typically result in io-domain being configured wrong,
>> and mmc tuning happen before io-domain is correctly configured.
>>
>> You can usually observe this by looking at the tuning value during boot
>> and comparing it to the tuning value after removing and re-insering a
>> sd-card.
>>
>> Because of this uhs modes was left out from initial DT submission, some
>> cards will work others wont, sd-uhs-sdr50 is known to be working with
>> most cards even with the probe order issue.
>>
>> Also I thought that lower speeds where implied?
>>
>> Regards,
>> Jonas
>>
>>>       vmmc-supply = <&vcc3v3_sd>;
>>>       vqmmc-supply = <&vccio_sd>;
>>>       status = "okay";
>>

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Dragan Simic]

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.

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Dragan Simic]

Hello Tamas,

On 2024-11-12 22:05, Tamás Szűcs wrote:
> On Tue, Nov 12, 2024 at 4:16 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-11-12 15:35, Tamás Szűcs wrote:
>> > I think it was totally fine to disable sdmmc2 at first, especially if
>> > it couldn’t be tested or wasn’t needed right away. From what I’ve
>> > seen, this board works great even at higher clock speeds than what
>> > rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
>> > and there don’t seem to be any limits mentioned in the TRM either.
>> > Overall, this board is doing just fine as it is.
>> 
>> Sorry, I'm missing the point of mentioning some clock speeds?  Any
>> chances, please, to clarify that a bit?
> 
> It's all about stress scenarios, right. Sustained transfer at maximum
> clock, multiple SD/MMC blocks used concurrently. That kind of thing.
> Different data rates forced. I hope that answers your question.

Ah, I see.  Though, I don't think we should worry much about that,
although testing it all is, of course, a good thing to do.

>> > Regarding device tree overlays, they would be ideal for implementing
>> > secondary functions, such as PCIe endpoint mode for users with
>> > specific requirements. However, the primary functions for PCIe on the
>> > M2E will be root complex mode, along with SDIO host, etc. In my view,
>> > the hardware is well-designed and interconnected. Users have a
>> > reasonable expectation that these primary functions should work
>> > seamlessly without additional configuration, right out of the box.
>> 
>> That's basically what I referred to in my earlier response, and in my
>> previous response regarding the UART.  Users would expect the 
>> Bluetooth
>> part to work as well, but the error messages I mentioned look nasty, 
>> so
>> perhaps something should be done about that first.
> 
> I'm not aware of any nasty error messages especially related to UART.
> Well, MMC core will acknowledge when the platform part fails to
> enumerate a device on sdmmc2, but there's nothing wrong with this.
> It's not even an error -- certainly not a nasty one.
> 
> [    1.799703] mmc_host mmc2: card is non-removable.
> [    1.935011] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
> 400000Hz, actual 375000HZ div = 0)
> [    7.195009] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
> 375000Hz, actual 375000HZ div = 0)
> [   13.029540] mmc2: Failed to initialize a non-removable card

This looks acceptable to me, but I'm now not really sure that we
should enable the sdmmc2 in the board dts.  Let me explain.

As Jonas demonstrated with the WiFi+Bluetooth DT overlay, additional
DT configuration is needed to actually make an SDIO M.2 module plugged
into the ROCK 3B's M.2 slot work, so what do we actually get from
enabling the sdmmc2 in the board dts?  Just some error messages in
the kernel log :) and AFAICT no additional functionality when an SDIO
M.2 module is actually plugged into the slot.

>> > Dragan, what did you mean by SDIO related power timing requirements?
>> 
>> Whenever there's an SDIO module, there's usually some required timing
>> of the power rails.  Though, I don't know what's that like with the
>> non-standard M.2 SDIO modules that Radxa sells, which are intended to
>> be used on Radxa boards with "hybrid" M.2 slots.
> 
> Ok, I see. Not always. I can't comment on Radxa's SDIO module but I'm
> sure it's reasonably standard. And so is the M.2 Key E on this board.
> Actually, part of the appeal is that all standard buses are very
> nicely wired up. I want everybody to be able to use them.

Yes, but getting it all wired in some way unfortunately doesn't mean
that it will all work without additional DT configuration in place,
as described above.

Also, I'm not really sure there's some strict standard for the "GPIO"
and "UART" M.2 modules, that part of the specification was/is a bit
blurry or perhaps even non-existent.  It's been a while since I wrote
the M.2 aricle on English Wikipedia, :) maybe it's become defined
better in the meantime.

>> Once again, please use inline replying. [*]
>> 
>> [*] https://en.wikipedia.org/wiki/Posting_style
>> 
>> > On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org>
>> > wrote:
>> >>
>> >> Hello Jonas and Tamas,
>> >>
>> >> On 2024-11-11 20:06, Jonas Karlman wrote:
>> >> > On 2024-11-11 19:17, Tamás Szűcs wrote:
>> >> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
>> >> >> rates and
>> >> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
>> >> >>
>> >> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
>> >> >> ---
>> >> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
>> >> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
>> >> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> >> @@ -688,14 +688,20 @@ &sdmmc2 {
>> >> >>      cap-sd-highspeed;
>> >> >>      cap-sdio-irq;
>> >> >>      keep-power-in-suspend;
>> >> >> +    max-frequency = <200000000>;
>> >> >>      mmc-pwrseq = <&sdio_pwrseq>;
>> >> >>      non-removable;
>> >> >>      pinctrl-names = "default";
>> >> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
>> >> >> +    sd-uhs-sdr12;
>> >> >> +    sd-uhs-sdr25;
>> >> >> +    sd-uhs-sdr50;
>> >> >
>> >> > I thought that lower speeds was implied by uhs-sdr104?
>> >>
>> >> Last time I went through the MMC drivers, they were implied.  IIRC,
>> >> such backward mode compatibility is actually a requirement made by
>> >> the MMC specification.
>> >>
>> >> >>      sd-uhs-sdr104;
>> >> >> +    sd-uhs-ddr50;
>> >> >>      vmmc-supply = <&vcc3v3_sys2>;
>> >> >>      vqmmc-supply = <&vcc_1v8>;
>> >> >> -    status = "disabled";
>> >> >> +    wakeup-source;
>> >> >> +    status = "okay";
>> >> >
>> >> > This should probably be enabled using an dt-overlay, there is no
>> >> > SDIO device embedded on the board and the reason I left it disabled
>> >> > in original board DT submission.
>> >>
>> >> Just went through the ROCK 3B schematic, version 1.51, and I think
>> >> there should be no need for a separate overlay, because sdmmc2 goes
>> >> to the M.2 slot on the board, which any user can plug an M.2 module
>> >> into, and the SDIO interface is kind-of self-discoverable.
>> >>
>> >> Of course, all that unless there are some horribly looking :) error
>> >> messages emitted to the kernel log when nothing is actually found,
>> >> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
>> >> not sure what do we do with the possible SDIO-related power timing
>> >> requirements?

Re: [PATCH 1/3] arm64: dts: rockchip: Add supported UHS-I rates to sdmmc0 on rock-3b

[Tamás Szűcs]

Hi Jonas,

On Tue, Nov 12, 2024 at 11:37 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Tamás,
>
> On 2024-11-12 15:36, Tamás Szűcs wrote:
> > Hi Jonas,
> >
> > Thank you for pointing this out! I haven't noticed this before. I've
> > done some testing and I believe I am able to reproduce the issue you
> > described, although I cannot confirm the reason.
> > The only occasion I encounter any problems is when a UHS SD card or
> > SDIO device is connected to sdmmc0 during bootup. Sometimes the device
> > is recognized as HS only. Obviously no tuning value reported. Also,
> > sdmmc2 cuts out completely. I'm booting from eMMC and when the SD card
> > is removed in this state I lose my rootfs. Certainly, this needs more
> > attention but it seems to be unrelated to the changes here.
> >
> > I need more time to check but are you sure this SD card during bootup
> > issue is gone with UHS-I disabled?
>
> Yes, the issue is that the io voltage domain must be configured to match
> the io signal voltage used, and to use uhs the voltage changes from 3v3
> to 1v8. Causing a miss-match between io voltage domain config and the
> regulator voltage used during initial probe, unless io-domain driver
> happens to be fully loaded before mmc devices are probed.
>
> >
> > Also, in every other case, when you connect any device to sdmmc0 after
> > bootup, performance and stability is perfect.
> > Interestingly I also don't experience this behavior with an eMMC
> > device and / or an SDIO device connected to sdmmc2 during bootup. Only
> > sdmmc0 is problematic and only during bootup.
>
> Yes, as you have discovered, inserting the sd-card after system has
> booted and io-domain driver has been loaded, everything can work as
> expected with uhs speeds.
>
> Until this probe race condition has been solved booting with a sd-card
> inserted may or may not result in wrong tuning or other related issues.
>
> Because of this I advice not to enable uhs mode for sdmmc0 at this time.

All right, and thank you for the explanation. My hands are full at the
moment but let me think about this.

>
> Regards,
> Jonas
>
> >
> > Any more thoughts on this are very welcome.
> >
> > Kind regards,
> > Tamas
> >
> >
> >
> > Tamás Szűcs
> > tszucs@linux.com
> >
> > On Mon, Nov 11, 2024 at 8:00 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> >>
> >> Hi Tamás,
> >>
> >> On 2024-11-11 19:17, Tamás Szűcs wrote:
> >>> Add all supported UHS-I rates to sdmmc0 and allow 200 MHz maximum clock to
> >>> benefit modern SD cards.
> >>>
> >>> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> >>> ---
> >>>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >>> index 3d0c1ccfaa79..242af5337cdf 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >>> @@ -670,8 +670,14 @@ &sdmmc0 {
> >>>       bus-width = <4>;
> >>>       cap-sd-highspeed;
> >>>       disable-wp;
> >>> +     max-frequency = <200000000>;
> >>>       pinctrl-names = "default";
> >>>       pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
> >>> +     sd-uhs-sdr12;
> >>> +     sd-uhs-sdr25;
> >>> +     sd-uhs-sdr50;
> >>> +     sd-uhs-sdr104;
> >>> +     sd-uhs-ddr50;
> >>
> >> There is an issue with io-domain driver not always being probed before
> >> mmc driver, this typically result in io-domain being configured wrong,
> >> and mmc tuning happen before io-domain is correctly configured.
> >>
> >> You can usually observe this by looking at the tuning value during boot
> >> and comparing it to the tuning value after removing and re-insering a
> >> sd-card.
> >>
> >> Because of this uhs modes was left out from initial DT submission, some
> >> cards will work others wont, sd-uhs-sdr50 is known to be working with
> >> most cards even with the probe order issue.
> >>
> >> Also I thought that lower speeds where implied?
> >>
> >> Regards,
> >> Jonas
> >>
> >>>       vmmc-supply = <&vcc3v3_sd>;
> >>>       vqmmc-supply = <&vccio_sd>;
> >>>       status = "okay";
> >>
>

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Tamás Szűcs]

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.

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Tamás Szűcs]

Hi Dragan,

On Wed, Nov 13, 2024 at 12:38 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Tamas,
>
> On 2024-11-12 22:05, Tamás Szűcs wrote:
> > On Tue, Nov 12, 2024 at 4:16 PM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >> On 2024-11-12 15:35, Tamás Szűcs wrote:
> >> > I think it was totally fine to disable sdmmc2 at first, especially if
> >> > it couldn’t be tested or wasn’t needed right away. From what I’ve
> >> > seen, this board works great even at higher clock speeds than what
> >> > rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
> >> > and there don’t seem to be any limits mentioned in the TRM either.
> >> > Overall, this board is doing just fine as it is.
> >>
> >> Sorry, I'm missing the point of mentioning some clock speeds?  Any
> >> chances, please, to clarify that a bit?
> >
> > It's all about stress scenarios, right. Sustained transfer at maximum
> > clock, multiple SD/MMC blocks used concurrently. That kind of thing.
> > Different data rates forced. I hope that answers your question.
>
> Ah, I see.  Though, I don't think we should worry much about that,
> although testing it all is, of course, a good thing to do.

Better be safe than sorry. Let's just say I've seen worse.

>
> >> > Regarding device tree overlays, they would be ideal for implementing
> >> > secondary functions, such as PCIe endpoint mode for users with
> >> > specific requirements. However, the primary functions for PCIe on the
> >> > M2E will be root complex mode, along with SDIO host, etc. In my view,
> >> > the hardware is well-designed and interconnected. Users have a
> >> > reasonable expectation that these primary functions should work
> >> > seamlessly without additional configuration, right out of the box.
> >>
> >> That's basically what I referred to in my earlier response, and in my
> >> previous response regarding the UART.  Users would expect the
> >> Bluetooth
> >> part to work as well, but the error messages I mentioned look nasty,
> >> so
> >> perhaps something should be done about that first.
> >
> > I'm not aware of any nasty error messages especially related to UART.
> > Well, MMC core will acknowledge when the platform part fails to
> > enumerate a device on sdmmc2, but there's nothing wrong with this.
> > It's not even an error -- certainly not a nasty one.
> >
> > [    1.799703] mmc_host mmc2: card is non-removable.
> > [    1.935011] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
> > 400000Hz, actual 375000HZ div = 0)
> > [    7.195009] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
> > 375000Hz, actual 375000HZ div = 0)
> > [   13.029540] mmc2: Failed to initialize a non-removable card
>
> This looks acceptable to me, but I'm now not really sure that we
> should enable the sdmmc2 in the board dts.  Let me explain.
>
> As Jonas demonstrated with the WiFi+Bluetooth DT overlay, additional
> DT configuration is needed to actually make an SDIO M.2 module plugged
> into the ROCK 3B's M.2 slot work, so what do we actually get from
> enabling the sdmmc2 in the board dts?  Just some error messages in
> the kernel log :) and AFAICT no additional functionality when an SDIO
> M.2 module is actually plugged into the slot.

I think if you want to add a specific bluetooth DT overlay for your
favorite module, you should.
Our modules need this much and it's very useful already. I'm not
interested in nailing down every single one we have in a separate
overlay at this point.

>
> >> > Dragan, what did you mean by SDIO related power timing requirements?
> >>
> >> Whenever there's an SDIO module, there's usually some required timing
> >> of the power rails.  Though, I don't know what's that like with the
> >> non-standard M.2 SDIO modules that Radxa sells, which are intended to
> >> be used on Radxa boards with "hybrid" M.2 slots.
> >
> > Ok, I see. Not always. I can't comment on Radxa's SDIO module but I'm
> > sure it's reasonably standard. And so is the M.2 Key E on this board.
> > Actually, part of the appeal is that all standard buses are very
> > nicely wired up. I want everybody to be able to use them.
>
> Yes, but getting it all wired in some way unfortunately doesn't mean
> that it will all work without additional DT configuration in place,
> as described above.

Agreed, well these are the common changes needed.


>
> Also, I'm not really sure there's some strict standard for the "GPIO"
> and "UART" M.2 modules, that part of the specification was/is a bit
> blurry or perhaps even non-existent.  It's been a while since I wrote
> the M.2 aricle on English Wikipedia, :) maybe it's become defined
> better in the meantime.
>
> >> Once again, please use inline replying. [*]
> >>
> >> [*] https://en.wikipedia.org/wiki/Posting_style
> >>
> >> > On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org>
> >> > wrote:
> >> >>
> >> >> Hello Jonas and Tamas,
> >> >>
> >> >> On 2024-11-11 20:06, Jonas Karlman wrote:
> >> >> > On 2024-11-11 19:17, Tamás Szűcs wrote:
> >> >> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
> >> >> >> rates and
> >> >> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
> >> >> >>
> >> >> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> >> >> >> ---
> >> >> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
> >> >> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
> >> >> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> >> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> >> >> @@ -688,14 +688,20 @@ &sdmmc2 {
> >> >> >>      cap-sd-highspeed;
> >> >> >>      cap-sdio-irq;
> >> >> >>      keep-power-in-suspend;
> >> >> >> +    max-frequency = <200000000>;
> >> >> >>      mmc-pwrseq = <&sdio_pwrseq>;
> >> >> >>      non-removable;
> >> >> >>      pinctrl-names = "default";
> >> >> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
> >> >> >> +    sd-uhs-sdr12;
> >> >> >> +    sd-uhs-sdr25;
> >> >> >> +    sd-uhs-sdr50;
> >> >> >
> >> >> > I thought that lower speeds was implied by uhs-sdr104?
> >> >>
> >> >> Last time I went through the MMC drivers, they were implied.  IIRC,
> >> >> such backward mode compatibility is actually a requirement made by
> >> >> the MMC specification.
> >> >>
> >> >> >>      sd-uhs-sdr104;
> >> >> >> +    sd-uhs-ddr50;
> >> >> >>      vmmc-supply = <&vcc3v3_sys2>;
> >> >> >>      vqmmc-supply = <&vcc_1v8>;
> >> >> >> -    status = "disabled";
> >> >> >> +    wakeup-source;
> >> >> >> +    status = "okay";
> >> >> >
> >> >> > This should probably be enabled using an dt-overlay, there is no
> >> >> > SDIO device embedded on the board and the reason I left it disabled
> >> >> > in original board DT submission.
> >> >>
> >> >> Just went through the ROCK 3B schematic, version 1.51, and I think
> >> >> there should be no need for a separate overlay, because sdmmc2 goes
> >> >> to the M.2 slot on the board, which any user can plug an M.2 module
> >> >> into, and the SDIO interface is kind-of self-discoverable.
> >> >>
> >> >> Of course, all that unless there are some horribly looking :) error
> >> >> messages emitted to the kernel log when nothing is actually found,
> >> >> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
> >> >> not sure what do we do with the possible SDIO-related power timing
> >> >> requirements?

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Dragan Simic]

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?

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Dragan Simic]

Hello Tamas,

On 2024-11-13 11:24, Tamás Szűcs wrote:
> On Wed, Nov 13, 2024 at 12:38 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-11-12 22:05, Tamás Szűcs wrote:
>> > On Tue, Nov 12, 2024 at 4:16 PM Dragan Simic <dsimic@manjaro.org>
>> > wrote:
>> >> On 2024-11-12 15:35, Tamás Szűcs wrote:
>> >> > I think it was totally fine to disable sdmmc2 at first, especially if
>> >> > it couldn’t be tested or wasn’t needed right away. From what I’ve
>> >> > seen, this board works great even at higher clock speeds than what
>> >> > rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
>> >> > and there don’t seem to be any limits mentioned in the TRM either.
>> >> > Overall, this board is doing just fine as it is.
>> >>
>> >> Sorry, I'm missing the point of mentioning some clock speeds?  Any
>> >> chances, please, to clarify that a bit?
>> >
>> > It's all about stress scenarios, right. Sustained transfer at maximum
>> > clock, multiple SD/MMC blocks used concurrently. That kind of thing.
>> > Different data rates forced. I hope that answers your question.
>> 
>> Ah, I see.  Though, I don't think we should worry much about that,
>> although testing it all is, of course, a good thing to do.
> 
> Better be safe than sorry. Let's just say I've seen worse.
> 
>> >> > Regarding device tree overlays, they would be ideal for implementing
>> >> > secondary functions, such as PCIe endpoint mode for users with
>> >> > specific requirements. However, the primary functions for PCIe on the
>> >> > M2E will be root complex mode, along with SDIO host, etc. In my view,
>> >> > the hardware is well-designed and interconnected. Users have a
>> >> > reasonable expectation that these primary functions should work
>> >> > seamlessly without additional configuration, right out of the box.
>> >>
>> >> That's basically what I referred to in my earlier response, and in my
>> >> previous response regarding the UART.  Users would expect the
>> >> Bluetooth
>> >> part to work as well, but the error messages I mentioned look nasty,
>> >> so
>> >> perhaps something should be done about that first.
>> >
>> > I'm not aware of any nasty error messages especially related to UART.
>> > Well, MMC core will acknowledge when the platform part fails to
>> > enumerate a device on sdmmc2, but there's nothing wrong with this.
>> > It's not even an error -- certainly not a nasty one.
>> >
>> > [    1.799703] mmc_host mmc2: card is non-removable.
>> > [    1.935011] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
>> > 400000Hz, actual 375000HZ div = 0)
>> > [    7.195009] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
>> > 375000Hz, actual 375000HZ div = 0)
>> > [   13.029540] mmc2: Failed to initialize a non-removable card
>> 
>> This looks acceptable to me, but I'm now not really sure that we
>> should enable the sdmmc2 in the board dts.  Let me explain.
>> 
>> As Jonas demonstrated with the WiFi+Bluetooth DT overlay, additional
>> DT configuration is needed to actually make an SDIO M.2 module plugged
>> into the ROCK 3B's M.2 slot work, so what do we actually get from
>> enabling the sdmmc2 in the board dts?  Just some error messages in
>> the kernel log :) and AFAICT no additional functionality when an SDIO
>> M.2 module is actually plugged into the slot.
> 
> I think if you want to add a specific bluetooth DT overlay for your
> favorite module, you should.
> Our modules need this much and it's very useful already. I'm not
> interested in nailing down every single one we have in a separate
> overlay at this point.

It would help if you'd share more details about the M.2 SDIO
module you're referring to, please.

>> >> > Dragan, what did you mean by SDIO related power timing requirements?
>> >>
>> >> Whenever there's an SDIO module, there's usually some required timing
>> >> of the power rails.  Though, I don't know what's that like with the
>> >> non-standard M.2 SDIO modules that Radxa sells, which are intended to
>> >> be used on Radxa boards with "hybrid" M.2 slots.
>> >
>> > Ok, I see. Not always. I can't comment on Radxa's SDIO module but I'm
>> > sure it's reasonably standard. And so is the M.2 Key E on this board.
>> > Actually, part of the appeal is that all standard buses are very
>> > nicely wired up. I want everybody to be able to use them.
>> 
>> Yes, but getting it all wired in some way unfortunately doesn't mean
>> that it will all work without additional DT configuration in place,
>> as described above.
> 
> Agreed, well these are the common changes needed.

They are common indeed, but unless demonstrated they're all that's
needed to get some M.2 SDIO module fully functional, it escapes me
to see what are the benefits of including (and more importantly,
enabling) these changes under the umbrella of commonality.

>> Also, I'm not really sure there's some strict standard for the "GPIO"
>> and "UART" M.2 modules, that part of the specification was/is a bit
>> blurry or perhaps even non-existent.  It's been a while since I wrote
>> the M.2 aricle on English Wikipedia, :) maybe it's become defined
>> better in the meantime.
>> 
>> >> Once again, please use inline replying. [*]
>> >>
>> >> [*] https://en.wikipedia.org/wiki/Posting_style
>> >>
>> >> > On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org>
>> >> > wrote:
>> >> >>
>> >> >> Hello Jonas and Tamas,
>> >> >>
>> >> >> On 2024-11-11 20:06, Jonas Karlman wrote:
>> >> >> > On 2024-11-11 19:17, Tamás Szűcs wrote:
>> >> >> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
>> >> >> >> rates and
>> >> >> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
>> >> >> >>
>> >> >> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
>> >> >> >> ---
>> >> >> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
>> >> >> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
>> >> >> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> >> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> >> >> @@ -688,14 +688,20 @@ &sdmmc2 {
>> >> >> >>      cap-sd-highspeed;
>> >> >> >>      cap-sdio-irq;
>> >> >> >>      keep-power-in-suspend;
>> >> >> >> +    max-frequency = <200000000>;
>> >> >> >>      mmc-pwrseq = <&sdio_pwrseq>;
>> >> >> >>      non-removable;
>> >> >> >>      pinctrl-names = "default";
>> >> >> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
>> >> >> >> +    sd-uhs-sdr12;
>> >> >> >> +    sd-uhs-sdr25;
>> >> >> >> +    sd-uhs-sdr50;
>> >> >> >
>> >> >> > I thought that lower speeds was implied by uhs-sdr104?
>> >> >>
>> >> >> Last time I went through the MMC drivers, they were implied.  IIRC,
>> >> >> such backward mode compatibility is actually a requirement made by
>> >> >> the MMC specification.
>> >> >>
>> >> >> >>      sd-uhs-sdr104;
>> >> >> >> +    sd-uhs-ddr50;
>> >> >> >>      vmmc-supply = <&vcc3v3_sys2>;
>> >> >> >>      vqmmc-supply = <&vcc_1v8>;
>> >> >> >> -    status = "disabled";
>> >> >> >> +    wakeup-source;
>> >> >> >> +    status = "okay";
>> >> >> >
>> >> >> > This should probably be enabled using an dt-overlay, there is no
>> >> >> > SDIO device embedded on the board and the reason I left it disabled
>> >> >> > in original board DT submission.
>> >> >>
>> >> >> Just went through the ROCK 3B schematic, version 1.51, and I think
>> >> >> there should be no need for a separate overlay, because sdmmc2 goes
>> >> >> to the M.2 slot on the board, which any user can plug an M.2 module
>> >> >> into, and the SDIO interface is kind-of self-discoverable.
>> >> >>
>> >> >> Of course, all that unless there are some horribly looking :) error
>> >> >> messages emitted to the kernel log when nothing is actually found,
>> >> >> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
>> >> >> not sure what do we do with the possible SDIO-related power timing
>> >> >> requirements?

Re: [PATCH 3/3] arm64: dts: rockchip: Enable UART8 on rock-3b

[Tamás Szűcs]

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

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Tamás Szűcs]

Hi Dragan,

On Wed, Nov 13, 2024 at 11:44 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:38 AM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >> On 2024-11-12 22:05, Tamás Szűcs wrote:
> >> > On Tue, Nov 12, 2024 at 4:16 PM Dragan Simic <dsimic@manjaro.org>
> >> > wrote:
> >> >> On 2024-11-12 15:35, Tamás Szűcs wrote:
> >> >> > I think it was totally fine to disable sdmmc2 at first, especially if
> >> >> > it couldn’t be tested or wasn’t needed right away. From what I’ve
> >> >> > seen, this board works great even at higher clock speeds than what
> >> >> > rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
> >> >> > and there don’t seem to be any limits mentioned in the TRM either.
> >> >> > Overall, this board is doing just fine as it is.
> >> >>
> >> >> Sorry, I'm missing the point of mentioning some clock speeds?  Any
> >> >> chances, please, to clarify that a bit?
> >> >
> >> > It's all about stress scenarios, right. Sustained transfer at maximum
> >> > clock, multiple SD/MMC blocks used concurrently. That kind of thing.
> >> > Different data rates forced. I hope that answers your question.
> >>
> >> Ah, I see.  Though, I don't think we should worry much about that,
> >> although testing it all is, of course, a good thing to do.
> >
> > Better be safe than sorry. Let's just say I've seen worse.
> >
> >> >> > Regarding device tree overlays, they would be ideal for implementing
> >> >> > secondary functions, such as PCIe endpoint mode for users with
> >> >> > specific requirements. However, the primary functions for PCIe on the
> >> >> > M2E will be root complex mode, along with SDIO host, etc. In my view,
> >> >> > the hardware is well-designed and interconnected. Users have a
> >> >> > reasonable expectation that these primary functions should work
> >> >> > seamlessly without additional configuration, right out of the box.
> >> >>
> >> >> That's basically what I referred to in my earlier response, and in my
> >> >> previous response regarding the UART.  Users would expect the
> >> >> Bluetooth
> >> >> part to work as well, but the error messages I mentioned look nasty,
> >> >> so
> >> >> perhaps something should be done about that first.
> >> >
> >> > I'm not aware of any nasty error messages especially related to UART.
> >> > Well, MMC core will acknowledge when the platform part fails to
> >> > enumerate a device on sdmmc2, but there's nothing wrong with this.
> >> > It's not even an error -- certainly not a nasty one.
> >> >
> >> > [    1.799703] mmc_host mmc2: card is non-removable.
> >> > [    1.935011] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
> >> > 400000Hz, actual 375000HZ div = 0)
> >> > [    7.195009] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
> >> > 375000Hz, actual 375000HZ div = 0)
> >> > [   13.029540] mmc2: Failed to initialize a non-removable card
> >>
> >> This looks acceptable to me, but I'm now not really sure that we
> >> should enable the sdmmc2 in the board dts.  Let me explain.
> >>
> >> As Jonas demonstrated with the WiFi+Bluetooth DT overlay, additional
> >> DT configuration is needed to actually make an SDIO M.2 module plugged
> >> into the ROCK 3B's M.2 slot work, so what do we actually get from
> >> enabling the sdmmc2 in the board dts?  Just some error messages in
> >> the kernel log :) and AFAICT no additional functionality when an SDIO
> >> M.2 module is actually plugged into the slot.
> >
> > I think if you want to add a specific bluetooth DT overlay for your
> > favorite module, you should.
> > Our modules need this much and it's very useful already. I'm not
> > interested in nailing down every single one we have in a separate
> > overlay at this point.
>
> It would help if you'd share more details about the M.2 SDIO
> module you're referring to, please.

Kindly refer to 3/3.

>
> >> >> > Dragan, what did you mean by SDIO related power timing requirements?
> >> >>
> >> >> Whenever there's an SDIO module, there's usually some required timing
> >> >> of the power rails.  Though, I don't know what's that like with the
> >> >> non-standard M.2 SDIO modules that Radxa sells, which are intended to
> >> >> be used on Radxa boards with "hybrid" M.2 slots.
> >> >
> >> > Ok, I see. Not always. I can't comment on Radxa's SDIO module but I'm
> >> > sure it's reasonably standard. And so is the M.2 Key E on this board.
> >> > Actually, part of the appeal is that all standard buses are very
> >> > nicely wired up. I want everybody to be able to use them.
> >>
> >> Yes, but getting it all wired in some way unfortunately doesn't mean
> >> that it will all work without additional DT configuration in place,
> >> as described above.
> >
> > Agreed, well these are the common changes needed.
>
> They are common indeed, but unless demonstrated they're all that's
> needed to get some M.2 SDIO module fully functional, it escapes me
> to see what are the benefits of including (and more importantly,
> enabling) these changes under the umbrella of commonality.

Please don't make it look harder than it is. Load device driver,
download firmware(s), you're set.

Kind regards,
Tamas


>
> >> Also, I'm not really sure there's some strict standard for the "GPIO"
> >> and "UART" M.2 modules, that part of the specification was/is a bit
> >> blurry or perhaps even non-existent.  It's been a while since I wrote
> >> the M.2 aricle on English Wikipedia, :) maybe it's become defined
> >> better in the meantime.
> >>
> >> >> Once again, please use inline replying. [*]
> >> >>
> >> >> [*] https://en.wikipedia.org/wiki/Posting_style
> >> >>
> >> >> > On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org>
> >> >> > wrote:
> >> >> >>
> >> >> >> Hello Jonas and Tamas,
> >> >> >>
> >> >> >> On 2024-11-11 20:06, Jonas Karlman wrote:
> >> >> >> > On 2024-11-11 19:17, Tamás Szűcs wrote:
> >> >> >> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
> >> >> >> >> rates and
> >> >> >> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
> >> >> >> >>
> >> >> >> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
> >> >> >> >> ---
> >> >> >> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
> >> >> >> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
> >> >> >> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> >> >> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >> >> >> >> @@ -688,14 +688,20 @@ &sdmmc2 {
> >> >> >> >>      cap-sd-highspeed;
> >> >> >> >>      cap-sdio-irq;
> >> >> >> >>      keep-power-in-suspend;
> >> >> >> >> +    max-frequency = <200000000>;
> >> >> >> >>      mmc-pwrseq = <&sdio_pwrseq>;
> >> >> >> >>      non-removable;
> >> >> >> >>      pinctrl-names = "default";
> >> >> >> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
> >> >> >> >> +    sd-uhs-sdr12;
> >> >> >> >> +    sd-uhs-sdr25;
> >> >> >> >> +    sd-uhs-sdr50;
> >> >> >> >
> >> >> >> > I thought that lower speeds was implied by uhs-sdr104?
> >> >> >>
> >> >> >> Last time I went through the MMC drivers, they were implied.  IIRC,
> >> >> >> such backward mode compatibility is actually a requirement made by
> >> >> >> the MMC specification.
> >> >> >>
> >> >> >> >>      sd-uhs-sdr104;
> >> >> >> >> +    sd-uhs-ddr50;
> >> >> >> >>      vmmc-supply = <&vcc3v3_sys2>;
> >> >> >> >>      vqmmc-supply = <&vcc_1v8>;
> >> >> >> >> -    status = "disabled";
> >> >> >> >> +    wakeup-source;
> >> >> >> >> +    status = "okay";
> >> >> >> >
> >> >> >> > This should probably be enabled using an dt-overlay, there is no
> >> >> >> > SDIO device embedded on the board and the reason I left it disabled
> >> >> >> > in original board DT submission.
> >> >> >>
> >> >> >> Just went through the ROCK 3B schematic, version 1.51, and I think
> >> >> >> there should be no need for a separate overlay, because sdmmc2 goes
> >> >> >> to the M.2 slot on the board, which any user can plug an M.2 module
> >> >> >> into, and the SDIO interface is kind-of self-discoverable.
> >> >> >>
> >> >> >> Of course, all that unless there are some horribly looking :) error
> >> >> >> messages emitted to the kernel log when nothing is actually found,
> >> >> >> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
> >> >> >> not sure what do we do with the possible SDIO-related power timing
> >> >> >> requirements?

Re: [PATCH 2/3] arm64: dts: rockchip: Enable sdmmc2 on rock-3b and set it up for SDIO devices

[Dragan Simic]

Hello Tamas,

On 2024-11-13 12:17, Tamás Szűcs wrote:
> On Wed, Nov 13, 2024 at 11:44 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-11-13 11:24, Tamás Szűcs wrote:
>> > On Wed, Nov 13, 2024 at 12:38 AM Dragan Simic <dsimic@manjaro.org>
>> > wrote:
>> >> On 2024-11-12 22:05, Tamás Szűcs wrote:
>> >> > On Tue, Nov 12, 2024 at 4:16 PM Dragan Simic <dsimic@manjaro.org>
>> >> > wrote:
>> >> >> On 2024-11-12 15:35, Tamás Szűcs wrote:
>> >> >> > I think it was totally fine to disable sdmmc2 at first, especially if
>> >> >> > it couldn’t be tested or wasn’t needed right away. From what I’ve
>> >> >> > seen, this board works great even at higher clock speeds than what
>> >> >> > rk356x-base.dtsi suggests. I don’t have access to the RK3568 errata,
>> >> >> > and there don’t seem to be any limits mentioned in the TRM either.
>> >> >> > Overall, this board is doing just fine as it is.
>> >> >>
>> >> >> Sorry, I'm missing the point of mentioning some clock speeds?  Any
>> >> >> chances, please, to clarify that a bit?
>> >> >
>> >> > It's all about stress scenarios, right. Sustained transfer at maximum
>> >> > clock, multiple SD/MMC blocks used concurrently. That kind of thing.
>> >> > Different data rates forced. I hope that answers your question.
>> >>
>> >> Ah, I see.  Though, I don't think we should worry much about that,
>> >> although testing it all is, of course, a good thing to do.
>> >
>> > Better be safe than sorry. Let's just say I've seen worse.
>> >
>> >> >> > Regarding device tree overlays, they would be ideal for implementing
>> >> >> > secondary functions, such as PCIe endpoint mode for users with
>> >> >> > specific requirements. However, the primary functions for PCIe on the
>> >> >> > M2E will be root complex mode, along with SDIO host, etc. In my view,
>> >> >> > the hardware is well-designed and interconnected. Users have a
>> >> >> > reasonable expectation that these primary functions should work
>> >> >> > seamlessly without additional configuration, right out of the box.
>> >> >>
>> >> >> That's basically what I referred to in my earlier response, and in my
>> >> >> previous response regarding the UART.  Users would expect the
>> >> >> Bluetooth
>> >> >> part to work as well, but the error messages I mentioned look nasty,
>> >> >> so
>> >> >> perhaps something should be done about that first.
>> >> >
>> >> > I'm not aware of any nasty error messages especially related to UART.
>> >> > Well, MMC core will acknowledge when the platform part fails to
>> >> > enumerate a device on sdmmc2, but there's nothing wrong with this.
>> >> > It's not even an error -- certainly not a nasty one.
>> >> >
>> >> > [    1.799703] mmc_host mmc2: card is non-removable.
>> >> > [    1.935011] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
>> >> > 400000Hz, actual 375000HZ div = 0)
>> >> > [    7.195009] mmc_host mmc2: Bus speed (slot 0) = 375000Hz (slot req
>> >> > 375000Hz, actual 375000HZ div = 0)
>> >> > [   13.029540] mmc2: Failed to initialize a non-removable card
>> >>
>> >> This looks acceptable to me, but I'm now not really sure that we
>> >> should enable the sdmmc2 in the board dts.  Let me explain.
>> >>
>> >> As Jonas demonstrated with the WiFi+Bluetooth DT overlay, additional
>> >> DT configuration is needed to actually make an SDIO M.2 module plugged
>> >> into the ROCK 3B's M.2 slot work, so what do we actually get from
>> >> enabling the sdmmc2 in the board dts?  Just some error messages in
>> >> the kernel log :) and AFAICT no additional functionality when an SDIO
>> >> M.2 module is actually plugged into the slot.
>> >
>> > I think if you want to add a specific bluetooth DT overlay for your
>> > favorite module, you should.
>> > Our modules need this much and it's very useful already. I'm not
>> > interested in nailing down every single one we have in a separate
>> > overlay at this point.
>> 
>> It would help if you'd share more details about the M.2 SDIO
>> module you're referring to, please.
> 
> Kindly refer to 3/3.

Just had a look at that product list page, and even tried having
a look at MAYA-W4_ProductSummary_UBXDOC-465451970-3565.pdf, for
example, and all I see are some high-level product descriptions,
with no technical details we'd need to have a look at.

>> >> >> > Dragan, what did you mean by SDIO related power timing requirements?
>> >> >>
>> >> >> Whenever there's an SDIO module, there's usually some required timing
>> >> >> of the power rails.  Though, I don't know what's that like with the
>> >> >> non-standard M.2 SDIO modules that Radxa sells, which are intended to
>> >> >> be used on Radxa boards with "hybrid" M.2 slots.
>> >> >
>> >> > Ok, I see. Not always. I can't comment on Radxa's SDIO module but I'm
>> >> > sure it's reasonably standard. And so is the M.2 Key E on this board.
>> >> > Actually, part of the appeal is that all standard buses are very
>> >> > nicely wired up. I want everybody to be able to use them.
>> >>
>> >> Yes, but getting it all wired in some way unfortunately doesn't mean
>> >> that it will all work without additional DT configuration in place,
>> >> as described above.
>> >
>> > Agreed, well these are the common changes needed.
>> 
>> They are common indeed, but unless demonstrated they're all that's
>> needed to get some M.2 SDIO module fully functional, it escapes me
>> to see what are the benefits of including (and more importantly,
>> enabling) these changes under the umbrella of commonality.
> 
> Please don't make it look harder than it is. Load device driver,
> download firmware(s), you're set.

Well, all I can say to this is that you may be making the things
look way simpler than they usually are.  Though, let's also see
what other people will respond with.

>> >> Also, I'm not really sure there's some strict standard for the "GPIO"
>> >> and "UART" M.2 modules, that part of the specification was/is a bit
>> >> blurry or perhaps even non-existent.  It's been a while since I wrote
>> >> the M.2 aricle on English Wikipedia, :) maybe it's become defined
>> >> better in the meantime.
>> >>
>> >> >> Once again, please use inline replying. [*]
>> >> >>
>> >> >> [*] https://en.wikipedia.org/wiki/Posting_style
>> >> >>
>> >> >> > On Tue, Nov 12, 2024 at 5:41 AM Dragan Simic <dsimic@manjaro.org>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> Hello Jonas and Tamas,
>> >> >> >>
>> >> >> >> On 2024-11-11 20:06, Jonas Karlman wrote:
>> >> >> >> > On 2024-11-11 19:17, Tamás Szűcs wrote:
>> >> >> >> >> Enable SDIO on Radxa ROCK 3 Model B M.2 Key E. Add all supported UHS-I
>> >> >> >> >> rates and
>> >> >> >> >> enable 200 MHz maximum clock. Also, allow host wakeup via SDIO IRQ.
>> >> >> >> >>
>> >> >> >> >> Signed-off-by: Tamás Szűcs <tszucs@linux.com>
>> >> >> >> >> ---
>> >> >> >> >>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 8 +++++++-
>> >> >> >> >>  1 file changed, 7 insertions(+), 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 242af5337cdf..b7527ba418f7 100644
>> >> >> >> >> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> >> >> >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
>> >> >> >> >> @@ -688,14 +688,20 @@ &sdmmc2 {
>> >> >> >> >>      cap-sd-highspeed;
>> >> >> >> >>      cap-sdio-irq;
>> >> >> >> >>      keep-power-in-suspend;
>> >> >> >> >> +    max-frequency = <200000000>;
>> >> >> >> >>      mmc-pwrseq = <&sdio_pwrseq>;
>> >> >> >> >>      non-removable;
>> >> >> >> >>      pinctrl-names = "default";
>> >> >> >> >>      pinctrl-0 = <&sdmmc2m0_bus4 &sdmmc2m0_clk &sdmmc2m0_cmd>;
>> >> >> >> >> +    sd-uhs-sdr12;
>> >> >> >> >> +    sd-uhs-sdr25;
>> >> >> >> >> +    sd-uhs-sdr50;
>> >> >> >> >
>> >> >> >> > I thought that lower speeds was implied by uhs-sdr104?
>> >> >> >>
>> >> >> >> Last time I went through the MMC drivers, they were implied.  IIRC,
>> >> >> >> such backward mode compatibility is actually a requirement made by
>> >> >> >> the MMC specification.
>> >> >> >>
>> >> >> >> >>      sd-uhs-sdr104;
>> >> >> >> >> +    sd-uhs-ddr50;
>> >> >> >> >>      vmmc-supply = <&vcc3v3_sys2>;
>> >> >> >> >>      vqmmc-supply = <&vcc_1v8>;
>> >> >> >> >> -    status = "disabled";
>> >> >> >> >> +    wakeup-source;
>> >> >> >> >> +    status = "okay";
>> >> >> >> >
>> >> >> >> > This should probably be enabled using an dt-overlay, there is no
>> >> >> >> > SDIO device embedded on the board and the reason I left it disabled
>> >> >> >> > in original board DT submission.
>> >> >> >>
>> >> >> >> Just went through the ROCK 3B schematic, version 1.51, and I think
>> >> >> >> there should be no need for a separate overlay, because sdmmc2 goes
>> >> >> >> to the M.2 slot on the board, which any user can plug an M.2 module
>> >> >> >> into, and the SDIO interface is kind-of self-discoverable.
>> >> >> >>
>> >> >> >> Of course, all that unless there are some horribly looking :) error
>> >> >> >> messages emitted to the kernel log when nothing is actually found,
>> >> >> >> in which case the SDIO/MMC driers should be fixed first.  Also, I'm
>> >> >> >> not sure what do we do with the possible SDIO-related power timing
>> >> >> >> requirements?