Message ID | 20240422-tiger-v1-1-8816b070d748@theobroma-systems.com |
---|---|
State | Superseded |
Delegated to: | Kever Yang |
Headers | show |
Series | rockchip: add support for Theobroma Systems SOM-RK3588-Q7 Tiger module | expand |
Hi Quentin, On 2024-04-22 18:41, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > UART2 controller is the controller in the reference design for debug > console. The default mux is M0 in that reference design. Until now, all > boards seemed to be using UART2M0 but RK3588 Tiger for example will be > using UART2M2 instead. > > Therefore, let's add support for UART2M1 and M2 as possible muxes for > the UART2 controller used as debug console. UART2M1 support was not > tested. > > The default value is M0 to match the one used currently by all devices > and the reference design. Is this really necessary? Use of board_debug_uart_init() should typically only be needed in TPL on Rockchip platform, and with ROCKCHIP_TPL being used it should be enough to use rkbin/ddrbin_tool to change uart config and just ensure correct pinctrl is used for uart node, and that the uart node is included in SPL for correct serial console use. May I suggest you try adding following to defconfig and drop this patch? # CONFIG_DEBUG_UART_BOARD_INIT is not set I would expect that should result in same/working behavior without having to add any new code. Regards, Jonas > > Cc: Quentin Schulz <foss+uboot@0leil.net> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > --- > arch/arm/mach-rockchip/rk3588/Kconfig | 10 ++++++++++ > arch/arm/mach-rockchip/rk3588/rk3588.c | 36 ++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/arch/arm/mach-rockchip/rk3588/Kconfig b/arch/arm/mach-rockchip/rk3588/Kconfig > index d7e4af31f24..cacdb0459c9 100644 > --- a/arch/arm/mach-rockchip/rk3588/Kconfig > +++ b/arch/arm/mach-rockchip/rk3588/Kconfig > @@ -221,6 +221,16 @@ config ROCKCHIP_COMMON_STACK_ADDR > config TEXT_BASE > default 0x00a00000 > > +config DEBUG_UART_CHANNEL > + int "Mux channel to use for debug UART2" > + depends on DEBUG_UART_BOARD_INIT > + default 0 > + range 0 2 > + help > + UART2 can use three different set of pins to route the output. > + For using the UART for early debugging the route to use needs > + to be declared (0, 1 or 2). > + > source board/edgeble/neural-compute-module-6/Kconfig > source board/friendlyelec/nanopc-t6-rk3588/Kconfig > source board/pine64/quartzpro64-rk3588/Kconfig > diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c > index eb65dafe3a2..e330ad6a697 100644 > --- a/arch/arm/mach-rockchip/rk3588/rk3588.c > +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c > @@ -94,9 +94,32 @@ enum { > GPIO0B6_UART2_RX_M0 = 10, > }; > > +/* GPIO3B_IOMUX_SEL_L */ > +enum { > + GPIO3B1_SHIFT = 4, > + GPIO3B1_MASK = GENMASK(7, 4), > + GPIO3B1_UART2_TX_M2 = 10, > + > + GPIO3B2_SHIFT = 8, > + GPIO3B2_MASK = GENMASK(11, 8), > + GPIO3B2_UART2_RX_M2 = 10, > +}; > + > +/* GPIO4D_IOMUX_SEL_L */ > +enum { > + GPIO4D0_SHIFT = 0, > + GPIO4D0_MASK = GENMASK(3, 0), > + GPIO4D0_UART2_TX_M1 = 10, > + > + GPIO4D1_SHIFT = 4, > + GPIO4D1_MASK = GENMASK(7, 4), > + GPIO4D1_UART2_RX_M1 = 10, > +}; > + > void board_debug_uart_init(void) > { > __maybe_unused static struct rk3588_bus_ioc * const bus_ioc = (void *)BUS_IOC_BASE; > +#if (CONFIG_DEBUG_UART_CHANNEL == 0) > static struct rk3588_pmu2_ioc * const pmu2_ioc = (void *)PMU2_IOC_BASE; > > /* Refer to BUS_IOC */ > @@ -110,6 +133,19 @@ void board_debug_uart_init(void) > GPIO0B6_MASK | GPIO0B5_MASK, > GPIO0B6_UART2_RX_M0 << GPIO0B6_SHIFT | > GPIO0B5_UART2_TX_M0 << GPIO0B5_SHIFT); > +#elif (CONFIG_DEBUG_UART_CHANNEL == 1) > + /* UART2_M1 Switch iomux */ > + rk_clrsetreg(&bus_ioc->gpio4d_iomux_sel_l, > + GPIO4D0_MASK | GPIO4D1_MASK, > + GPIO4D0_UART2_TX_M1 << GPIO4D0_UART2_TX_M1 | > + GPIO4D1_UART2_RX_M1 << GPIO4D1_SHIFT); > +#else > + /* UART2_M2 Switch iomux */ > + rk_clrsetreg(&bus_ioc->gpio3b_iomux_sel_l, > + GPIO3B1_MASK | GPIO3B2_MASK, > + GPIO3B1_UART2_TX_M2 << GPIO3B1_SHIFT | > + GPIO3B2_UART2_RX_M2 << GPIO3B2_SHIFT); > +#endif /* CONFIG_DEBUG_UART_CHANNEL */ > } > > #ifdef CONFIG_SPL_BUILD >
Hi Quentin, On 2024/4/23 00:41, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > UART2 controller is the controller in the reference design for debug > console. The default mux is M0 in that reference design. Until now, all > boards seemed to be using UART2M0 but RK3588 Tiger for example will be > using UART2M2 instead. This feature already been supported, please use CONFIG_DEBUG_UART_BASE and CONFIG_ROCKCHIP_UART_MUX_SEL_M to select your output channel. Thanks, - Kever > > Therefore, let's add support for UART2M1 and M2 as possible muxes for > the UART2 controller used as debug console. UART2M1 support was not > tested. > > The default value is M0 to match the one used currently by all devices > and the reference design. > > Cc: Quentin Schulz <foss+uboot@0leil.net> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > --- > arch/arm/mach-rockchip/rk3588/Kconfig | 10 ++++++++++ > arch/arm/mach-rockchip/rk3588/rk3588.c | 36 ++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/arch/arm/mach-rockchip/rk3588/Kconfig b/arch/arm/mach-rockchip/rk3588/Kconfig > index d7e4af31f24..cacdb0459c9 100644 > --- a/arch/arm/mach-rockchip/rk3588/Kconfig > +++ b/arch/arm/mach-rockchip/rk3588/Kconfig > @@ -221,6 +221,16 @@ config ROCKCHIP_COMMON_STACK_ADDR > config TEXT_BASE > default 0x00a00000 > > +config DEBUG_UART_CHANNEL > + int "Mux channel to use for debug UART2" > + depends on DEBUG_UART_BOARD_INIT > + default 0 > + range 0 2 > + help > + UART2 can use three different set of pins to route the output. > + For using the UART for early debugging the route to use needs > + to be declared (0, 1 or 2). > + > source board/edgeble/neural-compute-module-6/Kconfig > source board/friendlyelec/nanopc-t6-rk3588/Kconfig > source board/pine64/quartzpro64-rk3588/Kconfig > diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c > index eb65dafe3a2..e330ad6a697 100644 > --- a/arch/arm/mach-rockchip/rk3588/rk3588.c > +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c > @@ -94,9 +94,32 @@ enum { > GPIO0B6_UART2_RX_M0 = 10, > }; > > +/* GPIO3B_IOMUX_SEL_L */ > +enum { > + GPIO3B1_SHIFT = 4, > + GPIO3B1_MASK = GENMASK(7, 4), > + GPIO3B1_UART2_TX_M2 = 10, > + > + GPIO3B2_SHIFT = 8, > + GPIO3B2_MASK = GENMASK(11, 8), > + GPIO3B2_UART2_RX_M2 = 10, > +}; > + > +/* GPIO4D_IOMUX_SEL_L */ > +enum { > + GPIO4D0_SHIFT = 0, > + GPIO4D0_MASK = GENMASK(3, 0), > + GPIO4D0_UART2_TX_M1 = 10, > + > + GPIO4D1_SHIFT = 4, > + GPIO4D1_MASK = GENMASK(7, 4), > + GPIO4D1_UART2_RX_M1 = 10, > +}; > + > void board_debug_uart_init(void) > { > __maybe_unused static struct rk3588_bus_ioc * const bus_ioc = (void *)BUS_IOC_BASE; > +#if (CONFIG_DEBUG_UART_CHANNEL == 0) > static struct rk3588_pmu2_ioc * const pmu2_ioc = (void *)PMU2_IOC_BASE; > > /* Refer to BUS_IOC */ > @@ -110,6 +133,19 @@ void board_debug_uart_init(void) > GPIO0B6_MASK | GPIO0B5_MASK, > GPIO0B6_UART2_RX_M0 << GPIO0B6_SHIFT | > GPIO0B5_UART2_TX_M0 << GPIO0B5_SHIFT); > +#elif (CONFIG_DEBUG_UART_CHANNEL == 1) > + /* UART2_M1 Switch iomux */ > + rk_clrsetreg(&bus_ioc->gpio4d_iomux_sel_l, > + GPIO4D0_MASK | GPIO4D1_MASK, > + GPIO4D0_UART2_TX_M1 << GPIO4D0_UART2_TX_M1 | > + GPIO4D1_UART2_RX_M1 << GPIO4D1_SHIFT); > +#else > + /* UART2_M2 Switch iomux */ > + rk_clrsetreg(&bus_ioc->gpio3b_iomux_sel_l, > + GPIO3B1_MASK | GPIO3B2_MASK, > + GPIO3B1_UART2_TX_M2 << GPIO3B1_SHIFT | > + GPIO3B2_UART2_RX_M2 << GPIO3B2_SHIFT); > +#endif /* CONFIG_DEBUG_UART_CHANNEL */ > } > > #ifdef CONFIG_SPL_BUILD >
Hi Jonas, On 4/22/24 19:41, Jonas Karlman wrote: > Hi Quentin, > > On 2024-04-22 18:41, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> >> UART2 controller is the controller in the reference design for debug >> console. The default mux is M0 in that reference design. Until now, all >> boards seemed to be using UART2M0 but RK3588 Tiger for example will be >> using UART2M2 instead. >> >> Therefore, let's add support for UART2M1 and M2 as possible muxes for >> the UART2 controller used as debug console. UART2M1 support was not >> tested. >> >> The default value is M0 to match the one used currently by all devices >> and the reference design. > > Is this really necessary? > > Use of board_debug_uart_init() should typically only be needed in TPL on > Rockchip platform, and with ROCKCHIP_TPL being used it should be enough > to use rkbin/ddrbin_tool to change uart config and just ensure correct > pinctrl is used for uart node, and that the uart node is included in SPL > for correct serial console use. > ddrbin_tool is a blob that Rockchip refuses to provide sources of. Running a blob on the target is one thing, requiring our users to run a blob on their build machine is another thing (though I document it in the rST). However... I don't think we have another way around because I just remembered that if you have two muxes selected for the same UART controller, RX won't work. So while we would have UART output for U-Boot if Rockchip's TPL is one mux (e.g. m0, the default) and upstream U-Boot another one, we wouldn't be able to interact with it. It'll be necessary the day we have an open-source DRAM init though (I had to do this for PX30 for example). The issue is that since ddrbin_tool is a blob, it's not possible to use it in Yocto for automatically generating the appropriate ddr bin blob based on uart controller, mux and baudrate. So that will be my cross to bear. > May I suggest you try adding following to defconfig and drop this patch? > > # CONFIG_DEBUG_UART_BOARD_INIT is not set > > I would expect that should result in same/working behavior without > having to add any new code. > It does work, thanks for the suggestion, will send a v2. Cheers, Quentin
Hi Kever, On 4/23/24 03:09, Kever Yang wrote: > Hi Quentin, > > On 2024/4/23 00:41, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> >> UART2 controller is the controller in the reference design for debug >> console. The default mux is M0 in that reference design. Until now, all >> boards seemed to be using UART2M0 but RK3588 Tiger for example will be >> using UART2M2 instead. > This feature already been supported, please use CONFIG_DEBUG_UART_BASE and > CONFIG_ROCKCHIP_UART_MUX_SEL_M to select your output channel. > CONFIG_ROCKCHIP_UART_MUX_SEL_M is only available in Rockchip's downstream U-Boot. git log -p -S ROCKCHIP_UART_MUX_SEL_M returns nothing upstream, neither does git grep. I used the same mechanism as for PX30. This patch will be removed though as it's not needed until we have an open-source DRAM init. Until then we have to rely on the DDR bin to setup the UART. Cheers, Quentin
Hi Quentin, On 2024/4/23 17:31, Quentin Schulz wrote: > Hi Kever, > > On 4/23/24 03:09, Kever Yang wrote: >> Hi Quentin, >> >> On 2024/4/23 00:41, Quentin Schulz wrote: >>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>> >>> UART2 controller is the controller in the reference design for debug >>> console. The default mux is M0 in that reference design. Until now, all >>> boards seemed to be using UART2M0 but RK3588 Tiger for example will be >>> using UART2M2 instead. >> This feature already been supported, please use >> CONFIG_DEBUG_UART_BASE and >> CONFIG_ROCKCHIP_UART_MUX_SEL_M to select your output channel. >> > > CONFIG_ROCKCHIP_UART_MUX_SEL_M is only available in Rockchip's > downstream U-Boot. Sorry, my mistake, I'm searching the wrong code. I though we should already have solution for this, it should be DEBUG_UART_CHANNEL then. Thanks, - Kever > > git log -p -S ROCKCHIP_UART_MUX_SEL_M > > returns nothing upstream, neither does git grep. > > I used the same mechanism as for PX30. > > This patch will be removed though as it's not needed until we have an > open-source DRAM init. Until then we have to rely on the DDR bin to > setup the UART. > > Cheers, > Quentin
diff --git a/arch/arm/mach-rockchip/rk3588/Kconfig b/arch/arm/mach-rockchip/rk3588/Kconfig index d7e4af31f24..cacdb0459c9 100644 --- a/arch/arm/mach-rockchip/rk3588/Kconfig +++ b/arch/arm/mach-rockchip/rk3588/Kconfig @@ -221,6 +221,16 @@ config ROCKCHIP_COMMON_STACK_ADDR config TEXT_BASE default 0x00a00000 +config DEBUG_UART_CHANNEL + int "Mux channel to use for debug UART2" + depends on DEBUG_UART_BOARD_INIT + default 0 + range 0 2 + help + UART2 can use three different set of pins to route the output. + For using the UART for early debugging the route to use needs + to be declared (0, 1 or 2). + source board/edgeble/neural-compute-module-6/Kconfig source board/friendlyelec/nanopc-t6-rk3588/Kconfig source board/pine64/quartzpro64-rk3588/Kconfig diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c index eb65dafe3a2..e330ad6a697 100644 --- a/arch/arm/mach-rockchip/rk3588/rk3588.c +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c @@ -94,9 +94,32 @@ enum { GPIO0B6_UART2_RX_M0 = 10, }; +/* GPIO3B_IOMUX_SEL_L */ +enum { + GPIO3B1_SHIFT = 4, + GPIO3B1_MASK = GENMASK(7, 4), + GPIO3B1_UART2_TX_M2 = 10, + + GPIO3B2_SHIFT = 8, + GPIO3B2_MASK = GENMASK(11, 8), + GPIO3B2_UART2_RX_M2 = 10, +}; + +/* GPIO4D_IOMUX_SEL_L */ +enum { + GPIO4D0_SHIFT = 0, + GPIO4D0_MASK = GENMASK(3, 0), + GPIO4D0_UART2_TX_M1 = 10, + + GPIO4D1_SHIFT = 4, + GPIO4D1_MASK = GENMASK(7, 4), + GPIO4D1_UART2_RX_M1 = 10, +}; + void board_debug_uart_init(void) { __maybe_unused static struct rk3588_bus_ioc * const bus_ioc = (void *)BUS_IOC_BASE; +#if (CONFIG_DEBUG_UART_CHANNEL == 0) static struct rk3588_pmu2_ioc * const pmu2_ioc = (void *)PMU2_IOC_BASE; /* Refer to BUS_IOC */ @@ -110,6 +133,19 @@ void board_debug_uart_init(void) GPIO0B6_MASK | GPIO0B5_MASK, GPIO0B6_UART2_RX_M0 << GPIO0B6_SHIFT | GPIO0B5_UART2_TX_M0 << GPIO0B5_SHIFT); +#elif (CONFIG_DEBUG_UART_CHANNEL == 1) + /* UART2_M1 Switch iomux */ + rk_clrsetreg(&bus_ioc->gpio4d_iomux_sel_l, + GPIO4D0_MASK | GPIO4D1_MASK, + GPIO4D0_UART2_TX_M1 << GPIO4D0_UART2_TX_M1 | + GPIO4D1_UART2_RX_M1 << GPIO4D1_SHIFT); +#else + /* UART2_M2 Switch iomux */ + rk_clrsetreg(&bus_ioc->gpio3b_iomux_sel_l, + GPIO3B1_MASK | GPIO3B2_MASK, + GPIO3B1_UART2_TX_M2 << GPIO3B1_SHIFT | + GPIO3B2_UART2_RX_M2 << GPIO3B2_SHIFT); +#endif /* CONFIG_DEBUG_UART_CHANNEL */ } #ifdef CONFIG_SPL_BUILD