[PATCH V8 0/5] soc: imx: add scu power domain driver

[PATCH V8 0/5] soc: imx: add scu power domain driver

[A.s. Dong]

This patch set adds the scu based power domain driver.
It depends on SCU driver.

Change Log:
v7->v8:
 * update to #power-domain-cells 1 binding
v6->v7:
 * keep "fsl,scu-pd" as fall back compatible string
v5->v6:
 * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd
   accordingly
v4->v5:
 * some minor improvements according to Sascha's suggestion
   Note: did not use dev_* print functions due to we already have proper
         prefix by redefining pr_fmt. So it seems not quite neccesary
	 to pass in a struct device * pointer for debug purpose only in
	 each functions.
v3->v4:
 * only scu headfile path update

v2->v3:
 * structure and enums name update
 * api usage update


Dong Aisheng (5):
  dt-bindings: imx: add scu resource id headfile
  firmware: imx: remove resource id enums
  dt-bindings: fsl: scu: update power domain binding
  firmware: imx: add pm svc headfile
  firmware: imx: add SCU power domain driver

 .../devicetree/bindings/arm/freescale/fsl,scu.txt  |  37 +-
 drivers/firmware/imx/Kconfig                       |   6 +
 drivers/firmware/imx/Makefile                      |   3 +-
 drivers/firmware/imx/scu-pd.c                      | 302 +++++++++++
 include/dt-bindings/firmware/imx/rsrc.h            | 559 +++++++++++++++++++++
 include/linux/firmware/imx/sci.h                   |   1 +
 include/linux/firmware/imx/svc/pm.h                |  85 ++++
 include/linux/firmware/imx/types.h                 | 552 --------------------
 8 files changed, 963 insertions(+), 582 deletions(-)
 create mode 100644 drivers/firmware/imx/scu-pd.c
 create mode 100644 include/dt-bindings/firmware/imx/rsrc.h
 create mode 100644 include/linux/firmware/imx/svc/pm.h

-- 
2.7.4

[PATCH V8 1/5] dt-bindings: imx: add scu resource id headfile

[A.s. Dong]

SCU firmware uses resource id to provide services. Every device on
a SCU based system has a resource id. Exported it in device tree to
allow service bindings to use it. e.g. power domain.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v7->v8:
 * new patch
---
 include/dt-bindings/firmware/imx/rsrc.h | 559 ++++++++++++++++++++++++++++++++
 1 file changed, 559 insertions(+)
 create mode 100644 include/dt-bindings/firmware/imx/rsrc.h

diff --git a/include/dt-bindings/firmware/imx/rsrc.h b/include/dt-bindings/firmware/imx/rsrc.h
new file mode 100644
index 0000000..4481f2d
--- /dev/null
+++ b/include/dt-bindings/firmware/imx/rsrc.h
@@ -0,0 +1,559 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017-2018 NXP
+ */
+
+#ifndef __DT_BINDINGS_RSCRC_IMX_H
+#define __DT_BINDINGS_RSCRC_IMX_H
+
+/*
+ * These defines are used to indicate a resource. Resources include peripherals
+ * and bus masters (but not memory regions). Note items from list should
+ * never be changed or removed (only added to at the end of the list).
+ */
+
+#define IMX_SC_R_A53			0
+#define IMX_SC_R_A53_0			1
+#define IMX_SC_R_A53_1			2
+#define IMX_SC_R_A53_2			3
+#define IMX_SC_R_A53_3			4
+#define IMX_SC_R_A72			5
+#define IMX_SC_R_A72_0			6
+#define IMX_SC_R_A72_1			7
+#define IMX_SC_R_A72_2			8
+#define IMX_SC_R_A72_3			9
+#define IMX_SC_R_CCI			10
+#define IMX_SC_R_DB			11
+#define IMX_SC_R_DRC_0			12
+#define IMX_SC_R_DRC_1			13
+#define IMX_SC_R_GIC_SMMU		14
+#define IMX_SC_R_IRQSTR_M4_0		15
+#define IMX_SC_R_IRQSTR_M4_1		16
+#define IMX_SC_R_SMMU			17
+#define IMX_SC_R_GIC			18
+#define IMX_SC_R_DC_0_BLIT0		19
+#define IMX_SC_R_DC_0_BLIT1		20
+#define IMX_SC_R_DC_0_BLIT2		21
+#define IMX_SC_R_DC_0_BLIT_OUT		22
+#define IMX_SC_R_DC_0_CAPTURE0		23
+#define IMX_SC_R_DC_0_CAPTURE1		24
+#define IMX_SC_R_DC_0_WARP		25
+#define IMX_SC_R_DC_0_INTEGRAL0		26
+#define IMX_SC_R_DC_0_INTEGRAL1		27
+#define IMX_SC_R_DC_0_VIDEO0		28
+#define IMX_SC_R_DC_0_VIDEO1		29
+#define IMX_SC_R_DC_0_FRAC0		30
+#define IMX_SC_R_DC_0_FRAC1		31
+#define IMX_SC_R_DC_0			32
+#define IMX_SC_R_GPU_2_PID0		33
+#define IMX_SC_R_DC_0_PLL_0		34
+#define IMX_SC_R_DC_0_PLL_1		35
+#define IMX_SC_R_DC_1_BLIT0		36
+#define IMX_SC_R_DC_1_BLIT1		37
+#define IMX_SC_R_DC_1_BLIT2		38
+#define IMX_SC_R_DC_1_BLIT_OUT		39
+#define IMX_SC_R_DC_1_CAPTURE0		40
+#define IMX_SC_R_DC_1_CAPTURE1		41
+#define IMX_SC_R_DC_1_WARP		42
+#define IMX_SC_R_DC_1_INTEGRAL0		43
+#define IMX_SC_R_DC_1_INTEGRAL1		44
+#define IMX_SC_R_DC_1_VIDEO0		45
+#define IMX_SC_R_DC_1_VIDEO1		46
+#define IMX_SC_R_DC_1_FRAC0		47
+#define IMX_SC_R_DC_1_FRAC1		48
+#define IMX_SC_R_DC_1			49
+#define IMX_SC_R_GPU_3_PID0		50
+#define IMX_SC_R_DC_1_PLL_0		51
+#define IMX_SC_R_DC_1_PLL_1		52
+#define IMX_SC_R_SPI_0			53
+#define IMX_SC_R_SPI_1			54
+#define IMX_SC_R_SPI_2			55
+#define IMX_SC_R_SPI_3			56
+#define IMX_SC_R_UART_0			57
+#define IMX_SC_R_UART_1			58
+#define IMX_SC_R_UART_2			59
+#define IMX_SC_R_UART_3			60
+#define IMX_SC_R_UART_4			61
+#define IMX_SC_R_EMVSIM_0		62
+#define IMX_SC_R_EMVSIM_1		63
+#define IMX_SC_R_DMA_0_CH0		64
+#define IMX_SC_R_DMA_0_CH1		65
+#define IMX_SC_R_DMA_0_CH2		66
+#define IMX_SC_R_DMA_0_CH3		67
+#define IMX_SC_R_DMA_0_CH4		68
+#define IMX_SC_R_DMA_0_CH5		69
+#define IMX_SC_R_DMA_0_CH6		70
+#define IMX_SC_R_DMA_0_CH7		71
+#define IMX_SC_R_DMA_0_CH8		72
+#define IMX_SC_R_DMA_0_CH9		73
+#define IMX_SC_R_DMA_0_CH10		74
+#define IMX_SC_R_DMA_0_CH11		75
+#define IMX_SC_R_DMA_0_CH12		76
+#define IMX_SC_R_DMA_0_CH13		77
+#define IMX_SC_R_DMA_0_CH14		78
+#define IMX_SC_R_DMA_0_CH15		79
+#define IMX_SC_R_DMA_0_CH16		80
+#define IMX_SC_R_DMA_0_CH17		81
+#define IMX_SC_R_DMA_0_CH18		82
+#define IMX_SC_R_DMA_0_CH19		83
+#define IMX_SC_R_DMA_0_CH20		84
+#define IMX_SC_R_DMA_0_CH21		85
+#define IMX_SC_R_DMA_0_CH22		86
+#define IMX_SC_R_DMA_0_CH23		87
+#define IMX_SC_R_DMA_0_CH24		88
+#define IMX_SC_R_DMA_0_CH25		89
+#define IMX_SC_R_DMA_0_CH26		90
+#define IMX_SC_R_DMA_0_CH27		91
+#define IMX_SC_R_DMA_0_CH28		92
+#define IMX_SC_R_DMA_0_CH29		93
+#define IMX_SC_R_DMA_0_CH30		94
+#define IMX_SC_R_DMA_0_CH31		95
+#define IMX_SC_R_I2C_0			96
+#define IMX_SC_R_I2C_1			97
+#define IMX_SC_R_I2C_2			98
+#define IMX_SC_R_I2C_3			99
+#define IMX_SC_R_I2C_4			100
+#define IMX_SC_R_ADC_0			101
+#define IMX_SC_R_ADC_1			102
+#define IMX_SC_R_FTM_0			103
+#define IMX_SC_R_FTM_1			104
+#define IMX_SC_R_CAN_0			105
+#define IMX_SC_R_CAN_1			106
+#define IMX_SC_R_CAN_2			107
+#define IMX_SC_R_DMA_1_CH0		108
+#define IMX_SC_R_DMA_1_CH1		109
+#define IMX_SC_R_DMA_1_CH2		110
+#define IMX_SC_R_DMA_1_CH3		111
+#define IMX_SC_R_DMA_1_CH4		112
+#define IMX_SC_R_DMA_1_CH5		113
+#define IMX_SC_R_DMA_1_CH6		114
+#define IMX_SC_R_DMA_1_CH7		115
+#define IMX_SC_R_DMA_1_CH8		116
+#define IMX_SC_R_DMA_1_CH9		117
+#define IMX_SC_R_DMA_1_CH10		118
+#define IMX_SC_R_DMA_1_CH11		119
+#define IMX_SC_R_DMA_1_CH12		120
+#define IMX_SC_R_DMA_1_CH13		121
+#define IMX_SC_R_DMA_1_CH14		122
+#define IMX_SC_R_DMA_1_CH15		123
+#define IMX_SC_R_DMA_1_CH16		124
+#define IMX_SC_R_DMA_1_CH17		125
+#define IMX_SC_R_DMA_1_CH18		126
+#define IMX_SC_R_DMA_1_CH19		127
+#define IMX_SC_R_DMA_1_CH20		128
+#define IMX_SC_R_DMA_1_CH21		129
+#define IMX_SC_R_DMA_1_CH22		130
+#define IMX_SC_R_DMA_1_CH23		131
+#define IMX_SC_R_DMA_1_CH24		132
+#define IMX_SC_R_DMA_1_CH25		133
+#define IMX_SC_R_DMA_1_CH26		134
+#define IMX_SC_R_DMA_1_CH27		135
+#define IMX_SC_R_DMA_1_CH28		136
+#define IMX_SC_R_DMA_1_CH29		137
+#define IMX_SC_R_DMA_1_CH30		138
+#define IMX_SC_R_DMA_1_CH31		139
+#define IMX_SC_R_UNUSED1		140
+#define IMX_SC_R_UNUSED2		141
+#define IMX_SC_R_UNUSED3		142
+#define IMX_SC_R_UNUSED4		143
+#define IMX_SC_R_GPU_0_PID0		144
+#define IMX_SC_R_GPU_0_PID1		145
+#define IMX_SC_R_GPU_0_PID2		146
+#define IMX_SC_R_GPU_0_PID3		147
+#define IMX_SC_R_GPU_1_PID0		148
+#define IMX_SC_R_GPU_1_PID1		149
+#define IMX_SC_R_GPU_1_PID2		150
+#define IMX_SC_R_GPU_1_PID3		151
+#define IMX_SC_R_PCIE_A			152
+#define IMX_SC_R_SERDES_0		153
+#define IMX_SC_R_MATCH_0		154
+#define IMX_SC_R_MATCH_1		155
+#define IMX_SC_R_MATCH_2		156
+#define IMX_SC_R_MATCH_3		157
+#define IMX_SC_R_MATCH_4		158
+#define IMX_SC_R_MATCH_5		159
+#define IMX_SC_R_MATCH_6		160
+#define IMX_SC_R_MATCH_7		161
+#define IMX_SC_R_MATCH_8		162
+#define IMX_SC_R_MATCH_9		163
+#define IMX_SC_R_MATCH_10		164
+#define IMX_SC_R_MATCH_11		165
+#define IMX_SC_R_MATCH_12		166
+#define IMX_SC_R_MATCH_13		167
+#define IMX_SC_R_MATCH_14		168
+#define IMX_SC_R_PCIE_B			169
+#define IMX_SC_R_SATA_0			170
+#define IMX_SC_R_SERDES_1		171
+#define IMX_SC_R_HSIO_GPIO		172
+#define IMX_SC_R_MATCH_15		173
+#define IMX_SC_R_MATCH_16		174
+#define IMX_SC_R_MATCH_17		175
+#define IMX_SC_R_MATCH_18		176
+#define IMX_SC_R_MATCH_19		177
+#define IMX_SC_R_MATCH_20		178
+#define IMX_SC_R_MATCH_21		179
+#define IMX_SC_R_MATCH_22		180
+#define IMX_SC_R_MATCH_23		181
+#define IMX_SC_R_MATCH_24		182
+#define IMX_SC_R_MATCH_25		183
+#define IMX_SC_R_MATCH_26		184
+#define IMX_SC_R_MATCH_27		185
+#define IMX_SC_R_MATCH_28		186
+#define IMX_SC_R_LCD_0			187
+#define IMX_SC_R_LCD_0_PWM_0		188
+#define IMX_SC_R_LCD_0_I2C_0		189
+#define IMX_SC_R_LCD_0_I2C_1		190
+#define IMX_SC_R_PWM_0			191
+#define IMX_SC_R_PWM_1			192
+#define IMX_SC_R_PWM_2			193
+#define IMX_SC_R_PWM_3			194
+#define IMX_SC_R_PWM_4			195
+#define IMX_SC_R_PWM_5			196
+#define IMX_SC_R_PWM_6			197
+#define IMX_SC_R_PWM_7			198
+#define IMX_SC_R_GPIO_0			199
+#define IMX_SC_R_GPIO_1			200
+#define IMX_SC_R_GPIO_2			201
+#define IMX_SC_R_GPIO_3			202
+#define IMX_SC_R_GPIO_4			203
+#define IMX_SC_R_GPIO_5			204
+#define IMX_SC_R_GPIO_6			205
+#define IMX_SC_R_GPIO_7			206
+#define IMX_SC_R_GPT_0			207
+#define IMX_SC_R_GPT_1			208
+#define IMX_SC_R_GPT_2			209
+#define IMX_SC_R_GPT_3			210
+#define IMX_SC_R_GPT_4			211
+#define IMX_SC_R_KPP			212
+#define IMX_SC_R_MU_0A			213
+#define IMX_SC_R_MU_1A			214
+#define IMX_SC_R_MU_2A			215
+#define IMX_SC_R_MU_3A			216
+#define IMX_SC_R_MU_4A			217
+#define IMX_SC_R_MU_5A			218
+#define IMX_SC_R_MU_6A			219
+#define IMX_SC_R_MU_7A			220
+#define IMX_SC_R_MU_8A			221
+#define IMX_SC_R_MU_9A			222
+#define IMX_SC_R_MU_10A			223
+#define IMX_SC_R_MU_11A			224
+#define IMX_SC_R_MU_12A			225
+#define IMX_SC_R_MU_13A			226
+#define IMX_SC_R_MU_5B			227
+#define IMX_SC_R_MU_6B			228
+#define IMX_SC_R_MU_7B			229
+#define IMX_SC_R_MU_8B			230
+#define IMX_SC_R_MU_9B			231
+#define IMX_SC_R_MU_10B			232
+#define IMX_SC_R_MU_11B			233
+#define IMX_SC_R_MU_12B			234
+#define IMX_SC_R_MU_13B			235
+#define IMX_SC_R_ROM_0			236
+#define IMX_SC_R_FSPI_0			237
+#define IMX_SC_R_FSPI_1			238
+#define IMX_SC_R_IEE			239
+#define IMX_SC_R_IEE_R0			240
+#define IMX_SC_R_IEE_R1			241
+#define IMX_SC_R_IEE_R2			242
+#define IMX_SC_R_IEE_R3			243
+#define IMX_SC_R_IEE_R4			244
+#define IMX_SC_R_IEE_R5			245
+#define IMX_SC_R_IEE_R6			246
+#define IMX_SC_R_IEE_R7			247
+#define IMX_SC_R_SDHC_0			248
+#define IMX_SC_R_SDHC_1			249
+#define IMX_SC_R_SDHC_2			250
+#define IMX_SC_R_ENET_0			251
+#define IMX_SC_R_ENET_1			252
+#define IMX_SC_R_MLB_0			253
+#define IMX_SC_R_DMA_2_CH0		254
+#define IMX_SC_R_DMA_2_CH1		255
+#define IMX_SC_R_DMA_2_CH2		256
+#define IMX_SC_R_DMA_2_CH3		257
+#define IMX_SC_R_DMA_2_CH4		258
+#define IMX_SC_R_USB_0			259
+#define IMX_SC_R_USB_1			260
+#define IMX_SC_R_USB_0_PHY		261
+#define IMX_SC_R_USB_2			262
+#define IMX_SC_R_USB_2_PHY		263
+#define IMX_SC_R_DTCP			264
+#define IMX_SC_R_NAND			265
+#define IMX_SC_R_LVDS_0			266
+#define IMX_SC_R_LVDS_0_PWM_0		267
+#define IMX_SC_R_LVDS_0_I2C_0		268
+#define IMX_SC_R_LVDS_0_I2C_1		269
+#define IMX_SC_R_LVDS_1			270
+#define IMX_SC_R_LVDS_1_PWM_0		271
+#define IMX_SC_R_LVDS_1_I2C_0		272
+#define IMX_SC_R_LVDS_1_I2C_1		273
+#define IMX_SC_R_LVDS_2			274
+#define IMX_SC_R_LVDS_2_PWM_0		275
+#define IMX_SC_R_LVDS_2_I2C_0		276
+#define IMX_SC_R_LVDS_2_I2C_1		277
+#define IMX_SC_R_M4_0_PID0		278
+#define IMX_SC_R_M4_0_PID1		279
+#define IMX_SC_R_M4_0_PID2		280
+#define IMX_SC_R_M4_0_PID3		281
+#define IMX_SC_R_M4_0_PID4		282
+#define IMX_SC_R_M4_0_RGPIO		283
+#define IMX_SC_R_M4_0_SEMA42		284
+#define IMX_SC_R_M4_0_TPM		285
+#define IMX_SC_R_M4_0_PIT		286
+#define IMX_SC_R_M4_0_UART		287
+#define IMX_SC_R_M4_0_I2C		288
+#define IMX_SC_R_M4_0_INTMUX		289
+#define IMX_SC_R_M4_0_SIM		290
+#define IMX_SC_R_M4_0_WDOG		291
+#define IMX_SC_R_M4_0_MU_0B		292
+#define IMX_SC_R_M4_0_MU_0A0		293
+#define IMX_SC_R_M4_0_MU_0A1		294
+#define IMX_SC_R_M4_0_MU_0A2		295
+#define IMX_SC_R_M4_0_MU_0A3		296
+#define IMX_SC_R_M4_0_MU_1A		297
+#define IMX_SC_R_M4_1_PID0		298
+#define IMX_SC_R_M4_1_PID1		299
+#define IMX_SC_R_M4_1_PID2		300
+#define IMX_SC_R_M4_1_PID3		301
+#define IMX_SC_R_M4_1_PID4		302
+#define IMX_SC_R_M4_1_RGPIO		303
+#define IMX_SC_R_M4_1_SEMA42		304
+#define IMX_SC_R_M4_1_TPM		305
+#define IMX_SC_R_M4_1_PIT		306
+#define IMX_SC_R_M4_1_UART		307
+#define IMX_SC_R_M4_1_I2C		308
+#define IMX_SC_R_M4_1_INTMUX		309
+#define IMX_SC_R_M4_1_SIM		310
+#define IMX_SC_R_M4_1_WDOG		311
+#define IMX_SC_R_M4_1_MU_0B		312
+#define IMX_SC_R_M4_1_MU_0A0		313
+#define IMX_SC_R_M4_1_MU_0A1		314
+#define IMX_SC_R_M4_1_MU_0A2		315
+#define IMX_SC_R_M4_1_MU_0A3		316
+#define IMX_SC_R_M4_1_MU_1A		317
+#define IMX_SC_R_SAI_0			318
+#define IMX_SC_R_SAI_1			319
+#define IMX_SC_R_SAI_2			320
+#define IMX_SC_R_IRQSTR_SCU2		321
+#define IMX_SC_R_IRQSTR_DSP		322
+#define IMX_SC_R_ELCDIF_PLL		323
+#define IMX_SC_R_UNUSED6		324
+#define IMX_SC_R_AUDIO_PLL_0		325
+#define IMX_SC_R_PI_0			326
+#define IMX_SC_R_PI_0_PWM_0		327
+#define IMX_SC_R_PI_0_PWM_1		328
+#define IMX_SC_R_PI_0_I2C_0		329
+#define IMX_SC_R_PI_0_PLL		330
+#define IMX_SC_R_PI_1			331
+#define IMX_SC_R_PI_1_PWM_0		332
+#define IMX_SC_R_PI_1_PWM_1		333
+#define IMX_SC_R_PI_1_I2C_0		334
+#define IMX_SC_R_PI_1_PLL		335
+#define IMX_SC_R_SC_PID0		336
+#define IMX_SC_R_SC_PID1		337
+#define IMX_SC_R_SC_PID2		338
+#define IMX_SC_R_SC_PID3		339
+#define IMX_SC_R_SC_PID4		340
+#define IMX_SC_R_SC_SEMA42		341
+#define IMX_SC_R_SC_TPM			342
+#define IMX_SC_R_SC_PIT			343
+#define IMX_SC_R_SC_UART		344
+#define IMX_SC_R_SC_I2C			345
+#define IMX_SC_R_SC_MU_0B		346
+#define IMX_SC_R_SC_MU_0A0		347
+#define IMX_SC_R_SC_MU_0A1		348
+#define IMX_SC_R_SC_MU_0A2		349
+#define IMX_SC_R_SC_MU_0A3		350
+#define IMX_SC_R_SC_MU_1A		351
+#define IMX_SC_R_SYSCNT_RD		352
+#define IMX_SC_R_SYSCNT_CMP		353
+#define IMX_SC_R_DEBUG			354
+#define IMX_SC_R_SYSTEM			355
+#define IMX_SC_R_SNVS			356
+#define IMX_SC_R_OTP			357
+#define IMX_SC_R_VPU_PID0		358
+#define IMX_SC_R_VPU_PID1		359
+#define IMX_SC_R_VPU_PID2		360
+#define IMX_SC_R_VPU_PID3		361
+#define IMX_SC_R_VPU_PID4		362
+#define IMX_SC_R_VPU_PID5		363
+#define IMX_SC_R_VPU_PID6		364
+#define IMX_SC_R_VPU_PID7		365
+#define IMX_SC_R_VPU_UART		366
+#define IMX_SC_R_VPUCORE		367
+#define IMX_SC_R_VPUCORE_0		368
+#define IMX_SC_R_VPUCORE_1		369
+#define IMX_SC_R_VPUCORE_2		370
+#define IMX_SC_R_VPUCORE_3		371
+#define IMX_SC_R_DMA_4_CH0		372
+#define IMX_SC_R_DMA_4_CH1		373
+#define IMX_SC_R_DMA_4_CH2		374
+#define IMX_SC_R_DMA_4_CH3		375
+#define IMX_SC_R_DMA_4_CH4		376
+#define IMX_SC_R_ISI_CH0		377
+#define IMX_SC_R_ISI_CH1		378
+#define IMX_SC_R_ISI_CH2		379
+#define IMX_SC_R_ISI_CH3		380
+#define IMX_SC_R_ISI_CH4		381
+#define IMX_SC_R_ISI_CH5		382
+#define IMX_SC_R_ISI_CH6		383
+#define IMX_SC_R_ISI_CH7		384
+#define IMX_SC_R_MJPEG_DEC_S0		385
+#define IMX_SC_R_MJPEG_DEC_S1		386
+#define IMX_SC_R_MJPEG_DEC_S2		387
+#define IMX_SC_R_MJPEG_DEC_S3		388
+#define IMX_SC_R_MJPEG_ENC_S0		389
+#define IMX_SC_R_MJPEG_ENC_S1		390
+#define IMX_SC_R_MJPEG_ENC_S2		391
+#define IMX_SC_R_MJPEG_ENC_S3		392
+#define IMX_SC_R_MIPI_0			393
+#define IMX_SC_R_MIPI_0_PWM_0		394
+#define IMX_SC_R_MIPI_0_I2C_0		395
+#define IMX_SC_R_MIPI_0_I2C_1		396
+#define IMX_SC_R_MIPI_1			397
+#define IMX_SC_R_MIPI_1_PWM_0		398
+#define IMX_SC_R_MIPI_1_I2C_0		399
+#define IMX_SC_R_MIPI_1_I2C_1		400
+#define IMX_SC_R_CSI_0			401
+#define IMX_SC_R_CSI_0_PWM_0		402
+#define IMX_SC_R_CSI_0_I2C_0		403
+#define IMX_SC_R_CSI_1			404
+#define IMX_SC_R_CSI_1_PWM_0		405
+#define IMX_SC_R_CSI_1_I2C_0		406
+#define IMX_SC_R_HDMI			407
+#define IMX_SC_R_HDMI_I2S		408
+#define IMX_SC_R_HDMI_I2C_0		409
+#define IMX_SC_R_HDMI_PLL_0		410
+#define IMX_SC_R_HDMI_RX		411
+#define IMX_SC_R_HDMI_RX_BYPASS		412
+#define IMX_SC_R_HDMI_RX_I2C_0		413
+#define IMX_SC_R_ASRC_0			414
+#define IMX_SC_R_ESAI_0			415
+#define IMX_SC_R_SPDIF_0		416
+#define IMX_SC_R_SPDIF_1		417
+#define IMX_SC_R_SAI_3			418
+#define IMX_SC_R_SAI_4			419
+#define IMX_SC_R_SAI_5			420
+#define IMX_SC_R_GPT_5			421
+#define IMX_SC_R_GPT_6			422
+#define IMX_SC_R_GPT_7			423
+#define IMX_SC_R_GPT_8			424
+#define IMX_SC_R_GPT_9			425
+#define IMX_SC_R_GPT_10			426
+#define IMX_SC_R_DMA_2_CH5		427
+#define IMX_SC_R_DMA_2_CH6		428
+#define IMX_SC_R_DMA_2_CH7		429
+#define IMX_SC_R_DMA_2_CH8		430
+#define IMX_SC_R_DMA_2_CH9		431
+#define IMX_SC_R_DMA_2_CH10		432
+#define IMX_SC_R_DMA_2_CH11		433
+#define IMX_SC_R_DMA_2_CH12		434
+#define IMX_SC_R_DMA_2_CH13		435
+#define IMX_SC_R_DMA_2_CH14		436
+#define IMX_SC_R_DMA_2_CH15		437
+#define IMX_SC_R_DMA_2_CH16		438
+#define IMX_SC_R_DMA_2_CH17		439
+#define IMX_SC_R_DMA_2_CH18		440
+#define IMX_SC_R_DMA_2_CH19		441
+#define IMX_SC_R_DMA_2_CH20		442
+#define IMX_SC_R_DMA_2_CH21		443
+#define IMX_SC_R_DMA_2_CH22		444
+#define IMX_SC_R_DMA_2_CH23		445
+#define IMX_SC_R_DMA_2_CH24		446
+#define IMX_SC_R_DMA_2_CH25		447
+#define IMX_SC_R_DMA_2_CH26		448
+#define IMX_SC_R_DMA_2_CH27		449
+#define IMX_SC_R_DMA_2_CH28		450
+#define IMX_SC_R_DMA_2_CH29		451
+#define IMX_SC_R_DMA_2_CH30		452
+#define IMX_SC_R_DMA_2_CH31		453
+#define IMX_SC_R_ASRC_1			454
+#define IMX_SC_R_ESAI_1			455
+#define IMX_SC_R_SAI_6			456
+#define IMX_SC_R_SAI_7			457
+#define IMX_SC_R_AMIX			458
+#define IMX_SC_R_MQS_0			459
+#define IMX_SC_R_DMA_3_CH0		460
+#define IMX_SC_R_DMA_3_CH1		461
+#define IMX_SC_R_DMA_3_CH2		462
+#define IMX_SC_R_DMA_3_CH3		463
+#define IMX_SC_R_DMA_3_CH4		464
+#define IMX_SC_R_DMA_3_CH5		465
+#define IMX_SC_R_DMA_3_CH6		466
+#define IMX_SC_R_DMA_3_CH7		467
+#define IMX_SC_R_DMA_3_CH8		468
+#define IMX_SC_R_DMA_3_CH9		469
+#define IMX_SC_R_DMA_3_CH10		470
+#define IMX_SC_R_DMA_3_CH11		471
+#define IMX_SC_R_DMA_3_CH12		472
+#define IMX_SC_R_DMA_3_CH13		473
+#define IMX_SC_R_DMA_3_CH14		474
+#define IMX_SC_R_DMA_3_CH15		475
+#define IMX_SC_R_DMA_3_CH16		476
+#define IMX_SC_R_DMA_3_CH17		477
+#define IMX_SC_R_DMA_3_CH18		478
+#define IMX_SC_R_DMA_3_CH19		479
+#define IMX_SC_R_DMA_3_CH20		480
+#define IMX_SC_R_DMA_3_CH21		481
+#define IMX_SC_R_DMA_3_CH22		482
+#define IMX_SC_R_DMA_3_CH23		483
+#define IMX_SC_R_DMA_3_CH24		484
+#define IMX_SC_R_DMA_3_CH25		485
+#define IMX_SC_R_DMA_3_CH26		486
+#define IMX_SC_R_DMA_3_CH27		487
+#define IMX_SC_R_DMA_3_CH28		488
+#define IMX_SC_R_DMA_3_CH29		489
+#define IMX_SC_R_DMA_3_CH30		490
+#define IMX_SC_R_DMA_3_CH31		491
+#define IMX_SC_R_AUDIO_PLL_1		492
+#define IMX_SC_R_AUDIO_CLK_0		493
+#define IMX_SC_R_AUDIO_CLK_1		494
+#define IMX_SC_R_MCLK_OUT_0		495
+#define IMX_SC_R_MCLK_OUT_1		496
+#define IMX_SC_R_PMIC_0			497
+#define IMX_SC_R_PMIC_1			498
+#define IMX_SC_R_SECO			499
+#define IMX_SC_R_CAAM_JR1		500
+#define IMX_SC_R_CAAM_JR2		501
+#define IMX_SC_R_CAAM_JR3		502
+#define IMX_SC_R_SECO_MU_2		503
+#define IMX_SC_R_SECO_MU_3		504
+#define IMX_SC_R_SECO_MU_4		505
+#define IMX_SC_R_HDMI_RX_PWM_0		506
+#define IMX_SC_R_A35			507
+#define IMX_SC_R_A35_0			508
+#define IMX_SC_R_A35_1			509
+#define IMX_SC_R_A35_2			510
+#define IMX_SC_R_A35_3			511
+#define IMX_SC_R_DSP			512
+#define IMX_SC_R_DSP_RAM		513
+#define IMX_SC_R_CAAM_JR1_OUT		514
+#define IMX_SC_R_CAAM_JR2_OUT		515
+#define IMX_SC_R_CAAM_JR3_OUT		516
+#define IMX_SC_R_VPU_DEC_0		517
+#define IMX_SC_R_VPU_ENC_0		518
+#define IMX_SC_R_CAAM_JR0		519
+#define IMX_SC_R_CAAM_JR0_OUT		520
+#define IMX_SC_R_PMIC_2			521
+#define IMX_SC_R_DBLOGIC		522
+#define IMX_SC_R_HDMI_PLL_1		523
+#define IMX_SC_R_BOARD_R0		524
+#define IMX_SC_R_BOARD_R1		525
+#define IMX_SC_R_BOARD_R2		526
+#define IMX_SC_R_BOARD_R3		527
+#define IMX_SC_R_BOARD_R4		528
+#define IMX_SC_R_BOARD_R5		529
+#define IMX_SC_R_BOARD_R6		530
+#define IMX_SC_R_BOARD_R7		531
+#define IMX_SC_R_MJPEG_DEC_MP		532
+#define IMX_SC_R_MJPEG_ENC_MP		533
+#define IMX_SC_R_VPU_TS_0		534
+#define IMX_SC_R_VPU_MU_0		535
+#define IMX_SC_R_VPU_MU_1		536
+#define IMX_SC_R_VPU_MU_2		537
+#define IMX_SC_R_VPU_MU_3		538
+#define IMX_SC_R_VPU_ENC_1		539
+#define IMX_SC_R_VPU			540
+#define IMX_SC_R_LAST			541
+
+#endif /* __DT_BINDINGS_RSCRC_IMX_H */
-- 
2.7.4

Re: [PATCH V8 1/5] dt-bindings: imx: add scu resource id headfile

[Rob Herring]

On Sun, 28 Oct 2018 15:19:35 +0000, "A.s. Dong" wrote:
> SCU firmware uses resource id to provide services. Every device on
> a SCU based system has a resource id. Exported it in device tree to
> allow service bindings to use it. e.g. power domain.
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v7->v8:
>  * new patch
> ---
>  include/dt-bindings/firmware/imx/rsrc.h | 559 ++++++++++++++++++++++++++++++++
>  1 file changed, 559 insertions(+)
>  create mode 100644 include/dt-bindings/firmware/imx/rsrc.h
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH V8 2/5] firmware: imx: remove resource id enums

[A.s. Dong]

We already export resource id in dt-bindings headfile which can also
be used by drivers. So no need keep the same definitions in regular
headfile anymore.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v7->v8:
 * new patch
---
 include/linux/firmware/imx/types.h | 552 -------------------------------------
 1 file changed, 552 deletions(-)

diff --git a/include/linux/firmware/imx/types.h b/include/linux/firmware/imx/types.h
index 9cbf0c4..8082110 100644
--- a/include/linux/firmware/imx/types.h
+++ b/include/linux/firmware/imx/types.h
@@ -10,558 +10,6 @@
 #define _SC_TYPES_H
 
 /*
- * This type is used to indicate a resource. Resources include peripherals
- * and bus masters (but not memory regions). Note items from list should
- * never be changed or removed (only added to at the end of the list).
- */
-enum imx_sc_rsrc {
-	IMX_SC_R_A53 = 0,
-	IMX_SC_R_A53_0 = 1,
-	IMX_SC_R_A53_1 = 2,
-	IMX_SC_R_A53_2 = 3,
-	IMX_SC_R_A53_3 = 4,
-	IMX_SC_R_A72 = 5,
-	IMX_SC_R_A72_0 = 6,
-	IMX_SC_R_A72_1 = 7,
-	IMX_SC_R_A72_2 = 8,
-	IMX_SC_R_A72_3 = 9,
-	IMX_SC_R_CCI = 10,
-	IMX_SC_R_DB = 11,
-	IMX_SC_R_DRC_0 = 12,
-	IMX_SC_R_DRC_1 = 13,
-	IMX_SC_R_GIC_SMMU = 14,
-	IMX_SC_R_IRQSTR_M4_0 = 15,
-	IMX_SC_R_IRQSTR_M4_1 = 16,
-	IMX_SC_R_SMMU = 17,
-	IMX_SC_R_GIC = 18,
-	IMX_SC_R_DC_0_BLIT0 = 19,
-	IMX_SC_R_DC_0_BLIT1 = 20,
-	IMX_SC_R_DC_0_BLIT2 = 21,
-	IMX_SC_R_DC_0_BLIT_OUT = 22,
-	IMX_SC_R_DC_0_CAPTURE0 = 23,
-	IMX_SC_R_DC_0_CAPTURE1 = 24,
-	IMX_SC_R_DC_0_WARP = 25,
-	IMX_SC_R_DC_0_INTEGRAL0 = 26,
-	IMX_SC_R_DC_0_INTEGRAL1 = 27,
-	IMX_SC_R_DC_0_VIDEO0 = 28,
-	IMX_SC_R_DC_0_VIDEO1 = 29,
-	IMX_SC_R_DC_0_FRAC0 = 30,
-	IMX_SC_R_DC_0_FRAC1 = 31,
-	IMX_SC_R_DC_0 = 32,
-	IMX_SC_R_GPU_2_PID0 = 33,
-	IMX_SC_R_DC_0_PLL_0 = 34,
-	IMX_SC_R_DC_0_PLL_1 = 35,
-	IMX_SC_R_DC_1_BLIT0 = 36,
-	IMX_SC_R_DC_1_BLIT1 = 37,
-	IMX_SC_R_DC_1_BLIT2 = 38,
-	IMX_SC_R_DC_1_BLIT_OUT = 39,
-	IMX_SC_R_DC_1_CAPTURE0 = 40,
-	IMX_SC_R_DC_1_CAPTURE1 = 41,
-	IMX_SC_R_DC_1_WARP = 42,
-	IMX_SC_R_DC_1_INTEGRAL0 = 43,
-	IMX_SC_R_DC_1_INTEGRAL1 = 44,
-	IMX_SC_R_DC_1_VIDEO0 = 45,
-	IMX_SC_R_DC_1_VIDEO1 = 46,
-	IMX_SC_R_DC_1_FRAC0 = 47,
-	IMX_SC_R_DC_1_FRAC1 = 48,
-	IMX_SC_R_DC_1 = 49,
-	IMX_SC_R_GPU_3_PID0 = 50,
-	IMX_SC_R_DC_1_PLL_0 = 51,
-	IMX_SC_R_DC_1_PLL_1 = 52,
-	IMX_SC_R_SPI_0 = 53,
-	IMX_SC_R_SPI_1 = 54,
-	IMX_SC_R_SPI_2 = 55,
-	IMX_SC_R_SPI_3 = 56,
-	IMX_SC_R_UART_0 = 57,
-	IMX_SC_R_UART_1 = 58,
-	IMX_SC_R_UART_2 = 59,
-	IMX_SC_R_UART_3 = 60,
-	IMX_SC_R_UART_4 = 61,
-	IMX_SC_R_EMVSIM_0 = 62,
-	IMX_SC_R_EMVSIM_1 = 63,
-	IMX_SC_R_DMA_0_CH0 = 64,
-	IMX_SC_R_DMA_0_CH1 = 65,
-	IMX_SC_R_DMA_0_CH2 = 66,
-	IMX_SC_R_DMA_0_CH3 = 67,
-	IMX_SC_R_DMA_0_CH4 = 68,
-	IMX_SC_R_DMA_0_CH5 = 69,
-	IMX_SC_R_DMA_0_CH6 = 70,
-	IMX_SC_R_DMA_0_CH7 = 71,
-	IMX_SC_R_DMA_0_CH8 = 72,
-	IMX_SC_R_DMA_0_CH9 = 73,
-	IMX_SC_R_DMA_0_CH10 = 74,
-	IMX_SC_R_DMA_0_CH11 = 75,
-	IMX_SC_R_DMA_0_CH12 = 76,
-	IMX_SC_R_DMA_0_CH13 = 77,
-	IMX_SC_R_DMA_0_CH14 = 78,
-	IMX_SC_R_DMA_0_CH15 = 79,
-	IMX_SC_R_DMA_0_CH16 = 80,
-	IMX_SC_R_DMA_0_CH17 = 81,
-	IMX_SC_R_DMA_0_CH18 = 82,
-	IMX_SC_R_DMA_0_CH19 = 83,
-	IMX_SC_R_DMA_0_CH20 = 84,
-	IMX_SC_R_DMA_0_CH21 = 85,
-	IMX_SC_R_DMA_0_CH22 = 86,
-	IMX_SC_R_DMA_0_CH23 = 87,
-	IMX_SC_R_DMA_0_CH24 = 88,
-	IMX_SC_R_DMA_0_CH25 = 89,
-	IMX_SC_R_DMA_0_CH26 = 90,
-	IMX_SC_R_DMA_0_CH27 = 91,
-	IMX_SC_R_DMA_0_CH28 = 92,
-	IMX_SC_R_DMA_0_CH29 = 93,
-	IMX_SC_R_DMA_0_CH30 = 94,
-	IMX_SC_R_DMA_0_CH31 = 95,
-	IMX_SC_R_I2C_0 = 96,
-	IMX_SC_R_I2C_1 = 97,
-	IMX_SC_R_I2C_2 = 98,
-	IMX_SC_R_I2C_3 = 99,
-	IMX_SC_R_I2C_4 = 100,
-	IMX_SC_R_ADC_0 = 101,
-	IMX_SC_R_ADC_1 = 102,
-	IMX_SC_R_FTM_0 = 103,
-	IMX_SC_R_FTM_1 = 104,
-	IMX_SC_R_CAN_0 = 105,
-	IMX_SC_R_CAN_1 = 106,
-	IMX_SC_R_CAN_2 = 107,
-	IMX_SC_R_DMA_1_CH0 = 108,
-	IMX_SC_R_DMA_1_CH1 = 109,
-	IMX_SC_R_DMA_1_CH2 = 110,
-	IMX_SC_R_DMA_1_CH3 = 111,
-	IMX_SC_R_DMA_1_CH4 = 112,
-	IMX_SC_R_DMA_1_CH5 = 113,
-	IMX_SC_R_DMA_1_CH6 = 114,
-	IMX_SC_R_DMA_1_CH7 = 115,
-	IMX_SC_R_DMA_1_CH8 = 116,
-	IMX_SC_R_DMA_1_CH9 = 117,
-	IMX_SC_R_DMA_1_CH10 = 118,
-	IMX_SC_R_DMA_1_CH11 = 119,
-	IMX_SC_R_DMA_1_CH12 = 120,
-	IMX_SC_R_DMA_1_CH13 = 121,
-	IMX_SC_R_DMA_1_CH14 = 122,
-	IMX_SC_R_DMA_1_CH15 = 123,
-	IMX_SC_R_DMA_1_CH16 = 124,
-	IMX_SC_R_DMA_1_CH17 = 125,
-	IMX_SC_R_DMA_1_CH18 = 126,
-	IMX_SC_R_DMA_1_CH19 = 127,
-	IMX_SC_R_DMA_1_CH20 = 128,
-	IMX_SC_R_DMA_1_CH21 = 129,
-	IMX_SC_R_DMA_1_CH22 = 130,
-	IMX_SC_R_DMA_1_CH23 = 131,
-	IMX_SC_R_DMA_1_CH24 = 132,
-	IMX_SC_R_DMA_1_CH25 = 133,
-	IMX_SC_R_DMA_1_CH26 = 134,
-	IMX_SC_R_DMA_1_CH27 = 135,
-	IMX_SC_R_DMA_1_CH28 = 136,
-	IMX_SC_R_DMA_1_CH29 = 137,
-	IMX_SC_R_DMA_1_CH30 = 138,
-	IMX_SC_R_DMA_1_CH31 = 139,
-	IMX_SC_R_UNUSED1 = 140,
-	IMX_SC_R_UNUSED2 = 141,
-	IMX_SC_R_UNUSED3 = 142,
-	IMX_SC_R_UNUSED4 = 143,
-	IMX_SC_R_GPU_0_PID0 = 144,
-	IMX_SC_R_GPU_0_PID1 = 145,
-	IMX_SC_R_GPU_0_PID2 = 146,
-	IMX_SC_R_GPU_0_PID3 = 147,
-	IMX_SC_R_GPU_1_PID0 = 148,
-	IMX_SC_R_GPU_1_PID1 = 149,
-	IMX_SC_R_GPU_1_PID2 = 150,
-	IMX_SC_R_GPU_1_PID3 = 151,
-	IMX_SC_R_PCIE_A = 152,
-	IMX_SC_R_SERDES_0 = 153,
-	IMX_SC_R_MATCH_0 = 154,
-	IMX_SC_R_MATCH_1 = 155,
-	IMX_SC_R_MATCH_2 = 156,
-	IMX_SC_R_MATCH_3 = 157,
-	IMX_SC_R_MATCH_4 = 158,
-	IMX_SC_R_MATCH_5 = 159,
-	IMX_SC_R_MATCH_6 = 160,
-	IMX_SC_R_MATCH_7 = 161,
-	IMX_SC_R_MATCH_8 = 162,
-	IMX_SC_R_MATCH_9 = 163,
-	IMX_SC_R_MATCH_10 = 164,
-	IMX_SC_R_MATCH_11 = 165,
-	IMX_SC_R_MATCH_12 = 166,
-	IMX_SC_R_MATCH_13 = 167,
-	IMX_SC_R_MATCH_14 = 168,
-	IMX_SC_R_PCIE_B = 169,
-	IMX_SC_R_SATA_0 = 170,
-	IMX_SC_R_SERDES_1 = 171,
-	IMX_SC_R_HSIO_GPIO = 172,
-	IMX_SC_R_MATCH_15 = 173,
-	IMX_SC_R_MATCH_16 = 174,
-	IMX_SC_R_MATCH_17 = 175,
-	IMX_SC_R_MATCH_18 = 176,
-	IMX_SC_R_MATCH_19 = 177,
-	IMX_SC_R_MATCH_20 = 178,
-	IMX_SC_R_MATCH_21 = 179,
-	IMX_SC_R_MATCH_22 = 180,
-	IMX_SC_R_MATCH_23 = 181,
-	IMX_SC_R_MATCH_24 = 182,
-	IMX_SC_R_MATCH_25 = 183,
-	IMX_SC_R_MATCH_26 = 184,
-	IMX_SC_R_MATCH_27 = 185,
-	IMX_SC_R_MATCH_28 = 186,
-	IMX_SC_R_LCD_0 = 187,
-	IMX_SC_R_LCD_0_PWM_0 = 188,
-	IMX_SC_R_LCD_0_I2C_0 = 189,
-	IMX_SC_R_LCD_0_I2C_1 = 190,
-	IMX_SC_R_PWM_0 = 191,
-	IMX_SC_R_PWM_1 = 192,
-	IMX_SC_R_PWM_2 = 193,
-	IMX_SC_R_PWM_3 = 194,
-	IMX_SC_R_PWM_4 = 195,
-	IMX_SC_R_PWM_5 = 196,
-	IMX_SC_R_PWM_6 = 197,
-	IMX_SC_R_PWM_7 = 198,
-	IMX_SC_R_GPIO_0 = 199,
-	IMX_SC_R_GPIO_1 = 200,
-	IMX_SC_R_GPIO_2 = 201,
-	IMX_SC_R_GPIO_3 = 202,
-	IMX_SC_R_GPIO_4 = 203,
-	IMX_SC_R_GPIO_5 = 204,
-	IMX_SC_R_GPIO_6 = 205,
-	IMX_SC_R_GPIO_7 = 206,
-	IMX_SC_R_GPT_0 = 207,
-	IMX_SC_R_GPT_1 = 208,
-	IMX_SC_R_GPT_2 = 209,
-	IMX_SC_R_GPT_3 = 210,
-	IMX_SC_R_GPT_4 = 211,
-	IMX_SC_R_KPP = 212,
-	IMX_SC_R_MU_0A = 213,
-	IMX_SC_R_MU_1A = 214,
-	IMX_SC_R_MU_2A = 215,
-	IMX_SC_R_MU_3A = 216,
-	IMX_SC_R_MU_4A = 217,
-	IMX_SC_R_MU_5A = 218,
-	IMX_SC_R_MU_6A = 219,
-	IMX_SC_R_MU_7A = 220,
-	IMX_SC_R_MU_8A = 221,
-	IMX_SC_R_MU_9A = 222,
-	IMX_SC_R_MU_10A = 223,
-	IMX_SC_R_MU_11A = 224,
-	IMX_SC_R_MU_12A = 225,
-	IMX_SC_R_MU_13A = 226,
-	IMX_SC_R_MU_5B = 227,
-	IMX_SC_R_MU_6B = 228,
-	IMX_SC_R_MU_7B = 229,
-	IMX_SC_R_MU_8B = 230,
-	IMX_SC_R_MU_9B = 231,
-	IMX_SC_R_MU_10B = 232,
-	IMX_SC_R_MU_11B = 233,
-	IMX_SC_R_MU_12B = 234,
-	IMX_SC_R_MU_13B = 235,
-	IMX_SC_R_ROM_0 = 236,
-	IMX_SC_R_FSPI_0 = 237,
-	IMX_SC_R_FSPI_1 = 238,
-	IMX_SC_R_IEE = 239,
-	IMX_SC_R_IEE_R0 = 240,
-	IMX_SC_R_IEE_R1 = 241,
-	IMX_SC_R_IEE_R2 = 242,
-	IMX_SC_R_IEE_R3 = 243,
-	IMX_SC_R_IEE_R4 = 244,
-	IMX_SC_R_IEE_R5 = 245,
-	IMX_SC_R_IEE_R6 = 246,
-	IMX_SC_R_IEE_R7 = 247,
-	IMX_SC_R_SDHC_0 = 248,
-	IMX_SC_R_SDHC_1 = 249,
-	IMX_SC_R_SDHC_2 = 250,
-	IMX_SC_R_ENET_0 = 251,
-	IMX_SC_R_ENET_1 = 252,
-	IMX_SC_R_MLB_0 = 253,
-	IMX_SC_R_DMA_2_CH0 = 254,
-	IMX_SC_R_DMA_2_CH1 = 255,
-	IMX_SC_R_DMA_2_CH2 = 256,
-	IMX_SC_R_DMA_2_CH3 = 257,
-	IMX_SC_R_DMA_2_CH4 = 258,
-	IMX_SC_R_USB_0 = 259,
-	IMX_SC_R_USB_1 = 260,
-	IMX_SC_R_USB_0_PHY = 261,
-	IMX_SC_R_USB_2 = 262,
-	IMX_SC_R_USB_2_PHY = 263,
-	IMX_SC_R_DTCP = 264,
-	IMX_SC_R_NAND = 265,
-	IMX_SC_R_LVDS_0 = 266,
-	IMX_SC_R_LVDS_0_PWM_0 = 267,
-	IMX_SC_R_LVDS_0_I2C_0 = 268,
-	IMX_SC_R_LVDS_0_I2C_1 = 269,
-	IMX_SC_R_LVDS_1 = 270,
-	IMX_SC_R_LVDS_1_PWM_0 = 271,
-	IMX_SC_R_LVDS_1_I2C_0 = 272,
-	IMX_SC_R_LVDS_1_I2C_1 = 273,
-	IMX_SC_R_LVDS_2 = 274,
-	IMX_SC_R_LVDS_2_PWM_0 = 275,
-	IMX_SC_R_LVDS_2_I2C_0 = 276,
-	IMX_SC_R_LVDS_2_I2C_1 = 277,
-	IMX_SC_R_M4_0_PID0 = 278,
-	IMX_SC_R_M4_0_PID1 = 279,
-	IMX_SC_R_M4_0_PID2 = 280,
-	IMX_SC_R_M4_0_PID3 = 281,
-	IMX_SC_R_M4_0_PID4 = 282,
-	IMX_SC_R_M4_0_RGPIO = 283,
-	IMX_SC_R_M4_0_SEMA42 = 284,
-	IMX_SC_R_M4_0_TPM = 285,
-	IMX_SC_R_M4_0_PIT = 286,
-	IMX_SC_R_M4_0_UART = 287,
-	IMX_SC_R_M4_0_I2C = 288,
-	IMX_SC_R_M4_0_INTMUX = 289,
-	IMX_SC_R_M4_0_SIM = 290,
-	IMX_SC_R_M4_0_WDOG = 291,
-	IMX_SC_R_M4_0_MU_0B = 292,
-	IMX_SC_R_M4_0_MU_0A0 = 293,
-	IMX_SC_R_M4_0_MU_0A1 = 294,
-	IMX_SC_R_M4_0_MU_0A2 = 295,
-	IMX_SC_R_M4_0_MU_0A3 = 296,
-	IMX_SC_R_M4_0_MU_1A = 297,
-	IMX_SC_R_M4_1_PID0 = 298,
-	IMX_SC_R_M4_1_PID1 = 299,
-	IMX_SC_R_M4_1_PID2 = 300,
-	IMX_SC_R_M4_1_PID3 = 301,
-	IMX_SC_R_M4_1_PID4 = 302,
-	IMX_SC_R_M4_1_RGPIO = 303,
-	IMX_SC_R_M4_1_SEMA42 = 304,
-	IMX_SC_R_M4_1_TPM = 305,
-	IMX_SC_R_M4_1_PIT = 306,
-	IMX_SC_R_M4_1_UART = 307,
-	IMX_SC_R_M4_1_I2C = 308,
-	IMX_SC_R_M4_1_INTMUX = 309,
-	IMX_SC_R_M4_1_SIM = 310,
-	IMX_SC_R_M4_1_WDOG = 311,
-	IMX_SC_R_M4_1_MU_0B = 312,
-	IMX_SC_R_M4_1_MU_0A0 = 313,
-	IMX_SC_R_M4_1_MU_0A1 = 314,
-	IMX_SC_R_M4_1_MU_0A2 = 315,
-	IMX_SC_R_M4_1_MU_0A3 = 316,
-	IMX_SC_R_M4_1_MU_1A = 317,
-	IMX_SC_R_SAI_0 = 318,
-	IMX_SC_R_SAI_1 = 319,
-	IMX_SC_R_SAI_2 = 320,
-	IMX_SC_R_IRQSTR_SCU2 = 321,
-	IMX_SC_R_IRQSTR_DSP = 322,
-	IMX_SC_R_UNUSED5 = 323,
-	IMX_SC_R_UNUSED6 = 324,
-	IMX_SC_R_AUDIO_PLL_0 = 325,
-	IMX_SC_R_PI_0 = 326,
-	IMX_SC_R_PI_0_PWM_0 = 327,
-	IMX_SC_R_PI_0_PWM_1 = 328,
-	IMX_SC_R_PI_0_I2C_0 = 329,
-	IMX_SC_R_PI_0_PLL = 330,
-	IMX_SC_R_PI_1 = 331,
-	IMX_SC_R_PI_1_PWM_0 = 332,
-	IMX_SC_R_PI_1_PWM_1 = 333,
-	IMX_SC_R_PI_1_I2C_0 = 334,
-	IMX_SC_R_PI_1_PLL = 335,
-	IMX_SC_R_SC_PID0 = 336,
-	IMX_SC_R_SC_PID1 = 337,
-	IMX_SC_R_SC_PID2 = 338,
-	IMX_SC_R_SC_PID3 = 339,
-	IMX_SC_R_SC_PID4 = 340,
-	IMX_SC_R_SC_SEMA42 = 341,
-	IMX_SC_R_SC_TPM = 342,
-	IMX_SC_R_SC_PIT = 343,
-	IMX_SC_R_SC_UART = 344,
-	IMX_SC_R_SC_I2C = 345,
-	IMX_SC_R_SC_MU_0B = 346,
-	IMX_SC_R_SC_MU_0A0 = 347,
-	IMX_SC_R_SC_MU_0A1 = 348,
-	IMX_SC_R_SC_MU_0A2 = 349,
-	IMX_SC_R_SC_MU_0A3 = 350,
-	IMX_SC_R_SC_MU_1A = 351,
-	IMX_SC_R_SYSCNT_RD = 352,
-	IMX_SC_R_SYSCNT_CMP = 353,
-	IMX_SC_R_DEBUG = 354,
-	IMX_SC_R_SYSTEM = 355,
-	IMX_SC_R_SNVS = 356,
-	IMX_SC_R_OTP = 357,
-	IMX_SC_R_VPU_PID0 = 358,
-	IMX_SC_R_VPU_PID1 = 359,
-	IMX_SC_R_VPU_PID2 = 360,
-	IMX_SC_R_VPU_PID3 = 361,
-	IMX_SC_R_VPU_PID4 = 362,
-	IMX_SC_R_VPU_PID5 = 363,
-	IMX_SC_R_VPU_PID6 = 364,
-	IMX_SC_R_VPU_PID7 = 365,
-	IMX_SC_R_VPU_UART = 366,
-	IMX_SC_R_VPUCORE = 367,
-	IMX_SC_R_VPUCORE_0 = 368,
-	IMX_SC_R_VPUCORE_1 = 369,
-	IMX_SC_R_VPUCORE_2 = 370,
-	IMX_SC_R_VPUCORE_3 = 371,
-	IMX_SC_R_DMA_4_CH0 = 372,
-	IMX_SC_R_DMA_4_CH1 = 373,
-	IMX_SC_R_DMA_4_CH2 = 374,
-	IMX_SC_R_DMA_4_CH3 = 375,
-	IMX_SC_R_DMA_4_CH4 = 376,
-	IMX_SC_R_ISI_CH0 = 377,
-	IMX_SC_R_ISI_CH1 = 378,
-	IMX_SC_R_ISI_CH2 = 379,
-	IMX_SC_R_ISI_CH3 = 380,
-	IMX_SC_R_ISI_CH4 = 381,
-	IMX_SC_R_ISI_CH5 = 382,
-	IMX_SC_R_ISI_CH6 = 383,
-	IMX_SC_R_ISI_CH7 = 384,
-	IMX_SC_R_MJPEG_DEC_S0 = 385,
-	IMX_SC_R_MJPEG_DEC_S1 = 386,
-	IMX_SC_R_MJPEG_DEC_S2 = 387,
-	IMX_SC_R_MJPEG_DEC_S3 = 388,
-	IMX_SC_R_MJPEG_ENC_S0 = 389,
-	IMX_SC_R_MJPEG_ENC_S1 = 390,
-	IMX_SC_R_MJPEG_ENC_S2 = 391,
-	IMX_SC_R_MJPEG_ENC_S3 = 392,
-	IMX_SC_R_MIPI_0 = 393,
-	IMX_SC_R_MIPI_0_PWM_0 = 394,
-	IMX_SC_R_MIPI_0_I2C_0 = 395,
-	IMX_SC_R_MIPI_0_I2C_1 = 396,
-	IMX_SC_R_MIPI_1 = 397,
-	IMX_SC_R_MIPI_1_PWM_0 = 398,
-	IMX_SC_R_MIPI_1_I2C_0 = 399,
-	IMX_SC_R_MIPI_1_I2C_1 = 400,
-	IMX_SC_R_CSI_0 = 401,
-	IMX_SC_R_CSI_0_PWM_0 = 402,
-	IMX_SC_R_CSI_0_I2C_0 = 403,
-	IMX_SC_R_CSI_1 = 404,
-	IMX_SC_R_CSI_1_PWM_0 = 405,
-	IMX_SC_R_CSI_1_I2C_0 = 406,
-	IMX_SC_R_HDMI = 407,
-	IMX_SC_R_HDMI_I2S = 408,
-	IMX_SC_R_HDMI_I2C_0 = 409,
-	IMX_SC_R_HDMI_PLL_0 = 410,
-	IMX_SC_R_HDMI_RX = 411,
-	IMX_SC_R_HDMI_RX_BYPASS = 412,
-	IMX_SC_R_HDMI_RX_I2C_0 = 413,
-	IMX_SC_R_ASRC_0 = 414,
-	IMX_SC_R_ESAI_0 = 415,
-	IMX_SC_R_SPDIF_0 = 416,
-	IMX_SC_R_SPDIF_1 = 417,
-	IMX_SC_R_SAI_3 = 418,
-	IMX_SC_R_SAI_4 = 419,
-	IMX_SC_R_SAI_5 = 420,
-	IMX_SC_R_GPT_5 = 421,
-	IMX_SC_R_GPT_6 = 422,
-	IMX_SC_R_GPT_7 = 423,
-	IMX_SC_R_GPT_8 = 424,
-	IMX_SC_R_GPT_9 = 425,
-	IMX_SC_R_GPT_10 = 426,
-	IMX_SC_R_DMA_2_CH5 = 427,
-	IMX_SC_R_DMA_2_CH6 = 428,
-	IMX_SC_R_DMA_2_CH7 = 429,
-	IMX_SC_R_DMA_2_CH8 = 430,
-	IMX_SC_R_DMA_2_CH9 = 431,
-	IMX_SC_R_DMA_2_CH10 = 432,
-	IMX_SC_R_DMA_2_CH11 = 433,
-	IMX_SC_R_DMA_2_CH12 = 434,
-	IMX_SC_R_DMA_2_CH13 = 435,
-	IMX_SC_R_DMA_2_CH14 = 436,
-	IMX_SC_R_DMA_2_CH15 = 437,
-	IMX_SC_R_DMA_2_CH16 = 438,
-	IMX_SC_R_DMA_2_CH17 = 439,
-	IMX_SC_R_DMA_2_CH18 = 440,
-	IMX_SC_R_DMA_2_CH19 = 441,
-	IMX_SC_R_DMA_2_CH20 = 442,
-	IMX_SC_R_DMA_2_CH21 = 443,
-	IMX_SC_R_DMA_2_CH22 = 444,
-	IMX_SC_R_DMA_2_CH23 = 445,
-	IMX_SC_R_DMA_2_CH24 = 446,
-	IMX_SC_R_DMA_2_CH25 = 447,
-	IMX_SC_R_DMA_2_CH26 = 448,
-	IMX_SC_R_DMA_2_CH27 = 449,
-	IMX_SC_R_DMA_2_CH28 = 450,
-	IMX_SC_R_DMA_2_CH29 = 451,
-	IMX_SC_R_DMA_2_CH30 = 452,
-	IMX_SC_R_DMA_2_CH31 = 453,
-	IMX_SC_R_ASRC_1 = 454,
-	IMX_SC_R_ESAI_1 = 455,
-	IMX_SC_R_SAI_6 = 456,
-	IMX_SC_R_SAI_7 = 457,
-	IMX_SC_R_AMIX = 458,
-	IMX_SC_R_MQS_0 = 459,
-	IMX_SC_R_DMA_3_CH0 = 460,
-	IMX_SC_R_DMA_3_CH1 = 461,
-	IMX_SC_R_DMA_3_CH2 = 462,
-	IMX_SC_R_DMA_3_CH3 = 463,
-	IMX_SC_R_DMA_3_CH4 = 464,
-	IMX_SC_R_DMA_3_CH5 = 465,
-	IMX_SC_R_DMA_3_CH6 = 466,
-	IMX_SC_R_DMA_3_CH7 = 467,
-	IMX_SC_R_DMA_3_CH8 = 468,
-	IMX_SC_R_DMA_3_CH9 = 469,
-	IMX_SC_R_DMA_3_CH10 = 470,
-	IMX_SC_R_DMA_3_CH11 = 471,
-	IMX_SC_R_DMA_3_CH12 = 472,
-	IMX_SC_R_DMA_3_CH13 = 473,
-	IMX_SC_R_DMA_3_CH14 = 474,
-	IMX_SC_R_DMA_3_CH15 = 475,
-	IMX_SC_R_DMA_3_CH16 = 476,
-	IMX_SC_R_DMA_3_CH17 = 477,
-	IMX_SC_R_DMA_3_CH18 = 478,
-	IMX_SC_R_DMA_3_CH19 = 479,
-	IMX_SC_R_DMA_3_CH20 = 480,
-	IMX_SC_R_DMA_3_CH21 = 481,
-	IMX_SC_R_DMA_3_CH22 = 482,
-	IMX_SC_R_DMA_3_CH23 = 483,
-	IMX_SC_R_DMA_3_CH24 = 484,
-	IMX_SC_R_DMA_3_CH25 = 485,
-	IMX_SC_R_DMA_3_CH26 = 486,
-	IMX_SC_R_DMA_3_CH27 = 487,
-	IMX_SC_R_DMA_3_CH28 = 488,
-	IMX_SC_R_DMA_3_CH29 = 489,
-	IMX_SC_R_DMA_3_CH30 = 490,
-	IMX_SC_R_DMA_3_CH31 = 491,
-	IMX_SC_R_AUDIO_PLL_1 = 492,
-	IMX_SC_R_AUDIO_CLK_0 = 493,
-	IMX_SC_R_AUDIO_CLK_1 = 494,
-	IMX_SC_R_MCLK_OUT_0 = 495,
-	IMX_SC_R_MCLK_OUT_1 = 496,
-	IMX_SC_R_PMIC_0 = 497,
-	IMX_SC_R_PMIC_1 = 498,
-	IMX_SC_R_SECO = 499,
-	IMX_SC_R_CAAM_JR1 = 500,
-	IMX_SC_R_CAAM_JR2 = 501,
-	IMX_SC_R_CAAM_JR3 = 502,
-	IMX_SC_R_SECO_MU_2 = 503,
-	IMX_SC_R_SECO_MU_3 = 504,
-	IMX_SC_R_SECO_MU_4 = 505,
-	IMX_SC_R_HDMI_RX_PWM_0 = 506,
-	IMX_SC_R_A35 = 507,
-	IMX_SC_R_A35_0 = 508,
-	IMX_SC_R_A35_1 = 509,
-	IMX_SC_R_A35_2 = 510,
-	IMX_SC_R_A35_3 = 511,
-	IMX_SC_R_DSP = 512,
-	IMX_SC_R_DSP_RAM = 513,
-	IMX_SC_R_CAAM_JR1_OUT = 514,
-	IMX_SC_R_CAAM_JR2_OUT = 515,
-	IMX_SC_R_CAAM_JR3_OUT = 516,
-	IMX_SC_R_VPU_DEC_0 = 517,
-	IMX_SC_R_VPU_ENC_0 = 518,
-	IMX_SC_R_CAAM_JR0 = 519,
-	IMX_SC_R_CAAM_JR0_OUT = 520,
-	IMX_SC_R_PMIC_2 = 521,
-	IMX_SC_R_DBLOGIC = 522,
-	IMX_SC_R_HDMI_PLL_1 = 523,
-	IMX_SC_R_BOARD_R0 = 524,
-	IMX_SC_R_BOARD_R1 = 525,
-	IMX_SC_R_BOARD_R2 = 526,
-	IMX_SC_R_BOARD_R3 = 527,
-	IMX_SC_R_BOARD_R4 = 528,
-	IMX_SC_R_BOARD_R5 = 529,
-	IMX_SC_R_BOARD_R6 = 530,
-	IMX_SC_R_BOARD_R7 = 531,
-	IMX_SC_R_MJPEG_DEC_MP = 532,
-	IMX_SC_R_MJPEG_ENC_MP = 533,
-	IMX_SC_R_VPU_TS_0 = 534,
-	IMX_SC_R_VPU_MU_0 = 535,
-	IMX_SC_R_VPU_MU_1 = 536,
-	IMX_SC_R_VPU_MU_2 = 537,
-	IMX_SC_R_VPU_MU_3 = 538,
-	IMX_SC_R_VPU_ENC_1 = 539,
-	IMX_SC_R_VPU = 540,
-	IMX_SC_R_LAST
-};
-
-/* NOTE - please add by replacing some of the UNUSED from above! */
-
-/*
  * This type is used to indicate a control.
  */
 enum imx_sc_ctrl {
-- 
2.7.4

[PATCH V8 3/5] dt-bindings: fsl: scu: update power domain binding

[A.s. Dong]

Update the power domain binding with #power-domain-cells 1 format.
The first cell can be a global SCU power domain and the 2nd cell
the device ID. With this approach, we may remove all the sub power
domain nodes from device tree which can relief the device tree a lot.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-pm@vger.kernel.org
Cc: devicetree@vger.kernel.org
Suggested-by: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v7->v8:
 * new patch
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 37 +++++-----------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 46d0af1..c20f38e 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -58,19 +58,11 @@ This binding for the SCU power domain providers uses the generic power
 domain binding[2].
 
 Required properties:
-- compatible:		Should be "fsl,scu-pd".
-- #address-cells:	Should be 1.
-- #size-cells:		Should be 0.
-
-Required properties for power domain sub nodes:
-- #power-domain-cells:	Must be 0.
-
-Optional Properties:
-- reg:			Resource ID of this power domain.
-			No exist means uncontrollable by user.
+- compatible:		Should be "fsl,imx8qxp-scu-pd".
+- #power-domain-cells:	Must be 1. Contains the Resource ID used by
+			SCU commands.
 			See detailed Resource ID list from:
-			include/dt-bindings/power/imx-rsrc.h
-- power-domains:	phandle pointing to the parent power domain.
+			include/dt-bindings/firmware/imx/rsrc.h
 
 Clock bindings based on SCU Message Protocol
 ------------------------------------------------------------
@@ -152,22 +144,9 @@ firmware {
 			...
 		};
 
-		imx8qx-pm {
-			compatible = "fsl,scu-pd";
-			#address-cells = <1>;
-			#size-cells = <0>;
-
-			pd_dma: dma-power-domain {
-				#power-domain-cells = <0>;
-
-				pd_dma_lpuart0: dma-lpuart0@57 {
-					reg = <SC_R_UART_0>;
-					#power-domain-cells = <0>;
-					power-domains = <&pd_dma>;
-				};
-				...
-			};
-			...
+		pd: imx8qx-pd {
+			compatible = "fsl,imx8qxp-scu-pd";
+			#power-domain-cells = <1>;
 		};
 	};
 };
@@ -179,5 +158,5 @@ serial@5a060000 {
 	clocks = <&clk IMX8QXP_UART0_CLK>,
 		 <&clk IMX8QXP_UART0_IPG_CLK>;
 	clock-names = "per", "ipg";
-	power-domains = <&pd_dma_lpuart0>;
+	power-domains = <&pd IMX_SC_R_UART_0>;
 };
-- 
2.7.4

Re: [PATCH V8 3/5] dt-bindings: fsl: scu: update power domain binding

[Rob Herring]

On Sun, 28 Oct 2018 15:19:44 +0000, "A.s. Dong" wrote:
> Update the power domain binding with #power-domain-cells 1 format.
> The first cell can be a global SCU power domain and the 2nd cell
> the device ID. With this approach, we may remove all the sub power
> domain nodes from device tree which can relief the device tree a lot.
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Suggested-by: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> ChangeLog:
> v7->v8:
>  * new patch
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 37 +++++-----------------
>  1 file changed, 8 insertions(+), 29 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH V8 4/5] firmware: imx: add pm svc headfile

[A.s. Dong]

Add SCU PM SVC related protocol definitions which will be used by
a number of PM functions like Power Domain, Clock, Reset and etc.
The detailed implementation of each function will put in the individual
function drivers.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v4->v8:
 * no changes
v3->v4:
 * update firmware headfile patch from include/soc/imx to
   include/linux/firmware/imx
v2->v3:
 * name updated with IMX_SC prefix
v1->v2:
 * new introduced
---
 include/linux/firmware/imx/sci.h    |  1 +
 include/linux/firmware/imx/svc/pm.h | 85 +++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+)
 create mode 100644 include/linux/firmware/imx/svc/pm.h

diff --git a/include/linux/firmware/imx/sci.h b/include/linux/firmware/imx/sci.h
index 29ada60..ebc5509 100644
--- a/include/linux/firmware/imx/sci.h
+++ b/include/linux/firmware/imx/sci.h
@@ -14,4 +14,5 @@
 #include <linux/firmware/imx/types.h>
 
 #include <linux/firmware/imx/svc/misc.h>
+#include <linux/firmware/imx/svc/pm.h>
 #endif /* _SC_SCI_H */
diff --git a/include/linux/firmware/imx/svc/pm.h b/include/linux/firmware/imx/svc/pm.h
new file mode 100644
index 0000000..1f6975d
--- /dev/null
+++ b/include/linux/firmware/imx/svc/pm.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017-2018 NXP
+ *
+ * Header file containing the public API for the System Controller (SC)
+ * Power Management (PM) function. This includes functions for power state
+ * control, clock control, reset control, and wake-up event control.
+ *
+ * PM_SVC (SVC) Power Management Service
+ *
+ * Module for the Power Management (PM) service.
+ */
+
+#ifndef _SC_PM_API_H
+#define _SC_PM_API_H
+
+#include <linux/firmware/imx/sci.h>
+
+/*
+ * This type is used to indicate RPC PM function calls.
+ */
+enum imx_sc_pm_func {
+	IMX_SC_PM_FUNC_UNKNOWN = 0,
+	IMX_SC_PM_FUNC_SET_SYS_POWER_MODE = 19,
+	IMX_SC_PM_FUNC_SET_PARTITION_POWER_MODE = 1,
+	IMX_SC_PM_FUNC_GET_SYS_POWER_MODE = 2,
+	IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE = 3,
+	IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE = 4,
+	IMX_SC_PM_FUNC_REQ_LOW_POWER_MODE = 16,
+	IMX_SC_PM_FUNC_SET_CPU_RESUME_ADDR = 17,
+	IMX_SC_PM_FUNC_REQ_SYS_IF_POWER_MODE = 18,
+	IMX_SC_PM_FUNC_SET_CLOCK_RATE = 5,
+	IMX_SC_PM_FUNC_GET_CLOCK_RATE = 6,
+	IMX_SC_PM_FUNC_CLOCK_ENABLE = 7,
+	IMX_SC_PM_FUNC_SET_CLOCK_PARENT = 14,
+	IMX_SC_PM_FUNC_GET_CLOCK_PARENT = 15,
+	IMX_SC_PM_FUNC_RESET = 13,
+	IMX_SC_PM_FUNC_RESET_REASON = 10,
+	IMX_SC_PM_FUNC_BOOT = 8,
+	IMX_SC_PM_FUNC_REBOOT = 9,
+	IMX_SC_PM_FUNC_REBOOT_PARTITION = 12,
+	IMX_SC_PM_FUNC_CPU_START = 11,
+};
+
+/*
+ * Defines for ALL parameters
+ */
+#define IMX_SC_PM_CLK_ALL		UINT8_MAX	/* All clocks */
+
+/*
+ * Defines for SC PM Power Mode
+ */
+#define IMX_SC_PM_PW_MODE_OFF	0	/* Power off */
+#define IMX_SC_PM_PW_MODE_STBY	1	/* Power in standby */
+#define IMX_SC_PM_PW_MODE_LP	2	/* Power in low-power */
+#define IMX_SC_PM_PW_MODE_ON	3	/* Power on */
+
+/*
+ * Defines for SC PM CLK
+ */
+#define IMX_SC_PM_CLK_SLV_BUS	0	/* Slave bus clock */
+#define IMX_SC_PM_CLK_MST_BUS	1	/* Master bus clock */
+#define IMX_SC_PM_CLK_PER	2	/* Peripheral clock */
+#define IMX_SC_PM_CLK_PHY	3	/* Phy clock */
+#define IMX_SC_PM_CLK_MISC	4	/* Misc clock */
+#define IMX_SC_PM_CLK_MISC0	0	/* Misc 0 clock */
+#define IMX_SC_PM_CLK_MISC1	1	/* Misc 1 clock */
+#define IMX_SC_PM_CLK_MISC2	2	/* Misc 2 clock */
+#define IMX_SC_PM_CLK_MISC3	3	/* Misc 3 clock */
+#define IMX_SC_PM_CLK_MISC4	4	/* Misc 4 clock */
+#define IMX_SC_PM_CLK_CPU	2	/* CPU clock */
+#define IMX_SC_PM_CLK_PLL	4	/* PLL */
+#define IMX_SC_PM_CLK_BYPASS	4	/* Bypass clock */
+
+/*
+ * Defines for SC PM CLK Parent
+ */
+#define IMX_SC_PM_PARENT_XTAL	0	/* Parent is XTAL. */
+#define IMX_SC_PM_PARENT_PLL0	1	/* Parent is PLL0 */
+#define IMX_SC_PM_PARENT_PLL1	2	/* Parent is PLL1 or PLL0/2 */
+#define IMX_SC_PM_PARENT_PLL2	3	/* Parent in PLL2 or PLL0/4 */
+#define IMX_SC_PM_PARENT_BYPS	4	/* Parent is a bypass clock. */
+
+#endif /* _SC_PM_API_H */
-- 
2.7.4

[PATCH V8 5/5] firmware: imx: add SCU power domain driver

[A.s. Dong]

Some i.MX SoCs contain a system controller that is responsible for
controlling the state of the IPs that are present. Communication
between the host processor running an OS and the system controller
happens through a SCU protocol. This patch adds SCU protocol based
power domains drivers.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v7->v8:
 * update to #power-domain-cells 1 binding
v6->v7:
 * still using generic "fsl,scu-pd" to driver binding incase the future
   SoC is compatible
 * remove unnecessary cast
v5->v6:
 * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd
   accordingly
v4->v5:
 * minor improvements according to Sascha's suggestions
v3->v4:
 * update firmware headfile patch from include/soc/imx to
   include/linux/firmware/imx
v2->v3:
 * name of structures/enums updated with imx_sc prefix
v1->v2:
 * move into drivers/firmware/imx
 * Implement sc_pm_set_resource_power_mode() API in the driver instead
   of call it via SCU API according to Sascha's suggestion
---
 drivers/firmware/imx/Kconfig  |   6 +
 drivers/firmware/imx/Makefile |   3 +-
 drivers/firmware/imx/scu-pd.c | 302 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/imx/scu-pd.c

diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
index b170c28..6a7a7c2 100644
--- a/drivers/firmware/imx/Kconfig
+++ b/drivers/firmware/imx/Kconfig
@@ -9,3 +9,9 @@ config IMX_SCU
 
 	  This driver manages the IPC interface between host CPU and the
 	  SCU firmware running on M4.
+
+config IMX_SCU_PD
+	bool "IMX SCU Power Domain driver"
+	depends on IMX_SCU
+	help
+	  The System Controller Firmware (SCFW) based power domain driver.
diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index 0ac04df..1b2e15b 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_IMX_SCU)	+= imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU_PD)	+= scu-pd.o
diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
new file mode 100644
index 0000000..868938d
--- /dev/null
+++ b/drivers/firmware/imx/scu-pd.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017-2018 NXP
+ *	Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ * Implementation of the SCU based Power Domains
+ */
+
+#include <dt-bindings/firmware/imx/rsrc.h>
+#include <linux/firmware/imx/sci.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+
+/* SCU Power Mode Protocol definition */
+struct imx_sc_msg_req_set_resource_power_mode {
+	struct imx_sc_rpc_msg hdr;
+	u16 resource;
+	u8 mode;
+} __packed;
+
+#define IMX_SCU_PD_NAME_SIZE 20
+struct imx_sc_pm_domain {
+	struct generic_pm_domain pd;
+	char name[IMX_SCU_PD_NAME_SIZE];
+	u32 rsrc;
+};
+
+struct imx_sc_pd_range {
+	char *name;
+	u32 rsrc;
+	u8 num;
+	bool postfix;
+};
+
+struct imx_sc_pd_soc {
+	const struct imx_sc_pd_range *pd_ranges;
+	u8 num_ranges;
+};
+
+static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
+	/* LSIO SS */
+	{ "lsio-pwm", IMX_SC_R_PWM_0, 8, 1 },
+	{ "lsio-gpio", IMX_SC_R_GPIO_0, 8, 1 },
+	{ "lsio-gpt", IMX_SC_R_GPT_0, 5, 1 },
+	{ "lsio-kpp", IMX_SC_R_KPP, 1, 0 },
+	{ "lsio-fspi", IMX_SC_R_FSPI_0, 2, 1 },
+	{ "lsio-mu", IMX_SC_R_MU_0A, 14, 1 },
+
+	/* CONN SS */
+	{ "con-usb", IMX_SC_R_USB_0, 2, 1 },
+	{ "con-usb0phy", IMX_SC_R_USB_0_PHY, 1, 0 },
+	{ "con-usb2", IMX_SC_R_USB_2, 1, 0 },
+	{ "con-usb2phy", IMX_SC_R_USB_2_PHY, 1, 0 },
+	{ "con-sdhc", IMX_SC_R_SDHC_0, 3, 1 },
+	{ "con-enet", IMX_SC_R_ENET_0, 2, 1 },
+	{ "con-nand", IMX_SC_R_NAND, 1, 0 },
+	{ "con-mlb", IMX_SC_R_MLB_0, 1, 1 },
+
+	/* Audio DMA SS */
+	{ "adma-audio-pll0", IMX_SC_R_AUDIO_PLL_0, 1, 0 },
+	{ "adma-audio-pll1", IMX_SC_R_AUDIO_PLL_1, 1, 0 },
+	{ "adma-audio-clk-0", IMX_SC_R_AUDIO_CLK_0, 1, 0 },
+	{ "adma-dma0-ch", IMX_SC_R_DMA_0_CH0, 16, 1 },
+	{ "adma-dma1-ch", IMX_SC_R_DMA_1_CH0, 16, 1 },
+	{ "adma-dma2-ch", IMX_SC_R_DMA_2_CH0, 5, 1 },
+	{ "adma-asrc0", IMX_SC_R_ASRC_0, 1, 0 },
+	{ "adma-asrc1", IMX_SC_R_ASRC_1, 1, 0 },
+	{ "adma-esai0", IMX_SC_R_ESAI_0, 1, 0 },
+	{ "adma-spdif0", IMX_SC_R_SPDIF_0, 1, 0 },
+	{ "adma-sai", IMX_SC_R_SAI_0, 3, 1 },
+	{ "adma-amix", IMX_SC_R_AMIX, 1, 0 },
+	{ "adma-mqs0", IMX_SC_R_MQS_0, 1, 0 },
+	{ "adma-dsp", IMX_SC_R_DSP, 1, 0 },
+	{ "adma-dsp-ram", IMX_SC_R_DSP_RAM, 1, 0 },
+	{ "adma-can", IMX_SC_R_CAN_0, 3, 1 },
+	{ "adma-ftm", IMX_SC_R_FTM_0, 2, 1 },
+	{ "adma-lpi2c", IMX_SC_R_I2C_0, 4, 1 },
+	{ "adma-adc", IMX_SC_R_ADC_0, 1, 1 },
+	{ "adma-lcd", IMX_SC_R_LCD_0, 1, 1 },
+	{ "adma-lcd0-pwm", IMX_SC_R_LCD_0_PWM_0, 1, 1 },
+	{ "adma-lpuart", IMX_SC_R_UART_0, 4, 1 },
+	{ "adma-lpspi", IMX_SC_R_SPI_0, 4, 1 },
+
+	/* VPU SS  */
+	{ "vpu", IMX_SC_R_VPU, 1, 0 },
+	{ "vpu-pid", IMX_SC_R_VPU_PID0, 8, 1 },
+	{ "vpu-dec0", IMX_SC_R_VPU_DEC_0, 1, 0 },
+	{ "vpu-enc0", IMX_SC_R_VPU_ENC_0, 1, 0 },
+
+	/* GPU SS */
+	{ "gpu0-pid", IMX_SC_R_GPU_0_PID0, 4, 1 },
+
+	/* HSIO SS */
+	{ "hsio-pcie-b", IMX_SC_R_PCIE_B, 1, 0 },
+	{ "hsio-serdes-1", IMX_SC_R_SERDES_1, 1, 0 },
+	{ "hsio-gpio", IMX_SC_R_HSIO_GPIO, 1, 0 },
+
+	/* MIPI/LVDS SS */
+	{ "mipi0", IMX_SC_R_MIPI_0, 1, 0 },
+	{ "mipi0-pwm0", IMX_SC_R_MIPI_0_PWM_0, 1, 0 },
+	{ "mipi0-i2c", IMX_SC_R_MIPI_0_I2C_0, 2, 1 },
+	{ "lvds0", IMX_SC_R_LVDS_0, 1, 0 },
+
+	/* DC SS */
+	{ "dc0", IMX_SC_R_DC_0, 1, 0 },
+	{ "dc0-pll", IMX_SC_R_DC_0_PLL_0, 2, 1 },
+};
+
+static const struct imx_sc_pd_soc imx8qxp_scu_pd = {
+	.pd_ranges = imx8qxp_scu_pd_ranges,
+	.num_ranges = ARRAY_SIZE(imx8qxp_scu_pd_ranges),
+};
+
+static struct imx_sc_ipc *pm_ipc_handle;
+
+static inline struct imx_sc_pm_domain *
+to_imx_sc_pd(struct generic_pm_domain *genpd)
+{
+	return container_of(genpd, struct imx_sc_pm_domain, pd);
+}
+
+static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
+{
+	struct imx_sc_msg_req_set_resource_power_mode msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	struct imx_sc_pm_domain *pd;
+	int ret;
+
+	pd = to_imx_sc_pd(domain);
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_PM;
+	hdr->func = IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
+	hdr->size = 2;
+
+	msg.resource = pd->rsrc;
+	msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
+
+	ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
+	if (ret)
+		dev_err(&domain->dev, "failed to power %s resource %d ret %d\n",
+			power_on ? "up" : "off", pd->rsrc, ret);
+
+	return ret;
+}
+
+static int imx_sc_pd_power_on(struct generic_pm_domain *domain)
+{
+	return imx_sc_pd_power(domain, true);
+}
+
+static int imx_sc_pd_power_off(struct generic_pm_domain *domain)
+{
+	return imx_sc_pd_power(domain, false);
+}
+
+static struct generic_pm_domain *imx_scu_pd_xlate(struct of_phandle_args *spec,
+						  void *data)
+{
+	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
+	struct genpd_onecell_data *pd_data = data;
+	unsigned int i;
+
+	for (i = 0; i < pd_data->num_domains; i++) {
+		struct imx_sc_pm_domain *sc_pd;
+
+		sc_pd = to_imx_sc_pd(pd_data->domains[i]);
+		if (sc_pd->rsrc == spec->args[0]) {
+			domain = &sc_pd->pd;
+			break;
+		}
+	}
+
+	return domain;
+}
+
+static struct imx_sc_pm_domain *
+imx_scu_add_pm_domain(struct device *dev, int idx,
+		      const struct imx_sc_pd_range *pd_ranges)
+{
+	struct imx_sc_pm_domain *sc_pd;
+	int ret;
+
+	sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
+	if (!sc_pd)
+		return ERR_PTR(-ENOMEM);
+
+	sc_pd->rsrc = pd_ranges->rsrc + idx;
+	sc_pd->pd.power_off = imx_sc_pd_power_off;
+	sc_pd->pd.power_on = imx_sc_pd_power_on;
+
+	if (pd_ranges->postfix)
+		snprintf(sc_pd->name, sizeof(sc_pd->name),
+			 "%s%i", pd_ranges->name, idx);
+	else
+		snprintf(sc_pd->name, sizeof(sc_pd->name),
+			 "%s", pd_ranges->name);
+
+	sc_pd->pd.name = sc_pd->name;
+
+	if (sc_pd->rsrc >= IMX_SC_R_LAST) {
+		dev_warn(dev, "invalid pd %s rsrc id %d found",
+			 sc_pd->name, sc_pd->rsrc);
+
+		devm_kfree(dev, sc_pd);
+		return NULL;
+	}
+
+	ret = pm_genpd_init(&sc_pd->pd, NULL, true);
+	if (ret) {
+		dev_warn(dev, "failed to init pd %s rsrc id %d",
+			 sc_pd->name, sc_pd->rsrc);
+		devm_kfree(dev, sc_pd);
+		return NULL;
+	}
+
+	return sc_pd;
+}
+
+static int imx_scu_init_pm_domains(struct device *dev,
+				    const struct imx_sc_pd_soc *pd_soc)
+{
+	const struct imx_sc_pd_range *pd_ranges = pd_soc->pd_ranges;
+	struct generic_pm_domain **domains;
+	struct genpd_onecell_data *pd_data;
+	struct imx_sc_pm_domain *sc_pd;
+	u32 count = 0;
+	int i, j;
+
+	for (i = 0; i < pd_soc->num_ranges; i++)
+		count += pd_ranges[i].num;
+
+	domains = devm_kcalloc(dev, count, sizeof(*domains), GFP_KERNEL);
+	if (!domains)
+		return -ENOMEM;
+
+	pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL);
+	if (!pd_data)
+		return -ENOMEM;
+
+	count = 0;
+	for (i = 0; i < pd_soc->num_ranges; i++) {
+		for (j = 0; j < pd_ranges[i].num; j++) {
+			sc_pd = imx_scu_add_pm_domain(dev, j, &pd_ranges[i]);
+			if (IS_ERR_OR_NULL(sc_pd))
+				continue;
+
+			domains[count++] = &sc_pd->pd;
+			dev_dbg(dev, "added power domain %s\n", sc_pd->pd.name);
+		}
+	}
+
+	pd_data->domains = domains;
+	pd_data->num_domains = count;
+	pd_data->xlate = imx_scu_pd_xlate;
+
+	of_genpd_add_provider_onecell(dev->of_node, pd_data);
+
+	return 0;
+}
+
+static int imx_sc_pd_probe(struct platform_device *pdev)
+{
+	const struct imx_sc_pd_soc *pd_soc;
+	int ret;
+
+	ret = imx_scu_get_handle(&pm_ipc_handle);
+	if (ret)
+		return ret;
+
+	pd_soc = of_device_get_match_data(&pdev->dev);
+	if (!pd_soc)
+		return -ENODEV;
+
+	return imx_scu_init_pm_domains(&pdev->dev, pd_soc);
+}
+
+static const struct of_device_id imx_sc_pd_match[] = {
+	{ .compatible = "fsl,imx8qxp-scu-pd", &imx8qxp_scu_pd},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver imx_sc_pd_driver = {
+	.driver = {
+		.name = "imx-scu-pd",
+		.of_match_table = imx_sc_pd_match,
+	},
+	.probe = imx_sc_pd_probe,
+};
+builtin_platform_driver(imx_sc_pd_driver);
+
+MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
+MODULE_DESCRIPTION("IMX SCU Power Domain driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[Ulf Hansson]

[...]

}
> +
> +static struct imx_sc_pm_domain *
> +imx_scu_add_pm_domain(struct device *dev, int idx,
> +                     const struct imx_sc_pd_range *pd_ranges)
> +{
> +       struct imx_sc_pm_domain *sc_pd;
> +       int ret;
> +
> +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
> +       if (!sc_pd)
> +               return ERR_PTR(-ENOMEM);
> +
> +       sc_pd->rsrc = pd_ranges->rsrc + idx;
> +       sc_pd->pd.power_off = imx_sc_pd_power_off;
> +       sc_pd->pd.power_on = imx_sc_pd_power_on;

So, this means you are going to register one genpd per device (one for
each uart, pwm etc), but there is actually a better option.

What you seems to be needing is a way to "power on/off" devices,
rather than the PM domain. Right? The PM domain, is managed internally
by the FW, no?

In any case, it looks like what you should do is to implement the
->attach|detach_dev() callback for the genpd and use the regular
of_genpd_add_provider_simple(). From within the ->attach_dev(), you
should get the OF node for the device that is being attached and then
parse the power-domain cell containing the "resource id" and store
that in the per device struct generic_pm_domain_data (we have void
pointer there for storing these kind of things).

Additionally, you need to implement the ->stop() and ->start()
callbacks of genpd, which is where you "power on/off" devices, rather
than using the above ->power_on|off() callbacks.

Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I
should have pointed you towards that reference already in the earlier
version.

> +
> +       if (pd_ranges->postfix)
> +               snprintf(sc_pd->name, sizeof(sc_pd->name),
> +                        "%s%i", pd_ranges->name, idx);
> +       else
> +               snprintf(sc_pd->name, sizeof(sc_pd->name),
> +                        "%s", pd_ranges->name);
> +
> +       sc_pd->pd.name = sc_pd->name;
> +
> +       if (sc_pd->rsrc >= IMX_SC_R_LAST) {
> +               dev_warn(dev, "invalid pd %s rsrc id %d found",
> +                        sc_pd->name, sc_pd->rsrc);
> +
> +               devm_kfree(dev, sc_pd);
> +               return NULL;
> +       }
> +
> +       ret = pm_genpd_init(&sc_pd->pd, NULL, true);
> +       if (ret) {
> +               dev_warn(dev, "failed to init pd %s rsrc id %d",
> +                        sc_pd->name, sc_pd->rsrc);
> +               devm_kfree(dev, sc_pd);
> +               return NULL;
> +       }
> +
> +       return sc_pd;
> +}

[...]

Kind regards
Uffe

RE: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[A.s. Dong]

[...]

> }
> > +
> > +static struct imx_sc_pm_domain *
> > +imx_scu_add_pm_domain(struct device *dev, int idx,
> > +                     const struct imx_sc_pd_range *pd_ranges) {
> > +       struct imx_sc_pm_domain *sc_pd;
> > +       int ret;
> > +
> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
> > +       if (!sc_pd)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
> 
> So, this means you are going to register one genpd per device (one for each
> uart, pwm etc), but there is actually a better option.
> 
> What you seems to be needing is a way to "power on/off" devices, rather than
> the PM domain. Right? The PM domain, is managed internally by the FW, no?
> 

Yes, you're right.

> In any case, it looks like what you should do is to implement the
> ->attach|detach_dev() callback for the genpd and use the regular
> of_genpd_add_provider_simple(). From within the ->attach_dev(), you should
> get the OF node for the device that is being attached and then parse the
> power-domain cell containing the "resource id" and store that in the per device
> struct generic_pm_domain_data (we have void pointer there for storing these
> kind of things).
> 
> Additionally, you need to implement the ->stop() and ->start() callbacks of
> genpd, which is where you "power on/off" devices, rather than using the above
> ->power_on|off() callbacks.
> 
> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I should
> have pointed you towards that reference already in the earlier version.
> 

Appreciated for such detailed explanation. lt's a good suggestion and I really like to
switch to it.  However, after digged a bit more, i found a few blocking issues:
1. The .attach_dev() of power domain infrastructure still does not support multi domains case.

It looks like there's no way for .attach_dev() to understand which sub 'power domain'
the device is attaching for one global power domain provider like ti_sci as you suggested.
Secondly, the struct device *dev passed into is a virtual PD device. It does not help
for parsing the real device resource id. 

So framework probably needs some extension to support multi power domain cases.

2. It also breaks most of current drivers as the driver probe sequence behavior changed
If removing .power_on() and .power_off() callback and use .start() and .stop() instead.

genpd_dev_pm_attach will only power up the domain and attach device, but will not call .start()
which relies on device runtime pm. That means the device power is still not up before
running driver probe function. For SCU enabled platforms, all device drivers accessing registers/clock
without power domain enabled will trigger a HW access error. That means we need fix most
of drivers probe sequence with proper runtime pm. So I'm a bit wondering whether we should
still keep the exist way.

In summary, we probably may not be able to try ti_sci way before fixing above two issues.

Do you think I should wait for them to be fixed or if we could use the current way at first?
I might be a little intend to the second way as we now have a lot upstreaming work pending on
this. But please let me know if you have a different idea.

Regards
Dong Aisheng

> > +
> > +       if (pd_ranges->postfix)
> > +               snprintf(sc_pd->name, sizeof(sc_pd->name),
> > +                        "%s%i", pd_ranges->name, idx);
> > +       else
> > +               snprintf(sc_pd->name, sizeof(sc_pd->name),
> > +                        "%s", pd_ranges->name);
> > +
> > +       sc_pd->pd.name = sc_pd->name;
> > +
> > +       if (sc_pd->rsrc >= IMX_SC_R_LAST) {
> > +               dev_warn(dev, "invalid pd %s rsrc id %d found",
> > +                        sc_pd->name, sc_pd->rsrc);
> > +
> > +               devm_kfree(dev, sc_pd);
> > +               return NULL;
> > +       }
> > +
> > +       ret = pm_genpd_init(&sc_pd->pd, NULL, true);
> > +       if (ret) {
> > +               dev_warn(dev, "failed to init pd %s rsrc id %d",
> > +                        sc_pd->name, sc_pd->rsrc);
> > +               devm_kfree(dev, sc_pd);
> > +               return NULL;
> > +       }
> > +
> > +       return sc_pd;
> > +}
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[Ulf Hansson]

On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote:
> [...]
>
>> }
>> > +
>> > +static struct imx_sc_pm_domain *
>> > +imx_scu_add_pm_domain(struct device *dev, int idx,
>> > +                     const struct imx_sc_pd_range *pd_ranges) {
>> > +       struct imx_sc_pm_domain *sc_pd;
>> > +       int ret;
>> > +
>> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
>> > +       if (!sc_pd)
>> > +               return ERR_PTR(-ENOMEM);
>> > +
>> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
>> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
>> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
>>
>> So, this means you are going to register one genpd per device (one for each
>> uart, pwm etc), but there is actually a better option.
>>
>> What you seems to be needing is a way to "power on/off" devices, rather than
>> the PM domain. Right? The PM domain, is managed internally by the FW, no?
>>
>
> Yes, you're right.
>
>> In any case, it looks like what you should do is to implement the
>> ->attach|detach_dev() callback for the genpd and use the regular
>> of_genpd_add_provider_simple(). From within the ->attach_dev(), you should
>> get the OF node for the device that is being attached and then parse the
>> power-domain cell containing the "resource id" and store that in the per device
>> struct generic_pm_domain_data (we have void pointer there for storing these
>> kind of things).
>>
>> Additionally, you need to implement the ->stop() and ->start() callbacks of
>> genpd, which is where you "power on/off" devices, rather than using the above
>> ->power_on|off() callbacks.
>>
>> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I should
>> have pointed you towards that reference already in the earlier version.
>>
>
> Appreciated for such detailed explanation. lt's a good suggestion and I really like to
> switch to it.  However, after digged a bit more, i found a few blocking issues:
> 1. The .attach_dev() of power domain infrastructure still does not support multi domains case.
>
> It looks like there's no way for .attach_dev() to understand which sub 'power domain'
> the device is attaching for one global power domain provider like ti_sci as you suggested.
> Secondly, the struct device *dev passed into is a virtual PD device. It does not help
> for parsing the real device resource id.
>
> So framework probably needs some extension to support multi power domain cases.

Right, you are correct.

Let me think about this and see what I can come up with - unless you
already have something you want to propose?

>
> 2. It also breaks most of current drivers as the driver probe sequence behavior changed
> If removing .power_on() and .power_off() callback and use .start() and .stop() instead.
>
> genpd_dev_pm_attach will only power up the domain and attach device, but will not call .start()
> which relies on device runtime pm. That means the device power is still not up before
> running driver probe function. For SCU enabled platforms, all device drivers accessing registers/clock
> without power domain enabled will trigger a HW access error. That means we need fix most
> of drivers probe sequence with proper runtime pm. So I'm a bit wondering whether we should
> still keep the exist way.

I see what you are trying to say, but I am not sure why you consider
this as they would break? How did they work in the first place?

Anyway, the problem you are talking about have so far been addressed
by making buses/drivers to call pm_runtime_enable() and
pm_runtime_get_sync() - before trying to probe their devices.

Do you have an idea of the amount of drivers that needs to fixed, in
regards to this?

I guess an option could be to, that via the ->attach_dev() callback
perform the same operations as also being done during ->start(). Of
course, you may need to keep a flag about this being done, so that
operations that need to be balanced with get/put or the like is
managed correctly, the next time ->start|stop() becomes called.

>
> In summary, we probably may not be able to try ti_sci way before fixing above two issues.
>
> Do you think I should wait for them to be fixed or if we could use the current way at first?
> I might be a little intend to the second way as we now have a lot upstreaming work pending on
> this. But please let me know if you have a different idea.

As this is also about deploying a DTB with a correctly modeled HW, I
think we need to fix the above issues.

I am happy to help, in one way or the other.

[...]

Kind regards
Uffe

RE: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[A.s. Dong]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Wednesday, October 31, 2018 12:00 AM
[...]
> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > [...]
> >
> >> }
> >> > +
> >> > +static struct imx_sc_pm_domain *
> >> > +imx_scu_add_pm_domain(struct device *dev, int idx,
> >> > +                     const struct imx_sc_pd_range *pd_ranges) {
> >> > +       struct imx_sc_pm_domain *sc_pd;
> >> > +       int ret;
> >> > +
> >> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
> >> > +       if (!sc_pd)
> >> > +               return ERR_PTR(-ENOMEM);
> >> > +
> >> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
> >> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
> >> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
> >>
> >> So, this means you are going to register one genpd per device (one
> >> for each uart, pwm etc), but there is actually a better option.
> >>
> >> What you seems to be needing is a way to "power on/off" devices,
> >> rather than the PM domain. Right? The PM domain, is managed internally
> by the FW, no?
> >>
> >
> > Yes, you're right.
> >
> >> In any case, it looks like what you should do is to implement the
> >> ->attach|detach_dev() callback for the genpd and use the regular
> >> of_genpd_add_provider_simple(). From within the ->attach_dev(), you
> >> should get the OF node for the device that is being attached and then
> >> parse the power-domain cell containing the "resource id" and store
> >> that in the per device struct generic_pm_domain_data (we have void
> >> pointer there for storing these kind of things).
> >>
> >> Additionally, you need to implement the ->stop() and ->start()
> >> callbacks of genpd, which is where you "power on/off" devices, rather
> >> than using the above
> >> ->power_on|off() callbacks.
> >>
> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I
> >> should have pointed you towards that reference already in the earlier
> version.
> >>
> >
> > Appreciated for such detailed explanation. lt's a good suggestion and
> > I really like to switch to it.  However, after digged a bit more, i found a few
> blocking issues:
> > 1. The .attach_dev() of power domain infrastructure still does not support
> multi domains case.
> >
> > It looks like there's no way for .attach_dev() to understand which sub 'power
> domain'
> > the device is attaching for one global power domain provider like ti_sci as
> you suggested.
> > Secondly, the struct device *dev passed into is a virtual PD device.
> > It does not help for parsing the real device resource id.
> >
> > So framework probably needs some extension to support multi power
> domain cases.
> 
> Right, you are correct.
> 
> Let me think about this and see what I can come up with - unless you already
> have something you want to propose?
> 

I have thought we may need add two more params (real dev and pd index) for
.attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
But I'm not sure if they are proper.


> >
> > 2. It also breaks most of current drivers as the driver probe sequence
> > behavior changed If removing .power_on() and .power_off() callback and
> use .start() and .stop() instead.
> >
> > genpd_dev_pm_attach will only power up the domain and attach device,
> > but will not call .start() which relies on device runtime pm. That
> > means the device power is still not up before running driver probe
> > function. For SCU enabled platforms, all device drivers accessing
> > registers/clock without power domain enabled will trigger a HW access
> > error. That means we need fix most of drivers probe sequence with proper
> runtime pm. So I'm a bit wondering whether we should still keep the exist way.
> 
> I see what you are trying to say, but I am not sure why you consider this as
> they would break? How did they work in the first place?
> 

Most drivers does not require power domains before. E.g. GPIO, I2C, PWM, MMC.
Or even we need power domain(PUs/PCIE), they're enabled in .power_on()/.power_off()
which has been already up before probe.

> Anyway, the problem you are talking about have so far been addressed by
> making buses/drivers to call pm_runtime_enable() and
> pm_runtime_get_sync() - before trying to probe their devices.
>

Yes, called them early before accessing clock and hw registers.

> Do you have an idea of the amount of drivers that needs to fixed, in regards to
> this?
> 

So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc.
Suppose other need the same fix as we have not met this situation before.

> I guess an option could be to, that via the ->attach_dev() callback perform the
> same operations as also being done during ->start(). Of course, you may need
> to keep a flag about this being done, so that operations that need to be
> balanced with get/put or the like is managed correctly, the next time
> ->start|stop() becomes called.
> 

That might be a way to try. The follow seems may introduce a bit complexities
and driver needs to maintainer the runtime status along with framework in order to 
keep the balance?

> >
> > In summary, we probably may not be able to try ti_sci way before fixing
> above two issues.
> >
> > Do you think I should wait for them to be fixed or if we could use the current
> way at first?
> > I might be a little intend to the second way as we now have a lot
> > upstreaming work pending on this. But please let me know if you have a
> different idea.
> 
> As this is also about deploying a DTB with a correctly modeled HW, I think we
> need to fix the above issues.
> 

DTB are the same with #power-domain-cells 1 (resource ID) in either case.
So DTB don't need change and driver could be optimized.

> I am happy to help, in one way or the other.
> 

Thanks.

Regards
Dong Aisheng

> [...]
> 
> Kind regards
> Uffe

Re: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[Ulf Hansson]

On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Wednesday, October 31, 2018 12:00 AM
> [...]
>> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> > [...]
>> >
>> >> }
>> >> > +
>> >> > +static struct imx_sc_pm_domain *
>> >> > +imx_scu_add_pm_domain(struct device *dev, int idx,
>> >> > +                     const struct imx_sc_pd_range *pd_ranges) {
>> >> > +       struct imx_sc_pm_domain *sc_pd;
>> >> > +       int ret;
>> >> > +
>> >> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
>> >> > +       if (!sc_pd)
>> >> > +               return ERR_PTR(-ENOMEM);
>> >> > +
>> >> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
>> >> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
>> >> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
>> >>
>> >> So, this means you are going to register one genpd per device (one
>> >> for each uart, pwm etc), but there is actually a better option.
>> >>
>> >> What you seems to be needing is a way to "power on/off" devices,
>> >> rather than the PM domain. Right? The PM domain, is managed internally
>> by the FW, no?
>> >>
>> >
>> > Yes, you're right.
>> >
>> >> In any case, it looks like what you should do is to implement the
>> >> ->attach|detach_dev() callback for the genpd and use the regular
>> >> of_genpd_add_provider_simple(). From within the ->attach_dev(), you
>> >> should get the OF node for the device that is being attached and then
>> >> parse the power-domain cell containing the "resource id" and store
>> >> that in the per device struct generic_pm_domain_data (we have void
>> >> pointer there for storing these kind of things).
>> >>
>> >> Additionally, you need to implement the ->stop() and ->start()
>> >> callbacks of genpd, which is where you "power on/off" devices, rather
>> >> than using the above
>> >> ->power_on|off() callbacks.
>> >>
>> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I
>> >> should have pointed you towards that reference already in the earlier
>> version.
>> >>
>> >
>> > Appreciated for such detailed explanation. lt's a good suggestion and
>> > I really like to switch to it.  However, after digged a bit more, i found a few
>> blocking issues:
>> > 1. The .attach_dev() of power domain infrastructure still does not support
>> multi domains case.
>> >
>> > It looks like there's no way for .attach_dev() to understand which sub 'power
>> domain'
>> > the device is attaching for one global power domain provider like ti_sci as
>> you suggested.
>> > Secondly, the struct device *dev passed into is a virtual PD device.
>> > It does not help for parsing the real device resource id.
>> >
>> > So framework probably needs some extension to support multi power
>> domain cases.
>>
>> Right, you are correct.
>>
>> Let me think about this and see what I can come up with - unless you already
>> have something you want to propose?
>>
>
> I have thought we may need add two more params (real dev and pd index) for
> .attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
> But I'm not sure if they are proper.

Right.

I don't think we need to change the definition of the callbacks.
However, from within the callbacks (or at least the ->attach_dev()) we
need access to the data you suggest above.

To make that data available, genpd needs to store it in the per device
struct generic_pm_domain_data, but of course before invoking the
->attach_dev() callback.

>
>
>> >
>> > 2. It also breaks most of current drivers as the driver probe sequence
>> > behavior changed If removing .power_on() and .power_off() callback and
>> use .start() and .stop() instead.
>> >
>> > genpd_dev_pm_attach will only power up the domain and attach device,
>> > but will not call .start() which relies on device runtime pm. That
>> > means the device power is still not up before running driver probe
>> > function. For SCU enabled platforms, all device drivers accessing
>> > registers/clock without power domain enabled will trigger a HW access
>> > error. That means we need fix most of drivers probe sequence with proper
>> runtime pm. So I'm a bit wondering whether we should still keep the exist way.
>>
>> I see what you are trying to say, but I am not sure why you consider this as
>> they would break? How did they work in the first place?
>>
>
> Most drivers does not require power domains before. E.g. GPIO, I2C, PWM, MMC.
> Or even we need power domain(PUs/PCIE), they're enabled in .power_on()/.power_off()
> which has been already up before probe.
>
>> Anyway, the problem you are talking about have so far been addressed by
>> making buses/drivers to call pm_runtime_enable() and
>> pm_runtime_get_sync() - before trying to probe their devices.
>>
>
> Yes, called them early before accessing clock and hw registers.
>
>> Do you have an idea of the amount of drivers that needs to fixed, in regards to
>> this?
>>
>
> So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc.
> Suppose other need the same fix as we have not met this situation before.

Okay, I see.

>
>> I guess an option could be to, that via the ->attach_dev() callback perform the
>> same operations as also being done during ->start(). Of course, you may need
>> to keep a flag about this being done, so that operations that need to be
>> balanced with get/put or the like is managed correctly, the next time
>> ->start|stop() becomes called.
>>
>
> That might be a way to try. The follow seems may introduce a bit complexities
> and driver needs to maintainer the runtime status along with framework in order to
> keep the balance?

Yeah, it's likely not going to be very elegant, but should be rather
self contained in the PM domain driver and could serve as a way
forward - while working on fixing the drivers long term.

>
>> >
>> > In summary, we probably may not be able to try ti_sci way before fixing
>> above two issues.
>> >
>> > Do you think I should wait for them to be fixed or if we could use the current
>> way at first?
>> > I might be a little intend to the second way as we now have a lot
>> > upstreaming work pending on this. But please let me know if you have a
>> different idea.
>>
>> As this is also about deploying a DTB with a correctly modeled HW, I think we
>> need to fix the above issues.
>>
>
> DTB are the same with #power-domain-cells 1 (resource ID) in either case.
> So DTB don't need change and driver could be optimized.

You are correct.

Although, how does $subject patch solves the case where a device may
have multiple PM domains attached to it? Or is left as a future
improvement?

Kind regards
Uffe

RE: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[A.s. Dong]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Wednesday, October 31, 2018 11:56 PM
[...]
> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> -----Original Message-----
> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> Sent: Wednesday, October 31, 2018 12:00 AM
> > [...]
> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> > [...]
> >> >
> >> >> }
> >> >> > +
> >> >> > +static struct imx_sc_pm_domain * imx_scu_add_pm_domain(struct
> >> >> > +device *dev, int idx,
> >> >> > +                     const struct imx_sc_pd_range *pd_ranges) {
> >> >> > +       struct imx_sc_pm_domain *sc_pd;
> >> >> > +       int ret;
> >> >> > +
> >> >> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
> >> >> > +       if (!sc_pd)
> >> >> > +               return ERR_PTR(-ENOMEM);
> >> >> > +
> >> >> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
> >> >> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
> >> >> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
> >> >>
> >> >> So, this means you are going to register one genpd per device (one
> >> >> for each uart, pwm etc), but there is actually a better option.
> >> >>
> >> >> What you seems to be needing is a way to "power on/off" devices,
> >> >> rather than the PM domain. Right? The PM domain, is managed
> >> >> internally
> >> by the FW, no?
> >> >>
> >> >
> >> > Yes, you're right.
> >> >
> >> >> In any case, it looks like what you should do is to implement the
> >> >> ->attach|detach_dev() callback for the genpd and use the regular
> >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(),
> >> >> you should get the OF node for the device that is being attached
> >> >> and then parse the power-domain cell containing the "resource id"
> >> >> and store that in the per device struct generic_pm_domain_data (we
> >> >> have void pointer there for storing these kind of things).
> >> >>
> >> >> Additionally, you need to implement the ->stop() and ->start()
> >> >> callbacks of genpd, which is where you "power on/off" devices,
> >> >> rather than using the above
> >> >> ->power_on|off() callbacks.
> >> >>
> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c.
> >> >> Apologize, I should have pointed you towards that reference
> >> >> already in the earlier
> >> version.
> >> >>
> >> >
> >> > Appreciated for such detailed explanation. lt's a good suggestion
> >> > and I really like to switch to it.  However, after digged a bit
> >> > more, i found a few
> >> blocking issues:
> >> > 1. The .attach_dev() of power domain infrastructure still does not
> >> > support
> >> multi domains case.
> >> >
> >> > It looks like there's no way for .attach_dev() to understand which
> >> > sub 'power
> >> domain'
> >> > the device is attaching for one global power domain provider like
> >> > ti_sci as
> >> you suggested.
> >> > Secondly, the struct device *dev passed into is a virtual PD device.
> >> > It does not help for parsing the real device resource id.
> >> >
> >> > So framework probably needs some extension to support multi power
> >> domain cases.
> >>
> >> Right, you are correct.
> >>
> >> Let me think about this and see what I can come up with - unless you
> >> already have something you want to propose?
> >>
> >
> > I have thought we may need add two more params (real dev and pd index)
> > for
> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
> > But I'm not sure if they are proper.
> 
> Right.
> 
> I don't think we need to change the definition of the callbacks.
> However, from within the callbacks (or at least the ->attach_dev()) we need
> access to the data you suggest above.
> 
> To make that data available, genpd needs to store it in the per device struct
> generic_pm_domain_data, but of course before invoking the
> ->attach_dev() callback.
> 

Yes, that's an option.
Currently gpd_data is allocated in genpd_add_device() which looks like to be
designed to attach device to the specified power domain passed in, so it's
unware of whether it's multi domains or not. Then we may still need fine
tune those functions in different abstract layers.

> >
> >
> >> >
> >> > 2. It also breaks most of current drivers as the driver probe
> >> > sequence behavior changed If removing .power_on() and .power_off()
> >> > callback and
> >> use .start() and .stop() instead.
> >> >
> >> > genpd_dev_pm_attach will only power up the domain and attach
> >> > device, but will not call .start() which relies on device runtime
> >> > pm. That means the device power is still not up before running
> >> > driver probe function. For SCU enabled platforms, all device
> >> > drivers accessing registers/clock without power domain enabled will
> >> > trigger a HW access error. That means we need fix most of drivers
> >> > probe sequence with proper
> >> runtime pm. So I'm a bit wondering whether we should still keep the exist
> way.
> >>
> >> I see what you are trying to say, but I am not sure why you consider
> >> this as they would break? How did they work in the first place?
> >>
> >
> > Most drivers does not require power domains before. E.g. GPIO, I2C, PWM,
> MMC.
> > Or even we need power domain(PUs/PCIE), they're enabled in
> > .power_on()/.power_off() which has been already up before probe.
> >
> >> Anyway, the problem you are talking about have so far been addressed
> >> by making buses/drivers to call pm_runtime_enable() and
> >> pm_runtime_get_sync() - before trying to probe their devices.
> >>
> >
> > Yes, called them early before accessing clock and hw registers.
> >
> >> Do you have an idea of the amount of drivers that needs to fixed, in
> >> regards to this?
> >>
> >
> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc.
> > Suppose other need the same fix as we have not met this situation before.
> 
> Okay, I see.
> 
> >
> >> I guess an option could be to, that via the ->attach_dev() callback
> >> perform the same operations as also being done during ->start(). Of
> >> course, you may need to keep a flag about this being done, so that
> >> operations that need to be balanced with get/put or the like is
> >> managed correctly, the next time
> >> ->start|stop() becomes called.
> >>
> >
> > That might be a way to try. The follow seems may introduce a bit
> > complexities and driver needs to maintainer the runtime status along
> > with framework in order to keep the balance?
> 
> Yeah, it's likely not going to be very elegant, but should be rather self
> contained in the PM domain driver and could serve as a way forward - while
> working on fixing the drivers long term.
> 
> >
> >> >
> >> > In summary, we probably may not be able to try ti_sci way before
> >> > fixing
> >> above two issues.
> >> >
> >> > Do you think I should wait for them to be fixed or if we could use
> >> > the current
> >> way at first?
> >> > I might be a little intend to the second way as we now have a lot
> >> > upstreaming work pending on this. But please let me know if you
> >> > have a
> >> different idea.
> >>
> >> As this is also about deploying a DTB with a correctly modeled HW, I
> >> think we need to fix the above issues.
> >>
> >
> > DTB are the same with #power-domain-cells 1 (resource ID) in either case.
> > So DTB don't need change and driver could be optimized.
> 
> You are correct.
> 
> Although, how does $subject patch solves the case where a device may have
> multiple PM domains attached to it? Or is left as a future improvement?
> 

IIRC this patch does not have such issue for multi PM domains as we have
individual power domains for each resources. Devices will be attached to
each individual domains in __genpd_dev_pm_attach rather than the single
one global virtual SCU domain. Furthermore we're using domains .power_on()/
power_off() callback which means the power will be up before the probe.
(no the issue like when using .attach_dev()/.detach_dev() and run .start()/stop()
via runtime pm).

Actually I'm a bit wondering that if abstracting SCU domains into one
global virtual domain is certainly better than individual domains. For SCU based SoCs,
the power domain service is provided by SCU firmware, users don't have to care
too much about the HW internals. And according to SCU definition, each devices is
associated with a power domain, devices should treat it as a separate domain
and have to enable it before accessing HW. So one single global domain looks like
not exactly as SCU firmware defines. Not quite sure, but a bit feel like that....

Anyway, give us more time to fine tune it in the future might be a choice IMHO.

Thanks

Regards
Dong Aisheng

> Kind regards
> Uffe

Re: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[Ulf Hansson]

On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Wednesday, October 31, 2018 11:56 PM
> [...]
>> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >> -----Original Message-----
>> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> >> Sent: Wednesday, October 31, 2018 12:00 AM
>> > [...]
>> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >> > [...]
>> >> >
>> >> >> }
>> >> >> > +
>> >> >> > +static struct imx_sc_pm_domain * imx_scu_add_pm_domain(struct
>> >> >> > +device *dev, int idx,
>> >> >> > +                     const struct imx_sc_pd_range *pd_ranges) {
>> >> >> > +       struct imx_sc_pm_domain *sc_pd;
>> >> >> > +       int ret;
>> >> >> > +
>> >> >> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
>> >> >> > +       if (!sc_pd)
>> >> >> > +               return ERR_PTR(-ENOMEM);
>> >> >> > +
>> >> >> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
>> >> >> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
>> >> >> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
>> >> >>
>> >> >> So, this means you are going to register one genpd per device (one
>> >> >> for each uart, pwm etc), but there is actually a better option.
>> >> >>
>> >> >> What you seems to be needing is a way to "power on/off" devices,
>> >> >> rather than the PM domain. Right? The PM domain, is managed
>> >> >> internally
>> >> by the FW, no?
>> >> >>
>> >> >
>> >> > Yes, you're right.
>> >> >
>> >> >> In any case, it looks like what you should do is to implement the
>> >> >> ->attach|detach_dev() callback for the genpd and use the regular
>> >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(),
>> >> >> you should get the OF node for the device that is being attached
>> >> >> and then parse the power-domain cell containing the "resource id"
>> >> >> and store that in the per device struct generic_pm_domain_data (we
>> >> >> have void pointer there for storing these kind of things).
>> >> >>
>> >> >> Additionally, you need to implement the ->stop() and ->start()
>> >> >> callbacks of genpd, which is where you "power on/off" devices,
>> >> >> rather than using the above
>> >> >> ->power_on|off() callbacks.
>> >> >>
>> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c.
>> >> >> Apologize, I should have pointed you towards that reference
>> >> >> already in the earlier
>> >> version.
>> >> >>
>> >> >
>> >> > Appreciated for such detailed explanation. lt's a good suggestion
>> >> > and I really like to switch to it.  However, after digged a bit
>> >> > more, i found a few
>> >> blocking issues:
>> >> > 1. The .attach_dev() of power domain infrastructure still does not
>> >> > support
>> >> multi domains case.
>> >> >
>> >> > It looks like there's no way for .attach_dev() to understand which
>> >> > sub 'power
>> >> domain'
>> >> > the device is attaching for one global power domain provider like
>> >> > ti_sci as
>> >> you suggested.
>> >> > Secondly, the struct device *dev passed into is a virtual PD device.
>> >> > It does not help for parsing the real device resource id.
>> >> >
>> >> > So framework probably needs some extension to support multi power
>> >> domain cases.
>> >>
>> >> Right, you are correct.
>> >>
>> >> Let me think about this and see what I can come up with - unless you
>> >> already have something you want to propose?
>> >>
>> >
>> > I have thought we may need add two more params (real dev and pd index)
>> > for
>> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
>> > But I'm not sure if they are proper.
>>
>> Right.
>>
>> I don't think we need to change the definition of the callbacks.
>> However, from within the callbacks (or at least the ->attach_dev()) we need
>> access to the data you suggest above.
>>
>> To make that data available, genpd needs to store it in the per device struct
>> generic_pm_domain_data, but of course before invoking the
>> ->attach_dev() callback.
>>
>
> Yes, that's an option.
> Currently gpd_data is allocated in genpd_add_device() which looks like to be
> designed to attach device to the specified power domain passed in, so it's
> unware of whether it's multi domains or not. Then we may still need fine
> tune those functions in different abstract layers.

Yes, something like that. I don't think it should be that hard, I can
have a stab at it of you like?

>
>> >
>> >
>> >> >
>> >> > 2. It also breaks most of current drivers as the driver probe
>> >> > sequence behavior changed If removing .power_on() and .power_off()
>> >> > callback and
>> >> use .start() and .stop() instead.
>> >> >
>> >> > genpd_dev_pm_attach will only power up the domain and attach
>> >> > device, but will not call .start() which relies on device runtime
>> >> > pm. That means the device power is still not up before running
>> >> > driver probe function. For SCU enabled platforms, all device
>> >> > drivers accessing registers/clock without power domain enabled will
>> >> > trigger a HW access error. That means we need fix most of drivers
>> >> > probe sequence with proper
>> >> runtime pm. So I'm a bit wondering whether we should still keep the exist
>> way.
>> >>
>> >> I see what you are trying to say, but I am not sure why you consider
>> >> this as they would break? How did they work in the first place?
>> >>
>> >
>> > Most drivers does not require power domains before. E.g. GPIO, I2C, PWM,
>> MMC.
>> > Or even we need power domain(PUs/PCIE), they're enabled in
>> > .power_on()/.power_off() which has been already up before probe.
>> >
>> >> Anyway, the problem you are talking about have so far been addressed
>> >> by making buses/drivers to call pm_runtime_enable() and
>> >> pm_runtime_get_sync() - before trying to probe their devices.
>> >>
>> >
>> > Yes, called them early before accessing clock and hw registers.
>> >
>> >> Do you have an idea of the amount of drivers that needs to fixed, in
>> >> regards to this?
>> >>
>> >
>> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc.
>> > Suppose other need the same fix as we have not met this situation before.
>>
>> Okay, I see.
>>
>> >
>> >> I guess an option could be to, that via the ->attach_dev() callback
>> >> perform the same operations as also being done during ->start(). Of
>> >> course, you may need to keep a flag about this being done, so that
>> >> operations that need to be balanced with get/put or the like is
>> >> managed correctly, the next time
>> >> ->start|stop() becomes called.
>> >>
>> >
>> > That might be a way to try. The follow seems may introduce a bit
>> > complexities and driver needs to maintainer the runtime status along
>> > with framework in order to keep the balance?
>>
>> Yeah, it's likely not going to be very elegant, but should be rather self
>> contained in the PM domain driver and could serve as a way forward - while
>> working on fixing the drivers long term.
>>
>> >
>> >> >
>> >> > In summary, we probably may not be able to try ti_sci way before
>> >> > fixing
>> >> above two issues.
>> >> >
>> >> > Do you think I should wait for them to be fixed or if we could use
>> >> > the current
>> >> way at first?
>> >> > I might be a little intend to the second way as we now have a lot
>> >> > upstreaming work pending on this. But please let me know if you
>> >> > have a
>> >> different idea.
>> >>
>> >> As this is also about deploying a DTB with a correctly modeled HW, I
>> >> think we need to fix the above issues.
>> >>
>> >
>> > DTB are the same with #power-domain-cells 1 (resource ID) in either case.
>> > So DTB don't need change and driver could be optimized.
>>
>> You are correct.
>>
>> Although, how does $subject patch solves the case where a device may have
>> multiple PM domains attached to it? Or is left as a future improvement?
>>
>
> IIRC this patch does not have such issue for multi PM domains as we have
> individual power domains for each resources. Devices will be attached to
> each individual domains in __genpd_dev_pm_attach rather than the single
> one global virtual SCU domain. Furthermore we're using domains .power_on()/
> power_off() callback which means the power will be up before the probe.
> (no the issue like when using .attach_dev()/.detach_dev() and run .start()/stop()
> via runtime pm).

I don't follow your reasoning here, sorry. You stated earlier, that
there was a need to support multiple PM domains per device.

How can that problem be resolved by using one genpd per device? What
am I missing?

>
> Actually I'm a bit wondering that if abstracting SCU domains into one
> global virtual domain is certainly better than individual domains. For SCU based SoCs,
> the power domain service is provided by SCU firmware, users don't have to care
> too much about the HW internals. And according to SCU definition, each devices is
> associated with a power domain, devices should treat it as a separate domain
> and have to enable it before accessing HW. So one single global domain looks like
> not exactly as SCU firmware defines. Not quite sure, but a bit feel like that....

I don't agree with you here, sorry. As also pointed out by Rob
earlier, in regards to the DT bindings. Simply put, I doubt the SoC
deploys separate power rails/islands for each of the available
devices. Instead, it seems like the SCU interface, allows its clients
to controls "power" to each of the available *devices* - and not power
domains.

Anyway, we are debating how to implement this in software and not in DT. :-)

That said, if you strongly feel that the short cut, about registering
one genpd per device and thus suffer from the overhead it gives, is
the best approach to start with, feel free to do that.

Kind regards
Uffe

RE: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[A.s. Dong]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Thursday, November 1, 2018 5:49 AM
[...]
> 
> On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> -----Original Message-----
> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> Sent: Wednesday, October 31, 2018 11:56 PM
> > [...]
> >> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> >> Sent: Wednesday, October 31, 2018 12:00 AM
> >> > [...]
> >> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com>
> wrote:
> >> >> > [...]
> >> >> >
> >> >> >> }
> >> >> >> > +
> >> >> >> > +static struct imx_sc_pm_domain *
> >> >> >> > +imx_scu_add_pm_domain(struct device *dev, int idx,
> >> >> >> > +                     const struct imx_sc_pd_range *pd_ranges)
> {
> >> >> >> > +       struct imx_sc_pm_domain *sc_pd;
> >> >> >> > +       int ret;
> >> >> >> > +
> >> >> >> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
> >> >> >> > +       if (!sc_pd)
> >> >> >> > +               return ERR_PTR(-ENOMEM);
> >> >> >> > +
> >> >> >> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
> >> >> >> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
> >> >> >> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
> >> >> >>
> >> >> >> So, this means you are going to register one genpd per device
> >> >> >> (one for each uart, pwm etc), but there is actually a better option.
> >> >> >>
> >> >> >> What you seems to be needing is a way to "power on/off"
> >> >> >> devices, rather than the PM domain. Right? The PM domain, is
> >> >> >> managed internally
> >> >> by the FW, no?
> >> >> >>
> >> >> >
> >> >> > Yes, you're right.
> >> >> >
> >> >> >> In any case, it looks like what you should do is to implement
> >> >> >> the
> >> >> >> ->attach|detach_dev() callback for the genpd and use the
> >> >> >> ->attach|regular
> >> >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(),
> >> >> >> you should get the OF node for the device that is being
> >> >> >> attached and then parse the power-domain cell containing the
> "resource id"
> >> >> >> and store that in the per device struct generic_pm_domain_data
> >> >> >> (we have void pointer there for storing these kind of things).
> >> >> >>
> >> >> >> Additionally, you need to implement the ->stop() and ->start()
> >> >> >> callbacks of genpd, which is where you "power on/off" devices,
> >> >> >> rather than using the above
> >> >> >> ->power_on|off() callbacks.
> >> >> >>
> >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c.
> >> >> >> Apologize, I should have pointed you towards that reference
> >> >> >> already in the earlier
> >> >> version.
> >> >> >>
> >> >> >
> >> >> > Appreciated for such detailed explanation. lt's a good
> >> >> > suggestion and I really like to switch to it.  However, after
> >> >> > digged a bit more, i found a few
> >> >> blocking issues:
> >> >> > 1. The .attach_dev() of power domain infrastructure still does
> >> >> > not support
> >> >> multi domains case.
> >> >> >
> >> >> > It looks like there's no way for .attach_dev() to understand
> >> >> > which sub 'power
> >> >> domain'
> >> >> > the device is attaching for one global power domain provider
> >> >> > like ti_sci as
> >> >> you suggested.
> >> >> > Secondly, the struct device *dev passed into is a virtual PD device.
> >> >> > It does not help for parsing the real device resource id.
> >> >> >
> >> >> > So framework probably needs some extension to support multi
> >> >> > power
> >> >> domain cases.
> >> >>
> >> >> Right, you are correct.
> >> >>
> >> >> Let me think about this and see what I can come up with - unless
> >> >> you already have something you want to propose?
> >> >>
> >> >
> >> > I have thought we may need add two more params (real dev and pd
> >> > index) for
> >> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
> >> > But I'm not sure if they are proper.
> >>
> >> Right.
> >>
> >> I don't think we need to change the definition of the callbacks.
> >> However, from within the callbacks (or at least the ->attach_dev())
> >> we need access to the data you suggest above.
> >>
> >> To make that data available, genpd needs to store it in the per
> >> device struct generic_pm_domain_data, but of course before invoking
> >> the
> >> ->attach_dev() callback.
> >>
> >
> > Yes, that's an option.
> > Currently gpd_data is allocated in genpd_add_device() which looks like
> > to be designed to attach device to the specified power domain passed
> > in, so it's unware of whether it's multi domains or not. Then we may
> > still need fine tune those functions in different abstract layers.
> 
> Yes, something like that. I don't think it should be that hard, I can have a stab
> at it of you like?
> 

Yes, that's fine to me. I just don't have too much time to handle it right now.
I can help do some basic function test later. May not be stress as we still have
no devices using multi domains supported In kernel.
So I can construct simple case to test.

> >
> >> >
> >> >
> >> >> >
> >> >> > 2. It also breaks most of current drivers as the driver probe
> >> >> > sequence behavior changed If removing .power_on() and
> >> >> > .power_off() callback and
> >> >> use .start() and .stop() instead.
> >> >> >
> >> >> > genpd_dev_pm_attach will only power up the domain and attach
> >> >> > device, but will not call .start() which relies on device
> >> >> > runtime pm. That means the device power is still not up before
> >> >> > running driver probe function. For SCU enabled platforms, all
> >> >> > device drivers accessing registers/clock without power domain
> >> >> > enabled will trigger a HW access error. That means we need fix
> >> >> > most of drivers probe sequence with proper
> >> >> runtime pm. So I'm a bit wondering whether we should still keep
> >> >> the exist
> >> way.
> >> >>
> >> >> I see what you are trying to say, but I am not sure why you
> >> >> consider this as they would break? How did they work in the first place?
> >> >>
> >> >
> >> > Most drivers does not require power domains before. E.g. GPIO, I2C,
> >> > PWM,
> >> MMC.
> >> > Or even we need power domain(PUs/PCIE), they're enabled in
> >> > .power_on()/.power_off() which has been already up before probe.
> >> >
> >> >> Anyway, the problem you are talking about have so far been
> >> >> addressed by making buses/drivers to call pm_runtime_enable() and
> >> >> pm_runtime_get_sync() - before trying to probe their devices.
> >> >>
> >> >
> >> > Yes, called them early before accessing clock and hw registers.
> >> >
> >> >> Do you have an idea of the amount of drivers that needs to fixed,
> >> >> in regards to this?
> >> >>
> >> >
> >> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc.
> >> > Suppose other need the same fix as we have not met this situation before.
> >>
> >> Okay, I see.
> >>
> >> >
> >> >> I guess an option could be to, that via the ->attach_dev()
> >> >> callback perform the same operations as also being done during
> >> >> ->start(). Of course, you may need to keep a flag about this being
> >> >> done, so that operations that need to be balanced with get/put or
> >> >> the like is managed correctly, the next time
> >> >> ->start|stop() becomes called.
> >> >>
> >> >
> >> > That might be a way to try. The follow seems may introduce a bit
> >> > complexities and driver needs to maintainer the runtime status
> >> > along with framework in order to keep the balance?
> >>
> >> Yeah, it's likely not going to be very elegant, but should be rather
> >> self contained in the PM domain driver and could serve as a way
> >> forward - while working on fixing the drivers long term.
> >>
> >> >
> >> >> >
> >> >> > In summary, we probably may not be able to try ti_sci way before
> >> >> > fixing
> >> >> above two issues.
> >> >> >
> >> >> > Do you think I should wait for them to be fixed or if we could
> >> >> > use the current
> >> >> way at first?
> >> >> > I might be a little intend to the second way as we now have a
> >> >> > lot upstreaming work pending on this. But please let me know if
> >> >> > you have a
> >> >> different idea.
> >> >>
> >> >> As this is also about deploying a DTB with a correctly modeled HW,
> >> >> I think we need to fix the above issues.
> >> >>
> >> >
> >> > DTB are the same with #power-domain-cells 1 (resource ID) in either case.
> >> > So DTB don't need change and driver could be optimized.
> >>
> >> You are correct.
> >>
> >> Although, how does $subject patch solves the case where a device may
> >> have multiple PM domains attached to it? Or is left as a future
> improvement?
> >>
> >
> > IIRC this patch does not have such issue for multi PM domains as we
> > have individual power domains for each resources. Devices will be
> > attached to each individual domains in __genpd_dev_pm_attach rather
> > than the single one global virtual SCU domain. Furthermore we're using
> > domains .power_on()/
> > power_off() callback which means the power will be up before the probe.
> > (no the issue like when using .attach_dev()/.detach_dev() and run
> > .start()/stop() via runtime pm).
> 
> I don't follow your reasoning here, sorry. You stated earlier, that there was a
> need to support multiple PM domains per device.
> 
> How can that problem be resolved by using one genpd per device? What am I
> missing?

Sorry I may be not clear before.
We still support using multiple power domains.
For example:
Mipi_csi0 {
	...
	Power_domains = <&PD SC_R_IPI_CH0>,
				   <&PD SC_R_MIPI_CSI0>;
	Power_domains_names = "ss", "csi";
};

The difference is that each SC resource has individual power domains in driver
And __genpd_dev_pm_attach will bind the virtual devices to the correct individual
domain and do properly power_on/off of that domains.
So it does not have the issue that .attach_dev() has, also does not have issue that
power domain is still not up before running probe.
(The detailed using is just like Tegra xHCI you pointed to me before)

> 
> >
> > Actually I'm a bit wondering that if abstracting SCU domains into one
> > global virtual domain is certainly better than individual domains. For
> > SCU based SoCs, the power domain service is provided by SCU firmware,
> > users don't have to care too much about the HW internals. And
> > according to SCU definition, each devices is associated with a power
> > domain, devices should treat it as a separate domain and have to
> > enable it before accessing HW. So one single global domain looks like not
> exactly as SCU firmware defines. Not quite sure, but a bit feel like that....
> 
> I don't agree with you here, sorry. As also pointed out by Rob earlier, in regards
> to the DT bindings. Simply put, I doubt the SoC deploys separate power
> rails/islands for each of the available devices. Instead, it seems like the SCU
> interface, allows its clients to controls "power" to each of the available
> *devices* - and not power domains.
> 

Yes, that's the trick part. For me, I feel like devices are unware of HW internals
but using SCU power domains service. So devices think it's seaparte domains
may not be strictly wrong.
Anyway, I'm also agree with the later way (signal global power domains with
.attach_dev()/.start() to power on device). Just because we met those two
Issues blocking us to do it right now.

> Anyway, we are debating how to implement this in software and not in DT. :-)
> 
> That said, if you strongly feel that the short cut, about registering one genpd
> per device and thus suffer from the overhead it gives, is the best approach to
> start with, feel free to do that.
> 

I just feel like that starting with current way could give us more time to fine tune
it later without blocking other drivers upstreaming. And maybe we can do better
work once we have real multi domains users in kernel.

Anyway, I can do either, so please me know if you still think we should strictly
start with signal global power domain way.

Regards
Dong Aisheng

> Kind regards
> Uffe

Re: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[Ulf Hansson]

On 1 November 2018 at 02:28, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Thursday, November 1, 2018 5:49 AM
> [...]
>>
>> On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >> -----Original Message-----
>> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> >> Sent: Wednesday, October 31, 2018 11:56 PM
>> > [...]
>> >> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >> >> -----Original Message-----
>> >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> >> >> Sent: Wednesday, October 31, 2018 12:00 AM
>> >> > [...]
>> >> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com>
>> wrote:
>> >> >> > [...]
>> >> >> >
>> >> >> >> }
>> >> >> >> > +
>> >> >> >> > +static struct imx_sc_pm_domain *
>> >> >> >> > +imx_scu_add_pm_domain(struct device *dev, int idx,
>> >> >> >> > +                     const struct imx_sc_pd_range *pd_ranges)
>> {
>> >> >> >> > +       struct imx_sc_pm_domain *sc_pd;
>> >> >> >> > +       int ret;
>> >> >> >> > +
>> >> >> >> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL);
>> >> >> >> > +       if (!sc_pd)
>> >> >> >> > +               return ERR_PTR(-ENOMEM);
>> >> >> >> > +
>> >> >> >> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
>> >> >> >> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
>> >> >> >> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
>> >> >> >>
>> >> >> >> So, this means you are going to register one genpd per device
>> >> >> >> (one for each uart, pwm etc), but there is actually a better option.
>> >> >> >>
>> >> >> >> What you seems to be needing is a way to "power on/off"
>> >> >> >> devices, rather than the PM domain. Right? The PM domain, is
>> >> >> >> managed internally
>> >> >> by the FW, no?
>> >> >> >>
>> >> >> >
>> >> >> > Yes, you're right.
>> >> >> >
>> >> >> >> In any case, it looks like what you should do is to implement
>> >> >> >> the
>> >> >> >> ->attach|detach_dev() callback for the genpd and use the
>> >> >> >> ->attach|regular
>> >> >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(),
>> >> >> >> you should get the OF node for the device that is being
>> >> >> >> attached and then parse the power-domain cell containing the
>> "resource id"
>> >> >> >> and store that in the per device struct generic_pm_domain_data
>> >> >> >> (we have void pointer there for storing these kind of things).
>> >> >> >>
>> >> >> >> Additionally, you need to implement the ->stop() and ->start()
>> >> >> >> callbacks of genpd, which is where you "power on/off" devices,
>> >> >> >> rather than using the above
>> >> >> >> ->power_on|off() callbacks.
>> >> >> >>
>> >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c.
>> >> >> >> Apologize, I should have pointed you towards that reference
>> >> >> >> already in the earlier
>> >> >> version.
>> >> >> >>
>> >> >> >
>> >> >> > Appreciated for such detailed explanation. lt's a good
>> >> >> > suggestion and I really like to switch to it.  However, after
>> >> >> > digged a bit more, i found a few
>> >> >> blocking issues:
>> >> >> > 1. The .attach_dev() of power domain infrastructure still does
>> >> >> > not support
>> >> >> multi domains case.
>> >> >> >
>> >> >> > It looks like there's no way for .attach_dev() to understand
>> >> >> > which sub 'power
>> >> >> domain'
>> >> >> > the device is attaching for one global power domain provider
>> >> >> > like ti_sci as
>> >> >> you suggested.
>> >> >> > Secondly, the struct device *dev passed into is a virtual PD device.
>> >> >> > It does not help for parsing the real device resource id.
>> >> >> >
>> >> >> > So framework probably needs some extension to support multi
>> >> >> > power
>> >> >> domain cases.
>> >> >>
>> >> >> Right, you are correct.
>> >> >>
>> >> >> Let me think about this and see what I can come up with - unless
>> >> >> you already have something you want to propose?
>> >> >>
>> >> >
>> >> > I have thought we may need add two more params (real dev and pd
>> >> > index) for
>> >> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
>> >> > But I'm not sure if they are proper.
>> >>
>> >> Right.
>> >>
>> >> I don't think we need to change the definition of the callbacks.
>> >> However, from within the callbacks (or at least the ->attach_dev())
>> >> we need access to the data you suggest above.
>> >>
>> >> To make that data available, genpd needs to store it in the per
>> >> device struct generic_pm_domain_data, but of course before invoking
>> >> the
>> >> ->attach_dev() callback.
>> >>
>> >
>> > Yes, that's an option.
>> > Currently gpd_data is allocated in genpd_add_device() which looks like
>> > to be designed to attach device to the specified power domain passed
>> > in, so it's unware of whether it's multi domains or not. Then we may
>> > still need fine tune those functions in different abstract layers.
>>
>> Yes, something like that. I don't think it should be that hard, I can have a stab
>> at it of you like?
>>
>
> Yes, that's fine to me. I just don't have too much time to handle it right now.
> I can help do some basic function test later. May not be stress as we still have
> no devices using multi domains supported In kernel.
> So I can construct simple case to test.

Okay, I am adding it to my TODO list for genpd.

>
>> >
>> >> >
>> >> >
>> >> >> >
>> >> >> > 2. It also breaks most of current drivers as the driver probe
>> >> >> > sequence behavior changed If removing .power_on() and
>> >> >> > .power_off() callback and
>> >> >> use .start() and .stop() instead.
>> >> >> >
>> >> >> > genpd_dev_pm_attach will only power up the domain and attach
>> >> >> > device, but will not call .start() which relies on device
>> >> >> > runtime pm. That means the device power is still not up before
>> >> >> > running driver probe function. For SCU enabled platforms, all
>> >> >> > device drivers accessing registers/clock without power domain
>> >> >> > enabled will trigger a HW access error. That means we need fix
>> >> >> > most of drivers probe sequence with proper
>> >> >> runtime pm. So I'm a bit wondering whether we should still keep
>> >> >> the exist
>> >> way.
>> >> >>
>> >> >> I see what you are trying to say, but I am not sure why you
>> >> >> consider this as they would break? How did they work in the first place?
>> >> >>
>> >> >
>> >> > Most drivers does not require power domains before. E.g. GPIO, I2C,
>> >> > PWM,
>> >> MMC.
>> >> > Or even we need power domain(PUs/PCIE), they're enabled in
>> >> > .power_on()/.power_off() which has been already up before probe.
>> >> >
>> >> >> Anyway, the problem you are talking about have so far been
>> >> >> addressed by making buses/drivers to call pm_runtime_enable() and
>> >> >> pm_runtime_get_sync() - before trying to probe their devices.
>> >> >>
>> >> >
>> >> > Yes, called them early before accessing clock and hw registers.
>> >> >
>> >> >> Do you have an idea of the amount of drivers that needs to fixed,
>> >> >> in regards to this?
>> >> >>
>> >> >
>> >> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc.
>> >> > Suppose other need the same fix as we have not met this situation before.
>> >>
>> >> Okay, I see.
>> >>
>> >> >
>> >> >> I guess an option could be to, that via the ->attach_dev()
>> >> >> callback perform the same operations as also being done during
>> >> >> ->start(). Of course, you may need to keep a flag about this being
>> >> >> done, so that operations that need to be balanced with get/put or
>> >> >> the like is managed correctly, the next time
>> >> >> ->start|stop() becomes called.
>> >> >>
>> >> >
>> >> > That might be a way to try. The follow seems may introduce a bit
>> >> > complexities and driver needs to maintainer the runtime status
>> >> > along with framework in order to keep the balance?
>> >>
>> >> Yeah, it's likely not going to be very elegant, but should be rather
>> >> self contained in the PM domain driver and could serve as a way
>> >> forward - while working on fixing the drivers long term.
>> >>
>> >> >
>> >> >> >
>> >> >> > In summary, we probably may not be able to try ti_sci way before
>> >> >> > fixing
>> >> >> above two issues.
>> >> >> >
>> >> >> > Do you think I should wait for them to be fixed or if we could
>> >> >> > use the current
>> >> >> way at first?
>> >> >> > I might be a little intend to the second way as we now have a
>> >> >> > lot upstreaming work pending on this. But please let me know if
>> >> >> > you have a
>> >> >> different idea.
>> >> >>
>> >> >> As this is also about deploying a DTB with a correctly modeled HW,
>> >> >> I think we need to fix the above issues.
>> >> >>
>> >> >
>> >> > DTB are the same with #power-domain-cells 1 (resource ID) in either case.
>> >> > So DTB don't need change and driver could be optimized.
>> >>
>> >> You are correct.
>> >>
>> >> Although, how does $subject patch solves the case where a device may
>> >> have multiple PM domains attached to it? Or is left as a future
>> improvement?
>> >>
>> >
>> > IIRC this patch does not have such issue for multi PM domains as we
>> > have individual power domains for each resources. Devices will be
>> > attached to each individual domains in __genpd_dev_pm_attach rather
>> > than the single one global virtual SCU domain. Furthermore we're using
>> > domains .power_on()/
>> > power_off() callback which means the power will be up before the probe.
>> > (no the issue like when using .attach_dev()/.detach_dev() and run
>> > .start()/stop() via runtime pm).
>>
>> I don't follow your reasoning here, sorry. You stated earlier, that there was a
>> need to support multiple PM domains per device.
>>
>> How can that problem be resolved by using one genpd per device? What am I
>> missing?
>
> Sorry I may be not clear before.
> We still support using multiple power domains.
> For example:
> Mipi_csi0 {
>         ...
>         Power_domains = <&PD SC_R_IPI_CH0>,
>                                    <&PD SC_R_MIPI_CSI0>;
>         Power_domains_names = "ss", "csi";
> };
>
> The difference is that each SC resource has individual power domains in driver
> And __genpd_dev_pm_attach will bind the virtual devices to the correct individual
> domain and do properly power_on/off of that domains.
> So it does not have the issue that .attach_dev() has, also does not have issue that
> power domain is still not up before running probe.
> (The detailed using is just like Tegra xHCI you pointed to me before)

Aha, I see. Thanks for clarifying.

>
>>
>> >
>> > Actually I'm a bit wondering that if abstracting SCU domains into one
>> > global virtual domain is certainly better than individual domains. For
>> > SCU based SoCs, the power domain service is provided by SCU firmware,
>> > users don't have to care too much about the HW internals. And
>> > according to SCU definition, each devices is associated with a power
>> > domain, devices should treat it as a separate domain and have to
>> > enable it before accessing HW. So one single global domain looks like not
>> exactly as SCU firmware defines. Not quite sure, but a bit feel like that....
>>
>> I don't agree with you here, sorry. As also pointed out by Rob earlier, in regards
>> to the DT bindings. Simply put, I doubt the SoC deploys separate power
>> rails/islands for each of the available devices. Instead, it seems like the SCU
>> interface, allows its clients to controls "power" to each of the available
>> *devices* - and not power domains.
>>
>
> Yes, that's the trick part. For me, I feel like devices are unware of HW internals
> but using SCU power domains service. So devices think it's seaparte domains
> may not be strictly wrong.
> Anyway, I'm also agree with the later way (signal global power domains with
> .attach_dev()/.start() to power on device). Just because we met those two
> Issues blocking us to do it right now.
>
>> Anyway, we are debating how to implement this in software and not in DT. :-)
>>
>> That said, if you strongly feel that the short cut, about registering one genpd
>> per device and thus suffer from the overhead it gives, is the best approach to
>> start with, feel free to do that.
>>
>
> I just feel like that starting with current way could give us more time to fine tune
> it later without blocking other drivers upstreaming. And maybe we can do better
> work once we have real multi domains users in kernel.
>
> Anyway, I can do either, so please me know if you still think we should strictly
> start with signal global power domain way.

As stated, feel free to pick whatever option that makes sense to you.

However, if you decide to start with the approach taken in $subject
patch, it would be nice if the new drivers/firmware/imx/scu-pd.c file,
could have some comments in the top, about what things are needed to
convert to the "single global domain", just so we don't forget about
it.

Kind regards
Uffe

RE: [PATCH V8 5/5] firmware: imx: add SCU power domain driver

[A.s. Dong]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Thursday, November 1, 2018 5:52 PM
[...]
> 
> On 1 November 2018 at 02:28, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> -----Original Message-----
> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> Sent: Thursday, November 1, 2018 5:49 AM
> > [...]
> >>
> >> On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> >> Sent: Wednesday, October 31, 2018 11:56 PM
> >> > [...]
> >> >> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com>
> wrote:
> >> >> >> -----Original Message-----
> >> >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> >> >> Sent: Wednesday, October 31, 2018 12:00 AM
> >> >> > [...]
> >> >> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com>
> >> wrote:
> >> >> >> > [...]
> >> >> >> >
> >> >> >> >> }
> >> >> >> >> > +
> >> >> >> >> > +static struct imx_sc_pm_domain *
> >> >> >> >> > +imx_scu_add_pm_domain(struct device *dev, int idx,
> >> >> >> >> > +                     const struct imx_sc_pd_range
> >> >> >> >> > +*pd_ranges)
> >> {
> >> >> >> >> > +       struct imx_sc_pm_domain *sc_pd;
> >> >> >> >> > +       int ret;
> >> >> >> >> > +
> >> >> >> >> > +       sc_pd = devm_kzalloc(dev, sizeof(*sc_pd),
> GFP_KERNEL);
> >> >> >> >> > +       if (!sc_pd)
> >> >> >> >> > +               return ERR_PTR(-ENOMEM);
> >> >> >> >> > +
> >> >> >> >> > +       sc_pd->rsrc = pd_ranges->rsrc + idx;
> >> >> >> >> > +       sc_pd->pd.power_off = imx_sc_pd_power_off;
> >> >> >> >> > +       sc_pd->pd.power_on = imx_sc_pd_power_on;
> >> >> >> >>
> >> >> >> >> So, this means you are going to register one genpd per
> >> >> >> >> device (one for each uart, pwm etc), but there is actually a better
> option.
> >> >> >> >>
> >> >> >> >> What you seems to be needing is a way to "power on/off"
> >> >> >> >> devices, rather than the PM domain. Right? The PM domain, is
> >> >> >> >> managed internally
> >> >> >> by the FW, no?
> >> >> >> >>
> >> >> >> >
> >> >> >> > Yes, you're right.
> >> >> >> >
> >> >> >> >> In any case, it looks like what you should do is to
> >> >> >> >> implement the
> >> >> >> >> ->attach|detach_dev() callback for the genpd and use the
> >> >> >> >> ->attach|regular
> >> >> >> >> of_genpd_add_provider_simple(). From within the
> >> >> >> >> ->attach_dev(), you should get the OF node for the device
> >> >> >> >> that is being attached and then parse the power-domain cell
> >> >> >> >> containing the
> >> "resource id"
> >> >> >> >> and store that in the per device struct
> >> >> >> >> generic_pm_domain_data (we have void pointer there for storing
> these kind of things).
> >> >> >> >>
> >> >> >> >> Additionally, you need to implement the ->stop() and
> >> >> >> >> ->start() callbacks of genpd, which is where you "power
> >> >> >> >> on/off" devices, rather than using the above
> >> >> >> >> ->power_on|off() callbacks.
> >> >> >> >>
> >> >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c.
> >> >> >> >> Apologize, I should have pointed you towards that reference
> >> >> >> >> already in the earlier
> >> >> >> version.
> >> >> >> >>
> >> >> >> >
> >> >> >> > Appreciated for such detailed explanation. lt's a good
> >> >> >> > suggestion and I really like to switch to it.  However, after
> >> >> >> > digged a bit more, i found a few
> >> >> >> blocking issues:
> >> >> >> > 1. The .attach_dev() of power domain infrastructure still
> >> >> >> > does not support
> >> >> >> multi domains case.
> >> >> >> >
> >> >> >> > It looks like there's no way for .attach_dev() to understand
> >> >> >> > which sub 'power
> >> >> >> domain'
> >> >> >> > the device is attaching for one global power domain provider
> >> >> >> > like ti_sci as
> >> >> >> you suggested.
> >> >> >> > Secondly, the struct device *dev passed into is a virtual PD device.
> >> >> >> > It does not help for parsing the real device resource id.
> >> >> >> >
> >> >> >> > So framework probably needs some extension to support multi
> >> >> >> > power
> >> >> >> domain cases.
> >> >> >>
> >> >> >> Right, you are correct.
> >> >> >>
> >> >> >> Let me think about this and see what I can come up with -
> >> >> >> unless you already have something you want to propose?
> >> >> >>
> >> >> >
> >> >> > I have thought we may need add two more params (real dev and pd
> >> >> > index) for
> >> >> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case.
> >> >> > But I'm not sure if they are proper.
> >> >>
> >> >> Right.
> >> >>
> >> >> I don't think we need to change the definition of the callbacks.
> >> >> However, from within the callbacks (or at least the
> >> >> ->attach_dev()) we need access to the data you suggest above.
> >> >>
> >> >> To make that data available, genpd needs to store it in the per
> >> >> device struct generic_pm_domain_data, but of course before
> >> >> invoking the
> >> >> ->attach_dev() callback.
> >> >>
> >> >
> >> > Yes, that's an option.
> >> > Currently gpd_data is allocated in genpd_add_device() which looks
> >> > like to be designed to attach device to the specified power domain
> >> > passed in, so it's unware of whether it's multi domains or not.
> >> > Then we may still need fine tune those functions in different abstract
> layers.
> >>
> >> Yes, something like that. I don't think it should be that hard, I can
> >> have a stab at it of you like?
> >>
> >
> > Yes, that's fine to me. I just don't have too much time to handle it right now.
> > I can help do some basic function test later. May not be stress as we
> > still have no devices using multi domains supported In kernel.
> > So I can construct simple case to test.
> 
> Okay, I am adding it to my TODO list for genpd.
> 

Great.

> >
> >> >
> >> >> >
> >> >> >
> >> >> >> >
> >> >> >> > 2. It also breaks most of current drivers as the driver probe
> >> >> >> > sequence behavior changed If removing .power_on() and
> >> >> >> > .power_off() callback and
> >> >> >> use .start() and .stop() instead.
> >> >> >> >
> >> >> >> > genpd_dev_pm_attach will only power up the domain and attach
> >> >> >> > device, but will not call .start() which relies on device
> >> >> >> > runtime pm. That means the device power is still not up
> >> >> >> > before running driver probe function. For SCU enabled
> >> >> >> > platforms, all device drivers accessing registers/clock
> >> >> >> > without power domain enabled will trigger a HW access error.
> >> >> >> > That means we need fix most of drivers probe sequence with
> >> >> >> > proper
> >> >> >> runtime pm. So I'm a bit wondering whether we should still keep
> >> >> >> the exist
> >> >> way.
> >> >> >>
> >> >> >> I see what you are trying to say, but I am not sure why you
> >> >> >> consider this as they would break? How did they work in the first
> place?
> >> >> >>
> >> >> >
> >> >> > Most drivers does not require power domains before. E.g. GPIO,
> >> >> > I2C, PWM,
> >> >> MMC.
> >> >> > Or even we need power domain(PUs/PCIE), they're enabled in
> >> >> > .power_on()/.power_off() which has been already up before probe.
> >> >> >
> >> >> >> Anyway, the problem you are talking about have so far been
> >> >> >> addressed by making buses/drivers to call pm_runtime_enable()
> >> >> >> and
> >> >> >> pm_runtime_get_sync() - before trying to probe their devices.
> >> >> >>
> >> >> >
> >> >> > Yes, called them early before accessing clock and hw registers.
> >> >> >
> >> >> >> Do you have an idea of the amount of drivers that needs to
> >> >> >> fixed, in regards to this?
> >> >> >>
> >> >> >
> >> >> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and
> etc.
> >> >> > Suppose other need the same fix as we have not met this situation
> before.
> >> >>
> >> >> Okay, I see.
> >> >>
> >> >> >
> >> >> >> I guess an option could be to, that via the ->attach_dev()
> >> >> >> callback perform the same operations as also being done during
> >> >> >> ->start(). Of course, you may need to keep a flag about this
> >> >> >> ->being
> >> >> >> done, so that operations that need to be balanced with get/put
> >> >> >> or the like is managed correctly, the next time
> >> >> >> ->start|stop() becomes called.
> >> >> >>
> >> >> >
> >> >> > That might be a way to try. The follow seems may introduce a bit
> >> >> > complexities and driver needs to maintainer the runtime status
> >> >> > along with framework in order to keep the balance?
> >> >>
> >> >> Yeah, it's likely not going to be very elegant, but should be
> >> >> rather self contained in the PM domain driver and could serve as a
> >> >> way forward - while working on fixing the drivers long term.
> >> >>
> >> >> >
> >> >> >> >
> >> >> >> > In summary, we probably may not be able to try ti_sci way
> >> >> >> > before fixing
> >> >> >> above two issues.
> >> >> >> >
> >> >> >> > Do you think I should wait for them to be fixed or if we
> >> >> >> > could use the current
> >> >> >> way at first?
> >> >> >> > I might be a little intend to the second way as we now have a
> >> >> >> > lot upstreaming work pending on this. But please let me know
> >> >> >> > if you have a
> >> >> >> different idea.
> >> >> >>
> >> >> >> As this is also about deploying a DTB with a correctly modeled
> >> >> >> HW, I think we need to fix the above issues.
> >> >> >>
> >> >> >
> >> >> > DTB are the same with #power-domain-cells 1 (resource ID) in either
> case.
> >> >> > So DTB don't need change and driver could be optimized.
> >> >>
> >> >> You are correct.
> >> >>
> >> >> Although, how does $subject patch solves the case where a device
> >> >> may have multiple PM domains attached to it? Or is left as a
> >> >> future
> >> improvement?
> >> >>
> >> >
> >> > IIRC this patch does not have such issue for multi PM domains as we
> >> > have individual power domains for each resources. Devices will be
> >> > attached to each individual domains in __genpd_dev_pm_attach rather
> >> > than the single one global virtual SCU domain. Furthermore we're
> >> > using domains .power_on()/
> >> > power_off() callback which means the power will be up before the probe.
> >> > (no the issue like when using .attach_dev()/.detach_dev() and run
> >> > .start()/stop() via runtime pm).
> >>
> >> I don't follow your reasoning here, sorry. You stated earlier, that
> >> there was a need to support multiple PM domains per device.
> >>
> >> How can that problem be resolved by using one genpd per device? What
> >> am I missing?
> >
> > Sorry I may be not clear before.
> > We still support using multiple power domains.
> > For example:
> > Mipi_csi0 {
> >         ...
> >         Power_domains = <&PD SC_R_IPI_CH0>,
> >                                    <&PD SC_R_MIPI_CSI0>;
> >         Power_domains_names = "ss", "csi"; };
> >
> > The difference is that each SC resource has individual power domains
> > in driver And __genpd_dev_pm_attach will bind the virtual devices to
> > the correct individual domain and do properly power_on/off of that
> domains.
> > So it does not have the issue that .attach_dev() has, also does not
> > have issue that power domain is still not up before running probe.
> > (The detailed using is just like Tegra xHCI you pointed to me before)
> 
> Aha, I see. Thanks for clarifying.
> 
> >
> >>
> >> >
> >> > Actually I'm a bit wondering that if abstracting SCU domains into
> >> > one global virtual domain is certainly better than individual
> >> > domains. For SCU based SoCs, the power domain service is provided
> >> > by SCU firmware, users don't have to care too much about the HW
> >> > internals. And according to SCU definition, each devices is
> >> > associated with a power domain, devices should treat it as a
> >> > separate domain and have to enable it before accessing HW. So one
> >> > single global domain looks like not
> >> exactly as SCU firmware defines. Not quite sure, but a bit feel like that....
> >>
> >> I don't agree with you here, sorry. As also pointed out by Rob
> >> earlier, in regards to the DT bindings. Simply put, I doubt the SoC
> >> deploys separate power rails/islands for each of the available
> >> devices. Instead, it seems like the SCU interface, allows its clients
> >> to controls "power" to each of the available
> >> *devices* - and not power domains.
> >>
> >
> > Yes, that's the trick part. For me, I feel like devices are unware of
> > HW internals but using SCU power domains service. So devices think
> > it's seaparte domains may not be strictly wrong.
> > Anyway, I'm also agree with the later way (signal global power domains
> > with
> > .attach_dev()/.start() to power on device). Just because we met those
> > two Issues blocking us to do it right now.
> >
> >> Anyway, we are debating how to implement this in software and not in
> >> DT. :-)
> >>
> >> That said, if you strongly feel that the short cut, about registering
> >> one genpd per device and thus suffer from the overhead it gives, is
> >> the best approach to start with, feel free to do that.
> >>
> >
> > I just feel like that starting with current way could give us more
> > time to fine tune it later without blocking other drivers upstreaming.
> > And maybe we can do better work once we have real multi domains users in
> kernel.
> >
> > Anyway, I can do either, so please me know if you still think we
> > should strictly start with signal global power domain way.
> 
> As stated, feel free to pick whatever option that makes sense to you.
> 
> However, if you decide to start with the approach taken in $subject patch, it
> would be nice if the new drivers/firmware/imx/scu-pd.c file, could have some
> comments in the top, about what things are needed to convert to the "single
> global domain", just so we don't forget about it.
> 

Sounds good to me.
Thanks for the suggestion.
I will do it.

Regards
Dong Aisheng

> Kind regards
> Uffe

Re: [PATCH V8 0/5] soc: imx: add scu power domain driver

[Ulf Hansson]

On 28 October 2018 at 16:19, A.s. Dong <aisheng.dong@nxp.com> wrote:
> This patch set adds the scu based power domain driver.
> It depends on SCU driver.
>
> Change Log:
> v7->v8:
>  * update to #power-domain-cells 1 binding
> v6->v7:
>  * keep "fsl,scu-pd" as fall back compatible string
> v5->v6:
>  * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd
>    accordingly
> v4->v5:
>  * some minor improvements according to Sascha's suggestion
>    Note: did not use dev_* print functions due to we already have proper
>          prefix by redefining pr_fmt. So it seems not quite neccesary
>          to pass in a struct device * pointer for debug purpose only in
>          each functions.
> v3->v4:
>  * only scu headfile path update
>
> v2->v3:
>  * structure and enums name update
>  * api usage update
>
>
> Dong Aisheng (5):
>   dt-bindings: imx: add scu resource id headfile
>   firmware: imx: remove resource id enums
>   dt-bindings: fsl: scu: update power domain binding
>   firmware: imx: add pm svc headfile
>   firmware: imx: add SCU power domain driver
>
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  |  37 +-
>  drivers/firmware/imx/Kconfig                       |   6 +
>  drivers/firmware/imx/Makefile                      |   3 +-
>  drivers/firmware/imx/scu-pd.c                      | 302 +++++++++++
>  include/dt-bindings/firmware/imx/rsrc.h            | 559 +++++++++++++++++++++
>  include/linux/firmware/imx/sci.h                   |   1 +
>  include/linux/firmware/imx/svc/pm.h                |  85 ++++
>  include/linux/firmware/imx/types.h                 | 552 --------------------
>  8 files changed, 963 insertions(+), 582 deletions(-)
>  create mode 100644 drivers/firmware/imx/scu-pd.c
>  create mode 100644 include/dt-bindings/firmware/imx/rsrc.h
>  create mode 100644 include/linux/firmware/imx/svc/pm.h
>
> --
> 2.7.4
>

For patch 1 -> 4 (comments on patch5 is on its way).

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe