[PATCH] [OMAP] GPIO Module is reset during initialization

[PATCH] [OMAP] GPIO Module is reset during initialization

[charu]

From: Charulatha V <charu@ti.com>

During initialization, GPIO module is reset using soft reset bit
of SYSCONFIG register

Signed-off-by: Charulatha V <charu@ti.com>
---
 arch/arm/plat-omap/gpio.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 2304a5d..4579650 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -21,6 +21,7 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/delay.h>
 
 #include <mach/hardware.h>
 #include <asm/irq.h>
@@ -1670,7 +1671,7 @@ static int __init _omap_gpio_init(void)
 	}
 #endif
 	for (i = 0; i < gpio_bank_count; i++) {
-		int j, gpio_count = 16;
+		int j, gpio_count = 16, attempt = 0;
 
 		bank = &gpio_bank[i];
 		spin_lock_init(&bank->lock);
@@ -1698,6 +1699,15 @@ static int __init _omap_gpio_init(void)
 			static const u32 non_wakeup_gpios[] = {
 				0xe203ffc0, 0x08700040
 			};
+
+		/* Software Reset of GPIO module */
+		__raw_writel(0x0002, bank->base	+ OMAP24XX_GPIO_SYSCONFIG);
+		while (((__raw_readl(bank->base + OMAP24XX_GPIO_SYSSTATUS)
+					& 0x1) == 0) && attempt < 5) {
+			udelay(1);
+			attempt++;
+		}
+
 		if (cpu_is_omap44xx()) {
 			__raw_writel(0xffffffff, bank->base +
 						OMAP4_GPIO_IRQSTATUSCLR0);
-- 
1.6.0.4

Re: [PATCH] [OMAP] GPIO Module is reset during initialization

[Felipe Balbi]

On Fri, Oct 23, 2009 at 09:25:29PM +0530, charu@ti.com wrote:
> From: Charulatha V <charu@ti.com>
> 
> During initialization, GPIO module is reset using soft reset bit
> of SYSCONFIG register
> 
> Signed-off-by: Charulatha V <charu@ti.com>
> ---
>  arch/arm/plat-omap/gpio.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 2304a5d..4579650 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -21,6 +21,7 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/delay.h>
>  
>  #include <mach/hardware.h>
>  #include <asm/irq.h>
> @@ -1670,7 +1671,7 @@ static int __init _omap_gpio_init(void)
>  	}
>  #endif
>  	for (i = 0; i < gpio_bank_count; i++) {
> -		int j, gpio_count = 16;
> +		int j, gpio_count = 16, attempt = 0;

decrementing is better, so:

attempt = 1000

>  
>  		bank = &gpio_bank[i];
>  		spin_lock_init(&bank->lock);
> @@ -1698,6 +1699,15 @@ static int __init _omap_gpio_init(void)
>  			static const u32 non_wakeup_gpios[] = {
>  				0xe203ffc0, 0x08700040
>  			};
> +
> +		/* Software Reset of GPIO module */
> +		__raw_writel(0x0002, bank->base	+ OMAP24XX_GPIO_SYSCONFIG);
> +		while (((__raw_readl(bank->base + OMAP24XX_GPIO_SYSSTATUS)
> +					& 0x1) == 0) && attempt < 5) {

&& attemp)

> +			udelay(1);

i guess cpu_relax() is better here.

> +			attempt++;

attempt--;

-- 
balbi

Re: [PATCH] [OMAP] GPIO Module is reset during initialization

[Nishanth Menon]

Felipe Balbi had written, on 10/23/2009 05:56 PM, the following:
> On Fri, Oct 23, 2009 at 09:25:29PM +0530, charu@ti.com wrote:
>> From: Charulatha V <charu@ti.com>
>>
>> During initialization, GPIO module is reset using soft reset bit
>> of SYSCONFIG register
>>
>> Signed-off-by: Charulatha V <charu@ti.com>
>> ---
>>  arch/arm/plat-omap/gpio.c |   12 +++++++++++-
>>  1 files changed, 11 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>> index 2304a5d..4579650 100644
>> --- a/arch/arm/plat-omap/gpio.c
>> +++ b/arch/arm/plat-omap/gpio.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/err.h>
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>> +#include <linux/delay.h>
>>  
>>  #include <mach/hardware.h>
>>  #include <asm/irq.h>
>> @@ -1670,7 +1671,7 @@ static int __init _omap_gpio_init(void)
>>  	}
>>  #endif
>>  	for (i = 0; i < gpio_bank_count; i++) {
>> -		int j, gpio_count = 16;
>> +		int j, gpio_count = 16, attempt = 0;
> 
> decrementing is better, so:
> 
> attempt = 1000
please move attempt out of here to the {} for 3430.. Warning for OMAP1.

> 
>>  
>>  		bank = &gpio_bank[i];
>>  		spin_lock_init(&bank->lock);
>> @@ -1698,6 +1699,15 @@ static int __init _omap_gpio_init(void)
>>  			static const u32 non_wakeup_gpios[] = {
>>  				0xe203ffc0, 0x08700040
>>  			};
>> +
>> +		/* Software Reset of GPIO module */
>> +		__raw_writel(0x0002, bank->base	+ OMAP24XX_GPIO_SYSCONFIG);
>> +		while (((__raw_readl(bank->base + OMAP24XX_GPIO_SYSSTATUS)
>> +					& 0x1) == 0) && attempt < 5) {
> 
> && attemp)
> 
>> +			udelay(1);
> 
> i guess cpu_relax() is better here.
> 
>> +			attempt++;
> 
> attempt--;
> 
cant we improve this code as following:
{
	u8 attempts = 25;
	/* Software Reset of GPIO module */
	__raw_writel(0x0002, bank->base
				+ OMAP24XX_GPIO_SYSCONFIG);
	/* wait for reset to be done */	
	while (((__raw_readl(bank->base	+
			OMAP24XX_GPIO_SYSSTATUS) & 0x1)	== 0)
			&& attempts) {
		cpu_relax();
		if (attempts % 5)
			udelay(1);
		attempts--;
	}

allows the kernel to do somethin else while we also ensure we have a 5
usec guarenteed delay before giving up..

-- 
Regards,
Nishanth Menon

RE: [PATCH] [OMAP] GPIO Module is reset during initialization

[Varadarajan, Charu Latha]

>Felipe Balbi had written, on 10/23/2009 05:56 PM, the following:
>> On Fri, Oct 23, 2009 at 09:25:29PM +0530, charu@ti.com wrote:
>>> From: Charulatha V <charu@ti.com>
>>>
>>> During initialization, GPIO module is reset using soft reset bit
>>> of SYSCONFIG register
>>>
>>> Signed-off-by: Charulatha V <charu@ti.com>
>>> ---
>>>  arch/arm/plat-omap/gpio.c |   12 +++++++++++-
>>>  1 files changed, 11 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>>> index 2304a5d..4579650 100644
>>> --- a/arch/arm/plat-omap/gpio.c
>>> +++ b/arch/arm/plat-omap/gpio.c
>>> @@ -21,6 +21,7 @@
>>>  #include <linux/err.h>
>>>  #include <linux/clk.h>
>>>  #include <linux/io.h>
>>> +#include <linux/delay.h>
>>>
>>>  #include <mach/hardware.h>
>>>  #include <asm/irq.h>
>>> @@ -1670,7 +1671,7 @@ static int __init _omap_gpio_init(void)
>>>      }
>>>  #endif
>>>      for (i = 0; i < gpio_bank_count; i++) {
>>> -            int j, gpio_count = 16;
>>> +            int j, gpio_count = 16, attempt = 0;
>>
>> decrementing is better, so:
Okay
>>
>> attempt = 1000
>please move attempt out of here to the {} for 3430.. Warning for OMAP1.
Okay
>>
>>>
>>>              bank = &gpio_bank[i];
>>>              spin_lock_init(&bank->lock);
>>> @@ -1698,6 +1699,15 @@ static int __init _omap_gpio_init(void)
>>>                      static const u32 non_wakeup_gpios[] = {
>>>                              0xe203ffc0, 0x08700040
>>>                      };
>>> +
>>> +            /* Software Reset of GPIO module */
>>> +            __raw_writel(0x0002, bank->base + OMAP24XX_GPIO_SYSCONFIG);
>>> +            while (((__raw_readl(bank->base + OMAP24XX_GPIO_SYSSTATUS)
>>> +                                    & 0x1) == 0) && attempt < 5) {
>>
>> && attemp)
>>
>>> +                    udelay(1);
>>
>> i guess cpu_relax() is better here.
I guess cpu_relax is not required because this part of code is called only from 
board file during boot-up. Hence this would be the only code to be executed.
>>
>>> +                    attempt++;
>>>
>>> attempt--;
>>>
>>cant we improve this code as following:
>{
>        u8 attempts = 25;
>        /* Software Reset of GPIO module */
>        __raw_writel(0x0002, bank->base
>                                + OMAP24XX_GPIO_SYSCONFIG);
>        /* wait for reset to be done */
>        while (((__raw_readl(bank->base +
>                        OMAP24XX_GPIO_SYSSTATUS) & 0x1) == 0)
>                        && attempts) {
>                cpu_relax();
>                if (attempts % 5)
>                        udelay(1);
>                attempts--;
>        }
>
>allows the kernel to do somethin else while we also ensure we have a 5
>usec guarenteed delay before giving up..
Doesn't modulo operation cost more in terms of performance?  Any specific
reason for specific 5 microseconds? 

RE: [PATCH] [OMAP] GPIO Module is reset during initialization

[Menon, Nishanth]

> From: Varadarajan, Charu Latha
> Sent: Monday, October 26, 2009 4:26 AM
> To: Menon, Nishanth; me@felipebalbi.com
> 
> >Felipe Balbi had written, on 10/23/2009 05:56 PM, the following:
> >> On Fri, Oct 23, 2009 at 09:25:29PM +0530, charu@ti.com wrote:
> >>> From: Charulatha V <charu@ti.com>
> >>>
> >>> During initialization, GPIO module is reset using soft reset bit
> >>> of SYSCONFIG register
> >>>
> >>> Signed-off-by: Charulatha V <charu@ti.com>
> >>> ---
> >>>  arch/arm/plat-omap/gpio.c |   12 +++++++++++-
> >>>  1 files changed, 11 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> >>> index 2304a5d..4579650 100644
> >>> --- a/arch/arm/plat-omap/gpio.c
> >>> +++ b/arch/arm/plat-omap/gpio.c
> >>> @@ -21,6 +21,7 @@
> >>>  #include <linux/err.h>
> >>>  #include <linux/clk.h>
> >>>  #include <linux/io.h>
> >>> +#include <linux/delay.h>
> >>>
> >>>  #include <mach/hardware.h>
> >>>  #include <asm/irq.h>
> >>> @@ -1670,7 +1671,7 @@ static int __init _omap_gpio_init(void)
> >>>      }
> >>>  #endif
> >>>      for (i = 0; i < gpio_bank_count; i++) {
> >>> -            int j, gpio_count = 16;
> >>> +            int j, gpio_count = 16, attempt = 0;
> >>
> >> decrementing is better, so:
> Okay
> >>
> >> attempt = 1000
> >please move attempt out of here to the {} for 3430.. Warning for OMAP1.
> Okay
> >>
> >>>
> >>>              bank = &gpio_bank[i];
> >>>              spin_lock_init(&bank->lock);
> >>> @@ -1698,6 +1699,15 @@ static int __init _omap_gpio_init(void)
> >>>                      static const u32 non_wakeup_gpios[] = {
> >>>                              0xe203ffc0, 0x08700040
> >>>                      };
> >>> +
> >>> +            /* Software Reset of GPIO module */
> >>> +            __raw_writel(0x0002, bank->base +
> OMAP24XX_GPIO_SYSCONFIG);
> >>> +            while (((__raw_readl(bank->base +
> OMAP24XX_GPIO_SYSSTATUS)
> >>> +                                    & 0x1) == 0) && attempt < 5) {
> >>
> >> && attemp)
> >>
> >>> +                    udelay(1);
> >>
> >> i guess cpu_relax() is better here.
> I guess cpu_relax is not required because this part of code is called only
> from
> board file during boot-up. Hence this would be the only code to be
> executed.
> >>
> >>> +                    attempt++;
> >>>
> >>> attempt--;
> >>>
> >>cant we improve this code as following:
> >{
> >        u8 attempts = 25;
> >        /* Software Reset of GPIO module */
> >        __raw_writel(0x0002, bank->base
> >                                + OMAP24XX_GPIO_SYSCONFIG);
> >        /* wait for reset to be done */
> >        while (((__raw_readl(bank->base +
> >                        OMAP24XX_GPIO_SYSSTATUS) & 0x1) == 0)
> >                        && attempts) {
> >                cpu_relax();
> >                if (attempts % 5)
> >                        udelay(1);
> >                attempts--;
> >        }
> >
> >allows the kernel to do somethin else while we also ensure we have a 5
> >usec guarenteed delay before giving up..
> Doesn't modulo operation cost more in terms of performance?  Any specific
> reason for specific 5 microseconds? 
You could replace it with >> operator if you like and use 2^x multiples.. I am just sticking 5 us there based on your original code.. so the same logic over here I suppose.. unless I missed something?

Regards,
Nishanth Menon

RE: [PATCH] [OMAP] GPIO Module is reset during initialization

[Varadarajan, Charu Latha]

-----snippet----
>>>>
>>>> && attemp)
>>>>
>>>>> +                    udelay(1);
>>>>
>>>> i guess cpu_relax() is better here.
>> I guess cpu_relax is not required because this part of code is called only
>> from
>> board file during boot-up. Hence this would be the only code to be
>> executed.
What is your opinion on this?
>>>>
>>>>> +                    attempt++;
>>>>>
>>>>> attempt--;
>>>>>
>>>>cant we improve this code as following:
>>>{
>>>        u8 attempts = 25;
>>>        /* Software Reset of GPIO module */
>>>        __raw_writel(0x0002, bank->base
>>>                                + OMAP24XX_GPIO_SYSCONFIG);
>>>        /* wait for reset to be done */
>>>        while (((__raw_readl(bank->base +
>>>                        OMAP24XX_GPIO_SYSSTATUS) & 0x1) == 0)
>>>                        && attempts) {
>>>                cpu_relax();
>>>                if (attempts % 5)
>>>                        udelay(1);
>>>                attempts--;
>>>        }
>>>
>>>allows the kernel to do somethin else while we also ensure we have a 5
>>>usec guarenteed delay before giving up..
>> Doesn't modulo operation cost more in terms of performance?  Any specific
>> reason for specific 5 microseconds?
>You could replace it with >> operator if you like and use 2^x multiples.. 
>I am just sticking 5 us there based on your original code.. 
>so the same logic over here I suppose.. unless I missed something?
I was using attempts as 5, as my intension was to attempt only 5 times and not 
in terms  of usec as I could not find any details in any document for maximum 
time for the bit to get set/reset. According to your code, it would attempt to read 
the register 25 times with a delay of 5 microseconds min during worst case 
scenario.

RE: [PATCH] [OMAP] GPIO Module is reset during initialization

[Menon, Nishanth]

> -----Original Message-----
> From: Varadarajan, Charu Latha
> Sent: Monday, October 26, 2009 5:52 AM
> To: Menon, Nishanth; me@felipebalbi.com
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH] [OMAP] GPIO Module is reset during initialization
> 
> -----snippet----
> >>>>
> >>>> && attemp)
> >>>>
> >>>>> +                    udelay(1);
> >>>>
> >>>> i guess cpu_relax() is better here.
> >> I guess cpu_relax is not required because this part of code is called
> only
> >> from
> >> board file during boot-up. Hence this would be the only code to be
> >> executed.
> What is your opinion on this?
Cpu_relax is still the better way of doing it.

> >>>>
> >>>>> +                    attempt++;
> >>>>>
> >>>>> attempt--;
> >>>>>
> >>>>cant we improve this code as following:
> >>>{
> >>>        u8 attempts = 25;
> >>>        /* Software Reset of GPIO module */
> >>>        __raw_writel(0x0002, bank->base
> >>>                                + OMAP24XX_GPIO_SYSCONFIG);
> >>>        /* wait for reset to be done */
> >>>        while (((__raw_readl(bank->base +
> >>>                        OMAP24XX_GPIO_SYSSTATUS) & 0x1) == 0)
> >>>                        && attempts) {
> >>>                cpu_relax();
> >>>                if (attempts % 5)
> >>>                        udelay(1);
> >>>                attempts--;
> >>>        }
> >>>
> >>>allows the kernel to do somethin else while we also ensure we have a 5
> >>>usec guarenteed delay before giving up..
> >> Doesn't modulo operation cost more in terms of performance?  Any
> specific
> >> reason for specific 5 microseconds?
> >You could replace it with >> operator if you like and use 2^x multiples..
> >I am just sticking 5 us there based on your original code..
> >so the same logic over here I suppose.. unless I missed something?
> I was using attempts as 5, as my intension was to attempt only 5 times and
> not
> in terms  of usec as I could not find any details in any document for
> maximum
> time for the bit to get set/reset. According to your code, it would
> attempt to read
> the register 25 times with a delay of 5 microseconds min during worst case
> scenario.
Please find the h/w timeout value and resubmit the patch..

Regards,
Nishanth Menon

RE: [PATCH] [OMAP] GPIO Module is reset during initialization

[Varadarajan, Charu Latha]

----------------snippet---------------------
>> >>>> i guess cpu_relax() is better here.
>> >> I guess cpu_relax is not required because this part of code is called
>> only
>> >> from
>> >> board file during boot-up. Hence this would be the only code to be
>> >> executed.
>> What is your opinion on this?
>Cpu_relax is still the better way of doing it.
Please clarify why.

>> >>>allows the kernel to do somethin else while we also ensure we have a 5
>> >>>usec guarenteed delay before giving up..
>> >> Doesn't modulo operation cost more in terms of performance?  Any
>> specific
>> >> reason for specific 5 microseconds?
>> >You could replace it with >> operator if you like and use 2^x multiples..
>> >I am just sticking 5 us there based on your original code..
>> >so the same logic over here I suppose.. unless I missed something?
>> I was using attempts as 5, as my intension was to attempt only 5 times and
>> not
>> in terms  of usec as I could not find any details in any document for
>> maximum
>> time for the bit to get set/reset. According to your code, it would
>> attempt to read
>> the register 25 times with a delay of 5 microseconds min during worst case
>> scenario.
>Please find the h/w timeout value and resubmit the patch..
As of now, I do not have any information on these details. Please share 
any details if you can.

Re: [PATCH] [OMAP] GPIO Module is reset during initialization

[Kevin Hilman]

charu@ti.com writes:

> From: Charulatha V <charu@ti.com>
>
> During initialization, GPIO module is reset using soft reset bit
> of SYSCONFIG register
>
> Signed-off-by: Charulatha V <charu@ti.com>
> ---
>  arch/arm/plat-omap/gpio.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 2304a5d..4579650 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -21,6 +21,7 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/delay.h>
>  
>  #include <mach/hardware.h>
>  #include <asm/irq.h>
> @@ -1670,7 +1671,7 @@ static int __init _omap_gpio_init(void)
>  	}
>  #endif
>  	for (i = 0; i < gpio_bank_count; i++) {
> -		int j, gpio_count = 16;
> +		int j, gpio_count = 16, attempt = 0;
>  
>  		bank = &gpio_bank[i];
>  		spin_lock_init(&bank->lock);
> @@ -1698,6 +1699,15 @@ static int __init _omap_gpio_init(void)
>  			static const u32 non_wakeup_gpios[] = {
>  				0xe203ffc0, 0x08700040
>  			};
> +
> +		/* Software Reset of GPIO module */
> +		__raw_writel(0x0002, bank->base	+ OMAP24XX_GPIO_SYSCONFIG);
> +		while (((__raw_readl(bank->base + OMAP24XX_GPIO_SYSSTATUS)
> +					& 0x1) == 0) && attempt < 5) {
> +			udelay(1);
> +			attempt++;
> +		}
> +

NAK.  Would rather see an omap_hwmod added for GPIO, in which case we
get the reset for free.

Kevin