[PATCH 6/6] rockchip: rk3328: Add support for ROC-RK3328-CC board

[Kurt Miller <kurt@intricatesoftware.com>]

On Wed, 2020-04-01 at 18:09 +0800, Chen-Yu Tsai wrote:
> On Tue, Mar 31, 2020 at 8:37 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > 
> > 
> > On Tue, Mar 31, 2020 at 5:16 PM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > 
> > > 
> > > On Tue, Mar 31, 2020 at 6:57 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > 
> > > > 
> > > > On Mon, Mar 30, 2020 at 11:54 PM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > > > 
> > > > > 
> > > > > On Tue, Mar 31, 2020 at 1:44 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > Hi Chen-Yu,
> > > > > > 
> > > > > > On Fri, Mar 27, 2020 at 10:12 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > From: Chen-Yu Tsai <wens@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 <wens@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 <ldevulder@suse.com>
> > > > > > > +M:?????Chen-Yu Tsai <wens@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 <wens@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