Re: [PATCH 07/24] clk: zte: Add ZX279128S CRM clock and reset driver

["Stefan Dösinger" <stefandoesinger@gmail.com>]

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

Hi Navid,

The big item as I see it is to split the clock and reset components into their 
respective subsystems. Yes, there are plenty of old clock drivers that handle 
resets directly in drivers/clk, but new drivers are encouraged to split them.

To handle access to the hardware from two different drivers your options are 
MFD and the aux bus. For my zx29 clocks I chose MFD because of the syscon-
reset MFD child (and Philip Zabel's preference to have one mechanism for all 3 
CRMs). For both, you will have to convert the drivers to use regmaps instead 
of direct mmio. Please see zte/clk-regmap.c for the infrastructure I put in 
place already. If there are any additions needed we can change it to 
accommodate your needs.

> + * A PLL has two registers. The output is
> + * osc * (fbdiv + frac / 2^24) / pd1 / pd2.
> + */
> +#define PLL_FBDIV		GENMASK(17, 6)
> +#define PLL_PD1			GENMASK(5, 3)
> +#define PLL_PD2			GENMASK(2, 0)
> +#define PLL_FRAC		GENMASK(23, 0)	/* second register */
> +#define PLL_FRAC_BITS		24

This looks identical to the zx29 PLLs, so you should be able to reuse the code 
in pll-zx.c. This will give you PLL enable/disable support and limited rate 
setting for free.

My PLL code doesn't handle the fractional component because my hardware 
doesn't use it (although at least one PLL supports it). I can easily add read 
support for it if you are able to test that the calculations match the 
hardware behavior.

Is there no equivalent of REFDIV (bits 23:18) on your hardware? My HW has it, 
although I think the default PLL configs always set it to 1.

> +static DEFINE_SPINLOCK(zx_top_lock);

You shouldn't need that with regmaps - regmaps bring their own lock, which 
also helps synchronization with the clock driver in case clock and reset bits 
are in the same register.

> +	/*
> +	 * The formula above gives 200 MHz for the LSP PLL, but the vendor
> +	 * kernel has it at 1 GHz, with fixed taps for the bus (250 MHz) and
> +	 * the peripherals (100 MHz). Until that is measured, it is 
registered
> +	 * with the vendor's rate.
> +	 */
> +	lsp = clk_hw_register_fixed_factor_parent_hw(NULL, "pll_lsp", osc, 0,
> +						     40, 1);

Did you check if it is programmed with the same value on the vendor kernel?

> +	/* 49.152 MHz, and the 32.768 kHz derived from it */
> +	audio = zx_top_pll(np, "pll_audio", base + TOP_PLL_AUDIO);
> +	if (IS_ERR(audio))
> +		goto err;
> +	audio_32k = clk_hw_register_fixed_factor_parent_hw(NULL, "audio_32k",
> +							   audio, 0, 
1, 1500);
> +	if (IS_ERR(audio_32k))
> +		goto err;

Deriving 32k from a PLL seems odd, although not impossible. My board has a 
fixed 32k osc, and it is the parents of most of the timers. Did you try to turn 
off the PLL and see if the 32k clock stops / changes rate? Since your usb uses 
the 32k clock for something suspend related I suspect it is *not* fed by a 
PLL. PLLs need quite a bit of power, keeping a high frequency oscillator plus 
a PLL running to produce 32KHz seems wasteful to me.

Figuring out PLL parents is a bit tricky. I did most of this work from the 
early bootloader (which is run by a separate Cortex M0 core) and can be 
configured to run on the 26 MHz osc directly. Then I could disable the PLLs and 
see which devices still work.

My board also has 4 GPIO pins that can output a clock signal. By connecting 
both GPIO and the clock, and setting the PLL to a low rate (500-1000 KHz), I 
could poll the GPIO state and count low/high transitions to see which rate was 
produced.

> +	m200 = clk_hw_register_fixed_rate(NULL, "clk_200m", NULL, 0,
> +					  200000000);
> +	if (IS_ERR(m200))
> +		goto err;
> +	m20 = clk_hw_register_fixed_factor_parent_hw(NULL, "clk_20m", m200, 
0,
> +						     1, 10);
> +	if (IS_ERR(m20))
> +		goto err;

I suspect these are children of your 1 GHz PLL.

> +	en = base + TOP_CLK_EN;
> +	hws[ZX279128S_TOP_LSP0_100M] = zx_top_gate(en, "lsp0_100m", 
lsp_100m,
> +						   13, 0);
> +	hws[ZX279128S_TOP_LSP0_32K] = zx_top_gate(en, "lsp0_32k", audio_32k,
> +						  12, 0);
> +	hws[ZX279128S_TOP_LSP0_PCLK] = zx_top_gate(en, "lsp0_pclk", pclk, 
11, 0);

I suspect the board has a few more distribution nodes like this. Try to 
identify them: Even if they are all enabled by default, understanding them 
will give you answers about clock dependencies.

This TOP_CLK_EN declaration has gaps at bits 3 and 10. This may be legitimate, 
but please test if the bits are always 0 and add a comment.

> +	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, data))
> +		goto err;
> +
> +	if (zx_top_reset_init(np, base))
> +		pr_err("%pOF: failed to register the resets\n", np);
> +	return;
> +
> +err:
> +	/* Without its timer and bus clocks the system won't boot anyway */
> +	pr_err("%pOF: failed to register the clocks\n", np);

I am missing quite a few clocks here: My board has ~15 or so proprietary 
timers and watchdogs, a DMA controller, i2c, i2s, multiple SPIs. The ZTE 
kernel may not control them directly and rely on the boot loader to enable 
them, or it may have outsourced control to them to a proprietary blob (e.g. 
whatever is driving your hardware packet processing).

> +struct zx_lsp_periph {
> +	const char *name;
> +	unsigned int reg;
> +	int wclk;			/* index of the work clock, or -1 */
> +	int pclk;			/* index of the register clock */
> +	const char *parents[2];		/* the mux inputs, or one 
parent */
> +	u8 div_width;			/* 0: no divider */
> +};

Based on your

>+#define LSP_WCLK_SEL_SHIFT     9
>+#define LSP_WCLK_DIV_SHIFT     11

I'd expect 4 possible parents at least for some peripherals.

> +struct zx_lsp_data {
> +	const struct zx_lsp_periph *periphs;
> +	unsigned int num_periphs;
> +	unsigned int num_clks;
> +	bool aclk;			/* the group has an AXI clock */
> +};

I suspect somewhere in these 32 bits you'll have one or two reset controls 
too. Look if there are set bits outside of the known pclk, wclk, mux and div 
bits. On my LSP controls it is bits 8 and 9. Bit 8 would be a good guess on 
yours, since the mux bits start at 9.

Personally I like the kind of combined description structure. Philip Zabel 
(Reset maintainer) was not too happy about it. My regmap clocks don't have 
infrastructure to handle such a combined registration, but one way or another 
we can add it. In the end I just gave up on it though and listed muxes, divs 
and gates separately.

> +	if (p->parents[1]) {
...
> +	} else if (p->div_width) {
...
> +	}

Are mux and div mutually exclusive? On my hardware they aren't - the timers 
and watchdogs on the LSP bus have both.

> +static const struct zx_lsp_periph zx_lsp0_periphs[] = {
> +	{ "uart0", 0x10, ZX279128S_LSP0_UART0_WCLK, 
ZX279128S_LSP0_UART0_PCLK,
> +	  { "wclk25m", "wclk100m" } },
> +	{ "uart1", 0x14, ZX279128S_LSP0_UART1_WCLK, 
ZX279128S_LSP0_UART1_PCLK,
> +	  { "wclk25m", "wclk100m" } },
> +	{ "spi", 0x18, ZX279128S_LSP0_SPI_WCLK, ZX279128S_LSP0_SPI_PCLK,
> +	  { "wclk100m" }, 6 },
> +	{ "gpio", 0x1c, -1, ZX279128S_LSP0_GPIO_PCLK },
> +};

I suspect offsets 0x4, 0x8 and 0xc have clocks that may not be mentioned in 
ZTE's code. You can see if it has non-zero bits on boot and if you can modify 
the registers.

On my LSP bus the devices are on 0x1000 aligned offsets (0x0 is the CRM itself, 
0x1000 the first timer with clock controls at 0x4, 0x2000 is a watchdog with 
clocks at 0x8, ...). Considering that your uart0 sits at 0x4000 and has clock 
controls at 0x10, it is likely the same.

Furthermore every device has an ID register at device offset 0 that helps to 
identify the devices. My clk-zx297520v3.c has IDs for my devices. Yours may or 
may not be the same.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 870 bytes --]