Page MenuHomeFreeBSD

Add USB 3.0 support for Rockchip RK3328/RK3399 SoC
Needs ReviewPublic

Authored by greg_unrelenting.technology on Feb 24 2019, 9:48 PM.

Details

Reviewers
manu
Summary
  • Add DesignWare XHCI controller initialization imported from OpenBSD
  • Add a driver for the Rockchip glue node that wraps the DesignWare XHCI node

At first, I couldn't test that this works on my Pine64 ROCKPro64, as the 5V USB power went down almost as soon as the loader jumped to the kernel (right before seeing any device attachment messages on the serial console). I've tried writing a driver for the USB 2 PHY that would turn that on, but the vcc5v0_host regulator is supposedly already enabled. I just can't figure that out.

But today, I stumbled upon an article reviewing a Linux distro on this board, and saw this:

sudo cat /sys/kernel/debug/gpio
[…]
GPIOs 128-159, platform/pinctrl, gpio4:
 gpio-154 (                    |vcc5v0_host         ) out hi

That made the workaround obvious:

gpioctl -f /dev/gpioc4 26 1

With that command, the power is back on, and yes — the USB-A 3.0 port actually works!! :)

Figuring out how to make the power come on without running that command is out of scope for this patch, and might not even be required on boards other than the ROCKPro64…

What I've discovered is that pinctrl writes zero to that pin on boot, so probably the output is just late. Maybe enable-active-high support for fixed regulator is broken or something.

rk_pinctrl0: conf pin: bank 4 subbank 3 pin 26 function 0 bit 4 mask 3145728 reg 57388
rk_pinctrl0: conf pin: wrote 0 << 4 | 3145728 == 3145728

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

manu added a comment.Feb 25 2019, 10:11 AM

I have a local patch that I need to finish to do the switch from marvell xhci to generic-xhci, it also handle the phy properly. So I will try to do that this week and you can then rebase your work on it.
For the snps,dwc3 I think the better way is to add a different file, it's a big controller that can do a lot of thing and this doesn't belong in generic-xhci.
You also need to handle the drmode, on the RockPro64 you have two dwc3, one in host mode and one in otg mode.

sys/arm64/rockchip/rk_xhci.c
85

The clock are named in the bindings, please use the name instead of this loop.
Also the clock should be in softc so we can disable them in detach/suspend/resume.
And we don't have the clocks in the cru driver, this needs to go first.

94

Same thing here, use the names.
And same thing for the reset in the cru driver.

sys/dev/usb/controller/xhci_fdt.c
7 ↗(On Diff #54314)

I don't think that the OpenBSD file you based your work on have the same licence so you can't do that.
I also don't think that you changed enough material to add your copyright.

manu added inline comments.Feb 27 2019, 5:51 PM
sys/arm64/rockchip/rk_xhci.c
103

According to the bindings (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt?id=4436a3711e3249840e0679e92d3c951bcaf25515) the only requirement is to have a child node (and I think only one), so you need to clean and return ENXIO if no child node is present.

manu added a comment.Feb 27 2019, 9:08 PM

I've just commited my mv_xhci to generic_xhci patch, now we need a proper dwc3 driver.

greg_unrelenting.technology edited the summary of this revision. (Show Details)

rebased, depends on D19986

not tested yet — for some reason my rockpro64 can't boot: clknode_register hangs when registering the first PLL on rk3399_cru0 :(

greg_unrelenting.technology marked an inline comment as done.May 1 2019, 7:33 PM
greg_unrelenting.technology added inline comments.
sys/arm64/rockchip/rk_xhci.c
85

But not all of the clocks in the device tree are mentioned in the bindings txt, only a list of the ones that "Should" be present (I wonder if that's SHOULD in the RFC 2119 sense :D)

Linux actually has a generic driver for multiple glue nodes like this, not just Rockchip's one, and it iterates over all clocks

manu added a comment.May 2 2019, 8:10 AM

There is a lot of new stuff unrelated to this review now.

manu added a comment.May 2 2019, 8:11 AM

Meh sorry, it seems that I was watching the diff between the last review.

manu added a comment.May 2 2019, 8:17 AM

rebased, depends on D19986
not tested yet — for some reason my rockpro64 can't boot: clknode_register hangs when registering the first PLL on rk3399_cru0 :(

Could you test reverting r344626 ? ganbold@ have problem with this revision on the NanoPi T4. I personally don't have problems booting it (tried yesterday).

sys/arm64/rockchip/rk_xhci.c
85

Mhm yeah it seems that the bindings docs don't list "aclk_usb3_rksoc_axi_perf" and "aclk_usb3" but on our side we don't have those clocks defined.

manu added a subscriber: kevans.May 2 2019, 4:48 PM
In D19335#433397, @manu wrote:

rebased, depends on D19986
not tested yet — for some reason my rockpro64 can't boot: clknode_register hangs when registering the first PLL on rk3399_cru0 :(

Could you test reverting r344626 ? ganbold@ have problem with this revision on the NanoPi T4. I personally don't have problems booting it (tried yesterday).

Mhm no, I've unplugged my eMMC module from my RockPro64 for a long time now so it's not that.
Could you try with the latest dtb ? It's installed now but on the wrong path until @kevans commit his fix. (so it's in /boot/dtb/ directly right now)

In D19335#433606, @manu wrote:
In D19335#433397, @manu wrote:

rebased, depends on D19986
not tested yet — for some reason my rockpro64 can't boot: clknode_register hangs when registering the first PLL on rk3399_cru0 :(

Could you test reverting r344626 ? ganbold@ have problem with this revision on the NanoPi T4. I personally don't have problems booting it (tried yesterday).

Mhm no, I've unplugged my eMMC module from my RockPro64 for a long time now so it's not that.
Could you try with the latest dtb ? It's installed now but on the wrong path until @kevans commit his fix. (so it's in /boot/dtb/ directly right now)

I have rebuilt the dtb (it didn't really change since the last time I did that). Installation fix doesn't matter to me, I do everything by hand (custom netboot setup).

Yeah the SD clock stuff shouldn't matter because it doesn't get to that, it hangs when registering the *first* clock in the list.

I've been sort of suspecting the new entropy requirement thing, but /boot/entropy was loaded… (though the warning was there anyway)

update: what's hanging is the WRITE4 for putting the PLL into normal mode!

when commenting that out, there's also panic: clknode_init_parent_idx: Invalid parent index 5 for clock sclk_sdmmc, so for now my rockpro64 only boots with:

diff --git i/sys/arm64/rockchip/clk/rk3399_cru.c w/sys/arm64/rockchip/clk/rk3399_cru.c
index dbc1d0aa911..4b2dd7ee929 100644
--- i/sys/arm64/rockchip/clk/rk3399_cru.c
+++ w/sys/arm64/rockchip/clk/rk3399_cru.c
@@ -1434,7 +1434,7 @@ static struct rk_clk_composite_def hclk_sd = {
 
 #define	SCLK_SDMMC		76
 
-static const char *sclk_sdmmc_parents[] = {"cpll", "gpll", "npll", "ppll"};
+static const char *sclk_sdmmc_parents[] = {"cpll", "gpll", "npll", "ppll", "npll", "npll"};
 
 static struct rk_clk_composite_def sclk_sdmmc = {
 	.clkdef = {
diff --git i/sys/arm64/rockchip/clk/rk_clk_pll.c w/sys/arm64/rockchip/clk/rk_clk_pll.c
index a7c92123b35..8fff9e629c7 100644
--- i/sys/arm64/rockchip/clk/rk_clk_pll.c
+++ w/sys/arm64/rockchip/clk/rk_clk_pll.c
@@ -350,8 +350,9 @@ rk3399_clk_pll_init(struct clknode *clk, device_t dev)
 		/* Setting to normal mode */
 		reg = RK3399_CLK_PLL_MODE_NORMAL << RK3399_CLK_PLL_MODE_SHIFT;
 		reg |= RK3399_CLK_PLL_MODE_MASK << RK_CLK_PLL_MASK_SHIFT;
-		WRITE4(clk, sc->base_offset + RK3399_CLK_PLL_MODE_OFFSET,
-		    reg | RK3399_CLK_PLL_WRITE_MASK);
+		device_printf(dev, "normal %x %x\n", sc->base_offset + RK3399_CLK_PLL_MODE_OFFSET, reg | RK3399_CLK_PLL_WRITE_MASK);
+		/* WRITE4(clk, sc->base_offset + RK3399_CLK_PLL_MODE_OFFSET, */
+		/*     reg | RK3399_CLK_PLL_WRITE_MASK); */
 	}
 
 	clknode_init_parent_idx(clk, 0);

But I have confirmed that xhci works. (Still needs that GPIO call of course, any ideas about that?)