Re: [RFC PATCH 10/17] clk: sunxi: Add support for the D1 CCU

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

On Thu, 25 May 2023 18:34:45 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

thanks for staying on this!

> On 12/5/22 17:45, Andre Przywara wrote:
> > +static struct ccu_clk_gate d1_gates[] = {
> > +	[CLK_BUS_MMC0]		= GATE(0x84c, BIT(0)),
> > +	[CLK_BUS_MMC1]		= GATE(0x84c, BIT(1)),
> > +	[CLK_BUS_MMC2]		= GATE(0x84c, BIT(2)),
> > +	[CLK_BUS_UART0]		= GATE(0x90c, BIT(0)),
> > +	[CLK_BUS_UART1]		= GATE(0x90c, BIT(1)),
> > +	[CLK_BUS_UART2]		= GATE(0x90c, BIT(2)),
> > +	[CLK_BUS_UART3]		= GATE(0x90c, BIT(3)),
> > +	[CLK_BUS_UART4]		= GATE(0x90c, BIT(4)),
> > +	[CLK_BUS_UART5]		= GATE(0x90c, BIT(5)),
> > +	[CLK_BUS_I2C0]		= GATE(0x91c, BIT(0)),
> > +	[CLK_BUS_I2C1]		= GATE(0x91c, BIT(1)),
> > +	[CLK_BUS_I2C2]		= GATE(0x91c, BIT(2)),
> > +	[CLK_BUS_I2C3]		= GATE(0x91c, BIT(3)),
> > +	[CLK_SPI0]		= GATE(0x940, BIT(31)),
> > +	[CLK_SPI1]		= GATE(0x944, BIT(31)),
> > +	[CLK_BUS_SPI0]		= GATE(0x96c, BIT(0)),
> > +	[CLK_BUS_SPI1]		= GATE(0x96c, BIT(1)),
> > +
> > +	[CLK_BUS_EMAC]		= GATE(0x97c, BIT(0)),
> > +
> > +	[CLK_USB_OHCI0]		= GATE(0xa70, BIT(31)),
> > +	[CLK_USB_OHCI1]		= GATE(0xa74, BIT(31)),
> > +	[CLK_BUS_OHCI0]		= GATE(0xa8c, BIT(0)),
> > +	[CLK_BUS_OHCI1]		= GATE(0xa8c, BIT(1)),
> > +	[CLK_BUS_EHCI0]		= GATE(0xa8c, BIT(4)),
> > +	[CLK_BUS_EHCI1]		= GATE(0xa8c, BIT(5)),
> > +	[CLK_BUS_OTG]		= GATE(0xa8c, BIT(8)),
> > +	[CLK_BUS_LRADC]		= GATE(0xa9c, BIT(0)),
> > +
> > +	[CLK_RISCV]		= GATE(0xd04, BIT(31)),
> > +};  
> 
> Would it be reasonable to add (possibly one for APB1 also):
> [CLK_APB0] = GATE_DUMMY,
> 
> ...in order to suppress this warning at init:
> sunxi_set_gate: (CLK#24) unhandled

Yeah, seems like we are working in lockstep, as I found and fixed the
very same issue in the very same way on Wednesday ;-)

> As I understand it, CLK_APB0 is only for speed control and doesn't have 
> a gate, but since the FDT references it, other drivers are asking the 
> clock driver to ungate it, resulting in that (safe-to-ignore) warning.

The APB0 bus transports register accesses to a certain subset of "low
speed" peripherals, Those peripherals include the clock control unit
(CCU) itself, and the GPIO registers, so it's quite essential for
normal operation. CLK_APB0 is the clock driving that bus, and as it's
controlled via the CCU, you just cannot turn that off. Check "Figure
3-3 System bus tree" in the manual.

Linux never really touches that clock (for said reasons), and Allwinner
recommends a certain frequency, which we set up in the SPL.

So to not boil the ocean here for something that doesn't need control
anyway, we just introduced those "dummy gates", to appease U-Boot's
clock framework and avoid that warning.

Long story short: you did everything right ;-)

> PS: Do you have any plans for PSCI support, so we can get the second 
> core up too? I'd like to patch that in and include it in your series 
> once my available time permits. Just thought I'd check that you/someone 
> wasn't working on it already. :)

I checked the manuals, and it seems the required bits are documented,
but IIRC they differ from the other (much older) 32-bit parts. So it
would require some refactoring of the existing sunxi PSCI code to
accommodate the T113.
That's not really a problem, but I didn't find time yet to tackle this,
so if you want to beat me on it: be my guest.
For the basic operation SMP is not essential, so I don't want to let
that hold back the T113 U-Boot support in general. The plan is to have
an extra patch on top for PSCI, and merge that when it's ready - which
could be together with the basic support, if we get it done on time.

Cheers,
Andre