Re: [PATCH v2 13/15] clk: sunxi-ng: a523: add reset lines

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 18 Feb 2025 21:29:37 +0100
Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

Hi,

> Dne petek, 14. februar 2025 ob 13:53:57 Srednjeevropski standardni čas je Andre Przywara napisal(a):
> > Allwinner SoCs do not contain a separate reset controller, instead the
> > reset lines for the various devices are integrated into the "BGR" (Bus
> > Gate / Reset) registers, for each device group: one for all UARTs, one
> > for all SPI interfaces, and so on.
> > The Allwinner CCU driver also doubles as a reset provider, and since the
> > reset lines are indeed just single bits in those BGR register, we can
> > represent them easily in an array of structs, just containing the
> > register offset and the bit number.
> > 
> > Add the location of the reset bits for all devices in the A523/T527
> > SoCs, using the existing sunxi CCU infrastructure.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun55i-a523.c | 83 ++++++++++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> > 
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> > index fbed9b2b3b2f9..d57565f07a112 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> > +++ b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> > @@ -1475,11 +1475,94 @@ static struct clk_hw_onecell_data sun55i_a523_hw_clks = {
> >  	},
> >  };
> >  
> > +static struct ccu_reset_map sun55i_a523_ccu_resets[] = {
> > +	[RST_MBUS]		= { 0x540, BIT(30) },
> > +	[RST_BUS_NSI]		= { 0x54c, BIT(16) },
> > +	[RST_BUS_DE]		= { 0x60c, BIT(16) },
> > +	[RST_BUS_DI]		= { 0x62c, BIT(16) },
> > +	[RST_BUS_G2D]		= { 0x63c, BIT(16) },
> > +	[RST_BUS_SYS]		= { 0x64c, BIT(16) },
> > +	[RST_BUS_GPU]		= { 0x67c, BIT(16) },
> > +	[RST_BUS_CE]		= { 0x68c, BIT(16) },
> > +	[RST_BUS_SYS_CE]	= { 0x68c, BIT(17) },
> > +	[RST_BUS_VE]		= { 0x69c, BIT(16) },
> > +	[RST_BUS_DMA]		= { 0x70c, BIT(16) },
> > +	[RST_BUS_MSGBOX]	= { 0x71c, BIT(16) },
> > +	[RST_BUS_SPINLOCK]	= { 0x72c, BIT(16) },
> > +	[RST_BUS_CPUXTIMER]	= { 0x74c, BIT(16) },
> > +	[RST_BUS_DBG]		= { 0x78c, BIT(16) },
> > +	[RST_BUS_PWM0]		= { 0x7ac, BIT(16) },
> > +	[RST_BUS_PWM1]		= { 0x7ac, BIT(17) },
> > +	[RST_BUS_DRAM]		= { 0x80c, BIT(16) },
> > +	[RST_BUS_NAND]		= { 0x82c, BIT(16) },
> > +	[RST_BUS_MMC0]		= { 0x84c, BIT(16) },
> > +	[RST_BUS_MMC1]		= { 0x84c, BIT(17) },
> > +	[RST_BUS_MMC2]		= { 0x84c, BIT(18) },
> > +	[RST_BUS_SYSDAP]	= { 0x88c, BIT(16) },
> > +	[RST_BUS_UART0]		= { 0x90c, BIT(16) },
> > +	[RST_BUS_UART1]		= { 0x90c, BIT(17) },
> > +	[RST_BUS_UART2]		= { 0x90c, BIT(18) },
> > +	[RST_BUS_UART3]		= { 0x90c, BIT(19) },
> > +	[RST_BUS_UART4]		= { 0x90c, BIT(20) },
> > +	[RST_BUS_UART5]		= { 0x90c, BIT(21) },
> > +	[RST_BUS_UART6]		= { 0x90c, BIT(22) },
> > +	[RST_BUS_UART7]		= { 0x90c, BIT(23) },
> > +	[RST_BUS_I2C0]		= { 0x91c, BIT(16) },
> > +	[RST_BUS_I2C1]		= { 0x91c, BIT(17) },
> > +	[RST_BUS_I2C2]		= { 0x91c, BIT(18) },
> > +	[RST_BUS_I2C3]		= { 0x91c, BIT(19) },
> > +	[RST_BUS_I2C4]		= { 0x91c, BIT(20) },
> > +	[RST_BUS_I2C5]		= { 0x91c, BIT(21) },
> > +	[RST_BUS_CAN]		= { 0x92c, BIT(16) },
> > +	[RST_BUS_SPI0]		= { 0x96c, BIT(16) },
> > +	[RST_BUS_SPI1]		= { 0x96c, BIT(17) },
> > +	[RST_BUS_SPI2]		= { 0x96c, BIT(18) },
> > +	[RST_BUS_SPIFC]		= { 0x96c, BIT(19) },
> > +	[RST_BUS_EMAC0]		= { 0x97c, BIT(16) },
> > +	[RST_BUS_EMAC1]		= { 0x98c, BIT(16) | BIT(17) },	/* GMAC1-AXI */  
> 
> GMAC AXI reset should be separate.

I see where you are coming from, but what would be the advantage,
really? At the moment the generic STMMAC code and binding only knows
about one reset line, so we would need to add support for a second line
first, potentially even in generic code, but without any real win, I think.

On the other hand the reset struct supports a bit mask already, so
toggling both bits at the same time seems perfectly fine.
So to make things easier, I thought we should take advantage of that,
and having one line covering both bits. There is only one clock gate
bit for GMAC1 as well.

And I know this isn't a good argument, but the BSP does it like this as well ;-)

> 
> > +	[RST_BUS_IR_RX]		= { 0x99c, BIT(16) },
> > +	[RST_BUS_IR_TX]		= { 0x9cc, BIT(16) },
> > +	[RST_BUS_GPADC0]	= { 0x9ec, BIT(16) },
> > +	[RST_BUS_GPADC1]	= { 0x9ec, BIT(17) },
> > +	[RST_BUS_THS]		= { 0x9fc, BIT(16) },
> > +	[RST_USB_PHY0]		= { 0xa70, BIT(30) },
> > +	[RST_USB_PHY1]		= { 0xa74, BIT(30) },
> > +	[RST_BUS_OHCI0]		= { 0xa8c, BIT(16) },
> > +	[RST_BUS_OHCI1]		= { 0xa8c, BIT(17) },
> > +	[RST_BUS_EHCI0]		= { 0xa8c, BIT(20) },
> > +	[RST_BUS_EHCI1]		= { 0xa8c, BIT(21) },
> > +	[RST_BUS_OTG]		= { 0xa8c, BIT(24) },
> > +	[RST_BUS_3]		= { 0xa8c, BIT(25) },	/* BSP + register */
> > +	[RST_BUS_LRADC]		= { 0xa9c, BIT(16) },
> > +	[RST_BUS_PCIE_USB3]	= { 0xaac, BIT(16) },
> > +	[RST_BUS_DPSS_TOP]	= { 0xabc, BIT(16) },  
> 
> Docs say that there is extra display top reset at 0xacc.

Right, also the name is better there: RST_BUS_DISPLAY[01]. Fixed that.

Cheers,
Andre

> 
> > +	[RST_BUS_HDMI_MAIN]	= { 0xb1c, BIT(16) },
> > +	[RST_BUS_HDMI_SUB]	= { 0xb1c, BIT(17) },
> > +	[RST_BUS_MIPI_DSI0]	= { 0xb4c, BIT(16) },
> > +	[RST_BUS_MIPI_DSI1]	= { 0xb4c, BIT(17) },
> > +	[RST_BUS_TCON_LCD0]	= { 0xb7c, BIT(16) },
> > +	[RST_BUS_TCON_LCD1]	= { 0xb7c, BIT(17) },
> > +	[RST_BUS_TCON_LCD2]	= { 0xb7c, BIT(18) },
> > +	[RST_BUS_TCON_TV0]	= { 0xb9c, BIT(16) },
> > +	[RST_BUS_TCON_TV1]	= { 0xb9c, BIT(17) },
> > +	[RST_BUS_LVDS0]		= { 0xbac, BIT(16) },
> > +	[RST_BUS_LVDS1]		= { 0xbac, BIT(17) },
> > +	[RST_BUS_EDP]		= { 0xbbc, BIT(16) },
> > +	[RST_BUS_VIDEO_OUT0]	= { 0xbcc, BIT(16) },
> > +	[RST_BUS_VIDEO_OUT1]	= { 0xbcc, BIT(17) },
> > +	[RST_BUS_LEDC]		= { 0xbfc, BIT(16) },
> > +	[RST_BUS_CSI]		= { 0xc1c, BIT(16) },
> > +	[RST_BUS_ISP]		= { 0xc2c, BIT(16) },	/* BSP + register */
> > +};
> > +
> >  static const struct sunxi_ccu_desc sun55i_a523_ccu_desc = {
> >  	.ccu_clks	= sun55i_a523_ccu_clks,
> >  	.num_ccu_clks	= ARRAY_SIZE(sun55i_a523_ccu_clks),
> >  
> >  	.hw_clks	= &sun55i_a523_hw_clks,
> > +
> > +	.resets		= sun55i_a523_ccu_resets,
> > +	.num_resets	= ARRAY_SIZE(sun55i_a523_ccu_resets),
> >  };
> >  
> >  static const u32 pll_regs[] = {
> >   
> 
> 
> 
>