arm: omap3: am35x: Convert emac to hwmod & disable hlt when open

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs has a Davinci EMAC ethernet controller on-chip. Unfortunately, the EMAC is unable to wake the PRCM when there is network activity which leads to a hung or extremely slow system when the MPU has executed a 'wfi' instruction (because of pm_idle or CPUidle). To prevent this, add hooks to the EMAC pm_runtime suspend/resume calls so that hlt is disabled whenever the EMAC is in use.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/am35xx-emac.c     |   44 +++++++++++++++++++++++++++++----
 arch/arm/mach-omap2/am35xx-emac.h     |   16 +++++++++---
 arch/arm/mach-omap2/board-am3517evm.c |    3 ++-
 arch/arm/mach-omap2/board-cm-t3517.c  |    3 ++-
 4 files changed, 56 insertions(+), 10 deletions(-)

arch/arm/mach-omap2/am35xx-emac.c +39 -5

index 3bb5cb3..22ff968 100644
@@ -23,6 +23,37 @@
 #include "control.h"
 #include "am35xx-emac.h"
 
+/*
+ * Default pm_lats for the am35x.
+ * The net effect of using am35xx_emac_pm_lats[] is that
+ * pm_idle or CPUidle won't be called while the emac
+ * interface is open.  This is required because the
+ * EMAC can't wake up PRCM so if the MPU is executing
+ * a 'wfi' instruction (e.g., from pm_idle or CPUidle),
+ * it won't break out of it due to emac activity.
+ */
+static int am35xx_emac_deactivate_func(struct omap_device *od)
+{
+	enable_hlt();
+	return omap_device_idle_hwmods(od);
+}
+
+static int am35xx_emac_activate_func(struct omap_device *od)
+{
+	disable_hlt();
+	return omap_device_enable_hwmods(od);
+}
+
+struct omap_device_pm_latency am35xx_emac_pm_lats[] = {
+	{
+		.deactivate_func	= am35xx_emac_deactivate_func,
+		.activate_func		= am35xx_emac_activate_func,
+		.flags			= OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	},
+};
+
+int am35xx_emac_pm_lats_size = ARRAY_SIZE(am35xx_emac_pm_lats);
+
 static void am35xx_enable_emac_int(void)
 {
 	u32 regval;

@@ -61,12 +92,13 @@ static struct emac_platform_data am35xx_emac_pdata = {
 static struct mdio_platform_data am35xx_mdio_pdata;
 
 static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
-		void *pdata, int pdata_len)
+		void *pdata, int pdata_len,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct platform_device *pdev;
 
 	pdev = omap_device_build(oh->class->name, 0, oh, pdata, pdata_len,
-			NULL, 0, false);
+			pm_lats, pm_lats_size, false);
 	if (IS_ERR(pdev)) {
 		WARN(1, "Can't build omap_device for %s:%s.\n",
 					oh->class->name, oh->name);

@@ -76,7 +108,8 @@ static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
 	return 0;
 }
 
-void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
+void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct omap_hwmod *oh;
 	u32 regval;

@@ -91,7 +124,7 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_mdio_pdata.bus_freq = mdio_bus_freq;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_mdio_pdata,
-					 sizeof(am35xx_mdio_pdata));
+					 sizeof(am35xx_mdio_pdata), NULL, 0);
 	if (ret) {
 		pr_err("Could not build davinci_mdio hwmod device\n");
 		return;

@@ -106,7 +139,8 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_emac_pdata.rmii_en = rmii_en;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_emac_pdata,
-					 sizeof(am35xx_emac_pdata));
+					 sizeof(am35xx_emac_pdata),
+					 pm_lats, pm_lats_size);
 	if (ret) {
 		pr_err("Could not build davinci_emac hwmod device\n");
 		return;

arch/arm/mach-omap2/am35xx-emac.h +13 -3

index 15c6f9c..7c23808 100644
@@ -6,10 +6,20 @@
  * published by the Free Software Foundation.
  */
 
+#include <plat/omap_device.h>
+
 #define AM35XX_DEFAULT_MDIO_FREQUENCY	1000000
 
-#if defined(CONFIG_TI_DAVINCI_EMAC) || defined(CONFIG_TI_DAVINCI_EMAC_MODULE)
-void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en);
+#if IS_ENABLED(CONFIG_TI_DAVINCI_EMAC)
+extern struct omap_device_pm_latency am35xx_emac_pm_lats[];
+extern int am35xx_emac_pm_lats_size;
+
+void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size);
 #else
-static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en) {}
+#define am35xx_emac_pm_lats		NULL
+#define am35xx_emac_pm_lats_size	0
+
+static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size) {}
 #endif

arch/arm/mach-omap2/board-am3517evm.c +0 -0

index 3645285..fe3a304 100644

arch/arm/mach-omap2/board-am3517evm.c +0 -0

 	/* MUSB */
 	am3517_evm_musb_init();

arch/arm/mach-omap2/board-cm-t3517.c +2 -1

index 9e66e16..0a3dc7a 100644
@@ -292,7 +292,8 @@ static void __init cm_t3517_init(void)
 	cm_t3517_init_rtc();
 	cm_t3517_init_usbh();
 	cm_t3517_init_hecc();
-	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1);
+	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1,
+			 am35xx_emac_pm_lats, am35xx_emac_pm_lats_size);
 }
 
 MACHINE_START(CM_T3517, "Compulab CM-T3517")

Hi Mark,

Thanks for the great work!

Quoted 141 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs has a Davinci EMAC ethernet controller on-chip. Unfortunately, the EMAC is unable to wake the PRCM when there is network activity which leads to a hung or extremely slow system when the MPU has executed a 'wfi' instruction (because of pm_idle or CPUidle). To prevent this, add hooks to the EMAC pm_runtime suspend/resume calls so that hlt is disabled whenever the EMAC is in use.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/am35xx-emac.c     |   44 +++++++++++++++++++++++++++++----
 arch/arm/mach-omap2/am35xx-emac.h     |   16 +++++++++---
 arch/arm/mach-omap2/board-am3517evm.c |    3 ++-
 arch/arm/mach-omap2/board-cm-t3517.c  |    3 ++-
 4 files changed, 56 insertions(+), 10 deletions(-)

arch/arm/mach-omap2/am35xx-emac.c +39 -5

index 3bb5cb3..22ff968 100644
@@ -23,6 +23,37 @@
 #include "control.h"
 #include "am35xx-emac.h"
 
+/*
+ * Default pm_lats for the am35x.
+ * The net effect of using am35xx_emac_pm_lats[] is that
+ * pm_idle or CPUidle won't be called while the emac
+ * interface is open.  This is required because the
+ * EMAC can't wake up PRCM so if the MPU is executing
+ * a 'wfi' instruction (e.g., from pm_idle or CPUidle),
+ * it won't break out of it due to emac activity.
+ */
+static int am35xx_emac_deactivate_func(struct omap_device *od)
+{
+	enable_hlt();
+	return omap_device_idle_hwmods(od);
+}
+
+static int am35xx_emac_activate_func(struct omap_device *od)
+{
+	disable_hlt();
+	return omap_device_enable_hwmods(od);
+}
+
+struct omap_device_pm_latency am35xx_emac_pm_lats[] = {
+	{
+		.deactivate_func	= am35xx_emac_deactivate_func,
+		.activate_func		= am35xx_emac_activate_func,
+		.flags			= OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	},
+};
+
+int am35xx_emac_pm_lats_size = ARRAY_SIZE(am35xx_emac_pm_lats);
+
 static void am35xx_enable_emac_int(void)
 {
 	u32 regval;

@@ -61,12 +92,13 @@ static struct emac_platform_data am35xx_emac_pdata = {
 static struct mdio_platform_data am35xx_mdio_pdata;
 
 static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
-		void *pdata, int pdata_len)
+		void *pdata, int pdata_len,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct platform_device *pdev;
 
 	pdev = omap_device_build(oh->class->name, 0, oh, pdata, pdata_len,
-			NULL, 0, false);
+			pm_lats, pm_lats_size, false);
 	if (IS_ERR(pdev)) {
 		WARN(1, "Can't build omap_device for %s:%s.\n",
 					oh->class->name, oh->name);

@@ -76,7 +108,8 @@ static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
 	return 0;
 }
 
-void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
+void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct omap_hwmod *oh;
 	u32 regval;

@@ -91,7 +124,7 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_mdio_pdata.bus_freq = mdio_bus_freq;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_mdio_pdata,
-					 sizeof(am35xx_mdio_pdata));
+					 sizeof(am35xx_mdio_pdata), NULL, 0);
 	if (ret) {
 		pr_err("Could not build davinci_mdio hwmod device\n");
 		return;

@@ -106,7 +139,8 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_emac_pdata.rmii_en = rmii_en;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_emac_pdata,
-					 sizeof(am35xx_emac_pdata));
+					 sizeof(am35xx_emac_pdata),
+					 pm_lats, pm_lats_size);
 	if (ret) {
 		pr_err("Could not build davinci_emac hwmod device\n");
 		return;

arch/arm/mach-omap2/am35xx-emac.h +13 -3

index 15c6f9c..7c23808 100644
@@ -6,10 +6,20 @@
  * published by the Free Software Foundation.
  */
 
+#include <plat/omap_device.h>
+
 #define AM35XX_DEFAULT_MDIO_FREQUENCY	1000000
 
-#if defined(CONFIG_TI_DAVINCI_EMAC) || defined(CONFIG_TI_DAVINCI_EMAC_MODULE)
-void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en);
+#if IS_ENABLED(CONFIG_TI_DAVINCI_EMAC)
+extern struct omap_device_pm_latency am35xx_emac_pm_lats[];
+extern int am35xx_emac_pm_lats_size;
+
+void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size);
 #else
-static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en) {}
+#define am35xx_emac_pm_lats		NULL
+#define am35xx_emac_pm_lats_size	0
+
+static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size) {}
 #endif

arch/arm/mach-omap2/board-am3517evm.c +0 -0

index 3645285..fe3a304 100644
@@ -385,7 +385,8 @@ static void __init am3517_evm_init(void)
 	i2c_register_board_info(1, am3517evm_i2c1_boardinfo,
 				ARRAY_SIZE(am3517evm_i2c1_boardinfo));

arch/arm/mach-omap2/board-am3517evm.c static void __init am3517_evm_init(void) · line 389

Quoted 4 lines

arch/arm/mach-omap2/board-am3517evm.c +2 -1

 	/*Ethernet*/
-	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1);
+	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1,
+			 am35xx_emac_pm_lats, am35xx_emac_pm_lats_size);

Why is it essential for board file to pass these pm structures? Is it something board specific?
Can't you just use them inside the am35xx-emac.c file?

Quoted 1 line · Igor Grinberg

Hi Mark,

Hi Igor.

Quoted 144 lines

Thanks for the great work!

Quoted 141 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs has a Davinci EMAC ethernet controller on-chip. Unfortunately, the EMAC is unable to wake the PRCM when there is network activity which leads to a hung or extremely slow system when the MPU has executed a 'wfi' instruction (because of pm_idle or CPUidle). To prevent this, add hooks to the EMAC pm_runtime suspend/resume calls so that hlt is disabled whenever the EMAC is in use.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/am35xx-emac.c     |   44 +++++++++++++++++++++++++++++----
 arch/arm/mach-omap2/am35xx-emac.h     |   16 +++++++++---
 arch/arm/mach-omap2/board-am3517evm.c |    3 ++-
 arch/arm/mach-omap2/board-cm-t3517.c  |    3 ++-
 4 files changed, 56 insertions(+), 10 deletions(-)

arch/arm/mach-omap2/am35xx-emac.c +39 -5

index 3bb5cb3..22ff968 100644
@@ -23,6 +23,37 @@
 #include "control.h"
 #include "am35xx-emac.h"
 
+/*
+ * Default pm_lats for the am35x.
+ * The net effect of using am35xx_emac_pm_lats[] is that
+ * pm_idle or CPUidle won't be called while the emac
+ * interface is open.  This is required because the
+ * EMAC can't wake up PRCM so if the MPU is executing
+ * a 'wfi' instruction (e.g., from pm_idle or CPUidle),
+ * it won't break out of it due to emac activity.
+ */
+static int am35xx_emac_deactivate_func(struct omap_device *od)
+{
+	enable_hlt();
+	return omap_device_idle_hwmods(od);
+}
+
+static int am35xx_emac_activate_func(struct omap_device *od)
+{
+	disable_hlt();
+	return omap_device_enable_hwmods(od);
+}
+
+struct omap_device_pm_latency am35xx_emac_pm_lats[] = {
+	{
+		.deactivate_func	= am35xx_emac_deactivate_func,
+		.activate_func		= am35xx_emac_activate_func,
+		.flags			= OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	},
+};
+
+int am35xx_emac_pm_lats_size = ARRAY_SIZE(am35xx_emac_pm_lats);
+
 static void am35xx_enable_emac_int(void)
 {
 	u32 regval;

@@ -61,12 +92,13 @@ static struct emac_platform_data am35xx_emac_pdata = {
 static struct mdio_platform_data am35xx_mdio_pdata;
 
 static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
-		void *pdata, int pdata_len)
+		void *pdata, int pdata_len,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct platform_device *pdev;
 
 	pdev = omap_device_build(oh->class->name, 0, oh, pdata, pdata_len,
-			NULL, 0, false);
+			pm_lats, pm_lats_size, false);
 	if (IS_ERR(pdev)) {
 		WARN(1, "Can't build omap_device for %s:%s.\n",
 					oh->class->name, oh->name);

@@ -76,7 +108,8 @@ static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
 	return 0;
 }
 
-void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
+void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct omap_hwmod *oh;
 	u32 regval;

@@ -91,7 +124,7 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_mdio_pdata.bus_freq = mdio_bus_freq;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_mdio_pdata,
-					 sizeof(am35xx_mdio_pdata));
+					 sizeof(am35xx_mdio_pdata), NULL, 0);
 	if (ret) {
 		pr_err("Could not build davinci_mdio hwmod device\n");
 		return;

@@ -106,7 +139,8 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_emac_pdata.rmii_en = rmii_en;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_emac_pdata,
-					 sizeof(am35xx_emac_pdata));
+					 sizeof(am35xx_emac_pdata),
+					 pm_lats, pm_lats_size);
 	if (ret) {
 		pr_err("Could not build davinci_emac hwmod device\n");
 		return;

arch/arm/mach-omap2/am35xx-emac.h +13 -3

index 15c6f9c..7c23808 100644
@@ -6,10 +6,20 @@
  * published by the Free Software Foundation.
  */
 
+#include <plat/omap_device.h>
+
 #define AM35XX_DEFAULT_MDIO_FREQUENCY	1000000
 
-#if defined(CONFIG_TI_DAVINCI_EMAC) || defined(CONFIG_TI_DAVINCI_EMAC_MODULE)
-void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en);
+#if IS_ENABLED(CONFIG_TI_DAVINCI_EMAC)
+extern struct omap_device_pm_latency am35xx_emac_pm_lats[];
+extern int am35xx_emac_pm_lats_size;
+
+void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size);
 #else
-static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en) {}
+#define am35xx_emac_pm_lats		NULL
+#define am35xx_emac_pm_lats_size	0
+
+static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size) {}
 #endif

arch/arm/mach-omap2/board-am3517evm.c +0 -0

index 3645285..fe3a304 100644
@@ -385,7 +385,8 @@ static void __init am3517_evm_init(void)
 	i2c_register_board_info(1, am3517evm_i2c1_boardinfo,
 				ARRAY_SIZE(am3517evm_i2c1_boardinfo));

Quoted 8 lines

arch/arm/mach-omap2/board-am3517evm.c static void __init am3517_evm_init(void) · line 389

Quoted 4 lines

arch/arm/mach-omap2/board-am3517evm.c +2 -1

 	/*Ethernet*/
-	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1);
+	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1,
+			 am35xx_emac_pm_lats, am35xx_emac_pm_lats_size);

Why is it essential for board file to pass these pm structures? Is it something board specific?
Can't you just use them inside the am35xx-emac.c file?

Great question. I went back & forth on this mayself (I have 3 different versions sitting in my repo :). I ended up passing the pm structures in case there is a variant that comes along that doesn't need this fixup (or a board that wants to do this plus additional things).

The variants that I have are:

1) Leave am35xx_emac_init() interface the way it is and automatically add the necessary things. The problem is lack of flexibility if & when some other variant comes along.

2) Add pm_lats & pm_lats_size to am35xx_emac_init() but if they're NULL or 0, then use the defaults that are the same as what's in this patch. More flexible but doesn't easily allow a board file to NULL out the pm structure (have to make up and pass NULL version).

3) Pass the pm struct via am35xx_emac_init() and just make the default available for use. This is what I submitted since it was the most flexible.

Which one do you prefer or do you have something else in mind?

Quoted 177 lines · Mark A. Greer

Quoted 1 line · Igor Grinberg

Hi Mark,

Hi Igor.

Quoted 144 lines

Thanks for the great work!

Quoted 141 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs has a Davinci EMAC ethernet controller on-chip. Unfortunately, the EMAC is unable to wake the PRCM when there is network activity which leads to a hung or extremely slow system when the MPU has executed a 'wfi' instruction (because of pm_idle or CPUidle). To prevent this, add hooks to the EMAC pm_runtime suspend/resume calls so that hlt is disabled whenever the EMAC is in use.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/am35xx-emac.c     |   44 +++++++++++++++++++++++++++++----
 arch/arm/mach-omap2/am35xx-emac.h     |   16 +++++++++---
 arch/arm/mach-omap2/board-am3517evm.c |    3 ++-
 arch/arm/mach-omap2/board-cm-t3517.c  |    3 ++-
 4 files changed, 56 insertions(+), 10 deletions(-)

arch/arm/mach-omap2/am35xx-emac.c +39 -5

index 3bb5cb3..22ff968 100644
@@ -23,6 +23,37 @@
 #include "control.h"
 #include "am35xx-emac.h"
 
+/*
+ * Default pm_lats for the am35x.
+ * The net effect of using am35xx_emac_pm_lats[] is that
+ * pm_idle or CPUidle won't be called while the emac
+ * interface is open.  This is required because the
+ * EMAC can't wake up PRCM so if the MPU is executing
+ * a 'wfi' instruction (e.g., from pm_idle or CPUidle),
+ * it won't break out of it due to emac activity.
+ */
+static int am35xx_emac_deactivate_func(struct omap_device *od)
+{
+	enable_hlt();
+	return omap_device_idle_hwmods(od);
+}
+
+static int am35xx_emac_activate_func(struct omap_device *od)
+{
+	disable_hlt();
+	return omap_device_enable_hwmods(od);
+}
+
+struct omap_device_pm_latency am35xx_emac_pm_lats[] = {
+	{
+		.deactivate_func	= am35xx_emac_deactivate_func,
+		.activate_func		= am35xx_emac_activate_func,
+		.flags			= OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	},
+};
+
+int am35xx_emac_pm_lats_size = ARRAY_SIZE(am35xx_emac_pm_lats);
+
 static void am35xx_enable_emac_int(void)
 {
 	u32 regval;

@@ -61,12 +92,13 @@ static struct emac_platform_data am35xx_emac_pdata = {
 static struct mdio_platform_data am35xx_mdio_pdata;
 
 static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
-		void *pdata, int pdata_len)
+		void *pdata, int pdata_len,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct platform_device *pdev;
 
 	pdev = omap_device_build(oh->class->name, 0, oh, pdata, pdata_len,
-			NULL, 0, false);
+			pm_lats, pm_lats_size, false);
 	if (IS_ERR(pdev)) {
 		WARN(1, "Can't build omap_device for %s:%s.\n",
 					oh->class->name, oh->name);

@@ -76,7 +108,8 @@ static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
 	return 0;
 }
 
-void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
+void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct omap_hwmod *oh;
 	u32 regval;

@@ -91,7 +124,7 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_mdio_pdata.bus_freq = mdio_bus_freq;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_mdio_pdata,
-					 sizeof(am35xx_mdio_pdata));
+					 sizeof(am35xx_mdio_pdata), NULL, 0);
 	if (ret) {
 		pr_err("Could not build davinci_mdio hwmod device\n");
 		return;

@@ -106,7 +139,8 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_emac_pdata.rmii_en = rmii_en;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_emac_pdata,
-					 sizeof(am35xx_emac_pdata));
+					 sizeof(am35xx_emac_pdata),
+					 pm_lats, pm_lats_size);
 	if (ret) {
 		pr_err("Could not build davinci_emac hwmod device\n");
 		return;

arch/arm/mach-omap2/am35xx-emac.h +13 -3

index 15c6f9c..7c23808 100644
@@ -6,10 +6,20 @@
  * published by the Free Software Foundation.
  */
 
+#include <plat/omap_device.h>
+
 #define AM35XX_DEFAULT_MDIO_FREQUENCY	1000000
 
-#if defined(CONFIG_TI_DAVINCI_EMAC) || defined(CONFIG_TI_DAVINCI_EMAC_MODULE)
-void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en);
+#if IS_ENABLED(CONFIG_TI_DAVINCI_EMAC)
+extern struct omap_device_pm_latency am35xx_emac_pm_lats[];
+extern int am35xx_emac_pm_lats_size;
+
+void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size);
 #else
-static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en) {}
+#define am35xx_emac_pm_lats		NULL
+#define am35xx_emac_pm_lats_size	0
+
+static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size) {}
 #endif

arch/arm/mach-omap2/board-am3517evm.c +0 -0

index 3645285..fe3a304 100644
@@ -385,7 +385,8 @@ static void __init am3517_evm_init(void)
 	i2c_register_board_info(1, am3517evm_i2c1_boardinfo,
 				ARRAY_SIZE(am3517evm_i2c1_boardinfo));

Quoted 8 lines

arch/arm/mach-omap2/board-am3517evm.c static void __init am3517_evm_init(void) · line 389

Quoted 4 lines

arch/arm/mach-omap2/board-am3517evm.c +2 -1

 	/*Ethernet*/
-	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1);
+	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1,
+			 am35xx_emac_pm_lats, am35xx_emac_pm_lats_size);

Why is it essential for board file to pass these pm structures? Is it something board specific?
Can't you just use them inside the am35xx-emac.c file?

Great question. I went back & forth on this mayself (I have 3 different versions sitting in my repo :). I ended up passing the pm structures in case there is a variant that comes along that doesn't need this fixup (or a board that wants to do this plus additional things).

The variants that I have are:

1) Leave am35xx_emac_init() interface the way it is and automatically add the necessary things. The problem is lack of flexibility if & when some other variant comes along.

2) Add pm_lats & pm_lats_size to am35xx_emac_init() but if they're NULL or 0, then use the defaults that are the same as what's in this patch. More flexible but doesn't easily allow a board file to NULL out the pm structure (have to make up and pass NULL version).

3) Pass the pm struct via am35xx_emac_init() and just make the default available for use. This is what I submitted since it was the most flexible.

Quoted 2 lines

Which one do you prefer or do you have something else in mind?

For now, I suggest you stick with (1) until we have a reason to make it more flexible. AFAIK, all usages of this IP in OMAP-derivative parts have this same limitation.

Quoted 179 lines · Kevin Hilman

Quoted 177 lines · Mark A. Greer

Quoted 1 line · Igor Grinberg

Hi Mark,

Hi Igor.

Quoted 144 lines

Thanks for the great work!

Quoted 141 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs has a Davinci EMAC ethernet controller on-chip. Unfortunately, the EMAC is unable to wake the PRCM when there is network activity which leads to a hung or extremely slow system when the MPU has executed a 'wfi' instruction (because of pm_idle or CPUidle). To prevent this, add hooks to the EMAC pm_runtime suspend/resume calls so that hlt is disabled whenever the EMAC is in use.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/am35xx-emac.c     |   44 +++++++++++++++++++++++++++++----
 arch/arm/mach-omap2/am35xx-emac.h     |   16 +++++++++---
 arch/arm/mach-omap2/board-am3517evm.c |    3 ++-
 arch/arm/mach-omap2/board-cm-t3517.c  |    3 ++-
 4 files changed, 56 insertions(+), 10 deletions(-)

arch/arm/mach-omap2/am35xx-emac.c +39 -5

index 3bb5cb3..22ff968 100644
@@ -23,6 +23,37 @@
 #include "control.h"
 #include "am35xx-emac.h"
 
+/*
+ * Default pm_lats for the am35x.
+ * The net effect of using am35xx_emac_pm_lats[] is that
+ * pm_idle or CPUidle won't be called while the emac
+ * interface is open.  This is required because the
+ * EMAC can't wake up PRCM so if the MPU is executing
+ * a 'wfi' instruction (e.g., from pm_idle or CPUidle),
+ * it won't break out of it due to emac activity.
+ */
+static int am35xx_emac_deactivate_func(struct omap_device *od)
+{
+	enable_hlt();
+	return omap_device_idle_hwmods(od);
+}
+
+static int am35xx_emac_activate_func(struct omap_device *od)
+{
+	disable_hlt();
+	return omap_device_enable_hwmods(od);
+}
+
+struct omap_device_pm_latency am35xx_emac_pm_lats[] = {
+	{
+		.deactivate_func	= am35xx_emac_deactivate_func,
+		.activate_func		= am35xx_emac_activate_func,
+		.flags			= OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	},
+};
+
+int am35xx_emac_pm_lats_size = ARRAY_SIZE(am35xx_emac_pm_lats);
+
 static void am35xx_enable_emac_int(void)
 {
 	u32 regval;

@@ -61,12 +92,13 @@ static struct emac_platform_data am35xx_emac_pdata = {
 static struct mdio_platform_data am35xx_mdio_pdata;
 
 static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
-		void *pdata, int pdata_len)
+		void *pdata, int pdata_len,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct platform_device *pdev;
 
 	pdev = omap_device_build(oh->class->name, 0, oh, pdata, pdata_len,
-			NULL, 0, false);
+			pm_lats, pm_lats_size, false);
 	if (IS_ERR(pdev)) {
 		WARN(1, "Can't build omap_device for %s:%s.\n",
 					oh->class->name, oh->name);

@@ -76,7 +108,8 @@ static int __init omap_davinci_emac_dev_init(struct omap_hwmod *oh,
 	return 0;
 }
 
-void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
+void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size)
 {
 	struct omap_hwmod *oh;
 	u32 regval;

@@ -91,7 +124,7 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_mdio_pdata.bus_freq = mdio_bus_freq;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_mdio_pdata,
-					 sizeof(am35xx_mdio_pdata));
+					 sizeof(am35xx_mdio_pdata), NULL, 0);
 	if (ret) {
 		pr_err("Could not build davinci_mdio hwmod device\n");
 		return;

@@ -106,7 +139,8 @@ void __init am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en)
 	am35xx_emac_pdata.rmii_en = rmii_en;
 
 	ret = omap_davinci_emac_dev_init(oh, &am35xx_emac_pdata,
-					 sizeof(am35xx_emac_pdata));
+					 sizeof(am35xx_emac_pdata),
+					 pm_lats, pm_lats_size);
 	if (ret) {
 		pr_err("Could not build davinci_emac hwmod device\n");
 		return;

arch/arm/mach-omap2/am35xx-emac.h +13 -3

index 15c6f9c..7c23808 100644
@@ -6,10 +6,20 @@
  * published by the Free Software Foundation.
  */
 
+#include <plat/omap_device.h>
+
 #define AM35XX_DEFAULT_MDIO_FREQUENCY	1000000
 
-#if defined(CONFIG_TI_DAVINCI_EMAC) || defined(CONFIG_TI_DAVINCI_EMAC_MODULE)
-void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en);
+#if IS_ENABLED(CONFIG_TI_DAVINCI_EMAC)
+extern struct omap_device_pm_latency am35xx_emac_pm_lats[];
+extern int am35xx_emac_pm_lats_size;
+
+void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size);
 #else
-static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en) {}
+#define am35xx_emac_pm_lats		NULL
+#define am35xx_emac_pm_lats_size	0
+
+static inline void am35xx_emac_init(unsigned long mdio_bus_freq, u8 rmii_en,
+		struct omap_device_pm_latency *pm_lats, int pm_lats_size) {}
 #endif

arch/arm/mach-omap2/board-am3517evm.c +0 -0

index 3645285..fe3a304 100644
@@ -385,7 +385,8 @@ static void __init am3517_evm_init(void)
 	i2c_register_board_info(1, am3517evm_i2c1_boardinfo,
 				ARRAY_SIZE(am3517evm_i2c1_boardinfo));

Quoted 8 lines

arch/arm/mach-omap2/board-am3517evm.c static void __init am3517_evm_init(void) · line 389

Quoted 4 lines

arch/arm/mach-omap2/board-am3517evm.c +2 -1

 	/*Ethernet*/
-	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1);
+	am35xx_emac_init(AM35XX_DEFAULT_MDIO_FREQUENCY, 1,
+			 am35xx_emac_pm_lats, am35xx_emac_pm_lats_size);

Why is it essential for board file to pass these pm structures? Is it something board specific?
Can't you just use them inside the am35xx-emac.c file?

Great question. I went back & forth on this mayself (I have 3 different versions sitting in my repo :). I ended up passing the pm structures in case there is a variant that comes along that doesn't need this fixup (or a board that wants to do this plus additional things).

The variants that I have are:

1) Leave am35xx_emac_init() interface the way it is and automatically add the necessary things. The problem is lack of flexibility if & when some other variant comes along.

2) Add pm_lats & pm_lats_size to am35xx_emac_init() but if they're NULL or 0, then use the defaults that are the same as what's in this patch. More flexible but doesn't easily allow a board file to NULL out the pm structure (have to make up and pass NULL version).

3) Pass the pm struct via am35xx_emac_init() and just make the default available for use. This is what I submitted since it was the most flexible.

Quoted 6 lines

Quoted 2 lines

Which one do you prefer or do you have something else in mind?

For now, I suggest you stick with (1) until we have a reason to make it more flexible. AFAIK, all usages of this IP in OMAP-derivative parts have this same limitation.

I agree with Kevin - (1) should be used.

There is no need to complicate things in favor of something that might or might not exist some day, because even if it will exist some day, we probably anyway will see a better solution to this and will change the existing one.

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs has a Davinci EMAC ethernet controller on-chip. Unfortunately, the EMAC is unable to wake the PRCM when there is network activity which leads to a hung or extremely slow system when the MPU has executed a 'wfi' instruction (because of pm_idle or CPUidle). To prevent this, add hooks to the EMAC pm_runtime suspend/resume calls so that hlt is disabled whenever the EMAC is in use.

Signed-off-by Mark A. Greer <[email protected]>

arch/arm/mach-omap2/am35xx-emac.c +20 -0

@@ -23,6 +23,37 @@
 #include "control.h"
 #include "am35xx-emac.h"
 
+/*
+ * Default pm_lats for the am35x.
+ * The net effect of using am35xx_emac_pm_lats[] is that
+ * pm_idle or CPUidle won't be called while the emac
+ * interface is open.  This is required because the
+ * EMAC can't wake up PRCM so if the MPU is executing
+ * a 'wfi' instruction (e.g., from pm_idle or CPUidle),
+ * it won't break out of it due to emac activity.
+ */
+static int am35xx_emac_deactivate_func(struct omap_device *od)
+{
+	enable_hlt();
+	return omap_device_idle_hwmods(od);
+}
+
+static int am35xx_emac_activate_func(struct omap_device *od)
+{
+	disable_hlt();
+	return omap_device_enable_hwmods(od);
+}
 

From the patch description, it doesn't sound like it's WFI entry that's

Hi

From the patch description, it doesn't sound like it's WFI entry that's the problem. The EMAC can assert its interrupt lines to the INTC, since the EMAC is active. If the MPU and CORE powerdomains are ON, then the ARM core should wake up out of WFI. (Unless there's some weird bug; always possible.)

Probably the MPU DPLL has to stay running for it all to work, since I think that is activated and deactivated by the PRCM. Maybe the CORE DPLL has to stay running too (but I doubt it). But I'll bet that all the clocks downstream of the DPLLs can be gated. If it works, that would save a lot of energy over the disable_hlt() approach. With disable_hlt(), the ARM & interconnect is just going to be burning power waiting for the interrupt to come in.

Want to try something like this? It's your patch but modified to not use disable/enable_hlt(). If it doesn't work in your test case, maybe try uncommenting that second set of deny_idle / allow_idle ...

Quoted 3 lines · Paul Walmsley

Want to try something like this? It's your patch but modified to not use disable/enable_hlt(). If it doesn't work in your test case, maybe try uncommenting that second set of deny_idle / allow_idle ...

I tested the modified patch (to get it to compile) below.

It did not work with or without the core_dpll_ck deny_idle/allow_idle commented out.

Quoted 7 lines · Mark A. Greer

Quoted 3 lines · Paul Walmsley

Want to try something like this? It's your patch but modified to not use disable/enable_hlt(). If it doesn't work in your test case, maybe try uncommenting that second set of deny_idle / allow_idle ...

I tested the modified patch (to get it to compile) below.

Doh, sorry about that.

Quoted 2 lines

It did not work with or without the core_dpll_ck deny_idle/allow_idle commented out.

Here's a version with some of the CPUIdle states restricted. Maybe try this one if you're using CPUIdle?

If it happens to work, it would also be interesting to know if it works with the CORE DPLL part commented out.

...and, unfortunately, it didnt' work in either case.

Quoted 1 line · Mark A. Greer

...and, unfortunately, it didnt' work in either case.

OK thanks for the tests. Is the EMAC/MDIO really active and asserting interrupts while all this is happening?

Or has that driver called
pm_runtime_put*(), and so the EMAC/MDIO isn't waking up?

+ Ilya

Hi Mark

Maybe try something like this on top of the patch that disables the MPU DPLL autoidle?

I don't know what am35xx_enable_emac_int() is supposed to do. It seems strange to clear the interrupt status bits when one is supposed to enable the interrupts. Maybe Ilya can shed some light on it.

From: "Mark A. Greer" <[email protected]>

Paul, Kevin,

These patches convert the davinci emac support for the am35x SoC to use hwmod and add enable_hlt()/disable_hlt() calls to the pm_runtime hooks for that driver.

I have converted the davinci_emac driver to use pm_runtime but I can't formally submit it yet since it requires some changes on the mach-davinci side that haven't happened yet. I will send it as an RFC in a reply to this thread.

The patches are based on:

	git://git.pwsan.com/linux-2.6 am35xx_hwmod_data_fixes_a_3.5

Mark
--

Mark A. Greer (2):
arm: omap3: am35x: Add Davinci EMAC/MDIO hwmod support
arm: omap3: am35x: Disable hlt when using Davinci EMAC

 arch/arm/mach-omap2/am35xx-emac.c          |  120 ++++++++++++++++++----------
 arch/arm/mach-omap2/am35xx-emac.h          |   16 +++-
 arch/arm/mach-omap2/board-am3517evm.c      |    3 +-
 arch/arm/mach-omap2/board-cm-t3517.c       |    3 +-
 arch/arm/mach-omap2/clock3xxx_data.c       |    2 +-
 arch/arm/mach-omap2/include/mach/am35xx.h  |    2 +
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |   85 ++++++++++++++++++++
 7 files changed, 183 insertions(+), 48 deletions(-)

+Sekhar,

Quoted 7 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Paul, Kevin,

These patches convert the davinci emac support for the am35x SoC to use hwmod and add enable_hlt()/disable_hlt() calls to the pm_runtime hooks for that driver.

Great. I didn't look closely at the hwmod data, but the approach is right here, IMO.

Quoted 4 lines

I have converted the davinci_emac driver to use pm_runtime but I can't formally submit it yet since it requires some changes on the mach-davinci side that haven't happened yet. I will send it as an RFC in a reply to this thread.

Sekhar, are you planning to add runtime PM core support to davinci?

I recommend looking at the simple OMAP1 layer that implements a basic clocks-only runtime PM layer.