Hi 

On Tue, 13 Jan 2015, Tony Lindgren wrote:

From: Aida Mynzhasova <redacted>

This patch adds required definitions and structures for clockdomain
initialization, so omap3xxx_clockdomains_init() was substituted by
new ti81xx_clockdomains_init() while early initialization of
TI81XX platform.

This code is based on the TI81XX-LINUX-PSP-04.04.00.02 patches
published at:

http://downloads.ti.com/dsps/dsps_public_sw/psp/LinuxPSP/TI81XX_04_04/04_04_00_02/index_FDS.html

Cc: Brian Hutchinson <redacted>
Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Aida Mynzhasova <redacted>
[tony at atomide.com: updated to apply, renamed to clockdomains81xx.c
 fixed to use am33xx_clkdm_operations]
Signed-off-by: Tony Lindgren <tony@atomide.com>

A few comments below based on SPRUGX8B:

Minor comment: I guess these are to-do items; would suggest explicitly 
putting a "To-do" header on this list.

+ */
+
+/* Common TI81XX */

I can't comment on how many of these are truly common; since I haven't 
cross-checked this file against the 814x TRM.  But if you haven't had the 
chance to cross-check it against the 814x TRM either, I'd suggest starting 
by making this file explicitly 816x-specific.

+static struct clockdomain alwon_l3_slow_81xx_clkdm = {
+	.name		= "l3s_clkdm",

This should mention "alwon" in the name like the other L3 always-on 
clockdomains, since there's another L3 Slow clockdomain on this chip 
that's in the DEFAULT power domain, according to Section 18.7.6.4 
"CM_DEFAULT_L3_SLOW_CLKSTCTRL Register"

+	.pwrdm		= { .name = "alwon_pwrdm" },
+	.cm_inst	= TI81XX_CM_ALWON_MOD,
+	.clkdm_offs	= TI81XX_CM_ALWON_L3_SLOW_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-136 "CM_ALWON_L3_SLOW_CLKSTCTRL Register Field 
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's 
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain alwon_l3_med_81xx_clkdm = {
+	.name		= "alwon_l3_med_clkdm",
+	.pwrdm		= { .name = "alwon_pwrdm" },
+	.cm_inst	= TI81XX_CM_ALWON_MOD,
+	.clkdm_offs	= TI81XX_CM_ALWON_L3_MED_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-138 "CM_ALWON_L3_MED_CLKSTCTRL Register Field 
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain alwon_l3_fast_81xx_clkdm = {
+	.name		= "alwon_l3_fast_clkdm",
+	.pwrdm		= { .name = "alwon_pwrdm" },
+	.cm_inst	= TI81XX_CM_ALWON_MOD,
+	.clkdm_offs	= TI81XX_CM_ALWON_L3_FAST_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,
+};
+
+static struct clockdomain alwon_ethernet_81xx_clkdm = {
+	.name		= "alwon_ethernet_clkdm",
+	.pwrdm		= { .name = "alwon_pwrdm" },
+	.cm_inst	= TI81XX_CM_ALWON_MOD,
+	.clkdm_offs	= TI81XX_CM_ETHERNET_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-137 "CM_ETHERNET_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+/* OCMC clock domain */

Hmm I think this comment is wrong, there are two other clock domains 
labeled "OCMC".

+static struct clockdomain mmu_81xx_clkdm = {
+	.name		= "mmu_clkdm",
+	.pwrdm		= { .name = "alwon_pwrdm" },
+	.cm_inst	= TI81XX_CM_ALWON_MOD,
+	.clkdm_offs	= TI81XX_CM_MMU_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-139 "CM_MMU_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain mmu_cfg_81xx_clkdm = {
+	.name		= "mmu_cfg_clkdm",
+	.pwrdm		= { .name = "alwon_pwrdm" },
+	.cm_inst	= TI81XX_CM_ALWON_MOD,
+	.clkdm_offs	= TI81XX_CM_MMUCFG_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-140 "CM_MMUCFG_CLKSTCTRL Register Field 
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+/* TI816X only */
+static struct clockdomain alwon_mpu_816x_clkdm = {
+	.name		= "alwon_mpu_clkdm",
+	.pwrdm		= { .name = "alwon_pwrdm" },
+	.cm_inst	= TI81XX_CM_ALWON_MOD,
+	.clkdm_offs	= TI81XX_CM_ALWON_MPU_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-143 "CM_ALWON_MPU_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain active_gem_816x_clkdm = {
+	.name		= "active_gem_clkdm",
+	.pwrdm		= { .name = "active_pwrdm" },
+	.cm_inst	= TI816X_CM_ACTIVE_MOD,
+	.clkdm_offs	= TI816X_CM_ACTIVE_GEM_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-72 "CM_GEM_CLKSTCTRL Register Field Descriptions", 
only SW_SLEEP and SW_WKUP are supported.  So if that's correct, this 
should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain ivahd0_816x_clkdm = {
+	.name		= "ivahd0_clkdm",
+	.pwrdm		= { .name = "ivahd0_pwrdm" },
+	.cm_inst	= TI816X_CM_IVAHD0_MOD,
+	.clkdm_offs	= TI816X_CM_IVAHD0_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-91 "CM_IVAHD0_CLKSTCTRL Register Field 
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's 
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain ivahd1_816x_clkdm = {
+	.name		= "ivahd1_clkdm",
+	.pwrdm		= { .name = "ivahd1_pwrdm" },
+	.cm_inst	= TI816X_CM_IVAHD1_MOD,
+	.clkdm_offs	= TI816X_CM_IVAHD1_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-95 "CM_IVAHD1_CLKSTCTRL Register Field 
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's 
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain ivahd2_816x_clkdm = {
+	.name		= "ivahd2_clkdm",
+	.pwrdm		= { .name = "ivahd2_pwrdm" },
+	.cm_inst	= TI816X_CM_IVAHD2_MOD,
+	.clkdm_offs	= TI816X_CM_IVAHD2_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-99 "CM_IVAHD2_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain sgx_816x_clkdm = {
+	.name		= "sgx_clkdm",
+	.pwrdm		= { .name = "sgx_pwrdm" },
+	.cm_inst	= TI816X_CM_SGX_MOD,
+	.clkdm_offs	= TI816X_CM_SGX_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-103 "CM_SGX_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain default_l3_med_816x_clkdm = {
+	.name		= "default_l3_med_clkdm",
+	.pwrdm		= { .name = "default_pwrdm" },
+	.cm_inst	= TI816X_CM_DEFAULT_MOD,
+	.clkdm_offs	= TI816X_CM_DEFAULT_L3_MED_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-78 "CM_DEFAULT_L3_FAST_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain default_ducati_816x_clkdm = {

I can't find any mention of the Ducati in the TRM.  Does this SoC have a 
Ducati?

According to the TRM here, looks like this should just be 
"default_816x_clkdm" ?  The corresponding register is named 
CM_DEFAULT_CLKSTCTRL.  It references two clockdomains, GCLKINTR and 
GCLKIN200TR, but I can't find any other mention of those in the TRM, so, 
no idea what they're associated with.

+	.name		= "default_ducati_clkdm",
+	.pwrdm		= { .name = "default_pwrdm" },
+	.cm_inst	= TI816X_CM_DEFAULT_MOD,
+	.clkdm_offs	= TI816X_CM_DEFAULT_DUCATI_CLKDM,

(see 'Ducati' comments above)

+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-81 "CM_DEFAULT_CLKSTCTRL Register Field 
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's 
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain default_pcie_816x_clkdm = {
+	.name		= "default_pcie_clkdm",

The TRM calls this the "DEFAULT_PCI" clockdomain - would suggest sticking 
to the TRM and register name to avoid confusion.

+	.pwrdm		= { .name = "default_pwrdm" },
+	.cm_inst	= TI816X_CM_DEFAULT_MOD,
+	.clkdm_offs	= TI816X_CM_DEFAULT_PCI_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-79 "CM_DEFAULT_PCI_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain default_usb_816x_clkdm = {

The TRM calls this "DEFAULT_L3_SLOW" - would suggest sticking to the TRM 
and register name to avoid confusion.

+	.name		= "default_usb_clkdm",

As above

+	.pwrdm		= { .name = "default_pwrdm" },
+	.cm_inst	= TI816X_CM_DEFAULT_MOD,
+	.clkdm_offs	= TI816X_CM_DEFAULT_L3_SLOW_CLKDM,
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,

According to Table 18-80 "CM_DEFAULT_L3_SLOW_CLKSTCTRL Register Field
Descriptions", only SW_SLEEP and SW_WKUP are supported.  So if that's
correct, this should be CLKDM_CAN_SWSUP.

+};
+
+static struct clockdomain *clockdomains_ti81xx[] __initdata = {
+	&alwon_mpu_816x_clkdm,
+	&alwon_l3_slow_81xx_clkdm,
+	&alwon_l3_med_81xx_clkdm,
+	&alwon_l3_fast_81xx_clkdm,
+	&alwon_ethernet_81xx_clkdm,
+	&mmu_81xx_clkdm,
+	&mmu_cfg_81xx_clkdm,
+	&active_gem_816x_clkdm,
+	&ivahd0_816x_clkdm,
+	&ivahd1_816x_clkdm,
+	&ivahd2_816x_clkdm,
+	&sgx_816x_clkdm,
+	&default_l3_med_816x_clkdm,
+	&default_ducati_816x_clkdm,
+	&default_pcie_816x_clkdm,
+	&default_usb_816x_clkdm,
+	NULL,
+};

As the comment at the top of the file suggests, this is missing a lot 
of clockdomains.  The TRM lists 56 ALWON clockdomains, 13 DEFAULT 
clockdomains, 4 ACTIVE clockdomains, etc. 

Nit: please consider ordering these macros by offset, rather than 
alphabetically by name.

+
+/* ACTIVE */
+#define TI816X_CM_ACTIVE_GEM_CLKDM		0x0000
+
+/* IVAHD0 */
+#define TI816X_CM_IVAHD0_CLKDM			0x0000
+
+/* IVAHD1 */
+#define TI816X_CM_IVAHD1_CLKDM			0x0000
+
+/* IVAHD2 */
+#define TI816X_CM_IVAHD2_CLKDM			0x0000
+
+/* SGX */
+#define TI816X_CM_SGX_CLKDM			0x0000
+
+/* DEFAULT */
+#define TI816X_CM_DEFAULT_L3_MED_CLKDM		0x0004
+#define TI816X_CM_DEFAULT_DUCATI_CLKDM		0x0018

There's no Ducati clockdomain listed in the TRM, it's just 
"DEFAULT".  

+#define TI816X_CM_DEFAULT_PCI_CLKDM		0x0010
+#define TI816X_CM_DEFAULT_L3_SLOW_CLKDM		0x0014

Consider ordering these macros by offset, rather than 
alphabetically by name.

- Paul