Re: [U-Boot] [PATCH 4/4] x86: Switch to use DM sysreset driver
Hi Andy,
On Tue, Jul 3, 2018 at 6:42 PM, Andy Shevchenko
wrote:
> On Tue, 2018-07-03 at 02:48 -0700, Bin Meng wrote:
>> This converts all x86 boards over to DM sysreset.
>
>> -int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>> argv[])
>> -{
>> - printf("resetting ...\n");
>> -
>> - /* wait 50 ms */
>> - udelay(5);
>> - disable_interrupts();
>> - reset_cpu(0);
>
>> -}
>
>> --- a/arch/x86/cpu/tangier/tangier.c
>> +++ b/arch/x86/cpu/tangier/tangier.c
>> @@ -25,7 +25,15 @@ int print_cpuinfo(void)
>> return default_print_cpuinfo();
>> }
>>
>> +/* TODO: convert to DM sysreset */
>> void reset_cpu(ulong addr)
>> {
>> scu_ipc_simple_command(IPCMSG_COLD_RESET, 0);
>> }
>> +
>> +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>> argv[])
>> +{
>> + reset_cpu(0);
>> +
>> + return 0;
>
> This is not equivalent to the above.
>
> First of all, in some cases would be good to have at least a debug
> message that we got into do_reset().
>
> Second, I didn't test if udelay() + disable_interrupts() make any
> difference. So, I would leave them for now.
>
OK, will leave them in v2.
>> +}
>
>> diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts
>> index 9033532..a1d3c90 100644
>> --- a/arch/x86/dts/edison.dts
>> +++ b/arch/x86/dts/edison.dts
>> @@ -9,6 +9,7 @@
>> #include
>>
>> /include/ "skeleton.dtsi"
>
>> +/include/ "reset.dtsi"
>
> If i read this right we are not using generic reset sequence.
> Why do we include this here?
>
This is a mistake. Will fix in v2.
BTW: do you have time to convert the tangier SoC reset driver to DM?
>> /include/ "rtc.dtsi"
>> /include/ "tsc_timer.dtsi"
>
>> --- a/configs/edison_defconfig
>> +++ b/configs/edison_defconfig
>
>> +# CONFIG_SYSRESET is not set
>
> --
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] x86: Switch to use DM sysreset driver
Dear Andy,
In message <18255ae55e4faa5733fb8e93dc8ebbc1559a3694.ca...@linux.intel.com> you
wrote:
...
> > This converts all x86 boards over to DM sysreset.
>
> > -int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > argv[])
> > -{
> > - printf("resetting ...\n");
> > -
> > - /* wait 50 ms */
> > - udelay(5);
I am willing to bet that the delay here is a dirty surrogate for
flushing/closing the standard output channel, i. e. we wait until
the characters have actually been set over the serial console.
Don't we have a way to close() the device in DM (remove ?) ?
> First of all, in some cases would be good to have at least a debug
> message that we got into do_reset().
This should not only be a debug message, but a standard message;
actually even something that goes to STDERR.
> Second, I didn't test if udelay() + disable_interrupts() make any
> difference. So, I would leave them for now.
See above... I think the delay should be replaced by the proper way
to close the device in DM (remove?).
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
But the only way of discovering the limits of the possible is to
venture a little way past them into the impossible.
- _Profiles of the Future_ (1962; rev. 1973)
``Hazards of Prophecy: The Failure of Imagination''
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 4/4] x86: Switch to use DM sysreset driver
On Tue, 2018-07-03 at 02:48 -0700, Bin Meng wrote:
> This converts all x86 boards over to DM sysreset.
> -int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
> -{
> - printf("resetting ...\n");
> -
> - /* wait 50 ms */
> - udelay(5);
> - disable_interrupts();
> - reset_cpu(0);
> -}
> --- a/arch/x86/cpu/tangier/tangier.c
> +++ b/arch/x86/cpu/tangier/tangier.c
> @@ -25,7 +25,15 @@ int print_cpuinfo(void)
> return default_print_cpuinfo();
> }
>
> +/* TODO: convert to DM sysreset */
> void reset_cpu(ulong addr)
> {
> scu_ipc_simple_command(IPCMSG_COLD_RESET, 0);
> }
> +
> +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
> +{
> + reset_cpu(0);
> +
> + return 0;
This is not equivalent to the above.
First of all, in some cases would be good to have at least a debug
message that we got into do_reset().
Second, I didn't test if udelay() + disable_interrupts() make any
difference. So, I would leave them for now.
> +}
> diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts
> index 9033532..a1d3c90 100644
> --- a/arch/x86/dts/edison.dts
> +++ b/arch/x86/dts/edison.dts
> @@ -9,6 +9,7 @@
> #include
>
> /include/ "skeleton.dtsi"
> +/include/ "reset.dtsi"
If i read this right we are not using generic reset sequence.
Why do we include this here?
> /include/ "rtc.dtsi"
> /include/ "tsc_timer.dtsi"
> --- a/configs/edison_defconfig
> +++ b/configs/edison_defconfig
> +# CONFIG_SYSRESET is not set
--
Andy Shevchenko
Intel Finland Oy
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
