[PATCH v2 0/7] omap: mailbox: hwmod support and dependent cleanup patches

[PATCH v2 0/7] omap: mailbox: hwmod support and dependent cleanup patches

[Omar Ramirez Luna]

hwmod support for omap2 and omap3 chips, plus cleanups.

Tested on 3430 and 3630

** v2 **
- Added omap4 hwmod support.
- Moved "mailbox_ick" from hwmod to hwmod_if (omap 2/3)
- Declared sysc classes for omap 2/3

** v1 **
1. omap: mailbox: initial hwmod support for omap3
Changes were made to:
- Rebase to latest code.
- Detect the hwmod by filling prcm union for omap2, without
  this it was unable to build the hwmod at runtime.
- Replace magic number for defines.
- Use ioremap again instead of relying on the one made by hwmod,
  as noted in http://patchwork.kernel.org/patch/101661/

2. omap: mailbox: initial hwmod support for omap2
Was only compiled tested!! Unfortunately I don't have the HW for it.

4. omap: mailbox: fix detection for previously supported chips
Original patch was using features framework, but when compiled as a module
it was breaking, this patch uses cpu_is_omap34xx macro instead. This is
meant as a short term solution until proper cleanup is done, as suggested in:

http://marc.info/?l=linux-arm-kernel&m=128534253231481&w=2

Benoit Cousson (1):
  OMAP4: hwmod data: add mailbox data

Felipe Contreras (3):
  OMAP3: hwmod data: add mailbox data
  omap: mailbox: initial hwmod support
  omap: mailbox: add omap_device latency information

Omar Ramirez Luna (3):
  OMAP2: hwmod data: add mailbox data
  omap: mailbox: fix detection for previously supported chips
  omap: mailbox: remove unreachable return

 arch/arm/mach-omap2/devices.c              |  104 +++++++---------------------
 arch/arm/mach-omap2/mailbox.c              |   39 +++++-----
 arch/arm/mach-omap2/omap_hwmod_2420_data.c |   67 ++++++++++++++++++
 arch/arm/mach-omap2/omap_hwmod_2430_data.c |   66 ++++++++++++++++++
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |   66 ++++++++++++++++++
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   63 +++++++++++++++++
 arch/arm/plat-omap/include/plat/mailbox.h  |    6 ++
 7 files changed, 311 insertions(+), 100 deletions(-)

[PATCH v2 1/7] OMAP2: hwmod data: add mailbox data

[Omar Ramirez Luna]

hwmod data for omap2430 and 2420.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_2420_data.c |   67 ++++++++++++++++++++++++++++
 arch/arm/mach-omap2/omap_hwmod_2430_data.c |   66 +++++++++++++++++++++++++++
 2 files changed, 133 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
index a1a3dd6..76ad991 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
@@ -557,6 +557,72 @@ static struct omap_hwmod omap2420_i2c2_hwmod = {
 	.flags		= HWMOD_16BIT_REG,
 };
 
+/* mailbox */
+
+static struct omap_hwmod_class_sysconfig omap2420_mailbox_sysc = {
+	.rev_offs	= 0x000,
+	.sysc_offs	= 0x010,
+	.syss_offs	= 0x014,
+	.sysc_flags	= (SYSC_HAS_CLOCKACTIVITY | SYSC_HAS_SIDLEMODE |
+				SYSC_HAS_SOFTRESET | SYSC_HAS_AUTOIDLE),
+	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.sysc_fields	= &omap_hwmod_sysc_type1,
+};
+
+static struct omap_hwmod_class omap2420_mailbox_hwmod_class = {
+	.name = "mailbox",
+	.sysc = &omap2420_mailbox_sysc,
+};
+
+static struct omap_hwmod omap2420_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap2420_mailbox_addrs[] = {
+	{
+		.pa_start	= OMAP24XX_MAILBOX_BASE,
+		.pa_end		= OMAP24XX_MAILBOX_BASE + SZ_4K - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l4_core -> mailbox */
+static struct omap_hwmod_ocp_if omap2420_l4_core__mailbox = {
+	.master		= &omap2420_l4_core_hwmod,
+	.slave		= &omap2420_mailbox_hwmod,
+	.addr		= omap2420_mailbox_addrs,
+	.clk		= "mailboxes_ick",
+	.addr_cnt	= ARRAY_SIZE(omap2420_mailbox_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* mailbox slave ports */
+static struct omap_hwmod_ocp_if *omap2420_mailbox_slaves[] = {
+	&omap2420_l4_core__mailbox,
+};
+
+static struct omap_hwmod_irq_info omap2420_mailbox_irqs[] = {
+	{ .name = "dsp", .irq = INT_24XX_MAIL_U0_MPU, },
+	{ .name = "iva", .irq = INT_24XX_MAIL_U3_MPU, },
+};
+
+static struct omap_hwmod omap2420_mailbox_hwmod = {
+	.name		= "mailbox",
+	.class		= &omap2420_mailbox_hwmod_class,
+	.prcm		= {
+		.omap2 = {
+			.prcm_reg_id = 1,
+			.module_bit = OMAP24XX_EN_MAILBOXES_SHIFT,
+			.module_offs = CORE_MOD,
+			.idlest_reg_id = 1,
+			.idlest_idle_bit = OMAP24XX_ST_MAILBOXES_SHIFT,
+		},
+	},
+	.mpu_irqs	= omap2420_mailbox_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap2420_mailbox_irqs),
+	.slaves		= omap2420_mailbox_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap2420_mailbox_slaves),
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP2420),
+};
+
 static __initdata struct omap_hwmod *omap2420_hwmods[] = {
 	&omap2420_l3_main_hwmod,
 	&omap2420_l4_core_hwmod,
@@ -569,6 +635,7 @@ static __initdata struct omap_hwmod *omap2420_hwmods[] = {
 	&omap2420_uart3_hwmod,
 	&omap2420_i2c1_hwmod,
 	&omap2420_i2c2_hwmod,
+	&omap2420_mailbox_hwmod,
 	NULL,
 };
 
diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
index 7cf0d3a..991bd5b 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
@@ -569,6 +569,71 @@ static struct omap_hwmod omap2430_i2c2_hwmod = {
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP2430),
 };
 
+/* mailbox */
+
+static struct omap_hwmod_class_sysconfig omap2430_mailbox_sysc = {
+	.rev_offs	= 0x000,
+	.sysc_offs	= 0x010,
+	.syss_offs	= 0x014,
+	.sysc_flags	= (SYSC_HAS_CLOCKACTIVITY | SYSC_HAS_SIDLEMODE |
+				SYSC_HAS_SOFTRESET | SYSC_HAS_AUTOIDLE),
+	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.sysc_fields	= &omap_hwmod_sysc_type1,
+};
+
+static struct omap_hwmod_class omap2430_mailbox_hwmod_class = {
+	.name = "mailbox",
+	.sysc = &omap2430_mailbox_sysc,
+};
+
+static struct omap_hwmod omap2430_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap2430_mailbox_addrs[] = {
+	{
+		.pa_start	= OMAP24XX_MAILBOX_BASE,
+		.pa_end		= OMAP24XX_MAILBOX_BASE + SZ_4K - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l4_core -> mailbox */
+static struct omap_hwmod_ocp_if omap2430_l4_core__mailbox = {
+	.master		= &omap2430_l4_core_hwmod,
+	.slave		= &omap2430_mailbox_hwmod,
+	.addr		= omap2430_mailbox_addrs,
+	.clk		= "mailboxes_ick",
+	.addr_cnt	= ARRAY_SIZE(omap2430_mailbox_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* mailbox slave ports */
+static struct omap_hwmod_ocp_if *omap2430_mailbox_slaves[] = {
+	&omap2430_l4_core__mailbox,
+};
+
+static struct omap_hwmod_irq_info omap2430_mailbox_irqs[] = {
+	{ .name = "dsp", .irq = INT_24XX_MAIL_U0_MPU, },
+};
+
+static struct omap_hwmod omap2430_mailbox_hwmod = {
+	.name		= "mailbox",
+	.class		= &omap2430_mailbox_hwmod_class,
+	.prcm		= {
+		.omap2 = {
+			.prcm_reg_id = 1,
+			.module_bit = OMAP24XX_EN_MAILBOXES_SHIFT,
+			.module_offs = CORE_MOD,
+			.idlest_reg_id = 1,
+			.idlest_idle_bit = OMAP24XX_ST_MAILBOXES_SHIFT,
+		},
+	},
+	.mpu_irqs	= omap2430_mailbox_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap2430_mailbox_irqs),
+	.slaves		= omap2430_mailbox_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap2430_mailbox_slaves),
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP2430),
+};
+
 static __initdata struct omap_hwmod *omap2430_hwmods[] = {
 	&omap2430_l3_main_hwmod,
 	&omap2430_l4_core_hwmod,
@@ -581,6 +646,7 @@ static __initdata struct omap_hwmod *omap2430_hwmods[] = {
 	&omap2430_uart3_hwmod,
 	&omap2430_i2c1_hwmod,
 	&omap2430_i2c2_hwmod,
+	&omap2430_mailbox_hwmod,
 	NULL,
 };
 
-- 
1.7.1

[PATCH v2 2/7] OMAP3: hwmod data: add mailbox data

[Omar Ramirez Luna]

From: Felipe Contreras <felipe.contreras@gmail.com>

hwmod data for omap3 mailbox.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |   66 ++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index ed6bf4a..60d977e 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -736,6 +736,71 @@ static struct omap_hwmod omap3xxx_i2c3_hwmod = {
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
 };
 
+/* mailbox */
+
+static struct omap_hwmod_class_sysconfig omap3xxx_mailbox_sysc = {
+	.rev_offs	= 0x000,
+	.sysc_offs	= 0x010,
+	.syss_offs	= 0x014,
+	.sysc_flags	= (SYSC_HAS_CLOCKACTIVITY | SYSC_HAS_SIDLEMODE |
+				SYSC_HAS_SOFTRESET | SYSC_HAS_AUTOIDLE),
+	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.sysc_fields	= &omap_hwmod_sysc_type1,
+};
+
+static struct omap_hwmod_class omap3xxx_mailbox_hwmod_class = {
+	.name = "mailbox",
+	.sysc = &omap3xxx_mailbox_sysc,
+};
+
+static struct omap_hwmod omap3xxx_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap3xxx_mailbox_addrs[] = {
+	{
+		.pa_start	= OMAP34XX_MAILBOX_BASE,
+		.pa_end		= OMAP34XX_MAILBOX_BASE + SZ_4K - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l4_core -> mailbox */
+static struct omap_hwmod_ocp_if omap3xxx_l4_core__mailbox = {
+	.master		= &omap3xxx_l4_core_hwmod,
+	.slave		= &omap3xxx_mailbox_hwmod,
+	.addr		= omap3xxx_mailbox_addrs,
+	.clk		= "mailboxes_ick",
+	.addr_cnt	= ARRAY_SIZE(omap3xxx_mailbox_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* mailbox slave ports */
+static struct omap_hwmod_ocp_if *omap3xxx_mailbox_slaves[] = {
+	&omap3xxx_l4_core__mailbox,
+};
+
+static struct omap_hwmod_irq_info omap3xxx_mailbox_irqs[] = {
+	{ .name = "dsp", .irq = INT_24XX_MAIL_U0_MPU, },
+};
+
+static struct omap_hwmod omap3xxx_mailbox_hwmod = {
+	.name		= "mailbox",
+	.class		= &omap3xxx_mailbox_hwmod_class,
+	.prcm		= {
+		.omap2 = {
+			.prcm_reg_id = 1,
+			.module_bit = OMAP3430_EN_MAILBOXES_SHIFT,
+			.module_offs = CORE_MOD,
+			.idlest_reg_id = 1,
+			.idlest_idle_bit = OMAP3430_ST_MAILBOXES_SHIFT,
+		},
+	},
+	.mpu_irqs	= omap3xxx_mailbox_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap3xxx_mailbox_irqs),
+	.slaves		= omap3xxx_mailbox_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap3xxx_mailbox_slaves),
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+};
+
 static __initdata struct omap_hwmod *omap3xxx_hwmods[] = {
 	&omap3xxx_l3_main_hwmod,
 	&omap3xxx_l4_core_hwmod,
@@ -751,6 +816,7 @@ static __initdata struct omap_hwmod *omap3xxx_hwmods[] = {
 	&omap3xxx_i2c1_hwmod,
 	&omap3xxx_i2c2_hwmod,
 	&omap3xxx_i2c3_hwmod,
+	&omap3xxx_mailbox_hwmod,
 	NULL,
 };
 
-- 
1.7.1

[PATCH v2 3/7] OMAP4: hwmod data: add mailbox data

[Omar Ramirez Luna]

From: Benoit Cousson <b-cousson@ti.com>

hwmod data for omap4 mailbox.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   63 ++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 0d5c6eb..f7525e3 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -1043,6 +1043,66 @@ static struct omap_hwmod omap44xx_uart4_hwmod = {
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
 };
 
+/* mailbox */
+
+static struct omap_hwmod_class_sysconfig omap44xx_mailbox_sysc = {
+	.rev_offs	= 0x0000,
+	.sysc_offs	= 0x0010,
+	.sysc_flags	= (SYSC_HAS_RESET_STATUS | SYSC_HAS_SIDLEMODE |
+			   SYSC_HAS_SOFTRESET),
+	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.sysc_fields	= &omap_hwmod_sysc_type2,
+};
+
+static struct omap_hwmod_class omap44xx_mailbox_hwmod_class = {
+	.name = "mailbox",
+	.sysc = &omap44xx_mailbox_sysc,
+};
+
+static struct omap_hwmod omap44xx_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
+	{
+		.pa_start	= OMAP44XX_MAILBOX_BASE,
+		.pa_end		= OMAP44XX_MAILBOX_BASE + SZ_4K - 1,
+		.flags		= ADDR_TYPE_RT,
+	},
+};
+
+/* l4_cfg -> mailbox */
+static struct omap_hwmod_ocp_if omap44xx_l4_cfg__mailbox = {
+	.master		= &omap44xx_l4_cfg_hwmod,
+	.slave		= &omap44xx_mailbox_hwmod,
+	.clk		= "l4_div_ck",
+	.addr		= omap44xx_mailbox_addrs,
+	.addr_cnt	= ARRAY_SIZE(omap44xx_mailbox_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* mailbox slave ports */
+static struct omap_hwmod_ocp_if *omap44xx_mailbox_slaves[] = {
+	&omap44xx_l4_cfg__mailbox,
+};
+
+static struct omap_hwmod_irq_info omap44xx_mailbox_irqs[] = {
+	{ .name = "mbox", .irq = 26 + OMAP44XX_IRQ_GIC_START, },
+};
+
+static struct omap_hwmod omap44xx_mailbox_hwmod = {
+	.name		= "mailbox",
+	.class		= &omap44xx_mailbox_hwmod_class,
+	.prcm		= {
+		.omap4 = {
+			.clkctrl_reg = OMAP4430_CM_L4CFG_MAILBOX_CLKCTRL,
+		},
+	},
+	.mpu_irqs	= omap44xx_mailbox_irqs,
+	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_mailbox_irqs),
+	.slaves		= omap44xx_mailbox_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap44xx_mailbox_slaves),
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
+};
+
 static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
 	/* dmm class */
 	&omap44xx_dmm_hwmod,
@@ -1077,6 +1137,9 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
 	&omap44xx_uart2_hwmod,
 	&omap44xx_uart3_hwmod,
 	&omap44xx_uart4_hwmod,
+
+	/* mailbox */
+	&omap44xx_mailbox_hwmod,
 	NULL,
 };
 
-- 
1.7.1

[PATCH v2 4/7] omap: mailbox: initial hwmod support

[Omar Ramirez Luna]

From: Felipe Contreras <felipe.contreras@gmail.com>

hwmod support for omap mailbox module.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/devices.c |  100 ++++++----------------------------------
 1 files changed, 15 insertions(+), 85 deletions(-)

diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index eaf3799..d977572 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -141,95 +141,25 @@ static inline void omap_init_camera(void)
 #endif
 
 #if defined(CONFIG_OMAP_MBOX_FWK) || defined(CONFIG_OMAP_MBOX_FWK_MODULE)
-
-#define MBOX_REG_SIZE   0x120
-
-#ifdef CONFIG_ARCH_OMAP2
-static struct resource omap2_mbox_resources[] = {
-	{
-		.start		= OMAP24XX_MAILBOX_BASE,
-		.end		= OMAP24XX_MAILBOX_BASE + MBOX_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= INT_24XX_MAIL_U0_MPU,
-		.flags		= IORESOURCE_IRQ,
-		.name		= "dsp",
-	},
-	{
-		.start		= INT_24XX_MAIL_U3_MPU,
-		.flags		= IORESOURCE_IRQ,
-		.name		= "iva",
-	},
-};
-static int omap2_mbox_resources_sz = ARRAY_SIZE(omap2_mbox_resources);
-#else
-#define omap2_mbox_resources		NULL
-#define omap2_mbox_resources_sz		0
-#endif
-
-#ifdef CONFIG_ARCH_OMAP3
-static struct resource omap3_mbox_resources[] = {
-	{
-		.start		= OMAP34XX_MAILBOX_BASE,
-		.end		= OMAP34XX_MAILBOX_BASE + MBOX_REG_SIZE - 1,
-		.flags		= IORESOURCE_MEM,
-	},
-	{
-		.start		= INT_24XX_MAIL_U0_MPU,
-		.flags		= IORESOURCE_IRQ,
-		.name		= "dsp",
-	},
-};
-static int omap3_mbox_resources_sz = ARRAY_SIZE(omap3_mbox_resources);
-#else
-#define omap3_mbox_resources		NULL
-#define omap3_mbox_resources_sz		0
-#endif
-
-#ifdef CONFIG_ARCH_OMAP4
-
-#define OMAP4_MBOX_REG_SIZE	0x130
-static struct resource omap4_mbox_resources[] = {
-	{
-		.start          = OMAP44XX_MAILBOX_BASE,
-		.end            = OMAP44XX_MAILBOX_BASE +
-					OMAP4_MBOX_REG_SIZE - 1,
-		.flags          = IORESOURCE_MEM,
-	},
-	{
-		.start          = OMAP44XX_IRQ_MAIL_U0,
-		.flags          = IORESOURCE_IRQ,
-		.name		= "mbox",
-	},
-};
-static int omap4_mbox_resources_sz = ARRAY_SIZE(omap4_mbox_resources);
-#else
-#define omap4_mbox_resources		NULL
-#define omap4_mbox_resources_sz		0
-#endif
-
-static struct platform_device mbox_device = {
-	.name		= "omap-mailbox",
-	.id		= -1,
-};
-
 static inline void omap_init_mbox(void)
 {
-	if (cpu_is_omap24xx()) {
-		mbox_device.resource = omap2_mbox_resources;
-		mbox_device.num_resources = omap2_mbox_resources_sz;
-	} else if (cpu_is_omap34xx()) {
-		mbox_device.resource = omap3_mbox_resources;
-		mbox_device.num_resources = omap3_mbox_resources_sz;
-	} else if (cpu_is_omap44xx()) {
-		mbox_device.resource = omap4_mbox_resources;
-		mbox_device.num_resources = omap4_mbox_resources_sz;
-	} else {
-		pr_err("%s: platform not supported\n", __func__);
+	struct omap_hwmod *oh;
+	struct omap_device *od;
+
+	oh = omap_hwmod_lookup("mailbox");
+	if (!oh) {
+		pr_err("%s: unable to find hwmod\n", __func__);
+		return;
+	}
+
+	od = omap_device_build("omap-mailbox", -1, oh,
+			NULL, 0,
+			NULL, 0,
+			0);
+	if (!od) {
+		pr_err("%s: could not build device\n", __func__);
 		return;
 	}
-	platform_device_register(&mbox_device);
 }
 #else
 static inline void omap_init_mbox(void) { }
-- 
1.7.1

[PATCH v2 5/7] omap: mailbox: add omap_device latency information

[Omar Ramirez Luna]

From: Felipe Contreras <felipe.contreras@gmail.com>

So that we can enable the main clock.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/devices.c             |   18 ++++++++++++++++--
 arch/arm/mach-omap2/mailbox.c             |   21 +++++++++------------
 arch/arm/plat-omap/include/plat/mailbox.h |    6 ++++++
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index d977572..dafc23a 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -29,6 +29,7 @@
 #include <plat/dma.h>
 #include <plat/omap_hwmod.h>
 #include <plat/omap_device.h>
+#include <plat/mailbox.h>
 
 #include "mux.h"
 #include "control.h"
@@ -141,10 +142,19 @@ static inline void omap_init_camera(void)
 #endif
 
 #if defined(CONFIG_OMAP_MBOX_FWK) || defined(CONFIG_OMAP_MBOX_FWK_MODULE)
+static struct omap_device_pm_latency mbox_latencies[] = {
+	[0] = {
+		.activate_func = omap_device_enable_clocks,
+		.deactivate_func = omap_device_enable_clocks,
+		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	},
+};
+
 static inline void omap_init_mbox(void)
 {
 	struct omap_hwmod *oh;
 	struct omap_device *od;
+	struct omap_mbox_platform_data pdata;
 
 	oh = omap_hwmod_lookup("mailbox");
 	if (!oh) {
@@ -152,10 +162,14 @@ static inline void omap_init_mbox(void)
 		return;
 	}
 
+	pdata.device_enable = omap_device_enable;
+	pdata.device_disable = omap_device_idle;
+
 	od = omap_device_build("omap-mailbox", -1, oh,
-			NULL, 0,
-			NULL, 0,
+			&pdata, sizeof(pdata),
+			mbox_latencies, ARRAY_SIZE(mbox_latencies),
 			0);
+
 	if (!od) {
 		pr_err("%s: could not build device\n", __func__);
 		return;
diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 40ddeca..bf598a3 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -52,6 +52,7 @@
 #define OMAP4_MBOX_NR_REGS		(OMAP4_MBOX_REG_SIZE / sizeof(u32))
 
 static void __iomem *mbox_base;
+static struct platform_device *mbox_pdev;
 
 struct omap_mbox2_fifo {
 	unsigned long msg;
@@ -70,8 +71,6 @@ struct omap_mbox2_priv {
 	unsigned long irqdisable;
 };
 
-static struct clk *mbox_ick_handle;
-
 static void omap2_mbox_enable_irq(struct omap_mbox *mbox,
 				  omap_mbox_type_t irq);
 
@@ -90,14 +89,10 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
 {
 	u32 l;
 	unsigned long timeout;
+	struct omap_mbox_platform_data *pdata = mbox_pdev->dev.platform_data;
 
-	mbox_ick_handle = clk_get(NULL, "mailboxes_ick");
-	if (IS_ERR(mbox_ick_handle)) {
-		printk(KERN_ERR "Could not get mailboxes_ick: %ld\n",
-			PTR_ERR(mbox_ick_handle));
-		return PTR_ERR(mbox_ick_handle);
-	}
-	clk_enable(mbox_ick_handle);
+	if (pdata->device_enable)
+		pdata->device_enable(mbox_pdev);
 
 	if (cpu_is_omap44xx()) {
 		mbox_write_reg(OMAP4_SOFTRESET, MAILBOX_SYSCONFIG);
@@ -143,9 +138,9 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
 
 static void omap2_mbox_shutdown(struct omap_mbox *mbox)
 {
-	clk_disable(mbox_ick_handle);
-	clk_put(mbox_ick_handle);
-	mbox_ick_handle = NULL;
+	struct omap_mbox_platform_data *pdata = mbox_pdev->dev.platform_data;
+	if (pdata->device_disable)
+		pdata->device_disable(mbox_pdev);
 }
 
 /* Mailbox FIFO handle functions */
@@ -427,6 +422,8 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 	if (!mbox_base)
 		return -ENOMEM;
 
+	mbox_pdev = pdev;
+
 	ret = omap_mbox_register(&pdev->dev, list);
 	if (ret) {
 		iounmap(mbox_base);
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 9976565..59443b1 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -11,6 +11,7 @@
 
 typedef u32 mbox_msg_t;
 struct omap_mbox;
+struct platform_device;
 
 typedef int __bitwise omap_mbox_irq_t;
 #define IRQ_TX ((__force omap_mbox_irq_t) 1)
@@ -59,6 +60,11 @@ struct omap_mbox {
 	void			*priv;
 };
 
+struct omap_mbox_platform_data {
+	int (*device_enable)(struct platform_device *pdev);
+	int (*device_disable)(struct platform_device *pdev);
+};
+
 int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
 void omap_mbox_init_seq(struct omap_mbox *);
 
-- 
1.7.1

[PATCH v2 6/7] omap: mailbox: fix detection for previously supported chips

[Omar Ramirez Luna]

Fix the mailbox detection for OMAP3630 and 2430, also minor
cleanup on conditional ifdef's that could affect it.

Given that 2430 has an iva too include it, as the same steps
for omap3 apply.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/mailbox.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index bf598a3..5e9ea0f 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -276,7 +276,7 @@ static struct omap_mbox_ops omap2_mbox_ops = {
 
 /* FIXME: the following structs should be filled automatically by the user id */
 
-#if defined(CONFIG_ARCH_OMAP3430) || defined(CONFIG_ARCH_OMAP2420)
+#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP2)
 /* DSP */
 static struct omap_mbox2_priv omap2_mbox_dsp_priv = {
 	.tx_fifo = {
@@ -301,7 +301,7 @@ struct omap_mbox mbox_dsp_info = {
 };
 #endif
 
-#if defined(CONFIG_ARCH_OMAP3430)
+#if defined(CONFIG_ARCH_OMAP3)
 struct omap_mbox *omap3_mboxes[] = { &mbox_dsp_info, NULL };
 #endif
 
@@ -389,15 +389,19 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 
 	if (false)
 		;
-#if defined(CONFIG_ARCH_OMAP3430)
-	else if (cpu_is_omap3430()) {
+#if defined(CONFIG_ARCH_OMAP3)
+	else if (cpu_is_omap34xx()) {
 		list = omap3_mboxes;
 
 		list[0]->irq = platform_get_irq_byname(pdev, "dsp");
 	}
 #endif
-#if defined(CONFIG_ARCH_OMAP2420)
-	else if (cpu_is_omap2420()) {
+#if defined(CONFIG_ARCH_OMAP2)
+	else if (cpu_is_omap2430()) {
+		list = omap2_mboxes;
+
+		list[0]->irq = platform_get_irq_byname(pdev, "dsp");
+	} else if (cpu_is_omap2420()) {
 		list = omap2_mboxes;
 
 		list[0]->irq = platform_get_irq_byname(pdev, "dsp");
-- 
1.7.1

[PATCH v2 7/7] omap: mailbox: remove unreachable return

[Omar Ramirez Luna]

Remove unreachable return statement.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/mailbox.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 5e9ea0f..335eb2b 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -434,8 +434,6 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 		return ret;
 	}
 	return 0;
-
-	return ret;
 }
 
 static int __devexit omap2_mbox_remove(struct platform_device *pdev)
-- 
1.7.1

[PATCH v2 1/7] OMAP2: hwmod data: add mailbox data

[Cousson, Benoit]

Hi Omar,

On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> hwmod data for omap2430 and 2420.
>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod_2420_data.c |   67 ++++++++++++++++++++++++++++
>   arch/arm/mach-omap2/omap_hwmod_2430_data.c |   66 +++++++++++++++++++++++++++
>   2 files changed, 133 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
> index a1a3dd6..76ad991 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
> @@ -557,6 +557,72 @@ static struct omap_hwmod omap2420_i2c2_hwmod = {
>   	.flags		= HWMOD_16BIT_REG,
>   };
>
> +/* mailbox */
> +
> +static struct omap_hwmod_class_sysconfig omap2420_mailbox_sysc = {
> +	.rev_offs	= 0x000,
> +	.sysc_offs	= 0x010,
> +	.syss_offs	= 0x014,
> +	.sysc_flags	= (SYSC_HAS_CLOCKACTIVITY | SYSC_HAS_SIDLEMODE |
> +				SYSC_HAS_SOFTRESET | SYSC_HAS_AUTOIDLE),
> +	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
> +	.sysc_fields	=&omap_hwmod_sysc_type1,
> +};
> +
> +static struct omap_hwmod_class omap2420_mailbox_hwmod_class = {
> +	.name = "mailbox",
> +	.sysc =&omap2420_mailbox_sysc,
> +};
> +
> +static struct omap_hwmod omap2420_mailbox_hwmod;
> +
> +static struct omap_hwmod_addr_space omap2420_mailbox_addrs[] = {
> +	{
> +		.pa_start	= OMAP24XX_MAILBOX_BASE,
> +		.pa_end		= OMAP24XX_MAILBOX_BASE + SZ_4K - 1,
> +		.flags		= ADDR_TYPE_RT,
> +	},
> +};
> +
> +/* l4_core ->  mailbox */
> +static struct omap_hwmod_ocp_if omap2420_l4_core__mailbox = {
> +	.master		=&omap2420_l4_core_hwmod,
> +	.slave		=&omap2420_mailbox_hwmod,
> +	.addr		= omap2420_mailbox_addrs,
> +	.clk		= "mailboxes_ick",

Could you try to be consistent with the omap4 format? it is easier to 
diff between Soc version: .clk is before .addr.

> +	.addr_cnt	= ARRAY_SIZE(omap2420_mailbox_addrs),
> +	.user		= OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> +/* mailbox slave ports */
> +static struct omap_hwmod_ocp_if *omap2420_mailbox_slaves[] = {
> +	&omap2420_l4_core__mailbox,
> +};
> +
> +static struct omap_hwmod_irq_info omap2420_mailbox_irqs[] = {
> +	{ .name = "dsp", .irq = INT_24XX_MAIL_U0_MPU, },
> +	{ .name = "iva", .irq = INT_24XX_MAIL_U3_MPU, },
> +};
> +
> +static struct omap_hwmod omap2420_mailbox_hwmod = {
> +	.name		= "mailbox",
> +	.class		=&omap2420_mailbox_hwmod_class,
> +	.prcm		= {
> +		.omap2 = {
> +			.prcm_reg_id = 1,
> +			.module_bit = OMAP24XX_EN_MAILBOXES_SHIFT,
> +			.module_offs = CORE_MOD,
> +			.idlest_reg_id = 1,
> +			.idlest_idle_bit = OMAP24XX_ST_MAILBOXES_SHIFT,
> +		},
> +	},

Same things with the format, put that after irqs_cnt and before slaves.

> +	.mpu_irqs	= omap2420_mailbox_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap2420_mailbox_irqs),
> +	.slaves		= omap2420_mailbox_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap2420_mailbox_slaves),
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP2420),
> +};
> +
>   static __initdata struct omap_hwmod *omap2420_hwmods[] = {
>   	&omap2420_l3_main_hwmod,
>   	&omap2420_l4_core_hwmod,
> @@ -569,6 +635,7 @@ static __initdata struct omap_hwmod *omap2420_hwmods[] = {
>   	&omap2420_uart3_hwmod,
>   	&omap2420_i2c1_hwmod,
>   	&omap2420_i2c2_hwmod,

Add a blank line between each class. OK it was not done for i2c, but it 
should.

All the comments are applicable to the 2430 and 3430 data as well.

Thanks,
Benoit

> +	&omap2420_mailbox_hwmod,
>   	NULL,
>   };
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> index 7cf0d3a..991bd5b 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> @@ -569,6 +569,71 @@ static struct omap_hwmod omap2430_i2c2_hwmod = {
>   	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP2430),
>   };
>
> +/* mailbox */
> +
> +static struct omap_hwmod_class_sysconfig omap2430_mailbox_sysc = {
> +	.rev_offs	= 0x000,
> +	.sysc_offs	= 0x010,
> +	.syss_offs	= 0x014,
> +	.sysc_flags	= (SYSC_HAS_CLOCKACTIVITY | SYSC_HAS_SIDLEMODE |
> +				SYSC_HAS_SOFTRESET | SYSC_HAS_AUTOIDLE),
> +	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
> +	.sysc_fields	=&omap_hwmod_sysc_type1,
> +};
> +
> +static struct omap_hwmod_class omap2430_mailbox_hwmod_class = {
> +	.name = "mailbox",
> +	.sysc =&omap2430_mailbox_sysc,
> +};
> +
> +static struct omap_hwmod omap2430_mailbox_hwmod;
> +
> +static struct omap_hwmod_addr_space omap2430_mailbox_addrs[] = {
> +	{
> +		.pa_start	= OMAP24XX_MAILBOX_BASE,
> +		.pa_end		= OMAP24XX_MAILBOX_BASE + SZ_4K - 1,
> +		.flags		= ADDR_TYPE_RT,
> +	},
> +};
> +
> +/* l4_core ->  mailbox */
> +static struct omap_hwmod_ocp_if omap2430_l4_core__mailbox = {
> +	.master		=&omap2430_l4_core_hwmod,
> +	.slave		=&omap2430_mailbox_hwmod,
> +	.addr		= omap2430_mailbox_addrs,
> +	.clk		= "mailboxes_ick",
> +	.addr_cnt	= ARRAY_SIZE(omap2430_mailbox_addrs),
> +	.user		= OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> +/* mailbox slave ports */
> +static struct omap_hwmod_ocp_if *omap2430_mailbox_slaves[] = {
> +	&omap2430_l4_core__mailbox,
> +};
> +
> +static struct omap_hwmod_irq_info omap2430_mailbox_irqs[] = {
> +	{ .name = "dsp", .irq = INT_24XX_MAIL_U0_MPU, },
> +};
> +
> +static struct omap_hwmod omap2430_mailbox_hwmod = {
> +	.name		= "mailbox",
> +	.class		=&omap2430_mailbox_hwmod_class,
> +	.prcm		= {
> +		.omap2 = {
> +			.prcm_reg_id = 1,
> +			.module_bit = OMAP24XX_EN_MAILBOXES_SHIFT,
> +			.module_offs = CORE_MOD,
> +			.idlest_reg_id = 1,
> +			.idlest_idle_bit = OMAP24XX_ST_MAILBOXES_SHIFT,
> +		},
> +	},
> +	.mpu_irqs	= omap2430_mailbox_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap2430_mailbox_irqs),
> +	.slaves		= omap2430_mailbox_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap2430_mailbox_slaves),
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP2430),
> +};
> +
>   static __initdata struct omap_hwmod *omap2430_hwmods[] = {
>   	&omap2430_l3_main_hwmod,
>   	&omap2430_l4_core_hwmod,
> @@ -581,6 +646,7 @@ static __initdata struct omap_hwmod *omap2430_hwmods[] = {
>   	&omap2430_uart3_hwmod,
>   	&omap2430_i2c1_hwmod,
>   	&omap2430_i2c2_hwmod,
> +	&omap2430_mailbox_hwmod,
>   	NULL,
>   };
>

[PATCH v2 3/7] OMAP4: hwmod data: add mailbox data

[Cousson, Benoit]

I don't know why, but this patch has nothing to do with my original one.
Can you stick to the original code?
Of course, if you have valid comments or need to add extra data, you 
can, but in this case, I do not see any needed change.

On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> From: Benoit Cousson<b-cousson@ti.com>
>
> hwmod data for omap4 mailbox.
>
> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   63 ++++++++++++++++++++++++++++
>   1 files changed, 63 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index 0d5c6eb..f7525e3 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -1043,6 +1043,66 @@ static struct omap_hwmod omap44xx_uart4_hwmod = {
>   	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
>   };
>
> +/* mailbox */

The original comment is missing.

> +
> +static struct omap_hwmod_class_sysconfig omap44xx_mailbox_sysc = {
> +	.rev_offs	= 0x0000,
> +	.sysc_offs	= 0x0010,
> +	.sysc_flags	= (SYSC_HAS_RESET_STATUS | SYSC_HAS_SIDLEMODE |
> +			   SYSC_HAS_SOFTRESET),
> +	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
> +	.sysc_fields	=&omap_hwmod_sysc_type2,
> +};
> +
> +static struct omap_hwmod_class omap44xx_mailbox_hwmod_class = {
> +	.name = "mailbox",
> +	.sysc =&omap44xx_mailbox_sysc,
> +};
> +
> +static struct omap_hwmod omap44xx_mailbox_hwmod;
> +
> +static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
> +	{
> +		.pa_start	= OMAP44XX_MAILBOX_BASE,

If that physical address is not used elsewhere, and it should be the 
case, there is no need to create a define for it. That's why the 
physical address was directly used here.
There is no added value to create a define for that.

> +		.pa_end		= OMAP44XX_MAILBOX_BASE + SZ_4K - 1,
> +		.flags		= ADDR_TYPE_RT,
> +	},
> +};
> +
> +/* l4_cfg ->  mailbox */
> +static struct omap_hwmod_ocp_if omap44xx_l4_cfg__mailbox = {
> +	.master		=&omap44xx_l4_cfg_hwmod,
> +	.slave		=&omap44xx_mailbox_hwmod,
> +	.clk		= "l4_div_ck",
> +	.addr		= omap44xx_mailbox_addrs,
> +	.addr_cnt	= ARRAY_SIZE(omap44xx_mailbox_addrs),
> +	.user		= OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> +/* mailbox slave ports */
> +static struct omap_hwmod_ocp_if *omap44xx_mailbox_slaves[] = {
> +	&omap44xx_l4_cfg__mailbox,
> +};
> +
> +static struct omap_hwmod_irq_info omap44xx_mailbox_irqs[] = {
> +	{ .name = "mbox", .irq = 26 + OMAP44XX_IRQ_GIC_START, },
> +};
> +
> +static struct omap_hwmod omap44xx_mailbox_hwmod = {
> +	.name		= "mailbox",
> +	.class		=&omap44xx_mailbox_hwmod_class,
> +	.prcm		= {
> +		.omap4 = {
> +			.clkctrl_reg = OMAP4430_CM_L4CFG_MAILBOX_CLKCTRL,
> +		},
> +	},
> +	.mpu_irqs	= omap44xx_mailbox_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_mailbox_irqs),

The order is different than the previous one.

Benoit

> +	.slaves		= omap44xx_mailbox_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap44xx_mailbox_slaves),
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> +};
> +
>   static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
>   	/* dmm class */
>   	&omap44xx_dmm_hwmod,
> @@ -1077,6 +1137,9 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
>   	&omap44xx_uart2_hwmod,
>   	&omap44xx_uart3_hwmod,
>   	&omap44xx_uart4_hwmod,
> +
> +	/* mailbox */
> +	&omap44xx_mailbox_hwmod,
>   	NULL,
>   };
>

[PATCH v2 4/7] omap: mailbox: initial hwmod support

[Cousson, Benoit]

Hi Felipe,

On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> From: Felipe Contreras<felipe.contreras@gmail.com>
>
> hwmod support for omap mailbox module.
>
> Signed-off-by: Felipe Contreras<felipe.contreras@gmail.com>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/devices.c |  100 ++++++----------------------------------
>   1 files changed, 15 insertions(+), 85 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> index eaf3799..d977572 100644
> --- a/arch/arm/mach-omap2/devices.c
> +++ b/arch/arm/mach-omap2/devices.c
> @@ -141,95 +141,25 @@ static inline void omap_init_camera(void)
>   #endif
>
>   #if defined(CONFIG_OMAP_MBOX_FWK) || defined(CONFIG_OMAP_MBOX_FWK_MODULE)
> -
> -#define MBOX_REG_SIZE   0x120
> -
> -#ifdef CONFIG_ARCH_OMAP2
> -static struct resource omap2_mbox_resources[] = {
> -	{
> -		.start		= OMAP24XX_MAILBOX_BASE,
> -		.end		= OMAP24XX_MAILBOX_BASE + MBOX_REG_SIZE - 1,
> -		.flags		= IORESOURCE_MEM,
> -	},
> -	{
> -		.start		= INT_24XX_MAIL_U0_MPU,
> -		.flags		= IORESOURCE_IRQ,
> -		.name		= "dsp",
> -	},
> -	{
> -		.start		= INT_24XX_MAIL_U3_MPU,
> -		.flags		= IORESOURCE_IRQ,
> -		.name		= "iva",
> -	},
> -};
> -static int omap2_mbox_resources_sz = ARRAY_SIZE(omap2_mbox_resources);
> -#else
> -#define omap2_mbox_resources		NULL
> -#define omap2_mbox_resources_sz		0
> -#endif
> -
> -#ifdef CONFIG_ARCH_OMAP3
> -static struct resource omap3_mbox_resources[] = {
> -	{
> -		.start		= OMAP34XX_MAILBOX_BASE,
> -		.end		= OMAP34XX_MAILBOX_BASE + MBOX_REG_SIZE - 1,
> -		.flags		= IORESOURCE_MEM,
> -	},
> -	{
> -		.start		= INT_24XX_MAIL_U0_MPU,
> -		.flags		= IORESOURCE_IRQ,
> -		.name		= "dsp",
> -	},
> -};
> -static int omap3_mbox_resources_sz = ARRAY_SIZE(omap3_mbox_resources);
> -#else
> -#define omap3_mbox_resources		NULL
> -#define omap3_mbox_resources_sz		0
> -#endif
> -
> -#ifdef CONFIG_ARCH_OMAP4
> -
> -#define OMAP4_MBOX_REG_SIZE	0x130
> -static struct resource omap4_mbox_resources[] = {
> -	{
> -		.start          = OMAP44XX_MAILBOX_BASE,
> -		.end            = OMAP44XX_MAILBOX_BASE +
> -					OMAP4_MBOX_REG_SIZE - 1,
> -		.flags          = IORESOURCE_MEM,
> -	},
> -	{
> -		.start          = OMAP44XX_IRQ_MAIL_U0,
> -		.flags          = IORESOURCE_IRQ,
> -		.name		= "mbox",
> -	},
> -};
> -static int omap4_mbox_resources_sz = ARRAY_SIZE(omap4_mbox_resources);
> -#else
> -#define omap4_mbox_resources		NULL
> -#define omap4_mbox_resources_sz		0
> -#endif
> -
> -static struct platform_device mbox_device = {
> -	.name		= "omap-mailbox",
> -	.id		= -1,
> -};
> -
>   static inline void omap_init_mbox(void)
>   {
> -	if (cpu_is_omap24xx()) {
> -		mbox_device.resource = omap2_mbox_resources;
> -		mbox_device.num_resources = omap2_mbox_resources_sz;
> -	} else if (cpu_is_omap34xx()) {
> -		mbox_device.resource = omap3_mbox_resources;
> -		mbox_device.num_resources = omap3_mbox_resources_sz;
> -	} else if (cpu_is_omap44xx()) {
> -		mbox_device.resource = omap4_mbox_resources;
> -		mbox_device.num_resources = omap4_mbox_resources_sz;
> -	} else {
> -		pr_err("%s: platform not supported\n", __func__);
> +	struct omap_hwmod *oh;
> +	struct omap_device *od;
> +
> +	oh = omap_hwmod_lookup("mailbox");
> +	if (!oh) {
> +		pr_err("%s: unable to find hwmod\n", __func__);
> +		return;
> +	}
> +
> +	od = omap_device_build("omap-mailbox", -1, oh,

If possible, it will be better to use the "omap_" prefix in order to 
start having some consitency in the omap devices name.

Today, we have a little bit of a mess:

i2c_omap.1
omap2_mcspi.1
omap-mcbsp.1
mmci-omap-hs.0
musb_hdrc
omap-aes
omap-sham
omap_hdq.0
omap_rng
omap_wdt
omapdss


Thanks,
Benoit

> +			NULL, 0,
> +			NULL, 0,
> +			0);
> +	if (!od) {
> +		pr_err("%s: could not build device\n", __func__);
>   		return;
>   	}
> -	platform_device_register(&mbox_device);
>   }
>   #else
>   static inline void omap_init_mbox(void) { }

[PATCH v2 5/7] omap: mailbox: add omap_device latency information

[Cousson, Benoit]

On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> From: Felipe Contreras<felipe.contreras@gmail.com>
>
> So that we can enable the main clock.

Why not going directly to the runtime PM interface? It will save you 
some effort, because most of this code is useless with runtime PM.

Moreover, even without runtime PM, you should not need at all the 
SYSCONFIG stuff you still have in this driver.

   36 /* SYSCONFIG: register bit definition */
   37 #define AUTOIDLE        (1 << 0)
   38 #define SOFTRESET       (1 << 1)
   39 #define SMARTIDLE       (2 << 3)
   40 #define OMAP4_SOFTRESET (1 << 0)
   41 #define OMAP4_NOIDLE    (1 << 2)
   42 #define OMAP4_SMARTIDLE (2 << 2)

That patch seems to me the perfect place to get rid of that.

Thanks,
Benoit

>
> Signed-off-by: Felipe Contreras<felipe.contreras@gmail.com>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/devices.c             |   18 ++++++++++++++++--
>   arch/arm/mach-omap2/mailbox.c             |   21 +++++++++------------
>   arch/arm/plat-omap/include/plat/mailbox.h |    6 ++++++
>   3 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> index d977572..dafc23a 100644
> --- a/arch/arm/mach-omap2/devices.c
> +++ b/arch/arm/mach-omap2/devices.c
> @@ -29,6 +29,7 @@
>   #include<plat/dma.h>
>   #include<plat/omap_hwmod.h>
>   #include<plat/omap_device.h>
> +#include<plat/mailbox.h>
>
>   #include "mux.h"
>   #include "control.h"
> @@ -141,10 +142,19 @@ static inline void omap_init_camera(void)
>   #endif
>
>   #if defined(CONFIG_OMAP_MBOX_FWK) || defined(CONFIG_OMAP_MBOX_FWK_MODULE)
> +static struct omap_device_pm_latency mbox_latencies[] = {
> +	[0] = {
> +		.activate_func = omap_device_enable_clocks,
> +		.deactivate_func = omap_device_enable_clocks,
> +		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> +	},
> +};
> +
>   static inline void omap_init_mbox(void)
>   {
>   	struct omap_hwmod *oh;
>   	struct omap_device *od;
> +	struct omap_mbox_platform_data pdata;
>
>   	oh = omap_hwmod_lookup("mailbox");
>   	if (!oh) {
> @@ -152,10 +162,14 @@ static inline void omap_init_mbox(void)
>   		return;
>   	}
>
> +	pdata.device_enable = omap_device_enable;
> +	pdata.device_disable = omap_device_idle;
> +
>   	od = omap_device_build("omap-mailbox", -1, oh,
> -			NULL, 0,
> -			NULL, 0,
> +			&pdata, sizeof(pdata),
> +			mbox_latencies, ARRAY_SIZE(mbox_latencies),
>   			0);
> +
>   	if (!od) {
>   		pr_err("%s: could not build device\n", __func__);
>   		return;
> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
> index 40ddeca..bf598a3 100644
> --- a/arch/arm/mach-omap2/mailbox.c
> +++ b/arch/arm/mach-omap2/mailbox.c
> @@ -52,6 +52,7 @@
>   #define OMAP4_MBOX_NR_REGS		(OMAP4_MBOX_REG_SIZE / sizeof(u32))
>
>   static void __iomem *mbox_base;
> +static struct platform_device *mbox_pdev;
>
>   struct omap_mbox2_fifo {
>   	unsigned long msg;
> @@ -70,8 +71,6 @@ struct omap_mbox2_priv {
>   	unsigned long irqdisable;
>   };
>
> -static struct clk *mbox_ick_handle;
> -
>   static void omap2_mbox_enable_irq(struct omap_mbox *mbox,
>   				  omap_mbox_type_t irq);
>
> @@ -90,14 +89,10 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
>   {
>   	u32 l;
>   	unsigned long timeout;
> +	struct omap_mbox_platform_data *pdata = mbox_pdev->dev.platform_data;
>
> -	mbox_ick_handle = clk_get(NULL, "mailboxes_ick");
> -	if (IS_ERR(mbox_ick_handle)) {
> -		printk(KERN_ERR "Could not get mailboxes_ick: %ld\n",
> -			PTR_ERR(mbox_ick_handle));
> -		return PTR_ERR(mbox_ick_handle);
> -	}
> -	clk_enable(mbox_ick_handle);
> +	if (pdata->device_enable)
> +		pdata->device_enable(mbox_pdev);
>
>   	if (cpu_is_omap44xx()) {
>   		mbox_write_reg(OMAP4_SOFTRESET, MAILBOX_SYSCONFIG);
> @@ -143,9 +138,9 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
>
>   static void omap2_mbox_shutdown(struct omap_mbox *mbox)
>   {
> -	clk_disable(mbox_ick_handle);
> -	clk_put(mbox_ick_handle);
> -	mbox_ick_handle = NULL;
> +	struct omap_mbox_platform_data *pdata = mbox_pdev->dev.platform_data;
> +	if (pdata->device_disable)
> +		pdata->device_disable(mbox_pdev);
>   }
>
>   /* Mailbox FIFO handle functions */
> @@ -427,6 +422,8 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
>   	if (!mbox_base)
>   		return -ENOMEM;
>
> +	mbox_pdev = pdev;
> +
>   	ret = omap_mbox_register(&pdev->dev, list);
>   	if (ret) {
>   		iounmap(mbox_base);
> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
> index 9976565..59443b1 100644
> --- a/arch/arm/plat-omap/include/plat/mailbox.h
> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> @@ -11,6 +11,7 @@
>
>   typedef u32 mbox_msg_t;
>   struct omap_mbox;
> +struct platform_device;
>
>   typedef int __bitwise omap_mbox_irq_t;
>   #define IRQ_TX ((__force omap_mbox_irq_t) 1)
> @@ -59,6 +60,11 @@ struct omap_mbox {
>   	void			*priv;
>   };
>
> +struct omap_mbox_platform_data {
> +	int (*device_enable)(struct platform_device *pdev);
> +	int (*device_disable)(struct platform_device *pdev);
> +};
> +
>   int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
>   void omap_mbox_init_seq(struct omap_mbox *);
>

[PATCH v2 6/7] omap: mailbox: fix detection for previously supported chips

[Cousson, Benoit]

On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> Fix the mailbox detection for OMAP3630 and 2430, also minor
> cleanup on conditional ifdef's that could affect it.
>
> Given that 2430 has an iva too include it, as the same steps
> for omap3 apply.
>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/mailbox.c |   16 ++++++++++------
>   1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
> index bf598a3..5e9ea0f 100644
> --- a/arch/arm/mach-omap2/mailbox.c
> +++ b/arch/arm/mach-omap2/mailbox.c
> @@ -276,7 +276,7 @@ static struct omap_mbox_ops omap2_mbox_ops = {
>
>   /* FIXME: the following structs should be filled automatically by the user id */
>
> -#if defined(CONFIG_ARCH_OMAP3430) || defined(CONFIG_ARCH_OMAP2420)
> +#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP2)
>   /* DSP */
>   static struct omap_mbox2_priv omap2_mbox_dsp_priv = {
>   	.tx_fifo = {
> @@ -301,7 +301,7 @@ struct omap_mbox mbox_dsp_info = {
>   };
>   #endif
>
> -#if defined(CONFIG_ARCH_OMAP3430)
> +#if defined(CONFIG_ARCH_OMAP3)

Ideally you should get rid of all the CONFIG_ARCH_OMAPXXX or cpu_is_omap 
in that code. This is a driver, it should be generic.
If you have to handle differences between OMAP version, please do that 
in the devices, not in the driver.

This patch just contains a few of them, but the original mailbox.c file 
is full of that kind of test.
I know that you are not the original writer of this code, but since the 
clean it, it will be good to remove all the legacy code.

Thanks,
Benoit

>   struct omap_mbox *omap3_mboxes[] = {&mbox_dsp_info, NULL };
>   #endif
>
> @@ -389,15 +389,19 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
>
>   	if (false)
>   		;
> -#if defined(CONFIG_ARCH_OMAP3430)
> -	else if (cpu_is_omap3430()) {
> +#if defined(CONFIG_ARCH_OMAP3)
> +	else if (cpu_is_omap34xx()) {
>   		list = omap3_mboxes;
>
>   		list[0]->irq = platform_get_irq_byname(pdev, "dsp");
>   	}
>   #endif
> -#if defined(CONFIG_ARCH_OMAP2420)
> -	else if (cpu_is_omap2420()) {
> +#if defined(CONFIG_ARCH_OMAP2)
> +	else if (cpu_is_omap2430()) {
> +		list = omap2_mboxes;
> +
> +		list[0]->irq = platform_get_irq_byname(pdev, "dsp");
> +	} else if (cpu_is_omap2420()) {
>   		list = omap2_mboxes;
>
>   		list[0]->irq = platform_get_irq_byname(pdev, "dsp");

[PATCH v2 7/7] omap: mailbox: remove unreachable return

[Cousson, Benoit]

On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> Remove unreachable return statement.
>
> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
> ---
>   arch/arm/mach-omap2/mailbox.c |    2 --
>   1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
> index 5e9ea0f..335eb2b 100644
> --- a/arch/arm/mach-omap2/mailbox.c
> +++ b/arch/arm/mach-omap2/mailbox.c
> @@ -434,8 +434,6 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
>   		return ret;
>   	}

Please add a blank line here.

Otherwise you can add: Acked-by: Benoit Cousson <b-cousson@ti.com>

Thanks,
Benoit

>   	return 0;
> -
> -	return ret;
>   }
>
>   static int __devexit omap2_mbox_remove(struct platform_device *pdev)

[PATCH v2 0/7] omap: mailbox: hwmod support and dependent cleanup patches

[Cousson, Benoit]

On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> hwmod support for omap2 and omap3 chips, plus cleanups.
>
> Tested on 3430 and 3630
>
> ** v2 **
> - Added omap4 hwmod support.
> - Moved "mailbox_ick" from hwmod to hwmod_if (omap 2/3)
> - Declared sysc classes for omap 2/3
>
> ** v1 **
> 1. omap: mailbox: initial hwmod support for omap3
> Changes were made to:
> - Rebase to latest code.
> - Detect the hwmod by filling prcm union for omap2, without
>    this it was unable to build the hwmod at runtime.
> - Replace magic number for defines.
> - Use ioremap again instead of relying on the one made by hwmod,
>    as noted in http://patchwork.kernel.org/patch/101661/
>
> 2. omap: mailbox: initial hwmod support for omap2
> Was only compiled tested!! Unfortunately I don't have the HW for it.
>
> 4. omap: mailbox: fix detection for previously supported chips
> Original patch was using features framework, but when compiled as a module
> it was breaking, this patch uses cpu_is_omap34xx macro instead. This is
> meant as a short term solution until proper cleanup is done, as suggested in:
>
> http://marc.info/?l=linux-arm-kernel&m=128534253231481&w=2
>
> Benoit Cousson (1):
>    OMAP4: hwmod data: add mailbox data
>
> Felipe Contreras (3):
>    OMAP3: hwmod data: add mailbox data
>    omap: mailbox: initial hwmod support
>    omap: mailbox: add omap_device latency information

Could you please try to use a consistent naming in the subject?
You use OMAP3, OMAP4, OMAP2 so it should be OMAP and not omap.

Thanks,
Benoit


>
> Omar Ramirez Luna (3):
>    OMAP2: hwmod data: add mailbox data
>    omap: mailbox: fix detection for previously supported chips
>    omap: mailbox: remove unreachable return
>
>   arch/arm/mach-omap2/devices.c              |  104 +++++++---------------------
>   arch/arm/mach-omap2/mailbox.c              |   39 +++++-----
>   arch/arm/mach-omap2/omap_hwmod_2420_data.c |   67 ++++++++++++++++++
>   arch/arm/mach-omap2/omap_hwmod_2430_data.c |   66 ++++++++++++++++++
>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |   66 ++++++++++++++++++
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   63 +++++++++++++++++
>   arch/arm/plat-omap/include/plat/mailbox.h  |    6 ++
>   7 files changed, 311 insertions(+), 100 deletions(-)
>

[PATCH v2 1/7] OMAP2: hwmod data: add mailbox data

[Ramirez Luna, Omar]

 Hi Benoit,

On Sat, Nov 6, 2010 at 12:08 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>> +/* l4_core -> ?mailbox */
>> +static struct omap_hwmod_ocp_if omap2420_l4_core__mailbox = {
>> + ? ? ? .master ? ? ? ? =&omap2420_l4_core_hwmod,
>> + ? ? ? .slave ? ? ? ? ?=&omap2420_mailbox_hwmod,
>> + ? ? ? .addr ? ? ? ? ? = omap2420_mailbox_addrs,
>> + ? ? ? .clk ? ? ? ? ? ?= "mailboxes_ick",
>
> Could you try to be consistent with the omap4 format? it is easier to diff
> between Soc version: .clk is before .addr

In all the randomness of how the data files look, I didn't now omap4
was the standard. For this one, I tried to follow the
"omap_hwmod_ocp_if" structure definition where 'clk' is the 4th member
of that structure.

I'll change it to mimic omap4 looks...

>> +static struct omap_hwmod omap2420_mailbox_hwmod = {
>> + ? ? ? .name ? ? ? ? ? = "mailbox",
>> + ? ? ? .class ? ? ? ? ?=&omap2420_mailbox_hwmod_class,
>> + ? ? ? .prcm ? ? ? ? ? = {
>> + ? ? ? ? ? ? ? .omap2 = {
>> + ? ? ? ? ? ? ? ? ? ? ? .prcm_reg_id = 1,
>> + ? ? ? ? ? ? ? ? ? ? ? .module_bit = OMAP24XX_EN_MAILBOXES_SHIFT,
>> + ? ? ? ? ? ? ? ? ? ? ? .module_offs = CORE_MOD,
>> + ? ? ? ? ? ? ? ? ? ? ? .idlest_reg_id = 1,
>> + ? ? ? ? ? ? ? ? ? ? ? .idlest_idle_bit = OMAP24XX_ST_MAILBOXES_SHIFT,
>> + ? ? ? ? ? ? ? },
>> + ? ? ? },
>
> Same things with the format, put that after irqs_cnt and before slaves.

more or less the same, since 'prcm' was above in the definition order
I kept it a bit up of where it is supposed to be.

I'll change it too

>> @@ -569,6 +635,7 @@ static __initdata struct omap_hwmod *omap2420_hwmods[]
>> = {
>> ? ? ? ?&omap2420_uart3_hwmod,
>> ? ? ? ?&omap2420_i2c1_hwmod,
>> ? ? ? ?&omap2420_i2c2_hwmod,
>
> Add a blank line between each class. OK it was not done for i2c, but it
> should.
>
> All the comments are applicable to the 2430 and 3430 data as well.

Will do.

Thanks for your comments,

Omar

[PATCH v2 3/7] OMAP4: hwmod data: add mailbox data

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 12:18 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> I don't know why, but this patch has nothing to do with my original one.
> Can you stick to the original code?

no, apart from the ordering of structure members, that I will change,
since keeping the order of the original structure doesn't fly, I don't
see anything that needs to be changed.

- The magic numbers replaced for the defines, afaik it gives more clarity.
- mailbox irq has a name.
- overall defining block was improved:

        class
        ocp_if
        slave ports
        hwmod

If you see, each dependent reference is right before the structure
that is using it, which at least to me establishes some order, as of
today this ordering doesn't exists.

e.g. you are defining some hwmod and some how you are populating all
the members, if you are looking at your omap_hwmod struct and want to
see the irqs defined you need to scroll beyond the supposed first
reference in omap_hwmod (right now above ocp_if)

>
> On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
>> +/* mailbox */
>
> The original comment is missing.

<quote>
/*
 * 'mailbox' class
 * mailbox module allowing communication between the on-chip processors
 * useusing a queued mailbox-interrupt mechanism.
 */
</quote>

I don't think it adds anything to the patch, should we start
commenting on the functionality of the drivers for each hwmod?

>> +
>> +static struct omap_hwmod omap44xx_mailbox_hwmod;
>> +
>> +static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
>> + ? ? ? {
>> + ? ? ? ? ? ? ? .pa_start ? ? ? = OMAP44XX_MAILBOX_BASE,
>
> If that physical address is not used elsewhere, and it should be the case,
> there is no need to create a define for it. That's why the physical address
> was directly used here.
> There is no added value to create a define for that.

yes there is, apart from readability where '0x4a0f4000' doesn't say much

for me at least, if reviewing I need to open the TRM check if that is
the address and move on, with the define you know that someone have
checked the address before (when creating the define)

besides the define was already there

>> +static struct omap_hwmod omap44xx_mailbox_hwmod = {
>> + ? ? ? .name ? ? ? ? ? = "mailbox",
>> + ? ? ? .class ? ? ? ? ?=&omap44xx_mailbox_hwmod_class,
>> + ? ? ? .prcm ? ? ? ? ? = {
>> + ? ? ? ? ? ? ? .omap4 = {
>> + ? ? ? ? ? ? ? ? ? ? ? .clkctrl_reg = OMAP4430_CM_L4CFG_MAILBOX_CLKCTRL,
>> + ? ? ? ? ? ? ? },
>> + ? ? ? },
>> + ? ? ? .mpu_irqs ? ? ? = omap44xx_mailbox_irqs,
>> + ? ? ? .mpu_irqs_cnt ? = ARRAY_SIZE(omap44xx_mailbox_irqs),
>
> The order is different than the previous one.

as the order is given by omap4 data, and not the definition of the
structure, I'll keep the consistency with omap4 then.

Regards,

Omar

[PATCH v2 6/7] omap: mailbox: fix detection for previously supported chips

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 1:11 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>> -#if defined(CONFIG_ARCH_OMAP3430)
>> +#if defined(CONFIG_ARCH_OMAP3)
>
> Ideally you should get rid of all the CONFIG_ARCH_OMAPXXX or cpu_is_omap in
> that code. This is a driver, it should be generic.
> If you have to handle differences between OMAP version, please do that in
> the devices, not in the driver.
>
> This patch just contains a few of them, but the original mailbox.c file is
> full of that kind of test.
> I know that you are not the original writer of this code, but since the
> clean it, it will be good to remove all the legacy code.

I mentioned it in the cover-letter, I should have put it here too, my bad.

<quote>
This is meant as a short term solution until proper cleanup is done,
as suggested in:

http://marc.info/?l=linux-arm-kernel&m=128534253231481&w=2
</quote>

Does nobody care that the driver is not working right now for some
chips (since it was working before!!) and are willing to wait for more
time until the proper cleanup is done?

For me it is a hassle, because if I need to do something on 3630 I
have to merge this patch, then apply what I'm working into, then
remove the patch, apply everything again to see no dependencies are
there, then send.

Regards,

Omar

[PATCH v2 7/7] omap: mailbox: remove unreachable return

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 1:21 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
>>
>> Remove unreachable return statement.
>>
>> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
>> ---
>> ?arch/arm/mach-omap2/mailbox.c | ? ?2 --
>> ?1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
>> index 5e9ea0f..335eb2b 100644
>> --- a/arch/arm/mach-omap2/mailbox.c
>> +++ b/arch/arm/mach-omap2/mailbox.c
>> @@ -434,8 +434,6 @@ static int __devinit omap2_mbox_probe(struct
>> platform_device *pdev)
>> ? ? ? ? ? ? ? ?return ret;
>> ? ? ? ?}
>
> Please add a blank line here.

ok...

>
> Otherwise you can add: Acked-by: Benoit Cousson <b-cousson@ti.com>

Regards,

Omar

[PATCH v2 0/7] omap: mailbox: hwmod support and dependent cleanup patches

[Ramirez Luna, Omar]

On Sat, Nov 6, 2010 at 1:32 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
> Could you please try to use a consistent naming in the subject?
> You use OMAP3, OMAP4, OMAP2 so it should be OMAP and not omap.

Yes, I'll change it.

Regards,

Omar

[PATCH v2 6/7] omap: mailbox: fix detection for previously supported chips

[Felipe Contreras]

On Sun, Nov 7, 2010 at 5:15 PM, Ramirez Luna, Omar <omar.ramirez@ti.com> wrote:
> On Sat, Nov 6, 2010 at 1:11 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
>>> -#if defined(CONFIG_ARCH_OMAP3430)
>>> +#if defined(CONFIG_ARCH_OMAP3)
>>
>> Ideally you should get rid of all the CONFIG_ARCH_OMAPXXX or cpu_is_omap in
>> that code. This is a driver, it should be generic.
>> If you have to handle differences between OMAP version, please do that in
>> the devices, not in the driver.
>>
>> This patch just contains a few of them, but the original mailbox.c file is
>> full of that kind of test.
>> I know that you are not the original writer of this code, but since the
>> clean it, it will be good to remove all the legacy code.
>
> I mentioned it in the cover-letter, I should have put it here too, my bad.
>
> <quote>
> This is meant as a short term solution until proper cleanup is done,
> as suggested in:
>
> http://marc.info/?l=linux-arm-kernel&m=128534253231481&w=2
> </quote>
>
> Does nobody care that the driver is not working right now for some
> chips (since it was working before!!) and are willing to wait for more
> time until the proper cleanup is done?
>
> For me it is a hassle, because if I need to do something on 3630 I
> have to merge this patch, then apply what I'm working into, then
> remove the patch, apply everything again to see no dependencies are
> there, then send.

Huh? The mailbox is working for me on tidspbridge on 2.6.37-rc1.

-- 
Felipe Contreras

[PATCH v2 3/7] OMAP4: hwmod data: add mailbox data

[Cousson, Benoit]

Hi Omar,

On 11/7/2010 10:07 AM, Ramirez Luna, Omar wrote:
> On Sat, Nov 6, 2010 at 12:18 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> I don't know why, but this patch has nothing to do with my original one.
>> Can you stick to the original code?
>
> no, apart from the ordering of structure members, that I will change,

If you do want to change a patch already submitted to loml, the least 
you can do is to comment on the mailing list, and afaik, you didn't 
provide any comment on this code.
So do not change it without any explanation or any rational.
That's the most basic rule of the patch submission / review process in 
Linux.

> since keeping the order of the original structure doesn't fly, I don't
> see anything that needs to be changed.
>
> - The magic numbers replaced for the defines, afaik it gives more clarity.

No, since these are not magic numbers but physical address or dma number 
or irq channel.
That does not give any more clarity to add a define, and in fact it adds 
an extra level of indirection for nothing.
The hwmod file is "the" unique HW definition files. So all the 
information that used to be scattered all over various header files will 
have to be there.
You should considered this file as a global SoC HW definition file.

> - mailbox irq has a name.

Which is in the structure anyway, so again no need to add a define.
All the structure that are populated hrer are all unique to every IP, so 
having that kind of assignment does not seems very useful to me.

omap44xx_mailbox_irq = OMAP44XX_MAILBOX_IRQ;

whereas that:
omap44xx_mailbox_irq = 20;

Give you a little be more information, because you do not have have to 
read the TRM or another header file to get the real number.

FYI, that was discussed at least 6 months ago during the submission of 
the early hwmod series.

> - overall defining block was improved:
>
>          class
>          ocp_if
>          slave ports
>          hwmod
>
> If you see, each dependent reference is right before the structure
> that is using it, which at least to me establishes some order, as of
> today this ordering doesn't exists.
>
> e.g. you are defining some hwmod and some how you are populating all
> the members, if you are looking at your omap_hwmod struct and want to
> see the irqs defined you need to scroll beyond the supposed first
> reference in omap_hwmod (right now above ocp_if)

So what? If you have any issue with the original order, please feel free 
to comment on the original patch.
In case you didn't notice, all the OMAP4 data are following the same 
pattern. So any change to the structure should be applied everywhere.

I do not have any issue to improve the overall readability if that make 
sense for everybody, but again, please comment first.

>>
>> On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
>>> +/* mailbox */
>>
>> The original comment is missing.
>
> <quote>
> /*
>   * 'mailbox' class
>   * mailbox module allowing communication between the on-chip processors
>   * useusing a queued mailbox-interrupt mechanism.
>   */
> </quote>
>
> I don't think it adds anything to the patch, should we start
> commenting on the functionality of the drivers for each hwmod?

In that case, it does not hurt since this file is the C file version of 
the TRM. You might not care because you know what that module is doing, 
but most people don't.

>
>>> +
>>> +static struct omap_hwmod omap44xx_mailbox_hwmod;
>>> +
>>> +static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
>>> +       {
>>> +               .pa_start       = OMAP44XX_MAILBOX_BASE,
>>
>> If that physical address is not used elsewhere, and it should be the case,
>> there is no need to create a define for it. That's why the physical address
>> was directly used here.
>> There is no added value to create a define for that.
>
> yes there is, apart from readability where '0x4a0f4000' doesn't say much

It says everything... when you use a debugger, what kind of information 
OMAP44XX_MAILBOX_BASE will give you? Nothing, you will have to check 
again the TRM or the define to get the real useful information.

> for me at least, if reviewing I need to open the TRM check if that is
> the address and move on, with the define you know that someone have
> checked the address before (when creating the define)

And what if the define is wrong??? You have as well to check the TRM... 
FYI, that file is automatically generated from the HW data, so it has a 
much lower probably to contain wrong data. Especially compared to a 
manually written header file done from a buggy TRM.

Regards,
Benoit

[PATCH v2 6/7] omap: mailbox: fix detection for previously supported chips

[Ramirez Luna, Omar]

On Sun, Nov 7, 2010 at 3:05 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Huh? The mailbox is working for me on tidspbridge on 2.6.37-rc1.

Are you using an omap based board with a 3630 chip?

Regards,

Omar

[PATCH v2 3/7] OMAP4: hwmod data: add mailbox data

[Ramirez Luna, Omar]

Hi Benoit,

On Mon, Nov 8, 2010 at 2:56 AM, Cousson, Benoit <b-cousson@ti.com> wrote:
>> no, apart from the ordering of structure members, that I will change,
>
> If you do want to change a patch already submitted to loml, the least you
> can do is to comment on the mailing list, and afaik, you didn't provide any
> comment on this code.
> So do not change it without any explanation or any rational.
> That's the most basic rule of the patch submission / review process in
> Linux.

I'm submitting my changes to review, I accept that I should have put
loudly an clear the reasons why the ordering was changing. I guess you
are the maintainer of the code, right? so if you say the ordering
standard is omap4 then I'm fine with such triviality.

>> since keeping the order of the original structure doesn't fly, I don't
>> see anything that needs to be changed.
>>
>> - The magic numbers replaced for the defines, afaik it gives more clarity.
>
> No, since these are not magic numbers but physical address or dma number or
> irq channel.
> That does not give any more clarity to add a define, and in fact it adds an
> extra level of indirection for nothing.
> The hwmod file is "the" unique HW definition files. So all the information
> that used to be scattered all over various header files will have to be
> there.
> You should considered this file as a global SoC HW definition file.

Ok, I don't mind changing this too, I'm glad you clarify/review this
before more hwmods make it to the tree, following the same style I'm
using which is the one already there.

>> - mailbox irq has a name.
>
> Which is in the structure anyway, so again no need to add a define.

I didn't add a define for the irq name, I put the irq name because it
wasn't there.

> All the structure that are populated hrer are all unique to every IP, so
> having that kind of assignment does not seems very useful to me.
>
> omap44xx_mailbox_irq = OMAP44XX_MAILBOX_IRQ;
>
> whereas that:
> omap44xx_mailbox_irq = 20;
>
> Give you a little be more information, because you do not have have to read
> the TRM or another header file to get the real number.
>
> FYI, that was discussed at least 6 months ago during the submission of the
> early hwmod series.
>
>> - overall defining block was improved:
>>
>> ? ? ? ? class
>> ? ? ? ? ocp_if
>> ? ? ? ? slave ports
>> ? ? ? ? hwmod
>>
>> If you see, each dependent reference is right before the structure
>> that is using it, which at least to me establishes some order, as of
>> today this ordering doesn't exists.
>>
>> e.g. you are defining some hwmod and some how you are populating all
>> the members, if you are looking at your omap_hwmod struct and want to
>> see the irqs defined you need to scroll beyond the supposed first
>> reference in omap_hwmod (right now above ocp_if)
>
> So what? If you have any issue with the original order, please feel free to
> comment on the original patch.
> In case you didn't notice, all the OMAP4 data are following the same
> pattern. So any change to the structure should be applied everywhere.
>
> I do not have any issue to improve the overall readability if that make
> sense for everybody, but again, please comment first.

Again, I should have sent an RFC explaining such changes. will do.

I feel commenting in the original patch is useless because this code
is already in-tree, it might have already changed.

>> <quote>
>> /*
>> ?* 'mailbox' class
>> ?* mailbox module allowing communication between the on-chip processors
>> ?* useusing a queued mailbox-interrupt mechanism.
>> ?*/
>> </quote>
>>
>> I don't think it adds anything to the patch, should we start
>> commenting on the functionality of the drivers for each hwmod?
>
> In that case, it does not hurt since this file is the C file version of the
> TRM. You might not care because you know what that module is doing, but most
> people don't.

Ok, then.

>>>> +static struct omap_hwmod omap44xx_mailbox_hwmod;
>>>> +
>>>> +static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
>>>> + ? ? ? {
>>>> + ? ? ? ? ? ? ? .pa_start ? ? ? = OMAP44XX_MAILBOX_BASE,
>>>
>>> If that physical address is not used elsewhere, and it should be the
>>> case,
>>> there is no need to create a define for it. That's why the physical
>>> address
>>> was directly used here.
>>> There is no added value to create a define for that.
>>
>> yes there is, apart from readability where '0x4a0f4000' doesn't say much
>
> It says everything... when you use a debugger, what kind of information
> OMAP44XX_MAILBOX_BASE will give you? Nothing, you will have to check again
> the TRM or the define to get the real useful information.

Why is that? if you use a debugger you will see the address, if you
use an objdump you can easily traverse to the definition (with cscope,
ctags...). Again, I don't mind placing the hex number there, now that
there is an explanation for doing so.

Overall, I feel the lack of information made the code look as it is
right now for the existing LO hwmods, given that it is also a matter
of ordering stuff it should be pretty straight forward. I'll give you
my proposal offline, if you think it makes sense we can send it to LO
and avoid traffic of non-technical stuff.

Regards,

Omar

[PATCH v2 6/7] omap: mailbox: fix detection for previously supported chips

[Cousson, Benoit]

On 11/7/2010 10:15 AM, Ramirez Luna, Omar wrote:
> On Sat, Nov 6, 2010 at 1:11 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>>> -#if defined(CONFIG_ARCH_OMAP3430)
>>> +#if defined(CONFIG_ARCH_OMAP3)
>>
>> Ideally you should get rid of all the CONFIG_ARCH_OMAPXXX or cpu_is_omap in
>> that code. This is a driver, it should be generic.
>> If you have to handle differences between OMAP version, please do that in
>> the devices, not in the driver.
>>
>> This patch just contains a few of them, but the original mailbox.c file is
>> full of that kind of test.
>> I know that you are not the original writer of this code, but since the
>> clean it, it will be good to remove all the legacy code.
>
> I mentioned it in the cover-letter, I should have put it here too, my bad.
>
> <quote>
> This is meant as a short term solution until proper cleanup is done,
> as suggested in:
>
> http://marc.info/?l=linux-arm-kernel&m=128534253231481&w=2
> </quote>

OK, sorry I didn't realized that this email thread was about the 
mailbox. I'm glad to see that both Paul and Nishant are aligned with me.

> Does nobody care that the driver is not working right now for some
> chips (since it was working before!!) and are willing to wait for more
> time until the proper cleanup is done?

Sorry again, but removing these tests didn't not seems to be a huge task 
for my point of view.
Anyway, if you want to do another phase and if everybody agree on that, 
that's OK for me as well.

> For me it is a hassle, because if I need to do something on 3630 I
> have to merge this patch, then apply what I'm working into, then
> remove the patch, apply everything again to see no dependencies are
> there, then send.

Yeah, sometime life really sucks :-)

Benoit