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

Tue Mar 31 14:37:17 CEST 2020

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

Jagan.