[PATCHv3,2/9] ARM: OMAP2+: AM33XX: control: Add some control module registers and APIs

Commit Message

Dave Gerlach Aug. 6, 2013, 5:49 p.m. UTC

From: Vaibhav Bedia <[e-mail]>

Interacting with WKUP-M3 requires some more control
module register writes. Add the register offsets and
APIs to write to these.

Signed-off-by: Vaibhav Bedia <[e-mail]>
Signed-off-by: Dave Gerlach <[e-mail]>
---
 arch/arm/mach-omap2/control.c |   57 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/control.h |   54 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)

Comments

Russ Dill Aug. 8, 2013, 12:52 a.m. UTC | #1

On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach <[e-mail]> wrote:
> From: Vaibhav Bedia <[e-mail]>
>
> Interacting with WKUP-M3 requires some more control
> module register writes. Add the register offsets and
> APIs to write to these.
>
> Signed-off-by: Vaibhav Bedia <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>

Reviewed-by: Russ Dill <[e-mail]>

> ---
>  arch/arm/mach-omap2/control.c |   57 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/control.h |   54 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index 31e0dfe..934041a 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -605,3 +605,60 @@ int omap3_ctrl_save_padconf(void)
>  }
>
>  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
> +
> +#if defined(CONFIG_SOC_AM33XX) && defined(CONFIG_PM)
> +void am33xx_txev_eoi(void)
> +{
> +       omap_ctrl_writel(AM33XX_M3_TXEV_ACK, AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +void am33xx_txev_enable(void)
> +{
> +       omap_ctrl_writel(AM33XX_M3_TXEV_ENABLE, AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +/*
> + * Invalidate M3 firmware version before hardreset.
> + * Write invalid version in lower 4 nibbles of parameter
> + * register (ipc_regs + 0x8).
> + */
> +void am33xx_pm_version_clear(void)
> +{
> +       omap_ctrl_writel(0xffff0000, AM33XX_CONTROL_IPC_MSG_REG2);
> +}
> +
> +int am33xx_pm_version_get(void)
> +{
> +       return omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG2) & M3_VERSION_MASK;
> +}
> +
> +void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
> +{
> +       omap_ctrl_writel(data->resume_addr, AM33XX_CONTROL_IPC_MSG_REG0);
> +       omap_ctrl_writel(data->sleep_mode, AM33XX_CONTROL_IPC_MSG_REG1);
> +       omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
> +       omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
> +       omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
> +}
> +
> +int am33xx_pm_status(void)
> +{
> +       int i;
> +
> +       i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG1);
> +       i &= IPC_RESP_MASK;
> +       i >>= __ffs(IPC_RESP_MASK);
> +
> +       return i;
> +}
> +
> +int am33xx_pm_wake_src(void)
> +{
> +       int i;
> +
> +       i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG6);
> +       i &= 0xff;
> +
> +       return i;
> +}
> +#endif /* CONFIG_SOC_AM33XX && CONFIG_PM */
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index f7d7c2e..9be587c 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -370,6 +370,22 @@
>  #define AM33XX_DEV_FEATURE             0x604
>  #define AM33XX_SGX_MASK                        BIT(29)
>
> +/* AM33XX M3_TXEV_EOI register */
> +#define AM33XX_CONTROL_M3_TXEV_EOI     0x1324
> +
> +#define AM33XX_M3_TXEV_ACK             (0x1 << 0)
> +#define AM33XX_M3_TXEV_ENABLE          (0x0 << 0)
> +
> +/* AM33XX IPC message registers */
> +#define AM33XX_CONTROL_IPC_MSG_REG0    0x1328
> +#define AM33XX_CONTROL_IPC_MSG_REG1    0x132C
> +#define AM33XX_CONTROL_IPC_MSG_REG2    0x1330
> +#define AM33XX_CONTROL_IPC_MSG_REG3    0x1334
> +#define AM33XX_CONTROL_IPC_MSG_REG4    0x1338
> +#define AM33XX_CONTROL_IPC_MSG_REG5    0x133C
> +#define AM33XX_CONTROL_IPC_MSG_REG6    0x1340
> +#define AM33XX_CONTROL_IPC_MSG_REG7    0x1344
> +
>  /* CONTROL OMAP STATUS register to identify OMAP3 features */
>  #define OMAP3_CONTROL_OMAP_STATUS      0x044c
>
> @@ -429,6 +445,44 @@ extern void omap3630_ctrl_disable_rta(void);
>  extern int omap3_ctrl_save_padconf(void);
>  extern void omap2_set_globals_control(void __iomem *ctrl,
>                                       void __iomem *ctrl_pad);
> +struct am33xx_ipc_data {
> +       u32 resume_addr;
> +       u32 sleep_mode;
> +       u32 param1;
> +       u32 param2;
> +       u32 param3;
> +       u32 param4;
> +       u32 param5;
> +       u32 param6;
> +};
> +
> +#define IPC_RESP_SHIFT                 16
> +#define IPC_RESP_MASK                  (0xffff << 16)
> +
> +#define M3_VERSION_SHIFT               0
> +#define M3_VERSION_MASK                        (0xffff << 0)
> +
> +/*
> + * 9-4 = VTT GPIO PIN (6 Bits)
> + *   3 = VTT Status (1 Bit)
> + * 2-0 = Memory Type (2 Bits)
> +*/
> +#define MEM_TYPE_SHIFT         (0x0)
> +#define MEM_TYPE_MASK          (0x7 << 0)
> +#define VTT_STAT_SHIFT         (0x3)
> +#define VTT_STAT_MASK          (0x1 << 3)
> +#define VTT_GPIO_PIN_SHIFT     (0x4)
> +#define VTT_GPIO_PIN_MASK      (0x2f << 4)
> +
> +extern void am33xx_wkup_m3_ipc_cmd(struct am33xx_ipc_data *data);
> +extern void am33xx_txev_eoi(void);
> +extern void am33xx_txev_enable(void);
> +extern void am33xx_pm_version_clear(void);
> +extern int am33xx_pm_version_get(void);
> +extern void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data);
> +extern int am33xx_pm_status(void);
> +extern int am33xx_pm_wake_src(void);
> +
>  #else
>  #define omap_ctrl_base_get()           0
>  #define omap_ctrl_readb(x)             0
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Santosh Shilimkar Aug. 8, 2013, 1:44 p.m. UTC | #2

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia <[e-mail]>
> 
> Interacting with WKUP-M3 requires some more control
> module register writes. Add the register offsets and
> APIs to write to these.
> 
> Signed-off-by: Vaibhav Bedia <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>
> ---
>  arch/arm/mach-omap2/control.c |   57 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/control.h |   54 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index 31e0dfe..934041a 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -605,3 +605,60 @@ int omap3_ctrl_save_padconf(void)
>  }
>  
>  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
> +
> +#if defined(CONFIG_SOC_AM33XX) && defined(CONFIG_PM)
> +void am33xx_txev_eoi(void)
> +{
> +	omap_ctrl_writel(AM33XX_M3_TXEV_ACK, AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +void am33xx_txev_enable(void)
> +{
> +	omap_ctrl_writel(AM33XX_M3_TXEV_ENABLE, AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +/*
> + * Invalidate M3 firmware version before hardreset.
> + * Write invalid version in lower 4 nibbles of parameter
> + * register (ipc_regs + 0x8).
> + */
> +void am33xx_pm_version_clear(void)
> +{
> +	omap_ctrl_writel(0xffff0000, AM33XX_CONTROL_IPC_MSG_REG2);
> +}
> +
> +int am33xx_pm_version_get(void)
> +{
> +	return omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG2) & M3_VERSION_MASK;
> +}
> +
> +void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
> +{
> +	omap_ctrl_writel(data->resume_addr, AM33XX_CONTROL_IPC_MSG_REG0);
> +	omap_ctrl_writel(data->sleep_mode, AM33XX_CONTROL_IPC_MSG_REG1);
> +	omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
> +	omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
> +	omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
> +}
> +
> +int am33xx_pm_status(void)
> +{
> +	int i;
> +
> +	i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG1);
> +	i &= IPC_RESP_MASK;
> +	i >>= __ffs(IPC_RESP_MASK);
> +
> +	return i;
> +}
> +
> +int am33xx_pm_wake_src(void)
> +{
> +	int i;
> +
> +	i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG6);
> +	i &= 0xff;
> +
> +	return i;
> +}
> +#endif /* CONFIG_SOC_AM33XX && CONFIG_PM */
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index f7d7c2e..9be587c 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -370,6 +370,22 @@
>  #define AM33XX_DEV_FEATURE		0x604
>  #define AM33XX_SGX_MASK			BIT(29)
>  
> +/* AM33XX M3_TXEV_EOI register */
> +#define AM33XX_CONTROL_M3_TXEV_EOI	0x1324
> +
> +#define AM33XX_M3_TXEV_ACK		(0x1 << 0)
> +#define AM33XX_M3_TXEV_ENABLE		(0x0 << 0)
> +
> +/* AM33XX IPC message registers */
> +#define AM33XX_CONTROL_IPC_MSG_REG0	0x1328
> +#define AM33XX_CONTROL_IPC_MSG_REG1	0x132C
> +#define AM33XX_CONTROL_IPC_MSG_REG2	0x1330
> +#define AM33XX_CONTROL_IPC_MSG_REG3	0x1334
> +#define AM33XX_CONTROL_IPC_MSG_REG4	0x1338
> +#define AM33XX_CONTROL_IPC_MSG_REG5	0x133C
> +#define AM33XX_CONTROL_IPC_MSG_REG6	0x1340
> +#define AM33XX_CONTROL_IPC_MSG_REG7	0x1344
> +
>  /* CONTROL OMAP STATUS register to identify OMAP3 features */
>  #define OMAP3_CONTROL_OMAP_STATUS	0x044c
>  
> @@ -429,6 +445,44 @@ extern void omap3630_ctrl_disable_rta(void);
>  extern int omap3_ctrl_save_padconf(void);
>  extern void omap2_set_globals_control(void __iomem *ctrl,
>  				      void __iomem *ctrl_pad);
> +struct am33xx_ipc_data {
> +	u32 resume_addr;
> +	u32 sleep_mode;
> +	u32 param1;
> +	u32 param2;
> +	u32 param3;
> +	u32 param4;
> +	u32 param5;
> +	u32 param6;
> +};
> +
> +#define IPC_RESP_SHIFT			16
> +#define IPC_RESP_MASK			(0xffff << 16)
> +
> +#define M3_VERSION_SHIFT		0
> +#define M3_VERSION_MASK			(0xffff << 0)
> +
> +/*
> + * 9-4 = VTT GPIO PIN (6 Bits)
> + *   3 = VTT Status (1 Bit)
> + * 2-0 = Memory Type (2 Bits)
> +*/
> +#define MEM_TYPE_SHIFT		(0x0)
> +#define MEM_TYPE_MASK		(0x7 << 0)
> +#define VTT_STAT_SHIFT		(0x3)
> +#define VTT_STAT_MASK		(0x1 << 3)
> +#define VTT_GPIO_PIN_SHIFT	(0x4)
> +#define VTT_GPIO_PIN_MASK	(0x2f << 4)
> +
> +extern void am33xx_wkup_m3_ipc_cmd(struct am33xx_ipc_data *data);
> +extern void am33xx_txev_eoi(void);
> +extern void am33xx_txev_enable(void);
> +extern void am33xx_pm_version_clear(void);
> +extern int am33xx_pm_version_get(void);
> +extern void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data);
> +extern int am33xx_pm_status(void);
> +extern int am33xx_pm_wake_src(void);
> +
Lets address the above better. I don't see a need of 8 functions
exported doing one or 2 register writes.

Look M3 based handling is going to be there on future SOCs
as well and this kind of handling of IPC is very short cited.

Probably we should have a separate driver for M3 in linux which
can have all this local code instead of all these exports.

Regards,
Santosh

Dave Gerlach Aug. 8, 2013, 4:16 p.m. UTC | #3

On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
> On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
>> From: Vaibhav Bedia <[e-mail]>
>>
>> Interacting with WKUP-M3 requires some more control
>> module register writes. Add the register offsets and
>> APIs to write to these.
>>
>> Signed-off-by: Vaibhav Bedia <[e-mail]>
>> Signed-off-by: Dave Gerlach <[e-mail]>
>> ---
>>   arch/arm/mach-omap2/control.c |   57 +++++++++++++++++++++++++++++++++++++++++
>>   arch/arm/mach-omap2/control.h |   54 ++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 111 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
>> index 31e0dfe..934041a 100644
>> --- a/arch/arm/mach-omap2/control.c
>> +++ b/arch/arm/mach-omap2/control.c
>> @@ -605,3 +605,60 @@ int omap3_ctrl_save_padconf(void)
>>   }
>>
>>   #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
>> +
>> +#if defined(CONFIG_SOC_AM33XX) && defined(CONFIG_PM)
>> +void am33xx_txev_eoi(void)
>> +{
>> +	omap_ctrl_writel(AM33XX_M3_TXEV_ACK, AM33XX_CONTROL_M3_TXEV_EOI);
>> +}
>> +
>> +void am33xx_txev_enable(void)
>> +{
>> +	omap_ctrl_writel(AM33XX_M3_TXEV_ENABLE, AM33XX_CONTROL_M3_TXEV_EOI);
>> +}
>> +
>> +/*
>> + * Invalidate M3 firmware version before hardreset.
>> + * Write invalid version in lower 4 nibbles of parameter
>> + * register (ipc_regs + 0x8).
>> + */
>> +void am33xx_pm_version_clear(void)
>> +{
>> +	omap_ctrl_writel(0xffff0000, AM33XX_CONTROL_IPC_MSG_REG2);
>> +}
>> +
>> +int am33xx_pm_version_get(void)
>> +{
>> +	return omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG2) & M3_VERSION_MASK;
>> +}
>> +
>> +void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
>> +{
>> +	omap_ctrl_writel(data->resume_addr, AM33XX_CONTROL_IPC_MSG_REG0);
>> +	omap_ctrl_writel(data->sleep_mode, AM33XX_CONTROL_IPC_MSG_REG1);
>> +	omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
>> +	omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
>> +	omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
>> +}
>> +
>> +int am33xx_pm_status(void)
>> +{
>> +	int i;
>> +
>> +	i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG1);
>> +	i &= IPC_RESP_MASK;
>> +	i >>= __ffs(IPC_RESP_MASK);
>> +
>> +	return i;
>> +}
>> +
>> +int am33xx_pm_wake_src(void)
>> +{
>> +	int i;
>> +
>> +	i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG6);
>> +	i &= 0xff;
>> +
>> +	return i;
>> +}
>> +#endif /* CONFIG_SOC_AM33XX && CONFIG_PM */
>> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
>> index f7d7c2e..9be587c 100644
>> --- a/arch/arm/mach-omap2/control.h
>> +++ b/arch/arm/mach-omap2/control.h
>> @@ -370,6 +370,22 @@
>>   #define AM33XX_DEV_FEATURE		0x604
>>   #define AM33XX_SGX_MASK			BIT(29)
>>
>> +/* AM33XX M3_TXEV_EOI register */
>> +#define AM33XX_CONTROL_M3_TXEV_EOI	0x1324
>> +
>> +#define AM33XX_M3_TXEV_ACK		(0x1 << 0)
>> +#define AM33XX_M3_TXEV_ENABLE		(0x0 << 0)
>> +
>> +/* AM33XX IPC message registers */
>> +#define AM33XX_CONTROL_IPC_MSG_REG0	0x1328
>> +#define AM33XX_CONTROL_IPC_MSG_REG1	0x132C
>> +#define AM33XX_CONTROL_IPC_MSG_REG2	0x1330
>> +#define AM33XX_CONTROL_IPC_MSG_REG3	0x1334
>> +#define AM33XX_CONTROL_IPC_MSG_REG4	0x1338
>> +#define AM33XX_CONTROL_IPC_MSG_REG5	0x133C
>> +#define AM33XX_CONTROL_IPC_MSG_REG6	0x1340
>> +#define AM33XX_CONTROL_IPC_MSG_REG7	0x1344
>> +
>>   /* CONTROL OMAP STATUS register to identify OMAP3 features */
>>   #define OMAP3_CONTROL_OMAP_STATUS	0x044c
>>
>> @@ -429,6 +445,44 @@ extern void omap3630_ctrl_disable_rta(void);
>>   extern int omap3_ctrl_save_padconf(void);
>>   extern void omap2_set_globals_control(void __iomem *ctrl,
>>   				      void __iomem *ctrl_pad);
>> +struct am33xx_ipc_data {
>> +	u32 resume_addr;
>> +	u32 sleep_mode;
>> +	u32 param1;
>> +	u32 param2;
>> +	u32 param3;
>> +	u32 param4;
>> +	u32 param5;
>> +	u32 param6;
>> +};
>> +
>> +#define IPC_RESP_SHIFT			16
>> +#define IPC_RESP_MASK			(0xffff << 16)
>> +
>> +#define M3_VERSION_SHIFT		0
>> +#define M3_VERSION_MASK			(0xffff << 0)
>> +
>> +/*
>> + * 9-4 = VTT GPIO PIN (6 Bits)
>> + *   3 = VTT Status (1 Bit)
>> + * 2-0 = Memory Type (2 Bits)
>> +*/
>> +#define MEM_TYPE_SHIFT		(0x0)
>> +#define MEM_TYPE_MASK		(0x7 << 0)
>> +#define VTT_STAT_SHIFT		(0x3)
>> +#define VTT_STAT_MASK		(0x1 << 3)
>> +#define VTT_GPIO_PIN_SHIFT	(0x4)
>> +#define VTT_GPIO_PIN_MASK	(0x2f << 4)
>> +
>> +extern void am33xx_wkup_m3_ipc_cmd(struct am33xx_ipc_data *data);
>> +extern void am33xx_txev_eoi(void);
>> +extern void am33xx_txev_enable(void);
>> +extern void am33xx_pm_version_clear(void);
>> +extern int am33xx_pm_version_get(void);
>> +extern void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data);
>> +extern int am33xx_pm_status(void);
>> +extern int am33xx_pm_wake_src(void);
>> +
> Lets address the above better. I don't see a need of 8 functions
> exported doing one or 2 register writes.
>
> Look M3 based handling is going to be there on future SOCs
> as well and this kind of handling of IPC is very short cited.
>

The idea here was to move all control module register accesses into one 
file in planning of implementing a driver for the control module itself 
in the future.

> Probably we should have a separate driver for M3 in linux which
> can have all this local code instead of all these exports.

The wkup_m3 code has been moved to a small driver found in patch 8 of 
this series, would it better to move this code there rather than with 
the rest of the control module code?

>
> Regards,
> Santosh
>

Tony Lindgren Aug. 9, 2013, 5:11 a.m. UTC | #4

* Dave Gerlach <[e-mail]> [130808 09:23]:
> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
> >Lets address the above better. I don't see a need of 8 functions
> >exported doing one or 2 register writes.
> >
> >Look M3 based handling is going to be there on future SOCs
> >as well and this kind of handling of IPC is very short cited.
> >
> 
> The idea here was to move all control module register accesses into
> one file in planning of implementing a driver for the control module
> itself in the future.
> 
> >Probably we should have a separate driver for M3 in linux which
> >can have all this local code instead of all these exports.
> 
> The wkup_m3 code has been moved to a small driver found in patch 8
> of this series, would it better to move this code there rather than
> with the rest of the control module code?

Please make everything you can into regular device drivers.

We still have some dependencies to mach-omap2 code for PRCM
for example, but we're trying to get all that to live in
drivers.

So for new pieces, let's not add further dependencies to
complicate moving things to drivers.

Regards,

Tony

Dave Gerlach Aug. 9, 2013, 8:55 p.m. UTC | #5

On 08/09/2013 12:11 AM, Tony Lindgren wrote:
> * Dave Gerlach <[e-mail]> [130808 09:23]:
>> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
>>> Lets address the above better. I don't see a need of 8 functions
>>> exported doing one or 2 register writes.
>>>
>>> Look M3 based handling is going to be there on future SOCs
>>> as well and this kind of handling of IPC is very short cited.
>>>
>>
>> The idea here was to move all control module register accesses into
>> one file in planning of implementing a driver for the control module
>> itself in the future.
>>
>>> Probably we should have a separate driver for M3 in linux which
>>> can have all this local code instead of all these exports.
>>
>> The wkup_m3 code has been moved to a small driver found in patch 8
>> of this series, would it better to move this code there rather than
>> with the rest of the control module code?
>
> Please make everything you can into regular device drivers.
>
> We still have some dependencies to mach-omap2 code for PRCM
> for example, but we're trying to get all that to live in
> drivers.
>
> So for new pieces, let's not add further dependencies to
> complicate moving things to drivers.
>
> Regards,
>
> Tony
>

Ok I will go ahead and pull the control module code that handles IPC 
into the wkup_m3 driver. The wkup_m3.c file is still present in 
mach-omap2 as the right location for it wasn't decided in the last RFC. 
Any thoughts on a good location for it?

Regards,
Dave

Tony Lindgren Aug. 12, 2013, 7:54 a.m. UTC | #6

* Dave Gerlach <[e-mail]> [130809 14:02]:
>
> Ok I will go ahead and pull the control module code that handles IPC
> into the wkup_m3 driver. The wkup_m3.c file is still present in
> mach-omap2 as the right location for it wasn't decided in the last
> RFC. Any thoughts on a good location for it?

Well maybe try to think how to use these features in a Linux
generic way without having to export tons of custom functions?

Regards,

Tony

Kevin Hilman Aug. 12, 2013, 7:17 p.m. UTC | #7

Dave Gerlach <[e-mail]> writes:

> On 08/09/2013 12:11 AM, Tony Lindgren wrote:
>> * Dave Gerlach <[e-mail]> [130808 09:23]:
>>> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
>>>> Lets address the above better. I don't see a need of 8 functions
>>>> exported doing one or 2 register writes.
>>>>
>>>> Look M3 based handling is going to be there on future SOCs
>>>> as well and this kind of handling of IPC is very short cited.
>>>>
>>>
>>> The idea here was to move all control module register accesses into
>>> one file in planning of implementing a driver for the control module
>>> itself in the future.
>>>
>>>> Probably we should have a separate driver for M3 in linux which
>>>> can have all this local code instead of all these exports.
>>>
>>> The wkup_m3 code has been moved to a small driver found in patch 8
>>> of this series, would it better to move this code there rather than
>>> with the rest of the control module code?
>>
>> Please make everything you can into regular device drivers.
>>
>> We still have some dependencies to mach-omap2 code for PRCM
>> for example, but we're trying to get all that to live in
>> drivers.
>>
>> So for new pieces, let's not add further dependencies to
>> complicate moving things to drivers.
>>
>> Regards,
>>
>> Tony
>>
>
> Ok I will go ahead and pull the control module code that handles IPC
> into the wkup_m3 driver. 

Any control module register access still needs to stay in control.c.

> The wkup_m3.c file is still present in mach-omap2 as the right
> location for it wasn't decided in the last RFC. Any thoughts on a good
> location for it?

I raised this also in earlier reviews, but don't remember the if it was
answered... 

Why can't we handle the wkup_m3 using remoteproc/rpmsg instead of
creating another little driver that duplicates communication with the
M3.  Note also that the firmware load part would also be provided by
remoteproc/rpmsg.

Kevin

Dave Gerlach Aug. 12, 2013, 9:40 p.m. UTC | #8

On 08/12/2013 02:17 PM, Kevin Hilman wrote:
> Dave Gerlach <[e-mail]> writes:
>
>> On 08/09/2013 12:11 AM, Tony Lindgren wrote:
>>> * Dave Gerlach <[e-mail]> [130808 09:23]:
>>>> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
>>>>> Lets address the above better. I don't see a need of 8 functions
>>>>> exported doing one or 2 register writes.
>>>>>
>>>>> Look M3 based handling is going to be there on future SOCs
>>>>> as well and this kind of handling of IPC is very short cited.
>>>>>
>>>>
>>>> The idea here was to move all control module register accesses into
>>>> one file in planning of implementing a driver for the control module
>>>> itself in the future.
>>>>
>>>>> Probably we should have a separate driver for M3 in linux which
>>>>> can have all this local code instead of all these exports.
>>>>
>>>> The wkup_m3 code has been moved to a small driver found in patch 8
>>>> of this series, would it better to move this code there rather than
>>>> with the rest of the control module code?
>>>
>>> Please make everything you can into regular device drivers.
>>>
>>> We still have some dependencies to mach-omap2 code for PRCM
>>> for example, but we're trying to get all that to live in
>>> drivers.
>>>
>>> So for new pieces, let's not add further dependencies to
>>> complicate moving things to drivers.
>>>
>>> Regards,
>>>
>>> Tony
>>>
>>
>> Ok I will go ahead and pull the control module code that handles IPC
>> into the wkup_m3 driver.
>
> Any control module register access still needs to stay in control.c.
>
>> The wkup_m3.c file is still present in mach-omap2 as the right
>> location for it wasn't decided in the last RFC. Any thoughts on a good
>> location for it?
>
> I raised this also in earlier reviews, but don't remember the if it was
> answered...
>
> Why can't we handle the wkup_m3 using remoteproc/rpmsg instead of
> creating another little driver that duplicates communication with the
> M3.  Note also that the firmware load part would also be provided by
> remoteproc/rpmsg.

Looping Suman Anna who handled the IPC patches this patch set is based 
on top of...

For the wkup_m3, the mailbox isn't used in a traditional manner. It's 
only used with a dummy write to trigger an interrupt from the A8 to the 
M3 and then communication happens in IPC registers within the control 
module. No messages are actually sent through the mailbox in either 
direction so that's why it was done this way rather than bring in full 
support for the mailbox.

>
> Kevin
>

Kevin Hilman Aug. 13, 2013, 2:29 p.m. UTC | #9

Dave Gerlach <[e-mail]> writes:

> On 08/12/2013 02:17 PM, Kevin Hilman wrote:
>> Dave Gerlach <[e-mail]> writes:
>>
>>> On 08/09/2013 12:11 AM, Tony Lindgren wrote:
>>>> * Dave Gerlach <[e-mail]> [130808 09:23]:
>>>>> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
>>>>>> Lets address the above better. I don't see a need of 8 functions
>>>>>> exported doing one or 2 register writes.
>>>>>>
>>>>>> Look M3 based handling is going to be there on future SOCs
>>>>>> as well and this kind of handling of IPC is very short cited.
>>>>>>
>>>>>
>>>>> The idea here was to move all control module register accesses into
>>>>> one file in planning of implementing a driver for the control module
>>>>> itself in the future.
>>>>>
>>>>>> Probably we should have a separate driver for M3 in linux which
>>>>>> can have all this local code instead of all these exports.
>>>>>
>>>>> The wkup_m3 code has been moved to a small driver found in patch 8
>>>>> of this series, would it better to move this code there rather than
>>>>> with the rest of the control module code?
>>>>
>>>> Please make everything you can into regular device drivers.
>>>>
>>>> We still have some dependencies to mach-omap2 code for PRCM
>>>> for example, but we're trying to get all that to live in
>>>> drivers.
>>>>
>>>> So for new pieces, let's not add further dependencies to
>>>> complicate moving things to drivers.
>>>>
>>>> Regards,
>>>>
>>>> Tony
>>>>
>>>
>>> Ok I will go ahead and pull the control module code that handles IPC
>>> into the wkup_m3 driver.
>>
>> Any control module register access still needs to stay in control.c.
>>
>>> The wkup_m3.c file is still present in mach-omap2 as the right
>>> location for it wasn't decided in the last RFC. Any thoughts on a good
>>> location for it?
>>
>> I raised this also in earlier reviews, but don't remember the if it was
>> answered...
>>
>> Why can't we handle the wkup_m3 using remoteproc/rpmsg instead of
>> creating another little driver that duplicates communication with the
>> M3.  Note also that the firmware load part would also be provided by
>> remoteproc/rpmsg.
>
> Looping Suman Anna who handled the IPC patches this patch set is based
> on top of...
>
> For the wkup_m3, the mailbox isn't used in a traditional manner. It's
> only used with a dummy write to trigger an interrupt from the A8 to
> the M3 and then communication happens in IPC registers within the
> control module. No messages are actually sent through the mailbox in
> either direction so that's why it was done this way rather than bring
> in full support for the mailbox.

I don't believe remoteproc/rpmsg forces you to use the mailbox for
communication, and can use other IPC mechanisms.  This still sounds
cleaner than reinventing remoteproc/rpmsg because of slight variations.

The linux way is to use and extend what is already there, and if it's
extended to the breaking point, then make a case for why something new
is needed.

Kevin

Santosh Shilimkar Aug. 13, 2013, 3:08 p.m. UTC | #10

On Tuesday 13 August 2013 10:29 AM, Kevin Hilman wrote:
> Dave Gerlach <[e-mail]> writes:
> 
>> On 08/12/2013 02:17 PM, Kevin Hilman wrote:
>>> Dave Gerlach <[e-mail]> writes:
>>>
>>>> On 08/09/2013 12:11 AM, Tony Lindgren wrote:
>>>>> * Dave Gerlach <[e-mail]> [130808 09:23]:
>>>>>> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
>>>>>>> Lets address the above better. I don't see a need of 8 functions
>>>>>>> exported doing one or 2 register writes.
>>>>>>>
>>>>>>> Look M3 based handling is going to be there on future SOCs
>>>>>>> as well and this kind of handling of IPC is very short cited.
>>>>>>>
>>>>>>
>>>>>> The idea here was to move all control module register accesses into
>>>>>> one file in planning of implementing a driver for the control module
>>>>>> itself in the future.
>>>>>>
>>>>>>> Probably we should have a separate driver for M3 in linux which
>>>>>>> can have all this local code instead of all these exports.
>>>>>>
>>>>>> The wkup_m3 code has been moved to a small driver found in patch 8
>>>>>> of this series, would it better to move this code there rather than
>>>>>> with the rest of the control module code?
>>>>>
>>>>> Please make everything you can into regular device drivers.
>>>>>
>>>>> We still have some dependencies to mach-omap2 code for PRCM
>>>>> for example, but we're trying to get all that to live in
>>>>> drivers.
>>>>>
>>>>> So for new pieces, let's not add further dependencies to
>>>>> complicate moving things to drivers.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tony
>>>>>
>>>>
>>>> Ok I will go ahead and pull the control module code that handles IPC
>>>> into the wkup_m3 driver.
>>>
>>> Any control module register access still needs to stay in control.c.
>>>
>>>> The wkup_m3.c file is still present in mach-omap2 as the right
>>>> location for it wasn't decided in the last RFC. Any thoughts on a good
>>>> location for it?
>>>
>>> I raised this also in earlier reviews, but don't remember the if it was
>>> answered...
>>>
>>> Why can't we handle the wkup_m3 using remoteproc/rpmsg instead of
>>> creating another little driver that duplicates communication with the
>>> M3.  Note also that the firmware load part would also be provided by
>>> remoteproc/rpmsg.
>>
>> Looping Suman Anna who handled the IPC patches this patch set is based
>> on top of...
>>
>> For the wkup_m3, the mailbox isn't used in a traditional manner. It's
>> only used with a dummy write to trigger an interrupt from the A8 to
>> the M3 and then communication happens in IPC registers within the
>> control module. No messages are actually sent through the mailbox in
>> either direction so that's why it was done this way rather than bring
>> in full support for the mailbox.
> 
> I don't believe remoteproc/rpmsg forces you to use the mailbox for
> communication, and can use other IPC mechanisms.  This still sounds
> cleaner than reinventing remoteproc/rpmsg because of slight variations.
> 
> The linux way is to use and extend what is already there, and if it's
> extended to the breaking point, then make a case for why something new
> is needed.
> 
While I agree to re-use frameworks, am strongly against use of IPC for
the power management which is time sensitive. Imagine a scenario where
ACPI is asked to go through remoteproc/ipc for talking to firmware.
If it takes 10 to 15 uS just to send a command to firmware to change
some control, that just tells something is not right and thats what
IPC will do.

ARM world is also moving towards that by standardizing some of these
through (read PSCI) and thats the way to go in general. Specifically
for this series, I am also against having tons of exports and all
of that should be extracted properly but remoteproc is not going
to be the way. Firmware download has to happen much earlier(ROM
path) so thats not the requirement where we could have used the
remoteproc firmware download feature.

Regards,
Santosh

Kevin Hilman Aug. 13, 2013, 4:19 p.m. UTC | #11

+ Ohad

Santosh Shilimkar <[e-mail]> writes:

> On Tuesday 13 August 2013 10:29 AM, Kevin Hilman wrote:
>> Dave Gerlach <[e-mail]> writes:
>> 
>>> On 08/12/2013 02:17 PM, Kevin Hilman wrote:
>>>> Dave Gerlach <[e-mail]> writes:
>>>>
>>>>> On 08/09/2013 12:11 AM, Tony Lindgren wrote:
>>>>>> * Dave Gerlach <[e-mail]> [130808 09:23]:
>>>>>>> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
>>>>>>>> Lets address the above better. I don't see a need of 8 functions
>>>>>>>> exported doing one or 2 register writes.
>>>>>>>>
>>>>>>>> Look M3 based handling is going to be there on future SOCs
>>>>>>>> as well and this kind of handling of IPC is very short cited.
>>>>>>>>
>>>>>>>
>>>>>>> The idea here was to move all control module register accesses into
>>>>>>> one file in planning of implementing a driver for the control module
>>>>>>> itself in the future.
>>>>>>>
>>>>>>>> Probably we should have a separate driver for M3 in linux which
>>>>>>>> can have all this local code instead of all these exports.
>>>>>>>
>>>>>>> The wkup_m3 code has been moved to a small driver found in patch 8
>>>>>>> of this series, would it better to move this code there rather than
>>>>>>> with the rest of the control module code?
>>>>>>
>>>>>> Please make everything you can into regular device drivers.
>>>>>>
>>>>>> We still have some dependencies to mach-omap2 code for PRCM
>>>>>> for example, but we're trying to get all that to live in
>>>>>> drivers.
>>>>>>
>>>>>> So for new pieces, let's not add further dependencies to
>>>>>> complicate moving things to drivers.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Tony
>>>>>>
>>>>>
>>>>> Ok I will go ahead and pull the control module code that handles IPC
>>>>> into the wkup_m3 driver.
>>>>
>>>> Any control module register access still needs to stay in control.c.
>>>>
>>>>> The wkup_m3.c file is still present in mach-omap2 as the right
>>>>> location for it wasn't decided in the last RFC. Any thoughts on a good
>>>>> location for it?
>>>>
>>>> I raised this also in earlier reviews, but don't remember the if it was
>>>> answered...
>>>>
>>>> Why can't we handle the wkup_m3 using remoteproc/rpmsg instead of
>>>> creating another little driver that duplicates communication with the
>>>> M3.  Note also that the firmware load part would also be provided by
>>>> remoteproc/rpmsg.
>>>
>>> Looping Suman Anna who handled the IPC patches this patch set is based
>>> on top of...
>>>
>>> For the wkup_m3, the mailbox isn't used in a traditional manner. It's
>>> only used with a dummy write to trigger an interrupt from the A8 to
>>> the M3 and then communication happens in IPC registers within the
>>> control module. No messages are actually sent through the mailbox in
>>> either direction so that's why it was done this way rather than bring
>>> in full support for the mailbox.
>> 
>> I don't believe remoteproc/rpmsg forces you to use the mailbox for
>> communication, and can use other IPC mechanisms.  This still sounds
>> cleaner than reinventing remoteproc/rpmsg because of slight variations.
>> 
>> The linux way is to use and extend what is already there, and if it's
>> extended to the breaking point, then make a case for why something new
>> is needed.
>> 
> While I agree to re-use frameworks, am strongly against use of IPC for
> the power management which is time sensitive. 

Maybe I'm misunderstanding what you mean by IPC, but I'm not sure how
you can be against IPC here.  Communication with M3 requires it.

The only question is what framework to use: create a new wkup_m3 driver
which creates yet another set of IPC APIs as well as a seprate firmware
load path?  Or reuse remoteproc/rpmsg which provides both.

> Imagine a scenario where
> ACPI is asked to go through remoteproc/ipc for talking to firmware.
> If it takes 10 to 15 uS just to send a command to firmware to change
> some control, that just tells something is not right and thats what
> IPC will do.

Sounds like what you're saying is that remoteproc/rpmsg will be too slow
for this?  Is there any evidence of that?  Even if it is slow, it sounds
like a perfect opportuntity to enhance it.

Also, if speed is that much of a concern concern here, I wonder why
there's a 500msec timeout in in the communication between MPU and M3 in
the current code.

> ARM world is also moving towards that by standardizing some of these
> through (read PSCI) and thats the way to go in general. 

Agreed, but I'm not sure (yet) about enforcing PSCI on legacy platforms
that don't support it natively.  Are you saying that the AM33xx firmware
should be converted to be PSCI compliant?  Admittedly, I haven't read
the PSCI spec closely, but I'm wondering if the current role splitting
between MPU and M3 fits well with PSCI. 

> Specifically for this series, I am also against having tons of exports
> and all of that should be extracted properly but remoteproc is not
> going to be the way. Firmware download has to happen much earlier(ROM
> path) so thats not the requirement where we could have used the
> remoteproc firmware download feature.

At least for AM33xx, I don't see any requirement that firmware download
happens early (or at all.)  PM will simply be disabled until firmware is
available (and running.)

Kevin

Santosh Shilimkar Aug. 13, 2013, 6:18 p.m. UTC | #12

On Tuesday 13 August 2013 12:19 PM, Kevin Hilman wrote:
> + Ohad
> 
> Santosh Shilimkar <[e-mail]> writes:
> 
>> On Tuesday 13 August 2013 10:29 AM, Kevin Hilman wrote:
>>> Dave Gerlach <[e-mail]> writes:
>>>
>>>> On 08/12/2013 02:17 PM, Kevin Hilman wrote:
>>>>> Dave Gerlach <[e-mail]> writes:
>>>>>
>>>>>> On 08/09/2013 12:11 AM, Tony Lindgren wrote:
>>>>>>> * Dave Gerlach <[e-mail]> [130808 09:23]:
>>>>>>>> On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:
>>>>>>>>> Lets address the above better. I don't see a need of 8 functions
>>>>>>>>> exported doing one or 2 register writes.
>>>>>>>>>
>>>>>>>>> Look M3 based handling is going to be there on future SOCs
>>>>>>>>> as well and this kind of handling of IPC is very short cited.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The idea here was to move all control module register accesses into
>>>>>>>> one file in planning of implementing a driver for the control module
>>>>>>>> itself in the future.
>>>>>>>>
>>>>>>>>> Probably we should have a separate driver for M3 in linux which
>>>>>>>>> can have all this local code instead of all these exports.
>>>>>>>>
>>>>>>>> The wkup_m3 code has been moved to a small driver found in patch 8
>>>>>>>> of this series, would it better to move this code there rather than
>>>>>>>> with the rest of the control module code?
>>>>>>>
>>>>>>> Please make everything you can into regular device drivers.
>>>>>>>
>>>>>>> We still have some dependencies to mach-omap2 code for PRCM
>>>>>>> for example, but we're trying to get all that to live in
>>>>>>> drivers.
>>>>>>>
>>>>>>> So for new pieces, let's not add further dependencies to
>>>>>>> complicate moving things to drivers.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Tony
>>>>>>>
>>>>>>
>>>>>> Ok I will go ahead and pull the control module code that handles IPC
>>>>>> into the wkup_m3 driver.
>>>>>
>>>>> Any control module register access still needs to stay in control.c.
>>>>>
>>>>>> The wkup_m3.c file is still present in mach-omap2 as the right
>>>>>> location for it wasn't decided in the last RFC. Any thoughts on a good
>>>>>> location for it?
>>>>>
>>>>> I raised this also in earlier reviews, but don't remember the if it was
>>>>> answered...
>>>>>
>>>>> Why can't we handle the wkup_m3 using remoteproc/rpmsg instead of
>>>>> creating another little driver that duplicates communication with the
>>>>> M3.  Note also that the firmware load part would also be provided by
>>>>> remoteproc/rpmsg.
>>>>
>>>> Looping Suman Anna who handled the IPC patches this patch set is based
>>>> on top of...
>>>>
>>>> For the wkup_m3, the mailbox isn't used in a traditional manner. It's
>>>> only used with a dummy write to trigger an interrupt from the A8 to
>>>> the M3 and then communication happens in IPC registers within the
>>>> control module. No messages are actually sent through the mailbox in
>>>> either direction so that's why it was done this way rather than bring
>>>> in full support for the mailbox.
>>>
>>> I don't believe remoteproc/rpmsg forces you to use the mailbox for
>>> communication, and can use other IPC mechanisms.  This still sounds
>>> cleaner than reinventing remoteproc/rpmsg because of slight variations.
>>>
>>> The linux way is to use and extend what is already there, and if it's
>>> extended to the breaking point, then make a case for why something new
>>> is needed.
>>>
>> While I agree to re-use frameworks, am strongly against use of IPC for
>> the power management which is time sensitive. 
> 
> Maybe I'm misunderstanding what you mean by IPC, but I'm not sure how
> you can be against IPC here.  Communication with M3 requires it.
>
> The only question is what framework to use: create a new wkup_m3 driver
> which creates yet another set of IPC APIs as well as a seprate firmware
> load path?  Or reuse remoteproc/rpmsg which provides both.
>
I mean Linux IPC layers. 
 
>> Imagine a scenario where
>> ACPI is asked to go through remoteproc/ipc for talking to firmware.
>> If it takes 10 to 15 uS just to send a command to firmware to change
>> some control, that just tells something is not right and thats what
>> IPC will do.
> 
> Sounds like what you're saying is that remoteproc/rpmsg will be too slow
> for this?  Is there any evidence of that?  Even if it is slow, it sounds
> like a perfect opportuntity to enhance it.
>
Well 10's uS is fine for the pure IPC kind of messaging. The numbers were
measured on Linux OMAP IPC stack for different purpose.
 
> Also, if speed is that much of a concern concern here, I wonder why
> there's a 500msec timeout in in the communication between MPU and M3 in
> the current code.
> 
>> ARM world is also moving towards that by standardizing some of these
>> through (read PSCI) and thats the way to go in general. 
> 
> Agreed, but I'm not sure (yet) about enforcing PSCI on legacy platforms
> that don't support it natively.  Are you saying that the AM33xx firmware
> should be converted to be PSCI compliant?  Admittedly, I haven't read
> the PSCI spec closely, but I'm wondering if the current role splitting
> between MPU and M3 fits well with PSCI. 
>
I didn't mean for the AM3XXX specifically because its current job is rather
very limited. i.e suspend. My concern is that IPC is not viewed as
an option for power management controllers like M3 which can abstract
all the hardware gory details and export a simpled interface for OS
in form of PSCI/ACPI. 

>> Specifically for this series, I am also against having tons of exports
>> and all of that should be extracted properly but remoteproc is not
>> going to be the way. Firmware download has to happen much earlier(ROM
>> path) so thats not the requirement where we could have used the
>> remoteproc firmware download feature.
> 
> At least for AM33xx, I don't see any requirement that firmware download
> happens early (or at all.)  PM will simply be disabled until firmware is
> available (and running.)
> 
Thats because its role is pretty much limited. The point was mainly from
general PM firmware download strategy and not specifically for this
one device.

I just wanted to be sure that we will do all of this for AM3XXX and expect
the future SOCs to follow that model where M3 would have more prominent
role and more and more hardware sequencing for HLOS.

Regards,
Santosh

Russ Dill Aug. 13, 2013, 6:30 p.m. UTC | #13

>>> ARM world is also moving towards that by standardizing some of these
>>> through (read PSCI) and thats the way to go in general.
>>
>> Agreed, but I'm not sure (yet) about enforcing PSCI on legacy platforms
>> that don't support it natively.  Are you saying that the AM33xx firmware
>> should be converted to be PSCI compliant?  Admittedly, I haven't read
>> the PSCI spec closely, but I'm wondering if the current role splitting
>> between MPU and M3 fits well with PSCI.
>>
> I didn't mean for the AM3XXX specifically because its current job is rather
> very limited. i.e suspend. My concern is that IPC is not viewed as
> an option for power management controllers like M3 which can abstract
> all the hardware gory details and export a simpled interface for OS
> in form of PSCI/ACPI.

The IPC between the M3 and the A8 on the am335x is just a pair of
notification mechanisms (one from the A8, mailbox, and one from the
M3, sev) and a set of 8 32 bit registers. The 8 32 bit registers are
just a scratchpad and have no access rules or functionality beyond
being a scratchpad. How complicated do we want to make this?

Santosh Shilimkar Aug. 13, 2013, 6:40 p.m. UTC | #14

On Tuesday 13 August 2013 02:30 PM, Russ Dill wrote:
>>>> ARM world is also moving towards that by standardizing some of these
>>>> through (read PSCI) and thats the way to go in general.
>>>
>>> Agreed, but I'm not sure (yet) about enforcing PSCI on legacy platforms
>>> that don't support it natively.  Are you saying that the AM33xx firmware
>>> should be converted to be PSCI compliant?  Admittedly, I haven't read
>>> the PSCI spec closely, but I'm wondering if the current role splitting
>>> between MPU and M3 fits well with PSCI.
>>>
>> I didn't mean for the AM3XXX specifically because its current job is rather
>> very limited. i.e suspend. My concern is that IPC is not viewed as
>> an option for power management controllers like M3 which can abstract
>> all the hardware gory details and export a simpled interface for OS
>> in form of PSCI/ACPI.
> 
> The IPC between the M3 and the A8 on the am335x is just a pair of
> notification mechanisms (one from the A8, mailbox, and one from the
> M3, sev) and a set of 8 32 bit registers. The 8 32 bit registers are
> just a scratchpad and have no access rules or functionality beyond
> being a scratchpad. How complicated do we want to make this?
> 
Exactly what I mean as well.

Regards,
Santosh

Kevin Hilman Aug. 13, 2013, 7:11 p.m. UTC | #15

Russ Dill <[e-mail]> writes:

>>>> ARM world is also moving towards that by standardizing some of these
>>>> through (read PSCI) and thats the way to go in general.
>>>
>>> Agreed, but I'm not sure (yet) about enforcing PSCI on legacy platforms
>>> that don't support it natively.  Are you saying that the AM33xx firmware
>>> should be converted to be PSCI compliant?  Admittedly, I haven't read
>>> the PSCI spec closely, but I'm wondering if the current role splitting
>>> between MPU and M3 fits well with PSCI.
>>>
>> I didn't mean for the AM3XXX specifically because its current job is rather
>> very limited. i.e suspend. My concern is that IPC is not viewed as
>> an option for power management controllers like M3 which can abstract
>> all the hardware gory details and export a simpled interface for OS
>> in form of PSCI/ACPI.
>
> The IPC between the M3 and the A8 on the am335x is just a pair of
> notification mechanisms (one from the A8, mailbox, and one from the
> M3, sev) and a set of 8 32 bit registers. The 8 32 bit registers are
> just a scratchpad and have no access rules or functionality beyond
> being a scratchpad. How complicated do we want to make this?

The OMAP IPC between the MPU and DSP (read: any other remote processor)
is just a mailbox and some shared memory.  No complicated, and works
with remoteproc/rpmsg.

What's more complicated? reusing existing frameworks or re-inventing
them?  Don't forget that "simple" drivers rarely stay that way.

Also, this is not a hard stance on my part.  I just need to be
convinced.  

The fact is that there are existing frameworks for doing what is being
proposed here.  Either use the existing frameworks, or make a technical
argument based on why those frameworks are not a good fit (ideally,
after having tried to use those frameworks.)

Once again, I'm pretty sure I mentioned this in earlier reviews of this
series.

Kevin

Suman Anna Aug. 14, 2013, 5:27 p.m. UTC | #16

Kevin, Santosh, Dave, Russ,

On 08/13/2013 02:11 PM, Kevin Hilman wrote:
> Russ Dill <[e-mail]> writes:
> 
>>>>> ARM world is also moving towards that by standardizing some of these
>>>>> through (read PSCI) and thats the way to go in general.
>>>>
>>>> Agreed, but I'm not sure (yet) about enforcing PSCI on legacy platforms
>>>> that don't support it natively.  Are you saying that the AM33xx firmware
>>>> should be converted to be PSCI compliant?  Admittedly, I haven't read
>>>> the PSCI spec closely, but I'm wondering if the current role splitting
>>>> between MPU and M3 fits well with PSCI.
>>>>
>>> I didn't mean for the AM3XXX specifically because its current job is rather
>>> very limited. i.e suspend. My concern is that IPC is not viewed as
>>> an option for power management controllers like M3 which can abstract
>>> all the hardware gory details and export a simpled interface for OS
>>> in form of PSCI/ACPI.
>>
>> The IPC between the M3 and the A8 on the am335x is just a pair of
>> notification mechanisms (one from the A8, mailbox, and one from the
>> M3, sev) and a set of 8 32 bit registers. The 8 32 bit registers are
>> just a scratchpad and have no access rules or functionality beyond
>> being a scratchpad. How complicated do we want to make this?
> 
> The OMAP IPC between the MPU and DSP (read: any other remote processor)
> is just a mailbox and some shared memory.  No complicated, and works
> with remoteproc/rpmsg.

I had to catch up on all the threads before I responded, but here are my
notes regarding using remoteproc/rpmsg infrastructure for WkupM3. It
definitely brings in lot more complexity than what's present today. The
WkupM3 in AM335x does not have the same feature set as the Cortex-M3s
used within the IPU in OMAP4+ SoCs. It does not have an MMU and
peripheral accesses seems to be limited to just those in L4_Wkup domain.

A lot actually depends on whether the WkupM3 can use DDR. I am not sure
if there are any restrictions on WkupM3 from where it could run the code
or access data from. From what I can gather from the TRM (there's no big
separate chapter on WkupM3), it has just 16K UMEM and a 8K DMEM, and its
not much. The current firmware is loaded directly into the WkupM3's
internal UMem space and a check is being performed to make sure the
firmware is less than  16K, so that it can fit into UMem.

The remoteproc infrastructure is currently tied closely with the
virtio/rpmsg framework, and the boot requires that there are virtio
devices present in the resource table from the firmware image. The rpmsg
shared memory buffers are currently kinda fixed (512 buffers of 512
bytes each) and requires 3 pages just for the vring structures for this
many buffers. So, if there are restrictions on DDR access, then this
pretty much rules out remoteproc/rpmsg infrastructure.

If the DDR access is ok, then there are other challenges that needs to
be met. The current firmware definitely requires the addition of the
resource table and the lower level code for handling the virtio_ring
transport for receiving messages. It would also need its own remoteproc
driver for handling the firmware binary format and the signalling
required to trigger the rpmsg buffer processing. The firmware binary
format needs to be adapted to something that this driver would
understand. It definitely doesn't look like ELF currently, so something
on the lines of ste_modem_rproc needs to be done. Also, the
remoteproc/rpmsg infrastructure can support multiple vring transport
channels between the processor, and depending on how many are supported,
we either need to exchange the vq_id (like OMAP remoteproc), or process
the known virtqueues always (like DA8xx remoteproc). The former requires
that a message payload is used, and mandates the usage of the IPC data
registers in the control module given that WkupM3 on AM335 cannot access
any mailbox registers. Any usage of IPC data registers depends on where
we do it. If all the accesses were to be done within
mach-omap2/control.c, then there is no easy way for using this API from
drivers/remoteproc, until we have the control module driver. I do feel
that this needs to be done within the wkup_m3 driver currently. This
would be no different to the crossbar driver example on DRA7, which also
has to deal with registers in Control module. The IPC data registers do
not have any associated interrupts by itself, so just making this a
standalone driver is also not possible. Also, the dependencies with
using the omap_device API for dealing with the hard reset lines needs to
be dealt with.

The current communication uses the IPC data registers, and sometimes
uses them as plain status registers. There's certain registers used for
sharing status, version etc which are shared by both the processors.
Using rpmsg would require communicating every single message, and if
there were to be some shared variables to be used simultaneously, then
this has to be exchanged through a new remoteproc resource type.

One additional aspect is that the current remoteproc core does not have
the necessary runtime pm support, but in general the approach would be
to treat the remoteprocs as true slave devices. I would imagine the
driver core to put the remoteprocs into reset state, after asking them
to save their context during suspend.

regards
Suman

Russ Dill Aug. 14, 2013, 7:16 p.m. UTC | #17

On Wed, Aug 14, 2013 at 10:27 AM, Suman Anna <[e-mail]> wrote:
> Kevin, Santosh, Dave, Russ,
>
> On 08/13/2013 02:11 PM, Kevin Hilman wrote:
>> Russ Dill <[e-mail]> writes:
>>
>>>>>> ARM world is also moving towards that by standardizing some of these
>>>>>> through (read PSCI) and thats the way to go in general.
>>>>>
>>>>> Agreed, but I'm not sure (yet) about enforcing PSCI on legacy platforms
>>>>> that don't support it natively.  Are you saying that the AM33xx firmware
>>>>> should be converted to be PSCI compliant?  Admittedly, I haven't read
>>>>> the PSCI spec closely, but I'm wondering if the current role splitting
>>>>> between MPU and M3 fits well with PSCI.
>>>>>
>>>> I didn't mean for the AM3XXX specifically because its current job is rather
>>>> very limited. i.e suspend. My concern is that IPC is not viewed as
>>>> an option for power management controllers like M3 which can abstract
>>>> all the hardware gory details and export a simpled interface for OS
>>>> in form of PSCI/ACPI.
>>>
>>> The IPC between the M3 and the A8 on the am335x is just a pair of
>>> notification mechanisms (one from the A8, mailbox, and one from the
>>> M3, sev) and a set of 8 32 bit registers. The 8 32 bit registers are
>>> just a scratchpad and have no access rules or functionality beyond
>>> being a scratchpad. How complicated do we want to make this?
>>
>> The OMAP IPC between the MPU and DSP (read: any other remote processor)
>> is just a mailbox and some shared memory.  No complicated, and works
>> with remoteproc/rpmsg.
>
> I had to catch up on all the threads before I responded, but here are my
> notes regarding using remoteproc/rpmsg infrastructure for WkupM3. It
> definitely brings in lot more complexity than what's present today. The
> WkupM3 in AM335x does not have the same feature set as the Cortex-M3s
> used within the IPU in OMAP4+ SoCs. It does not have an MMU and
> peripheral accesses seems to be limited to just those in L4_Wkup domain.
>
> A lot actually depends on whether the WkupM3 can use DDR. I am not sure
> if there are any restrictions on WkupM3 from where it could run the code
> or access data from. From what I can gather from the TRM (there's no big
> separate chapter on WkupM3), it has just 16K UMEM and a 8K DMEM, and its
> not much. The current firmware is loaded directly into the WkupM3's
> internal UMem space and a check is being performed to make sure the
> firmware is less than  16K, so that it can fit into UMem.

Even if the WkupM3 could access DMEM, it would defeat the whole
purpose of using it, as it lives to do things like enable VTP, but the
EMIF clocks into bypass, turn off the power domain the EMIF is
contained in, etc.

> The remoteproc infrastructure is currently tied closely with the
> virtio/rpmsg framework, and the boot requires that there are virtio
> devices present in the resource table from the firmware image. The rpmsg
> shared memory buffers are currently kinda fixed (512 buffers of 512
> bytes each) and requires 3 pages just for the vring structures for this
> many buffers. So, if there are restrictions on DDR access, then this
> pretty much rules out remoteproc/rpmsg infrastructure.
>
> If the DDR access is ok, then there are other challenges that needs to
> be met. The current firmware definitely requires the addition of the
> resource table and the lower level code for handling the virtio_ring
> transport for receiving messages. It would also need its own remoteproc
> driver for handling the firmware binary format and the signalling
> required to trigger the rpmsg buffer processing. The firmware binary
> format needs to be adapted to something that this driver would
> understand. It definitely doesn't look like ELF currently, so something
> on the lines of ste_modem_rproc needs to be done. Also, the
> remoteproc/rpmsg infrastructure can support multiple vring transport
> channels between the processor, and depending on how many are supported,
> we either need to exchange the vq_id (like OMAP remoteproc), or process
> the known virtqueues always (like DA8xx remoteproc). The former requires
> that a message payload is used, and mandates the usage of the IPC data
> registers in the control module given that WkupM3 on AM335 cannot access
> any mailbox registers. Any usage of IPC data registers depends on where
> we do it. If all the accesses were to be done within
> mach-omap2/control.c, then there is no easy way for using this API from
> drivers/remoteproc, until we have the control module driver. I do feel
> that this needs to be done within the wkup_m3 driver currently. This
> would be no different to the crossbar driver example on DRA7, which also
> has to deal with registers in Control module. The IPC data registers do
> not have any associated interrupts by itself, so just making this a
> standalone driver is also not possible. Also, the dependencies with
> using the omap_device API for dealing with the hard reset lines needs to
> be dealt with.
>
> The current communication uses the IPC data registers, and sometimes
> uses them as plain status registers. There's certain registers used for
> sharing status, version etc which are shared by both the processors.
> Using rpmsg would require communicating every single message, and if
> there were to be some shared variables to be used simultaneously, then
> this has to be exchanged through a new remoteproc resource type.
>
> One additional aspect is that the current remoteproc core does not have
> the necessary runtime pm support, but in general the approach would be
> to treat the remoteprocs as true slave devices. I would imagine the
> driver core to put the remoteprocs into reset state, after asking them
> to save their context during suspend.
>
> regards
> Suman
>

Paul Walmsley Aug. 20, 2013, 11:39 p.m. UTC | #18

Hi

a few comments

On Wed, 14 Aug 2013, Suman Anna wrote:

> The remoteproc infrastructure is currently tied closely with the
> virtio/rpmsg framework, and the boot requires that there are virtio
> devices present in the resource table from the firmware image.

Using static channels is something that can be added to the existing code, 
if you don't want to use dynamic channels.

> The rpmsg shared memory buffers are currently kinda fixed (512 buffers 
> of 512 bytes each) and requires 3 pages just for the vring structures 
> for this many buffers. So, if there are restrictions on DDR access, then 
> this pretty much rules out remoteproc/rpmsg infrastructure.

It should be possible to patch that code to vary the size and count of the 
memory buffers, based on the remote processor.  So no direct DDR access 
should be required - for that reason, anyway.

> If the DDR access is ok, then there are other challenges that needs to
> be met. The current firmware definitely requires the addition of the
> resource table and the lower level code for handling the virtio_ring
> transport for receiving messages. It would also need its own remoteproc
> driver for handling the firmware binary format 

Hmm, could you explain this further?  Are you just referring to the 
process of parsing out the dynamic channel data during initialization?

> and the signalling required to trigger the rpmsg buffer processing. The 
> firmware binary format needs to be adapted to something that this driver 
> would understand.  It definitely doesn't look like ELF currently, so 
> something on the lines of ste_modem_rproc needs to be done.

Or just use ELF or static channels.

> Also, the remoteproc/rpmsg infrastructure can support multiple vring 
> transport channels between the processor, and depending on how many are 
> supported, we either need to exchange the vq_id (like OMAP remoteproc), 
> or process the known virtqueues always (like DA8xx remoteproc). The 
> former requires that a message payload is used, and mandates the usage 
> of the IPC data registers in the control module given that WkupM3 on 
> AM335 cannot access any mailbox registers. Any usage of IPC data 
> registers depends on where we do it. If all the accesses were to be done 
> within mach-omap2/control.c, then there is no easy way for using this 
> API from drivers/remoteproc, until we have the control module driver.

Yep, real SCM drivers have been needed for some time now, for pretty much 
all of the OMAP SoCs.  It should be pretty easy to prototype for your 
purposes, though.

> The current communication uses the IPC data registers, and sometimes
> uses them as plain status registers. There's certain registers used for
> sharing status, version etc which are shared by both the processors.
> Using rpmsg would require communicating every single message, and if
> there were to be some shared variables to be used simultaneously, then
> this has to be exchanged through a new remoteproc resource type.

I don't quite understand this last part - "shared variables to be used 
simultaneously".  How does the existing code synchronize them?

> One additional aspect is that the current remoteproc core does not have
> the necessary runtime pm support, but in general the approach would be
> to treat the remoteprocs as true slave devices. I would imagine the
> driver core to put the remoteprocs into reset state, after asking them
> to save their context during suspend.

Why is runtime PM support needed in the remoteproc core?  Wouldn't that 
only be needed in the remote processor's device driver?


- Paul

Suman Anna Aug. 21, 2013, 5:32 p.m. UTC | #19

Paul,

On 08/20/2013 06:39 PM, Paul Walmsley wrote:
> Hi
> 
> a few comments
> 
> On Wed, 14 Aug 2013, Suman Anna wrote:
> 
>> The remoteproc infrastructure is currently tied closely with the
>> virtio/rpmsg framework, and the boot requires that there are virtio
>> devices present in the resource table from the firmware image.
> 
> Using static channels is something that can be added to the existing code, 
> if you don't want to use dynamic channels.

The resource table is used for both indicating the remoteproc resources
as well as allowing the driver to publish the data back so that the
remote processor can configure itself. The virtio devices do have other
configuration fields and this allows the driver to update the status
fields which the remote processor can read. This functionality is
definitely lost if we use static virtio devices.

Also, the remoteproc boot is a two-step process currently. It reads the
virtio device information from the resource table and creates the virtio
devices. The virtio device probe then triggers the remoteproc boot,
which processes the firmware segments and handles the actual processor
boot. The memory for firmware segments are allocated through CMA and the
allocated addresses are published through the resource table to the
remote processor-side. The core definitely needs to be patched up to
support loading internal memory segments.

> 
>> The rpmsg shared memory buffers are currently kinda fixed (512 buffers 
>> of 512 bytes each) and requires 3 pages just for the vring structures 
>> for this many buffers. So, if there are restrictions on DDR access, then 
>> this pretty much rules out remoteproc/rpmsg infrastructure.
> 
> It should be possible to patch that code to vary the size and count of the 
> memory buffers, based on the remote processor.  So no direct DDR access 
> should be required - for that reason, anyway.

Yep, I agree that this needs to be programmable. Support needs to be
added to both the remoteproc and rpmsg cores to deal with internal
memory for the vrings and vring buffers. Currently, it is all allocated
dynamically through CMA. That said, the WkupM3 really has a very small
memory regions, 16K of UMEM and 8K of DMEM. Everything has to fit within
that including the vrings and vring buffers. I have to check if there
are alignment restrictions imposed by the virtio code on the vrings and
if that overshoots the available internal memory.

> 
>> If the DDR access is ok, then there are other challenges that needs to
>> be met. The current firmware definitely requires the addition of the
>> resource table and the lower level code for handling the virtio_ring
>> transport for receiving messages. It would also need its own remoteproc
>> driver for handling the firmware binary format 
> 
> Hmm, could you explain this further?  Are you just referring to the 
> process of parsing out the dynamic channel data during initialization?

Yes, for parsing out where the resource table is. The remoteproc core
provides the necessary hooks for finding the resource table and loading
the firmware segments. If the firmware file is gonna remain a binary
blob, then these hooks need to be implemented for that binary format. No
issues if it were an ELF file, since we expect the resource table to be
present in a special section.

> 
>> and the signalling required to trigger the rpmsg buffer processing. The 
>> firmware binary format needs to be adapted to something that this driver 
>> would understand.  It definitely doesn't look like ELF currently, so 
>> something on the lines of ste_modem_rproc needs to be done.
> 
> Or just use ELF or static channels.

I have taken a look at the firmware tree, and the binary is generated
from an ELF file, so the format is not an issue as long as the memory
footprint is satisfied. static channels is more of an issue as pointed
above.

> 
>> Also, the remoteproc/rpmsg infrastructure can support multiple vring 
>> transport channels between the processor, and depending on how many are 
>> supported, we either need to exchange the vq_id (like OMAP remoteproc), 
>> or process the known virtqueues always (like DA8xx remoteproc). The 
>> former requires that a message payload is used, and mandates the usage 
>> of the IPC data registers in the control module given that WkupM3 on 
>> AM335 cannot access any mailbox registers. Any usage of IPC data 
>> registers depends on where we do it. If all the accesses were to be done 
>> within mach-omap2/control.c, then there is no easy way for using this 
>> API from drivers/remoteproc, until we have the control module driver.
> 
> Yep, real SCM drivers have been needed for some time now, for pretty much 
> all of the OMAP SoCs.  It should be pretty easy to prototype for your 
> purposes, though.
> 
>> The current communication uses the IPC data registers, and sometimes
>> uses them as plain status registers. There's certain registers used for
>> sharing status, version etc which are shared by both the processors.
>> Using rpmsg would require communicating every single message, and if
>> there were to be some shared variables to be used simultaneously, then
>> this has to be exchanged through a new remoteproc resource type.
> 
> I don't quite understand this last part - "shared variables to be used 
> simultaneously".  How does the existing code synchronize them?

The IPC data registers are just like regular registers, values written
in them stay that way until changed. The current PM code uses some of
these registers as fixed status variables, while some of them are
changed based on the state machine, and an interrupt is sent to process
that PM command. The message payload is well within these 8 registers
for the needs of PM.

> 
>> One additional aspect is that the current remoteproc core does not have
>> the necessary runtime pm support, but in general the approach would be
>> to treat the remoteprocs as true slave devices. I would imagine the
>> driver core to put the remoteprocs into reset state, after asking them
>> to save their context during suspend.
> 
> Why is runtime PM support needed in the remoteproc core?  Wouldn't that 
> only be needed in the remote processor's device driver?

The actual low-level operations would be in the remote processor's
device driver, but since the core maintains the overall device
management including loading and state machine, doing it in the core
would be nice as it provides a common base logic (can be implemented
through device type and ops)  instead of replicating it in every device
driver.

regards
Suman

Patch

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 31e0dfe..934041a 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -605,3 +605,60 @@  int omap3_ctrl_save_padconf(void)
 }
 
 #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
+
+#if defined(CONFIG_SOC_AM33XX) && defined(CONFIG_PM)
+void am33xx_txev_eoi(void)
+{
+	omap_ctrl_writel(AM33XX_M3_TXEV_ACK, AM33XX_CONTROL_M3_TXEV_EOI);
+}
+
+void am33xx_txev_enable(void)
+{
+	omap_ctrl_writel(AM33XX_M3_TXEV_ENABLE, AM33XX_CONTROL_M3_TXEV_EOI);
+}
+
+/*
+ * Invalidate M3 firmware version before hardreset.
+ * Write invalid version in lower 4 nibbles of parameter
+ * register (ipc_regs + 0x8).
+ */
+void am33xx_pm_version_clear(void)
+{
+	omap_ctrl_writel(0xffff0000, AM33XX_CONTROL_IPC_MSG_REG2);
+}
+
+int am33xx_pm_version_get(void)
+{
+	return omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG2) & M3_VERSION_MASK;
+}
+
+void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
+{
+	omap_ctrl_writel(data->resume_addr, AM33XX_CONTROL_IPC_MSG_REG0);
+	omap_ctrl_writel(data->sleep_mode, AM33XX_CONTROL_IPC_MSG_REG1);
+	omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
+	omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
+	omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
+}
+
+int am33xx_pm_status(void)
+{
+	int i;
+
+	i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG1);
+	i &= IPC_RESP_MASK;
+	i >>= __ffs(IPC_RESP_MASK);
+
+	return i;
+}
+
+int am33xx_pm_wake_src(void)
+{
+	int i;
+
+	i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG6);
+	i &= 0xff;
+
+	return i;
+}
+#endif /* CONFIG_SOC_AM33XX && CONFIG_PM */
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index f7d7c2e..9be587c 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -370,6 +370,22 @@ 
 #define AM33XX_DEV_FEATURE		0x604
 #define AM33XX_SGX_MASK			BIT(29)
 
+/* AM33XX M3_TXEV_EOI register */
+#define AM33XX_CONTROL_M3_TXEV_EOI	0x1324
+
+#define AM33XX_M3_TXEV_ACK		(0x1 << 0)
+#define AM33XX_M3_TXEV_ENABLE		(0x0 << 0)
+
+/* AM33XX IPC message registers */
+#define AM33XX_CONTROL_IPC_MSG_REG0	0x1328
+#define AM33XX_CONTROL_IPC_MSG_REG1	0x132C
+#define AM33XX_CONTROL_IPC_MSG_REG2	0x1330
+#define AM33XX_CONTROL_IPC_MSG_REG3	0x1334
+#define AM33XX_CONTROL_IPC_MSG_REG4	0x1338
+#define AM33XX_CONTROL_IPC_MSG_REG5	0x133C
+#define AM33XX_CONTROL_IPC_MSG_REG6	0x1340
+#define AM33XX_CONTROL_IPC_MSG_REG7	0x1344
+
 /* CONTROL OMAP STATUS register to identify OMAP3 features */
 #define OMAP3_CONTROL_OMAP_STATUS	0x044c
 
@@ -429,6 +445,44 @@  extern void omap3630_ctrl_disable_rta(void);
 extern int omap3_ctrl_save_padconf(void);
 extern void omap2_set_globals_control(void __iomem *ctrl,
 				      void __iomem *ctrl_pad);
+struct am33xx_ipc_data {
+	u32 resume_addr;
+	u32 sleep_mode;
+	u32 param1;
+	u32 param2;
+	u32 param3;
+	u32 param4;
+	u32 param5;
+	u32 param6;
+};
+
+#define IPC_RESP_SHIFT			16
+#define IPC_RESP_MASK			(0xffff << 16)
+
+#define M3_VERSION_SHIFT		0
+#define M3_VERSION_MASK			(0xffff << 0)
+
+/*
+ * 9-4 = VTT GPIO PIN (6 Bits)
+ *   3 = VTT Status (1 Bit)
+ * 2-0 = Memory Type (2 Bits)
+*/
+#define MEM_TYPE_SHIFT		(0x0)
+#define MEM_TYPE_MASK		(0x7 << 0)
+#define VTT_STAT_SHIFT		(0x3)
+#define VTT_STAT_MASK		(0x1 << 3)
+#define VTT_GPIO_PIN_SHIFT	(0x4)
+#define VTT_GPIO_PIN_MASK	(0x2f << 4)
+
+extern void am33xx_wkup_m3_ipc_cmd(struct am33xx_ipc_data *data);
+extern void am33xx_txev_eoi(void);
+extern void am33xx_txev_enable(void);
+extern void am33xx_pm_version_clear(void);
+extern int am33xx_pm_version_get(void);
+extern void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data);
+extern int am33xx_pm_status(void);
+extern int am33xx_pm_wake_src(void);
+
 #else
 #define omap_ctrl_base_get()		0
 #define omap_ctrl_readb(x)		0