On Wed, 2020-04-01 at 18:09 +0800, Chen-Yu Tsai wrote: > On Tue, Mar 31, 2020 at 8:37 PM Jagan Teki <ja...@amarulasolutions.com> wrote: > > > > > > On Tue, Mar 31, 2020 at 5:16 PM Chen-Yu Tsai <w...@kernel.org> wrote: > > > > > > > > > On Tue, Mar 31, 2020 at 6:57 PM Jagan Teki <ja...@amarulasolutions.com> > > > wrote: > > > > > > > > > > > > On Mon, Mar 30, 2020 at 11:54 PM Chen-Yu Tsai <w...@kernel.org> wrote: > > > > > > > > > > > > > > > On Tue, Mar 31, 2020 at 1:44 AM Jagan Teki > > > > > <ja...@amarulasolutions.com> wrote: > > > > > > > > > > > > > > > > > > Hi Chen-Yu, > > > > > > > > > > > > On Fri, Mar 27, 2020 at 10:12 AM Chen-Yu Tsai <w...@kernel.org> > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > From: Chen-Yu Tsai <w...@csie.org> > > > > > > > > > > > > > > The ROC-RK3328-CC from Firefly and Libre Computer Project is a > > > > > > > credit > > > > > > > card size development board based on the Rockchip RK3328 SoC, > > > > > > > with: > > > > > > > > > > > > > > - 1/2/4 GB DDR4 DRAM > > > > > > > - eMMC connector for optional module > > > > > > > - micro SD card slot > > > > > > > - 1 x USB 3.0 host port > > > > > > > - 2 x USB 2.0 host port > > > > > > > - 1 x USB 2.0 OTG port > > > > > > > - HDMI video output > > > > > > > - TRRS connector with audio and composite video output > > > > > > > - gigabit Ethernet > > > > > > > - consumer IR receiver > > > > > > > - debug UART pins > > > > > > > > > > > > > > The ROC-RK3328-CC has the enable pin of the SD card power switch > > > > > > > tied > > > > > > > to GPIO_0_D6. This pin also has the function SDMMC0_PWREN, which > > > > > > > is > > > > > > > muxed by default. SDMMC0_PWREN is an active high signal > > > > > > > controlled by > > > > > > > the MMC controller, however the switch enable is active low, and > > > > > > > pulled low (enabled) by default to make things work on boot. > > > > > > > > > > > > > > As such, we need to mux away from SDMMC0_PWREN and use GPIO to > > > > > > > enable > > > > > > > power to the card. The default GPIO state for the pin is > > > > > > > pull-down and > > > > > > > input, which doesn't require extra configuration when paired with > > > > > > > the > > > > > > > external pull-down and active low switch. > > > > > > > > > > > > > > Thus we make a custom target for this board and do the muxing in > > > > > > > its > > > > > > > spl_board_init() function. > > > > > > > > > > > > > > The device tree file is synced from the Linux kernel > > > > > > > next-20200324. > > > > > > > > > > > > > > Signed-off-by: Chen-Yu Tsai <w...@csie.org> > > > > > > > --- > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > diff --git a/board/firefly/roc-cc-rk3328/MAINTAINERS > > > > > > > b/board/firefly/roc-cc-rk3328/MAINTAINERS > > > > > > > new file mode 100644 > > > > > > > index 000000000000..f2318e71ac33 > > > > > > > --- /dev/null > > > > > > > +++ b/board/firefly/roc-cc-rk3328/MAINTAINERS > > > > > > > @@ -0,0 +1,7 @@ > > > > > > > +ROC-RK3328-CC > > > > > > > +M: Loic Devulder <ldevul...@suse.com> > > > > > > > +M: Chen-Yu Tsai <w...@csie.org> > > > > > > > +S: Maintained > > > > > > > +F: board/firefly/roc-cc-rk3328/ > > > > > > > +F: configs/roc-rk3328-cc_defconfig > > > > > > > +F: arch/arm/dts/rk3328-roc-cc-u-boot.dtsi > > > > > > > diff --git a/board/firefly/roc-cc-rk3328/Makefile > > > > > > > b/board/firefly/roc-cc-rk3328/Makefile > > > > > > > new file mode 100644 > > > > > > > index 000000000000..1550b5f5f16e > > > > > > > --- /dev/null > > > > > > > +++ b/board/firefly/roc-cc-rk3328/Makefile > > > > > > > @@ -0,0 +1 @@ > > > > > > > +obj-y := board.o > > > > > > > diff --git a/board/firefly/roc-cc-rk3328/board.c > > > > > > > b/board/firefly/roc-cc-rk3328/board.c > > > > > > > new file mode 100644 > > > > > > > index 000000000000..eca58c86b53e > > > > > > > --- /dev/null > > > > > > > +++ b/board/firefly/roc-cc-rk3328/board.c > > > > > > > @@ -0,0 +1,38 @@ > > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > > +/* > > > > > > > + * (C) Copyright 2020 Chen-Yu Tsai <w...@csie.org> > > > > > > > + */ > > > > > > > +#include <asm/io.h> > > > > > > > + > > > > > > > +#include <asm/arch-rockchip/grf_rk3328.h> > > > > > > > +#include <asm/arch-rockchip/hardware.h> > > > > > > > + > > > > > > > +#if defined(CONFIG_SPL_BUILD) > > > > > > > + > > > > > > > +#define GRF_BASE 0xFF100000 > > > > > > > + > > > > > > > +enum { > > > > > > > + GPIO_0_D6_GPIO = 0 << 12, > > > > > > > + GPIO_0_D6_MASK = 0x3 << 12 > > > > > > > +}; > > > > > > > + > > > > > > > +/* > > > > > > > + * The ROC-RK3328-CC has the enable pin of the SD card power > > > > > > > switch tied > > > > > > > + * to GPIO_0_D6. This pin also has the function SDMMC0_PWREN, > > > > > > > which is > > > > > > > + * muxed by default. SDMMC0_PWREN is an active high signal > > > > > > > controlled by > > > > > > > + * the MMC controller, however the switch enable is active low, > > > > > > > and > > > > > > > + * pulled low (enabled) by default to make things work on boot. > > > > > > > + * > > > > > > > + * As such, we need to mux away from SDMMC0_PWREN and use GPIO > > > > > > > to enable > > > > > > > + * power to the card. The default GPIO state for the pin is > > > > > > > pull-down and > > > > > > > + * input, which doesn't require extra configuration when paired > > > > > > > with the > > > > > > > + * external pull-down and active low switch. > > > > > > > + */ > > > > > > > +void spl_board_init(void) > > > > > > > +{ > > > > > > > + struct rk3328_grf_regs * const grf = (void *)GRF_BASE; > > > > > > > + > > > > > > > + rk_clrsetreg(&grf->gpio0d_iomux, GPIO_0_D6_MASK, > > > > > > > GPIO_0_D6_GPIO); > > > > > > > +} > > > > > > So, this seem toggle the bit 12, 13 of GRF_GPIO0D_IOMUX so-that it > > > > > > operate like a gpio(not like default sdmmc0_pwrenm1), isn't it > > > > > > required for the next stages like U-Boot proper and Linux? these has > > > > > > vcc_sd node where it operates a fixed regulator to active low the > > > > > > same > > > > > > pin. > > > > > > > > > > > > gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>; > > > > > So U-boot proper and Linux both have pinctrl and GPIO and all the > > > > > stuff > > > > > to deal with this. SPL doesn't have GPIO enabled, and I believe all > > > > > the > > > > > pinctrl properties are striped out. (BTW, not sure why we're enabling > > > > > pinctrl driver support in SPL if nothing is triggering them.) It seems > > > > > a bit heavy pulling all that stuff in just for one pin mux in SPL. The > > > > > same is also done for the UART pins. > > > > look like it was missed missed enable pinctrl in rk3328-u-boot.dtsi > > > > but usually it should have. I tried of managing all stuff in SPL to > > > > make GPIO-enabled but existing dts never pulled low to make working > > > > like what the above change does. > > > The defconfigs all have > > > > > > CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names > > > interrupt-parent assigned-clocks assigned-clock-rates > > > assigned-clock-parents" > > > > > > So by that default all pinctrl stuff is stripped from the device trees > > > for SPL, which means pins are not muxed properly despite having pinctrl > > > support built into SPL. > > > > > > The question is do we want to add back all the pinctrl properties back > > > to SPL's dtb or do we want to take the programmed way out? > > I'd prefer with SPL's dtb since rockchip platforms are using this kind > > since from early. > > > > > > > > > > > Doing that adds a couple hundred bytes to the SPL dtb. > > > > > > Before: > > > > > > -rw-r--r-- 1 wens wens 62942 Mar 31 19:30 spl/u-boot-spl-dtb.bin > > > -rw-r--r-- 1 wens wens 3166 Mar 31 19:30 spl/u-boot-spl.dtb > > > > > > After: > > > > > > -rw-r--r-- 1 wens wens 63094 Mar 31 19:27 spl/u-boot-spl-dtb.bin > > > -rw-r--r-- 1 wens wens 3318 Mar 31 19:27 spl/u-boot-spl.dtb > > > > > > Then you'll need to enable GPIO, power/regulator (fixed, gpio), > > > I2C (because rk8xx needs it) in SPL, which adds 10+ KB. > > > > > > -rw-r--r-- 1 wens wens 76662 Mar 31 19:33 spl/u-boot-spl-dtb.bin > > > -rw-r--r-- 1 wens wens 3318 Mar 31 19:33 spl/u-boot-spl.dtb > > > > > > Note I haven't tested the last one, only built it. > > > > > > And no, you got it wrong. It's not that it's not pulled down, it's > > > that the PWREN signal from the MMC signal is likely driving the pin > > > high, overriding the pull-down, but the regulator is active low. > > True, this is what I mean above but with typo of pulled low instead of > > pulled-down. > > > > > > > > > > > So again, the question is do we want to add 15 KB just to deal > > > with a signal pinmux. My personal preference is to not do that. > > Correct, I'm also see the ~15KB (14592) increment SPL on overall. > > > > -rw-r--r-- 1 jagan jagan 77910 Mar 31 17:49 spl/u-boot-spl.bin > > -rw-r--r-- 1 jagan jagan 77910 Mar 31 17:49 spl/u-boot-spl-dtb.bin > > > > -rw-r--r-- 1 jagan jagan 63318 Mar 31 17:53 spl/u-boot-spl.bin > > -rw-r--r-- 1 jagan jagan 63318 Mar 31 17:53 spl/u-boot-spl-dtb.bin > > > > I did enable the respective things on SPL and it worked with that logic. > > > > On summary, I'd prefer to enable these stuff from SPL since > > 1. rockchip platforms (including this rk3328) do support TPL, so SPL > > size is not much bothered element at this point. > > 2. due to fact that most of TPL-enabled boards or platforms do enabled > > the stuff similar. > > 3. loader1 from official rockchip do have enough partition size > > (include/configs/rockchip-common.h) > OK. I fiddled around with everything and got it to work after > a few tries. Marking pinconf nodes to be included was definitely > not expected lol. I'll send out v2 later.
I've been following along with the conversation. If I understand correctly, enabling pinctl in SPL will address the rock64 v3 boot hang as well. When the patch set is ready, I'll be happy to test it. > If we want to trim it down, we could make RK8xx SPL support optional > as it seems not all boards use it? > > ChenYu