Re: [PATCH v4] ARM: omap: edma: add suspend suspend/resume hooks

2013-11-07 Thread Gururaja Hebbar 
On Wednesday 06 November 2013 11:06 PM, Joel Fernandes wrote:
 Hi Vaibhav,
 
 On 10/31/2013 05:25 PM, Vaibhav Bedia wrote:
 Hi Daniel,

 On Wed, Oct 30, 2013 at 4:21 PM, Daniel Mack zon...@gmail.com wrote:
 [...]
 +
 +static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);
 +
  static struct platform_driver edma_driver = {
 .driver = {
 .name   = edma,
 +   .pm = edma_pm_ops,
 .of_match_table = edma_of_ids,
 },

 A while back we discovered a nasty race condition here that had us move the 
 EDMA
 PM callbacks to the noirq phase. IIRC the MMC driver was resuming
 before the EDMA
 driver had a chance to run and that was leading to a deadlock. I am
 not sure how to force
 this scenario but i do remember spending time debugging this on a
 random codebase.
 Maybe some else has some better ideas on how to force this race condition...
 
 I think you're talking about the patch at [1] which is not upstream. A quick
 question with my limited knowledge of suspend/resume- How can there be pending
 I/O operations between suspend/resume cycles? 

AFAIK, MMC framework started talking to cards immediately after resume.
Due to race condition, EDMA resume callback had not yet completed and
HSMMC driver had initiated a DMA operation. This resulted in Deadlock.

regards
Gururaja

 The sync is done before suspend,
 so I don't understand how one is receiving a response from the card after 
 resume
 before EDMA can resume? I'd imagine that until all devices are resumed, there
 will be no I/O operation issued. Let me know your thoughts.
 
 thanks,
 
 -Joel
 
 [1]
 https://www.gitorious.org/x0148406-public/linux-kernel/commit/b81bf04091986fa3893f31955564594567be3b61
 

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

Re: [PATCH v4] ARM: omap: edma: add suspend suspend/resume hooks

2013-11-07 Thread Gururaja Hebbar 
On Thursday 07 November 2013 11:07 PM, Daniel Mack wrote:
 Hi Joel,
 
 On 11/07/2013 05:34 PM, Joel Fernandes wrote:
 Thanks for your followup patch on this. It looks much better now using 
 existing
 functions to save/restore the state.
 
 Yes, thanks for the suggesting it in the first place.
 
 On 10/30/2013 03:21 PM, Daniel Mack wrote:
 diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
 [..]
 +static int edma_pm_resume(struct device *dev)
 +{
 +   int i, j;
 +
 +   pm_runtime_get_sync(dev);
 +
 +   for (j = 0; j  arch_num_cc; j++) {
 +   struct edma *cc = edma_cc[j];
 +
 +   s8 (*queue_priority_mapping)[2];
 +   s8 (*queue_tc_mapping)[2];
 +
 +   queue_tc_mapping = cc-info-queue_tc_mapping;
 +   queue_priority_mapping = cc-info-queue_priority_mapping;
 +
 +   /* Event queue to TC mapping */
 +   for (i = 0; queue_tc_mapping[i][0] != -1; i++)
 +   map_queue_tc(j, queue_tc_mapping[i][0],
 +   queue_tc_mapping[i][1]);
 +
 +   /* Event queue priority mapping */
 +   for (i = 0; queue_priority_mapping[i][0] != -1; i++)
 +   assign_priority_to_queue(j,
 +   queue_priority_mapping[i][0],
 +   queue_priority_mapping[i][1]);

 I know ti,edma-regions property is not currently being used, but we should
 future proof this by setting up DRAE for like done in probe:

 for (i = 0; i  info[j]-n_region; i++) {
 edma_write_array2(j, EDMA_DRAE, i, 0, 0x0);
 edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
 edma_write_array(j, EDMA_QRAE, i, 0x0);
 }
 
 That doesn't work for me. I'm running long-time tests here on a device
 which has a mwifiex connected to omap_hsmmc. The test procedure includes:
 
 a) a script on the device that puts the device to sleep some seconds
 after it has been woken up



 
 b) a script on a host that wakes up the device with wake-on-lan every 10
 seconds
 
 c) a flood ping that checks whether the device is responding

can you share above 2 (b  C) test scripts? (pastebin or inline if small)

thanks in advance

regards
Gururaja


 
 
 That precedure is running since a couple of hourse here, and it works
 well with both by v3 and v4 patches. Moving the functions to
 .suspend/resume _noirq doesn't seem to break anything.
 
 Setting QRAE to 0 as you mentioned above, however, makes the device fail
 at resume.
 
 +static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);

 I agree with Nishanth here, it is better to do this in .suspend/resume _noirq
 stage to rule out any ordering bugs that may show up in the future, since 
 such
 an issue already showed up in earlier testing.
 
 Alright, I already did that.
 
 I would appreciate it if you can make these 2 changes and post a v5. Thanks 
 for
 a lot for all the hardwork.
 
 No problem at all :)
 
 Acked-by: Joel Fernandes jo...@ti.com
 
 Still sure about that? What about your follow-up to your own reply?
 
 
 Many thanks for all the feedback!
 
 Daniel
 
 
 
 

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

Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks

2013-10-09 Thread Gururaja Hebbar 
On Wednesday 09 October 2013 11:33 AM, Fernandes, Joel wrote:
 Some temporary issues with my mua so forgive any artifacts in this email.
 
 On Oct 9, 2013, at 12:14 AM, Hebbar, Gururaja gururaja.heb...@ti.com 
 wrote:
 
 On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote:
 On 10/01/2013 10:04 AM, Daniel Mack wrote:
 This patch makes the edma driver resume correctly after suspend. Tested
 on an AM33xx platform with cyclic audio streams.

 The code was shamelessly taken from an ancient BSP tree.

 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
 arch/arm/common/edma.c | 133 
 +
 1 file changed, 133 insertions(+)

 ..snip..
 ..snip..

 +edma_cc[j]-context.ch_map =
 +kzalloc((sizeof(unsigned int) *
 + edma_cc[j]-num_channels), GFP_KERNEL);
 +edma_cc[j]-context.que_num =
 +kzalloc((sizeof(unsigned int) * 8), GFP_KERNEL);

 Can these allocations be moved to the suspend path? For systems that don't
 suspend/resume even once, I feel we shouldn't allocate memory that we don't 
 use.
 These allocations are better to do there.

 AFAIK, Suspend/resume should be quick. Allocating and deallocating on
 every iterating would be useless and time consuming.
 
 Nobody said allocate and deallocate on every iteration. Allocate once during 
 the first suspend call and then don't have to allocate on subsequent calls.

I couldn't find any code which allocates parameters inside suspend.
Could you show me some code which does this?

 
 As for suspend resume being quick, that argument can flipped the other way 
 too, booting should be quick which is far more frequent than suspend/resume. 
 Apart from the fact that we're not allocating useless memory we would never 
 use.
 

 Also this task is one time and quick.
 
 Exactly.

i was referring to allocating in probe call..

 

 Are there any systems (Linux based for now) which doesn't
 suspend/resume? I believe the probability is very less.
 
 Nobody talked about suspend/resume not being supported in Linux so not sure 
 what your argument is here.

I meant linux systems which doesn't go to suspend and resume. Not
suspend/resume feature.

Also, I was referring to your 1st comment ... For systems that don't
suspend/resume even once, 


regards
Gururaja

 
 regards,
 
 -Joel
 

 regards
 Gururaja


 -Joel

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


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

Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks

2013-10-08 Thread Gururaja Hebbar 
On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote:
 On 10/01/2013 10:04 AM, Daniel Mack wrote:
 This patch makes the edma driver resume correctly after suspend. Tested
 on an AM33xx platform with cyclic audio streams.

 The code was shamelessly taken from an ancient BSP tree.

 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
  arch/arm/common/edma.c | 133 
 +
  1 file changed, 133 insertions(+)


..snip..
..snip..
 
 +edma_cc[j]-context.ch_map =
 +kzalloc((sizeof(unsigned int) *
 + edma_cc[j]-num_channels), GFP_KERNEL);
 +edma_cc[j]-context.que_num =
 +kzalloc((sizeof(unsigned int) * 8), GFP_KERNEL);
 
 Can these allocations be moved to the suspend path? For systems that don't
 suspend/resume even once, I feel we shouldn't allocate memory that we don't 
 use.
 These allocations are better to do there.

AFAIK, Suspend/resume should be quick. Allocating and deallocating on
every iterating would be useless and time consuming.

Also this task is one time and quick.

Are there any systems (Linux based for now) which doesn't
suspend/resume? I believe the probability is very less.

regards
Gururaja

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

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

Re: [PATCH] ARM: omap: edma: add suspend suspend/resume hooks

2013-10-03 Thread Gururaja Hebbar 
On Tuesday 01 October 2013 07:21 PM, Daniel Mack wrote:
 This patch makes the edma driver resume correctly after suspend. Tested
 on an AM33xx platform with cyclic audio streams.

I believe you have confirmed below are the only registers/fields which
will lose context across suspend/resume

 
 The code was shamelessly taken from an ancient BSP tree.

Can you pick and merge below commit as well..

http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;h=b81bf04091986fa3893f31955564594567be3b61

Thanks  regards
Gururaja

 
 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
  arch/arm/common/edma.c | 133 
 +
  1 file changed, 133 insertions(+)
 
 diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
 index 2a72169..d787f14 100644
 --- a/arch/arm/common/edma.c
 +++ b/arch/arm/common/edma.c
 @@ -258,6 +258,20 @@ struct edma {
   void *data);
   void *data;
   } intr_data[EDMA_MAX_DMACH];
 +
 + struct {
 + struct edmacc_param *prm_set;
 + unsigned int *ch_map;   /* 64 registers */
 + unsigned int *que_num;  /* 8 registers */
 + unsigned int sh_esr;
 + unsigned int sh_esrh;
 + unsigned int sh_eesr;
 + unsigned int sh_eesrh;
 + unsigned int sh_iesr;
 + unsigned int sh_iesrh;
 + unsigned int que_tc_map;
 + unsigned int que_pri;
 + } context;
  };
  
  static struct edma *edma_cc[EDMA_MAX_CC];
 @@ -1655,6 +1669,16 @@ static int edma_probe(struct platform_device *pdev)
   memcpy_toio(edmacc_regs_base[j] + PARM_OFFSET(i),
   dummy_paramset, PARM_SIZE);
  
 + /* resume context */
 + edma_cc[j]-context.prm_set =
 + kzalloc((sizeof(struct edmacc_param) *
 +  edma_cc[j]-num_slots), GFP_KERNEL);
 + edma_cc[j]-context.ch_map =
 + kzalloc((sizeof(unsigned int) *
 +  edma_cc[j]-num_channels), GFP_KERNEL);
 + edma_cc[j]-context.que_num =
 + kzalloc((sizeof(unsigned int) * 8), GFP_KERNEL);
 +
   /* Mark all channels as unused */
   memset(edma_cc[j]-edma_unused, 0xff,
   sizeof(edma_cc[j]-edma_unused));
 @@ -1762,6 +1786,114 @@ static int edma_probe(struct platform_device *pdev)
   return 0;
  }
  
 +static int edma_pm_suspend(struct device *dev)
 +{
 + int i, j;
 +
 + pm_runtime_get_sync(dev);
 +
 + for (i = 0; i  arch_num_cc; i++) {
 + struct edma *ecc = edma_cc[i];
 +
 + /* backup channel data */
 + for (j = 0; j  ecc-num_channels; j++)
 + ecc-context.ch_map[j] =
 + edma_read_array(i, EDMA_DCHMAP, j);
 +
 + /* backup DMA Queue Number */
 + for (j = 0; j  8; j++)
 + ecc-context.que_num[j] =
 + edma_read_array(i, EDMA_DMAQNUM, j);
 +
 + /* backup DMA shadow Event Set data */
 + ecc-context.sh_esr = edma_shadow0_read_array(i, SH_ESR, 0);
 + ecc-context.sh_esrh = edma_shadow0_read_array(i, SH_ESR, 1);
 +
 + /* backup DMA Shadow Event Enable Set data */
 + ecc-context.sh_eesr =
 + edma_shadow0_read_array(i, SH_EER, 0);
 + ecc-context.sh_eesrh =
 + edma_shadow0_read_array(i, SH_EER, 1);
 +
 + /* backup DMA Shadow Interrupt Enable Set data */
 + ecc-context.sh_iesr =
 + edma_shadow0_read_array(i, SH_IER, 0);
 + ecc-context.sh_iesrh =
 + edma_shadow0_read_array(i, SH_IER, 1);
 +
 + ecc-context.que_tc_map = edma_read(i, EDMA_QUETCMAP);
 +
 + /* backup DMA Queue Priority data */
 + ecc-context.que_pri = edma_read(i, EDMA_QUEPRI);
 +
 + /* backup paramset */
 + for (j = 0; j  ecc-num_slots; j++)
 + memcpy_fromio(ecc-context.prm_set[j],
 +   edmacc_regs_base[i] + PARM_OFFSET(j),
 +   PARM_SIZE);
 + }
 +
 + pm_runtime_put_sync(dev);
 +
 + return 0;
 +}
 +
 +static int edma_pm_resume(struct device *dev)
 +{
 + int i, j;
 +
 + pm_runtime_get_sync(dev);
 +
 + for (i = 0; i  arch_num_cc; i++) {
 + struct edma *ecc = edma_cc[i];
 +
 + /* restore channel data */
 + for (j = 0; j  ecc-num_channels; j++) {
 + edma_write_array(i, EDMA_DCHMAP, j,
 +  ecc-context.ch_map[j]);
 + }
 +
 + /* restore DMA Queue Number */
 + for (j = 0; j  8; j++) {
 +

Re: [PATCH v4 1/4] ARM: OMAP2+: AM33XX: I2C Sleep/wake sequence support

2013-08-21 Thread Gururaja Hebbar 
On 8/20/2013 10:03 PM, Russ Dill wrote:
 On Sun, Aug 18, 2013 at 10:49 PM, Gururaja Hebbar
 gururaja.heb...@ti.com wrote:

 On 8/15/2013 4:04 AM, Russ Dill wrote:
 On Wed, Aug 14, 2013 at 3:18 AM, Gururaja Hebbar gururaja.heb...@ti.com 
 wrote:
 On 8/14/2013 3:50 AM, Russ Dill wrote:
 Changes since v1:
 * Rebased onto new am335x PM branch
 * Changed to use 5th param register



 [snip]
 [snip]

 +
 + wkup_m3_reset_data_pos();
 + if (am33xx_i2c_sleep_sequence) {
 + pos = wkup_m3_copy_data(am33xx_i2c_sleep_sequence,
 + i2c_sleep_sequence_sz);

 Why do we need to copy this data (same constant data) on every iteration?

 Because the CM3 firmware is reset after each suspend/resume cycle. The
 firmware reset handler reinitializes the DMEM.

 Well in that why can't the i2c payload be copied to UMEM?

 Thanks  regards
 Gururaja

 
 I've given this some thought, and gone back and forth a bit. UMEM is a
 bit more complicated because the linker decides what should go where.

but linker doesn't know about the copy and the location we are doing at
runtime.

 Also, it may be that in the future, either the PM for am335x or am43xx
 will be writing different sequences depending on the suspend
 mode/options.

still these info will be coming from DT and can be copied to UMEM. only
issue could be space.

 
 Is there a specific aspect of copying the data each suspend cycle that
 you are trying to fix? Is it just that the data could only be copied
 once? Or is it the latency of the copy?

Copy latency on every iterations is what worries me. it will surely add
a delay for suspend.

 
 Thanks!
 

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

Re: [PATCHv3 0/9] ARM: OMAP2+: AM33XX: Add suspend-resume support

2013-08-19 Thread Gururaja Hebbar 
On 8/6/2013 11:19 PM, Dave Gerlach wrote:
 Hi,
 
 This is the third version of the patch series for adding basic suspend-resume
 support for AM33XX, previously submitted by Vaibhav Bedia. This patchset 
 is based on 3.11-rc4 and depends on a forthcoming patchset from Suman Anna
 that adds mailbox support for the wkup_m3. The patches at [1], [2], and [3] 
 are 
 required for the pm code to properly suspend and resume.
 
 The PM code uses the firmware interface and expects the userspace to load 
 the WKUP_M3 binary before the suspend-resume functionality is made available.
 The binary file (and the source-code for WKUP_M3) can be obtained from the 
 'next2' branch at [4]. The WKUP_M3 binary can either be loaded post bootup 
 via the sysfs entry './sys/devices/ocp.2/wkup_m3.4/firmware' or it can be 
 included in the kernel image as part of the build process.
 
 Suspend to mem is tested on am335x-bone and am335x-evm.
 
 More details on AM335x suspend-resume are provided within the commit logs
 for each patch.

can you share the working repo which has all these patches applied?

Thanks  Regards
Gururaja

 
 Changes in v3:
 - Moved wkup_m3 code into separate driver
 - Split up ti_emif header move
 - Addressed clean-up comments
 - Removed mailbox patches
 - v2-v3 Discussion:
 http://marc.info/?l=linux-arm-kernelm=135698501821090w=4
 
 Changes in v2:
 - Broke patches up to isolate assembly code and build hookup.
 - Moved control module code to separate module
 - v1-v2 Discussion:
 http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129508.html
 
 Regards,
 Dave
 
 [1] http://marc.info/?l=linux-arm-kernelm=137303736714638w=4
 [2] http://marc.info/?l=linux-omapm=137303723114610w=4
 [3] http://marc.info/?l=linux-omapm=137401384611934w=4
 [4] 
 http://arago-project.org/git/projects/?p=am33x-cm3.git;a=shortlog;h=refs/heads/next2
 
 
 Dave Gerlach (1):
   memory: emif: Move EMIF register defines to include/linux/
 
 Vaibhav Bedia (8):
   ARM: OMAP2+: AM33XX: control: Add some control module registers and
 APIs
   ARM: OMAP: DTB: Update IRQ data for WKUP_M3
   ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec
   ARM: OMAP2+: AM33XX: Add assembly code for PM operations
   ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device
   ARM: OMAP: omap_device: Add APIs to enable and idle hwmods
   ARM: OMAP2+: AM33XX: Basic suspend resume support
   ARM: OMAP2+: AM33XX: Hookup AM33XX PM code into OMAP builds
 
  arch/arm/boot/dts/am33xx.dtsi   |1 +
  arch/arm/mach-omap2/Kconfig |7 +-
  arch/arm/mach-omap2/Makefile|2 +
  arch/arm/mach-omap2/board-generic.c |3 +-
  arch/arm/mach-omap2/common.c|   28 ++
  arch/arm/mach-omap2/common.h|   14 +
  arch/arm/mach-omap2/control.c   |   57 
  arch/arm/mach-omap2/control.h   |   54 
  arch/arm/mach-omap2/io.c|6 +
  arch/arm/mach-omap2/omap_device.c   |8 +
  arch/arm/mach-omap2/omap_device.h   |2 +
  arch/arm/mach-omap2/pm.c|3 +-
  arch/arm/mach-omap2/pm.h|5 +
  arch/arm/mach-omap2/pm33xx.c|  474 +
  arch/arm/mach-omap2/pm33xx.h|   77 +
  arch/arm/mach-omap2/sleep33xx.S |  350 ++
  arch/arm/mach-omap2/sram.c  |   10 +-
  arch/arm/mach-omap2/sram.h  |2 +
  arch/arm/mach-omap2/timer.c |   32 ++
  arch/arm/mach-omap2/wkup_m3.c   |  183 
  drivers/memory/emif.h   |  543 +-
  include/linux/ti_emif.h |  558 
 +++
  22 files changed, 1872 insertions(+), 547 deletions(-)
  create mode 100644 arch/arm/mach-omap2/pm33xx.c
  create mode 100644 arch/arm/mach-omap2/pm33xx.h
  create mode 100644 arch/arm/mach-omap2/sleep33xx.S
  create mode 100644 arch/arm/mach-omap2/wkup_m3.c
  create mode 100644 include/linux/ti_emif.h
 

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

Re: [PATCHv3 5/9] ARM: OMAP2+: AM33XX: Add assembly code for PM operations

2013-08-19 Thread Gururaja Hebbar 
Hi,

On 8/6/2013 11:19 PM, Dave Gerlach wrote:
 From: Vaibhav Bedia vaibhav.be...@ti.com
 
 In preparation for suspend-resume support for AM33XX, add
 the assembly file with the code which is copied to internal
 memory (OCMC RAM) during bootup and runs from there.
 
 As part of the low power entry (DeepSleep0 mode in AM33XX TRM),
 the code running from OCMC RAM does the following
 1. Stores the EMIF configuration
 2. Puts external memory in self-refresh
 3. Disables EMIF clock
 4. Executes WFI after writing to MPU_CLKCTRL register.
 

...snip...
...snip...


 + ldr r1, [r0, #EMIF_DDR_PHY_CTRL_1]
 + str r1, emif_rd_lat_val
 +
 + /* Put SDRAM in self-refresh */
 + ldr r1, [r0, #EMIF_POWER_MANAGEMENT_CONTROL]
 + orr r1, r1, #0xa0
 + str r1, [r0, #EMIF_POWER_MANAGEMENT_CTRL_SHDW]
 + str r1, [r0, #4]

This seems to be a bug which I had pointed out to VB earlier.

r0 --- base of emif module

r0 + 4 --- EMIF4_0_SDRAM_STATUS   === which is read only register


Above 2 lines should be as below

+   str r1, [r0, #EMIF_POWER_MANAGEMENT_CONTROL]
+   str r1, [r0, #EMIF_POWER_MANAGEMENT_CTRL_SHDW]


It works even with the bug because the Shadow register is updated and
that some how seems to take precedence.


Thanks  regards
Gururaja


 +
 + ldr r1, dram_sync_word  @ a dummy access to DDR as per spec
 + ldr r2, [r1, #0]

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

Re: [PATCH v4 1/4] ARM: OMAP2+: AM33XX: I2C Sleep/wake sequence support

2013-08-18 Thread Gururaja Hebbar 
On 8/15/2013 4:04 AM, Russ Dill wrote:
 On Wed, Aug 14, 2013 at 3:18 AM, Gururaja Hebbar gururaja.heb...@ti.com 
 wrote:
 On 8/14/2013 3:50 AM, Russ Dill wrote:
 Changes since v1:
 * Rebased onto new am335x PM branch
 * Changed to use 5th param register



[snip]
[snip]

 +
 + wkup_m3_reset_data_pos();
 + if (am33xx_i2c_sleep_sequence) {
 + pos = wkup_m3_copy_data(am33xx_i2c_sleep_sequence,
 + i2c_sleep_sequence_sz);

 Why do we need to copy this data (same constant data) on every iteration?
 
 Because the CM3 firmware is reset after each suspend/resume cycle. The
 firmware reset handler reinitializes the DMEM.

Well in that why can't the i2c payload be copied to UMEM?

Thanks  regards
Gururaja

 
 + /* Lower 16 bits stores offset to sleep sequence */
 + param4 = ~0x;
 + param4 |= pos;
 + }
 +
 + if (am33xx_i2c_wake_sequence) {
 + pos = wkup_m3_copy_data(am33xx_i2c_wake_sequence,
 + i2c_wake_sequence_sz);
 + /* Upper 16 bits stores offset to wake sequence */
 + param4 = ~0x;
 + param4 |= pos  16;
 + }
 +

 Seems above entire change can be done only once.

   am33xx_pm-ipc.sleep_mode   = IPC_CMD_DS0;
   am33xx_pm-ipc.param1   = DS_IPC_DEFAULT;
   am33xx_pm-ipc.param2   = DS_IPC_DEFAULT;
 + am33xx_pm-ipc.param4   = param4;

   am33xx_pm_ipc_cmd(am33xx_pm-ipc);

 @@ -386,6 +413,62 @@ static int __init am33xx_map_emif(void)
   return 0;
  }

 +static int __init am33xx_setup_sleep_sequence(void)
 +{
 + int ret;
 + int sz;
 + const void *prop;
 + struct device *dev;
 + u32 freq_hz = 10;

 Magic number?
 
 It's taken from drivers/i2c/busses/i2c-omap.c
 
 u32 freq = 10; /* default to 10 Hz */
 
 I'll add a comment to that effect.
 
 + unsigned short freq_khz;
 +
 + /*
 +  * We put the device tree node in the I2C controller that will
 +  * be sending the sequence. i2c1 is the only controller that can
 +  * be accessed by the firmware as it is the only controller in the
 +  * WKUP domain.

 and on which the PMIC sits I believe?
 
 Yes, but this code is designed not to be PMIC specific as one could
 chose to regulate VDD_CORE with any PMIC, or even with a standalone
 I2C controlled regulator.
 
 +  */
 + dev = omap_device_get_by_hwmod_name(i2c1);
 + if (IS_ERR(dev))
 + return PTR_ERR(dev);
 +
 + of_property_read_u32(dev-of_node, clock-frequency, freq_hz);
 + freq_khz = freq_hz / 1000;

 Magic number?
 
 Nah, converting between metric prefixes this way is pretty common in the 
 kernel.
 
 +
 + prop = of_get_property(dev-of_node, sleep_sequence, sz);
 + if (prop) {
 + /*
 +  * Length is sequence length + 2 bytes for freq_khz, and 1
 +  * byte for terminator.
 +  */
 + am33xx_i2c_sleep_sequence = kzalloc(sz + 3, GFP_KERNEL);
 + if (!am33xx_i2c_sleep_sequence)
 + return -ENOMEM;
 + put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
 + memcpy(am33xx_i2c_sleep_sequence + 2, prop, sz);

 so, looking at entire code, it seems there is 3 memory space for same
 data (or 1 original + 2 copy)

 1. in DT
 2. in am33xx_i2c_[sleep/wake]_sequence
 3. in SRAm by call to wkup_m3_copy_data()

 why not directly copy to SRAM from DT?
 
 As pointed out above, the firmware reset handler would wipe it out.
 
 + i2c_sleep_sequence_sz = sz + 3;
 + }
 +
 + prop = of_get_property(dev-of_node, wake_sequence, sz);
 + if (prop) {
 + am33xx_i2c_wake_sequence = kzalloc(sz + 3, GFP_KERNEL);
 + if (!am33xx_i2c_wake_sequence) {
 + ret = -ENOMEM;
 + goto cleanup_sleep;
 + }
 + put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
 + memcpy(am33xx_i2c_wake_sequence + 2, prop, sz);
 + i2c_wake_sequence_sz = sz + 3;
 + }
 +
 + return 0;
 +
 +cleanup_sleep:
 + kfree(am33xx_i2c_sleep_sequence);
 + am33xx_i2c_sleep_sequence = NULL;
 + return ret;
 +}
 +
  int __init am33xx_pm_init(void)
  {
   int ret;
 @@ -451,6 +534,12 @@ int __init am33xx_pm_init(void)
   }
   }

 + ret = am33xx_setup_sleep_sequence();
 + if (ret) {
 + pr_err(Error fetching I2C sleep/wake sequence\n);
 + goto err;
 + }
 +
   (void) clkdm_for_each(omap_pm_clkdms_setup, NULL);

   /* CEFUSE domain can be turned off post bootup */
 diff --git a/arch/arm/mach-omap2/pm33xx.h b/arch/arm/mach-omap2/pm33xx.h
 index befdd11..d0f08a5 100644
 --- a/arch/arm/mach-omap2/pm33xx.h
 +++ b/arch/arm/mach-omap2/pm33xx.h
 @@ -52,6 +52,8 @@ struct forced_standby_module {
  };

  int wkup_m3_copy_code(const u8

Re: [PATCH v4 1/4] ARM: OMAP2+: AM33XX: I2C Sleep/wake sequence support

2013-08-16 Thread Gururaja Hebbar 
On 8/15/2013 4:04 AM, Russ Dill wrote:
 On Wed, Aug 14, 2013 at 3:18 AM, Gururaja Hebbar gururaja.heb...@ti.com 
 wrote:
 On 8/14/2013 3:50 AM, Russ Dill wrote:
 Changes since v1:
 * Rebased onto new am335x PM branch
 * Changed to use 5th param register

 Changes since v2:
 * Passes I2C bus speed in kHz to M3 firmware

 Changes since v3:
 * Rebased to 3.11-rc3, moves some functionality to wkup_m3.c
 * Additional comments
 * Added device-tree binding documentation

 AFAIK, Change logs should come below scissors line (---).
 However there was some discussion to keep it in the commit message.
 Don't know what happened to it finally. But till date, all revision
 patch set has the change log under the scissor line.
 
 Will fix is v2.
 

 This patch adds the ability to pass an I2C sleep sequence and wake sequence
 to the Cortex-M3. This is useful for adjusting voltages during sleep that
 cannot be lowered while SDRAM is active. A modified M3 firmware with I2C
 support is required.

 Each sequence is a series of I2C transfers in the form:

 u8 length | u8 chip address | u8 byte0/reg address | u8 byte 1 | u8 byte n 
 ...

 The length indicates the number of bytes to transfer, including the register
 address. The length of each transfer is limited by the I2C buffer size of
 32 bytes.

 The sequences are taken from the i2c1 node in the device tree. The property
 name for the sleep sequence is sleep_sequence and the property name for
 the wake sequence is wake_sequence. Each property should be an array of
 bytes.

 No actions are performed if the properties are not present in the device
 tree.

 Signed-off-by: Russ Dill russ.d...@ti.com
 ---
  .../devicetree/bindings/i2c/i2c-suspend-resume.txt | 44 +++
  arch/arm/mach-omap2/control.c  |  1 +
  arch/arm/mach-omap2/pm33xx.c   | 89 
 ++
  arch/arm/mach-omap2/pm33xx.h   |  2 +
  arch/arm/mach-omap2/wkup_m3.c  | 57 --
  5 files changed, 186 insertions(+), 7 deletions(-)
  create mode 100644 
 Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt

 diff --git a/Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt 
 b/Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt
 new file mode 100644
 index 000..af19372
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt
 @@ -0,0 +1,44 @@
 +I2C suspend/resume sequences
 +
 +This provides the ability for a simple I2C sequence to be written at
 +suspend time and resume time. This is for sequences that cannot be written
 +by the I2C bus driver for reasons such as needing to be run from SRAM
 +or needing to be written by firmware.
 +
 +The sequence is composed of messages. Each message contains a length byte,
 +an address byte, and then the message.
 +
 +Optional properties:
 +- sleep_sequence
 + I2C sequence to write during suspend
 +
 +- wake_sequence
 + I2C sequence to write during wake
 +
 +Examples :
 +
 +i2c0: i2c@0 {
 + /* Set OPP50 (0.95V) for VDD core */
 + sleep_sequence = /bits/ 8 
 + 0x02 0x24 0x0b 0x6d /* Password unlock 1 */
 + 0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
 + 0x02 0x24 0x0b 0x6d /* Password unlock 2 */
 + 0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 1 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 2 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + ;

 This data is not related to i2c. Right? Then why is it under i2c dt node?

 There is already a wakeup node (wkup_m3) and a pmic node (pmic node for
 respective boards). Can't these details go under those nodes?
 
 The i2c node was chosen for several reasons. First that the bus speed
 is included in the message to the CM3 firmware. The bus speed and
 sequence can then be read from the same device tree node. Second so
 it's clear which I2C bus this is being sent out on. Including it under
 the PMIC node would mean that the code would have to search each child
 of the I2C bus for a sequence and if multiple children had sequences,
 it would not know in which order to send those sequences out.

Agreed

 
 +
 + /* Set OPP100 (1.10V) for VDD core */
 + wake_sequence = /bits/ 8 
 + 0x02 0x24 0x0b 0x6d /* Password unlock 1 */
 + 0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
 + 0x02 0x24 0x0b 0x6d /* Password unlock 2 */
 + 0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 1 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 2 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + ;
 +}
 diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
 index 934041a..dfbd5f0 100644
 --- a/arch/arm

Re: [PATCH v3 0/2] rtc: omap: update AM335x rtc ip revision

2013-08-16 Thread Gururaja Hebbar 
Hi All,

Kindly ignore this series. there is mistake/typo because of which it
will not directly apply. I will resend the patch-set.

sorry for the trouble.

regards
Gururaja

On 8/16/2013 5:06 PM, Hebbar, Gururaja wrote:
 The syntax of compatible property in DT is to mention the Most specific
 match to most generic match.
 
 Since AM335x is the platform with latest IP revision, add it 1st in
 the device id table.
 
 This way, we can add new matching compatible as 1st and maintain old
 compatible string for backwards compatibility.
 
 ex:
   compatible = ti,am3352-rtc, ti,da830-rtc;
 
 Also, update am335x .dtsi file (am33xx.dtsi) as above.
 
 Note:
 This is a part of previously submitted patch-set [1]. Out of 4 patches,
 3 was accepted and 1 got an acceptable NAK. Hence resubmitting only the
 rejected + updated changes.
 
 Changes in V3:
   - As per Mark Rutland's suggestion [2], maintain old compatible
 string and add the new matching string as the 1st compatible.
 
 Changes in V2:
   - Coding style corrections (remove extra space, use lower case
 for hex numbers
   - use prefix ARM: for commit subject keeping with arch/arm
 convention)
   - use [AM/am]3352 instead of [AM/am]335x to keep the all
 usages in sync.
   - Use index defined for struct members so they remain in sync
   - Add new compatible to existing one so that when driver
 supports enhanced features of hardware, they are available
 to the user else the basic functionality still works
 
 [2]
 https://lkml.org/lkml/2013/7/3/74
 [1]
 https://lkml.org/lkml/2013/8/1/442
 
 Hebbar Gururaja (1):
   ARM: dts: AM33XX: update rtc node compatibility
 
 Hebbar, Gururaja (1):
   rtc: omap: update of_device_id to reflect latest ip revisions
 
  arch/arm/boot/dts/am33xx.dtsi |2 +-
  drivers/rtc/rtc-omap.c|6 +++---
  2 files changed, 4 insertions(+), 4 deletions(-)
 

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

Re: [PATCH v4 2/4] ARM: dts: add AM33XX vdd core opp50 suspend for Beaglebone.

2013-08-14 Thread Gururaja Hebbar 
On 8/14/2013 3:50 AM, Russ Dill wrote:
 Changes since v1:
 * Rebased onto new am335x PM branch
 
 This adds a sleep and wake sequence to set the VDD core voltage to the
 OPP50 level, 0.950V. This saves power during suspend. The sequences are
 specific to the Beaglebone layout and PMIC, the TPS65217. The sequences
 are written out by the Cortex-M3.
 
 Signed-off-by: Russ Dill russ.d...@ti.com
 ---
  arch/arm/boot/dts/am335x-bone.dts | 25 -
  1 file changed, 24 insertions(+), 1 deletion(-)
 
 diff --git a/arch/arm/boot/dts/am335x-bone.dts 
 b/arch/arm/boot/dts/am335x-bone.dts
 index 444b4ed..3f6528d 100644
 --- a/arch/arm/boot/dts/am335x-bone.dts
 +++ b/arch/arm/boot/dts/am335x-bone.dts
 @@ -127,10 +127,33 @@
   status = okay;
   clock-frequency = 40;
  
 + /* Set OPP50 (0.95V) for VDD core */
 + sleep_sequence = /bits/ 8 

For user readability, can you mention the PMIC used here as a comment?

 + 0x02 0x24 0x0b 0x6d /* Password unlock 1 */
 + 0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
 + 0x02 0x24 0x0b 0x6d /* Password unlock 2 */
 + 0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 1 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 2 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + ;
 +
 + /* Set OPP100 (1.10V) for VDD core */
 + wake_sequence = /bits/ 8 
 + 0x02 0x24 0x0b 0x6d /* Password unlock 1 */
 + 0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
 + 0x02 0x24 0x0b 0x6d /* Password unlock 2 */
 + 0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 1 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 2 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + ;
 +
   tps: tps@24 {
   reg = 0x24;
   };
 -
   };
   };
  
 

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

Re: [PATCH v4 1/4] ARM: OMAP2+: AM33XX: I2C Sleep/wake sequence support

2013-08-14 Thread Gururaja Hebbar 
On 8/14/2013 3:50 AM, Russ Dill wrote:
 Changes since v1:
 * Rebased onto new am335x PM branch
 * Changed to use 5th param register
 
 Changes since v2:
 * Passes I2C bus speed in kHz to M3 firmware
 
 Changes since v3:
 * Rebased to 3.11-rc3, moves some functionality to wkup_m3.c
 * Additional comments
 * Added device-tree binding documentation

AFAIK, Change logs should come below scissors line (---).
However there was some discussion to keep it in the commit message.
Don't know what happened to it finally. But till date, all revision
patch set has the change log under the scissor line.

 
 This patch adds the ability to pass an I2C sleep sequence and wake sequence
 to the Cortex-M3. This is useful for adjusting voltages during sleep that
 cannot be lowered while SDRAM is active. A modified M3 firmware with I2C
 support is required.
 
 Each sequence is a series of I2C transfers in the form:
 
 u8 length | u8 chip address | u8 byte0/reg address | u8 byte 1 | u8 byte n ...
 
 The length indicates the number of bytes to transfer, including the register
 address. The length of each transfer is limited by the I2C buffer size of
 32 bytes.
 
 The sequences are taken from the i2c1 node in the device tree. The property
 name for the sleep sequence is sleep_sequence and the property name for
 the wake sequence is wake_sequence. Each property should be an array of
 bytes.
 
 No actions are performed if the properties are not present in the device
 tree.
 
 Signed-off-by: Russ Dill russ.d...@ti.com
 ---
  .../devicetree/bindings/i2c/i2c-suspend-resume.txt | 44 +++
  arch/arm/mach-omap2/control.c  |  1 +
  arch/arm/mach-omap2/pm33xx.c   | 89 
 ++
  arch/arm/mach-omap2/pm33xx.h   |  2 +
  arch/arm/mach-omap2/wkup_m3.c  | 57 --
  5 files changed, 186 insertions(+), 7 deletions(-)
  create mode 100644 
 Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt
 
 diff --git a/Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt 
 b/Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt
 new file mode 100644
 index 000..af19372
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt
 @@ -0,0 +1,44 @@
 +I2C suspend/resume sequences
 +
 +This provides the ability for a simple I2C sequence to be written at
 +suspend time and resume time. This is for sequences that cannot be written
 +by the I2C bus driver for reasons such as needing to be run from SRAM
 +or needing to be written by firmware.
 +
 +The sequence is composed of messages. Each message contains a length byte,
 +an address byte, and then the message.
 +
 +Optional properties:
 +- sleep_sequence
 + I2C sequence to write during suspend
 +
 +- wake_sequence
 + I2C sequence to write during wake
 +
 +Examples :
 +
 +i2c0: i2c@0 {
 + /* Set OPP50 (0.95V) for VDD core */
 + sleep_sequence = /bits/ 8 
 + 0x02 0x24 0x0b 0x6d /* Password unlock 1 */
 + 0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
 + 0x02 0x24 0x0b 0x6d /* Password unlock 2 */
 + 0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 1 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 2 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + ;

This data is not related to i2c. Right? Then why is it under i2c dt node?

There is already a wakeup node (wkup_m3) and a pmic node (pmic node for
respective boards). Can't these details go under those nodes?

 +
 + /* Set OPP100 (1.10V) for VDD core */
 + wake_sequence = /bits/ 8 
 + 0x02 0x24 0x0b 0x6d /* Password unlock 1 */
 + 0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
 + 0x02 0x24 0x0b 0x6d /* Password unlock 2 */
 + 0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 1 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + 0x02 0x24 0x0b 0x6c /* Password unlock 2 */
 + 0x02 0x24 0x11 0x86 /* Apply DCDC changes */
 + ;
 +}
 diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
 index 934041a..dfbd5f0 100644
 --- a/arch/arm/mach-omap2/control.c
 +++ b/arch/arm/mach-omap2/control.c
 @@ -639,6 +639,7 @@ void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
   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);
 + omap_ctrl_writel(data-param4, AM33XX_CONTROL_IPC_MSG_REG5);
  }
  
  int am33xx_pm_status(void)
 diff --git a/arch/arm/mach-omap2/pm33xx.c b/arch/arm/mach-omap2/pm33xx.c
 index d291c76..8880da3 100644
 --- a/arch/arm/mach-omap2/pm33xx.c
 +++ b/arch/arm/mach-omap2/pm33xx.c
 @@ -29,6 +29,7 @@

Re: [Patch V2 4/4] ARM: dts: AM33XX: update rtc node compatibility

2013-08-02 Thread Gururaja Hebbar 

On 8/1/2013 10:35 PM, Mark Rutland wrote:

On Tue, Jul 30, 2013 at 05:21:14PM +0100, Sekhar Nori wrote:

On 7/30/2013 8:25 PM, Mark Rutland wrote:

On Tue, Jul 30, 2013 at 06:05:52AM +0100, Gururaja Hebbar wrote:

Hi,

On 7/3/2013 2:17 PM, Hebbar Gururaja wrote:

Since AM33xx  RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up.

Update the rtc compatible property to ti,am3352-rtc to enable handling
of this feature inside rtc-omap driver.


The other 2 rtc driver related patches have been pulled up. If you have
no comments, can you please pull this up.

Regards
Gururaja



Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com
---
:100644 100644 77aa1b0... dde180a... M  arch/arm/boot/dts/am33xx.dtsi
   arch/arm/boot/dts/am33xx.dtsi |2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 77aa1b0..dde180a 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -297,7 +297,7 @@
};

rtc@44e3e000 {
-   compatible = ti,da830-rtc;
+   compatible = ti,am3352-rtc;


Given that this is backwards compatible with the old binding, it would
be nicer to have this as:

compatible = ti,am3352-rtc, ti,da830-rtc;

We must get into the habit of changing dts files in a
backwards-compatible fashion.


Right, I suggested this when v1 was posted. It turns out the current
kernel does not handle the compatilble list correctly and the string
selected actually depends on the order in which it appears in match
table in driver instead.

I saw there were patches being discussed to fix this issue, but until
that is fixed, we cannot really use what you (and I before) suggested.


A temporary solution would be to list the ti,am3352-rtc first in the
of_match_table kernel-side.


If above method is followed, then it would cause trouble on davinci 
platform because this rtc-omap driver is also used by Davinci platform.
On davinci Plaform the driver would match with ti,am3352-rtc for 
compatible.


Regards
Gururaja


That way you can keep the old compatible
string in the dts compatible list, and maintain backwards compatibilty
with older kernels.

Later if the way Linux matches compatible strings is changed, this
shouldn't break the probe order, and the of_match_table order could be
changed.

Have I missed something?

Thanks,
Mark.



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

Re: [Patch V2 4/4] ARM: dts: AM33XX: update rtc node compatibility

2013-08-02 Thread Gururaja Hebbar 

On 8/2/2013 4:50 PM, Mark Rutland wrote:

On Fri, Aug 02, 2013 at 12:07:36PM +0100, Gururaja Hebbar wrote:

On 8/1/2013 10:35 PM, Mark Rutland wrote:

On Tue, Jul 30, 2013 at 05:21:14PM +0100, Sekhar Nori wrote:

On 7/30/2013 8:25 PM, Mark Rutland wrote:

On Tue, Jul 30, 2013 at 06:05:52AM +0100, Gururaja Hebbar wrote:

Hi,

On 7/3/2013 2:17 PM, Hebbar Gururaja wrote:

Since AM33xx  RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up.

Update the rtc compatible property to ti,am3352-rtc to enable handling
of this feature inside rtc-omap driver.


The other 2 rtc driver related patches have been pulled up. If you have
no comments, can you please pull this up.

Regards
Gururaja



Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com
---
:100644 100644 77aa1b0... dde180a... M  arch/arm/boot/dts/am33xx.dtsi
arch/arm/boot/dts/am33xx.dtsi |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 77aa1b0..dde180a 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -297,7 +297,7 @@
};

rtc@44e3e000 {
-   compatible = ti,da830-rtc;
+   compatible = ti,am3352-rtc;


Given that this is backwards compatible with the old binding, it would
be nicer to have this as:

compatible = ti,am3352-rtc, ti,da830-rtc;

We must get into the habit of changing dts files in a
backwards-compatible fashion.


Right, I suggested this when v1 was posted. It turns out the current
kernel does not handle the compatilble list correctly and the string
selected actually depends on the order in which it appears in match
table in driver instead.

I saw there were patches being discussed to fix this issue, but until
that is fixed, we cannot really use what you (and I before) suggested.


A temporary solution would be to list the ti,am3352-rtc first in the
of_match_table kernel-side.


If above method is followed, then it would cause trouble on davinci
platform because this rtc-omap driver is also used by Davinci platform.
On davinci Plaform the driver would match with ti,am3352-rtc for
compatible.


Sorry, I don't follow. Does the davinci dt have ti,am3352-rtc in it's
compatible string list?


no no. Davinci .dts uses ti,da830-rtc.

I was saying if we reverse the order of of_device_id structure in 
rtc-omap driver, then it would work nicely for am335x but will cause 
trouble on davinci platform.




If so, and it's not compatible, the dts is wrong.

If not, then we won't use the behaviour specific to ti,am3352-rtc, and
there's no problem.

What trouble would this cause?

Thanks,
Mark.



Regards
Gururaja


That way you can keep the old compatible
string in the dts compatible list, and maintain backwards compatibilty
with older kernels.

Later if the way Linux matches compatible strings is changed, this
shouldn't break the probe order, and the of_match_table order could be
changed.

Have I missed something?

Thanks,
Mark.




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



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

Re: [Patch V2 4/4] ARM: dts: AM33XX: update rtc node compatibility

2013-07-29 Thread Gururaja Hebbar 

Hi,

On 7/3/2013 2:17 PM, Hebbar Gururaja wrote:

Since AM33xx  RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up.

Update the rtc compatible property to ti,am3352-rtc to enable handling
of this feature inside rtc-omap driver.


The other 2 rtc driver related patches have been pulled up. If you have 
no comments, can you please pull this up.


Regards
Gururaja



Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com
---
:100644 100644 77aa1b0... dde180a... M  arch/arm/boot/dts/am33xx.dtsi
  arch/arm/boot/dts/am33xx.dtsi |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 77aa1b0..dde180a 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -297,7 +297,7 @@
};

rtc@44e3e000 {
-   compatible = ti,da830-rtc;
+   compatible = ti,am3352-rtc;
reg = 0x44e3e000 0x1000;
interrupts = 75
  76;



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

Re: [Patch V2 3/4] rtc: omap: add rtc wakeup support to alarm events

2013-07-24 Thread Gururaja Hebbar 

Hi Andrew,

On 7/3/2013 2:17 PM, Hebbar Gururaja wrote:

On some platforms (like AM33xx), a special register (RTC_IRQWAKEEN)
is available to enable Alarm Wakeup feature. This register needs to be
properly handled for the rtcwake to work properly.

Platforms using such IP should set ti,am3352-rtc in rtc device dt
compatibility node.


I just checked that the 1st patch in this series is pulled in and this
patch [3/4] is not pulled . If you do not have any comments, can you 
pull this one also. I have already got Acks from Sekhar  Kevin for the 
same.


Thanks  regards
Gururaja



Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com
Acked-by: Kevin Hilman khil...@linaro.org
Acked-by: Sekhar Nori nsek...@ti.com
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Rob Landley r...@landley.net
Cc: Alessandro Zummo a.zu...@towertech.it
Cc: rtc-li...@googlegroups.com
Cc: devicetree-disc...@lists.ozlabs.org
Cc: linux-...@vger.kernel.org
---
Changes in V2:
- Coding style corrections (use lower case for hex numbers)
- use [AM/am]3352 instead of [AM/am]335x to keep the all
  usages in sync.
- Use index defined for struct members so they remain in sync

:100644 100644 b47aa41... 5a0f02d... M  
Documentation/devicetree/bindings/rtc/rtc-omap.txt
:100644 100644 761919d... c2e18fe... M  drivers/rtc/rtc-omap.c
  Documentation/devicetree/bindings/rtc/rtc-omap.txt |6 +-
  drivers/rtc/rtc-omap.c |   60 +---
  2 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt 
b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
index b47aa41..5a0f02d 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
@@ -1,7 +1,11 @@
  TI Real Time Clock

  Required properties:
-- compatible: ti,da830-rtc
+- compatible:
+   - ti,da830-rtc  - for RTC IP used similar to that on DA8xx SoC family.
+   - ti,am3352-rtc - for RTC IP used similar to that on AM335x SoC 
family.
+   This RTC IP has special WAKE-EN Register to enable
+   Wakeup generation for event Alarm.
  - reg: Address range of rtc register set
  - interrupts: rtc timer, alarm interrupts in order
  - interrupt-parent: phandle for the interrupt controller
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 761919d..c2e18fe 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -72,6 +72,8 @@
  #define OMAP_RTC_KICK0_REG0x6c
  #define OMAP_RTC_KICK1_REG0x70

+#define OMAP_RTC_IRQWAKEEN 0x7c
+
  /* OMAP_RTC_CTRL_REG bit fields: */
  #define OMAP_RTC_CTRL_SPLIT   (17)
  #define OMAP_RTC_CTRL_DISABLE (16)
@@ -96,12 +98,21 @@
  #define OMAP_RTC_INTERRUPTS_IT_ALARM(13)
  #define OMAP_RTC_INTERRUPTS_IT_TIMER(12)

+/* OMAP_RTC_IRQWAKEEN bit fields: */
+#define OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN(11)
+
  /* OMAP_RTC_KICKER values */
  #define   KICK0_VALUE 0x83e70b13
  #define   KICK1_VALUE 0x95a4f1e0

  #define   OMAP_RTC_HAS_KICKER 0x1

+/*
+ * Few RTC IP revisions has special WAKE-EN Register to enable Wakeup
+ * generation for event Alarm.
+ */
+#defineOMAP_RTC_HAS_IRQWAKEEN  0x2
+
  static void __iomem   *rtc_base;

  #define rtc_read(addr)readb(rtc_base + (addr))
@@ -301,12 +312,18 @@ static struct rtc_class_ops omap_rtc_ops = {
  static int omap_rtc_alarm;
  static int omap_rtc_timer;

-#defineOMAP_RTC_DATA_DA830_IDX 1
+#defineOMAP_RTC_DATA_AM3352_IDX1
+#defineOMAP_RTC_DATA_DA830_IDX 2

  static struct platform_device_id omap_rtc_devtype[] = {
{
.name   = DRIVER_NAME,
-   }, {
+   },
+   [OMAP_RTC_DATA_AM3352_IDX] = {
+   .name   = am3352-rtc,
+   .driver_data = OMAP_RTC_HAS_KICKER | OMAP_RTC_HAS_IRQWAKEEN,
+   },
+   [OMAP_RTC_DATA_DA830_IDX] = {
.name   = da830-rtc,
.driver_data = OMAP_RTC_HAS_KICKER,
},
@@ -318,6 +335,9 @@ static const struct of_device_id omap_rtc_of_match[] = {
{   .compatible = ti,da830-rtc,
.data   = omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX],
},
+   {   .compatible = ti,am3352-rtc,
+   .data   = omap_rtc_devtype[OMAP_RTC_DATA_AM3352_IDX],
+   },
{},
  };
  MODULE_DEVICE_TABLE(of, omap_rtc_of_match);
@@ -466,16 +486,28 @@ static u8 irqstat;

  static int omap_rtc_suspend(struct device *dev)
  {
+   u8 irqwake_stat;
+   struct platform_device *pdev = to_platform_device(dev);
+   const struct platform_device_id *id_entry =
+   platform_get_device_id(pdev);
+
irqstat = rtc_read(OMAP_RTC_INTERRUPTS_REG);

/*

Re: [RESEND PATCH V2] mmc: omap_hsmmc: Enable HSPE bit for high speed cards

2012-10-31 Thread Gururaja Hebbar 

On 10/31/2012 3:34 PM, Felipe Balbi wrote:

Hi,

On Wed, Oct 31, 2012 at 01:58:35PM +0530, Hebbar, Gururaja wrote:

HSMMC IP on AM33xx need a special setting to handle High-speed cards.
Other platforms like TI81xx, OMAP4 may need this as-well. This depends
on the HSMMC IP timing closure done for the high speed cards.

 From AM335x TRM (SPRUH73F - 18.3.12 Output Signals Generation)

The MMC/SD/SDIO output signals can be driven on either falling edge or
rising edge depending on the SD_HCTL[2] HSPE bit. This feature allows
to reach better timing performance, and thus to increase data transfer
frequency.

There are few pre-requisites for enabling the HSPE bit
- Controller should support High-Speed-Enable Bit and
- Controller should not be using DDR Mode and
- Controller should advertise that it supports High Speed in
   capabilities register and
- MMC/SD clock coming out of controller  25MHz

Note:
The implementation reuses the output of calc_divisor() so as to reduce
code addition.

Signed-off-by: Hebbar, Gururaja gururaja.heb...@ti.com
---
Rebased on mmc-next (v3.7.0-rc1)
Only Build tested since EDMA support for AM335x is not yet added

Changes in V2
- Added note in commit message about code re-use
- replaced ((a  BIT(n) == BIT(n))) with (a  BIT(n)) since
  effectively both are same.

:100644 100644 be76a23... ed271fc... M  
Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
:100644 100644 8b4e4f2... 346af5b... M  arch/arm/plat-omap/include/plat/mmc.h
:100644 100644 54bfd0c... 5ea4c9d... M  drivers/mmc/host/omap_hsmmc.c
  .../devicetree/bindings/mmc/ti-omap-hsmmc.txt  |1 +
  arch/arm/plat-omap/include/plat/mmc.h  |1 +
  drivers/mmc/host/omap_hsmmc.c  |   30 +++-
  3 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt 
b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
index be76a23..ed271fc 100644
--- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
+++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
@@ -19,6 +19,7 @@ ti,dual-volt: boolean, supports dual voltage cards
  supply-name examples are vmmc, vmmc_aux etc
  ti,non-removable: non-removable slot (like eMMC)
  ti,needs-special-reset: Requires a special softreset sequence
+ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High 
Speed
  
  Example:

mmc1: mmc@0x4809c000 {
diff --git a/arch/arm/plat-omap/include/plat/mmc.h 
b/arch/arm/plat-omap/include/plat/mmc.h
index 8b4e4f2..346af5b 100644
--- a/arch/arm/plat-omap/include/plat/mmc.h
+++ b/arch/arm/plat-omap/include/plat/mmc.h
@@ -126,6 +126,7 @@ struct omap_mmc_platform_data {
/* we can put the features above into this variable */
  #define HSMMC_HAS_PBIAS   (1  0)
  #define HSMMC_HAS_UPDATED_RESET   (1  1)
+#define HSMMC_HAS_HSPE_SUPPORT (1  2)
unsigned features;
  
  		int switch_pin;			/* gpio (card detect) */

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 54bfd0c..5ea4c9d 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -62,6 +62,7 @@
  
  #define VS18			(1  26)

  #define VS30  (1  25)
+#define HSS(1  21)
  #define SDVS18(0x5  9)
  #define SDVS30(0x6  9)
  #define SDVS33(0x7  9)
@@ -89,6 +90,7 @@
  #define MSBS  (1  5)
  #define BCE   (1  1)
  #define FOUR_BIT  (1  1)
+#define HSPE   (1  2)
  #define DDR   (1  19)
  #define DW8   (1  5)
  #define CC0x1
@@ -489,6 +491,7 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host 
*host)
struct mmc_ios *ios = host-mmc-ios;
unsigned long regval;
unsigned long timeout;
+   unsigned long clkdiv;
  
  	dev_vdbg(mmc_dev(host-mmc), Set clock to %uHz\n, ios-clock);
  
@@ -496,7 +499,8 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host *host)
  
  	regval = OMAP_HSMMC_READ(host-base, SYSCTL);

regval = regval  ~(CLKD_MASK | DTO_MASK);
-   regval = regval | (calc_divisor(host, ios)  6) | (DTO  16);
+   clkdiv = calc_divisor(host, ios);
+   regval = regval | (clkdiv  6) | (DTO  16);
OMAP_HSMMC_WRITE(host-base, SYSCTL, regval);
OMAP_HSMMC_WRITE(host-base, SYSCTL,
OMAP_HSMMC_READ(host-base, SYSCTL) | ICE);
@@ -507,6 +511,27 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host 
*host)
 time_before(jiffies, timeout))
cpu_relax();
  
+	/*

+* Enable High-Speed Support
+* Pre-Requisites
+*  - Controller should support High-Speed-Enable Bit
+*  - Controller should not be using DDR Mode
+*  - Controller should advertise that it supports High Speed
+