Patchwork Support system reset in Exynos4210

login
register
mail settings
Submitter Dmitry Zhurikhin
Date April 4, 2012, 10:08 a.m.
Message ID <1333534133-9917-1-git-send-email-zhur@ispras.ru>
Download mbox | patch
Permalink /patch/150706/
State New
Headers show

Comments

Dmitry Zhurikhin - April 4, 2012, 10:08 a.m.
Reset the system when 1 is written to SWRESET register

Signed-off-by: Dmitry Zhurikhin <zhur@ispras.ru>
---
 hw/exynos4210_pmu.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
Maksim E. Kozlov - April 4, 2012, 11:55 a.m.
04.04.2012 14:08, Dmitry Zhurikhin пишет:
> Reset the system when 1 is written to SWRESET register
>
> Signed-off-by: Dmitry Zhurikhin<zhur@ispras.ru>
> ---
>   hw/exynos4210_pmu.c |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
> index c12d750..edf6e34 100644
> --- a/hw/exynos4210_pmu.c
> +++ b/hw/exynos4210_pmu.c
> @@ -25,6 +25,7 @@
>    */
>
>   #include "sysbus.h"
> +#include "sysemu.h"
>
>   #ifndef DEBUG_PMU
>   #define DEBUG_PMU           0
> @@ -422,6 +423,16 @@ static void exynos4210_pmu_write(void *opaque, target_phys_addr_t offset,
>           if (reg_p->offset == offset) {
>               PRINT_DEBUG_EXTEND("%s<0x%04x>  <- 0x%04x\n", reg_p->name,
>                       (uint32_t)offset, (uint32_t)val);
> +            switch (offset) {
> +            case SWRESET:
> +                if (val&  1) {
> +                    qemu_system_reset_request();
> +                }
> +                break;
> +            default:
> +                /* Nothing */
> +                break;
> +            }
>               s->reg[i] = val;
>               return;
>           }
It's not quite well. At first, when you do reset, appropriate status 
must be set in RST_STAT register. At second, not all registers in PMU 
should be set in default value after reset, so you should change PMU 
reset function for handling different resets (see spec) So, this 
functionality should be wrote more carefully

And use #define for registers and fields of the registers. It's more 
clearly, as for me.

Regards,
MK
Dmitry Zhurikhin - April 4, 2012, 12:35 p.m.
On 2012-04-04 15:55, Maksim Kozlov wrote:
> 04.04.2012 14:08, Dmitry Zhurikhin пишет:
>> Reset the system when 1 is written to SWRESET register
>>
>> Signed-off-by: Dmitry Zhurikhin<zhur@ispras.ru>
>> ---
>>   hw/exynos4210_pmu.c |   11 +++++++++++
>>   1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
>> index c12d750..edf6e34 100644
>> --- a/hw/exynos4210_pmu.c
>> +++ b/hw/exynos4210_pmu.c
>> @@ -25,6 +25,7 @@
>>    */
>>
>>   #include "sysbus.h"
>> +#include "sysemu.h"
>>
>>   #ifndef DEBUG_PMU
>>   #define DEBUG_PMU           0
>> @@ -422,6 +423,16 @@ static void exynos4210_pmu_write(void *opaque,
>> target_phys_addr_t offset,
>>           if (reg_p->offset == offset) {
>>               PRINT_DEBUG_EXTEND("%s<0x%04x>  <- 0x%04x\n", reg_p->name,
>>                       (uint32_t)offset, (uint32_t)val);
>> +            switch (offset) {
>> +            case SWRESET:
>> +                if (val&  1) {
>> +                    qemu_system_reset_request();
>> +                }
>> +                break;
>> +            default:
>> +                /* Nothing */
>> +                break;
>> +            }
>>               s->reg[i] = val;
>>               return;
>>           }
> It's not quite well. At first, when you do reset, appropriate status
> must be set in RST_STAT register. At second, not all registers in PMU
> should be set in default value after reset, so you should change PMU
> reset function for handling different resets (see spec) So, this
> functionality should be wrote more carefully
Well, this is the case when there is a need to modify booting procedure
depending on the values of these registers.  I haven't found any such
code in the current kernel.  As I now remember saving their values was
indeed important when we were trying to use an U-Boot bootloader.  But
as long as we are sticking with QEMU bootloader it doesn't matter. 
Anyway this information is unfortunately absent in Exynos4210 public
documentation.
>
> And use #define for registers and fields of the registers. It's more
> clearly, as for me.
As you say.
>
> Regards,
> MK

    Regards,
    Dmitry
Maksim E. Kozlov - April 4, 2012, 4:16 p.m.
04.04.2012 16:35, Dmitry Zhurikhin пишет:
> On 2012-04-04 15:55, Maksim Kozlov wrote:
>> 04.04.2012 14:08, Dmitry Zhurikhin пишет:
>>> Reset the system when 1 is written to SWRESET register
>>>
>>> Signed-off-by: Dmitry Zhurikhin<zhur@ispras.ru>
>>> ---
>>>    hw/exynos4210_pmu.c |   11 +++++++++++
>>>    1 files changed, 11 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
>>> index c12d750..edf6e34 100644
>>> --- a/hw/exynos4210_pmu.c
>>> +++ b/hw/exynos4210_pmu.c
>>> @@ -25,6 +25,7 @@
>>>     */
>>>
>>>    #include "sysbus.h"
>>> +#include "sysemu.h"
>>>
>>>    #ifndef DEBUG_PMU
>>>    #define DEBUG_PMU           0
>>> @@ -422,6 +423,16 @@ static void exynos4210_pmu_write(void *opaque,
>>> target_phys_addr_t offset,
>>>            if (reg_p->offset == offset) {
>>>                PRINT_DEBUG_EXTEND("%s<0x%04x>   <- 0x%04x\n", reg_p->name,
>>>                        (uint32_t)offset, (uint32_t)val);
>>> +            switch (offset) {
>>> +            case SWRESET:
>>> +                if (val&   1) {
>>> +                    qemu_system_reset_request();
>>> +                }
>>> +                break;
>>> +            default:
>>> +                /* Nothing */
>>> +                break;
>>> +            }
>>>                s->reg[i] = val;
>>>                return;
>>>            }
>> It's not quite well. At first, when you do reset, appropriate status
>> must be set in RST_STAT register. At second, not all registers in PMU
>> should be set in default value after reset, so you should change PMU
>> reset function for handling different resets (see spec) So, this
>> functionality should be wrote more carefully
> Well, this is the case when there is a need to modify booting procedure
> depending on the values of these registers.  I haven't found any such
> code in the current kernel.  As I now remember saving their values was
> indeed important when we were trying to use an U-Boot bootloader.  But
> as long as we are sticking with QEMU bootloader it doesn't matter.

3.4-rc1 and 2.6.36 kernels use INFORM5 register which should keep his 
value during sw reset. And U-Boot (which can be used instead of kernel 
for some purposes) uses INFORM[456] registers which should be saved as well.

My opinion is we should not add code which describe incorrect behavior 
of the device regardless of whether kernel uses some registers or no.

> Anyway this information is unfortunately absent in Exynos4210 public
> documentation.
Hm... Really, I've just found out that public specification doesn't 
contain information about PMU
>>
>> And use #define for registers and fields of the registers. It's more
>> clearly, as for me.
> As you say.
:)
>>
>> Regards,
>> MK
>
>      Regards,
>      Dmitry
>
>
Dmitry Zhurikhin - April 4, 2012, 5:37 p.m.
On 2012-04-04 20:16, Maksim Kozlov wrote:
> 04.04.2012 16:35, Dmitry Zhurikhin пишет:
>> On 2012-04-04 15:55, Maksim Kozlov wrote:
>>> 04.04.2012 14:08, Dmitry Zhurikhin пишет:
>>>> Reset the system when 1 is written to SWRESET register
>>>>
>>>> Signed-off-by: Dmitry Zhurikhin<zhur@ispras.ru>
>>>> ---
>>>>    hw/exynos4210_pmu.c |   11 +++++++++++
>>>>    1 files changed, 11 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
>>>> index c12d750..edf6e34 100644
>>>> --- a/hw/exynos4210_pmu.c
>>>> +++ b/hw/exynos4210_pmu.c
>>>> @@ -25,6 +25,7 @@
>>>>     */
>>>>
>>>>    #include "sysbus.h"
>>>> +#include "sysemu.h"
>>>>
>>>>    #ifndef DEBUG_PMU
>>>>    #define DEBUG_PMU           0
>>>> @@ -422,6 +423,16 @@ static void exynos4210_pmu_write(void *opaque,
>>>> target_phys_addr_t offset,
>>>>            if (reg_p->offset == offset) {
>>>>                PRINT_DEBUG_EXTEND("%s<0x%04x>   <- 0x%04x\n",
>>>> reg_p->name,
>>>>                        (uint32_t)offset, (uint32_t)val);
>>>> +            switch (offset) {
>>>> +            case SWRESET:
>>>> +                if (val&   1) {
>>>> +                    qemu_system_reset_request();
>>>> +                }
>>>> +                break;
>>>> +            default:
>>>> +                /* Nothing */
>>>> +                break;
>>>> +            }
>>>>                s->reg[i] = val;
>>>>                return;
>>>>            }
>>> It's not quite well. At first, when you do reset, appropriate status
>>> must be set in RST_STAT register. At second, not all registers in PMU
>>> should be set in default value after reset, so you should change PMU
>>> reset function for handling different resets (see spec) So, this
>>> functionality should be wrote more carefully
>> Well, this is the case when there is a need to modify booting procedure
>> depending on the values of these registers.  I haven't found any such
>> code in the current kernel.  As I now remember saving their values was
>> indeed important when we were trying to use an U-Boot bootloader.  But
>> as long as we are sticking with QEMU bootloader it doesn't matter.
>
> 3.4-rc1 and 2.6.36 kernels use INFORM5 register which should keep his
> value during sw reset. And U-Boot (which can be used instead of kernel
> for some purposes) uses INFORM[456] registers which should be saved as
> well.
>
> My opinion is we should not add code which describe incorrect behavior
> of the device regardless of whether kernel uses some registers or no.
OK, this is a righteous position.  Again, I think in this case
implementing what you propose won't be neither bad nor good.  But since
you've asked, I'll try to do it.
>
>> Anyway this information is unfortunately absent in Exynos4210 public
>> documentation.
> Hm... Really, I've just found out that public specification doesn't
> contain information about PMU
I can make the changes according to Exynos3210 specification since they
are pretty close.  But of course I'd prefer that you send me full
specifications.  Dmitry, what you say?
>>>
>>> And use #define for registers and fields of the registers. It's more
>>> clearly, as for me.
>> As you say.
> :)
>>>
>>> Regards,
>>> MK
>>
>>      Regards,
>>      Dmitry
>>
>>
>
    Regards,
    Dmitry
Maksim E. Kozlov - April 6, 2012, 7:22 a.m.
04.04.2012 21:37, Dmitry Zhurikhin пишет:
> On 2012-04-04 20:16, Maksim Kozlov wrote:
>> 04.04.2012 16:35, Dmitry Zhurikhin пишет:
>>> On 2012-04-04 15:55, Maksim Kozlov wrote:
>>>> 04.04.2012 14:08, Dmitry Zhurikhin пишет:
>>>>> Reset the system when 1 is written to SWRESET register
>>>>>
>>>>> Signed-off-by: Dmitry Zhurikhin<zhur@ispras.ru>
>>>>> ---
>>>>>     hw/exynos4210_pmu.c |   11 +++++++++++
>>>>>     1 files changed, 11 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
>>>>> index c12d750..edf6e34 100644
>>>>> --- a/hw/exynos4210_pmu.c
>>>>> +++ b/hw/exynos4210_pmu.c
>>>>> @@ -25,6 +25,7 @@
>>>>>      */
>>>>>
>>>>>     #include "sysbus.h"
>>>>> +#include "sysemu.h"
>>>>>
>>>>>     #ifndef DEBUG_PMU
>>>>>     #define DEBUG_PMU           0
>>>>> @@ -422,6 +423,16 @@ static void exynos4210_pmu_write(void *opaque,
>>>>> target_phys_addr_t offset,
>>>>>             if (reg_p->offset == offset) {
>>>>>                 PRINT_DEBUG_EXTEND("%s<0x%04x>    <- 0x%04x\n",
>>>>> reg_p->name,
>>>>>                         (uint32_t)offset, (uint32_t)val);
>>>>> +            switch (offset) {
>>>>> +            case SWRESET:
>>>>> +                if (val&    1) {
>>>>> +                    qemu_system_reset_request();
>>>>> +                }
>>>>> +                break;
>>>>> +            default:
>>>>> +                /* Nothing */
>>>>> +                break;
>>>>> +            }
>>>>>                 s->reg[i] = val;
>>>>>                 return;
>>>>>             }
>>>> It's not quite well. At first, when you do reset, appropriate status
>>>> must be set in RST_STAT register. At second, not all registers in PMU
>>>> should be set in default value after reset, so you should change PMU
>>>> reset function for handling different resets (see spec) So, this
>>>> functionality should be wrote more carefully
>>> Well, this is the case when there is a need to modify booting procedure
>>> depending on the values of these registers.  I haven't found any such
>>> code in the current kernel.  As I now remember saving their values was
>>> indeed important when we were trying to use an U-Boot bootloader.  But
>>> as long as we are sticking with QEMU bootloader it doesn't matter.
>>
>> 3.4-rc1 and 2.6.36 kernels use INFORM5 register which should keep his
>> value during sw reset. And U-Boot (which can be used instead of kernel
>> for some purposes) uses INFORM[456] registers which should be saved as
>> well.
>>
>> My opinion is we should not add code which describe incorrect behavior
>> of the device regardless of whether kernel uses some registers or no.
> OK, this is a righteous position.  Again, I think in this case
> implementing what you propose won't be neither bad nor good.  But since
> you've asked, I'll try to do it.
>>
>>> Anyway this information is unfortunately absent in Exynos4210 public
>>> documentation.
>> Hm... Really, I've just found out that public specification doesn't
>> contain information about PMU
> I can make the changes according to Exynos3210 specification since they
> are pretty close.  But of course I'd prefer that you send me full
> specifications.  Dmitry, what you say?
PMUs in exynos4210 and in exynos3210 is not pretty close. They have 
significant differences.

>>>>
>>>> And use #define for registers and fields of the registers. It's more
>>>> clearly, as for me.
>>> As you say.
>> :)
>>>>
>>>> Regards,
>>>> MK
>>>
>>>       Regards,
>>>       Dmitry
>>>
>>>
>>
>      Regards,
>      Dmitry
>
>
>
Dmitry Solodkiy - April 6, 2012, 7:26 a.m.
Hi Dmitry,

  Suggest you first get internal specifications via official routes (I have no authority to distribute it) and then propose patches which wouldn't break existing exynos4210 model.

Thanks,
     Dmitry Solodkiy,
     Emulator/Kernel PL, Mobile Group,
     Moscow R&D center, Samsung Electronics

-----Original Message-----
From: Dmitry Zhurikhin [mailto:zhur@ispras.ru] 
Sent: Wednesday, April 04, 2012 9:38 PM
To: Maksim Kozlov
Cc: Kyungmin Park; Igor Mitsyanko; qemu-devel@nongnu.org; Evgeny Voevodin; Dmitry Solodkiy
Subject: Re: [PATCH] Support system reset in Exynos4210

On 2012-04-04 20:16, Maksim Kozlov wrote:
> 04.04.2012 16:35, Dmitry Zhurikhin пишет:
>> On 2012-04-04 15:55, Maksim Kozlov wrote:
>>> 04.04.2012 14:08, Dmitry Zhurikhin пишет:
>>>> Reset the system when 1 is written to SWRESET register
>>>>
>>>> Signed-off-by: Dmitry Zhurikhin<zhur@ispras.ru>
>>>> ---
>>>>    hw/exynos4210_pmu.c |   11 +++++++++++
>>>>    1 files changed, 11 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c index 
>>>> c12d750..edf6e34 100644
>>>> --- a/hw/exynos4210_pmu.c
>>>> +++ b/hw/exynos4210_pmu.c
>>>> @@ -25,6 +25,7 @@
>>>>     */
>>>>
>>>>    #include "sysbus.h"
>>>> +#include "sysemu.h"
>>>>
>>>>    #ifndef DEBUG_PMU
>>>>    #define DEBUG_PMU           0
>>>> @@ -422,6 +423,16 @@ static void exynos4210_pmu_write(void *opaque, 
>>>> target_phys_addr_t offset,
>>>>            if (reg_p->offset == offset) {
>>>>                PRINT_DEBUG_EXTEND("%s<0x%04x>   <- 0x%04x\n",
>>>> reg_p->name,
>>>>                        (uint32_t)offset, (uint32_t)val);
>>>> +            switch (offset) {
>>>> +            case SWRESET:
>>>> +                if (val&   1) {
>>>> +                    qemu_system_reset_request();
>>>> +                }
>>>> +                break;
>>>> +            default:
>>>> +                /* Nothing */
>>>> +                break;
>>>> +            }
>>>>                s->reg[i] = val;
>>>>                return;
>>>>            }
>>> It's not quite well. At first, when you do reset, appropriate status 
>>> must be set in RST_STAT register. At second, not all registers in 
>>> PMU should be set in default value after reset, so you should change 
>>> PMU reset function for handling different resets (see spec) So, this 
>>> functionality should be wrote more carefully
>> Well, this is the case when there is a need to modify booting 
>> procedure depending on the values of these registers.  I haven't 
>> found any such code in the current kernel.  As I now remember saving 
>> their values was indeed important when we were trying to use an 
>> U-Boot bootloader.  But as long as we are sticking with QEMU bootloader it doesn't matter.
>
> 3.4-rc1 and 2.6.36 kernels use INFORM5 register which should keep his 
> value during sw reset. And U-Boot (which can be used instead of kernel 
> for some purposes) uses INFORM[456] registers which should be saved as 
> well.
>
> My opinion is we should not add code which describe incorrect behavior 
> of the device regardless of whether kernel uses some registers or no.
OK, this is a righteous position.  Again, I think in this case implementing what you propose won't be neither bad nor good.  But since you've asked, I'll try to do it.
>
>> Anyway this information is unfortunately absent in Exynos4210 public 
>> documentation.
> Hm... Really, I've just found out that public specification doesn't 
> contain information about PMU
I can make the changes according to Exynos3210 specification since they are pretty close.  But of course I'd prefer that you send me full specifications.  Dmitry, what you say?
>>>
>>> And use #define for registers and fields of the registers. It's more 
>>> clearly, as for me.
>> As you say.
> :)
>>>
>>> Regards,
>>> MK
>>
>>      Regards,
>>      Dmitry
>>
>>
>
    Regards,
    Dmitry

Patch

diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
index c12d750..edf6e34 100644
--- a/hw/exynos4210_pmu.c
+++ b/hw/exynos4210_pmu.c
@@ -25,6 +25,7 @@ 
  */
 
 #include "sysbus.h"
+#include "sysemu.h"
 
 #ifndef DEBUG_PMU
 #define DEBUG_PMU           0
@@ -422,6 +423,16 @@  static void exynos4210_pmu_write(void *opaque, target_phys_addr_t offset,
         if (reg_p->offset == offset) {
             PRINT_DEBUG_EXTEND("%s <0x%04x> <- 0x%04x\n", reg_p->name,
                     (uint32_t)offset, (uint32_t)val);
+            switch (offset) {
+            case SWRESET:
+                if (val & 1) {
+                    qemu_system_reset_request();
+                }
+                break;
+            default:
+                /* Nothing */
+                break;
+            }
             s->reg[i] = val;
             return;
         }