[PATCHv3,4/9] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec

Commit Message

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

From: Vaibhav Bedia <[e-mail]>

SDRAM controller on AM33XX requires that a modification of certain
bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
AM335x-Rev H) is followed by a dummy read access to SDRAM. This
scenario arises when entering a low power state like DeepSleep.
To ensure that the read is not from a cached region we reserve
some memory during bootup using the arm_memblock_steal() API.

A subsequent patch will pass along the location of the reserved
memory location to the AM335x suspend handler which modifies the
PWR_MGMT_CTRL register in the EMIF.

Signed-off-by: Vaibhav Bedia <[e-mail]>
Signed-off-by: Dave Gerlach <[e-mail]>
---
 arch/arm/mach-omap2/board-generic.c |    2 +-
 arch/arm/mach-omap2/common.c        |   28 ++++++++++++++++++++++++++++
 arch/arm/mach-omap2/common.h        |    4 ++++
 arch/arm/mach-omap2/io.c            |    1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

Comments

Russ Dill Aug. 8, 2013, 2:30 a.m. UTC | #1

On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach <[e-mail]> wrote:
> From: Vaibhav Bedia <[e-mail]>
>
> SDRAM controller on AM33XX requires that a modification of certain
> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
> scenario arises when entering a low power state like DeepSleep.
> To ensure that the read is not from a cached region we reserve
> some memory during bootup using the arm_memblock_steal() API.
>
> A subsequent patch will pass along the location of the reserved
> memory location to the AM335x suspend handler which modifies the
> PWR_MGMT_CTRL register in the EMIF.
>
> Signed-off-by: Vaibhav Bedia <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>

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

> ---
>  arch/arm/mach-omap2/board-generic.c |    2 +-
>  arch/arm/mach-omap2/common.c        |   28 ++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/common.h        |    4 ++++
>  arch/arm/mach-omap2/io.c            |    1 +
>  4 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
> index be5d005..aed750c 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -156,7 +156,7 @@ static const char *am33xx_boards_compat[] __initdata = {
>  };
>
>  DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)")
> -       .reserve        = omap_reserve,
> +       .reserve        = am33xx_reserve,
>         .map_io         = am33xx_map_io,
>         .init_early     = am33xx_init_early,
>         .init_irq       = omap_intc_of_init,
> diff --git a/arch/arm/mach-omap2/common.c b/arch/arm/mach-omap2/common.c
> index 2dabb9e..756586f 100644
> --- a/arch/arm/mach-omap2/common.c
> +++ b/arch/arm/mach-omap2/common.c
> @@ -15,6 +15,8 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/platform_data/dsp-omap.h>
> +#include <asm/memblock.h>
> +#include <asm/mach/map.h>
>
>  #include "common.h"
>  #include "omap-secure.h"
> @@ -34,3 +36,29 @@ void __init omap_reserve(void)
>         omap_secure_ram_reserve_memblock();
>         omap_barrier_reserve_memblock();
>  }
> +
> +static phys_addr_t am33xx_paddr;
> +static u32 am33xx_size;
> +
> +/* Steal one page physical memory for uncached read DeepSleep */
> +void __init am33xx_reserve(void)
> +{
> +       am33xx_size = ALIGN(PAGE_SIZE, SZ_1M);
> +       am33xx_paddr = arm_memblock_steal(am33xx_size, SZ_1M);
> +}
> +
> +void __iomem *am33xx_dram_sync;
> +
> +void __init am33xx_dram_sync_init(void)
> +{
> +       struct map_desc dram_io_desc[1];
> +
> +       dram_io_desc[0].virtual = __phys_to_virt(am33xx_paddr);
> +       dram_io_desc[0].pfn = __phys_to_pfn(am33xx_paddr);
> +       dram_io_desc[0].length = am33xx_size;
> +       dram_io_desc[0].type = MT_MEMORY_SO;
> +
> +       iotable_init(dram_io_desc, ARRAY_SIZE(dram_io_desc));
> +
> +       am33xx_dram_sync = (void __iomem *) dram_io_desc[0].virtual;
> +}
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> index dfcc182..6b8ef74 100644
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -294,6 +294,10 @@ struct omap2_hsmmc_info;
>  extern int omap4_twl6030_hsmmc_init(struct omap2_hsmmc_info *controllers);
>  extern void omap_reserve(void);
>
> +extern void am33xx_reserve(void);
> +extern void am33xx_dram_sync_init(void);
> +extern void __iomem *am33xx_dram_sync;
> +
>  struct omap_hwmod;
>  extern int omap_dss_reset(struct omap_hwmod *);
>
> diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> index 4a3f06f..3ad7543 100644
> --- a/arch/arm/mach-omap2/io.c
> +++ b/arch/arm/mach-omap2/io.c
> @@ -322,6 +322,7 @@ void __init ti81xx_map_io(void)
>  void __init am33xx_map_io(void)
>  {
>         iotable_init(omapam33xx_io_desc, ARRAY_SIZE(omapam33xx_io_desc));
> +       am33xx_dram_sync_init();
>  }
>  #endif
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia <[e-mail]>
> 
> SDRAM controller on AM33XX requires that a modification of certain
> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
> scenario arises when entering a low power state like DeepSleep.
> To ensure that the read is not from a cached region we reserve
> some memory during bootup using the arm_memblock_steal() API.
> 
> A subsequent patch will pass along the location of the reserved
> memory location to the AM335x suspend handler which modifies the
> PWR_MGMT_CTRL register in the EMIF.
> 
> Signed-off-by: Vaibhav Bedia <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>
> ---
Acked-by: Santosh Shilimkar <[e-mail]>

Kevin Hilman Aug. 8, 2013, 6:16 p.m. UTC | #3

Dave Gerlach <[e-mail]> writes:

> From: Vaibhav Bedia <[e-mail]>
>
> SDRAM controller on AM33XX requires that a modification of certain
> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
> scenario arises when entering a low power state like DeepSleep.
> To ensure that the read is not from a cached region we reserve
> some memory during bootup using the arm_memblock_steal() API.

Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?

However, you remove that feature when you remove omap_reserve() here
becasue omap_reserve() ss not called from your new reserve function.
Are you *really* sure you don't need the functions called there?  This
is a pretty big change that's not mentioned anywhere in the changelog.

Kevin

Santosh Shilimkar Aug. 8, 2013, 7:31 p.m. UTC | #4

On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
> Dave Gerlach <[e-mail]> writes:
> 
>> From: Vaibhav Bedia <[e-mail]>
>>
>> SDRAM controller on AM33XX requires that a modification of certain
>> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
>> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
>> scenario arises when entering a low power state like DeepSleep.
>> To ensure that the read is not from a cached region we reserve
>> some memory during bootup using the arm_memblock_steal() API.
> 
> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>
All the credit of that awful omap_bus_sync() goes to me since 
I introduced it. And I keep beating the hardware guys
who have not left a choice but to introduce the ugly work
around in software. ;-)

Regards,
Santosh

Kevin Hilman Aug. 8, 2013, 8:05 p.m. UTC | #5

Santosh Shilimkar <[e-mail]> writes:

> On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
>> Dave Gerlach <[e-mail]> writes:
>> 
>>> From: Vaibhav Bedia <[e-mail]>
>>>
>>> SDRAM controller on AM33XX requires that a modification of certain
>>> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
>>> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
>>> scenario arises when entering a low power state like DeepSleep.
>>> To ensure that the read is not from a cached region we reserve
>>> some memory during bootup using the arm_memblock_steal() API.
>> 
>> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>>
> All the credit of that awful omap_bus_sync() goes to me since 
> I introduced it. And I keep beating the hardware guys
> who have not left a choice but to introduce the ugly work
> around in software. ;-)

Agreed, but what's even more awful than the current version is
duplicating it in a slightly different way using yet another whole page
mapping for a single read/write location.

Kevin

Santosh Shilimkar Aug. 8, 2013, 8:11 p.m. UTC | #6

On Thursday 08 August 2013 04:05 PM, Kevin Hilman wrote:
> Santosh Shilimkar <[e-mail]> writes:
> 
>> On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
>>> Dave Gerlach <[e-mail]> writes:
>>>
>>>> From: Vaibhav Bedia <[e-mail]>
>>>>
>>>> SDRAM controller on AM33XX requires that a modification of certain
>>>> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
>>>> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
>>>> scenario arises when entering a low power state like DeepSleep.
>>>> To ensure that the read is not from a cached region we reserve
>>>> some memory during bootup using the arm_memblock_steal() API.
>>>
>>> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>>>
>> All the credit of that awful omap_bus_sync() goes to me since 
>> I introduced it. And I keep beating the hardware guys
>> who have not left a choice but to introduce the ugly work
>> around in software. ;-)
> 
> Agreed, but what's even more awful than the current version is
> duplicating it in a slightly different way using yet another whole page
> mapping for a single read/write location.
> 
The real issue is limitation of the kernel memory steal(memblock) API which
won't let you still less than 1 MB. It would have been ok for page allocation
because that is any way what you will get minimum on standard non-cached
allocations.

Regards,
Santosh

Kevin Hilman Aug. 9, 2013, 3:11 p.m. UTC | #7

Santosh Shilimkar <[e-mail]> writes:

> On Thursday 08 August 2013 04:05 PM, Kevin Hilman wrote:
>> Santosh Shilimkar <[e-mail]> writes:
>> 
>>> On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
>>>> Dave Gerlach <[e-mail]> writes:
>>>>
>>>>> From: Vaibhav Bedia <[e-mail]>
>>>>>
>>>>> SDRAM controller on AM33XX requires that a modification of certain
>>>>> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
>>>>> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
>>>>> scenario arises when entering a low power state like DeepSleep.
>>>>> To ensure that the read is not from a cached region we reserve
>>>>> some memory during bootup using the arm_memblock_steal() API.
>>>>
>>>> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>>>>
>>> All the credit of that awful omap_bus_sync() goes to me since 
>>> I introduced it. And I keep beating the hardware guys
>>> who have not left a choice but to introduce the ugly work
>>> around in software. ;-)
>> 
>> Agreed, but what's even more awful than the current version is
>> duplicating it in a slightly different way using yet another whole page
>> mapping for a single read/write location.
>> 
> The real issue is limitation of the kernel memory steal(memblock) API which
> won't let you still less than 1 MB. It would have been ok for page allocation
> because that is any way what you will get minimum on standard non-cached
> allocations.

All the more reason that the omap_bus_sync() should be refactored
slightly in a way that would be reusable for AM33xx.

Kevin

Dave Gerlach Aug. 9, 2013, 4:25 p.m. UTC | #8

On 08/09/2013 10:11 AM, Kevin Hilman wrote:
> Santosh Shilimkar <[e-mail]> writes:
>
>> On Thursday 08 August 2013 04:05 PM, Kevin Hilman wrote:
>>> Santosh Shilimkar <[e-mail]> writes:
>>>
>>>> On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
>>>>> Dave Gerlach <[e-mail]> writes:
>>>>>
>>>>>> From: Vaibhav Bedia <[e-mail]>
>>>>>>
>>>>>> SDRAM controller on AM33XX requires that a modification of certain
>>>>>> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
>>>>>> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
>>>>>> scenario arises when entering a low power state like DeepSleep.
>>>>>> To ensure that the read is not from a cached region we reserve
>>>>>> some memory during bootup using the arm_memblock_steal() API.
>>>>>
>>>>> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>>>>>
>>>> All the credit of that awful omap_bus_sync() goes to me since
>>>> I introduced it. And I keep beating the hardware guys
>>>> who have not left a choice but to introduce the ugly work
>>>> around in software. ;-)
>>>
>>> Agreed, but what's even more awful than the current version is
>>> duplicating it in a slightly different way using yet another whole page
>>> mapping for a single read/write location.
>>>
>> The real issue is limitation of the kernel memory steal(memblock) API which
>> won't let you still less than 1 MB. It would have been ok for page allocation
>> because that is any way what you will get minimum on standard non-cached
>> allocations.
>
> All the more reason that the omap_bus_sync() should be refactored
> slightly in a way that would be reusable for AM33xx.
>
> Kevin
>

I will look in to doing it this way. I do not know if there was any 
specific reason for doing it the way it was done.

Dave

Patch

diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index be5d005..aed750c 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -156,7 +156,7 @@  static const char *am33xx_boards_compat[] __initdata = {
 };
 
 DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)")
-	.reserve	= omap_reserve,
+	.reserve	= am33xx_reserve,
 	.map_io		= am33xx_map_io,
 	.init_early	= am33xx_init_early,
 	.init_irq	= omap_intc_of_init,
diff --git a/arch/arm/mach-omap2/common.c b/arch/arm/mach-omap2/common.c
index 2dabb9e..756586f 100644
--- a/arch/arm/mach-omap2/common.c
+++ b/arch/arm/mach-omap2/common.c
@@ -15,6 +15,8 @@ 
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_data/dsp-omap.h>
+#include <asm/memblock.h>
+#include <asm/mach/map.h>
 
 #include "common.h"
 #include "omap-secure.h"
@@ -34,3 +36,29 @@  void __init omap_reserve(void)
 	omap_secure_ram_reserve_memblock();
 	omap_barrier_reserve_memblock();
 }
+
+static phys_addr_t am33xx_paddr;
+static u32 am33xx_size;
+
+/* Steal one page physical memory for uncached read DeepSleep */
+void __init am33xx_reserve(void)
+{
+	am33xx_size = ALIGN(PAGE_SIZE, SZ_1M);
+	am33xx_paddr = arm_memblock_steal(am33xx_size, SZ_1M);
+}
+
+void __iomem *am33xx_dram_sync;
+
+void __init am33xx_dram_sync_init(void)
+{
+	struct map_desc dram_io_desc[1];
+
+	dram_io_desc[0].virtual = __phys_to_virt(am33xx_paddr);
+	dram_io_desc[0].pfn = __phys_to_pfn(am33xx_paddr);
+	dram_io_desc[0].length = am33xx_size;
+	dram_io_desc[0].type = MT_MEMORY_SO;
+
+	iotable_init(dram_io_desc, ARRAY_SIZE(dram_io_desc));
+
+	am33xx_dram_sync = (void __iomem *) dram_io_desc[0].virtual;
+}
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index dfcc182..6b8ef74 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -294,6 +294,10 @@  struct omap2_hsmmc_info;
 extern int omap4_twl6030_hsmmc_init(struct omap2_hsmmc_info *controllers);
 extern void omap_reserve(void);
 
+extern void am33xx_reserve(void);
+extern void am33xx_dram_sync_init(void);
+extern void __iomem *am33xx_dram_sync;
+
 struct omap_hwmod;
 extern int omap_dss_reset(struct omap_hwmod *);
 
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 4a3f06f..3ad7543 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -322,6 +322,7 @@  void __init ti81xx_map_io(void)
 void __init am33xx_map_io(void)
 {
 	iotable_init(omapam33xx_io_desc, ARRAY_SIZE(omapam33xx_io_desc));
+	am33xx_dram_sync_init();
 }
 #endif