[PATCH v2 0/3] Fixes in preparation for enabling Dsp in omap4

[PATCH v2 0/3] Fixes in preparation for enabling Dsp in omap4

[Juan Gutierrez]

This set of patches provides the foundation for enabling
OMAP4 Dsp (Tesla) as remote processor.

The first patch is a generic fix for mailbox in mach-omap2.
Without this patch, the irq for a mailbox instance other than
the first one is not properly enabled.

The second patch fixes the clock and irq names for the Tesla
iommu in omap4 and enable it by default.

The third patch adds a generic mechanism in remoteproc for
loading the boot address in a special bootloader register, if
needed by the remoteproc like is the case for Tesla in omap4.

History:
=======
Changes from v1:
  - Whitespaces and tabs were added when sending the patches
    by the mailer, due to wrong configuration. 
  - No code changes.

Juan Gutierrez (3):
  omap: mailbox: enable mailbox irq per instance
  OMAP4: iommu: fix irq and clock name for dsp-iommu
  omap: remoteproc: add support for a boot register

 arch/arm/mach-omap2/mailbox.c                |    2 --
 arch/arm/mach-omap2/omap-iommu.c             |    6 ++----
 arch/arm/plat-omap/include/plat/remoteproc.h |    2 ++
 arch/arm/plat-omap/mailbox.c                 |    3 +++
 drivers/remoteproc/omap_remoteproc.c         |   19 +++++++++++++++++++
 5 files changed, 26 insertions(+), 6 deletions(-)

[PATCH v2 0/3] Fixes in preparation for enabling Dsp in omap4

[Juan Gutierrez]

This set of patches provides the foundation for enabling
OMAP4 Dsp (Tesla) as remote processor.

The first patch is a generic fix for mailbox in mach-omap2.
Without this patch, the irq for a mailbox instance other than
the first one is not properly enabled.

The second patch fixes the clock and irq names for the Tesla
iommu in omap4 and enable it by default.

The third patch adds a generic mechanism in remoteproc for
loading the boot address in a special bootloader register, if
needed by the remoteproc like is the case for Tesla in omap4.

History:
=======
Changes from v1:
  - Whitespaces and tabs were added when sending the patches
    by the mailer, due to wrong configuration. 
  - No code changes.

Juan Gutierrez (3):
  omap: mailbox: enable mailbox irq per instance
  OMAP4: iommu: fix irq and clock name for dsp-iommu
  omap: remoteproc: add support for a boot register

 arch/arm/mach-omap2/mailbox.c                |    2 --
 arch/arm/mach-omap2/omap-iommu.c             |    6 ++----
 arch/arm/plat-omap/include/plat/remoteproc.h |    2 ++
 arch/arm/plat-omap/mailbox.c                 |    3 +++
 drivers/remoteproc/omap_remoteproc.c         |   19 +++++++++++++++++++
 5 files changed, 26 insertions(+), 6 deletions(-)

[PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Juan Gutierrez]

The machine-specific omap2_mbox_startup is called only once
to initialize the whole mbox module. Enabling mbox irq at
startup is only enabling the irq of the very first mailbox
instance created.

The enable_irq function should be called every time a
new mbox instance is created, in the generic platform
mailbox layer.

Signed-off-by: Juan Gutierrez <jgutierrez@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 arch/arm/mach-omap2/mailbox.c |    2 --
 arch/arm/plat-omap/mailbox.c  |    3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 415a6f1..f727034 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -83,8 +83,6 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
 	l = mbox_read_reg(MAILBOX_REVISION);
 	pr_debug("omap mailbox rev %d.%d\n", (l & 0xf0) >> 4, (l & 0x0f));
 
-	omap2_mbox_enable_irq(mbox, IRQ_RX);
-
 	return 0;
 }
 
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index ad32621..ebc1ba5 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -282,6 +282,8 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 		}
 		mbox->rxq = mq;
 		mq->mbox = mbox;
+
+		mbox->ops->enable_irq(mbox, IRQ_RX);
 	}
 	mutex_unlock(&mbox_configured_lock);
 	return 0;
@@ -305,6 +307,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 	mutex_lock(&mbox_configured_lock);
 
 	if (!--mbox->use_count) {
+		mbox->ops->disable_irq(mbox, IRQ_RX);
 		free_irq(mbox->irq, mbox);
 		tasklet_kill(&mbox->txq->tasklet);
 		flush_work_sync(&mbox->rxq->work);
-- 
1.7.1

[PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Juan Gutierrez]

The machine-specific omap2_mbox_startup is called only once
to initialize the whole mbox module. Enabling mbox irq at
startup is only enabling the irq of the very first mailbox
instance created.

The enable_irq function should be called every time a
new mbox instance is created, in the generic platform
mailbox layer.

Signed-off-by: Juan Gutierrez <jgutierrez@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 arch/arm/mach-omap2/mailbox.c |    2 --
 arch/arm/plat-omap/mailbox.c  |    3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 415a6f1..f727034 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -83,8 +83,6 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
 	l = mbox_read_reg(MAILBOX_REVISION);
 	pr_debug("omap mailbox rev %d.%d\n", (l & 0xf0) >> 4, (l & 0x0f));
 
-	omap2_mbox_enable_irq(mbox, IRQ_RX);
-
 	return 0;
 }
 
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index ad32621..ebc1ba5 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -282,6 +282,8 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 		}
 		mbox->rxq = mq;
 		mq->mbox = mbox;
+
+		mbox->ops->enable_irq(mbox, IRQ_RX);
 	}
 	mutex_unlock(&mbox_configured_lock);
 	return 0;
@@ -305,6 +307,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 	mutex_lock(&mbox_configured_lock);
 
 	if (!--mbox->use_count) {
+		mbox->ops->disable_irq(mbox, IRQ_RX);
 		free_irq(mbox->irq, mbox);
 		tasklet_kill(&mbox->txq->tasklet);
 		flush_work_sync(&mbox->rxq->work);
-- 
1.7.1

[PATCH v2 2/3] OMAP4: iommu: fix irq and clock name for dsp-iommu

[Juan Gutierrez]

Irq and clock names were wrong for dsp iommu configuration
for omap4.

Renamed tesla_ick to dsp_fck.
Renamed INT_44XX_DSP_MMU to OMAP44XX_IRQ_TESLA_MMU.

dsp-iommu is enabled by default.

Signed-off-by: Juan Gutierrez <jgutierrez@ti.com>
---
 arch/arm/mach-omap2/omap-iommu.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index ac49384..1be8bcb 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -73,19 +73,17 @@ static struct iommu_device omap4_devices[] = {
 			.da_end = 0xFFFFF000,
 		},
 	},
-#if defined(CONFIG_MPU_TESLA_IOMMU)
 	{
 		.base = OMAP4_MMU2_BASE,
-		.irq = INT_44XX_DSP_MMU,
+		.irq = OMAP44XX_IRQ_TESLA_MMU,
 		.pdata = {
 			.name = "tesla",
 			.nr_tlb_entries = 32,
-			.clk_name = "tesla_ick",
+			.clk_name = "dsp_fck",
 			.da_start = 0x0,
 			.da_end = 0xFFFFF000,
 		},
 	},
-#endif
 };
 #define NR_OMAP4_IOMMU_DEVICES ARRAY_SIZE(omap4_devices)
 static struct platform_device *omap4_iommu_pdev[NR_OMAP4_IOMMU_DEVICES];
-- 
1.7.1

[PATCH v2 2/3] OMAP4: iommu: fix irq and clock name for dsp-iommu

[Juan Gutierrez]

Irq and clock names were wrong for dsp iommu configuration
for omap4.

Renamed tesla_ick to dsp_fck.
Renamed INT_44XX_DSP_MMU to OMAP44XX_IRQ_TESLA_MMU.

dsp-iommu is enabled by default.

Signed-off-by: Juan Gutierrez <jgutierrez@ti.com>
---
 arch/arm/mach-omap2/omap-iommu.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index ac49384..1be8bcb 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -73,19 +73,17 @@ static struct iommu_device omap4_devices[] = {
 			.da_end = 0xFFFFF000,
 		},
 	},
-#if defined(CONFIG_MPU_TESLA_IOMMU)
 	{
 		.base = OMAP4_MMU2_BASE,
-		.irq = INT_44XX_DSP_MMU,
+		.irq = OMAP44XX_IRQ_TESLA_MMU,
 		.pdata = {
 			.name = "tesla",
 			.nr_tlb_entries = 32,
-			.clk_name = "tesla_ick",
+			.clk_name = "dsp_fck",
 			.da_start = 0x0,
 			.da_end = 0xFFFFF000,
 		},
 	},
-#endif
 };
 #define NR_OMAP4_IOMMU_DEVICES ARRAY_SIZE(omap4_devices)
 static struct platform_device *omap4_iommu_pdev[NR_OMAP4_IOMMU_DEVICES];
-- 
1.7.1

[PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Juan Gutierrez]

Some remote processors (like Omap4's DSP) need to explicitly
configure a boot address in a special register or memory
location, and this is the address from which they start
executing code when taken out of reset.

Support for this infrastructure has been added to remoteproc
code. The memory or register address where the boot address
is to be stored is supplied through the boot_reg field of the
platform data in the remoteproc. The omap_rproc_start function
will fetch the boot address from the image, and write this
address into the boot register, if provided, before taking the
processor out of reset.

Signed-off-by: Juan Gutierrez <jgutierrez@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 arch/arm/plat-omap/include/plat/remoteproc.h |    2 ++
 drivers/remoteproc/omap_remoteproc.c         |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/remoteproc.h b/arch/arm/plat-omap/include/plat/remoteproc.h
index b10eac8..2066a14 100644
--- a/arch/arm/plat-omap/include/plat/remoteproc.h
+++ b/arch/arm/plat-omap/include/plat/remoteproc.h
@@ -30,6 +30,7 @@ struct platform_device;
  * @ops: start/stop rproc handlers
  * @device_enable: omap-specific handler for enabling a device
  * @device_shutdown: omap-specific handler for shutting down a device
+ * @boot_reg: physical address of the control register for storing boot address
  */
 struct omap_rproc_pdata {
 	const char *name;
@@ -40,6 +41,7 @@ struct omap_rproc_pdata {
 	const struct rproc_ops *ops;
 	int (*device_enable) (struct platform_device *pdev);
 	int (*device_shutdown) (struct platform_device *pdev);
+	u32 boot_reg;
 };
 
 #if defined(CONFIG_OMAP_REMOTEPROC) || defined(CONFIG_OMAP_REMOTEPROC_MODULE)
diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c
index 69425c4..e9b2f85 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -39,11 +39,13 @@
  * @mbox: omap mailbox handle
  * @nb: notifier block that will be invoked on inbound mailbox messages
  * @rproc: rproc handle
+ * @boot_reg: virtual address of the register where the bootaddr is stored
  */
 struct omap_rproc {
 	struct omap_mbox *mbox;
 	struct notifier_block nb;
 	struct rproc *rproc;
+	void __iomem *boot_reg;
 };
 
 /**
@@ -114,6 +116,10 @@ static int omap_rproc_start(struct rproc *rproc)
 	struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
 	int ret;
 
+	/* load remote processor boot address if needed. */
+	if (oproc->boot_reg)
+		writel(rproc->bootaddr, oproc->boot_reg);
+
 	oproc->nb.notifier_call = omap_rproc_mbox_callback;
 
 	/* every omap rproc is assigned a mailbox instance for messaging */
@@ -194,6 +200,12 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
 	oproc = rproc->priv;
 	oproc->rproc = rproc;
 
+	if (pdata->boot_reg) {
+		oproc->boot_reg = ioremap(pdata->boot_reg, sizeof(u32));
+		if (!oproc->boot_reg)
+			goto err_ioremap;
+	}
+
 	platform_set_drvdata(pdev, rproc);
 
 	ret = rproc_register(rproc);
@@ -203,6 +215,9 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
 	return 0;
 
 free_rproc:
+	if (oproc->boot_reg)
+		iounmap(oproc->boot_reg);
+err_ioremap:
 	rproc_free(rproc);
 	return ret;
 }
@@ -210,6 +225,10 @@ free_rproc:
 static int __devexit omap_rproc_remove(struct platform_device *pdev)
 {
 	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct omap_rproc *oproc = rproc->priv;
+
+	if (oproc->boot_reg)
+		iounmap(oproc->boot_reg);
 
 	return rproc_unregister(rproc);
 }
-- 
1.7.1

[PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Juan Gutierrez]

Some remote processors (like Omap4's DSP) need to explicitly
configure a boot address in a special register or memory
location, and this is the address from which they start
executing code when taken out of reset.

Support for this infrastructure has been added to remoteproc
code. The memory or register address where the boot address
is to be stored is supplied through the boot_reg field of the
platform data in the remoteproc. The omap_rproc_start function
will fetch the boot address from the image, and write this
address into the boot register, if provided, before taking the
processor out of reset.

Signed-off-by: Juan Gutierrez <jgutierrez@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 arch/arm/plat-omap/include/plat/remoteproc.h |    2 ++
 drivers/remoteproc/omap_remoteproc.c         |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/remoteproc.h b/arch/arm/plat-omap/include/plat/remoteproc.h
index b10eac8..2066a14 100644
--- a/arch/arm/plat-omap/include/plat/remoteproc.h
+++ b/arch/arm/plat-omap/include/plat/remoteproc.h
@@ -30,6 +30,7 @@ struct platform_device;
  * @ops: start/stop rproc handlers
  * @device_enable: omap-specific handler for enabling a device
  * @device_shutdown: omap-specific handler for shutting down a device
+ * @boot_reg: physical address of the control register for storing boot address
  */
 struct omap_rproc_pdata {
 	const char *name;
@@ -40,6 +41,7 @@ struct omap_rproc_pdata {
 	const struct rproc_ops *ops;
 	int (*device_enable) (struct platform_device *pdev);
 	int (*device_shutdown) (struct platform_device *pdev);
+	u32 boot_reg;
 };
 
 #if defined(CONFIG_OMAP_REMOTEPROC) || defined(CONFIG_OMAP_REMOTEPROC_MODULE)
diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c
index 69425c4..e9b2f85 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -39,11 +39,13 @@
  * @mbox: omap mailbox handle
  * @nb: notifier block that will be invoked on inbound mailbox messages
  * @rproc: rproc handle
+ * @boot_reg: virtual address of the register where the bootaddr is stored
  */
 struct omap_rproc {
 	struct omap_mbox *mbox;
 	struct notifier_block nb;
 	struct rproc *rproc;
+	void __iomem *boot_reg;
 };
 
 /**
@@ -114,6 +116,10 @@ static int omap_rproc_start(struct rproc *rproc)
 	struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
 	int ret;
 
+	/* load remote processor boot address if needed. */
+	if (oproc->boot_reg)
+		writel(rproc->bootaddr, oproc->boot_reg);
+
 	oproc->nb.notifier_call = omap_rproc_mbox_callback;
 
 	/* every omap rproc is assigned a mailbox instance for messaging */
@@ -194,6 +200,12 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
 	oproc = rproc->priv;
 	oproc->rproc = rproc;
 
+	if (pdata->boot_reg) {
+		oproc->boot_reg = ioremap(pdata->boot_reg, sizeof(u32));
+		if (!oproc->boot_reg)
+			goto err_ioremap;
+	}
+
 	platform_set_drvdata(pdev, rproc);
 
 	ret = rproc_register(rproc);
@@ -203,6 +215,9 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
 	return 0;
 
 free_rproc:
+	if (oproc->boot_reg)
+		iounmap(oproc->boot_reg);
+err_ioremap:
 	rproc_free(rproc);
 	return ret;
 }
@@ -210,6 +225,10 @@ free_rproc:
 static int __devexit omap_rproc_remove(struct platform_device *pdev)
 {
 	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct omap_rproc *oproc = rproc->priv;
+
+	if (oproc->boot_reg)
+		iounmap(oproc->boot_reg);
 
 	return rproc_unregister(rproc);
 }
-- 
1.7.1

Re: [PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Ohad Ben-Cohen]

Hi Juan,

Thanks for the patches ! it's exciting to see that support for OMAP's
DSP is (relatively) easily achieved now. I hope your work can be a
good basis for an OMAP3 port, too.

Overall the patches look good, I have only some minor comments inline.

And - sorry for the late response. I've just left for a (somewhat
long) business trip when you posted this.

On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
> Some remote processors (like Omap4's DSP) need to explicitly
> configure a boot address in a special register or memory
> location

Let's just slightly rephrase this to prevent confusion: some remote
processors "need to have the boot address configured" in a special
register (as opposed to "need to configure").

> @@ -30,6 +30,7 @@ struct platform_device;
>  * @ops: start/stop rproc handlers
>  * @device_enable: omap-specific handler for enabling a device
>  * @device_shutdown: omap-specific handler for shutting down a device
> + * @boot_reg: physical address of the control register for storing boot address
>  */
>  struct omap_rproc_pdata {
>        const char *name;
> @@ -40,6 +41,7 @@ struct omap_rproc_pdata {
>        const struct rproc_ops *ops;
>        int (*device_enable) (struct platform_device *pdev);
>        int (*device_shutdown) (struct platform_device *pdev);
> +       u32 boot_reg;

We might want to use an IORESOURCE_MEM resource instead, since we're
dealing with a platform device anyway.

The driver can then fetch the address using platform_get_resource.

> @@ -203,6 +215,9 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
>        return 0;
>
>  free_rproc:
> +       if (oproc->boot_reg)
> +               iounmap(oproc->boot_reg);
> +err_ioremap:
>        rproc_free(rproc);
>        return ret;
>  }

I tend to call those cleanup snippets with names that indicate what they do.

In this case I'd slightly prefer to keep calling the lower snippet
with "free_rproc" and just name the new snippet with something like
"unmap_bootreg". It's then a bit easier to read the code that calls
into those snippets (the reader easily know which cleanup snippet is
invoked by each 'goto').

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Ohad Ben-Cohen]

Hi Juan,

Thanks for the patches ! it's exciting to see that support for OMAP's
DSP is (relatively) easily achieved now. I hope your work can be a
good basis for an OMAP3 port, too.

Overall the patches look good, I have only some minor comments inline.

And - sorry for the late response. I've just left for a (somewhat
long) business trip when you posted this.

On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
> Some remote processors (like Omap4's DSP) need to explicitly
> configure a boot address in a special register or memory
> location

Let's just slightly rephrase this to prevent confusion: some remote
processors "need to have the boot address configured" in a special
register (as opposed to "need to configure").

> @@ -30,6 +30,7 @@ struct platform_device;
> ?* @ops: start/stop rproc handlers
> ?* @device_enable: omap-specific handler for enabling a device
> ?* @device_shutdown: omap-specific handler for shutting down a device
> + * @boot_reg: physical address of the control register for storing boot address
> ?*/
> ?struct omap_rproc_pdata {
> ? ? ? ?const char *name;
> @@ -40,6 +41,7 @@ struct omap_rproc_pdata {
> ? ? ? ?const struct rproc_ops *ops;
> ? ? ? ?int (*device_enable) (struct platform_device *pdev);
> ? ? ? ?int (*device_shutdown) (struct platform_device *pdev);
> + ? ? ? u32 boot_reg;

We might want to use an IORESOURCE_MEM resource instead, since we're
dealing with a platform device anyway.

The driver can then fetch the address using platform_get_resource.

> @@ -203,6 +215,9 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
> ? ? ? ?return 0;
>
> ?free_rproc:
> + ? ? ? if (oproc->boot_reg)
> + ? ? ? ? ? ? ? iounmap(oproc->boot_reg);
> +err_ioremap:
> ? ? ? ?rproc_free(rproc);
> ? ? ? ?return ret;
> ?}

I tend to call those cleanup snippets with names that indicate what they do.

In this case I'd slightly prefer to keep calling the lower snippet
with "free_rproc" and just name the new snippet with something like
"unmap_bootreg". It's then a bit easier to read the code that calls
into those snippets (the reader easily know which cleanup snippet is
invoked by each 'goto').

Thanks,
Ohad.

Re: [PATCH v2 2/3] OMAP4: iommu: fix irq and clock name for dsp-iommu

[Ohad Ben-Cohen]

Hi Juan,

On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
> Irq and clock names were wrong for dsp iommu configuration
> for omap4.

Looks good.

> Renamed tesla_ick to dsp_fck.

If you're resubmitting the series, please indicate here why this
change is needed (specifically let's mention commit 0e43327 "OMAP4:
clock: Fix clock names and align with hwmod names").

Thanks,
Ohad.

[PATCH v2 2/3] OMAP4: iommu: fix irq and clock name for dsp-iommu

[Ohad Ben-Cohen]

Hi Juan,

On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
> Irq and clock names were wrong for dsp iommu configuration
> for omap4.

Looks good.

> Renamed tesla_ick to dsp_fck.

If you're resubmitting the series, please indicate here why this
change is needed (specifically let's mention commit 0e43327 "OMAP4:
clock: Fix clock names and align with hwmod names").

Thanks,
Ohad.

Re: [PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Ohad Ben-Cohen]

Hi Juan,

On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
> The machine-specific omap2_mbox_startup is called only once
> to initialize the whole mbox module. Enabling mbox irq at
> startup is only enabling the irq of the very first mailbox
> instance created.
>
> The enable_irq function should be called every time a
> new mbox instance is created,

s/created/opened/

> in the generic platform mailbox layer.

This patch removes an omap2-specific code and then adds it to the
"generic" layer, which also deals with omap1.

Are we sure it's ok ?

OMAP1 doesn't seem to enable its irq at this point: it doesn't even
have a ->startup() handler (which actually somewhat implies it's been
anyway broken for a long time now: omap_mbox_startup() seem to
unhappily bail out if it doesn't find a machine-specific ->startup()
handler).

> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index ad32621..ebc1ba5 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -282,6 +282,8 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
>                }
>                mbox->rxq = mq;
>                mq->mbox = mbox;
> +
> +               mbox->ops->enable_irq(mbox, IRQ_RX);

Might be nicer to use omap_mbox_enable_irq() here instead of reaching
out for the internal ops.

It also looks like there's a race here: omap_mbox_get() registers the
notifier_block only after omap_mbox_startup() returns, which probably
means there's a small window where an interrupt can be received
without us invoking the user's notifier callback.

This isn't related to your patch of course, but since you're touching
this area, it might be nice if you can fix it (i.e. simply by
registering the notifier_block before enabling the mbox's irq).

>        }
>        mutex_unlock(&mbox_configured_lock);
>        return 0;
> @@ -305,6 +307,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
>        mutex_lock(&mbox_configured_lock);
>
>        if (!--mbox->use_count) {
> +               mbox->ops->disable_irq(mbox, IRQ_RX);

omap_mbox_disable_irq() ?

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Ohad Ben-Cohen]

Hi Juan,

On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
> The machine-specific omap2_mbox_startup is called only once
> to initialize the whole mbox module. Enabling mbox irq at
> startup is only enabling the irq of the very first mailbox
> instance created.
>
> The enable_irq function should be called every time a
> new mbox instance is created,

s/created/opened/

> in the generic platform mailbox layer.

This patch removes an omap2-specific code and then adds it to the
"generic" layer, which also deals with omap1.

Are we sure it's ok ?

OMAP1 doesn't seem to enable its irq at this point: it doesn't even
have a ->startup() handler (which actually somewhat implies it's been
anyway broken for a long time now: omap_mbox_startup() seem to
unhappily bail out if it doesn't find a machine-specific ->startup()
handler).

> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index ad32621..ebc1ba5 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -282,6 +282,8 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
> ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?mbox->rxq = mq;
> ? ? ? ? ? ? ? ?mq->mbox = mbox;
> +
> + ? ? ? ? ? ? ? mbox->ops->enable_irq(mbox, IRQ_RX);

Might be nicer to use omap_mbox_enable_irq() here instead of reaching
out for the internal ops.

It also looks like there's a race here: omap_mbox_get() registers the
notifier_block only after omap_mbox_startup() returns, which probably
means there's a small window where an interrupt can be received
without us invoking the user's notifier callback.

This isn't related to your patch of course, but since you're touching
this area, it might be nice if you can fix it (i.e. simply by
registering the notifier_block before enabling the mbox's irq).

> ? ? ? ?}
> ? ? ? ?mutex_unlock(&mbox_configured_lock);
> ? ? ? ?return 0;
> @@ -305,6 +307,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
> ? ? ? ?mutex_lock(&mbox_configured_lock);
>
> ? ? ? ?if (!--mbox->use_count) {
> + ? ? ? ? ? ? ? mbox->ops->disable_irq(mbox, IRQ_RX);

omap_mbox_disable_irq() ?

Thanks,
Ohad.

Re: [PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Ohad Ben-Cohen]

Hi Juan,

On Mon, May 7, 2012 at 5:38 PM, Gutierrez, Juan <jgutierrez@ti.com> wrote:
>> > @@ -40,6 +41,7 @@ struct omap_rproc_pdata {
>> >        const struct rproc_ops *ops;
>> >        int (*device_enable) (struct platform_device *pdev);
>> >        int (*device_shutdown) (struct platform_device *pdev);
>> > +       u32 boot_reg;
>>
>> We might want to use an IORESOURCE_MEM resource instead, since we're
>> dealing with a platform device anyway.
>>
>> The driver can then fetch the address using platform_get_resource.
>
> Ok, Do you mean using resource table to get the boot_reg address, right?

Sorry for the confusion - I meant using a 'struct resource' as the
standard way to pass this kind of information to platform drivers.

For example take a look how omap2_mbox_probe uses
platform_get_resource to retrieve the parameters which it then feeds
to ioremap.

Using this standard approach will probably make life easier when we
migrate to DT, too.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Ohad Ben-Cohen]

Hi Juan,

On Mon, May 7, 2012 at 5:38 PM, Gutierrez, Juan <jgutierrez@ti.com> wrote:
>> > @@ -40,6 +41,7 @@ struct omap_rproc_pdata {
>> > ? ? ? ?const struct rproc_ops *ops;
>> > ? ? ? ?int (*device_enable) (struct platform_device *pdev);
>> > ? ? ? ?int (*device_shutdown) (struct platform_device *pdev);
>> > + ? ? ? u32 boot_reg;
>>
>> We might want to use an IORESOURCE_MEM resource instead, since we're
>> dealing with a platform device anyway.
>>
>> The driver can then fetch the address using platform_get_resource.
>
> Ok, Do you mean using resource table to get the boot_reg address, right?

Sorry for the confusion - I meant using a 'struct resource' as the
standard way to pass this kind of information to platform drivers.

For example take a look how omap2_mbox_probe uses
platform_get_resource to retrieve the parameters which it then feeds
to ioremap.

Using this standard approach will probably make life easier when we
migrate to DT, too.

Thanks,
Ohad.

Re: [PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Gutierrez, Juan]

Hi Ohad

Thanks for reviewing.

On Sun, May 6, 2012 at 6:21 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Juan,
>
> Thanks for the patches ! it's exciting to see that support for OMAP's
> DSP is (relatively) easily achieved now. I hope your work can be a
> good basis for an OMAP3 port, too.
>
> Overall the patches look good, I have only some minor comments inline.
>
> And - sorry for the late response. I've just left for a (somewhat
> long) business trip when you posted this.
>
> On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
>> Some remote processors (like Omap4's DSP) need to explicitly
>> configure a boot address in a special register or memory
>> location
>
> Let's just slightly rephrase this to prevent confusion: some remote
> processors "need to have the boot address configured" in a special
> register (as opposed to "need to configure").
>
Ok, I will do it.

>> @@ -30,6 +30,7 @@ struct platform_device;
>>  * @ops: start/stop rproc handlers
>>  * @device_enable: omap-specific handler for enabling a device
>>  * @device_shutdown: omap-specific handler for shutting down a device
>> + * @boot_reg: physical address of the control register for storing boot address
>>  */
>>  struct omap_rproc_pdata {
>>        const char *name;
>> @@ -40,6 +41,7 @@ struct omap_rproc_pdata {
>>        const struct rproc_ops *ops;
>>        int (*device_enable) (struct platform_device *pdev);
>>        int (*device_shutdown) (struct platform_device *pdev);
>> +       u32 boot_reg;
>
> We might want to use an IORESOURCE_MEM resource instead, since we're
> dealing with a platform device anyway.
>
> The driver can then fetch the address using platform_get_resource.
>
Ok, I will investigate about this, and include it in next series

>> @@ -203,6 +215,9 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
>>        return 0;
>>
>>  free_rproc:
>> +       if (oproc->boot_reg)
>> +               iounmap(oproc->boot_reg);
>> +err_ioremap:
>>        rproc_free(rproc);
>>        return ret;
>>  }
>
> I tend to call those cleanup snippets with names that indicate what they do.
>
> In this case I'd slightly prefer to keep calling the lower snippet
> with "free_rproc" and just name the new snippet with something like
> "unmap_bootreg". It's then a bit easier to read the code that calls
> into those snippets (the reader easily know which cleanup snippet is
> invoked by each 'goto').
>
Agree. I will do the change.

> Thanks,
> Ohad.



-- 
Thanks

juan gutierrez
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] omap: remoteproc: add support for a boot register

[Gutierrez, Juan]

Hi Ohad

Thanks for reviewing.

On Sun, May 6, 2012 at 6:21 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Juan,
>
> Thanks for the patches ! it's exciting to see that support for OMAP's
> DSP is (relatively) easily achieved now. I hope your work can be a
> good basis for an OMAP3 port, too.
>
> Overall the patches look good, I have only some minor comments inline.
>
> And - sorry for the late response. I've just left for a (somewhat
> long) business trip when you posted this.
>
> On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
>> Some remote processors (like Omap4's DSP) need to explicitly
>> configure a boot address in a special register or memory
>> location
>
> Let's just slightly rephrase this to prevent confusion: some remote
> processors "need to have the boot address configured" in a special
> register (as opposed to "need to configure").
>
Ok, I will do it.

>> @@ -30,6 +30,7 @@ struct platform_device;
>> ?* @ops: start/stop rproc handlers
>> ?* @device_enable: omap-specific handler for enabling a device
>> ?* @device_shutdown: omap-specific handler for shutting down a device
>> + * @boot_reg: physical address of the control register for storing boot address
>> ?*/
>> ?struct omap_rproc_pdata {
>> ? ? ? ?const char *name;
>> @@ -40,6 +41,7 @@ struct omap_rproc_pdata {
>> ? ? ? ?const struct rproc_ops *ops;
>> ? ? ? ?int (*device_enable) (struct platform_device *pdev);
>> ? ? ? ?int (*device_shutdown) (struct platform_device *pdev);
>> + ? ? ? u32 boot_reg;
>
> We might want to use an IORESOURCE_MEM resource instead, since we're
> dealing with a platform device anyway.
>
> The driver can then fetch the address using platform_get_resource.
>
Ok, I will investigate about this, and include it in next series

>> @@ -203,6 +215,9 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
>> ? ? ? ?return 0;
>>
>> ?free_rproc:
>> + ? ? ? if (oproc->boot_reg)
>> + ? ? ? ? ? ? ? iounmap(oproc->boot_reg);
>> +err_ioremap:
>> ? ? ? ?rproc_free(rproc);
>> ? ? ? ?return ret;
>> ?}
>
> I tend to call those cleanup snippets with names that indicate what they do.
>
> In this case I'd slightly prefer to keep calling the lower snippet
> with "free_rproc" and just name the new snippet with something like
> "unmap_bootreg". It's then a bit easier to read the code that calls
> into those snippets (the reader easily know which cleanup snippet is
> invoked by each 'goto').
>
Agree. I will do the change.

> Thanks,
> Ohad.



-- 
Thanks

juan gutierrez

Re: [PATCH v2 2/3] OMAP4: iommu: fix irq and clock name for dsp-iommu

[Gutierrez, Juan]

Hi Ohad

On Sun, May 6, 2012 at 7:33 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Juan,
>
> On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
>> Irq and clock names were wrong for dsp iommu configuration
>> for omap4.
>
> Looks good.
>
>> Renamed tesla_ick to dsp_fck.
>
> If you're resubmitting the series, please indicate here why this
> change is needed (specifically let's mention commit 0e43327 "OMAP4:
> clock: Fix clock names and align with hwmod names").
>
Ok, I will mention the reason of this change in next series.

> Thanks,
> Ohad.



-- 
Thanks

juan gutierrez

[PATCH v2 2/3] OMAP4: iommu: fix irq and clock name for dsp-iommu

[Gutierrez, Juan]

Hi Ohad

On Sun, May 6, 2012 at 7:33 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Juan,
>
> On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
>> Irq and clock names were wrong for dsp iommu configuration
>> for omap4.
>
> Looks good.
>
>> Renamed tesla_ick to dsp_fck.
>
> If you're resubmitting the series, please indicate here why this
> change is needed (specifically let's mention commit 0e43327 "OMAP4:
> clock: Fix clock names and align with hwmod names").
>
Ok, I will mention the reason of this change in next series.

> Thanks,
> Ohad.



-- 
Thanks

juan gutierrez

Re: [PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Gutierrez, Juan]

Hi Ohad

On Sun, May 6, 2012 at 11:00 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Juan,
>
> On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
>> The machine-specific omap2_mbox_startup is called only once
>> to initialize the whole mbox module. Enabling mbox irq at
>> startup is only enabling the irq of the very first mailbox
>> instance created.
>>
>> The enable_irq function should be called every time a
>> new mbox instance is created,
>
> s/created/opened/
>
>> in the generic platform mailbox layer.
>
> This patch removes an omap2-specific code and then adds it to the
> "generic" layer, which also deals with omap1.
>
> Are we sure it's ok ?
>
> OMAP1 doesn't seem to enable its irq at this point: it doesn't even
> have a ->startup() handler (which actually somewhat implies it's been
> anyway broken for a long time now: omap_mbox_startup() seem to
> unhappily bail out if it doesn't find a machine-specific ->startup()
> handler).
>

In the ISR the mbox queue are used. If they are not ready a crash might happen.
So enabling_irq should be called after mbox queue allocation and after
ISR registration.
So I think for sure startup is not the right place. Besides that, its
reference counter is
preventing other mbox instances to enable irq's.

Startup it is now only used to enable pm runtime of the whole mbox module.

I think, if request_irq is called from the generic layer, it is kind
of logical to enable_irq
next to it.


>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>> index ad32621..ebc1ba5 100644
>> --- a/arch/arm/plat-omap/mailbox.c
>> +++ b/arch/arm/plat-omap/mailbox.c
>> @@ -282,6 +282,8 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
>>                }
>>                mbox->rxq = mq;
>>                mq->mbox = mbox;
>> +
>> +               mbox->ops->enable_irq(mbox, IRQ_RX);
>
> Might be nicer to use omap_mbox_enable_irq() here instead of reaching
> out for the internal ops.
>
Ok, yes I will use omap_mbox_enable_irq

> It also looks like there's a race here: omap_mbox_get() registers the
> notifier_block only after omap_mbox_startup() returns, which probably
> means there's a small window where an interrupt can be received
> without us invoking the user's notifier callback.
>
> This isn't related to your patch of course, but since you're touching
> this area, it might be nice if you can fix it (i.e. simply by
> registering the notifier_block before enabling the mbox's irq).
>
Ok I can include this fix.

>>        }
>>        mutex_unlock(&mbox_configured_lock);
>>        return 0;
>> @@ -305,6 +307,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
>>        mutex_lock(&mbox_configured_lock);
>>
>>        if (!--mbox->use_count) {
>> +               mbox->ops->disable_irq(mbox, IRQ_RX);
>
> omap_mbox_disable_irq() ?
>
Ok

> Thanks,
> Ohad.



-- 
Thanks

juan gutierrez

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Gutierrez, Juan]

Hi Ohad

On Sun, May 6, 2012 at 11:00 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Juan,
>
> On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@ti.com> wrote:
>> The machine-specific omap2_mbox_startup is called only once
>> to initialize the whole mbox module. Enabling mbox irq at
>> startup is only enabling the irq of the very first mailbox
>> instance created.
>>
>> The enable_irq function should be called every time a
>> new mbox instance is created,
>
> s/created/opened/
>
>> in the generic platform mailbox layer.
>
> This patch removes an omap2-specific code and then adds it to the
> "generic" layer, which also deals with omap1.
>
> Are we sure it's ok ?
>
> OMAP1 doesn't seem to enable its irq at this point: it doesn't even
> have a ->startup() handler (which actually somewhat implies it's been
> anyway broken for a long time now: omap_mbox_startup() seem to
> unhappily bail out if it doesn't find a machine-specific ->startup()
> handler).
>

In the ISR the mbox queue are used. If they are not ready a crash might happen.
So enabling_irq should be called after mbox queue allocation and after
ISR registration.
So I think for sure startup is not the right place. Besides that, its
reference counter is
preventing other mbox instances to enable irq's.

Startup it is now only used to enable pm runtime of the whole mbox module.

I think, if request_irq is called from the generic layer, it is kind
of logical to enable_irq
next to it.


>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>> index ad32621..ebc1ba5 100644
>> --- a/arch/arm/plat-omap/mailbox.c
>> +++ b/arch/arm/plat-omap/mailbox.c
>> @@ -282,6 +282,8 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
>> ? ? ? ? ? ? ? ?}
>> ? ? ? ? ? ? ? ?mbox->rxq = mq;
>> ? ? ? ? ? ? ? ?mq->mbox = mbox;
>> +
>> + ? ? ? ? ? ? ? mbox->ops->enable_irq(mbox, IRQ_RX);
>
> Might be nicer to use omap_mbox_enable_irq() here instead of reaching
> out for the internal ops.
>
Ok, yes I will use omap_mbox_enable_irq

> It also looks like there's a race here: omap_mbox_get() registers the
> notifier_block only after omap_mbox_startup() returns, which probably
> means there's a small window where an interrupt can be received
> without us invoking the user's notifier callback.
>
> This isn't related to your patch of course, but since you're touching
> this area, it might be nice if you can fix it (i.e. simply by
> registering the notifier_block before enabling the mbox's irq).
>
Ok I can include this fix.

>> ? ? ? ?}
>> ? ? ? ?mutex_unlock(&mbox_configured_lock);
>> ? ? ? ?return 0;
>> @@ -305,6 +307,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
>> ? ? ? ?mutex_lock(&mbox_configured_lock);
>>
>> ? ? ? ?if (!--mbox->use_count) {
>> + ? ? ? ? ? ? ? mbox->ops->disable_irq(mbox, IRQ_RX);
>
> omap_mbox_disable_irq() ?
>
Ok

> Thanks,
> Ohad.



-- 
Thanks

juan gutierrez

Re: [PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Ohad Ben-Cohen]

Hi Juan,

On Tue, May 8, 2012 at 1:09 AM, Gutierrez, Juan <jgutierrez@ti.com> wrote:
> Startup it is now only used to enable pm runtime of the whole mbox module.

Btw I really doubt we need to do that in a machine-specific handler,
or whether we really need this startup/shutdown abstraction at all.
But that's not related to your patch of course...

> I think, if request_irq is called from the generic layer, it is kind
> of logical to enable_irq next to it.

I somewhat agree, though I was wondering about the implications for
OMAP1, which didn't seem to enable_irq at startup at all. Not sure
there's much to do here (unless you have some OMAP1 hardware ? :).

Thanks,
Ohad.

[PATCH v2 1/3] omap: mailbox: enable mailbox irq per instance

[Ohad Ben-Cohen]

Hi Juan,

On Tue, May 8, 2012 at 1:09 AM, Gutierrez, Juan <jgutierrez@ti.com> wrote:
> Startup it is now only used to enable pm runtime of the whole mbox module.

Btw I really doubt we need to do that in a machine-specific handler,
or whether we really need this startup/shutdown abstraction at all.
But that's not related to your patch of course...

> I think, if request_irq is called from the generic layer, it is kind
> of logical to enable_irq next to it.

I somewhat agree, though I was wondering about the implications for
OMAP1, which didn't seem to enable_irq at startup at all. Not sure
there's much to do here (unless you have some OMAP1 hardware ? :).

Thanks,
Ohad.