Re: [PATCH v2 1/2] sunxi: A523: Move NSI init routine into generic function

[Paul Kocialkowski <paulk@sys-base.io>]

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

Hi,

Thanks again for picking this up!

On Sun 03 May 26, 10:07, Jernej Škrabec wrote:
> Dne četrtek, 30. april 2026 ob 15:58:37 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> > In previous generations of Allwinner SoCs, the memory bus (MBUS) access
> > arbitration was configured as part of the DRAM top registers. This is no
> > longer the case with for instance the A133 or A523, which have a dedicated
> > base address for the bus arbiter that is now called NSI instead of MBUS.
> > 
> > NSI appears to be a later iteration of MBUS design, with new dedicated
> > registers that resemble the previous MBUS ones. Despite NSI not being
> > documented in the manual, the A133 BSP includes a nsi driver with some
> > description of the registers. Like previous generations, it implements
> > port arbitration priority for DRAM access and also supports an optional
> > QoS mode based on bandwidth limits.
> > 
> > In preparation for re-using code for other SoCs, factor out the existing
> > NSI init routine from the A523 DRAM code, which was a bit ad-hoc and A523
> > specific, into a separate function, and abstract the settings a bit.
> > No functional change.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Co-develeoped-by: Paul Kocialkowski <paulk@sys-base.io>
> > Suggested-by: Jernej Škrabec <jernej.skrabec@gmail.com>
> > Sponsored-by: MEC Electronics GmbH <https://www.mec.at/>
> > ---
> >  .../include/asm/arch-sunxi/cpu_sunxi_ncat2.h  |  1 +
> >  .../include/asm/arch-sunxi/dram_sun55i_a523.h | 29 +++++++++++
> >  arch/arm/include/asm/arch-sunxi/sunxi_nsi.h   | 25 ++++++++++
> >  arch/arm/mach-sunxi/Makefile                  |  2 +-
> >  arch/arm/mach-sunxi/dram_sun55i_a523.c        | 49 +++++++++----------
> >  arch/arm/mach-sunxi/sunxi_nsi.c               | 31 ++++++++++++
> >  6 files changed, 110 insertions(+), 27 deletions(-)
> >  create mode 100644 arch/arm/include/asm/arch-sunxi/sunxi_nsi.h
> >  create mode 100644 arch/arm/mach-sunxi/sunxi_nsi.c
> > 
> > diff --git a/arch/arm/include/asm/arch-sunxi/cpu_sunxi_ncat2.h b/arch/arm/include/asm/arch-sunxi/cpu_sunxi_ncat2.h
> > index 7cee7efe8b4..fd7d9b22058 100644
> > --- a/arch/arm/include/asm/arch-sunxi/cpu_sunxi_ncat2.h
> > +++ b/arch/arm/include/asm/arch-sunxi/cpu_sunxi_ncat2.h
> > @@ -9,6 +9,7 @@
> >  
> >  #define SUNXI_TZPC_BASE			0x02000800
> >  #define SUNXI_CCM_BASE			0x02001000
> > +#define SUNXI_NSI_BASE			0x02020000
> >  #define SUNXI_TIMER_BASE		0x02050000
> >  
> >  #define SUNXI_TWI0_BASE			0x02502000
> > diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun55i_a523.h b/arch/arm/include/asm/arch-sunxi/dram_sun55i_a523.h
> > index 08bfe462856..462d4726a21 100644
> > --- a/arch/arm/include/asm/arch-sunxi/dram_sun55i_a523.h
> > +++ b/arch/arm/include/asm/arch-sunxi/dram_sun55i_a523.h
> > @@ -20,6 +20,35 @@ enum sunxi_dram_type {
> >  #define MCTL_COM_UNK_008        0x008
> >  #define MCTL_COM_MAER0          0x020
> >  
> > +enum sunxi_nsi_port {
> > +	SUNXI_NSI_PORT_GPU      = 0,
> > +	SUNXI_NSI_PORT_GIC,	= 1,
> > +	SUNXI_NSI_PORT_USB3,	= 2,
> > +	SUNXI_NSI_PORT_PCIE,	= 3,
> > +	SUNXI_NSI_PORT_CE,	= 4,
> > +	SUNXI_NSI_PORT_NPU,	= 5,
> > +	SUNXI_NSI_PORT_ISP,	= 6,
> > +	SUNXI_NSI_PORT_DSP,	= 7,
> > +	SUNXI_NSI_PORT_G2D,	= 8,
> > +	SUNXI_NSI_PORT_DI,	= 9,
> > +	SUNXI_NSI_PORT_IOMMU,	= 10,
> > +	SUNXI_NSI_PORT_VE_R,	= 11,
> > +	SUNXI_NSI_PORT_VE_RW,	= 12,
> > +	SUNXI_NSI_PORT_DE,	= 13,
> > +	SUNXI_NSI_PORT_CSI,	= 14,
> > +	SUNXI_NSI_PORT_GMAC0,	= 18,
> > +	SUNXI_NSI_PORT_GMAC1,	= 19,
> > +	SUNXI_NSI_PORT_MMC0,	= 20,
> > +	SUNXI_NSI_PORT_MMC1,	= 21,
> > +	SUNXI_NSI_PORT_MMC2,	= 22,
> > +	SUNXI_NSI_PORT_USB0,	= 23,
> > +	SUNXI_NSI_PORT_USB1,	= 24,
> > +	SUNXI_NSI_PORT_USB2,	= 25,
> > +	SUNXI_NSI_PORT_NPD,	= 26,
> > +	SUNXI_NSI_PORT_DMAC,	= 27,
> > +	SUNXI_NSI_PORT_DMA,	= 28,
> > +};
> > +
> >  /*
> >   * Controller registers seems to be the same or at least very similar
> >   * to those in H6.
> > diff --git a/arch/arm/include/asm/arch-sunxi/sunxi_nsi.h b/arch/arm/include/asm/arch-sunxi/sunxi_nsi.h
> > new file mode 100644
> > index 00000000000..7d41f9318b5
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-sunxi/sunxi_nsi.h
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier:	GPL-2.0+
> > +/*
> > + * A133/A523 NSI interconnect register and constant defines
> > + *
> > + * (C) Copyright 2026 Arm Ltd.
> > + */
> > +
> > +#ifndef SUNXI_NSI_H__
> > +#define SUNXI_NSI_H__
> > +
> > +#define SUNXI_NSI_PRI_CFG_LOWEST	0
> > +#define SUNXI_NSI_PRI_CFG_LOW		1
> > +#define SUNXI_NSI_PRI_CFG_HIGH		2
> > +#define SUNXI_NSI_PRI_CFG_HIGHEST	3
> > +
> > +#define SUNXI_NSI_IO_CFG_QOS_SEL_OUTPUT	0
> > +#define SUNXI_NSI_IO_CFG_QOS_SEL_INPUT	1
> > +
> > +#define NSI_CONF(port, pri, qos_sel) \
> > +	{ SUNXI_NSI_PORT_ ## port, SUNXI_NSI_PRI_CFG_ ## pri, \
> > +	  SUNXI_NSI_IO_CFG_QOS_SEL_ ## qos_sel }
> > +
> > +void nsi_configure_port(unsigned int port, u8 pri, u8 qos_sel);
> > +
> > +#endif /* SUNXI_NSI_H__ */
> > diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> > index 0bee04d660f..3ef0113ea43 100644
> > --- a/arch/arm/mach-sunxi/Makefile
> > +++ b/arch/arm/mach-sunxi/Makefile
> > @@ -50,6 +50,6 @@ obj-$(CONFIG_DRAM_SUN50I_H616)	+= dram_sun50i_h616.o dram_dw_helpers.o
> >  obj-$(CONFIG_DRAM_SUN50I_H616)	+= dram_timings/
> >  obj-$(CONFIG_DRAM_SUN50I_A133)	+= dram_sun50i_a133.o
> >  obj-$(CONFIG_DRAM_SUN50I_A133)	+= dram_timings/
> > -obj-$(CONFIG_MACH_SUN55I_A523)	+= dram_sun55i_a523.o dram_dw_helpers.o
> > +obj-$(CONFIG_MACH_SUN55I_A523)	+= dram_sun55i_a523.o dram_dw_helpers.o sunxi_nsi.o
> >  obj-$(CONFIG_DRAM_SUN55I_A523)	+= dram_timings/
> >  endif
> > diff --git a/arch/arm/mach-sunxi/dram_sun55i_a523.c b/arch/arm/mach-sunxi/dram_sun55i_a523.c
> > index 1ffb62863e2..9fb054cea84 100644
> > --- a/arch/arm/mach-sunxi/dram_sun55i_a523.c
> > +++ b/arch/arm/mach-sunxi/dram_sun55i_a523.c
> > @@ -15,6 +15,7 @@
> >  #include <asm/arch/dram_dw_helpers.h>
> >  #include <asm/arch/cpu.h>
> >  #include <asm/arch/prcm.h>
> > +#include <asm/arch/sunxi_nsi.h>
> >  #include <linux/bitops.h>
> >  #include <linux/delay.h>
> >  
> > @@ -1412,40 +1413,36 @@ static const struct dram_para para = {
> >  	.tpr10 = CONFIG_DRAM_SUNXI_TPR10,
> >  };
> >  
> > -static void sunxi_nsi_init(void)
> > +static void nsi_set_master_priority(void)
> >  {
> > -	/* IOMMU prio 3 */
> > -	writel(0x1, 0x02021418);
> > -	writel(0xf, 0x02021414);
> > -	/* DE prio 2 */
> > -	writel(0x1, 0x02021a18);
> > -	writel(0xa, 0x02021a14);
> > -	/* VE R prio 2 */
> > -	writel(0x1, 0x02021618);
> > -	writel(0xa, 0x02021614);
> > -	/* VE RW prio 2 */
> > -	writel(0x1, 0x02021818);
> > -	writel(0xa, 0x02021814);
> > -	/* ISP prio 2 */
> > -	writel(0x1, 0x02020c18);
> > -	writel(0xa, 0x02020c14);
> > -	/* CSI prio 2 */
> > -	writel(0x1, 0x02021c18);
> > -	writel(0xa, 0x02021c14);
> > -	/* NPU prio 2 */
> > -	writel(0x1, 0x02020a18);
> > -	writel(0xa, 0x02020a14);
> > +	struct {
> > +		unsigned int port;
> > +		u8 pri;
> > +		u8 qos_sel;
> 
> What about introducing above struct in sunxi_nsi.h? I imagine same pattern
> will be used in future code.

Sounds like a good idea!

> > +	} ports[] = {
> > +		NSI_CONF(NPU,	HIGH,		INPUT),
> > +		NSI_CONF(ISP,	HIGH,		INPUT),
> > +		NSI_CONF(IOMMU,	HIGHEST,	INPUT),
> > +		NSI_CONF(VE_R,	HIGH,		INPUT),
> > +		NSI_CONF(VE_RW,	HIGH,		INPUT),
> > +		NSI_CONF(DE,	HIGH,		INPUT),
> > +		NSI_CONF(CSI,	HIGH,		INPUT),
> > +	};
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ports); i++)
> > +		nsi_configure_port(ports[i].port, ports[i].pri,
> > +				   ports[i].qos_sel);
> >  
> >  	/* close ra0 autogating */
> > -	writel(0x0, 0x02023c00);
> > +	writel(0x0, 0x02023c00);	/* port 30 */
> >  	/* close ta autogating */
> > -	writel(0x0, 0x02023e00);
> > +	writel(0x0, 0x02023e00);	/* port 31 */
> >  	/* close pcie autogating */
> >  	writel(0x0, 0x02020600);
> 
> Use SUNXI_NSI_BASE + ... above

We should probably introduce proper defines for this. From what I understand
the RA and TA are special ports but they share the same layout for the
autogating register. So something like:

	SUNXI_NSI_PORT_RA,	= 30,
	SUNXI_NSI_PORT_TA,	= 31,

Then a definition for:

#define SUNXI_NSI_AUTOGATING_REG(i)		((i) * 0x200)

And a helper like:
void nsi_configure_port_autogating(unsigned int port)

To set the reg to zero and "close" autogating.

What do you think?

> Best regards,
> Jernej
> 
> >  }
> >  
> >  static void init_something(void)
> > -
> >  {
> >  	u32 *ptr = (u32 *)0x02000804;
> >  
> > @@ -1507,7 +1504,7 @@ unsigned long sunxi_dram_init(void)
> >  
> >  	size = mctl_calc_size(&config);
> >  
> > -	sunxi_nsi_init();
> > +	nsi_set_master_priority();
> >  	init_something();
> >  
> >  	return size;
> > diff --git a/arch/arm/mach-sunxi/sunxi_nsi.c b/arch/arm/mach-sunxi/sunxi_nsi.c
> > new file mode 100644
> > index 00000000000..3d7eb43df46
> > --- /dev/null
> > +++ b/arch/arm/mach-sunxi/sunxi_nsi.c
> > @@ -0,0 +1,31 @@
> > +#include <asm/io.h>
> > +#include <asm/arch/cpu.h>
> > +#include <asm/arch/sunxi_nsi.h>
> > +
> > +#define SUNXI_NSI_MODE_REG(i)		((i) * 0x200 + 0x10)
> > +#define SUNXI_NSI_PRI_CFG_REG(i)	((i) * 0x200 + 0x14)
> > +#define SUNXI_NSI_PRI_CFG_RD(v)		(((v) & 0x3) << 2)
> > +#define SUNXI_NSI_PRI_CFG_WR(v)		((v) & 0x3)
> > +#define SUNXI_NSI_IO_CFG_REG(i)		((i) * 0x200 + 0x18)
> > +#define SUNXI_NSI_ENABLE_REG(i)		((i) * 0x200 + 0xc0)

I'd move this to the header unless there's a particular reason to not
include it there.

All the best,

Paul

> > +
> > +void nsi_configure_port(unsigned int port, u8 pri, u8 qos_sel)
> > +{
> > +	void *base = (void *)SUNXI_NSI_BASE;
> > +	u32 pri_cfg;
> > +
> > +	/* QoS with bandwidth limits is not supported, disable it. */
> > +	writel(0, base + SUNXI_NSI_MODE_REG(port));
> > +	writel(0, base + SUNXI_NSI_ENABLE_REG(port));
> > +
> > +	/*
> > +	 * QoS direction selection should not be in use, but set it nevertheless
> > +	 * to match the BSP behavior (in case it has some other meaning).
> > +	 */
> > +	writel(qos_sel, base + SUNXI_NSI_IO_CFG_REG(port));
> > +
> > +	/* Port priority is always active. */
> > +	pri_cfg = SUNXI_NSI_PRI_CFG_RD(pri) | SUNXI_NSI_PRI_CFG_WR(pri);
> > +
> > +	writel(pri_cfg, base + SUNXI_NSI_PRI_CFG_REG(port));
> > +}
> > 
> 
> 
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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