gpio/omap: Some more driver cleanup and fixes

There is no more need to have saved_wakeup because bank->context.wake_en already holds that value. So getting rid of read/write operation associated with this field.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
 drivers/gpio/gpio-omap.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/gpio/gpio-omap.c +0 -1

index 3a4f151..3b91ade 100644
@@ -57,7 +57,6 @@ struct gpio_bank {
 	u16 irq;
 	int irq_base;
 	struct irq_domain *domain;
-	u32 saved_wakeup;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
 	struct gpio_regs context;

Hi,

Just bumped across this patch and have a query.

Quoted 26 lines · Tarun Kanti DebBarma

There is no more need to have saved_wakeup because bank->context.wake_en already holds that value. So getting rid of read/write operation associated with this field.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
 drivers/gpio/gpio-omap.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/gpio/gpio-omap.c +0 -1

index 3a4f151..3b91ade 100644
@@ -57,7 +57,6 @@ struct gpio_bank {
      u16 irq;
      int irq_base;
      struct irq_domain *domain;
-     u32 saved_wakeup;
      u32 non_wakeup_gpios;
      u32 enabled_non_wakeup_gpios;
      struct gpio_regs context;

@@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
      unsigned long           flags;
 

drivers/gpio/gpio-omap.c static int omap_mpuio_suspend_noirq(struct device *dev) · line 779

Quoted 3 lines

drivers/gpio/gpio-omap.c +0 -1

      spin_lock_irqsave(&bank->lock, flags);
-     bank->saved_wakeup = __raw_readl(mask_reg);
      __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);

OK, here you are overwriting the mask_reg with the wakeup bitmask without saving the mask_reg's original content.

Quoted 6 lines

drivers/gpio/gpio-omap.c +0 -0

      spin_unlock_irqrestore(&bank->lock, flags);
 

@@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
      unsigned long           flags;
 

drivers/gpio/gpio-omap.c static int omap_mpuio_resume_noirq(struct device *dev) · line 794

Quoted 3 lines

drivers/gpio/gpio-omap.c +1 -1

      spin_lock_irqsave(&bank->lock, flags);
-     __raw_writel(bank->saved_wakeup, mask_reg);
+     __raw_writel(bank->context.wake_en, mask_reg);

Now you are restoring nothing but the same content that you stored during suspend. This will cause the non-wakeup gpio interrupts to get masked between a suspend/resume. So isn't this a bug?

Proper solution would be to save the mask_reg context into another register than context.wake_en during suspend.

drivers/gpio/gpio-omap.c static int omap_mpuio_resume_noirq(struct device *dev) · line 797

Quoted 4 lines

drivers/gpio/gpio-omap.c +0 -0

      spin_unlock_irqrestore(&bank->lock, flags);
 
      return 0;

cheers,
-roger

Hi Roger,

Quoted 31 lines · Roger Quadros

Hi,

Just bumped across this patch and have a query.

Quoted 26 lines · Tarun Kanti DebBarma

There is no more need to have saved_wakeup because bank->context.wake_en already holds that value. So getting rid of read/write operation associated with this field.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
 drivers/gpio/gpio-omap.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/gpio/gpio-omap.c +0 -1

index 3a4f151..3b91ade 100644
@@ -57,7 +57,6 @@ struct gpio_bank {
      u16 irq;
      int irq_base;
      struct irq_domain *domain;
-     u32 saved_wakeup;
      u32 non_wakeup_gpios;
      u32 enabled_non_wakeup_gpios;
      struct gpio_regs context;

@@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
      unsigned long           flags;
 

Quoted 6 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_suspend_noirq(struct device *dev) · line 779

Quoted 3 lines

drivers/gpio/gpio-omap.c +0 -1

      spin_lock_irqsave(&bank->lock, flags);
-     bank->saved_wakeup = __raw_readl(mask_reg);
      __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);

OK, here you are overwriting the mask_reg with the wakeup bitmask without saving the mask_reg's original content.

This is based upon understanding that set_gpio_trigger() is the common function where update of wake_en register takes place. Unless, mask_reg in this case refers to something else, effectively we would be saving the same value to saved_wakeup what is already present in wake_en. I will verify this specific to this function.

Quoted 6 lines

Quoted 6 lines

drivers/gpio/gpio-omap.c +0 -0

      spin_unlock_irqrestore(&bank->lock, flags);
 

@@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
      unsigned long           flags;
 

Quoted 7 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_resume_noirq(struct device *dev) · line 794

Quoted 3 lines

drivers/gpio/gpio-omap.c +1 -1

      spin_lock_irqsave(&bank->lock, flags);
-     __raw_writel(bank->saved_wakeup, mask_reg);
+     __raw_writel(bank->context.wake_en, mask_reg);

Now you are restoring nothing but the same content that you stored during suspend. This will cause the non-wakeup gpio interrupts to get masked between a suspend/resume. So isn't this a bug?

That's right, the same value is restored back which was last updated in set_gpio_trigger() that got stored in wake_en register. Let me know if I am missing your points here.

Quoted 4 lines

Proper solution would be to save the mask_reg context into another register than context.wake_en during suspend.

As I said, this would make sense if mask_reg is referring to different register than what is used in set_gpio_trigger(). I will have a look.

BTW, did you observe anything unusual during some testing?

Tarun,

Quoted 59 lines · Tarun Kanti

Hi Roger,

Quoted 31 lines · Roger Quadros

Hi,

Just bumped across this patch and have a query.

Quoted 26 lines · Tarun Kanti DebBarma

There is no more need to have saved_wakeup because bank->context.wake_en already holds that value. So getting rid of read/write operation associated with this field.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
 drivers/gpio/gpio-omap.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/gpio/gpio-omap.c +0 -1

index 3a4f151..3b91ade 100644
@@ -57,7 +57,6 @@ struct gpio_bank {
      u16 irq;
      int irq_base;
      struct irq_domain *domain;
-     u32 saved_wakeup;
      u32 non_wakeup_gpios;
      u32 enabled_non_wakeup_gpios;
      struct gpio_regs context;

@@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
      unsigned long           flags;
 

Quoted 6 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_suspend_noirq(struct device *dev) · line 779

Quoted 3 lines

drivers/gpio/gpio-omap.c +0 -1

      spin_lock_irqsave(&bank->lock, flags);
-     bank->saved_wakeup = __raw_readl(mask_reg);
      __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);

OK, here you are overwriting the mask_reg with the wakeup bitmask without saving the mask_reg's original content.

This is based upon understanding that set_gpio_trigger() is the common function where update of wake_en register takes place. Unless, mask_reg in this case refers to something else, effectively we would be saving the same value to saved_wakeup what is already present in wake_en. I will verify this specific to this function.

Quoted 13 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_resume_noirq(struct device *dev) · line 794

Quoted 9 lines

drivers/gpio/gpio-omap.c +1 -1

      spin_unlock_irqrestore(&bank->lock, flags);
 

@@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
      unsigned long           flags;
 
      spin_lock_irqsave(&bank->lock, flags);
-     __raw_writel(bank->saved_wakeup, mask_reg);
+     __raw_writel(bank->context.wake_en, mask_reg);

Now you are restoring nothing but the same content that you stored during suspend. This will cause the non-wakeup gpio interrupts to get masked between a suspend/resume. So isn't this a bug?

Quoted 3 lines

That's right, the same value is restored back which was last updated in set_gpio_trigger() that got stored in wake_en register. Let me know if I am missing your points here.

If it is writing the same thing then isn't this write redundant?

Quoted 7 lines

Quoted 5 lines

Proper solution would be to save the mask_reg context into another register than context.wake_en during suspend.

As I said, this would make sense if mask_reg is referring to different register than what is used in set_gpio_trigger(). I will have a look.

OK thanks.

Quoted 2 lines

BTW, did you observe anything unusual during some testing?

No, I haven't done any tests.

cheers,
-roger

Quoted 62 lines · Roger Quadros

Tarun,

Quoted 59 lines · Tarun Kanti

Hi Roger,

Quoted 31 lines · Roger Quadros

Hi,

Just bumped across this patch and have a query.

Quoted 26 lines · Tarun Kanti DebBarma

There is no more need to have saved_wakeup because bank->context.wake_en already holds that value. So getting rid of read/write operation associated with this field.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
 drivers/gpio/gpio-omap.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/gpio/gpio-omap.c +0 -1

index 3a4f151..3b91ade 100644
@@ -57,7 +57,6 @@ struct gpio_bank {
      u16 irq;
      int irq_base;
      struct irq_domain *domain;
-     u32 saved_wakeup;
      u32 non_wakeup_gpios;
      u32 enabled_non_wakeup_gpios;
      struct gpio_regs context;

@@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
      unsigned long           flags;
 

Quoted 6 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_suspend_noirq(struct device *dev) · line 779

Quoted 3 lines

drivers/gpio/gpio-omap.c +0 -1

      spin_lock_irqsave(&bank->lock, flags);
-     bank->saved_wakeup = __raw_readl(mask_reg);
      __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);

OK, here you are overwriting the mask_reg with the wakeup bitmask without saving the mask_reg's original content.

This is based upon understanding that set_gpio_trigger() is the common function where update of wake_en register takes place. Unless, mask_reg in this case refers to something else, effectively we would be saving the same value to saved_wakeup what is already present in wake_en. I will verify this specific to this function.

Quoted 13 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_resume_noirq(struct device *dev) · line 794

Quoted 9 lines

drivers/gpio/gpio-omap.c +1 -1

      spin_unlock_irqrestore(&bank->lock, flags);
 

@@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
      unsigned long           flags;
 
      spin_lock_irqsave(&bank->lock, flags);
-     __raw_writel(bank->saved_wakeup, mask_reg);
+     __raw_writel(bank->context.wake_en, mask_reg);

Now you are restoring nothing but the same content that you stored during suspend. This will cause the non-wakeup gpio interrupts to get masked between a suspend/resume. So isn't this a bug?

Quoted 5 lines

Quoted 3 lines

That's right, the same value is restored back which was last updated in set_gpio_trigger() that got stored in wake_en register. Let me know if I am missing your points here.

If it is writing the same thing then isn't this write redundant?

Not, really. During suspend if the register has lost the context we need to restore the value from wake_en.

--
Tarun

Quoted 13 lines

Quoted 7 lines

Quoted 5 lines

Proper solution would be to save the mask_reg context into another register than context.wake_en during suspend.

As I said, this would make sense if mask_reg is referring to different register than what is used in set_gpio_trigger(). I will have a look.

OK thanks.

Quoted 2 lines

BTW, did you observe anything unusual during some testing?

Hi Roger,

Quoted 31 lines · Roger Quadros

Hi,

Just bumped across this patch and have a query.

Quoted 26 lines · Tarun Kanti DebBarma

There is no more need to have saved_wakeup because bank->context.wake_en already holds that value. So getting rid of read/write operation associated with this field.

Signed-off-by Tarun Kanti DebBarma <[email protected]>

Reviewed-by Santosh Shilimkar <[email protected]>

Acked-by Felipe Balbi <[email protected]>

---
 drivers/gpio/gpio-omap.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/gpio/gpio-omap.c +0 -1

index 3a4f151..3b91ade 100644
@@ -57,7 +57,6 @@ struct gpio_bank {
      u16 irq;
      int irq_base;
      struct irq_domain *domain;
-     u32 saved_wakeup;
      u32 non_wakeup_gpios;
      u32 enabled_non_wakeup_gpios;
      struct gpio_regs context;

@@ -777,7 +776,6 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
      unsigned long           flags;
 

Quoted 6 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_suspend_noirq(struct device *dev) · line 779

Quoted 3 lines

drivers/gpio/gpio-omap.c +0 -1

      spin_lock_irqsave(&bank->lock, flags);
-     bank->saved_wakeup = __raw_readl(mask_reg);
      __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);

OK, here you are overwriting the mask_reg with the wakeup bitmask without saving the mask_reg's original content.

This is based upon understanding that set_gpio_trigger() is the common function where update of wake_en register takes place. Unless, mask_reg in this case refers to something else, effectively we would be saving the same value to saved_wakeup what is already present in wake_en. I will verify this specific to this function.

Quoted 7 lines

Quoted 7 lines

drivers/gpio/gpio-omap.c +0 -0

      spin_unlock_irqrestore(&bank->lock, flags);
 

@@ -793,7 +791,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
      unsigned long           flags;
 

Quoted 7 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_resume_noirq(struct device *dev) · line 794

Quoted 3 lines

drivers/gpio/gpio-omap.c +1 -1

      spin_lock_irqsave(&bank->lock, flags);
-     __raw_writel(bank->saved_wakeup, mask_reg);
+     __raw_writel(bank->context.wake_en, mask_reg);

Now you are restoring nothing but the same content that you stored during suspend. This will cause the non-wakeup gpio interrupts to get masked between a suspend/resume. So isn't this a bug?

That's right, the same value is restored back which was last updated in set_gpio_trigger() that got stored in wake_en register. Let me know if I am missing your points here.

Quoted 3 lines

Proper solution would be to save the mask_reg context into another register than context.wake_en during suspend.

As I said, this would make sense if mask_reg is referring to different register than what is used in set_gpio_trigger(). I will have a look.

BTW, did you observe anything unusual during some testing?

Thanks.
--
Tarun

Quoted 8 lines

drivers/gpio/gpio-omap.c static int omap_mpuio_resume_noirq(struct device *dev) · line 797

Quoted 4 lines

drivers/gpio/gpio-omap.c +0 -0

      spin_unlock_irqrestore(&bank->lock, flags);
 
      return 0;

cheers,
-roger

The cleanup is mostly getting rid of redundant fields in struct gpio_bank{} as we already have them as part of bank->context now. Also, remove un-used variable from gpio_irq_handler. Also, the suspend/resume callbacks are removed bacause they are not needed any more.

The fixes include correction of _set_gpio_irqenable() implementation, missing wakeup_en register update in set_gpio_wakeup(), type mismatch of gpio trigger parameter in set_gpio_trigger(), incorrect dataout register update in set_gpio_dataout_() and few corrections in context save logic.

It is baselined on top of:
git://git.secretlab.ca/git/linux-2.6.git gpio/next
Commit: 81b279d80a63628e580c71a31d30a8c3b3047ad4

Series is available here for reference:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_more_cleanup_fixes

Power Test:
Off-mode and Retention on OMAP3430 (Suspend and Idle paths). Also confirmed that dataout register content preserved over off-mode.

Quoted 4 lines · Tarun Kanti DebBarma

The cleanup is mostly getting rid of redundant fields in struct gpio_bank{} as we already have them as part of bank->context now. Also, remove un-used variable from gpio_irq_handler. Also, the suspend/resume callbacks are removed bacause they are not needed any more.

OK.

Quoted 5 lines

The fixes include correction of _set_gpio_irqenable() implementation, missing wakeup_en register update in set_gpio_wakeup(), type mismatch of gpio trigger parameter in set_gpio_trigger(), incorrect dataout register update in set_gpio_dataout_() and few corrections in context save logic.

These fixes need to be separated out from the cleanups and prepared for v3.4-rc. ?We need these as standlone fixes (indepenent of the cleanups) so they can be applied for 3.4-rc. ?The rest of the cleanups will have to wait until 3.5.

Quoted 8 lines

It is baselined on top of:
git://git.secretlab.ca/git/linux-2.6.git gpio/next
Commit: 81b279d80a63628e580c71a31d30a8c3b3047ad4

Series is available here for reference:
git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_more_cleanup_fixes

Quoted 4 lines

Power Test:
Off-mode and Retention on OMAP3430 (Suspend and Idle paths). Also confirmed that dataout register content preserved over off-mode.

I've confirmed these fixes also correct off-mode GPIO ouptut glitches that I was seeing on n900.

Kevin