Hi Dragan, On Wed, Nov 13, 2024 at 11:44 AM Dragan Simic <dsimic@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > wrote: > >> On 2024-11-12 22:05, Tamás Szűcs wrote: > >> > On Tue, Nov 12, 2024 at 4:16 PM Dragan Simic <dsimic@xxxxxxxxxxx> > >> > 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@xxxxxxxxxxx> > >> >> > 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@xxxxxxxxx> > >> >> >> >> --- > >> >> >> >> 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?
