Re: [PATCH 03/10] ARM: i.MX6: Record reset reason as a part of startup
On Tue, Apr 17, 2018 at 08:35:12AM -0700, Andrey Smirnov wrote: > On Mon, Apr 16, 2018 at 11:49 PM, Sascha Hauerwrote: > > On Mon, Apr 16, 2018 at 06:28:11AM -0700, Andrey Smirnov wrote: > >> On Mon, Apr 16, 2018 at 12:13 AM, Sascha Hauer > >> wrote: > >> >> @@ -151,6 +152,7 @@ int imx6_init(void) > >> >> { > >> >> const char *cputypestr; > >> >> u32 mx6_silicon_revision; > >> >> + void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR); > >> >> > >> >> imx6_init_lowlevel(); > >> >> > >> >> @@ -195,7 +197,7 @@ int imx6_init(void) > >> >> } > >> >> > >> >> imx_set_silicon_revision(cputypestr, mx6_silicon_revision); > >> >> - > >> >> + imx_set_reset_reason(src + IMX6_SRC_SRSR); > >> > > >> > This will get overwritten by the watchdog driver if enabled. > >> > > >> > >> I am not sure I see how. Imx_watchdog_detect_reset_source() reports > >> reset sources with the same priority as this code, so > >> reset_source_set_priority() should bail out early without changing > >> anything in that case. > > > > I wasn't aware there is a priority mechanism involved. Indeed the > > behaviour seems to be correct. Maybe we should even higher the priority > > of imx_set_reset_reason()? This information seems the more accurate one, > > so it should be used, and we shouldn't depend on the order of execution. > > > > I think if I add support for "reset-source-priority" to imxwd.c and > set it up to use the value less then RESET_SOURCE_DEFAULT_PRIORITY by > default we can get both what you describe and allow users to override > the behavior and prioritize reset source provided by watchdog driver. > > Let me know if this sounds like a bad idea. It doesn't sound like a bad idea, I'm just not convinced it's a good idea ;) Reading the SRC registers seems to give us more accurate informations than reading the watchdog. Provided this is true I don't see a reason to make the priority configurable. If it was only for i.MX6/7 I would say, we could remove the reset source detection from the watchdog driver entirely, but we need the informations from the driver on older SoCs. Anyway, before making this configurable I think it's better to wait until somebody to show up who really needs this (and thus can explain the reasons), rather than to just add it in case it might be useful. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 03/10] ARM: i.MX6: Record reset reason as a part of startup
On Mon, Apr 16, 2018 at 11:49 PM, Sascha Hauerwrote: > On Mon, Apr 16, 2018 at 06:28:11AM -0700, Andrey Smirnov wrote: >> On Mon, Apr 16, 2018 at 12:13 AM, Sascha Hauer >> wrote: >> > Hi Andrey, >> > >> > On Sat, Apr 14, 2018 at 10:50:17AM -0700, Andrey Smirnov wrote: >> >> Signed-off-by: Andrey Smirnov >> >> --- >> >> arch/arm/mach-imx/imx6.c | 4 +++- >> >> arch/arm/mach-imx/include/mach/reset-reason.h | 2 ++ >> >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/arch/arm/mach-imx/imx6.c b/arch/arm/mach-imx/imx6.c >> >> index 14a1cba5a..3d81c2785 100644 >> >> --- a/arch/arm/mach-imx/imx6.c >> >> +++ b/arch/arm/mach-imx/imx6.c >> >> @@ -19,6 +19,7 @@ >> >> #include >> >> #include >> >> #include >> >> +#include >> >> #include >> >> #include >> >> #include >> >> @@ -151,6 +152,7 @@ int imx6_init(void) >> >> { >> >> const char *cputypestr; >> >> u32 mx6_silicon_revision; >> >> + void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR); >> >> >> >> imx6_init_lowlevel(); >> >> >> >> @@ -195,7 +197,7 @@ int imx6_init(void) >> >> } >> >> >> >> imx_set_silicon_revision(cputypestr, mx6_silicon_revision); >> >> - >> >> + imx_set_reset_reason(src + IMX6_SRC_SRSR); >> > >> > This will get overwritten by the watchdog driver if enabled. >> > >> >> I am not sure I see how. Imx_watchdog_detect_reset_source() reports >> reset sources with the same priority as this code, so >> reset_source_set_priority() should bail out early without changing >> anything in that case. > > I wasn't aware there is a priority mechanism involved. Indeed the > behaviour seems to be correct. Maybe we should even higher the priority > of imx_set_reset_reason()? This information seems the more accurate one, > so it should be used, and we shouldn't depend on the order of execution. > I think if I add support for "reset-source-priority" to imxwd.c and set it up to use the value less then RESET_SOURCE_DEFAULT_PRIORITY by default we can get both what you describe and allow users to override the behavior and prioritize reset source provided by watchdog driver. Let me know if this sounds like a bad idea. Thanks, Andrey Smirnov ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 03/10] ARM: i.MX6: Record reset reason as a part of startup
On Mon, Apr 16, 2018 at 06:28:11AM -0700, Andrey Smirnov wrote: > On Mon, Apr 16, 2018 at 12:13 AM, Sascha Hauerwrote: > > Hi Andrey, > > > > On Sat, Apr 14, 2018 at 10:50:17AM -0700, Andrey Smirnov wrote: > >> Signed-off-by: Andrey Smirnov > >> --- > >> arch/arm/mach-imx/imx6.c | 4 +++- > >> arch/arm/mach-imx/include/mach/reset-reason.h | 2 ++ > >> 2 files changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/mach-imx/imx6.c b/arch/arm/mach-imx/imx6.c > >> index 14a1cba5a..3d81c2785 100644 > >> --- a/arch/arm/mach-imx/imx6.c > >> +++ b/arch/arm/mach-imx/imx6.c > >> @@ -19,6 +19,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> #include > >> #include > >> @@ -151,6 +152,7 @@ int imx6_init(void) > >> { > >> const char *cputypestr; > >> u32 mx6_silicon_revision; > >> + void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR); > >> > >> imx6_init_lowlevel(); > >> > >> @@ -195,7 +197,7 @@ int imx6_init(void) > >> } > >> > >> imx_set_silicon_revision(cputypestr, mx6_silicon_revision); > >> - > >> + imx_set_reset_reason(src + IMX6_SRC_SRSR); > > > > This will get overwritten by the watchdog driver if enabled. > > > > I am not sure I see how. Imx_watchdog_detect_reset_source() reports > reset sources with the same priority as this code, so > reset_source_set_priority() should bail out early without changing > anything in that case. I wasn't aware there is a priority mechanism involved. Indeed the behaviour seems to be correct. Maybe we should even higher the priority of imx_set_reset_reason()? This information seems the more accurate one, so it should be used, and we shouldn't depend on the order of execution. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 03/10] ARM: i.MX6: Record reset reason as a part of startup
On Mon, Apr 16, 2018 at 12:13 AM, Sascha Hauerwrote: > Hi Andrey, > > On Sat, Apr 14, 2018 at 10:50:17AM -0700, Andrey Smirnov wrote: >> Signed-off-by: Andrey Smirnov >> --- >> arch/arm/mach-imx/imx6.c | 4 +++- >> arch/arm/mach-imx/include/mach/reset-reason.h | 2 ++ >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-imx/imx6.c b/arch/arm/mach-imx/imx6.c >> index 14a1cba5a..3d81c2785 100644 >> --- a/arch/arm/mach-imx/imx6.c >> +++ b/arch/arm/mach-imx/imx6.c >> @@ -19,6 +19,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -151,6 +152,7 @@ int imx6_init(void) >> { >> const char *cputypestr; >> u32 mx6_silicon_revision; >> + void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR); >> >> imx6_init_lowlevel(); >> >> @@ -195,7 +197,7 @@ int imx6_init(void) >> } >> >> imx_set_silicon_revision(cputypestr, mx6_silicon_revision); >> - >> + imx_set_reset_reason(src + IMX6_SRC_SRSR); > > This will get overwritten by the watchdog driver if enabled. > I am not sure I see how. Imx_watchdog_detect_reset_source() reports reset sources with the same priority as this code, so reset_source_set_priority() should bail out early without changing anything in that case. Regardless, even if it were true, I think that behavior is perfectly OK. First of all, startup log will still be present, which is IMHO pretty useful. Second, the platforms that doen't use/rely on built-in watchdog would benefit from this. Another thing we can do is add code to respect "reset-source-priority" to imxwd.c, so that people could ajdust what reset source info they want to see via DT. Let me know what you think. Thanks, Andrey Smirnov ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 03/10] ARM: i.MX6: Record reset reason as a part of startup
Hi Andrey, On Sat, Apr 14, 2018 at 10:50:17AM -0700, Andrey Smirnov wrote: > Signed-off-by: Andrey Smirnov> --- > arch/arm/mach-imx/imx6.c | 4 +++- > arch/arm/mach-imx/include/mach/reset-reason.h | 2 ++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-imx/imx6.c b/arch/arm/mach-imx/imx6.c > index 14a1cba5a..3d81c2785 100644 > --- a/arch/arm/mach-imx/imx6.c > +++ b/arch/arm/mach-imx/imx6.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -151,6 +152,7 @@ int imx6_init(void) > { > const char *cputypestr; > u32 mx6_silicon_revision; > + void __iomem *src = IOMEM(MX6_SRC_BASE_ADDR); > > imx6_init_lowlevel(); > > @@ -195,7 +197,7 @@ int imx6_init(void) > } > > imx_set_silicon_revision(cputypestr, mx6_silicon_revision); > - > + imx_set_reset_reason(src + IMX6_SRC_SRSR); This will get overwritten by the watchdog driver if enabled. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox