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

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 {

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?

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?

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?

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?

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?

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?

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?

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?

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?