Re: [u-boot PATCH] arm: mach-k3: am6_init: Prioritize MSMC traffic over DDR in NAVSS Northbridge

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4397 bytes --]

On Tue, Sep 07, 2021 at 09:41:23PM +0200, Jan Kiszka wrote:
> On 02.09.21 08:36, Jan Kiszka wrote:
> > On 28.07.21 11:10, Jan Kiszka wrote:
> >> On 30.01.20 09:05, Roger Quadros wrote:
> >>> NB0 is bridge to SRAM and NB1 is bridge to DDR.
> >>>
> >>> To ensure that SRAM transfers are not stalled due to
> >>> delays during DDR refreshes, SRAM traffic should be higher
> >>> priority (threadmap=2) than DDR traffic (threadmap=0).
> >>>
> >>> This patch does just that.
> >>>
> >>> This is required to fix ICSSG TX lock-ups due to delays in
> >>> MSMC transfers due to incorrect Northbridge configuration.
> >>>
> >>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >>> Acked-by: Andrew F. Davis <afd@ti.com>
> >>> Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>> Acked-by: Benoit Parrot <bparrot@ti.com>
> >>> ---
> >>>  arch/arm/mach-k3/am6_init.c                  | 14 ++++++++++++++
> >>>  arch/arm/mach-k3/include/mach/am6_hardware.h |  7 +++++++
> >>>  2 files changed, 21 insertions(+)
> >>>
> >>> diff --git a/arch/arm/mach-k3/am6_init.c b/arch/arm/mach-k3/am6_init.c
> >>> index 8d107b870b..9379b95bdb 100644
> >>> --- a/arch/arm/mach-k3/am6_init.c
> >>> +++ b/arch/arm/mach-k3/am6_init.c
> >>> @@ -86,6 +86,18 @@ static void store_boot_index_from_rom(void)
> >>>  	bootindex = *(u32 *)(CONFIG_SYS_K3_BOOT_PARAM_TABLE_INDEX);
> >>>  }
> >>>  
> >>> +static void setup_am654_navss_northbridge(void)
> >>> +{
> >>> +	/*
> >>> +	 * NB0 is bridge to SRAM and NB1 is bridge to DDR.
> >>> +	 * To ensure that SRAM transfers are not stalled due to
> >>> +	 * delays during DDR refreshes, SRAM traffic should be higher
> >>> +	 * priority (threadmap=2) than DDR traffic (threadmap=0).
> >>> +	 */
> >>> +	writel(0x2, NAVSS0_NBSS_NB0_CFG_BASE + NAVSS_NBSS_THREADMAP);
> >>> +	writel(0x0, NAVSS0_NBSS_NB1_CFG_BASE + NAVSS_NBSS_THREADMAP);
> >>> +}
> >>> +
> >>>  void board_init_f(ulong dummy)
> >>>  {
> >>>  #if defined(CONFIG_K3_LOAD_SYSFW) || defined(CONFIG_K3_AM654_DDRSS)
> >>> @@ -101,6 +113,8 @@ void board_init_f(ulong dummy)
> >>>  	/* Make all control module registers accessible */
> >>>  	ctrl_mmr_unlock();
> >>>  
> >>> +	setup_am654_navss_northbridge();
> >>> +
> >>>  #ifdef CONFIG_CPU_V7R
> >>>  	disable_linefill_optimization();
> >>>  	setup_k3_mpu_regions();
> >>> diff --git a/arch/arm/mach-k3/include/mach/am6_hardware.h b/arch/arm/mach-k3/include/mach/am6_hardware.h
> >>> index 6df7631545..45a5b31c52 100644
> >>> --- a/arch/arm/mach-k3/include/mach/am6_hardware.h
> >>> +++ b/arch/arm/mach-k3/include/mach/am6_hardware.h
> >>> @@ -47,4 +47,11 @@
> >>>  /* MCU SCRATCHPAD usage */
> >>>  #define TI_SRAM_SCRATCH_BOARD_EEPROM_START CONFIG_SYS_K3_MCU_SCRATCHPAD_BASE
> >>>  
> >>> +/* NAVSS Northbridge config */
> >>> +#define	NAVSS0_NBSS_NB0_CFG_BASE	0x03802000
> >>> +#define	NAVSS0_NBSS_NB1_CFG_BASE	0x03803000
> >>> +
> >>> +#define	NAVSS_NBSS_PID		0x0
> >>> +#define	NAVSS_NBSS_THREADMAP	0x10
> >>> +
> >>>  #endif /* __ASM_ARCH_AM6_HARDWARE_H */
> >>>
> >>
> >> This was never merged, not even commented on (only apparently rejected
> >> in patchwork) - but it is crucial as we now found out:
> >>
> >> prueth will quickly stall when these priorities are not applied, at
> >> least with SR1.0-based AM65x designs. And you probably know what else
> >> could go wrong. Please clarify and merge, possibly reducing the scope to
> >> SR1.0 if you can confirm that SR2.0 cannot be affected by design (I can
> >> only say this based on few practical experiments here).
> >>
> >> If it was good for several TI SDK releases by now, at least something
> >> similar should be good for upstream as well, I believe.
> >>
> > 
> > Ping. We need at least some confirmation on what is actually needed.
> > Then, if you do not like to add it to the generic path, it would be easy
> > for us to carry it in the IOT2050 board init only - with the appropriate
> > condition check.
> 
> Did some more experiments on SR2 silicon, and while I'm not seeing
> stalls without this patch there, iperf tests with prueth show high retry
> rates and, thus, about 10% worse throughput. So it seems to be required
> for newer silicon as well.

At this point, I'd really appreciate it if someone from within TI can
chime in here as I expect someone must suspect something here, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]