Patchwork [U-Boot,v3,06/11] arm:reset: call the reset_misc() before the cpu reset

login
register
mail settings
Submitter Przemyslaw Marczak
Date June 26, 2014, 2:15 p.m.
Message ID <1403792137-3113-7-git-send-email-p.marczak@samsung.com>
Download mbox | patch
Permalink /patch/364556/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Przemyslaw Marczak - June 26, 2014, 2:15 p.m.
On an Odroid U3 board, the SOC is unable to reset the eMMC card
in the DWMMC mode by the cpu software reset. Manual reset of the card
by switching proper gpio pin - fixes this issue.

Such solution needs to add a call to pre reset function.
This is done by the reset_misc() function, which is called before reset_cpu().
The function reset_misc() is a weak function.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
---
 arch/arm/lib/reset.c | 7 +++++++
 include/common.h     | 1 +
 2 files changed, 8 insertions(+)
Minkyu Kang - June 27, 2014, 9:40 a.m.
Dear Przemyslaw Marczak,

On 26/06/14 23:15, Przemyslaw Marczak wrote:
> On an Odroid U3 board, the SOC is unable to reset the eMMC card
> in the DWMMC mode by the cpu software reset. Manual reset of the card
> by switching proper gpio pin - fixes this issue.
> 
> Such solution needs to add a call to pre reset function.
> This is done by the reset_misc() function, which is called before reset_cpu().
> The function reset_misc() is a weak function.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Minkyu Kang <mk7.kang@samsung.com>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Tom Rini <trini@ti.com>
> ---
>  arch/arm/lib/reset.c | 7 +++++++
>  include/common.h     | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
> index 7a03580..3b39466 100644
> --- a/arch/arm/lib/reset.c
> +++ b/arch/arm/lib/reset.c
> @@ -23,6 +23,11 @@
>  
>  #include <common.h>
>  
> +void __reset_misc(void) {}
> +
> +void reset_misc(void)
> +	__attribute((weak, alias("__reset_misc")));
> +
>  int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>  	puts ("resetting ...\n");
> @@ -30,6 +35,8 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	udelay (50000);				/* wait 50 ms */
>  
>  	disable_interrupts();
> +
> +	reset_misc();
>  	reset_cpu(0);
>  
>  	/*NOTREACHED*/
> diff --git a/include/common.h b/include/common.h
> index 232136c..04bab78 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -629,6 +629,7 @@ int	checkicache   (void);
>  int	checkdcache   (void);
>  void	upmconfig     (unsigned int, unsigned int *, unsigned int);
>  ulong	get_tbclk     (void);
> +void	reset_misc    (void);
>  void	reset_cpu     (ulong addr);
>  #if defined (CONFIG_OF_LIBFDT) && defined (CONFIG_OF_BOARD_SETUP)
>  void ft_cpu_setup(void *blob, bd_t *bd);
> 

I'm not sure that we really need to add this function to arm common.
We can do this in reset_cpu (arch/arm/cpu/armv7/exynos/soc.c).
But if other SoCs also need to add such things then, it can be added as arm common.

Thanks,
Minkyu Kang.
Przemyslaw Marczak - June 27, 2014, 11:34 a.m.
On 06/27/2014 11:40 AM, Minkyu Kang wrote:
> Dear Przemyslaw Marczak,
>
> On 26/06/14 23:15, Przemyslaw Marczak wrote:
>> On an Odroid U3 board, the SOC is unable to reset the eMMC card
>> in the DWMMC mode by the cpu software reset. Manual reset of the card
>> by switching proper gpio pin - fixes this issue.
>>
>> Such solution needs to add a call to pre reset function.
>> This is done by the reset_misc() function, which is called before reset_cpu().
>> The function reset_misc() is a weak function.
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> Cc: Tom Rini <trini@ti.com>
>> ---
>>   arch/arm/lib/reset.c | 7 +++++++
>>   include/common.h     | 1 +
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
>> index 7a03580..3b39466 100644
>> --- a/arch/arm/lib/reset.c
>> +++ b/arch/arm/lib/reset.c
>> @@ -23,6 +23,11 @@
>>
>>   #include <common.h>
>>
>> +void __reset_misc(void) {}
>> +
>> +void reset_misc(void)
>> +	__attribute((weak, alias("__reset_misc")));
>> +
>>   int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>   {
>>   	puts ("resetting ...\n");
>> @@ -30,6 +35,8 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>   	udelay (50000);				/* wait 50 ms */
>>
>>   	disable_interrupts();
>> +
>> +	reset_misc();
>>   	reset_cpu(0);
>>
>>   	/*NOTREACHED*/
>> diff --git a/include/common.h b/include/common.h
>> index 232136c..04bab78 100644
>> --- a/include/common.h
>> +++ b/include/common.h
>> @@ -629,6 +629,7 @@ int	checkicache   (void);
>>   int	checkdcache   (void);
>>   void	upmconfig     (unsigned int, unsigned int *, unsigned int);
>>   ulong	get_tbclk     (void);
>> +void	reset_misc    (void);
>>   void	reset_cpu     (ulong addr);
>>   #if defined (CONFIG_OF_LIBFDT) && defined (CONFIG_OF_BOARD_SETUP)
>>   void ft_cpu_setup(void *blob, bd_t *bd);
>>
>
> I'm not sure that we really need to add this function to arm common.
> We can do this in reset_cpu (arch/arm/cpu/armv7/exynos/soc.c).
> But if other SoCs also need to add such things then, it can be added as arm common.
>
> Thanks,
> Minkyu Kang.
>

No one used this before, so probably better is to move it into exynos 
soc.c code.

Thank you,
Jeroen Hofstee - June 27, 2014, 8:20 p.m.
Hello Przemyslaw,

On 27-06-14 13:34, Przemyslaw Marczak wrote:
> On 06/27/2014 11:40 AM, Minkyu Kang wrote:
>> Dear Przemyslaw Marczak,
>>
>> On 26/06/14 23:15, Przemyslaw Marczak wrote:
>>> On an Odroid U3 board, the SOC is unable to reset the eMMC card
>>> in the DWMMC mode by the cpu software reset. Manual reset of the card
>>> by switching proper gpio pin - fixes this issue.
>>>
>>> Such solution needs to add a call to pre reset function.
>>> This is done by the reset_misc() function, which is called before 
>>> reset_cpu().
>>> The function reset_misc() is a weak function.
>>>
>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>> Cc: Tom Rini <trini@ti.com>
>>> ---
>>>   arch/arm/lib/reset.c | 7 +++++++
>>>   include/common.h     | 1 +
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
>>> index 7a03580..3b39466 100644
>>> --- a/arch/arm/lib/reset.c
>>> +++ b/arch/arm/lib/reset.c
>>> @@ -23,6 +23,11 @@
>>>
>>>   #include <common.h>
>>>
>>> +void __reset_misc(void) {}
>>> +
>>> +void reset_misc(void)
>>> +    __attribute((weak, alias("__reset_misc")));
>>> +
can you please use __weak here and provide a prototype, wherever it
ends up in the end. It prevents 3 warnings and makes it type safe..

Regards,
Jeroen
Przemyslaw Marczak - June 30, 2014, 8:41 a.m.
Hello Jeroen,

On 06/27/2014 10:20 PM, Jeroen Hofstee wrote:
> Hello Przemyslaw,
>
> On 27-06-14 13:34, Przemyslaw Marczak wrote:
>> On 06/27/2014 11:40 AM, Minkyu Kang wrote:
>>> Dear Przemyslaw Marczak,
>>>
>>> On 26/06/14 23:15, Przemyslaw Marczak wrote:
>>>> On an Odroid U3 board, the SOC is unable to reset the eMMC card
>>>> in the DWMMC mode by the cpu software reset. Manual reset of the card
>>>> by switching proper gpio pin - fixes this issue.
>>>>
>>>> Such solution needs to add a call to pre reset function.
>>>> This is done by the reset_misc() function, which is called before
>>>> reset_cpu().
>>>> The function reset_misc() is a weak function.
>>>>
>>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>>> Cc: Tom Rini <trini@ti.com>
>>>> ---
>>>>   arch/arm/lib/reset.c | 7 +++++++
>>>>   include/common.h     | 1 +
>>>>   2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
>>>> index 7a03580..3b39466 100644
>>>> --- a/arch/arm/lib/reset.c
>>>> +++ b/arch/arm/lib/reset.c
>>>> @@ -23,6 +23,11 @@
>>>>
>>>>   #include <common.h>
>>>>
>>>> +void __reset_misc(void) {}
>>>> +
>>>> +void reset_misc(void)
>>>> +    __attribute((weak, alias("__reset_misc")));
>>>> +
> can you please use __weak here and provide a prototype, wherever it
> ends up in the end. It prevents 3 warnings and makes it type safe..
>
> Regards,
> Jeroen

Thanks, I will add the __weak prefix there.

The prototype of this new function is in file common.h,
so this is type safe.

I checked the compilation with options: -W and -pedantic
on two configs: trats and odroid, and there was no warnings about the 
function reset_misc.

Thank you,
Jeroen Hofstee - June 30, 2014, 6:30 p.m.
Hello Przemyslaw.

[...]
>>>>>
>>>>>   #include <common.h>
>>>>>
>>>>> +void __reset_misc(void) {}
>>>>> +
>>>>> +void reset_misc(void)
>>>>> +    __attribute((weak, alias("__reset_misc")));
>>>>> +
>> can you please use __weak here and provide a prototype, wherever it
>> ends up in the end. It prevents 3 warnings and makes it type safe..
>>
>
> Thanks, I will add the __weak prefix there.
>
thanks
> The prototype of this new function is in file common.h,
> so this is type safe.
>
yup, I see, don't know how I missed that.
> I checked the compilation with options: -W and -pedantic
> on two configs: trats and odroid, and there was no warnings about the 
> function reset_misc.
>
You won't see the warning with -Wall -Wpendantic, but when running make W=1,
which adds -Wmissing-declarations. The alias version typically warns 
with something
like __reset_misc has no previous declaration. That is useful at times,
since it would e.g. complain about [1].

Regards,
Jeroen

[1] http://lists.denx.de/pipermail/u-boot/2014-June/182781.html
Przemyslaw Marczak - July 1, 2014, 2:16 p.m.
Hello Jeroen,

On 06/30/2014 08:30 PM, Jeroen Hofstee wrote:
> Hello Przemyslaw.
>
> [...]
>>>>>>
>>>>>>   #include <common.h>
>>>>>>
>>>>>> +void __reset_misc(void) {}
>>>>>> +
>>>>>> +void reset_misc(void)
>>>>>> +    __attribute((weak, alias("__reset_misc")));
>>>>>> +
>>> can you please use __weak here and provide a prototype, wherever it
>>> ends up in the end. It prevents 3 warnings and makes it type safe..
>>>
>>
>> Thanks, I will add the __weak prefix there.
>>
> thanks
>> The prototype of this new function is in file common.h,
>> so this is type safe.
>>
> yup, I see, don't know how I missed that.
No problem.
>> I checked the compilation with options: -W and -pedantic
>> on two configs: trats and odroid, and there was no warnings about the
>> function reset_misc.
>>
> You won't see the warning with -Wall -Wpendantic, but when running make
> W=1,
> which adds -Wmissing-declarations. The alias version typically warns
> with something
> like __reset_misc has no previous declaration. That is useful at times,
> since it would e.g. complain about [1].
>
> Regards,
> Jeroen
>
> [1] http://lists.denx.de/pipermail/u-boot/2014-June/182781.html
Thanks for useful info.

Regards,
Przemyslaw Marczak - July 1, 2014, 2:36 p.m.
Hello Minkyu,

On 06/27/2014 01:34 PM, Przemyslaw Marczak wrote:
> On 06/27/2014 11:40 AM, Minkyu Kang wrote:
>> Dear Przemyslaw Marczak,
>>
>> On 26/06/14 23:15, Przemyslaw Marczak wrote:
>>> On an Odroid U3 board, the SOC is unable to reset the eMMC card
>>> in the DWMMC mode by the cpu software reset. Manual reset of the card
>>> by switching proper gpio pin - fixes this issue.
>>>
>>> Such solution needs to add a call to pre reset function.
>>> This is done by the reset_misc() function, which is called before
>>> reset_cpu().
>>> The function reset_misc() is a weak function.
>>>
>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>> Cc: Tom Rini <trini@ti.com>
>>> ---
>>>   arch/arm/lib/reset.c | 7 +++++++
>>>   include/common.h     | 1 +
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
>>> index 7a03580..3b39466 100644
>>> --- a/arch/arm/lib/reset.c
>>> +++ b/arch/arm/lib/reset.c
>>> @@ -23,6 +23,11 @@
>>>
>>>   #include <common.h>
>>>
>>> +void __reset_misc(void) {}
>>> +
>>> +void reset_misc(void)
>>> +    __attribute((weak, alias("__reset_misc")));
>>> +
>>>   int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>> argv[])
>>>   {
>>>       puts ("resetting ...\n");
>>> @@ -30,6 +35,8 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc,
>>> char * const argv[])
>>>       udelay (50000);                /* wait 50 ms */
>>>
>>>       disable_interrupts();
>>> +
>>> +    reset_misc();
>>>       reset_cpu(0);
>>>
>>>       /*NOTREACHED*/
>>> diff --git a/include/common.h b/include/common.h
>>> index 232136c..04bab78 100644
>>> --- a/include/common.h
>>> +++ b/include/common.h
>>> @@ -629,6 +629,7 @@ int    checkicache   (void);
>>>   int    checkdcache   (void);
>>>   void    upmconfig     (unsigned int, unsigned int *, unsigned int);
>>>   ulong    get_tbclk     (void);
>>> +void    reset_misc    (void);
>>>   void    reset_cpu     (ulong addr);
>>>   #if defined (CONFIG_OF_LIBFDT) && defined (CONFIG_OF_BOARD_SETUP)
>>>   void ft_cpu_setup(void *blob, bd_t *bd);
>>>
>>
>> I'm not sure that we really need to add this function to arm common.
>> We can do this in reset_cpu (arch/arm/cpu/armv7/exynos/soc.c).
>> But if other SoCs also need to add such things then, it can be added
>> as arm common.
>>
>> Thanks,
>> Minkyu Kang.
>>
>
> No one used this before, so probably better is to move it into exynos
> soc.c code.
>
> Thank you,

I'm working on a next patch set version. And the idea with calling 
reset_misc() from soc.c is quite bad. Function reset_cpu() is actually 
dedicated only for cpu software reset - as its name suggest.
So I prefer leave the reset_misc call inside do_reset().

Thanks,
Minkyu Kang - July 2, 2014, 7:03 a.m.
On 01/07/14 23:36, Przemyslaw Marczak wrote:
> Hello Minkyu,
> 
> On 06/27/2014 01:34 PM, Przemyslaw Marczak wrote:
>> On 06/27/2014 11:40 AM, Minkyu Kang wrote:
>>> Dear Przemyslaw Marczak,
>>>
>>> On 26/06/14 23:15, Przemyslaw Marczak wrote:
>>>> On an Odroid U3 board, the SOC is unable to reset the eMMC card
>>>> in the DWMMC mode by the cpu software reset. Manual reset of the card
>>>> by switching proper gpio pin - fixes this issue.
>>>>
>>>> Such solution needs to add a call to pre reset function.
>>>> This is done by the reset_misc() function, which is called before
>>>> reset_cpu().
>>>> The function reset_misc() is a weak function.
>>>>
>>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>>> Cc: Tom Rini <trini@ti.com>
>>>> ---
>>>>   arch/arm/lib/reset.c | 7 +++++++
>>>>   include/common.h     | 1 +
>>>>   2 files changed, 8 insertions(+)
>>>
>>> I'm not sure that we really need to add this function to arm common.
>>> We can do this in reset_cpu (arch/arm/cpu/armv7/exynos/soc.c).
>>> But if other SoCs also need to add such things then, it can be added
>>> as arm common.
>>>
>>> Thanks,
>>> Minkyu Kang.
>>>
>>
>> No one used this before, so probably better is to move it into exynos
>> soc.c code.
>>
>> Thank you,
> 
> I'm working on a next patch set version. And the idea with calling reset_misc() from soc.c is quite bad. Function reset_cpu() is actually dedicated only for cpu software reset - as its name suggest.
> So I prefer leave the reset_misc call inside do_reset().

Sorry. I didn't understand the reason.
Could you please explain more in detail?

Thanks,
Minkyu Kang.
Przemyslaw Marczak - July 2, 2014, 10:27 a.m.
Hello Minkyu,

On 07/02/2014 09:03 AM, Minkyu Kang wrote:
> On 01/07/14 23:36, Przemyslaw Marczak wrote:
>> Hello Minkyu,
>>
>> On 06/27/2014 01:34 PM, Przemyslaw Marczak wrote:
>>> On 06/27/2014 11:40 AM, Minkyu Kang wrote:
>>>> Dear Przemyslaw Marczak,
>>>>
>>>> On 26/06/14 23:15, Przemyslaw Marczak wrote:
>>>>> On an Odroid U3 board, the SOC is unable to reset the eMMC card
>>>>> in the DWMMC mode by the cpu software reset. Manual reset of the card
>>>>> by switching proper gpio pin - fixes this issue.
>>>>>
>>>>> Such solution needs to add a call to pre reset function.
>>>>> This is done by the reset_misc() function, which is called before
>>>>> reset_cpu().
>>>>> The function reset_misc() is a weak function.
>>>>>
>>>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>>>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>>>>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>>>> Cc: Tom Rini <trini@ti.com>
>>>>> ---
>>>>>    arch/arm/lib/reset.c | 7 +++++++
>>>>>    include/common.h     | 1 +
>>>>>    2 files changed, 8 insertions(+)
>>>>
>>>> I'm not sure that we really need to add this function to arm common.
>>>> We can do this in reset_cpu (arch/arm/cpu/armv7/exynos/soc.c).
>>>> But if other SoCs also need to add such things then, it can be added
>>>> as arm common.
>>>>
>>>> Thanks,
>>>> Minkyu Kang.
>>>>
>>>
>>> No one used this before, so probably better is to move it into exynos
>>> soc.c code.
>>>
>>> Thank you,
>>
>> I'm working on a next patch set version. And the idea with calling reset_misc() from soc.c is quite bad. Function reset_cpu() is actually dedicated only for cpu software reset - as its name suggest.
>> So I prefer leave the reset_misc call inside do_reset().
>
> Sorry. I didn't understand the reason.
> Could you please explain more in detail?
>
> Thanks,
> Minkyu Kang.
>
I mean that we should take in to account the function names, e.g.:
- do_reset() - do the all reset needed things, and call:
  - reset_misc() - reset/prepare various devices before the CPU reset
  - reset_cpu() - do the CPU software reset only

The particular Odroid reset procedure is more board specific than SOC 
specific, and this is why it is implemented in board file.
So I think that the place of call to reset_misc() in common code is good.

Thanks

Patch

diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
index 7a03580..3b39466 100644
--- a/arch/arm/lib/reset.c
+++ b/arch/arm/lib/reset.c
@@ -23,6 +23,11 @@ 
 
 #include <common.h>
 
+void __reset_misc(void) {}
+
+void reset_misc(void)
+	__attribute((weak, alias("__reset_misc")));
+
 int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	puts ("resetting ...\n");
@@ -30,6 +35,8 @@  int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	udelay (50000);				/* wait 50 ms */
 
 	disable_interrupts();
+
+	reset_misc();
 	reset_cpu(0);
 
 	/*NOTREACHED*/
diff --git a/include/common.h b/include/common.h
index 232136c..04bab78 100644
--- a/include/common.h
+++ b/include/common.h
@@ -629,6 +629,7 @@  int	checkicache   (void);
 int	checkdcache   (void);
 void	upmconfig     (unsigned int, unsigned int *, unsigned int);
 ulong	get_tbclk     (void);
+void	reset_misc    (void);
 void	reset_cpu     (ulong addr);
 #if defined (CONFIG_OF_LIBFDT) && defined (CONFIG_OF_BOARD_SETUP)
 void ft_cpu_setup(void *blob, bd_t *bd);