Message-ID: <a680ed50-1f21-2e16-b970-fe19329ff210@arm.com>
Date:   Wed, 2 Dec 2020 23:06:37 +0000
From:   André Przywara <andre.przywara@....com>
To:     Jernej Škrabec <jernej.skrabec@...l.net>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-sunxi@...glegroups.com,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Yangtao Li <frank@...winnertech.com>,
        linux-kernel@...r.kernel.org,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        linux-clk@...r.kernel.org,
        Clément Péron <peron.clem@...il.com>
Subject: Re: [PATCH 5/8] clk: sunxi-ng: Add support for the Allwinner H616 CCU

On 02/12/2020 21:03, Jernej Škrabec wrote:
> Dne sreda, 02. december 2020 ob 14:54:06 CET je Andre Przywara napisal(a):
>> While the clocks are fairly similar to the H6, many differ in tiny
>> details, so a separate clock driver seems indicated.
>>
>> Derived from the H6 clock driver, and adjusted according to the manual.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@....com>
>> ---
>>  drivers/clk/sunxi-ng/Kconfig                |    7 +-
>>  drivers/clk/sunxi-ng/Makefile               |    1 +
>>  drivers/clk/sunxi-ng/ccu-sun50i-h616.c      | 1134 +++++++++++++++++++
>>  drivers/clk/sunxi-ng/ccu-sun50i-h616.h      |   58 +
>>  include/dt-bindings/clock/sun50i-h616-ccu.h |  110 ++
>>  include/dt-bindings/reset/sun50i-h616-ccu.h |   67 ++
>>  6 files changed, 1376 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.c
>>  create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.h
>>  create mode 100644 include/dt-bindings/clock/sun50i-h616-ccu.h
>>  create mode 100644 include/dt-bindings/reset/sun50i-h616-ccu.h
>>
>> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
>> index ce5f5847d5d3..cd46d8853876 100644
>> --- a/drivers/clk/sunxi-ng/Kconfig
>> +++ b/drivers/clk/sunxi-ng/Kconfig

....

>> +static int sun50i_h616_ccu_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	void __iomem *reg;
>> +	u32 val;
>> +	int i;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	reg = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(reg))
>> +		return PTR_ERR(reg);
>> +
>> +	/* Enable the lock bits on all PLLs */
>> +	for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
>> +		val = readl(reg + pll_regs[i]);
>> +		val |= BIT(29);
> 
> You should also add BIT(27) here. It enables PLL output (new functionality in 
> H616). Without this only clocks which are child of PLL_CPUX and PLL_PERIPH0 
> would work (set up by U-Boot). I'm pretty sure that's not intended usage but 
> until we know better, it's ok imo.

Ah right, the output enable bit. I was wondering if the A100 solution
would better here: use bit 27 as the .enable value, and actually enable
(bit31) all PLLs here.
Or we add another field, maybe a flag, to allow the kernel to decide
which bit to use. Clement suggested something like that on IRC.
But for now I can surely just set bit 27 here as well.

>> +		writel(val, reg + pll_regs[i]);
>> +	}
>> +
>> +	/*
>> +	 * Force the output divider of video PLLs to 0.
>> +	 *
>> +	 * See the comment before pll-video0 definition for the reason.
>> +	 */
>> +	for (i = 0; i < ARRAY_SIZE(pll_video_regs); i++) {
>> +		val = readl(reg + pll_video_regs[i]);
>> +		val &= ~BIT(0);
>> +		writel(val, reg + pll_video_regs[i]);
>> +	}
>> +
>> +	/*
>> +	 * Force OHCI 12M clock sources to 00 (12MHz divided from 48MHz)
>> +	 *
>> +	 * This clock mux is still mysterious, and the code just enforces
>> +	 * it to have a valid clock parent.
>> +	 */
>> +	for (i = 0; i < ARRAY_SIZE(usb2_clk_regs); i++) {
>> +		val = readl(reg + usb2_clk_regs[i]);
>> +		val &= ~GENMASK(25, 24);
>> +		writel (val, reg + usb2_clk_regs[i]);
>> +	}
>> +
>> +	/*
>> +	 * Force the post-divider of pll-audio to 12 and the output divider
>> +	 * of it to 2, so 24576000 and 22579200 rates can be set exactly.
>> +	 */
>> +	val = readl(reg + SUN50I_H616_PLL_AUDIO_REG);
>> +	val &= ~(GENMASK(21, 16) | BIT(0));
>> +	writel(val | (11 << 16) | BIT(0), reg + SUN50I_H616_PLL_AUDIO_REG);
>> +
>> +	/*
>> +	 * First clock parent (osc32K) is unusable for CEC. But since there
>> +	 * is no good way to force parent switch (both run with same frequency),
>> +	 * just set second clock parent here.
>> +	 */
>> +	val = readl(reg + SUN50I_H616_HDMI_CEC_CLK_REG);
>> +	val |= BIT(24);
>> +	writel(val, reg + SUN50I_H616_HDMI_CEC_CLK_REG);
>> +
>> +	return sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_h616_ccu_desc);
>> +}
>> +
>> +static const struct of_device_id sun50i_h616_ccu_ids[] = {
>> +	{ .compatible = "allwinner,sun50i-h616-ccu",
>> +		.data = &sun50i_h616_ccu_desc },
>> +	{ }
>> +};
>> +
>> +static struct platform_driver sun50i_h616_ccu_driver = {
>> +	.probe	= sun50i_h616_ccu_probe,
>> +	.driver	= {
>> +		.name	= "sun50i-h616-ccu",
>> +		.of_match_table	= sun50i_h616_ccu_ids,
>> +	},
>> +};
>> +builtin_platform_driver(sun50i_h616_ccu_driver);
> 
> Please use CLK_OF_DECLARE() instead. That way clocks will be initialized 
> earlier and it will be actually possible to register both timer peripherals 
> (once DT nodes are added). If pdev or dev is ever needed, two stage 
> initialization can be made later.

Sure, will do.

Thanks for having a look!

Andre

> 
> Best regards,
> Jernej
> 
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.h b/drivers/clk/sunxi-ng/ccu-sun50i-h616.h
>> new file mode 100644
>> index 000000000000..da8f0b1206f9
>> --- /dev/null
>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.h
>> @@ -0,0 +1,58 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright 2020 Arm Ltd.
>> + */
>> +
>> +#ifndef _CCU_SUN50I_H616_H_
>> +#define _CCU_SUN50I_H616_H_
>> +
>> +#include <dt-bindings/clock/sun50i-h616-ccu.h>
>> +#include <dt-bindings/reset/sun50i-h616-ccu.h>
>> +
>> +#define CLK_OSC12M		0
>> +#define CLK_PLL_CPUX		1
>> +#define CLK_PLL_DDR0		2
>> +#define CLK_PLL_DDR1		3
>> +
>> +/* PLL_PERIPH0 exported for PRCM */
>> +
>> +#define CLK_PLL_PERIPH0_2X	5
>> +#define CLK_PLL_PERIPH0_4X	6
>> +#define CLK_PLL_PERIPH1		7
>> +#define CLK_PLL_PERIPH1_2X	8
>> +#define CLK_PLL_PERIPH1_4X	9
>> +#define CLK_PLL_GPU		10
>> +#define CLK_PLL_VIDEO0		11
>> +#define CLK_PLL_VIDEO0_4X	12
>> +#define CLK_PLL_VIDEO1		13
>> +#define CLK_PLL_VIDEO1_4X	14
>> +#define CLK_PLL_VIDEO2		15
>> +#define CLK_PLL_VIDEO2_4X	16
>> +#define CLK_PLL_VE		17
>> +#define CLK_PLL_DE		18
>> +#define CLK_PLL_AUDIO_HS	19
>> +#define CLK_PLL_AUDIO_1X	20
>> +#define CLK_PLL_AUDIO_2X	21
>> +#define CLK_PLL_AUDIO_4X	22
>> +
>> +/* CPUX clock exported for DVFS */
>> +
>> +#define CLK_AXI			24
>> +#define CLK_CPUX_APB		25
>> +#define CLK_PSI_AHB1_AHB2	26
>> +#define CLK_AHB3		27
>> +
>> +/* APB1 clock exported for PIO */
>> +
>> +#define CLK_APB2		29
>> +#define CLK_MBUS		30
>> +
>> +/* All module clocks and bus gates are exported except DRAM */
>> +
>> +#define CLK_DRAM		51
>> +
>> +#define CLK_BUS_DRAM		58
>> +
>> +#define CLK_NUMBER		(CLK_BUS_HDCP + 1)
>> +
>> +#endif /* _CCU_SUN50I_H616_H_ */
>> diff --git a/include/dt-bindings/clock/sun50i-h616-ccu.h b/include/dt-bindings/clock/sun50i-h616-ccu.h
>> new file mode 100644
>> index 000000000000..a9cc8844e3a9
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/sun50i-h616-ccu.h
>> @@ -0,0 +1,110 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
>> +/*
>> + * Copyright (C) 2020 Arm Ltd.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_CLK_SUN50I_H616_H_
>> +#define _DT_BINDINGS_CLK_SUN50I_H616_H_
>> +
>> +#define CLK_PLL_PERIPH0		4
>> +
>> +#define CLK_CPUX		23
>> +
>> +#define CLK_APB1		28
>> +
>> +#define CLK_DE			31
>> +#define CLK_BUS_DE		32
>> +#define CLK_DEINTERLACE		33
>> +#define CLK_BUS_DEINTERLACE	34
>> +#define CLK_G2D			35
>> +#define CLK_BUS_G2D		36
>> +#define CLK_GPU0		37
>> +#define CLK_BUS_GPU		38
>> +#define CLK_GPU1		39
>> +#define CLK_CE			40
>> +#define CLK_BUS_CE		41
>> +#define CLK_VE			42
>> +#define CLK_BUS_VE		43
>> +#define CLK_BUS_DMA		44
>> +#define CLK_BUS_HSTIMER		45
>> +#define CLK_AVS			46
>> +#define CLK_BUS_DBG		47
>> +#define CLK_BUS_PSI		48
>> +#define CLK_BUS_PWM		49
>> +#define CLK_BUS_IOMMU		50
>> +
>> +#define CLK_MBUS_DMA		52
>> +#define CLK_MBUS_VE		53
>> +#define CLK_MBUS_CE		54
>> +#define CLK_MBUS_TS		55
>> +#define CLK_MBUS_NAND		56
>> +#define CLK_MBUS_G2D		57
>> +
>> +#define CLK_NAND0		59
>> +#define CLK_NAND1		60
>> +#define CLK_BUS_NAND		61
>> +#define CLK_MMC0		62
>> +#define CLK_MMC1		63
>> +#define CLK_MMC2		64
>> +#define CLK_BUS_MMC0		65
>> +#define CLK_BUS_MMC1		66
>> +#define CLK_BUS_MMC2		67
>> +#define CLK_BUS_UART0		68
>> +#define CLK_BUS_UART1		69
>> +#define CLK_BUS_UART2		70
>> +#define CLK_BUS_UART3		71
>> +#define CLK_BUS_UART4		72
>> +#define CLK_BUS_UART5		73
>> +#define CLK_BUS_I2C0		74
>> +#define CLK_BUS_I2C1		75
>> +#define CLK_BUS_I2C2		76
>> +#define CLK_BUS_I2C3		77
>> +#define CLK_BUS_I2C4		78
>> +#define CLK_SPI0		79
>> +#define CLK_SPI1		80
>> +#define CLK_BUS_SPI0		81
>> +#define CLK_BUS_SPI1		82
>> +#define CLK_EMAC_25M		83
>> +#define CLK_BUS_EMAC0		84
>> +#define CLK_BUS_EMAC1		85
>> +#define CLK_TS			86
>> +#define CLK_BUS_TS		87
>> +#define CLK_BUS_THS		88
>> +#define CLK_SPDIF		89
>> +#define CLK_BUS_SPDIF		90
>> +#define CLK_DMIC		91
>> +#define CLK_BUS_DMIC		92
>> +#define CLK_AUDIO_CODEC_1X	93
>> +#define CLK_AUDIO_CODEC_4X	94
>> +#define CLK_BUS_AUDIO_CODEC	95
>> +#define CLK_AUDIO_HUB		96
>> +#define CLK_BUS_AUDIO_HUB	97
>> +#define CLK_USB_OHCI0		98
>> +#define CLK_USB_PHY0		99
>> +#define CLK_USB_OHCI1		100
>> +#define CLK_USB_PHY1		101
>> +#define CLK_USB_OHCI2		102
>> +#define CLK_USB_PHY2		103
>> +#define CLK_USB_OHCI3		104
>> +#define CLK_USB_PHY3		105
>> +#define CLK_BUS_OHCI0		106
>> +#define CLK_BUS_OHCI1		107
>> +#define CLK_BUS_OHCI2		108
>> +#define CLK_BUS_OHCI3		109
>> +#define CLK_BUS_EHCI0		110
>> +#define CLK_BUS_EHCI1		111
>> +#define CLK_BUS_EHCI2		112
>> +#define CLK_BUS_EHCI3		113
>> +#define CLK_BUS_OTG		114
>> +#define CLK_BUS_KEYADC		115
>> +#define CLK_HDMI		116
>> +#define CLK_HDMI_SLOW		117
>> +#define CLK_HDMI_CEC		118
>> +#define CLK_BUS_HDMI		119
>> +#define CLK_BUS_TCON_TOP	120
>> +#define CLK_TCON_TV0		121
>> +#define CLK_BUS_TCON_TV0	122
>> +#define CLK_HDCP		123
>> +#define CLK_BUS_HDCP		124
>> +
>> +#endif /* _DT_BINDINGS_CLK_SUN50I_H616_H_ */
>> diff --git a/include/dt-bindings/reset/sun50i-h616-ccu.h b/include/dt-bindings/reset/sun50i-h616-ccu.h
>> new file mode 100644
>> index 000000000000..1c992cfbbbab
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/sun50i-h616-ccu.h
>> @@ -0,0 +1,67 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
>> +/*
>> + * Copyright (C) 2017 Icenowy Zheng <icenowy@...c.io>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_RESET_SUN50I_H616_H_
>> +#define _DT_BINDINGS_RESET_SUN50I_H616_H_
>> +
>> +#define RST_MBUS		0
>> +#define RST_BUS_DE		1
>> +#define RST_BUS_DEINTERLACE	2
>> +#define RST_BUS_GPU		3
>> +#define RST_BUS_CE		4
>> +#define RST_BUS_VE		5
>> +#define RST_BUS_DMA		6
>> +#define RST_BUS_HSTIMER		7
>> +#define RST_BUS_DBG		8
>> +#define RST_BUS_PSI		9
>> +#define RST_BUS_PWM		10
>> +#define RST_BUS_IOMMU		11
>> +#define RST_BUS_DRAM		12
>> +#define RST_BUS_NAND		13
>> +#define RST_BUS_MMC0		14
>> +#define RST_BUS_MMC1		15
>> +#define RST_BUS_MMC2		16
>> +#define RST_BUS_UART0		17
>> +#define RST_BUS_UART1		18
>> +#define RST_BUS_UART2		19
>> +#define RST_BUS_UART3		20
>> +#define RST_BUS_UART4		21
>> +#define RST_BUS_UART5		22
>> +#define RST_BUS_I2C0		23
>> +#define RST_BUS_I2C1		24
>> +#define RST_BUS_I2C2		25
>> +#define RST_BUS_I2C3		26
>> +#define RST_BUS_I2C4		27
>> +#define RST_BUS_SPI0		28
>> +#define RST_BUS_SPI1		29
>> +#define RST_BUS_EMAC0		30
>> +#define RST_BUS_EMAC1		31
>> +#define RST_BUS_TS		32
>> +#define RST_BUS_THS		33
>> +#define RST_BUS_SPDIF		34
>> +#define RST_BUS_DMIC		35
>> +#define RST_BUS_AUDIO_CODEC	36
>> +#define RST_BUS_AUDIO_HUB	37
>> +#define RST_USB_PHY0		38
>> +#define RST_USB_PHY1		39
>> +#define RST_USB_PHY2		40
>> +#define RST_USB_PHY3		41
>> +#define RST_BUS_OHCI0		42
>> +#define RST_BUS_OHCI1		43
>> +#define RST_BUS_OHCI2		44
>> +#define RST_BUS_OHCI3		45
>> +#define RST_BUS_EHCI0		46
>> +#define RST_BUS_EHCI1		47
>> +#define RST_BUS_EHCI2		48
>> +#define RST_BUS_EHCI3		49
>> +#define RST_BUS_OTG		50
>> +#define RST_BUS_HDMI		51
>> +#define RST_BUS_HDMI_SUB	52
>> +#define RST_BUS_TCON_TOP	53
>> +#define RST_BUS_TCON_TV0	54
>> +#define RST_BUS_HDCP		55
>> +#define RST_BUS_KEYADC		56
>> +
>> +#endif /* _DT_BINDINGS_RESET_SUN50I_H616_H_ */
>> -- 
>> 2.17.5
>>
>>
>>
> 
> 

Powered by blists - more mailing lists