Re: [PATCH v2 4/6] clk: sunxi-ng: add support for the Allwinner H6 CCU

[Maxime Ripard]

On Wed, Feb 07, 2018 at 05:11:11PM +0800, Icenowy Zheng wrote:
> 
> 
> ä 2018å2æ7æ GMT+08:00 äå5:02:10, Maxime Ripard <maxime.ripard@xxxxxxxxxxx> åå:
> >Hi,
> >
> >On Sat, Feb 03, 2018 at 11:49:40PM +0800, Icenowy Zheng wrote:
> >> +	/* Force the output divider of video PLLs to 0 */
> >> +	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]);
> >> +	}
> >
> >Why?
> 
> According to the manual the output divider here is
> test only, and for daily use it should be 0.
> 
> >
> >> +	/* Force OHCI 12M clock sources to 00 (12MHz divided from 48MHz) */
> >> +	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]);
> >> +	}
> >
> >Why?
> 
> Just make them have a sure working parent. Why this
> muxs exist is still mysterious.
> 
> >
> >> +	/*
> >> +	 * Force the post-divider of pll-video to 8 and the output divider
> 
> oh here's a typo -- it's audio, not video.
> 
> >> +	 * of it to 1.
> >> +	 */
> >> +	val = readl(reg + SUN50I_H6_PLL_AUDIO_REG);
> >> +	val &= ~(GENMASK(21, 16) | BIT(0));
> >> +	writel(val | (7 << 16), reg + SUN50I_H6_PLL_AUDIO_REG);
> >
> >Why?
> 
> See the comments about pll-audio in the former sources :-)

You should have a comments for all those :)

> 
> >> diff --git a/include/dt-bindings/clock/sun50i-h6-ccu.h
> >b/include/dt-bindings/clock/sun50i-h6-ccu.h
> >> new file mode 100644
> >> index 000000000000..f7ddb241012c
> >> --- /dev/null
> >> +++ b/include/dt-bindings/clock/sun50i-h6-ccu.h
> >> @@ -0,0 +1,125 @@
> >> +/*
> >> + * Copyright (C) 2017 Icenowy Zheng <icenowy@xxxxxxx>
> >> + *
> >> + * SPDX-License-Identifier: (GPL-2.0+ or MIT)
> >> + */
> >> +
> >> +#ifndef _DT_BINDINGS_CLK_SUN50I_H6_H_
> >> +#define _DT_BINDINGS_CLK_SUN50I_H6_H_
> >> +
> >> +#define CLK_PLL_PERIPH0		3
> >
> >Why is that needed?
> 
> Parent of AR100 clock in PRCM.

ACK.

> >
> >> +
> >> +#define CLK_CPUX		21
> >> +
> >> +#define CLK_APB1		26
> >
> >Same thing here
> 
> As discussed in v1, the APB bus clock is used for PIO.

ACK

> >
> >> +
> >> +#define CLK_DE			29
> >> +#define CLK_BUS_DE		30
> >> +#define CLK_DEINTERLACE		31
> >> +#define CLK_BUS_DEINTERLACE	32
> >> +#define CLK_GPU			33
> >> +#define CLK_BUS_GPU		34
> >> +#define CLK_CE			35
> >> +#define CLK_BUS_CE		36
> >> +#define CLK_VE			37
> >> +#define CLK_BUS_VE		38
> >> +#define CLK_EMCE		39
> >> +#define CLK_BUS_EMCE		40
> >> +#define CLK_VP9			41
> >> +#define CLK_BUS_VP9		42
> >> +#define CLK_BUS_DMA		43
> >> +#define CLK_BUS_MSGBOX		44
> >> +#define CLK_BUS_SPINLOCK	45
> >> +#define CLK_BUS_HSTIMER		46
> >> +#define CLK_AVS			47
> >> +#define CLK_BUS_DBG		48
> >> +#define CLK_BUS_PSI		49
> >> +#define CLK_BUS_PWM		50
> >> +#define CLK_BUS_IOMMU		51
> >> +
> >> +#define CLK_MBUS_DMA		53
> >> +#define CLK_MBUS_VE		54
> >> +#define CLK_MBUS_CE		55
> >> +#define CLK_MBUS_TS		56
> >> +#define CLK_MBUS_NAND		57
> >> +#define CLK_MBUS_CSI		58
> >> +#define CLK_MBUS_DEINTERLACE	59
> >> +
> >> +#define CLK_NAND0		61
> >> +#define CLK_NAND1		62
> >> +#define CLK_BUS_NAND		63
> >> +#define CLK_MMC0		64
> >> +#define CLK_MMC1		65
> >> +#define CLK_MMC2		66
> >> +#define CLK_BUS_MMC0		67
> >> +#define CLK_BUS_MMC1		68
> >> +#define CLK_BUS_MMC2		69
> >> +#define CLK_BUS_UART0		70
> >> +#define CLK_BUS_UART1		71
> >> +#define CLK_BUS_UART2		72
> >> +#define CLK_BUS_UART3		73
> >> +#define CLK_BUS_I2C0		74
> >> +#define CLK_BUS_I2C1		75
> >> +#define CLK_BUS_I2C2		76
> >> +#define CLK_BUS_I2C3		77
> >> +#define CLK_BUS_SCR0		78
> >> +#define CLK_BUS_SCR1		79
> >> +#define CLK_SPI0		80
> >> +#define CLK_SPI1		81
> >> +#define CLK_BUS_SPI0		82
> >> +#define CLK_BUS_SPI1		83
> >> +#define CLK_BUS_EMAC		84
> >> +#define CLK_TS			85
> >> +#define CLK_BUS_TS		86
> >> +#define CLK_IR_TX		87
> >> +#define CLK_BUS_IR_TX		88
> >> +#define CLK_BUS_THS		89
> >> +#define CLK_I2S3		90
> >> +#define CLK_I2S0		91
> >> +#define CLK_I2S1		92
> >> +#define CLK_I2S2		93
> >> +#define CLK_BUS_I2S0		94
> >> +#define CLK_BUS_I2S1		95
> >> +#define CLK_BUS_I2S2		96
> >> +#define CLK_BUS_I2S3		97
> >> +#define CLK_SPDIF		98
> >> +#define CLK_BUS_SPDIF		99
> >> +#define CLK_DMIC		100
> >> +#define CLK_BUS_DMIC		101
> >> +#define CLK_AUDIO_HUB		102
> >> +#define CLK_BUS_AUDIO_HUB	103
> >> +#define CLK_USB_OHCI0		104
> >> +#define CLK_USB_PHY0		105
> >> +#define CLK_USB_PHY1		106
> >> +#define CLK_USB_OHCI3		107
> >> +#define CLK_USB_PHY3		108
> >> +#define CLK_USB_HSIC_12M	109
> >> +#define CLK_USB_HSIC		110
> >> +#define CLK_BUS_OHCI0		111
> >> +#define CLK_BUS_OHCI3		112
> >> +#define CLK_BUS_EHCI0		113
> >> +#define CLK_BUS_XHCI		114
> >> +#define CLK_BUS_EHCI3		115
> >> +#define CLK_BUS_OTG		116
> >> +#define CLK_PCIE_REF_100M	117
> >> +#define CLK_PCIE_REF		118
> >> +#define CLK_PCIE_REF_OUT	119
> >> +#define CLK_PCIE_MAXI		120
> >> +#define CLK_PCIE_AUX		121
> >
> >Dito.
> 
> I think AUX clock is used in PCIe code.
> 
> BTW the REF_100M clock is intermediate, and
> does't need to be exported. Will fix it in the next version.
> 
> >
> >> +#define CLK_BUS_PCIE		122
> >> +#define CLK_HDMI		123
> >> +#define CLK_HDMI_CEC		124
> >> +#define CLK_BUS_HDMI		125
> >> +#define CLK_BUS_TCON_TOP	126
> >
> >Ditto.
> 
> TCON_TOP is not a clock parent -- it's a dedicated hw
> part which controlls the connection between
> DE2/3 mixers and TCONs.

Ok, great, thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Attachment: signature.asc
Description: PGP signature