diff mbox series

[U-Boot] cmd: reset: add parameters to specify reboot_mode

Message ID 20190503202523.23423-1-simon.k.r.goldschmidt@gmail.com
State Rejected, archived
Delegated to: Simon Goldschmidt
Headers show
Series [U-Boot] cmd: reset: add parameters to specify reboot_mode | expand

Commit Message

Simon Goldschmidt May 3, 2019, 8:25 p.m. UTC
This patch adds parameter support for the 'reset' command to specify
the reboot mode (cold vs. warm).

Checking these parameters is implemented in the DM implementation.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 cmd/boot.c                         |  4 ++--
 drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 3 deletions(-)

Comments

Marek Vasut May 3, 2019, 8:27 p.m. UTC | #1
On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
> This patch adds parameter support for the 'reset' command to specify
> the reboot mode (cold vs. warm).
> 
> Checking these parameters is implemented in the DM implementation.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
>  cmd/boot.c                         |  4 ++--
>  drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/cmd/boot.c b/cmd/boot.c
> index 9150fce80b..c3f33a9ca3 100644
> --- a/cmd/boot.c
> +++ b/cmd/boot.c
> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>  #endif
>  
>  U_BOOT_CMD(
> -	reset, 1, 0,	do_reset,
> +	reset, 2, 0,	do_reset,
>  	"Perform RESET of the CPU",
> -	""
> +	"[<cold|warm>] - type of reboot"
>  );
>  
>  #ifdef CONFIG_CMD_POWEROFF
> diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
> index ad831c703a..fbda3f44f2 100644
> --- a/drivers/sysreset/sysreset-uclass.c
> +++ b/drivers/sysreset/sysreset-uclass.c
> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>  
>  int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> +	enum sysreset_t reboot_mode = SYSRESET_COLD;
> +
> +	if (argc > 1 && argv[1]) {
> +		switch (*argv[1]) {
> +		case 'w':
> +			reboot_mode = SYSRESET_WARM;
> +			printf("warm ");
> +			break;
> +		case 'c':
> +			reboot_mode = SYSRESET_COLD;
> +			printf("cold ");
> +			break;

This looks like a platform or driver specific stuff ?
Simon Goldschmidt May 3, 2019, 8:33 p.m. UTC | #2
On 03.05.19 22:27, Marek Vasut wrote:
> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
>> This patch adds parameter support for the 'reset' command to specify
>> the reboot mode (cold vs. warm).
>>
>> Checking these parameters is implemented in the DM implementation.
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>> ---
>>
>>   cmd/boot.c                         |  4 ++--
>>   drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>>   2 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/cmd/boot.c b/cmd/boot.c
>> index 9150fce80b..c3f33a9ca3 100644
>> --- a/cmd/boot.c
>> +++ b/cmd/boot.c
>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>>   #endif
>>   
>>   U_BOOT_CMD(
>> -	reset, 1, 0,	do_reset,
>> +	reset, 2, 0,	do_reset,
>>   	"Perform RESET of the CPU",
>> -	""
>> +	"[<cold|warm>] - type of reboot"
>>   );
>>   
>>   #ifdef CONFIG_CMD_POWEROFF
>> diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
>> index ad831c703a..fbda3f44f2 100644
>> --- a/drivers/sysreset/sysreset-uclass.c
>> +++ b/drivers/sysreset/sysreset-uclass.c
>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>>   
>>   int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>   {
>> +	enum sysreset_t reboot_mode = SYSRESET_COLD;
>> +
>> +	if (argc > 1 && argv[1]) {
>> +		switch (*argv[1]) {
>> +		case 'w':
>> +			reboot_mode = SYSRESET_WARM;
>> +			printf("warm ");
>> +			break;
>> +		case 'c':
>> +			reboot_mode = SYSRESET_COLD;
>> +			printf("cold ");
>> +			break;
> 
> This looks like a platform or driver specific stuff ?

Ouch, I just saw the extra printf might have to be removed in a v2...

Anyway, except for that, I don't think it's platform or driver specific. 
It's just a way to make the cmd 'reset' take arguments that map to enum 
sysreset_t.

There is a problem in that other platforms without UCLASS_SYSRESET don't 
handle these arguments, but I thought UCLASS_SYSRESET would be the 
future, and the rest would be "legacy"?

Regards,
SImon
Marek Vasut May 3, 2019, 8:37 p.m. UTC | #3
On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
> 
> 
> On 03.05.19 22:27, Marek Vasut wrote:
>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
>>> This patch adds parameter support for the 'reset' command to specify
>>> the reboot mode (cold vs. warm).
>>>
>>> Checking these parameters is implemented in the DM implementation.
>>>
>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>> ---
>>>
>>>   cmd/boot.c                         |  4 ++--
>>>   drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>>>   2 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/cmd/boot.c b/cmd/boot.c
>>> index 9150fce80b..c3f33a9ca3 100644
>>> --- a/cmd/boot.c
>>> +++ b/cmd/boot.c
>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>>>   #endif
>>>     U_BOOT_CMD(
>>> -    reset, 1, 0,    do_reset,
>>> +    reset, 2, 0,    do_reset,
>>>       "Perform RESET of the CPU",
>>> -    ""
>>> +    "[<cold|warm>] - type of reboot"
>>>   );
>>>     #ifdef CONFIG_CMD_POWEROFF
>>> diff --git a/drivers/sysreset/sysreset-uclass.c
>>> b/drivers/sysreset/sysreset-uclass.c
>>> index ad831c703a..fbda3f44f2 100644
>>> --- a/drivers/sysreset/sysreset-uclass.c
>>> +++ b/drivers/sysreset/sysreset-uclass.c
>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>>>     int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>> argv[])
>>>   {
>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
>>> +
>>> +    if (argc > 1 && argv[1]) {
>>> +        switch (*argv[1]) {
>>> +        case 'w':
>>> +            reboot_mode = SYSRESET_WARM;
>>> +            printf("warm ");
>>> +            break;
>>> +        case 'c':
>>> +            reboot_mode = SYSRESET_COLD;
>>> +            printf("cold ");
>>> +            break;
>>
>> This looks like a platform or driver specific stuff ?
> 
> Ouch, I just saw the extra printf might have to be removed in a v2...
> 
> Anyway, except for that, I don't think it's platform or driver specific.
> It's just a way to make the cmd 'reset' take arguments that map to enum
> sysreset_t.

Cold vs. Warm reset is socfpga specific. The reset driver should
probably somehow register supported reset modes (warm/cold) with the
reset core code.

> There is a problem in that other platforms without UCLASS_SYSRESET don't
> handle these arguments, but I thought UCLASS_SYSRESET would be the
> future, and the rest would be "legacy"?

Yep.
Simon Goldschmidt May 3, 2019, 8:40 p.m. UTC | #4
On 03.05.19 22:37, Marek Vasut wrote:
> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
>>
>>
>> On 03.05.19 22:27, Marek Vasut wrote:
>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
>>>> This patch adds parameter support for the 'reset' command to specify
>>>> the reboot mode (cold vs. warm).
>>>>
>>>> Checking these parameters is implemented in the DM implementation.
>>>>
>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>> ---
>>>>
>>>>    cmd/boot.c                         |  4 ++--
>>>>    drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>>>>    2 files changed, 18 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/cmd/boot.c b/cmd/boot.c
>>>> index 9150fce80b..c3f33a9ca3 100644
>>>> --- a/cmd/boot.c
>>>> +++ b/cmd/boot.c
>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>>>>    #endif
>>>>      U_BOOT_CMD(
>>>> -    reset, 1, 0,    do_reset,
>>>> +    reset, 2, 0,    do_reset,
>>>>        "Perform RESET of the CPU",
>>>> -    ""
>>>> +    "[<cold|warm>] - type of reboot"
>>>>    );
>>>>      #ifdef CONFIG_CMD_POWEROFF
>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
>>>> b/drivers/sysreset/sysreset-uclass.c
>>>> index ad831c703a..fbda3f44f2 100644
>>>> --- a/drivers/sysreset/sysreset-uclass.c
>>>> +++ b/drivers/sysreset/sysreset-uclass.c
>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>>>>      int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>>> argv[])
>>>>    {
>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
>>>> +
>>>> +    if (argc > 1 && argv[1]) {
>>>> +        switch (*argv[1]) {
>>>> +        case 'w':
>>>> +            reboot_mode = SYSRESET_WARM;
>>>> +            printf("warm ");
>>>> +            break;
>>>> +        case 'c':
>>>> +            reboot_mode = SYSRESET_COLD;
>>>> +            printf("cold ");
>>>> +            break;
>>>
>>> This looks like a platform or driver specific stuff ?
>>
>> Ouch, I just saw the extra printf might have to be removed in a v2...
>>
>> Anyway, except for that, I don't think it's platform or driver specific.
>> It's just a way to make the cmd 'reset' take arguments that map to enum
>> sysreset_t.
> 
> Cold vs. Warm reset is socfpga specific. The reset driver should
> probably somehow register supported reset modes (warm/cold) with the
> reset core code.

But it's a thing the UCLASS_SYSRESET already exposes. I haven't added 
the enum values for that, I merely exposed them to the cmd "API". I 
can't see anything socfpga specific about it.

> 
>> There is a problem in that other platforms without UCLASS_SYSRESET don't
>> handle these arguments, but I thought UCLASS_SYSRESET would be the
>> future, and the rest would be "legacy"?
> 
> Yep.
>
Marek Vasut May 3, 2019, 8:43 p.m. UTC | #5
On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
> 
> 
> On 03.05.19 22:37, Marek Vasut wrote:
>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
>>>
>>>
>>> On 03.05.19 22:27, Marek Vasut wrote:
>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
>>>>> This patch adds parameter support for the 'reset' command to specify
>>>>> the reboot mode (cold vs. warm).
>>>>>
>>>>> Checking these parameters is implemented in the DM implementation.
>>>>>
>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>>> ---
>>>>>
>>>>>    cmd/boot.c                         |  4 ++--
>>>>>    drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>>>>>    2 files changed, 18 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
>>>>> index 9150fce80b..c3f33a9ca3 100644
>>>>> --- a/cmd/boot.c
>>>>> +++ b/cmd/boot.c
>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>>>>>    #endif
>>>>>      U_BOOT_CMD(
>>>>> -    reset, 1, 0,    do_reset,
>>>>> +    reset, 2, 0,    do_reset,
>>>>>        "Perform RESET of the CPU",
>>>>> -    ""
>>>>> +    "[<cold|warm>] - type of reboot"
>>>>>    );
>>>>>      #ifdef CONFIG_CMD_POWEROFF
>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
>>>>> b/drivers/sysreset/sysreset-uclass.c
>>>>> index ad831c703a..fbda3f44f2 100644
>>>>> --- a/drivers/sysreset/sysreset-uclass.c
>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>>>>>      int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>>>> argv[])
>>>>>    {
>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
>>>>> +
>>>>> +    if (argc > 1 && argv[1]) {
>>>>> +        switch (*argv[1]) {
>>>>> +        case 'w':
>>>>> +            reboot_mode = SYSRESET_WARM;
>>>>> +            printf("warm ");
>>>>> +            break;
>>>>> +        case 'c':
>>>>> +            reboot_mode = SYSRESET_COLD;
>>>>> +            printf("cold ");
>>>>> +            break;
>>>>
>>>> This looks like a platform or driver specific stuff ?
>>>
>>> Ouch, I just saw the extra printf might have to be removed in a v2...
>>>
>>> Anyway, except for that, I don't think it's platform or driver specific.
>>> It's just a way to make the cmd 'reset' take arguments that map to enum
>>> sysreset_t.
>>
>> Cold vs. Warm reset is socfpga specific. The reset driver should
>> probably somehow register supported reset modes (warm/cold) with the
>> reset core code.
> 
> But it's a thing the UCLASS_SYSRESET already exposes. I haven't added
> the enum values for that, I merely exposed them to the cmd "API". I
> can't see anything socfpga specific about it.

Ah, then socfpga just maps to that "API" very well, nice. I wasn't aware
of it.
Simon Glass May 3, 2019, 9:10 p.m. UTC | #6
Hi Simon,

On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
>
> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
> >
> >
> > On 03.05.19 22:37, Marek Vasut wrote:
> >> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
> >>>
> >>>
> >>> On 03.05.19 22:27, Marek Vasut wrote:
> >>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
> >>>>> This patch adds parameter support for the 'reset' command to specify
> >>>>> the reboot mode (cold vs. warm).
> >>>>>
> >>>>> Checking these parameters is implemented in the DM implementation.
> >>>>>
> >>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >>>>> ---
> >>>>>
> >>>>>    cmd/boot.c                         |  4 ++--
> >>>>>    drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
> >>>>>    2 files changed, 18 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/cmd/boot.c b/cmd/boot.c
> >>>>> index 9150fce80b..c3f33a9ca3 100644
> >>>>> --- a/cmd/boot.c
> >>>>> +++ b/cmd/boot.c
> >>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
> >>>>>    #endif
> >>>>>      U_BOOT_CMD(
> >>>>> -    reset, 1, 0,    do_reset,
> >>>>> +    reset, 2, 0,    do_reset,
> >>>>>        "Perform RESET of the CPU",
> >>>>> -    ""
> >>>>> +    "[<cold|warm>] - type of reboot"
> >>>>>    );
> >>>>>      #ifdef CONFIG_CMD_POWEROFF
> >>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
> >>>>> b/drivers/sysreset/sysreset-uclass.c
> >>>>> index ad831c703a..fbda3f44f2 100644
> >>>>> --- a/drivers/sysreset/sysreset-uclass.c
> >>>>> +++ b/drivers/sysreset/sysreset-uclass.c
> >>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
> >>>>>      int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> >>>>> argv[])
> >>>>>    {
> >>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
> >>>>> +
> >>>>> +    if (argc > 1 && argv[1]) {
> >>>>> +        switch (*argv[1]) {
> >>>>> +        case 'w':
> >>>>> +            reboot_mode = SYSRESET_WARM;
> >>>>> +            printf("warm ");
> >>>>> +            break;
> >>>>> +        case 'c':
> >>>>> +            reboot_mode = SYSRESET_COLD;
> >>>>> +            printf("cold ");
> >>>>> +            break;

Please can we have a pytest for this command?

Regards,
Simon

> >>>>
> >>>> This looks like a platform or driver specific stuff ?
> >>>
> >>> Ouch, I just saw the extra printf might have to be removed in a v2...
> >>>
> >>> Anyway, except for that, I don't think it's platform or driver specific.
> >>> It's just a way to make the cmd 'reset' take arguments that map to enum
> >>> sysreset_t.
> >>
> >> Cold vs. Warm reset is socfpga specific. The reset driver should
> >> probably somehow register supported reset modes (warm/cold) with the
> >> reset core code.
> >
> > But it's a thing the UCLASS_SYSRESET already exposes. I haven't added
> > the enum values for that, I merely exposed them to the cmd "API". I
> > can't see anything socfpga specific about it.
>
> Ah, then socfpga just maps to that "API" very well, nice. I wasn't aware
> of it.
>
> --
> Best regards,
> Marek Vasut
Simon Goldschmidt May 4, 2019, 6:23 p.m. UTC | #7
Am 03.05.2019 um 23:10 schrieb Simon Glass:
> Hi Simon,
> 
> On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
>>
>> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
>>>
>>>
>>> On 03.05.19 22:37, Marek Vasut wrote:
>>>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
>>>>>
>>>>>
>>>>> On 03.05.19 22:27, Marek Vasut wrote:
>>>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
>>>>>>> This patch adds parameter support for the 'reset' command to specify
>>>>>>> the reboot mode (cold vs. warm).
>>>>>>>
>>>>>>> Checking these parameters is implemented in the DM implementation.
>>>>>>>
>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>     cmd/boot.c                         |  4 ++--
>>>>>>>     drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>>>>>>>     2 files changed, 18 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
>>>>>>> index 9150fce80b..c3f33a9ca3 100644
>>>>>>> --- a/cmd/boot.c
>>>>>>> +++ b/cmd/boot.c
>>>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>>>>>>>     #endif
>>>>>>>       U_BOOT_CMD(
>>>>>>> -    reset, 1, 0,    do_reset,
>>>>>>> +    reset, 2, 0,    do_reset,
>>>>>>>         "Perform RESET of the CPU",
>>>>>>> -    ""
>>>>>>> +    "[<cold|warm>] - type of reboot"
>>>>>>>     );
>>>>>>>       #ifdef CONFIG_CMD_POWEROFF
>>>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
>>>>>>> b/drivers/sysreset/sysreset-uclass.c
>>>>>>> index ad831c703a..fbda3f44f2 100644
>>>>>>> --- a/drivers/sysreset/sysreset-uclass.c
>>>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
>>>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>>>>>>>       int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>>>>>> argv[])
>>>>>>>     {
>>>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
>>>>>>> +
>>>>>>> +    if (argc > 1 && argv[1]) {
>>>>>>> +        switch (*argv[1]) {
>>>>>>> +        case 'w':
>>>>>>> +            reboot_mode = SYSRESET_WARM;
>>>>>>> +            printf("warm ");
>>>>>>> +            break;
>>>>>>> +        case 'c':
>>>>>>> +            reboot_mode = SYSRESET_COLD;
>>>>>>> +            printf("cold ");
>>>>>>> +            break;
> 
> Please can we have a pytest for this command?

There's 'test_sandbox_exit.py' that seems to test that the "reset" 
command exits sandbox process. How would I test differing between "warm" 
and "cold" exit?

Regards,
Simon

> 
> Regards,
> Simon
> 
>>>>>>
>>>>>> This looks like a platform or driver specific stuff ?
>>>>>
>>>>> Ouch, I just saw the extra printf might have to be removed in a v2...
>>>>>
>>>>> Anyway, except for that, I don't think it's platform or driver specific.
>>>>> It's just a way to make the cmd 'reset' take arguments that map to enum
>>>>> sysreset_t.
>>>>
>>>> Cold vs. Warm reset is socfpga specific. The reset driver should
>>>> probably somehow register supported reset modes (warm/cold) with the
>>>> reset core code.
>>>
>>> But it's a thing the UCLASS_SYSRESET already exposes. I haven't added
>>> the enum values for that, I merely exposed them to the cmd "API". I
>>> can't see anything socfpga specific about it.
>>
>> Ah, then socfpga just maps to that "API" very well, nice. I wasn't aware
>> of it.
>>
>> --
>> Best regards,
>> Marek Vasut
Simon Goldschmidt July 10, 2019, 6:50 p.m. UTC | #8
Simon,

I know I've slept for a while before retriggering this, but...

Am 04.05.2019 um 20:23 schrieb Simon Goldschmidt:
> Am 03.05.2019 um 23:10 schrieb Simon Glass:
>> Hi Simon,
>>
>> On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
>>>>
>>>>
>>>> On 03.05.19 22:37, Marek Vasut wrote:
>>>>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
>>>>>>
>>>>>>
>>>>>> On 03.05.19 22:27, Marek Vasut wrote:
>>>>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
>>>>>>>> This patch adds parameter support for the 'reset' command to specify
>>>>>>>> the reboot mode (cold vs. warm).
>>>>>>>>
>>>>>>>> Checking these parameters is implemented in the DM implementation.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      cmd/boot.c                         |  4 ++--
>>>>>>>>      drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>>>>>>>>      2 files changed, 18 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
>>>>>>>> index 9150fce80b..c3f33a9ca3 100644
>>>>>>>> --- a/cmd/boot.c
>>>>>>>> +++ b/cmd/boot.c
>>>>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>>>>>>>>      #endif
>>>>>>>>        U_BOOT_CMD(
>>>>>>>> -    reset, 1, 0,    do_reset,
>>>>>>>> +    reset, 2, 0,    do_reset,
>>>>>>>>          "Perform RESET of the CPU",
>>>>>>>> -    ""
>>>>>>>> +    "[<cold|warm>] - type of reboot"
>>>>>>>>      );
>>>>>>>>        #ifdef CONFIG_CMD_POWEROFF
>>>>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
>>>>>>>> b/drivers/sysreset/sysreset-uclass.c
>>>>>>>> index ad831c703a..fbda3f44f2 100644
>>>>>>>> --- a/drivers/sysreset/sysreset-uclass.c
>>>>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
>>>>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>>>>>>>>        int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>>>>>>> argv[])
>>>>>>>>      {
>>>>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
>>>>>>>> +
>>>>>>>> +    if (argc > 1 && argv[1]) {
>>>>>>>> +        switch (*argv[1]) {
>>>>>>>> +        case 'w':
>>>>>>>> +            reboot_mode = SYSRESET_WARM;
>>>>>>>> +            printf("warm ");
>>>>>>>> +            break;
>>>>>>>> +        case 'c':
>>>>>>>> +            reboot_mode = SYSRESET_COLD;
>>>>>>>> +            printf("cold ");
>>>>>>>> +            break;
>>
>> Please can we have a pytest for this command?
> 
> There's 'test_sandbox_exit.py' that seems to test that the "reset"
> command exits sandbox process. How would I test differing between "warm"
> and "cold" exit?

If this request for a test is what remains, can you please elaborate on 
what I should write to get this accepted?

Thanks,
Simon

> 
> Regards,
> Simon
> 
>>
>> Regards,
>> Simon
>>
>>>>>>>
>>>>>>> This looks like a platform or driver specific stuff ?
>>>>>>
>>>>>> Ouch, I just saw the extra printf might have to be removed in a v2...
>>>>>>
>>>>>> Anyway, except for that, I don't think it's platform or driver specific.
>>>>>> It's just a way to make the cmd 'reset' take arguments that map to enum
>>>>>> sysreset_t.
>>>>>
>>>>> Cold vs. Warm reset is socfpga specific. The reset driver should
>>>>> probably somehow register supported reset modes (warm/cold) with the
>>>>> reset core code.
>>>>
>>>> But it's a thing the UCLASS_SYSRESET already exposes. I haven't added
>>>> the enum values for that, I merely exposed them to the cmd "API". I
>>>> can't see anything socfpga specific about it.
>>>
>>> Ah, then socfpga just maps to that "API" very well, nice. I wasn't aware
>>> of it.
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
>
Simon Goldschmidt July 29, 2019, 10:47 a.m. UTC | #9
Simon,

On Wed, Jul 10, 2019 at 8:50 PM Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> Simon,
>
> I know I've slept for a while before retriggering this, but...
>
> Am 04.05.2019 um 20:23 schrieb Simon Goldschmidt:
> > Am 03.05.2019 um 23:10 schrieb Simon Glass:
> >> Hi Simon,
> >>
> >> On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
> >>>
> >>> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
> >>>>
> >>>>
> >>>> On 03.05.19 22:37, Marek Vasut wrote:
> >>>>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 03.05.19 22:27, Marek Vasut wrote:
> >>>>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
> >>>>>>>> This patch adds parameter support for the 'reset' command to specify
> >>>>>>>> the reboot mode (cold vs. warm).
> >>>>>>>>
> >>>>>>>> Checking these parameters is implemented in the DM implementation.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>>      cmd/boot.c                         |  4 ++--
> >>>>>>>>      drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
> >>>>>>>>      2 files changed, 18 insertions(+), 3 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
> >>>>>>>> index 9150fce80b..c3f33a9ca3 100644
> >>>>>>>> --- a/cmd/boot.c
> >>>>>>>> +++ b/cmd/boot.c
> >>>>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
> >>>>>>>>      #endif
> >>>>>>>>        U_BOOT_CMD(
> >>>>>>>> -    reset, 1, 0,    do_reset,
> >>>>>>>> +    reset, 2, 0,    do_reset,
> >>>>>>>>          "Perform RESET of the CPU",
> >>>>>>>> -    ""
> >>>>>>>> +    "[<cold|warm>] - type of reboot"
> >>>>>>>>      );
> >>>>>>>>        #ifdef CONFIG_CMD_POWEROFF
> >>>>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
> >>>>>>>> b/drivers/sysreset/sysreset-uclass.c
> >>>>>>>> index ad831c703a..fbda3f44f2 100644
> >>>>>>>> --- a/drivers/sysreset/sysreset-uclass.c
> >>>>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
> >>>>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
> >>>>>>>>        int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> >>>>>>>> argv[])
> >>>>>>>>      {
> >>>>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
> >>>>>>>> +
> >>>>>>>> +    if (argc > 1 && argv[1]) {
> >>>>>>>> +        switch (*argv[1]) {
> >>>>>>>> +        case 'w':
> >>>>>>>> +            reboot_mode = SYSRESET_WARM;
> >>>>>>>> +            printf("warm ");
> >>>>>>>> +            break;
> >>>>>>>> +        case 'c':
> >>>>>>>> +            reboot_mode = SYSRESET_COLD;
> >>>>>>>> +            printf("cold ");
> >>>>>>>> +            break;
> >>
> >> Please can we have a pytest for this command?
> >
> > There's 'test_sandbox_exit.py' that seems to test that the "reset"
> > command exits sandbox process. How would I test differing between "warm"
> > and "cold" exit?
>
> If this request for a test is what remains, can you please elaborate on
> what I should write to get this accepted?

Sorry, but I can't implement a test without you telling me what kind
of test you mean.

I'll be sending v2 (that removes the extra printf's from above) without
such a test and hope it can still be accepted.

Regards,
Simon

>
> Thanks,
> Simon
>
> >
> > Regards,
> > Simon
> >
> >>
> >> Regards,
> >> Simon
> >>
> >>>>>>>
> >>>>>>> This looks like a platform or driver specific stuff ?
> >>>>>>
> >>>>>> Ouch, I just saw the extra printf might have to be removed in a v2...
> >>>>>>
> >>>>>> Anyway, except for that, I don't think it's platform or driver specific.
> >>>>>> It's just a way to make the cmd 'reset' take arguments that map to enum
> >>>>>> sysreset_t.
> >>>>>
> >>>>> Cold vs. Warm reset is socfpga specific. The reset driver should
> >>>>> probably somehow register supported reset modes (warm/cold) with the
> >>>>> reset core code.
> >>>>
> >>>> But it's a thing the UCLASS_SYSRESET already exposes. I haven't added
> >>>> the enum values for that, I merely exposed them to the cmd "API". I
> >>>> can't see anything socfpga specific about it.
> >>>
> >>> Ah, then socfpga just maps to that "API" very well, nice. I wasn't aware
> >>> of it.
> >>>
> >>> --
> >>> Best regards,
> >>> Marek Vasut
> >
>
Simon Glass Aug. 1, 2019, 10:45 p.m. UTC | #10
Hi Simon,

On Mon, 29 Jul 2019 at 04:47, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> Simon,
>
> On Wed, Jul 10, 2019 at 8:50 PM Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
> >
> > Simon,
> >
> > I know I've slept for a while before retriggering this, but...
> >
> > Am 04.05.2019 um 20:23 schrieb Simon Goldschmidt:
> > > Am 03.05.2019 um 23:10 schrieb Simon Glass:
> > >> Hi Simon,
> > >>
> > >> On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
> > >>>
> > >>> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
> > >>>>
> > >>>>
> > >>>> On 03.05.19 22:37, Marek Vasut wrote:
> > >>>>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>> On 03.05.19 22:27, Marek Vasut wrote:
> > >>>>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
> > >>>>>>>> This patch adds parameter support for the 'reset' command to specify
> > >>>>>>>> the reboot mode (cold vs. warm).
> > >>>>>>>>
> > >>>>>>>> Checking these parameters is implemented in the DM implementation.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > >>>>>>>> ---
> > >>>>>>>>
> > >>>>>>>>      cmd/boot.c                         |  4 ++--
> > >>>>>>>>      drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
> > >>>>>>>>      2 files changed, 18 insertions(+), 3 deletions(-)
> > >>>>>>>>
> > >>>>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
> > >>>>>>>> index 9150fce80b..c3f33a9ca3 100644
> > >>>>>>>> --- a/cmd/boot.c
> > >>>>>>>> +++ b/cmd/boot.c
> > >>>>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
> > >>>>>>>>      #endif
> > >>>>>>>>        U_BOOT_CMD(
> > >>>>>>>> -    reset, 1, 0,    do_reset,
> > >>>>>>>> +    reset, 2, 0,    do_reset,
> > >>>>>>>>          "Perform RESET of the CPU",
> > >>>>>>>> -    ""
> > >>>>>>>> +    "[<cold|warm>] - type of reboot"
> > >>>>>>>>      );
> > >>>>>>>>        #ifdef CONFIG_CMD_POWEROFF
> > >>>>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
> > >>>>>>>> b/drivers/sysreset/sysreset-uclass.c
> > >>>>>>>> index ad831c703a..fbda3f44f2 100644
> > >>>>>>>> --- a/drivers/sysreset/sysreset-uclass.c
> > >>>>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
> > >>>>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
> > >>>>>>>>        int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > >>>>>>>> argv[])
> > >>>>>>>>      {
> > >>>>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
> > >>>>>>>> +
> > >>>>>>>> +    if (argc > 1 && argv[1]) {
> > >>>>>>>> +        switch (*argv[1]) {
> > >>>>>>>> +        case 'w':
> > >>>>>>>> +            reboot_mode = SYSRESET_WARM;
> > >>>>>>>> +            printf("warm ");
> > >>>>>>>> +            break;
> > >>>>>>>> +        case 'c':
> > >>>>>>>> +            reboot_mode = SYSRESET_COLD;
> > >>>>>>>> +            printf("cold ");
> > >>>>>>>> +            break;
> > >>
> > >> Please can we have a pytest for this command?
> > >
> > > There's 'test_sandbox_exit.py' that seems to test that the "reset"
> > > command exits sandbox process. How would I test differing between "warm"
> > > and "cold" exit?
> >
> > If this request for a test is what remains, can you please elaborate on
> > what I should write to get this accepted?
>
> Sorry, but I can't implement a test without you telling me what kind
> of test you mean.
>
> I'll be sending v2 (that removes the extra printf's from above) without
> such a test and hope it can still be accepted.

Well we have test_unknown_cmd.py for example, and test_pinmux.py,
which you could use as examples, just to check that the command works
and provides the right output.

Regards,
Simon
Simon Goldschmidt Aug. 2, 2019, 5:53 a.m. UTC | #11
On Fri, Aug 2, 2019 at 12:45 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> On Mon, 29 Jul 2019 at 04:47, Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
> >
> > Simon,
> >
> > On Wed, Jul 10, 2019 at 8:50 PM Simon Goldschmidt
> > <simon.k.r.goldschmidt@gmail.com> wrote:
> > >
> > > Simon,
> > >
> > > I know I've slept for a while before retriggering this, but...
> > >
> > > Am 04.05.2019 um 20:23 schrieb Simon Goldschmidt:
> > > > Am 03.05.2019 um 23:10 schrieb Simon Glass:
> > > >> Hi Simon,
> > > >>
> > > >> On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
> > > >>>
> > > >>> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
> > > >>>>
> > > >>>>
> > > >>>> On 03.05.19 22:37, Marek Vasut wrote:
> > > >>>>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
> > > >>>>>>
> > > >>>>>>
> > > >>>>>> On 03.05.19 22:27, Marek Vasut wrote:
> > > >>>>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
> > > >>>>>>>> This patch adds parameter support for the 'reset' command to specify
> > > >>>>>>>> the reboot mode (cold vs. warm).
> > > >>>>>>>>
> > > >>>>>>>> Checking these parameters is implemented in the DM implementation.
> > > >>>>>>>>
> > > >>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > >>>>>>>> ---
> > > >>>>>>>>
> > > >>>>>>>>      cmd/boot.c                         |  4 ++--
> > > >>>>>>>>      drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
> > > >>>>>>>>      2 files changed, 18 insertions(+), 3 deletions(-)
> > > >>>>>>>>
> > > >>>>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
> > > >>>>>>>> index 9150fce80b..c3f33a9ca3 100644
> > > >>>>>>>> --- a/cmd/boot.c
> > > >>>>>>>> +++ b/cmd/boot.c
> > > >>>>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
> > > >>>>>>>>      #endif
> > > >>>>>>>>        U_BOOT_CMD(
> > > >>>>>>>> -    reset, 1, 0,    do_reset,
> > > >>>>>>>> +    reset, 2, 0,    do_reset,
> > > >>>>>>>>          "Perform RESET of the CPU",
> > > >>>>>>>> -    ""
> > > >>>>>>>> +    "[<cold|warm>] - type of reboot"
> > > >>>>>>>>      );
> > > >>>>>>>>        #ifdef CONFIG_CMD_POWEROFF
> > > >>>>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
> > > >>>>>>>> b/drivers/sysreset/sysreset-uclass.c
> > > >>>>>>>> index ad831c703a..fbda3f44f2 100644
> > > >>>>>>>> --- a/drivers/sysreset/sysreset-uclass.c
> > > >>>>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
> > > >>>>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
> > > >>>>>>>>        int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > > >>>>>>>> argv[])
> > > >>>>>>>>      {
> > > >>>>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
> > > >>>>>>>> +
> > > >>>>>>>> +    if (argc > 1 && argv[1]) {
> > > >>>>>>>> +        switch (*argv[1]) {
> > > >>>>>>>> +        case 'w':
> > > >>>>>>>> +            reboot_mode = SYSRESET_WARM;
> > > >>>>>>>> +            printf("warm ");
> > > >>>>>>>> +            break;
> > > >>>>>>>> +        case 'c':
> > > >>>>>>>> +            reboot_mode = SYSRESET_COLD;
> > > >>>>>>>> +            printf("cold ");
> > > >>>>>>>> +            break;
> > > >>
> > > >> Please can we have a pytest for this command?
> > > >
> > > > There's 'test_sandbox_exit.py' that seems to test that the "reset"
> > > > command exits sandbox process. How would I test differing between "warm"
> > > > and "cold" exit?
> > >
> > > If this request for a test is what remains, can you please elaborate on
> > > what I should write to get this accepted?
> >
> > Sorry, but I can't implement a test without you telling me what kind
> > of test you mean.
> >
> > I'll be sending v2 (that removes the extra printf's from above) without
> > such a test and hope it can still be accepted.
>
> Well we have test_unknown_cmd.py for example, and test_pinmux.py,
> which you could use as examples, just to check that the command works
> and provides the right output.

The tests are run in sandbox, right? And my understanding was that the reset
command makes sandbox exit. That's why I asked this question: I just don'tests
know if/how I can write a test when the command under test exits sandbox.

Regards,
Simon
Simon Glass Aug. 2, 2019, 2:41 p.m. UTC | #12
Hi SImon,

On Thu, 1 Aug 2019 at 23:53, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> On Fri, Aug 2, 2019 at 12:45 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Simon,
> >
> > On Mon, 29 Jul 2019 at 04:47, Simon Goldschmidt
> > <simon.k.r.goldschmidt@gmail.com> wrote:
> > >
> > > Simon,
> > >
> > > On Wed, Jul 10, 2019 at 8:50 PM Simon Goldschmidt
> > > <simon.k.r.goldschmidt@gmail.com> wrote:
> > > >
> > > > Simon,
> > > >
> > > > I know I've slept for a while before retriggering this, but...
> > > >
> > > > Am 04.05.2019 um 20:23 schrieb Simon Goldschmidt:
> > > > > Am 03.05.2019 um 23:10 schrieb Simon Glass:
> > > > >> Hi Simon,
> > > > >>
> > > > >> On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
> > > > >>>
> > > > >>> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
> > > > >>>>
> > > > >>>>
> > > > >>>> On 03.05.19 22:37, Marek Vasut wrote:
> > > > >>>>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>> On 03.05.19 22:27, Marek Vasut wrote:
> > > > >>>>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
> > > > >>>>>>>> This patch adds parameter support for the 'reset' command to specify
> > > > >>>>>>>> the reboot mode (cold vs. warm).
> > > > >>>>>>>>
> > > > >>>>>>>> Checking these parameters is implemented in the DM implementation.
> > > > >>>>>>>>
> > > > >>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > > >>>>>>>> ---
> > > > >>>>>>>>
> > > > >>>>>>>>      cmd/boot.c                         |  4 ++--
> > > > >>>>>>>>      drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
> > > > >>>>>>>>      2 files changed, 18 insertions(+), 3 deletions(-)
> > > > >>>>>>>>
> > > > >>>>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
> > > > >>>>>>>> index 9150fce80b..c3f33a9ca3 100644
> > > > >>>>>>>> --- a/cmd/boot.c
> > > > >>>>>>>> +++ b/cmd/boot.c
> > > > >>>>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
> > > > >>>>>>>>      #endif
> > > > >>>>>>>>        U_BOOT_CMD(
> > > > >>>>>>>> -    reset, 1, 0,    do_reset,
> > > > >>>>>>>> +    reset, 2, 0,    do_reset,
> > > > >>>>>>>>          "Perform RESET of the CPU",
> > > > >>>>>>>> -    ""
> > > > >>>>>>>> +    "[<cold|warm>] - type of reboot"
> > > > >>>>>>>>      );
> > > > >>>>>>>>        #ifdef CONFIG_CMD_POWEROFF
> > > > >>>>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
> > > > >>>>>>>> b/drivers/sysreset/sysreset-uclass.c
> > > > >>>>>>>> index ad831c703a..fbda3f44f2 100644
> > > > >>>>>>>> --- a/drivers/sysreset/sysreset-uclass.c
> > > > >>>>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
> > > > >>>>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
> > > > >>>>>>>>        int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > > > >>>>>>>> argv[])
> > > > >>>>>>>>      {
> > > > >>>>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
> > > > >>>>>>>> +
> > > > >>>>>>>> +    if (argc > 1 && argv[1]) {
> > > > >>>>>>>> +        switch (*argv[1]) {
> > > > >>>>>>>> +        case 'w':
> > > > >>>>>>>> +            reboot_mode = SYSRESET_WARM;
> > > > >>>>>>>> +            printf("warm ");
> > > > >>>>>>>> +            break;
> > > > >>>>>>>> +        case 'c':
> > > > >>>>>>>> +            reboot_mode = SYSRESET_COLD;
> > > > >>>>>>>> +            printf("cold ");
> > > > >>>>>>>> +            break;
> > > > >>
> > > > >> Please can we have a pytest for this command?
> > > > >
> > > > > There's 'test_sandbox_exit.py' that seems to test that the "reset"
> > > > > command exits sandbox process. How would I test differing between "warm"
> > > > > and "cold" exit?
> > > >
> > > > If this request for a test is what remains, can you please elaborate on
> > > > what I should write to get this accepted?
> > >
> > > Sorry, but I can't implement a test without you telling me what kind
> > > of test you mean.
> > >
> > > I'll be sending v2 (that removes the extra printf's from above) without
> > > such a test and hope it can still be accepted.
> >
> > Well we have test_unknown_cmd.py for example, and test_pinmux.py,
> > which you could use as examples, just to check that the command works
> > and provides the right output.
>
> The tests are run in sandbox, right? And my understanding was that the reset
> command makes sandbox exit. That's why I asked this question: I just don'tests
> know if/how I can write a test when the command under test exits sandbox.

The sandbox sysreset driver has a way to avoid exiting. But even if
you do have it set to exit, that should be OK for the test.

Regards,
Simon
Simon Goldschmidt Oct. 9, 2019, 6:13 p.m. UTC | #13
Am 02.08.2019 um 16:41 schrieb Simon Glass:
> Hi SImon,
> 
> On Thu, 1 Aug 2019 at 23:53, Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
>>
>> On Fri, Aug 2, 2019 at 12:45 AM Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Mon, 29 Jul 2019 at 04:47, Simon Goldschmidt
>>> <simon.k.r.goldschmidt@gmail.com> wrote:
>>>>
>>>> Simon,
>>>>
>>>> On Wed, Jul 10, 2019 at 8:50 PM Simon Goldschmidt
>>>> <simon.k.r.goldschmidt@gmail.com> wrote:
>>>>>
>>>>> Simon,
>>>>>
>>>>> I know I've slept for a while before retriggering this, but...
>>>>>
>>>>> Am 04.05.2019 um 20:23 schrieb Simon Goldschmidt:
>>>>>> Am 03.05.2019 um 23:10 schrieb Simon Glass:
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> On Fri, 3 May 2019 at 14:43, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 5/3/19 10:40 PM, Simon Goldschmidt wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 03.05.19 22:37, Marek Vasut wrote:
>>>>>>>>>> On 5/3/19 10:33 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 03.05.19 22:27, Marek Vasut wrote:
>>>>>>>>>>>> On 5/3/19 10:25 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>>> This patch adds parameter support for the 'reset' command to specify
>>>>>>>>>>>>> the reboot mode (cold vs. warm).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Checking these parameters is implemented in the DM implementation.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>>       cmd/boot.c                         |  4 ++--
>>>>>>>>>>>>>       drivers/sysreset/sysreset-uclass.c | 17 ++++++++++++++++-
>>>>>>>>>>>>>       2 files changed, 18 insertions(+), 3 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/cmd/boot.c b/cmd/boot.c
>>>>>>>>>>>>> index 9150fce80b..c3f33a9ca3 100644
>>>>>>>>>>>>> --- a/cmd/boot.c
>>>>>>>>>>>>> +++ b/cmd/boot.c
>>>>>>>>>>>>> @@ -56,9 +56,9 @@ U_BOOT_CMD(
>>>>>>>>>>>>>       #endif
>>>>>>>>>>>>>         U_BOOT_CMD(
>>>>>>>>>>>>> -    reset, 1, 0,    do_reset,
>>>>>>>>>>>>> +    reset, 2, 0,    do_reset,
>>>>>>>>>>>>>           "Perform RESET of the CPU",
>>>>>>>>>>>>> -    ""
>>>>>>>>>>>>> +    "[<cold|warm>] - type of reboot"
>>>>>>>>>>>>>       );
>>>>>>>>>>>>>         #ifdef CONFIG_CMD_POWEROFF
>>>>>>>>>>>>> diff --git a/drivers/sysreset/sysreset-uclass.c
>>>>>>>>>>>>> b/drivers/sysreset/sysreset-uclass.c
>>>>>>>>>>>>> index ad831c703a..fbda3f44f2 100644
>>>>>>>>>>>>> --- a/drivers/sysreset/sysreset-uclass.c
>>>>>>>>>>>>> +++ b/drivers/sysreset/sysreset-uclass.c
>>>>>>>>>>>>> @@ -111,9 +111,24 @@ void reset_cpu(ulong addr)
>>>>>>>>>>>>>         int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>>>>>>>>>>>> argv[])
>>>>>>>>>>>>>       {
>>>>>>>>>>>>> +    enum sysreset_t reboot_mode = SYSRESET_COLD;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    if (argc > 1 && argv[1]) {
>>>>>>>>>>>>> +        switch (*argv[1]) {
>>>>>>>>>>>>> +        case 'w':
>>>>>>>>>>>>> +            reboot_mode = SYSRESET_WARM;
>>>>>>>>>>>>> +            printf("warm ");
>>>>>>>>>>>>> +            break;
>>>>>>>>>>>>> +        case 'c':
>>>>>>>>>>>>> +            reboot_mode = SYSRESET_COLD;
>>>>>>>>>>>>> +            printf("cold ");
>>>>>>>>>>>>> +            break;
>>>>>>>
>>>>>>> Please can we have a pytest for this command?
>>>>>>
>>>>>> There's 'test_sandbox_exit.py' that seems to test that the "reset"
>>>>>> command exits sandbox process. How would I test differing between "warm"
>>>>>> and "cold" exit?
>>>>>
>>>>> If this request for a test is what remains, can you please elaborate on
>>>>> what I should write to get this accepted?
>>>>
>>>> Sorry, but I can't implement a test without you telling me what kind
>>>> of test you mean.
>>>>
>>>> I'll be sending v2 (that removes the extra printf's from above) without
>>>> such a test and hope it can still be accepted.
>>>
>>> Well we have test_unknown_cmd.py for example, and test_pinmux.py,
>>> which you could use as examples, just to check that the command works
>>> and provides the right output.
>>
>> The tests are run in sandbox, right? And my understanding was that the reset
>> command makes sandbox exit. That's why I asked this question: I just don'tests
>> know if/how I can write a test when the command under test exits sandbox.
> 
> The sandbox sysreset driver has a way to avoid exiting. But even if
> you do have it set to exit, that should be OK for the test.

I'll drop this one, as it's not required any more.

Regards,
Simon
diff mbox series

Patch

diff --git a/cmd/boot.c b/cmd/boot.c
index 9150fce80b..c3f33a9ca3 100644
--- a/cmd/boot.c
+++ b/cmd/boot.c
@@ -56,9 +56,9 @@  U_BOOT_CMD(
 #endif
 
 U_BOOT_CMD(
-	reset, 1, 0,	do_reset,
+	reset, 2, 0,	do_reset,
 	"Perform RESET of the CPU",
-	""
+	"[<cold|warm>] - type of reboot"
 );
 
 #ifdef CONFIG_CMD_POWEROFF
diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
index ad831c703a..fbda3f44f2 100644
--- a/drivers/sysreset/sysreset-uclass.c
+++ b/drivers/sysreset/sysreset-uclass.c
@@ -111,9 +111,24 @@  void reset_cpu(ulong addr)
 
 int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
+	enum sysreset_t reboot_mode = SYSRESET_COLD;
+
+	if (argc > 1 && argv[1]) {
+		switch (*argv[1]) {
+		case 'w':
+			reboot_mode = SYSRESET_WARM;
+			printf("warm ");
+			break;
+		case 'c':
+			reboot_mode = SYSRESET_COLD;
+			printf("cold ");
+			break;
+		}
+	}
+
 	printf("resetting ...\n");
 
-	sysreset_walk_halt(SYSRESET_COLD);
+	sysreset_walk_halt(reboot_mode);
 
 	return 0;
 }