[PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Shenwei Wang]

On i.MX8QM/QXP/DXL SoCs, even a GPIO is selected as the wakeup source,
the GPIO block will be powered off when system enters into suspend
state. This can greatly reduce the power consumption of suspend state
because the whole partition can be shutdown. This is called PAD wakeup
feature on i.MX8x platform.

This patch enables this wakeup feature on i.MX8QM/QXP/DXL platforms.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 Changes in v2:
 - fix a bug when finding the pad index

 drivers/gpio/gpio-mxc.c | 765 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 751 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index c871602fc5ba..e18af80ce50b 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -24,6 +24,33 @@
 #include <linux/of_device.h>
 #include <linux/bug.h>

+#include <dt-bindings/pinctrl/pads-imx8dxl.h>
+#include <dt-bindings/pinctrl/pads-imx8qxp.h>
+#include <dt-bindings/pinctrl/pads-imx8qm.h>
+#include <linux/firmware/imx/sci.h>
+
+#define IMX_SC_PAD_FUNC_GET_WAKEUP	9
+#define IMX_SC_PAD_FUNC_SET_WAKEUP	4
+#define IMX_SC_IRQ_GROUP_WAKE           3   /* Wakeup interrupts */
+#define IMX_SC_IRQ_PAD			2    /* Pad wakeup */
+
+#define IMX_SC_PAD_WAKEUP_OFF		0
+#define IMX_SC_PAD_WAKEUP_LOW_LVL	4
+#define IMX_SC_PAD_WAKEUP_FALL_EDGE	5
+#define IMX_SC_PAD_WAKEUP_RISE_EDGE	6
+#define IMX_SC_PAD_WAKEUP_HIGH_LVL	7
+
+struct imx_sc_msg_gpio_set_pad_wakeup {
+	struct imx_sc_rpc_msg hdr;
+	u16 pad;
+	u8 wakeup;
+} __packed __aligned(4);
+
+struct mxc_gpio_pad_map {
+	u32 index;
+	u32 pad_index;
+};
+
 /* device type dependent stuff */
 struct mxc_gpio_hwdata {
 	unsigned dr_reg;
@@ -38,6 +65,8 @@ struct mxc_gpio_hwdata {
 	unsigned high_level;
 	unsigned rise_edge;
 	unsigned fall_edge;
+	unsigned pad_map_size;
+	const struct mxc_gpio_pad_map *pad_map;
 };

 struct mxc_gpio_reg_saved {
@@ -61,9 +90,562 @@ struct mxc_gpio_port {
 	u32 both_edges;
 	struct mxc_gpio_reg_saved gpio_saved_reg;
 	bool power_off;
+	int gpio_index;
+	u32 wakeup_pads;
+	bool is_pad_wakeup;
+	u32 pad_type[32];
 	const struct mxc_gpio_hwdata *hwdata;
 };

+const struct mxc_gpio_pad_map imx8qm_pad_map[] = {
+	/* GPIO0 */
+	{  0, IMX8QM_SIM0_CLK},
+	{  1, IMX8QM_SIM0_RST},
+	{  2, IMX8QM_SIM0_IO},
+	{  3, IMX8QM_SIM0_PD},
+	{  4, IMX8QM_SIM0_POWER_EN},
+	{  5, IMX8QM_SIM0_GPIO0_00},
+	{  6, IMX8QM_M40_I2C0_SCL},
+	{  7, IMX8QM_M40_I2C0_SDA},
+	{  8, IMX8QM_M40_GPIO0_00},
+	{  9, IMX8QM_M40_GPIO0_01},
+	{ 10, IMX8QM_M41_I2C0_SCL},
+	{ 11, IMX8QM_M41_I2C0_SDA},
+	{ 12, IMX8QM_M41_GPIO0_00},
+	{ 13, IMX8QM_M41_GPIO0_01},
+	{ 14, IMX8QM_GPT0_CLK},
+	{ 15, IMX8QM_GPT0_CAPTURE},
+	{ 16, IMX8QM_GPT0_COMPARE},
+	{ 17, IMX8QM_GPT1_CLK},
+	{ 18, IMX8QM_GPT1_CAPTURE},
+	{ 19, IMX8QM_GPT1_COMPARE},
+	{ 20, IMX8QM_UART0_RX},
+	{ 21, IMX8QM_UART0_TX},
+	{ 22, IMX8QM_UART0_RTS_B},
+	{ 23, IMX8QM_UART0_CTS_B},
+	{ 24, IMX8QM_UART1_TX},
+	{ 25, IMX8QM_UART1_RX},
+	{ 26, IMX8QM_UART1_RTS_B},
+	{ 27, IMX8QM_UART1_CTS_B},
+	{ 28, IMX8QM_SCU_GPIO0_00},
+	{ 29, IMX8QM_SCU_GPIO0_01},
+	{ 30, IMX8QM_SCU_GPIO0_02},
+	{ 31, IMX8QM_SCU_GPIO0_03},
+
+	/* GPIO1 */
+	{ 32, IMX8QM_SCU_GPIO0_04},
+	{ 33, IMX8QM_SCU_GPIO0_05},
+	{ 34, IMX8QM_SCU_GPIO0_06},
+	{ 35, IMX8QM_SCU_GPIO0_07},
+	{ 36, IMX8QM_LVDS0_GPIO00},
+	{ 37, IMX8QM_LVDS0_GPIO01},
+	{ 38, IMX8QM_LVDS0_I2C0_SCL},
+	{ 39, IMX8QM_LVDS0_I2C0_SDA},
+	{ 40, IMX8QM_LVDS0_I2C1_SCL},
+	{ 41, IMX8QM_LVDS0_I2C1_SDA},
+	{ 42, IMX8QM_LVDS1_GPIO00},
+	{ 43, IMX8QM_LVDS1_GPIO01},
+	{ 44, IMX8QM_LVDS1_I2C0_SCL},
+	{ 45, IMX8QM_LVDS1_I2C0_SDA},
+	{ 46, IMX8QM_LVDS1_I2C1_SCL},
+	{ 47, IMX8QM_LVDS1_I2C1_SDA},
+	{ 48, IMX8QM_MIPI_DSI0_I2C0_SCL},
+	{ 49, IMX8QM_MIPI_DSI0_I2C0_SDA},
+	{ 50, IMX8QM_MIPI_DSI0_GPIO0_00},
+	{ 51, IMX8QM_MIPI_DSI0_GPIO0_01},
+	{ 52, IMX8QM_MIPI_DSI1_I2C0_SCL},
+	{ 53, IMX8QM_MIPI_DSI1_I2C0_SDA},
+	{ 54, IMX8QM_MIPI_DSI1_GPIO0_00},
+	{ 55, IMX8QM_MIPI_DSI1_GPIO0_01},
+	{ 56, IMX8QM_MIPI_CSI0_MCLK_OUT},
+	{ 57, IMX8QM_MIPI_CSI0_I2C0_SCL},
+	{ 58, IMX8QM_MIPI_CSI0_I2C0_SDA},
+	{ 59, IMX8QM_MIPI_CSI0_GPIO0_00},
+	{ 60, IMX8QM_MIPI_CSI0_GPIO0_01},
+	{ 61, IMX8QM_MIPI_CSI1_MCLK_OUT},
+	{ 62, IMX8QM_MIPI_CSI1_GPIO0_00},
+	{ 63, IMX8QM_MIPI_CSI1_GPIO0_01},
+
+	/* GPIO2 */
+	{ 64, IMX8QM_MIPI_CSI1_I2C0_SCL},
+	{ 65, IMX8QM_MIPI_CSI1_I2C0_SDA},
+	{ 66, IMX8QM_HDMI_TX0_TS_SCL},
+	{ 67, IMX8QM_HDMI_TX0_TS_SDA},
+	{ 68, IMX8QM_ESAI1_FSR},
+	{ 69, IMX8QM_ESAI1_FST},
+	{ 70, IMX8QM_ESAI1_SCKR},
+	{ 71, IMX8QM_ESAI1_SCKT},
+	{ 72, IMX8QM_ESAI1_TX0},
+	{ 73, IMX8QM_ESAI1_TX1},
+	{ 74, IMX8QM_ESAI1_TX2_RX3},
+	{ 75, IMX8QM_ESAI1_TX3_RX2},
+	{ 76, IMX8QM_ESAI1_TX4_RX1},
+	{ 77, IMX8QM_ESAI1_TX5_RX0},
+	{ 78, IMX8QM_SPDIF0_RX},
+	{ 79, IMX8QM_SPDIF0_TX},
+	{ 80, IMX8QM_SPDIF0_EXT_CLK},
+	{ 81, IMX8QM_SPI3_SCK},
+	{ 82, IMX8QM_SPI3_SDO},
+	{ 83, IMX8QM_SPI3_SDI},
+	{ 84, IMX8QM_SPI3_CS0},
+	{ 85, IMX8QM_SPI3_CS1},
+	{ 86, IMX8QM_ESAI0_FSR},
+	{ 87, IMX8QM_ESAI0_FST},
+	{ 88, IMX8QM_ESAI0_SCKR},
+	{ 89, IMX8QM_ESAI0_SCKT},
+	{ 90, IMX8QM_ESAI0_TX0},
+	{ 91, IMX8QM_ESAI0_TX1},
+	{ 92, IMX8QM_ESAI0_TX2_RX3},
+	{ 93, IMX8QM_ESAI0_TX3_RX2},
+	{ 94, IMX8QM_ESAI0_TX4_RX1},
+	{ 95, IMX8QM_ESAI0_TX5_RX0},
+
+	/* GPIO3 */
+	{ 96, IMX8QM_MCLK_IN0},
+	{ 97, IMX8QM_MCLK_OUT0},
+	{ 98, IMX8QM_SPI0_SCK},
+	{ 99, IMX8QM_SPI0_SDO},
+	{100, IMX8QM_SPI0_SDI},
+	{101, IMX8QM_SPI0_CS0},
+	{102, IMX8QM_SPI0_CS1},
+	{103, IMX8QM_SPI2_SCK},
+	{104, IMX8QM_SPI2_SDO},
+	{105, IMX8QM_SPI2_SDI},
+	{106, IMX8QM_SPI2_CS0},
+	{107, IMX8QM_SPI2_CS1},
+	{108, IMX8QM_SAI1_RXC},
+	{109, IMX8QM_SAI1_RXD},
+	{110, IMX8QM_SAI1_RXFS},
+	{111, IMX8QM_SAI1_TXC},
+	{112, IMX8QM_SAI1_TXD},
+	{113, IMX8QM_SAI1_TXFS},
+	{114, IMX8QM_ADC_IN0},
+	{115, IMX8QM_ADC_IN1},
+	{116, IMX8QM_ADC_IN2},
+	{117, IMX8QM_ADC_IN3},
+	{118, IMX8QM_ADC_IN4},
+	{119, IMX8QM_ADC_IN5},
+	{120, IMX8QM_ADC_IN6},
+	{121, IMX8QM_ADC_IN7},
+	{122, IMX8QM_MLB_SIG},
+	{123, IMX8QM_MLB_CLK},
+	{124, IMX8QM_MLB_DATA},
+	{125, IMX8QM_FLEXCAN0_RX},
+	{126, IMX8QM_FLEXCAN0_TX},
+	{127, IMX8QM_FLEXCAN1_RX},
+
+	/* GPIO4 */
+	{128, IMX8QM_FLEXCAN1_TX},
+	{129, IMX8QM_FLEXCAN2_RX},
+	{130, IMX8QM_FLEXCAN2_TX},
+	{131, IMX8QM_USB_SS3_TC0},
+	{132, IMX8QM_USB_SS3_TC1},
+	{133, IMX8QM_USB_SS3_TC2},
+	{134, IMX8QM_USB_SS3_TC3},
+	{135, IMX8QM_USDHC1_RESET_B},
+	{136, IMX8QM_USDHC1_VSELECT},
+	{137, IMX8QM_USDHC2_RESET_B},
+	{138, IMX8QM_USDHC2_VSELECT},
+	{139, IMX8QM_USDHC2_WP},
+	{140, IMX8QM_USDHC2_CD_B},
+	{141, IMX8QM_ENET0_MDIO},
+	{142, IMX8QM_ENET0_MDC},
+	{143, IMX8QM_ENET0_REFCLK_125M_25M},
+	{144, IMX8QM_ENET1_REFCLK_125M_25M},
+	{145, IMX8QM_ENET1_MDIO},
+	{146, IMX8QM_ENET1_MDC},
+	{147, IMX8QM_QSPI1A_SS0_B},
+	{148, IMX8QM_QSPI1A_SS1_B},
+	{149, IMX8QM_QSPI1A_SCLK},
+	{150, IMX8QM_QSPI1A_DQS},
+	{151, IMX8QM_QSPI1A_DATA3},
+	{152, IMX8QM_QSPI1A_DATA2},
+	{153, IMX8QM_QSPI1A_DATA1},
+	{154, IMX8QM_QSPI1A_DATA0},
+	{155, IMX8QM_PCIE_CTRL0_CLKREQ_B},
+	{156, IMX8QM_PCIE_CTRL0_WAKE_B},
+	{157, IMX8QM_PCIE_CTRL0_PERST_B},
+	{158, IMX8QM_PCIE_CTRL1_CLKREQ_B},
+	{159, IMX8QM_PCIE_CTRL1_WAKE_B},
+
+	/* GPIO5 */
+	{160, IMX8QM_PCIE_CTRL1_PERST_B},
+	{161, IMX8QM_USB_HSIC0_DATA},
+	{162, IMX8QM_USB_HSIC0_STROBE},
+	{163, IMX8QM_EMMC0_CMD},
+	{164, IMX8QM_EMMC0_DATA0},
+	{165, IMX8QM_EMMC0_DATA1},
+	{166, IMX8QM_EMMC0_DATA2},
+	{167, IMX8QM_EMMC0_DATA3},
+	{168, IMX8QM_EMMC0_DATA4},
+	{169, IMX8QM_EMMC0_DATA5},
+	{170, IMX8QM_EMMC0_DATA6},
+	{171, IMX8QM_EMMC0_DATA7},
+	{172, IMX8QM_EMMC0_STROBE},
+	{173, IMX8QM_EMMC0_RESET_B},
+	{174, IMX8QM_USDHC1_CMD},
+	{175, IMX8QM_USDHC1_DATA0},
+	{176, IMX8QM_USDHC1_DATA1},
+	{177, IMX8QM_USDHC1_DATA2},
+	{178, IMX8QM_USDHC1_DATA3},
+	{179, IMX8QM_USDHC1_DATA4},
+	{180, IMX8QM_USDHC1_DATA5},
+	{181, IMX8QM_USDHC1_DATA6},
+	{182, IMX8QM_USDHC1_DATA7},
+	{183, IMX8QM_USDHC1_STROBE},
+	{184, IMX8QM_USDHC2_CLK},
+	{185, IMX8QM_USDHC2_CMD},
+	{186, IMX8QM_USDHC2_DATA0},
+	{187, IMX8QM_USDHC2_DATA1},
+	{188, IMX8QM_USDHC2_DATA2},
+	{189, IMX8QM_USDHC2_DATA3},
+	{190, IMX8QM_ENET0_RGMII_TXC},
+	{191, IMX8QM_ENET0_RGMII_TX_CTL},
+
+	/* GPIO6 */
+	{192, IMX8QM_ENET0_RGMII_TXD0},
+	{193, IMX8QM_ENET0_RGMII_TXD1},
+	{194, IMX8QM_ENET0_RGMII_TXD2},
+	{195, IMX8QM_ENET0_RGMII_TXD3},
+	{196, IMX8QM_ENET0_RGMII_RXC},
+	{197, IMX8QM_ENET0_RGMII_RX_CTL},
+	{198, IMX8QM_ENET0_RGMII_RXD0},
+	{199, IMX8QM_ENET0_RGMII_RXD1},
+	{200, IMX8QM_ENET0_RGMII_RXD2},
+	{201, IMX8QM_ENET0_RGMII_RXD3},
+	{202, IMX8QM_ENET1_RGMII_TXC},
+	{203, IMX8QM_ENET1_RGMII_TX_CTL},
+	{204, IMX8QM_ENET1_RGMII_TXD0},
+	{205, IMX8QM_ENET1_RGMII_TXD1},
+	{206, IMX8QM_ENET1_RGMII_TXD2},
+	{207, IMX8QM_ENET1_RGMII_TXD3},
+	{208, IMX8QM_ENET1_RGMII_RXC},
+	{209, IMX8QM_ENET1_RGMII_RX_CTL},
+	{210, IMX8QM_ENET1_RGMII_RXD0},
+	{211, IMX8QM_ENET1_RGMII_RXD1},
+	{212, IMX8QM_ENET1_RGMII_RXD2},
+	{213, IMX8QM_ENET1_RGMII_RXD3},
+
+	/* GPIO7 */
+};
+
+const struct mxc_gpio_pad_map imx8qxp_pad_map[] = {
+	/* GPIO0 */
+	{  1, IMX8QXP_ESAI0_FST},
+	{  2, IMX8QXP_ESAI0_SCKR},
+	{  3, IMX8QXP_ESAI0_SCKT},
+	{  4, IMX8QXP_ESAI0_TX0},
+	{  5, IMX8QXP_ESAI0_TX1},
+	{  6, IMX8QXP_ESAI0_TX2_RX3},
+	{  7, IMX8QXP_ESAI0_TX3_RX2},
+	{  8, IMX8QXP_ESAI0_TX4_RX1},
+	{  9, IMX8QXP_ESAI0_TX5_RX0},
+	{ 10, IMX8QXP_SPDIF0_RX},
+	{ 11, IMX8QXP_SPDIF0_TX},
+	{ 12, IMX8QXP_SPDIF0_EXT_CLK},
+	{ 13, IMX8QXP_SPI3_SCK},
+	{ 14, IMX8QXP_SPI3_SDO},
+	{ 15, IMX8QXP_SPI3_SDI},
+	{ 16, IMX8QXP_SPI3_CS0},
+	{ 19, IMX8QXP_MCLK_IN0},
+	{ 20, IMX8QXP_MCLK_OUT0},
+	{ 21, IMX8QXP_UART1_TX},
+	{ 22, IMX8QXP_UART1_RX},
+	{ 24, IMX8QXP_UART1_CTS_B},
+	{ 25, IMX8QXP_SAI0_TXD},
+	{ 26, IMX8QXP_SAI0_TXC},
+	{ 27, IMX8QXP_SAI0_RXD},
+	{ 28, IMX8QXP_SAI0_TXFS},
+	{ 29, IMX8QXP_SAI1_RXD},
+	{ 30, IMX8QXP_SAI1_RXC},
+	{ 31, IMX8QXP_SAI1_RXFS},
+
+	/* GPIO1 */
+	{ 32, IMX8QXP_SPI2_CS0},
+	{ 33, IMX8QXP_SPI2_SDO},
+	{ 34, IMX8QXP_SPI2_SDI},
+	{ 35, IMX8QXP_SPI2_SCK},
+	{ 36, IMX8QXP_SPI0_SCK},
+	{ 37, IMX8QXP_SPI0_SDI},
+	{ 38, IMX8QXP_SPI0_SDO},
+	{ 39, IMX8QXP_SPI0_CS1},
+	{ 40, IMX8QXP_SPI0_CS0},
+	{ 41, IMX8QXP_ADC_IN1},
+	{ 42, IMX8QXP_ADC_IN0},
+	{ 43, IMX8QXP_ADC_IN3},
+	{ 44, IMX8QXP_ADC_IN2},
+	{ 45, IMX8QXP_ADC_IN5},
+	{ 46, IMX8QXP_ADC_IN4},
+	{ 47, IMX8QXP_FLEXCAN0_RX},
+	{ 48, IMX8QXP_FLEXCAN0_TX},
+	{ 49, IMX8QXP_FLEXCAN1_RX},
+	{ 50, IMX8QXP_FLEXCAN1_TX},
+	{ 51, IMX8QXP_FLEXCAN2_RX},
+	{ 52, IMX8QXP_FLEXCAN2_TX},
+	{ 53, IMX8QXP_UART0_RX},
+	{ 54, IMX8QXP_UART0_TX},
+	{ 55, IMX8QXP_UART2_TX},
+	{ 56, IMX8QXP_UART2_RX},
+	{ 57, IMX8QXP_MIPI_DSI0_I2C0_SCL},
+	{ 58, IMX8QXP_MIPI_DSI0_I2C0_SDA},
+	{ 59, IMX8QXP_MIPI_DSI0_GPIO0_00},
+	{ 60, IMX8QXP_MIPI_DSI0_GPIO0_01},
+	{ 61, IMX8QXP_MIPI_DSI1_I2C0_SCL},
+	{ 62, IMX8QXP_MIPI_DSI1_I2C0_SDA},
+	{ 63, IMX8QXP_MIPI_DSI1_GPIO0_00},
+
+	/* GPIO2 */
+	{ 64, IMX8QXP_MIPI_DSI1_GPIO0_01},
+	{ 65, IMX8QXP_PMIC_I2C_SCL},
+	{ 66, IMX8QXP_PMIC_I2C_SDA},
+	{ 67, IMX8QXP_SCU_GPIO0_00},
+
+	/* GPIO3 */
+	{ 96, IMX8QXP_CSI_PCLK},
+	{ 97, IMX8QXP_CSI_MCLK},
+	{ 98, IMX8QXP_CSI_EN},
+	{ 99, IMX8QXP_CSI_RESET},
+	{100, IMX8QXP_MIPI_CSI0_MCLK_OUT},
+	{101, IMX8QXP_MIPI_CSI0_I2C0_SCL},
+	{102, IMX8QXP_MIPI_CSI0_I2C0_SDA},
+	{103, IMX8QXP_MIPI_CSI0_GPIO0_01},
+	{104, IMX8QXP_MIPI_CSI0_GPIO0_00},
+	{105, IMX8QXP_QSPI0A_DATA0},
+	{106, IMX8QXP_QSPI0A_DATA1},
+	{107, IMX8QXP_QSPI0A_DATA2},
+	{108, IMX8QXP_QSPI0A_DATA3},
+	{109, IMX8QXP_QSPI0A_DQS},
+	{110, IMX8QXP_QSPI0A_SS0_B},
+	{111, IMX8QXP_QSPI0A_SS1_B},
+	{112, IMX8QXP_QSPI0A_SCLK},
+	{113, IMX8QXP_QSPI0B_SCLK},
+	{114, IMX8QXP_QSPI0B_DATA0},
+	{115, IMX8QXP_QSPI0B_DATA1},
+	{116, IMX8QXP_QSPI0B_DATA2},
+	{117, IMX8QXP_QSPI0B_DATA3},
+	{118, IMX8QXP_QSPI0B_DQS},
+	{119, IMX8QXP_QSPI0B_SS0_B},
+	{120, IMX8QXP_QSPI0B_SS1_B},
+
+	/* GPIO4 */
+	{128, IMX8QXP_PCIE_CTRL0_PERST_B},
+	{129, IMX8QXP_PCIE_CTRL0_CLKREQ_B},
+	{130, IMX8QXP_PCIE_CTRL0_WAKE_B},
+	{131, IMX8QXP_USB_SS3_TC0},
+	{132, IMX8QXP_USB_SS3_TC1},
+	{133, IMX8QXP_USB_SS3_TC2},
+	{134, IMX8QXP_USB_SS3_TC3},
+	{135, IMX8QXP_EMMC0_CLK},
+	{136, IMX8QXP_EMMC0_CMD},
+	{137, IMX8QXP_EMMC0_DATA0},
+	{138, IMX8QXP_EMMC0_DATA1},
+	{139, IMX8QXP_EMMC0_DATA2},
+	{140, IMX8QXP_EMMC0_DATA3},
+	{141, IMX8QXP_EMMC0_DATA4},
+	{142, IMX8QXP_EMMC0_DATA5},
+	{143, IMX8QXP_EMMC0_DATA6},
+	{144, IMX8QXP_EMMC0_DATA7},
+	{145, IMX8QXP_EMMC0_STROBE},
+	{146, IMX8QXP_EMMC0_RESET_B},
+	{147, IMX8QXP_USDHC1_RESET_B},
+	{148, IMX8QXP_USDHC1_VSELECT},
+	{149, IMX8QXP_USDHC1_WP},
+	{150, IMX8QXP_USDHC1_CD_B},
+	{151, IMX8QXP_USDHC1_CLK},
+	{152, IMX8QXP_USDHC1_CMD},
+	{153, IMX8QXP_USDHC1_DATA0},
+	{154, IMX8QXP_USDHC1_DATA1},
+	{155, IMX8QXP_USDHC1_DATA2},
+	{156, IMX8QXP_USDHC1_DATA3},
+	{157, IMX8QXP_ENET0_RGMII_TXC},
+	{158, IMX8QXP_ENET0_RGMII_TX_CTL},
+	{159, IMX8QXP_ENET0_RGMII_TXD0},
+
+	/* GPIO5 */
+	{160, IMX8QXP_ENET0_RGMII_TXD1},
+	{161, IMX8QXP_ENET0_RGMII_TXD2},
+	{162, IMX8QXP_ENET0_RGMII_TXD3},
+	{163, IMX8QXP_ENET0_RGMII_RXC},
+	{164, IMX8QXP_ENET0_RGMII_RX_CTL},
+	{165, IMX8QXP_ENET0_RGMII_RXD0},
+	{166, IMX8QXP_ENET0_RGMII_RXD1},
+	{167, IMX8QXP_ENET0_RGMII_RXD2},
+	{168, IMX8QXP_ENET0_RGMII_RXD3},
+	{169, IMX8QXP_ENET0_REFCLK_125M_25M},
+	{170, IMX8QXP_ENET0_MDIO},
+	{171, IMX8QXP_ENET0_MDC},
+
+	/* GPIO6 */
+
+	/* GPIO7 */
+};
+
+const struct mxc_gpio_pad_map imx8dxl_pad_map[] = {
+	/* GPIO0 */
+	{  0, IMX8DXL_ENET1_RGMII_TXC},
+	{  1, IMX8DXL_ENET1_RGMII_TXD2},
+	{  2, IMX8DXL_ENET1_RGMII_TX_CTL},
+	{  3, IMX8DXL_ENET1_RGMII_TXD3},
+	{  4, IMX8DXL_ENET1_RGMII_RXC},
+	{  5, IMX8DXL_ENET1_RGMII_RXD3},
+	{  6, IMX8DXL_ENET1_RGMII_RXD2},
+	{  7, IMX8DXL_ENET1_RGMII_RXD1},
+	{  8, IMX8DXL_ENET1_RGMII_TXD0},
+	{  9, IMX8DXL_ENET1_RGMII_TXD1},
+	{ 10, IMX8DXL_ENET1_RGMII_RXD0},
+	{ 11, IMX8DXL_ENET1_RGMII_RX_CTL},
+	{ 12, IMX8DXL_ENET1_REFCLK_125M_25M},
+	{ 13, IMX8DXL_SPI3_SCK},
+	{ 14, IMX8DXL_SPI3_SDO},
+	{ 15, IMX8DXL_SPI3_SDI},
+	{ 16, IMX8DXL_SPI3_CS0},
+	{ 19, IMX8DXL_MCLK_IN0},
+	{ 20, IMX8DXL_MCLK_OUT0},
+	{ 21, IMX8DXL_UART1_TX},
+	{ 22, IMX8DXL_UART1_RX},
+	{ 24, IMX8DXL_UART1_CTS_B},
+
+	/* GPIO1 */
+	{ 36, IMX8DXL_SPI0_SCK},
+	{ 37, IMX8DXL_SPI0_SDI},
+	{ 38, IMX8DXL_SPI0_SDO},
+	{ 39, IMX8DXL_SPI0_CS1},
+	{ 40, IMX8DXL_SPI0_CS0},
+	{ 41, IMX8DXL_ADC_IN1},
+	{ 42, IMX8DXL_ADC_IN0},
+	{ 43, IMX8DXL_ADC_IN3},
+	{ 44, IMX8DXL_ADC_IN2},
+	{ 45, IMX8DXL_ADC_IN5},
+	{ 46, IMX8DXL_ADC_IN4},
+	{ 47, IMX8DXL_FLEXCAN0_RX},
+	{ 48, IMX8DXL_FLEXCAN0_TX},
+	{ 49, IMX8DXL_FLEXCAN1_RX},
+	{ 50, IMX8DXL_FLEXCAN1_TX},
+	{ 51, IMX8DXL_FLEXCAN2_RX},
+	{ 52, IMX8DXL_FLEXCAN2_TX},
+	{ 53, IMX8DXL_UART0_RX},
+	{ 54, IMX8DXL_UART0_TX},
+	{ 55, IMX8DXL_UART2_TX},
+	{ 56, IMX8DXL_UART2_RX},
+
+	/* GPIO2 */
+	{ 65, IMX8DXL_PMIC_I2C_SCL},
+	{ 66, IMX8DXL_PMIC_I2C_SDA},
+	{ 67, IMX8DXL_SCU_GPIO0_00},
+	{ 69, IMX8DXL_SNVS_TAMPER_OUT1},
+	{ 70, IMX8DXL_SNVS_TAMPER_OUT2},
+	{ 71, IMX8DXL_SNVS_TAMPER_OUT3},
+	{ 72, IMX8DXL_SNVS_TAMPER_OUT4},
+	{ 73, IMX8DXL_SNVS_TAMPER_IN0},
+	{ 74, IMX8DXL_SNVS_TAMPER_IN1},
+	{ 75, IMX8DXL_SNVS_TAMPER_IN2},
+	{ 76, IMX8DXL_SNVS_TAMPER_IN3},
+
+	/* GPIO3 */
+	{ 96, IMX8DXL_SPI1_SCK},
+	{ 97, IMX8DXL_SPI1_SDO},
+	{ 98, IMX8DXL_SPI1_SDI},
+	{ 99, IMX8DXL_SPI1_CS0},
+	{105, IMX8DXL_QSPI0A_DATA0},
+	{106, IMX8DXL_QSPI0A_DATA1},
+	{107, IMX8DXL_QSPI0A_DATA2},
+	{108, IMX8DXL_QSPI0A_DATA3},
+	{109, IMX8DXL_QSPI0A_DQS},
+	{110, IMX8DXL_QSPI0A_SS0_B},
+	{112, IMX8DXL_QSPI0A_SCLK},
+	{113, IMX8DXL_QSPI0B_SCLK},
+	{114, IMX8DXL_QSPI0B_DATA0},
+	{115, IMX8DXL_QSPI0B_DATA1},
+	{116, IMX8DXL_QSPI0B_DATA2},
+	{117, IMX8DXL_QSPI0B_DATA3},
+	{118, IMX8DXL_QSPI0B_DQS},
+	{119, IMX8DXL_QSPI0B_SS0_B},
+
+	/* GPIO4 */
+	{128, IMX8DXL_PCIE_CTRL0_PERST_B},
+	{129, IMX8DXL_PCIE_CTRL0_CLKREQ_B},
+	{130, IMX8DXL_PCIE_CTRL0_WAKE_B},
+	{131, IMX8DXL_USB_SS3_TC0},
+	{132, IMX8DXL_USB_SS3_TC1},
+	{133, IMX8DXL_USB_SS3_TC2},
+	{134, IMX8DXL_USB_SS3_TC3},
+	{135, IMX8DXL_EMMC0_CLK},
+	{136, IMX8DXL_EMMC0_CMD},
+	{137, IMX8DXL_EMMC0_DATA0},
+	{138, IMX8DXL_EMMC0_DATA1},
+	{139, IMX8DXL_EMMC0_DATA2},
+	{140, IMX8DXL_EMMC0_DATA3},
+	{141, IMX8DXL_EMMC0_DATA4},
+	{142, IMX8DXL_EMMC0_DATA5},
+	{143, IMX8DXL_EMMC0_DATA6},
+	{144, IMX8DXL_EMMC0_DATA7},
+	{145, IMX8DXL_EMMC0_STROBE},
+	{146, IMX8DXL_EMMC0_RESET_B},
+	{147, IMX8DXL_USDHC1_RESET_B},
+	{148, IMX8DXL_USDHC1_VSELECT},
+	{149, IMX8DXL_USDHC1_WP},
+	{150, IMX8DXL_USDHC1_CD_B},
+	{157, IMX8DXL_ENET0_RGMII_TXC},
+	{158, IMX8DXL_ENET0_RGMII_TX_CTL},
+	{159, IMX8DXL_ENET0_RGMII_TXD0},
+
+	/* GPIO5 */
+	{160, IMX8DXL_ENET0_RGMII_TXD1},
+	{161, IMX8DXL_ENET0_RGMII_TXD2},
+	{162, IMX8DXL_ENET0_RGMII_TXD3},
+	{163, IMX8DXL_ENET0_RGMII_RXC},
+	{164, IMX8DXL_ENET0_RGMII_RX_CTL},
+	{165, IMX8DXL_ENET0_RGMII_RXD0},
+	{166, IMX8DXL_ENET0_RGMII_RXD1},
+	{167, IMX8DXL_ENET0_RGMII_RXD2},
+	{168, IMX8DXL_ENET0_RGMII_RXD3},
+	{169, IMX8DXL_ENET0_REFCLK_125M_25M},
+	{170, IMX8DXL_ENET0_MDIO},
+	{171, IMX8DXL_ENET0_MDC},
+
+	/* GPIO6 */
+	{192, IMX8DXL_ENET1_RGMII_RXD2},
+	{193, IMX8DXL_ENET1_RGMII_RXD1},
+	{194, IMX8DXL_ENET1_RGMII_TXD0},
+	{195, IMX8DXL_ENET1_RGMII_TXD1},
+	{196, IMX8DXL_ENET1_RGMII_RXD0},
+	{197, IMX8DXL_ENET1_RGMII_RX_CTL},
+	{198, IMX8DXL_ENET1_REFCLK_125M_25M},
+	{200, IMX8DXL_FLEXCAN0_RX},
+	{201, IMX8DXL_FLEXCAN0_TX},
+	{202, IMX8DXL_FLEXCAN1_RX},
+	{203, IMX8DXL_FLEXCAN1_TX},
+	{204, IMX8DXL_FLEXCAN2_RX},
+	{205, IMX8DXL_FLEXCAN2_TX},
+	{206, IMX8DXL_UART0_RX},
+	{207, IMX8DXL_UART0_TX},
+	{208, IMX8DXL_UART2_TX},
+	{209, IMX8DXL_UART2_RX},
+	{211, IMX8DXL_SNVS_TAMPER_OUT1},
+	{212, IMX8DXL_SNVS_TAMPER_OUT2},
+	{213, IMX8DXL_SNVS_TAMPER_OUT3},
+	{214, IMX8DXL_SNVS_TAMPER_OUT4},
+	{215, IMX8DXL_SNVS_TAMPER_IN0},
+	{216, IMX8DXL_SNVS_TAMPER_IN1},
+	{217, IMX8DXL_SNVS_TAMPER_IN2},
+	{218, IMX8DXL_SNVS_TAMPER_IN3},
+
+	/* GPIO7 */
+	{224, IMX8DXL_PCIE_CTRL0_PERST_B},
+	{225, IMX8DXL_PCIE_CTRL0_CLKREQ_B},
+	{226, IMX8DXL_PCIE_CTRL0_WAKE_B},
+	{227, IMX8DXL_USB_SS3_TC0},
+	{228, IMX8DXL_USB_SS3_TC1},
+	{229, IMX8DXL_USB_SS3_TC2},
+	{230, IMX8DXL_USB_SS3_TC3},
+	{232, IMX8DXL_USDHC1_RESET_B},
+	{233, IMX8DXL_USDHC1_VSELECT},
+	{234, IMX8DXL_USDHC1_WP},
+	{235, IMX8DXL_USDHC1_CD_B},
+	{240, IMX8DXL_ENET0_MDIO},
+	{241, IMX8DXL_ENET0_MDC},
+};
+
 static struct mxc_gpio_hwdata imx1_imx21_gpio_hwdata = {
 	.dr_reg		= 0x1c,
 	.gdir_reg	= 0x00,
@@ -94,20 +676,35 @@ static struct mxc_gpio_hwdata imx31_gpio_hwdata = {
 	.fall_edge	= 0x03,
 };

-static struct mxc_gpio_hwdata imx35_gpio_hwdata = {
-	.dr_reg		= 0x00,
-	.gdir_reg	= 0x04,
-	.psr_reg	= 0x08,
-	.icr1_reg	= 0x0c,
-	.icr2_reg	= 0x10,
-	.imr_reg	= 0x14,
-	.isr_reg	= 0x18,
-	.edge_sel_reg	= 0x1c,
-	.low_level	= 0x00,
-	.high_level	= 0x01,
-	.rise_edge	= 0x02,
-	.fall_edge	= 0x03,
-};
+#define MXC_GPIO_HWDATA(soc, map, size) \
+	static __maybe_unused struct mxc_gpio_hwdata soc##_gpio_hwdata = { \
+	.dr_reg		= 0x00,	\
+	.gdir_reg	= 0x04,	\
+	.psr_reg	= 0x08,	\
+	.icr1_reg	= 0x0c,	\
+	.icr2_reg	= 0x10,	\
+	.imr_reg	= 0x14,	\
+	.isr_reg	= 0x18,	\
+	.edge_sel_reg	= 0x1c,	\
+	.low_level	= 0x00,	\
+	.high_level	= 0x01,	\
+	.rise_edge	= 0x02,	\
+	.fall_edge	= 0x03,	\
+	.pad_map	= map,	\
+	.pad_map_size	= size,	\
+}
+
+static const u32
+imx8qm_pad_map_size = sizeof(imx8qm_pad_map)/sizeof(struct mxc_gpio_pad_map);
+static const u32
+imx8qxp_pad_map_size = sizeof(imx8qxp_pad_map)/sizeof(struct mxc_gpio_pad_map);
+static const u32
+imx8dxl_pad_map_size = sizeof(imx8dxl_pad_map)/sizeof(struct mxc_gpio_pad_map);
+
+MXC_GPIO_HWDATA(imx35, 0, 0);
+MXC_GPIO_HWDATA(imx8qm, imx8qm_pad_map, imx8qm_pad_map_size);
+MXC_GPIO_HWDATA(imx8qxp, imx8qxp_pad_map, imx8qxp_pad_map_size);
+MXC_GPIO_HWDATA(imx8dxl, imx8dxl_pad_map, imx8dxl_pad_map_size);

 #define GPIO_DR			(port->hwdata->dr_reg)
 #define GPIO_GDIR		(port->hwdata->gdir_reg)
@@ -130,6 +727,9 @@ static const struct of_device_id mxc_gpio_dt_ids[] = {
 	{ .compatible = "fsl,imx31-gpio", .data = &imx31_gpio_hwdata },
 	{ .compatible = "fsl,imx35-gpio", .data = &imx35_gpio_hwdata },
 	{ .compatible = "fsl,imx7d-gpio", .data = &imx35_gpio_hwdata },
+	{ .compatible = "fsl,imx8qm-gpio", .data = &imx8qm_gpio_hwdata },
+	{ .compatible = "fsl,imx8qxp-gpio", .data = &imx8qxp_gpio_hwdata },
+	{ .compatible = "fsl,imx8dxl-gpio", .data = &imx8dxl_gpio_hwdata },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mxc_gpio_dt_ids);
@@ -141,6 +741,32 @@ MODULE_DEVICE_TABLE(of, mxc_gpio_dt_ids);
  */
 static LIST_HEAD(mxc_gpio_ports);

+static int get_gpio_index_on_imx8(struct device_node *np, int base)
+{
+	static const u32 gpio_array[] = {
+		0x5d080000,	/* GPIO 0 */
+		0x5d090000,	/* GPIO 1 */
+		0x5d0a0000,	/* GPIO 2 */
+		0x5d0b0000,	/* GPIO 3 */
+		0x5d0c0000,	/* GPIO 4 */
+		0x5d0d0000,	/* GPIO 5 */
+		0x5d0e0000,	/* GPIO 6 */
+		0x5d0f0000,	/* GPIO 7 */
+	};
+	int i;
+
+	if (of_device_is_compatible(np, "fsl,imx8dxl-gpio") ||
+		of_device_is_compatible(np, "fsl,imx8qxp-gpio") ||
+		of_device_is_compatible(np, "fsl,imx35-gpio")) {
+
+		for (i = 0; i < sizeof(gpio_array)/sizeof(u32); i++)
+			if (base == gpio_array[i])
+				return i;
+	}
+
+	return -1;
+}
+
 /* Note: This driver assumes 32 GPIOs are handled in one register */

 static int gpio_set_irq_type(struct irq_data *d, u32 type)
@@ -203,6 +829,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	}

 	writel(1 << gpio_idx, port->base + GPIO_ISR);
+	port->pad_type[gpio_idx] = type;

 	return 0;
 }
@@ -306,11 +933,13 @@ static int gpio_set_wake_irq(struct irq_data *d, u32 enable)
 			ret = enable_irq_wake(port->irq_high);
 		else
 			ret = enable_irq_wake(port->irq);
+		port->wakeup_pads |= (1<<gpio_idx);
 	} else {
 		if (port->irq_high && (gpio_idx >= 16))
 			ret = disable_irq_wake(port->irq_high);
 		else
 			ret = disable_irq_wake(port->irq);
+		port->wakeup_pads &= ~(1<<gpio_idx);
 	}

 	return ret;
@@ -368,6 +997,11 @@ static int mxc_gpio_probe(struct platform_device *pdev)

 	port->hwdata = device_get_match_data(&pdev->dev);

+	port->gpio_index = get_gpio_index_on_imx8(np, pdev->resource->start);
+	if (port->gpio_index != -1)
+		imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_WAKE,
+					 IMX_SC_IRQ_PAD, true);
+
 	port->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(port->base))
 		return PTR_ERR(port->base);
@@ -498,6 +1132,108 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port *port)
 	writel(port->gpio_saved_reg.dr, port->base + GPIO_DR);
 }

+static int mxc_gpio_get_pad_index(const struct mxc_gpio_pad_map *pad_map,
+				  int size, int idx)
+{
+	const struct mxc_gpio_pad_map *map;
+	int i;
+
+	if (!pad_map)
+		return -1;
+
+	for (i = 0; i < size; i++) {
+		map = pad_map + i;
+		if (map && (map->index == idx))
+			return map->pad_index;
+	}
+
+	return -1;
+}
+
+static void mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
+{
+	struct imx_sc_msg_gpio_set_pad_wakeup msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	struct imx_sc_ipc *ipc_handle;
+	int ret, idx, type;
+	int i;
+
+	static const u32 pad_type_map[] = {
+		IMX_SC_PAD_WAKEUP_OFF,		/* 0 */
+		IMX_SC_PAD_WAKEUP_RISE_EDGE,	/* IRQ_TYPE_EDGE_RISING */
+		IMX_SC_PAD_WAKEUP_FALL_EDGE,	/* IRQ_TYPE_EDGE_FALLING */
+		IMX_SC_PAD_WAKEUP_FALL_EDGE,	/* IRQ_TYPE_EDGE_BOTH */
+		IMX_SC_PAD_WAKEUP_HIGH_LVL,	/* IRQ_TYPE_LEVEL_HIGH */
+		IMX_SC_PAD_WAKEUP_OFF,		/* 5 */
+		IMX_SC_PAD_WAKEUP_OFF,		/* 6 */
+		IMX_SC_PAD_WAKEUP_OFF,		/* 7 */
+		IMX_SC_PAD_WAKEUP_LOW_LVL,	/* IRQ_TYPE_LEVEL_LOW */
+	};
+
+	if (port->gpio_index == -1)
+		return;
+
+	ret = imx_scu_get_handle(&ipc_handle);
+	if (ret) {
+		dev_err(port->gc.parent, "scu error: ret=%d\n", ret);
+		return;
+	}
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_PAD;
+	hdr->func = IMX_SC_PAD_FUNC_SET_WAKEUP;
+	hdr->size = 2;
+
+	for (i = 0; i < 32; i++) {
+		if ((port->wakeup_pads & (1<<i))) {
+			idx = mxc_gpio_get_pad_index(port->hwdata->pad_map,
+					       port->hwdata->pad_map_size,
+					       port->gpio_index * 32 + i);
+
+			if (idx < 0)
+				continue;
+
+			type = port->pad_type[i];
+			msg.pad = idx;
+			msg.wakeup = enable ? pad_type_map[type]
+				: IMX_SC_PAD_WAKEUP_OFF;
+			ret = imx_scu_call_rpc(ipc_handle, &msg, true);
+			if (ret < 0)
+				dev_err(port->gc.parent, "set pad %d ret=%d\n",
+					idx, ret);
+		}
+	}
+}
+
+static int __maybe_unused mxc_gpio_noirq_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+
+	if (port->wakeup_pads > 0) {
+		mxc_gpio_set_pad_wakeup(port, true);
+		port->is_pad_wakeup = 1;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused mxc_gpio_noirq_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+
+	if (port->wakeup_pads > 0)
+		mxc_gpio_set_pad_wakeup(port, false);
+	port->is_pad_wakeup = 0;
+
+	return 0;
+}
+
+static const struct dev_pm_ops mxc_gpio_dev_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mxc_gpio_noirq_suspend, mxc_gpio_noirq_resume)
+};
+
 static int mxc_gpio_syscore_suspend(void)
 {
 	struct mxc_gpio_port *port;
@@ -537,6 +1273,7 @@ static struct platform_driver mxc_gpio_driver = {
 		.name	= "gpio-mxc",
 		.of_match_table = mxc_gpio_dt_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mxc_gpio_dev_pm_ops,
 	},
 	.probe		= mxc_gpio_probe,
 };
--
2.34.1

Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Linus Walleij]

On Fri, Oct 7, 2022 at 5:29 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:

> On i.MX8QM/QXP/DXL SoCs, even a GPIO is selected as the wakeup source,
> the GPIO block will be powered off when system enters into suspend
> state. This can greatly reduce the power consumption of suspend state
> because the whole partition can be shutdown. This is called PAD wakeup
> feature on i.MX8x platform.
>
> This patch enables this wakeup feature on i.MX8QM/QXP/DXL platforms.
>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  Changes in v2:
>  - fix a bug when finding the pad index

This:

> +#define IMX_SC_PAD_FUNC_GET_WAKEUP     9
> +#define IMX_SC_PAD_FUNC_SET_WAKEUP     4
> +#define IMX_SC_IRQ_GROUP_WAKE           3   /* Wakeup interrupts */
> +#define IMX_SC_IRQ_PAD                 2    /* Pad wakeup */
> +
> +#define IMX_SC_PAD_WAKEUP_OFF          0
> +#define IMX_SC_PAD_WAKEUP_LOW_LVL      4
> +#define IMX_SC_PAD_WAKEUP_FALL_EDGE    5
> +#define IMX_SC_PAD_WAKEUP_RISE_EDGE    6
> +#define IMX_SC_PAD_WAKEUP_HIGH_LVL     7
(...)

> +const struct mxc_gpio_pad_map imx8qm_pad_map[] = {
> +       /* GPIO0 */
> +       {  0, IMX8QM_SIM0_CLK},
> +       {  1, IMX8QM_SIM0_RST},
> +       {  2, IMX8QM_SIM0_IO},
> +       {  3, IMX8QM_SIM0_PD},

this is pin control.

I think you need to think about adding this to the i.MX pin control
driver instead, possibly cooperaring with the GPIO driver.

I'm not entirely certain about the relationship between MXC and
i.MX so correct me if I get this wrong...

To me this looks like two drivers could end up fighting about the
control of the same hardware if you don't.

Also CC i.MX Freescale pin control maintainers on all patches
like this, thanks! (Added on to-line.)

Yours,
Linus Walleij

RE: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Shenwei Wang]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Monday, October 17, 2022 4:39 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Aisheng Dong
> <aisheng.dong@nxp.com>; Fabio Estevam <festevam@gmail.com>; Shawn Guo
> <shawnguo@kernel.org>; Jacky Bai <ping.bai@nxp.com>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>; linux-gpio@vger.kernel.org;
> imx@lists.linux.dev; Pengutronix Kernel Team <kernel@pengutronix.de>
> Subject: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for
> i.mx8x SoCs
> 
> Caution: EXT Email
> 
> On Fri, Oct 7, 2022 at 5:29 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:
> 
> > On i.MX8QM/QXP/DXL SoCs, even a GPIO is selected as the wakeup source,
> > the GPIO block will be powered off when system enters into suspend
> > state. This can greatly reduce the power consumption of suspend state
> > because the whole partition can be shutdown. This is called PAD wakeup
> > feature on i.MX8x platform.
> >
> > This patch enables this wakeup feature on i.MX8QM/QXP/DXL platforms.
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > ---
> >  Changes in v2:
> >  - fix a bug when finding the pad index
> 
> This:
> 
> > +#define IMX_SC_PAD_FUNC_GET_WAKEUP     9
> > +#define IMX_SC_PAD_FUNC_SET_WAKEUP     4
> > +#define IMX_SC_IRQ_GROUP_WAKE           3   /* Wakeup interrupts */
> > +#define IMX_SC_IRQ_PAD                 2    /* Pad wakeup */
> > +
> > +#define IMX_SC_PAD_WAKEUP_OFF          0
> > +#define IMX_SC_PAD_WAKEUP_LOW_LVL      4
> > +#define IMX_SC_PAD_WAKEUP_FALL_EDGE    5
> > +#define IMX_SC_PAD_WAKEUP_RISE_EDGE    6
> > +#define IMX_SC_PAD_WAKEUP_HIGH_LVL     7
> (...)
> 
> > +const struct mxc_gpio_pad_map imx8qm_pad_map[] = {
> > +       /* GPIO0 */
> > +       {  0, IMX8QM_SIM0_CLK},
> > +       {  1, IMX8QM_SIM0_RST},
> > +       {  2, IMX8QM_SIM0_IO},
> > +       {  3, IMX8QM_SIM0_PD},
> 
> this is pin control.
> 
> I think you need to think about adding this to the i.MX pin control driver instead,
> possibly cooperaring with the GPIO driver.
> 
> I'm not entirely certain about the relationship between MXC and i.MX so correct
> me if I get this wrong...

This is not pin control. The wakeup function doesn't change any PINCtrl settings.  On i.MX8
platform, there is an extra pad wakeup logic and each pin can be configured as the wakeup source 
during system suspend. Because GPIO module is powered off on i.MX8 platform in suspend 
state, the GPIO's native wakeup won't work anymore. This patch just maps a GPIO wakeup to the 
corresponding PAD wakeup, and this mapping doesn't impact or change any PINCtrl settings.
Also the pad wakeup feature here has a great power consumption advantage comparing with the 
GPIO module's native wakeup.

> 
> To me this looks like two drivers could end up fighting about the control of the
> same hardware if you don't.
> 

It won't. This patch doesn't interact with any pinctrl relating registers at all.

> Also CC i.MX Freescale pin control maintainers on all patches like this, thanks!
> (Added on to-line.)
> 

Will add them in the future.

Regards,
Shenwei

> Yours,
> Linus Walleij

Re: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Linus Walleij]

On Mon, Oct 17, 2022 at 3:55 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:
> > On Fri, Oct 7, 2022 at 5:29 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:
> >
> > > On i.MX8QM/QXP/DXL SoCs, even a GPIO is selected as the wakeup source,
> > > the GPIO block will be powered off when system enters into suspend
> > > state. This can greatly reduce the power consumption of suspend state
> > > because the whole partition can be shutdown. This is called PAD wakeup
> > > feature on i.MX8x platform.
> > >
> > > This patch enables this wakeup feature on i.MX8QM/QXP/DXL platforms.
> > >
> > > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > > ---
> > >  Changes in v2:
> > >  - fix a bug when finding the pad index
> >
> > This:
> >
> > > +#define IMX_SC_PAD_FUNC_GET_WAKEUP     9
> > > +#define IMX_SC_PAD_FUNC_SET_WAKEUP     4
> > > +#define IMX_SC_IRQ_GROUP_WAKE           3   /* Wakeup interrupts */
> > > +#define IMX_SC_IRQ_PAD                 2    /* Pad wakeup */
> > > +
> > > +#define IMX_SC_PAD_WAKEUP_OFF          0
> > > +#define IMX_SC_PAD_WAKEUP_LOW_LVL      4
> > > +#define IMX_SC_PAD_WAKEUP_FALL_EDGE    5
> > > +#define IMX_SC_PAD_WAKEUP_RISE_EDGE    6
> > > +#define IMX_SC_PAD_WAKEUP_HIGH_LVL     7
> > (...)
> >
> > > +const struct mxc_gpio_pad_map imx8qm_pad_map[] = {
> > > +       /* GPIO0 */
> > > +       {  0, IMX8QM_SIM0_CLK},
> > > +       {  1, IMX8QM_SIM0_RST},
> > > +       {  2, IMX8QM_SIM0_IO},
> > > +       {  3, IMX8QM_SIM0_PD},
> >
> > this is pin control.
> >
> > I think you need to think about adding this to the i.MX pin control driver instead,
> > possibly cooperaring with the GPIO driver.
> >
> > I'm not entirely certain about the relationship between MXC and i.MX so correct
> > me if I get this wrong...
>
> This is not pin control.

So when something in a driver starts to enumerate all the pads on a
system and assign
them properties, I tend to think it is some kind of pin control.

You have things like this:

+const struct mxc_gpio_pad_map imx8qm_pad_map[] = {
+       /* GPIO0 */
+       {  0, IMX8QM_SIM0_CLK},
+       {  1, IMX8QM_SIM0_RST},
+       {  2, IMX8QM_SIM0_IO},
+       {  3, IMX8QM_SIM0_PD},
+       {  4, IMX8QM_SIM0_POWER_EN},
+       {  5, IMX8QM_SIM0_GPIO0_00},
+       {  6, IMX8QM_M40_I2C0_SCL},
+       {  7, IMX8QM_M40_I2C0_SDA},
+       {  8, IMX8QM_M40_GPIO0_00},
+       {  9, IMX8QM_M40_GPIO0_01},
+       { 10, IMX8QM_M41_I2C0_SCL},
(...)


Which is duplicating the same defines in
drivers/pinctrl/freescale/pinctrl-imx8qm.c:

static const struct pinctrl_pin_desc imx8qm_pinctrl_pads[] = {
        IMX_PINCTRL_PIN(IMX8QM_SIM0_CLK),
        IMX_PINCTRL_PIN(IMX8QM_SIM0_RST),
        IMX_PINCTRL_PIN(IMX8QM_SIM0_IO),
        IMX_PINCTRL_PIN(IMX8QM_SIM0_PD),
        IMX_PINCTRL_PIN(IMX8QM_SIM0_POWER_EN),
        IMX_PINCTRL_PIN(IMX8QM_SIM0_GPIO0_00),
        IMX_PINCTRL_PIN(IMX8QM_COMP_CTL_GPIO_1V8_3V3_SIM),
        IMX_PINCTRL_PIN(IMX8QM_M40_I2C0_SCL),
        IMX_PINCTRL_PIN(IMX8QM_M40_I2C0_SDA),
        IMX_PINCTRL_PIN(IMX8QM_M40_GPIO0_00),
        IMX_PINCTRL_PIN(IMX8QM_M40_GPIO0_01),
        IMX_PINCTRL_PIN(IMX8QM_M41_I2C0_SCL),
(...)

This is blatant code duplication, at the very least we need to find a way
to share this pin library, because it is irresponsible to put the same
information into the kernel twice. You can easily imagine what happens
if one of these pin tables contain an error and it only gets fixed
in one of these two places.

> The wakeup function doesn't change any PINCtrl settings.  On i.MX8
> platform, there is an extra pad wakeup logic and each pin can be configured as the wakeup source
> during system suspend.

So pads, pins, fingers - all these funny names for things like that - those are
handled by the pin control subsystem.

> Because GPIO module is powered off on i.MX8 platform in suspend
> state, the GPIO's native wakeup won't work anymore. This patch just maps a GPIO wakeup to the
> corresponding PAD wakeup, and this mapping doesn't impact or change any PINCtrl settings.
> Also the pad wakeup feature here has a great power consumption advantage comparing with the
> GPIO module's native wakeup.
>
> >
> > To me this looks like two drivers could end up fighting about the control of the
> > same hardware if you don't.
> >
>
> It won't. This patch doesn't interact with any pinctrl relating registers at all.

It interacts with the i.MX SCU and the i.MX pin control does exactly that.

What it does at the end is to call this RPC to set up the asynchronous pad ring:

ret = imx_scu_call_rpc(ipc_handle, &msg, true);

Consider:
drivers/pinctrl/freescale/pinctrl-scu.c

Which also issues a while bunch of imx_scu_call_rpc().

What I think you should do is call into the pin control back-end and have
pinctrl-scu.c handle this for you.

An ordinary vanilla pin controller would use the configs from
include/linux/pinctrl/pinconf-generic.h
but I think the i.MX pin config is custom different, so you need to
figure out what config to pass and how.

drivers/gpio/gpio-mxc.c should however implement gpio line
configuration, somewhere along the lines of:

port->gc.set_config = gpiochip_generic_config;

Then if you look into gpiochip_generic_config you can see that this will
call into the pin controller back-end:

int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
                            unsigned long config)
{
        return pinctrl_gpio_set_config(gc->gpiodev->base + offset, config);
}

At this point there must be a mechanism in place to cross-reference
between GPIO lines and pins, the most common way to solve this is
to use gpio-ranges, see Documentation/devicetree/bindings/gpio/gpio.txt.

This will hopefully end up in .pin_config_set()->imx_pinconf_set, in
drivers/pinctrl/freescale/pinctrl-imx.c which looks like this:

static int imx_pinconf_set(struct pinctrl_dev *pctldev,
                           unsigned pin_id, unsigned long *configs,
                           unsigned num_configs)
{
        struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
        const struct imx_pinctrl_soc_info *info = ipctl->info;

        if (info->flags & IMX_USE_SCU)
                return info->imx_pinconf_set(pctldev, pin_id,
                                           configs, num_configs);
        else
                return imx_pinconf_set_mmio(pctldev, pin_id,
                                            configs, num_configs);
}

Here one path goes down into the SCU, and there your custom wakeup
config will be handled.

At least this is how I imagine it, the i.MX maintainers need to say how
this should work in their opinion, but code duplication and two unsyncronized
clients sending random messages to the SCU surely is not an
option.

Yours,
Linus Walleij

RE: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Shenwei Wang]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Tuesday, October 18, 2022 3:21 AM
> > > > +       {  2, IMX8QM_SIM0_IO},
> > > > +       {  3, IMX8QM_SIM0_PD},
> > >
> > > this is pin control.
> > >
> > > I think you need to think about adding this to the i.MX pin control
> > > driver instead, possibly cooperaring with the GPIO driver.
> > >
> > > I'm not entirely certain about the relationship between MXC and i.MX
> > > so correct me if I get this wrong...
> >
> > This is not pin control.
> 
> So when something in a driver starts to enumerate all the pads on a system and
> assign them properties, I tend to think it is some kind of pin control.
> 
> You have things like this:
> 
> +const struct mxc_gpio_pad_map imx8qm_pad_map[] = {
> +       /* GPIO0 */
> +       {  0, IMX8QM_SIM0_CLK},
> +       {  1, IMX8QM_SIM0_RST},
> +       {  2, IMX8QM_SIM0_IO},
> +       {  3, IMX8QM_SIM0_PD},
> +       {  4, IMX8QM_SIM0_POWER_EN},
> +       {  5, IMX8QM_SIM0_GPIO0_00},
> +       {  6, IMX8QM_M40_I2C0_SCL},
> +       {  7, IMX8QM_M40_I2C0_SDA},
> +       {  8, IMX8QM_M40_GPIO0_00},
> +       {  9, IMX8QM_M40_GPIO0_01},
> +       { 10, IMX8QM_M41_I2C0_SCL},
> (...)
> 
> 
> Which is duplicating the same defines in
> drivers/pinctrl/freescale/pinctrl-imx8qm.c:
> 
> static const struct pinctrl_pin_desc imx8qm_pinctrl_pads[] = {
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_CLK),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_RST),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_IO),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_PD),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_POWER_EN),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_GPIO0_00),
>         IMX_PINCTRL_PIN(IMX8QM_COMP_CTL_GPIO_1V8_3V3_SIM),
>         IMX_PINCTRL_PIN(IMX8QM_M40_I2C0_SCL),
>         IMX_PINCTRL_PIN(IMX8QM_M40_I2C0_SDA),
>         IMX_PINCTRL_PIN(IMX8QM_M40_GPIO0_00),
>         IMX_PINCTRL_PIN(IMX8QM_M40_GPIO0_01),
>         IMX_PINCTRL_PIN(IMX8QM_M41_I2C0_SCL),
> (...)
> 

Thank you very much for the detailed review.

#define PINCTRL_PIN(a, b) { .number = a, .name = b }
#define IMX_PINCTRL_PIN(pin) PINCTRL_PIN(pin, #pin)

The imx8qm_pinctrl_pads array gives each PIN a string type of name.  The array I defined in
this patch adds another dimension information of the PIN, which is the GPIO index. These
two are not duplicated information. 

When I started to implement this feature, the first thing I thought is to use the existing
information to  calculate the GPIO index, but sadly I couldn't find a solution in the end.
The purpose for the array here is to record the relationship of a GPIO line with a PIN.
From the PINCTRL driver, you may get the information regarding a PIN's Mux status
and something relating to the signal settings like pullup/pulldown/drive strength. So
you can know if a PIN is a GPIO function PIN or not, but you can't know which GPIO port and
which GPIO line the PIN belongs to.  

> This is blatant code duplication, at the very least we need to find a way to share
> this pin library, because it is irresponsible to put the same information into the
> kernel twice. You can easily imagine what happens if one of these pin tables
> contain an error and it only gets fixed in one of these two places.
> 

As I explained above, this is not the duplicated information. It adds another
dimension of a information regarding a PIN. As these information is standalone
and self-contained,  no matter how you change the pin tables, it won't impact
the function here.

> > The wakeup function doesn't change any PINCtrl settings.  On i.MX8
> > platform, there is an extra pad wakeup logic and each pin can be
> > configured as the wakeup source during system suspend.
> 
> So pads, pins, fingers - all these funny names for things like that - those are
> handled by the pin control subsystem.
> 

This is a kind of GPIO wakeup function, and it happened to be given a name called
pad wakeup. From the user point of view, it works the same way as the native
GPIO wakeup. Except the name itself, it has nothing to do with the PINCTRL.

> > Because GPIO module is powered off on i.MX8 platform in suspend state,
> > the GPIO's native wakeup won't work anymore. This patch just maps a
> > GPIO wakeup to the corresponding PAD wakeup, and this mapping doesn't
> impact or change any PINCtrl settings.
> > Also the pad wakeup feature here has a great power consumption
> > advantage comparing with the GPIO module's native wakeup.
> >
> > >
> > > To me this looks like two drivers could end up fighting about the
> > > control of the same hardware if you don't.
> > >
> >
> > It won't. This patch doesn't interact with any pinctrl relating registers at all.
> 
> It interacts with the i.MX SCU and the i.MX pin control does exactly that.
> 
> What it does at the end is to call this RPC to set up the asynchronous pad ring:
> 
> ret = imx_scu_call_rpc(ipc_handle, &msg, true);
> 
> Consider:
> drivers/pinctrl/freescale/pinctrl-scu.c
> 
> Which also issues a while bunch of imx_scu_call_rpc().
> 
> What I think you should do is call into the pin control back-end and have pinctrl-
> scu.c handle this for you.
> 

Normally there are multiple information behind a resource for the scu API calls. 
Each information may determine its own functionality. Let take the 
resource IMX_SC_R_UART_0 as an example, it has Power domain and Clock functions 
associated.  
You will call imx_scu_call_rpc in the power domain driver to turn on/off its power, and call
the function imx_scu_call_rpc again in the clock driver to turn on/off its clock. However,
you don't need to synchronize the power domain driver and the clock driver, because they
are controlling two different resources. The pad wakeup function is the same concept here 
comparing with the other PINCTRL information.

> An ordinary vanilla pin controller would use the configs from
> include/linux/pinctrl/pinconf-generic.h
> but I think the i.MX pin config is custom different, so you need to figure out
> what config to pass and how.
> 
> drivers/gpio/gpio-mxc.c should however implement gpio line configuration,
> somewhere along the lines of:
> 
> port->gc.set_config = gpiochip_generic_config;
> 
> Then if you look into gpiochip_generic_config you can see that this will call into
> the pin controller back-end:
> 
> int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
>                             unsigned long config) {
>         return pinctrl_gpio_set_config(gc->gpiodev->base + offset, config); }
> 
> At this point there must be a mechanism in place to cross-reference between
> GPIO lines and pins, the most common way to solve this is to use gpio-ranges,
> see Documentation/devicetree/bindings/gpio/gpio.txt.
> 

In order to build up the cross-reference between GPIO lines and the PINs, you need
have a pre-prepared gpio-ranges mapping in advance. Because the relationship between
the PIN and the GPIO line is not a linear, you end up to build up the gpio-ranges in the same 
way like the array in this patch. This turns to a chicken-egg problem.

> This will hopefully end up in .pin_config_set()->imx_pinconf_set, in
> drivers/pinctrl/freescale/pinctrl-imx.c which looks like this:
> 
> static int imx_pinconf_set(struct pinctrl_dev *pctldev,
>                            unsigned pin_id, unsigned long *configs,
>                            unsigned num_configs) {
>         struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
>         const struct imx_pinctrl_soc_info *info = ipctl->info;
> 
>         if (info->flags & IMX_USE_SCU)
>                 return info->imx_pinconf_set(pctldev, pin_id,
>                                            configs, num_configs);
>         else
>                 return imx_pinconf_set_mmio(pctldev, pin_id,
>                                             configs, num_configs); }
> 
> Here one path goes down into the SCU, and there your custom wakeup config
> will be handled.
> 
> At least this is how I imagine it, the i.MX maintainers need to say how this should
> work in their opinion, but code duplication and two unsyncronized clients
> sending random messages to the SCU surely is not an option.

As I explained in the above, this is not the problem of two clients accessing the same
resource, so there is no conflict between the two drivers. It works in the same way 
like SCU power domain driver and SCU clock driver. The communication integrity is 
guaranteed by the call of imx_scu_call_rpc itself.

Regards,
Shenwei

> 
> Yours,
> Linus Walleij

Re: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Linus Walleij]

On Tue, Oct 18, 2022 at 9:42 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:

> #define PINCTRL_PIN(a, b) { .number = a, .name = b }
> #define IMX_PINCTRL_PIN(pin) PINCTRL_PIN(pin, #pin)
>
> The imx8qm_pinctrl_pads array gives each PIN a string type of name.  The array I defined in
> this patch adds another dimension information of the PIN, which is the GPIO index. These
> two are not duplicated information.
(...)
> The purpose for the array here is to record the relationship of a GPIO line with a PIN.
(...)
> From the PINCTRL driver, you may get the information regarding a PIN's Mux status
> and something relating to the signal settings like pullup/pulldown/drive strength. So
> you can know if a PIN is a GPIO function PIN or not, but you can't know which GPIO port and
> which GPIO line the PIN belongs to.

Cross-referencing GPIO line numbers to pins/pads is literally the
job of gpio-ranges (the DT property) and we have excellent support in
the GPIO library to do exactly this.

It can even be made using static data in the driver if gpio-ranges for
some reason cannot be encoded in the device tree.

(...)
> In order to build up the cross-reference between GPIO lines and the PINs, you need
> have a pre-prepared gpio-ranges mapping in advance. Because the relationship between
> the PIN and the GPIO line is not a linear, you end up to build up the gpio-ranges in the same
> way like the array in this patch. This turns to a chicken-egg problem.

I don't get it. gpio-ranges can contain any number of "holes" and whatever,
the name "rangeS" (plural) implies it can be an array of ranges. If you want
you can have a list of single-pin ranges, no problem.

> As I explained above, this is not the duplicated information. It adds another
> dimension of a information regarding a PIN. As these information is standalone
> and self-contained,  no matter how you change the pin tables, it won't impact
> the function here.

Since the information/setting pertains to an electronic or similar property
of a pin it falls under the pin config umbrella.

> This is a kind of GPIO wakeup function, and it happened to be given a name called
> pad wakeup. From the user point of view, it works the same way as the native
> GPIO wakeup. Except the name itself, it has nothing to do with the PINCTRL.

Hardly.

The MMC/SD card bus has ways of waking up devices by pulling a line
low, as does things such as UARTs. And then the pin isn't even used as
GPIO.

I bet a million to one that the exact same setting is used inside the
SCU for waking up on such events on the pins.

I don't believe that just because a pin is muxed to something else than GPIO
it cannot be programmed to wake the system up.

If you look into my presentation "building GPIO and pins from the ground up":
http://www.df.lth.se/~triad/papers/pincontrol.pdf
see the picture on page 13 and page 18. The asynchronous edge detector is
what takes the system online in sleep. That is a property of the pin
cell, it has
nothing to do with the GPIO block, which is further in.

It belongs in pin control because it is a property of the pin hardware.

> As I explained in the above, this is not the problem of two clients accessing the same
> resource, so there is no conflict between the two drivers. It works in the same way
> like SCU power domain driver and SCU clock driver. The communication integrity is
> guaranteed by the call of imx_scu_call_rpc itself.

I understand that it makes your life simpler to just implement this as a hack
in the MXC GPIO driver like this, but it is not a proper solution, and I ask you
to do the more complicated and work consuming task instead.

This is because it will have partitioned the problem in a clear way that is
recognizable by the maintainers and will scale to other SoCs in the future.

I also wonder where all the other i.MX maintainers are. I need their input
in this discussion.

Yours,
Linus Walleij

RE: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Shenwei Wang]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Friday, October 21, 2022 3:27 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> > function PIN or not, but you can't know which GPIO port and which GPIO line
> the PIN belongs to.
> 
> Cross-referencing GPIO line numbers to pins/pads is literally the job of gpio-
> ranges (the DT property) and we have excellent support in the GPIO library to do
> exactly this.
> 
> It can even be made using static data in the driver if gpio-ranges for some
> reason cannot be encoded in the device tree.
> 

The current implementation is using the static data inside the gpio driver, although it
is not used the gpio-range data structure. 

> (...)
> > In order to build up the cross-reference between GPIO lines and the
> > PINs, you need have a pre-prepared gpio-ranges mapping in advance.
> > Because the relationship between the PIN and the GPIO line is not a
> > linear, you end up to build up the gpio-ranges in the same way like the array in
> this patch. This turns to a chicken-egg problem.
> 
> I don't get it. gpio-ranges can contain any number of "holes" and whatever, the
> name "rangeS" (plural) implies it can be an array of ranges. If you want you can
> have a list of single-pin ranges, no problem.
> 

 No matter you prepare the mapping by using the gpio-ranges or the static array in the 
driver,  they serve the same purpose. The only user to use the mapping is the gpio driver,
and the mapping is constant for a SoC family. That's why I chose to define the array inside
the driver.

> > As I explained above, this is not the duplicated information. It adds
> > another dimension of a information regarding a PIN. As these
> > information is standalone and self-contained,  no matter how you
> > change the pin tables, it won't impact the function here.
> 
> Since the information/setting pertains to an electronic or similar property of a
> pin it falls under the pin config umbrella.
> 
> > This is a kind of GPIO wakeup function, and it happened to be given a
> > name called pad wakeup. From the user point of view, it works the same
> > way as the native GPIO wakeup. Except the name itself, it has nothing to do
> with the PINCTRL.
> 
> Hardly.
> 
> The MMC/SD card bus has ways of waking up devices by pulling a line low, as
> does things such as UARTs. And then the pin isn't even used as GPIO.
> 

That's another kind of wakup method. Currently we are talking about the pad
wakeup. The examples you gave like SD/UART, they are not pad wakeup feature,
we can call them as IP native wakeup. For example, both GPIO and SD ( or UART) have their
own IP native wakeup function. The difference here on i.MX8 platform is that
if you use UART's native wakeup, you have to keep power/clock to the UART IP block during the 
sleep state. The same requirement for the GPIO and SD's native wakeup too. 

> I bet a million to one that the exact same setting is used inside the SCU for
> waking up on such events on the pins.
> 

For those IP blocks' native wakeup, they are not managed by SCU. It is managed by
the block's driver directly. For example, the UART's native wakeup function is
enabled or disabled via the driver itself, and no communication required for SCU.

> I don't believe that just because a pin is muxed to something else than GPIO it
> cannot be programmed to wake the system up.
> 
> If you look into my presentation "building GPIO and pins from the ground up":
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.df.lt
> h.se%2F~triad%2Fpapers%2Fpincontrol.pdf&amp;data=05%7C01%7Cshenwei.w
> ang%40nxp.com%7C5f49f959229e452da06408dab33e0e38%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C1%7C638019376349152251%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Cqud0jkMikeKz1Ok%2Bgm55mx
> kCeK3HQlqRU5n5RxU8VU%3D&amp;reserved=0
> see the picture on page 13 and page 18. The asynchronous edge detector is
> what takes the system online in sleep. That is a property of the pin cell, it has
> nothing to do with the GPIO block, which is further in.
> 
> It belongs in pin control because it is a property of the pin hardware.

That's a very good presentation, but it is away from the topic. The GPIO block has
its own edge detector logic for its native wakeup function. Here the pad wakeup
feature is implemented via an extra hardware logic, and it is an independent IP block.

> 
> > As I explained in the above, this is not the problem of two clients
> > accessing the same resource, so there is no conflict between the two
> > drivers. It works in the same way like SCU power domain driver and SCU
> > clock driver. The communication integrity is guaranteed by the call of
> imx_scu_call_rpc itself.
> 
> I understand that it makes your life simpler to just implement this as a hack in
> the MXC GPIO driver like this, but it is not a proper solution, and I ask you to do
> the more complicated and work consuming task instead.
> 

This is not a hack because the imx_scu_call_rpc function is designed to serve multiple threads
use case. As the pad wakeup function is an independent feature accompany 
with the gpio function, I prefer to put the implementation inside the gpio driver.

Regards,
Shenwei

> This is because it will have partitioned the problem in a clear way that is
> recognizable by the maintainers and will scale to other SoCs in the future.
> 
> I also wonder where all the other i.MX maintainers are. I need their input in this
> discussion.
> 
> Yours,
> Linus Walleij

Re: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Linus Walleij]

On Mon, Oct 24, 2022 at 5:05 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:

> > Cross-referencing GPIO line numbers to pins/pads is literally the job of gpio-
> > ranges (the DT property) and we have excellent support in the GPIO library to do
> > exactly this.
> >
> > It can even be made using static data in the driver if gpio-ranges for some
> > reason cannot be encoded in the device tree.
> >
>
> The current implementation is using the static data inside the gpio driver, although it
> is not used the gpio-range data structure.

So get to it. Use gpio-ranges and put this functionality in the pin control
portions.

> > I don't get it. gpio-ranges can contain any number of "holes" and whatever, the
> > name "rangeS" (plural) implies it can be an array of ranges. If you want you can
> > have a list of single-pin ranges, no problem.
> >
>
>  No matter you prepare the mapping by using the gpio-ranges or the static array in the
> driver,  they serve the same purpose. The only user to use the mapping is the gpio driver,
> and the mapping is constant for a SoC family. That's why I chose to define the array inside
> the driver.

This is not an excuse to not use gpio-ranges and duplicate code and
data. Switch to using gpio-ranges.

> > The MMC/SD card bus has ways of waking up devices by pulling a line low, as
> > does things such as UARTs. And then the pin isn't even used as GPIO.
> >
>
> That's another kind of wakup method. Currently we are talking about the pad
> wakeup. The examples you gave like SD/UART, they are not pad wakeup feature,
> we can call them as IP native wakeup.

Hm intersting!

> For those IP blocks' native wakeup, they are not managed by SCU. It is managed by
> the block's driver directly. For example, the UART's native wakeup function is
> enabled or disabled via the driver itself, and no communication required for SCU.

OK I was wrong about that.

> > > As I explained in the above, this is not the problem of two clients
> > > accessing the same resource, so there is no conflict between the two
> > > drivers. It works in the same way like SCU power domain driver and SCU
> > > clock driver. The communication integrity is guaranteed by the call of
> > imx_scu_call_rpc itself.
> >
> > I understand that it makes your life simpler to just implement this as a hack in
> > the MXC GPIO driver like this, but it is not a proper solution, and I ask you to do
> > the more complicated and work consuming task instead.
>
> This is not a hack because the imx_scu_call_rpc function is designed to serve multiple threads
> use case. As the pad wakeup function is an independent feature accompany
> with the gpio function, I prefer to put the implementation inside the gpio driver.

Our job as maintainers is to make sure the code is easy to maintain.

If duplicate data is stored in the GPIO driver and the pin control driver
just because gpio-ranges are not used to be able to put the wakeup code
together with the rest of the pad/pin related data, that means more work for
us.

For this reason:
1. use gpio-ranges to reference the pin control subsystem
2. Act on the config inside the SCU pin control driver which already knows
  about all pads/pins.

It just makes more sense for us.

Yours,
Linus Walleij