| Message ID | 20211109161402.23111-1-kabel@kernel.org |
|---|---|
| State | Accepted |
| Commit | 10c3befc682d8be2f5200db31333d88b4ac16d8c |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | [u-boot-marvell] arm: mvebu: turris_omnia: enable A385 watchdog before disabling MCU watchdog | expand |
On 11/9/21 17:14, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > Commit aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in > SPL when booting over UART") disabled MCU watchdog when booting over > UART to ensure that watchdog does not reboot the board before UART > transfer finishes. > > But if UART transfer fails for some reason, or if U-Boot binary crashes, > then board hangs forever as there is no watchdog running which could > reset it. > > To fix this issue, enable A385 watchdog with very high timeout before > disabling MCU watchdog to ensure that even slow transfer can finish > successfully before watchdog timer expires and also to ensure that if > board hangs for some reason, watchdog will reset it. > > Omnia's MCU watchdog has fixed 120 seconds timer and it cannot be > changed (without updating MCU firmware). A385 watchdog by default uses > 25 MHz input clock and so the largest timeout value (2^32-1) can be > just 171 seconds. But A385 watchdog can be switched to use NBCLK (L2) as > input clock (on Turris Omnia it is 800 MHz clock) and in this case final > watchdog clock frequency is calculated as: > > freq = NBCLK / 2 / (2 ^ R) > > So A385 watchdog on Turris Omnia can be configured to at most 1374 > seconds (about 22 minutes). We set it to 10 minutes, which should be > enough even for bigger U-Boot binaries or slower UART transfers. > > Both U-Boot and Linux kernel, when initializing A385 watchdog, switch > watchdog timer to 25 MHz input clock, so usage of NBCLK input clock in > U-Boot SPL does not cause any issues. > > Fixes: aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART") > Signed-off-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Marek Behún <marek.behun@nic.cz> I'm wondering if it makes sense to add this "slow" watchdog support to the common watchdog driver instead (orion_wdt.c). So that it dynamically configures the needed input clock for the desired timeout. Would this be possible? Thanks, Stefan > --- > board/CZ.NIC/turris_omnia/turris_omnia.c | 65 +++++++++++++++++++++++- > 1 file changed, 63 insertions(+), 2 deletions(-) > > diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c > index 36c596efc5..ae24d14b76 100644 > --- a/board/CZ.NIC/turris_omnia/turris_omnia.c > +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c > @@ -43,6 +43,23 @@ DECLARE_GLOBAL_DATA_PTR; > #define OMNIA_I2C_EEPROM_CHIP_LEN 2 > #define OMNIA_I2C_EEPROM_MAGIC 0x0341a034 > > +#define SYS_RSTOUT_MASK MVEBU_REGISTER(0x18260) > +#define SYS_RSTOUT_MASK_WD BIT(10) > + > +#define A385_WDT_GLOBAL_CTRL MVEBU_REGISTER(0x20300) > +#define A385_WDT_GLOBAL_RATIO_MASK GENMASK(18, 16) > +#define A385_WDT_GLOBAL_RATIO_SHIFT 16 > +#define A385_WDT_GLOBAL_25MHZ BIT(10) > +#define A385_WDT_GLOBAL_ENABLE BIT(8) > + > +#define A385_WDT_GLOBAL_STATUS MVEBU_REGISTER(0x20304) > +#define A385_WDT_GLOBAL_EXPIRED BIT(31) > + > +#define A385_WDT_DURATION MVEBU_REGISTER(0x20334) > + > +#define A385_WD_RSTOUT_UNMASK MVEBU_REGISTER(0x20704) > +#define A385_WD_RSTOUT_UNMASK_GLOBAL BIT(8) > + > enum mcu_commands { > CMD_GET_STATUS_WORD = 0x01, > CMD_GET_RESET = 0x09, > @@ -141,6 +158,47 @@ static int omnia_mcu_write(u8 cmd, const void *buf, int len) > return dm_i2c_write(chip, cmd, buf, len); > } > > +static void enable_a385_watchdog(unsigned int timeout_minutes) > +{ > + struct sar_freq_modes sar_freq; > + u32 watchdog_freq; > + > + printf("Enabling A385 watchdog with %u minutes timeout...\n", > + timeout_minutes); > + > + /* > + * Use NBCLK clock (a.k.a. L2 clock) as watchdog input clock with > + * its maximal ratio 7 instead of default fixed 25 MHz clock. > + * It allows to set watchdog duration up to the 22 minutes. > + */ > + clrsetbits_32(A385_WDT_GLOBAL_CTRL, > + A385_WDT_GLOBAL_25MHZ | A385_WDT_GLOBAL_RATIO_MASK, > + 7 << A385_WDT_GLOBAL_RATIO_SHIFT); > + > + /* > + * Calculate watchdog clock frequency. It is defined by formula: > + * freq = NBCLK / 2 / (2 ^ ratio) > + * We set ratio to the maximal possible value 7. > + */ > + get_sar_freq(&sar_freq); > + watchdog_freq = sar_freq.nb_clk * 1000000 / 2 / (1 << 7); > + > + /* Set watchdog duration */ > + writel(timeout_minutes * 60 * watchdog_freq, A385_WDT_DURATION); > + > + /* Clear the watchdog expiration bit */ > + clrbits_32(A385_WDT_GLOBAL_STATUS, A385_WDT_GLOBAL_EXPIRED); > + > + /* Enable watchdog timer */ > + setbits_32(A385_WDT_GLOBAL_CTRL, A385_WDT_GLOBAL_ENABLE); > + > + /* Enable reset on watchdog */ > + setbits_32(A385_WD_RSTOUT_UNMASK, A385_WD_RSTOUT_UNMASK_GLOBAL); > + > + /* Unmask reset for watchdog */ > + clrbits_32(SYS_RSTOUT_MASK, SYS_RSTOUT_MASK_WD); > +} > + > static bool disable_mcu_watchdog(void) > { > int ret; > @@ -423,10 +481,13 @@ void spl_board_init(void) > { > /* > * If booting from UART, disable MCU watchdog in SPL, since uploading > - * U-Boot proper can take too much time and trigger it. > + * U-Boot proper can take too much time and trigger it. Instead enable > + * A385 watchdog with very high timeout (10 minutes) to prevent hangup. > */ > - if (get_boot_device() == BOOT_DEVICE_UART) > + if (get_boot_device() == BOOT_DEVICE_UART) { > + enable_a385_watchdog(10); > disable_mcu_watchdog(); > + } > } > > int board_init(void) > Viele Grüße, Stefan Roese
On Friday 12 November 2021 14:44:15 Stefan Roese wrote: > On 11/9/21 17:14, Marek Behún wrote: > > From: Pali Rohár <pali@kernel.org> > > > > Commit aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in > > SPL when booting over UART") disabled MCU watchdog when booting over > > UART to ensure that watchdog does not reboot the board before UART > > transfer finishes. > > > > But if UART transfer fails for some reason, or if U-Boot binary crashes, > > then board hangs forever as there is no watchdog running which could > > reset it. > > > > To fix this issue, enable A385 watchdog with very high timeout before > > disabling MCU watchdog to ensure that even slow transfer can finish > > successfully before watchdog timer expires and also to ensure that if > > board hangs for some reason, watchdog will reset it. > > > > Omnia's MCU watchdog has fixed 120 seconds timer and it cannot be > > changed (without updating MCU firmware). A385 watchdog by default uses > > 25 MHz input clock and so the largest timeout value (2^32-1) can be > > just 171 seconds. But A385 watchdog can be switched to use NBCLK (L2) as > > input clock (on Turris Omnia it is 800 MHz clock) and in this case final > > watchdog clock frequency is calculated as: > > > > freq = NBCLK / 2 / (2 ^ R) > > > > So A385 watchdog on Turris Omnia can be configured to at most 1374 > > seconds (about 22 minutes). We set it to 10 minutes, which should be > > enough even for bigger U-Boot binaries or slower UART transfers. > > > > Both U-Boot and Linux kernel, when initializing A385 watchdog, switch > > watchdog timer to 25 MHz input clock, so usage of NBCLK input clock in > > U-Boot SPL does not cause any issues. > > > > Fixes: aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART") > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > I'm wondering if it makes sense to add this "slow" watchdog support to > the common watchdog driver instead (orion_wdt.c). So that it dynamically > configures the needed input clock for the desired timeout. > > Would this be possible? It should be possible to extend orion_wdt.c driver to add support for L2/NBCLK as a clock source. Currently neither kernel nor U-Boot has this support. There is an issue how to get this clock property in watchdog driver as it should be described in DTS. And name "orion" says that this driver is compatible back to the old Marvell Orion products. I have no idea how this "slow" clock source is supported on older products. But I think that the main issue here is how the watchdog in this board code is used. It is there just to start it and letting it as is. No period kicking, no other actions. It is activated here only to ensure that when U-Boot SPL returns to bootrom, watchdog is activated and resets board in case bootrom fails to load proper U-Boot. So for using it, following steps are needed: * extend U-Boot watchdog code to allow initializing watchdog drivers without periodic reset, but allowing to start them with specific timeout * extend that code, so there can be different configurations per different watchdogs and different between SPL and proper U-Boot * figure out how can be that watchdog configured on non-A385 platforms * extend DTS structure in a way that be used also in Linux kernel and figure out how to specify correct clock in DTS I think that this is too much work, just for one specific usage for one board. I do not see how can be such long time period (20 minutes) used as watchdog timeout on other boards. Is there any use case for other board? If not, I think it is overkill to implement it, if there is no benefit from it. I think that watchdog timeout is generally set to 60s. > Thanks, > Stefan > > > --- > > board/CZ.NIC/turris_omnia/turris_omnia.c | 65 +++++++++++++++++++++++- > > 1 file changed, 63 insertions(+), 2 deletions(-) > > > > diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c > > index 36c596efc5..ae24d14b76 100644 > > --- a/board/CZ.NIC/turris_omnia/turris_omnia.c > > +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c > > @@ -43,6 +43,23 @@ DECLARE_GLOBAL_DATA_PTR; > > #define OMNIA_I2C_EEPROM_CHIP_LEN 2 > > #define OMNIA_I2C_EEPROM_MAGIC 0x0341a034 > > +#define SYS_RSTOUT_MASK MVEBU_REGISTER(0x18260) > > +#define SYS_RSTOUT_MASK_WD BIT(10) > > + > > +#define A385_WDT_GLOBAL_CTRL MVEBU_REGISTER(0x20300) > > +#define A385_WDT_GLOBAL_RATIO_MASK GENMASK(18, 16) > > +#define A385_WDT_GLOBAL_RATIO_SHIFT 16 > > +#define A385_WDT_GLOBAL_25MHZ BIT(10) > > +#define A385_WDT_GLOBAL_ENABLE BIT(8) > > + > > +#define A385_WDT_GLOBAL_STATUS MVEBU_REGISTER(0x20304) > > +#define A385_WDT_GLOBAL_EXPIRED BIT(31) > > + > > +#define A385_WDT_DURATION MVEBU_REGISTER(0x20334) > > + > > +#define A385_WD_RSTOUT_UNMASK MVEBU_REGISTER(0x20704) > > +#define A385_WD_RSTOUT_UNMASK_GLOBAL BIT(8) > > + > > enum mcu_commands { > > CMD_GET_STATUS_WORD = 0x01, > > CMD_GET_RESET = 0x09, > > @@ -141,6 +158,47 @@ static int omnia_mcu_write(u8 cmd, const void *buf, int len) > > return dm_i2c_write(chip, cmd, buf, len); > > } > > +static void enable_a385_watchdog(unsigned int timeout_minutes) > > +{ > > + struct sar_freq_modes sar_freq; > > + u32 watchdog_freq; > > + > > + printf("Enabling A385 watchdog with %u minutes timeout...\n", > > + timeout_minutes); > > + > > + /* > > + * Use NBCLK clock (a.k.a. L2 clock) as watchdog input clock with > > + * its maximal ratio 7 instead of default fixed 25 MHz clock. > > + * It allows to set watchdog duration up to the 22 minutes. > > + */ > > + clrsetbits_32(A385_WDT_GLOBAL_CTRL, > > + A385_WDT_GLOBAL_25MHZ | A385_WDT_GLOBAL_RATIO_MASK, > > + 7 << A385_WDT_GLOBAL_RATIO_SHIFT); > > + > > + /* > > + * Calculate watchdog clock frequency. It is defined by formula: > > + * freq = NBCLK / 2 / (2 ^ ratio) > > + * We set ratio to the maximal possible value 7. > > + */ > > + get_sar_freq(&sar_freq); > > + watchdog_freq = sar_freq.nb_clk * 1000000 / 2 / (1 << 7); > > + > > + /* Set watchdog duration */ > > + writel(timeout_minutes * 60 * watchdog_freq, A385_WDT_DURATION); > > + > > + /* Clear the watchdog expiration bit */ > > + clrbits_32(A385_WDT_GLOBAL_STATUS, A385_WDT_GLOBAL_EXPIRED); > > + > > + /* Enable watchdog timer */ > > + setbits_32(A385_WDT_GLOBAL_CTRL, A385_WDT_GLOBAL_ENABLE); > > + > > + /* Enable reset on watchdog */ > > + setbits_32(A385_WD_RSTOUT_UNMASK, A385_WD_RSTOUT_UNMASK_GLOBAL); > > + > > + /* Unmask reset for watchdog */ > > + clrbits_32(SYS_RSTOUT_MASK, SYS_RSTOUT_MASK_WD); > > +} > > + > > static bool disable_mcu_watchdog(void) > > { > > int ret; > > @@ -423,10 +481,13 @@ void spl_board_init(void) > > { > > /* > > * If booting from UART, disable MCU watchdog in SPL, since uploading > > - * U-Boot proper can take too much time and trigger it. > > + * U-Boot proper can take too much time and trigger it. Instead enable > > + * A385 watchdog with very high timeout (10 minutes) to prevent hangup. > > */ > > - if (get_boot_device() == BOOT_DEVICE_UART) > > + if (get_boot_device() == BOOT_DEVICE_UART) { > > + enable_a385_watchdog(10); > > disable_mcu_watchdog(); > > + } > > } > > int board_init(void) > > > > Viele Grüße, > Stefan Roese > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
On 11/12/21 16:23, Pali Rohár wrote: > On Friday 12 November 2021 14:44:15 Stefan Roese wrote: >> On 11/9/21 17:14, Marek Behún wrote: >>> From: Pali Rohár <pali@kernel.org> >>> >>> Commit aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in >>> SPL when booting over UART") disabled MCU watchdog when booting over >>> UART to ensure that watchdog does not reboot the board before UART >>> transfer finishes. >>> >>> But if UART transfer fails for some reason, or if U-Boot binary crashes, >>> then board hangs forever as there is no watchdog running which could >>> reset it. >>> >>> To fix this issue, enable A385 watchdog with very high timeout before >>> disabling MCU watchdog to ensure that even slow transfer can finish >>> successfully before watchdog timer expires and also to ensure that if >>> board hangs for some reason, watchdog will reset it. >>> >>> Omnia's MCU watchdog has fixed 120 seconds timer and it cannot be >>> changed (without updating MCU firmware). A385 watchdog by default uses >>> 25 MHz input clock and so the largest timeout value (2^32-1) can be >>> just 171 seconds. But A385 watchdog can be switched to use NBCLK (L2) as >>> input clock (on Turris Omnia it is 800 MHz clock) and in this case final >>> watchdog clock frequency is calculated as: >>> >>> freq = NBCLK / 2 / (2 ^ R) >>> >>> So A385 watchdog on Turris Omnia can be configured to at most 1374 >>> seconds (about 22 minutes). We set it to 10 minutes, which should be >>> enough even for bigger U-Boot binaries or slower UART transfers. >>> >>> Both U-Boot and Linux kernel, when initializing A385 watchdog, switch >>> watchdog timer to 25 MHz input clock, so usage of NBCLK input clock in >>> U-Boot SPL does not cause any issues. >>> >>> Fixes: aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART") >>> Signed-off-by: Pali Rohár <pali@kernel.org> >>> Signed-off-by: Marek Behún <marek.behun@nic.cz> >> >> I'm wondering if it makes sense to add this "slow" watchdog support to >> the common watchdog driver instead (orion_wdt.c). So that it dynamically >> configures the needed input clock for the desired timeout. >> >> Would this be possible? > > It should be possible to extend orion_wdt.c driver to add support for > L2/NBCLK as a clock source. Currently neither kernel nor U-Boot has this > support. There is an issue how to get this clock property in watchdog > driver as it should be described in DTS. And name "orion" says that this > driver is compatible back to the old Marvell Orion products. I have no > idea how this "slow" clock source is supported on older products. > > But I think that the main issue here is how the watchdog in this board > code is used. It is there just to start it and letting it as is. No > period kicking, no other actions. > > It is activated here only to ensure that when U-Boot SPL returns to > bootrom, watchdog is activated and resets board in case bootrom fails to > load proper U-Boot. > > So for using it, following steps are needed: > * extend U-Boot watchdog code to allow initializing watchdog drivers > without periodic reset, but allowing to start them with specific > timeout > * extend that code, so there can be different configurations per > different watchdogs and different between SPL and proper U-Boot > * figure out how can be that watchdog configured on non-A385 platforms > * extend DTS structure in a way that be used also in Linux kernel > and figure out how to specify correct clock in DTS > > I think that this is too much work, just for one specific usage for one > board. I do not see how can be such long time period (20 minutes) used > as watchdog timeout on other boards. Is there any use case for other > board? If not, I think it is overkill to implement it, if there is no > benefit from it. I think that watchdog timeout is generally set to 60s. This was my feeling as well. But I wanted to be sure here, before adding this code to one board only, that would "look better" in the watchdog driver (no defined for the reg offsets etc). But if this will never be used in other board, of which I'm pretty sure of, then let's stick with this current board specific implementation. Especially if it's also quite complex to configure this specific setup in the common WDT driver. Thanks anyways for looking into it, Stefan >> Thanks, >> Stefan >> >>> --- >>> board/CZ.NIC/turris_omnia/turris_omnia.c | 65 +++++++++++++++++++++++- >>> 1 file changed, 63 insertions(+), 2 deletions(-) >>> >>> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c >>> index 36c596efc5..ae24d14b76 100644 >>> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c >>> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c >>> @@ -43,6 +43,23 @@ DECLARE_GLOBAL_DATA_PTR; >>> #define OMNIA_I2C_EEPROM_CHIP_LEN 2 >>> #define OMNIA_I2C_EEPROM_MAGIC 0x0341a034 >>> +#define SYS_RSTOUT_MASK MVEBU_REGISTER(0x18260) >>> +#define SYS_RSTOUT_MASK_WD BIT(10) >>> + >>> +#define A385_WDT_GLOBAL_CTRL MVEBU_REGISTER(0x20300) >>> +#define A385_WDT_GLOBAL_RATIO_MASK GENMASK(18, 16) >>> +#define A385_WDT_GLOBAL_RATIO_SHIFT 16 >>> +#define A385_WDT_GLOBAL_25MHZ BIT(10) >>> +#define A385_WDT_GLOBAL_ENABLE BIT(8) >>> + >>> +#define A385_WDT_GLOBAL_STATUS MVEBU_REGISTER(0x20304) >>> +#define A385_WDT_GLOBAL_EXPIRED BIT(31) >>> + >>> +#define A385_WDT_DURATION MVEBU_REGISTER(0x20334) >>> + >>> +#define A385_WD_RSTOUT_UNMASK MVEBU_REGISTER(0x20704) >>> +#define A385_WD_RSTOUT_UNMASK_GLOBAL BIT(8) >>> + >>> enum mcu_commands { >>> CMD_GET_STATUS_WORD = 0x01, >>> CMD_GET_RESET = 0x09, >>> @@ -141,6 +158,47 @@ static int omnia_mcu_write(u8 cmd, const void *buf, int len) >>> return dm_i2c_write(chip, cmd, buf, len); >>> } >>> +static void enable_a385_watchdog(unsigned int timeout_minutes) >>> +{ >>> + struct sar_freq_modes sar_freq; >>> + u32 watchdog_freq; >>> + >>> + printf("Enabling A385 watchdog with %u minutes timeout...\n", >>> + timeout_minutes); >>> + >>> + /* >>> + * Use NBCLK clock (a.k.a. L2 clock) as watchdog input clock with >>> + * its maximal ratio 7 instead of default fixed 25 MHz clock. >>> + * It allows to set watchdog duration up to the 22 minutes. >>> + */ >>> + clrsetbits_32(A385_WDT_GLOBAL_CTRL, >>> + A385_WDT_GLOBAL_25MHZ | A385_WDT_GLOBAL_RATIO_MASK, >>> + 7 << A385_WDT_GLOBAL_RATIO_SHIFT); >>> + >>> + /* >>> + * Calculate watchdog clock frequency. It is defined by formula: >>> + * freq = NBCLK / 2 / (2 ^ ratio) >>> + * We set ratio to the maximal possible value 7. >>> + */ >>> + get_sar_freq(&sar_freq); >>> + watchdog_freq = sar_freq.nb_clk * 1000000 / 2 / (1 << 7); >>> + >>> + /* Set watchdog duration */ >>> + writel(timeout_minutes * 60 * watchdog_freq, A385_WDT_DURATION); >>> + >>> + /* Clear the watchdog expiration bit */ >>> + clrbits_32(A385_WDT_GLOBAL_STATUS, A385_WDT_GLOBAL_EXPIRED); >>> + >>> + /* Enable watchdog timer */ >>> + setbits_32(A385_WDT_GLOBAL_CTRL, A385_WDT_GLOBAL_ENABLE); >>> + >>> + /* Enable reset on watchdog */ >>> + setbits_32(A385_WD_RSTOUT_UNMASK, A385_WD_RSTOUT_UNMASK_GLOBAL); >>> + >>> + /* Unmask reset for watchdog */ >>> + clrbits_32(SYS_RSTOUT_MASK, SYS_RSTOUT_MASK_WD); >>> +} >>> + >>> static bool disable_mcu_watchdog(void) >>> { >>> int ret; >>> @@ -423,10 +481,13 @@ void spl_board_init(void) >>> { >>> /* >>> * If booting from UART, disable MCU watchdog in SPL, since uploading >>> - * U-Boot proper can take too much time and trigger it. >>> + * U-Boot proper can take too much time and trigger it. Instead enable >>> + * A385 watchdog with very high timeout (10 minutes) to prevent hangup. >>> */ >>> - if (get_boot_device() == BOOT_DEVICE_UART) >>> + if (get_boot_device() == BOOT_DEVICE_UART) { >>> + enable_a385_watchdog(10); >>> disable_mcu_watchdog(); >>> + } >>> } >>> int board_init(void) >>> >> >> Viele Grüße, >> Stefan Roese >> >> -- >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de Viele Grüße, Stefan Roese
On Fri, Nov 12, 2021 at 02:44:15PM +0100, Stefan Roese wrote: > On 11/9/21 17:14, Marek Behún wrote: > > From: Pali Rohár <pali@kernel.org> > > > > Commit aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in > > SPL when booting over UART") disabled MCU watchdog when booting over > > UART to ensure that watchdog does not reboot the board before UART > > transfer finishes. > > > > But if UART transfer fails for some reason, or if U-Boot binary crashes, > > then board hangs forever as there is no watchdog running which could > > reset it. > > > > To fix this issue, enable A385 watchdog with very high timeout before > > disabling MCU watchdog to ensure that even slow transfer can finish > > successfully before watchdog timer expires and also to ensure that if > > board hangs for some reason, watchdog will reset it. > > > > Omnia's MCU watchdog has fixed 120 seconds timer and it cannot be > > changed (without updating MCU firmware). A385 watchdog by default uses > > 25 MHz input clock and so the largest timeout value (2^32-1) can be > > just 171 seconds. But A385 watchdog can be switched to use NBCLK (L2) as > > input clock (on Turris Omnia it is 800 MHz clock) and in this case final > > watchdog clock frequency is calculated as: > > > > freq = NBCLK / 2 / (2 ^ R) > > > > So A385 watchdog on Turris Omnia can be configured to at most 1374 > > seconds (about 22 minutes). We set it to 10 minutes, which should be > > enough even for bigger U-Boot binaries or slower UART transfers. > > > > Both U-Boot and Linux kernel, when initializing A385 watchdog, switch > > watchdog timer to 25 MHz input clock, so usage of NBCLK input clock in > > U-Boot SPL does not cause any issues. > > > > Fixes: aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART") > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > I'm wondering if it makes sense to add this "slow" watchdog support to > the common watchdog driver instead (orion_wdt.c). So that it dynamically > configures the needed input clock for the desired timeout. > > Would this be possible? I know I'm late to the thread here, but why can't / aren't we servicing the 120s watchdog during UART loading? Or have I gone crazy and we don't do that on say OMAP?
On Friday 12 November 2021 11:31:38 Tom Rini wrote: > On Fri, Nov 12, 2021 at 02:44:15PM +0100, Stefan Roese wrote: > > On 11/9/21 17:14, Marek Behún wrote: > > > From: Pali Rohár <pali@kernel.org> > > > > > > Commit aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in > > > SPL when booting over UART") disabled MCU watchdog when booting over > > > UART to ensure that watchdog does not reboot the board before UART > > > transfer finishes. > > > > > > But if UART transfer fails for some reason, or if U-Boot binary crashes, > > > then board hangs forever as there is no watchdog running which could > > > reset it. > > > > > > To fix this issue, enable A385 watchdog with very high timeout before > > > disabling MCU watchdog to ensure that even slow transfer can finish > > > successfully before watchdog timer expires and also to ensure that if > > > board hangs for some reason, watchdog will reset it. > > > > > > Omnia's MCU watchdog has fixed 120 seconds timer and it cannot be > > > changed (without updating MCU firmware). A385 watchdog by default uses > > > 25 MHz input clock and so the largest timeout value (2^32-1) can be > > > just 171 seconds. But A385 watchdog can be switched to use NBCLK (L2) as > > > input clock (on Turris Omnia it is 800 MHz clock) and in this case final > > > watchdog clock frequency is calculated as: > > > > > > freq = NBCLK / 2 / (2 ^ R) > > > > > > So A385 watchdog on Turris Omnia can be configured to at most 1374 > > > seconds (about 22 minutes). We set it to 10 minutes, which should be > > > enough even for bigger U-Boot binaries or slower UART transfers. > > > > > > Both U-Boot and Linux kernel, when initializing A385 watchdog, switch > > > watchdog timer to 25 MHz input clock, so usage of NBCLK input clock in > > > U-Boot SPL does not cause any issues. > > > > > > Fixes: aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART") > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > > > I'm wondering if it makes sense to add this "slow" watchdog support to > > the common watchdog driver instead (orion_wdt.c). So that it dynamically > > configures the needed input clock for the desired timeout. > > > > Would this be possible? > > I know I'm late to the thread here, but why can't / aren't we servicing > the 120s watchdog during UART loading? UART loading is not done by U-Boot. It is done by BootROM. BootROM first loads SPL. Then it starts executing SPL and SPL must return control back to BootROM. BootROM then continue loading of proper U-Boot via UART and after successful transfer it executes proper U-Boot. BootROM does not enable watchdog and does not kick watchdog during UART transfer. So if we enable 120s watchdog in SPL and BootROM does not finish loading of proper U-Boot then board is rebooted. Turris Omnia has additional MCU watchdog, which is activated after reset, but there is no function to kick this watchdog. It can be only stopped. Commit mentioned in Fixes: tag turned this MCU watchdog in SPL and this new commit enable one-shot A385 watchdog with large timeout which should be enough for loading proper U-Boot. > Or have I gone crazy and we don't do that on say OMAP? I do not know how is loading over UART implemented on OMAP. On OMAP3 I always used only loading over USB and watchdog was not enabled in X-Loader / SPL. > -- > Tom
On Fri, Nov 12, 2021 at 05:40:42PM +0100, Pali Rohár wrote: > On Friday 12 November 2021 11:31:38 Tom Rini wrote: > > On Fri, Nov 12, 2021 at 02:44:15PM +0100, Stefan Roese wrote: > > > On 11/9/21 17:14, Marek Behún wrote: > > > > From: Pali Rohár <pali@kernel.org> > > > > > > > > Commit aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in > > > > SPL when booting over UART") disabled MCU watchdog when booting over > > > > UART to ensure that watchdog does not reboot the board before UART > > > > transfer finishes. > > > > > > > > But if UART transfer fails for some reason, or if U-Boot binary crashes, > > > > then board hangs forever as there is no watchdog running which could > > > > reset it. > > > > > > > > To fix this issue, enable A385 watchdog with very high timeout before > > > > disabling MCU watchdog to ensure that even slow transfer can finish > > > > successfully before watchdog timer expires and also to ensure that if > > > > board hangs for some reason, watchdog will reset it. > > > > > > > > Omnia's MCU watchdog has fixed 120 seconds timer and it cannot be > > > > changed (without updating MCU firmware). A385 watchdog by default uses > > > > 25 MHz input clock and so the largest timeout value (2^32-1) can be > > > > just 171 seconds. But A385 watchdog can be switched to use NBCLK (L2) as > > > > input clock (on Turris Omnia it is 800 MHz clock) and in this case final > > > > watchdog clock frequency is calculated as: > > > > > > > > freq = NBCLK / 2 / (2 ^ R) > > > > > > > > So A385 watchdog on Turris Omnia can be configured to at most 1374 > > > > seconds (about 22 minutes). We set it to 10 minutes, which should be > > > > enough even for bigger U-Boot binaries or slower UART transfers. > > > > > > > > Both U-Boot and Linux kernel, when initializing A385 watchdog, switch > > > > watchdog timer to 25 MHz input clock, so usage of NBCLK input clock in > > > > U-Boot SPL does not cause any issues. > > > > > > > > Fixes: aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART") > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > > > > > I'm wondering if it makes sense to add this "slow" watchdog support to > > > the common watchdog driver instead (orion_wdt.c). So that it dynamically > > > configures the needed input clock for the desired timeout. > > > > > > Would this be possible? > > > > I know I'm late to the thread here, but why can't / aren't we servicing > > the 120s watchdog during UART loading? > > UART loading is not done by U-Boot. It is done by BootROM. Ah, that's the bit of the system I didn't know, ok. > BootROM first loads SPL. Then it starts executing SPL and SPL must > return control back to BootROM. BootROM then continue loading of proper > U-Boot via UART and after successful transfer it executes proper U-Boot. > > BootROM does not enable watchdog and does not kick watchdog during UART > transfer. > > So if we enable 120s watchdog in SPL and BootROM does not finish loading > of proper U-Boot then board is rebooted. > > Turris Omnia has additional MCU watchdog, which is activated after > reset, but there is no function to kick this watchdog. It can be only > stopped. Commit mentioned in Fixes: tag turned this MCU watchdog in SPL > and this new commit enable one-shot A385 watchdog with large timeout > which should be enough for loading proper U-Boot. I'm not sure I can think of anything better to handle this than what you're describing off-hand, but I'll keep thinking about it in case I do.. > > Or have I gone crazy and we don't do that on say OMAP? > > I do not know how is loading over UART implemented on OMAP. On OMAP3 I > always used only loading over USB and watchdog was not enabled in > X-Loader / SPL. Right, OK, it wasn't until AM33xx and later that I started enabling SPL watchdog support (which could be done on omap3 too) and since we implement the normal servicing mechanisms and that U-Boot SPL is what's doing the load over UART of main U-Boot, this case is fine there.
On 11/9/21 17:14, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > Commit aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in > SPL when booting over UART") disabled MCU watchdog when booting over > UART to ensure that watchdog does not reboot the board before UART > transfer finishes. > > But if UART transfer fails for some reason, or if U-Boot binary crashes, > then board hangs forever as there is no watchdog running which could > reset it. > > To fix this issue, enable A385 watchdog with very high timeout before > disabling MCU watchdog to ensure that even slow transfer can finish > successfully before watchdog timer expires and also to ensure that if > board hangs for some reason, watchdog will reset it. > > Omnia's MCU watchdog has fixed 120 seconds timer and it cannot be > changed (without updating MCU firmware). A385 watchdog by default uses > 25 MHz input clock and so the largest timeout value (2^32-1) can be > just 171 seconds. But A385 watchdog can be switched to use NBCLK (L2) as > input clock (on Turris Omnia it is 800 MHz clock) and in this case final > watchdog clock frequency is calculated as: > > freq = NBCLK / 2 / (2 ^ R) > > So A385 watchdog on Turris Omnia can be configured to at most 1374 > seconds (about 22 minutes). We set it to 10 minutes, which should be > enough even for bigger U-Boot binaries or slower UART transfers. > > Both U-Boot and Linux kernel, when initializing A385 watchdog, switch > watchdog timer to 25 MHz input clock, so usage of NBCLK input clock in > U-Boot SPL does not cause any issues. > > Fixes: aeb0ca64dbb5 ("arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART") > Signed-off-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Marek Behún <marek.behun@nic.cz> Applied to u-boot-marvell/master Thanks, Stefan > --- > board/CZ.NIC/turris_omnia/turris_omnia.c | 65 +++++++++++++++++++++++- > 1 file changed, 63 insertions(+), 2 deletions(-) > > diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c > index 36c596efc5..ae24d14b76 100644 > --- a/board/CZ.NIC/turris_omnia/turris_omnia.c > +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c > @@ -43,6 +43,23 @@ DECLARE_GLOBAL_DATA_PTR; > #define OMNIA_I2C_EEPROM_CHIP_LEN 2 > #define OMNIA_I2C_EEPROM_MAGIC 0x0341a034 > > +#define SYS_RSTOUT_MASK MVEBU_REGISTER(0x18260) > +#define SYS_RSTOUT_MASK_WD BIT(10) > + > +#define A385_WDT_GLOBAL_CTRL MVEBU_REGISTER(0x20300) > +#define A385_WDT_GLOBAL_RATIO_MASK GENMASK(18, 16) > +#define A385_WDT_GLOBAL_RATIO_SHIFT 16 > +#define A385_WDT_GLOBAL_25MHZ BIT(10) > +#define A385_WDT_GLOBAL_ENABLE BIT(8) > + > +#define A385_WDT_GLOBAL_STATUS MVEBU_REGISTER(0x20304) > +#define A385_WDT_GLOBAL_EXPIRED BIT(31) > + > +#define A385_WDT_DURATION MVEBU_REGISTER(0x20334) > + > +#define A385_WD_RSTOUT_UNMASK MVEBU_REGISTER(0x20704) > +#define A385_WD_RSTOUT_UNMASK_GLOBAL BIT(8) > + > enum mcu_commands { > CMD_GET_STATUS_WORD = 0x01, > CMD_GET_RESET = 0x09, > @@ -141,6 +158,47 @@ static int omnia_mcu_write(u8 cmd, const void *buf, int len) > return dm_i2c_write(chip, cmd, buf, len); > } > > +static void enable_a385_watchdog(unsigned int timeout_minutes) > +{ > + struct sar_freq_modes sar_freq; > + u32 watchdog_freq; > + > + printf("Enabling A385 watchdog with %u minutes timeout...\n", > + timeout_minutes); > + > + /* > + * Use NBCLK clock (a.k.a. L2 clock) as watchdog input clock with > + * its maximal ratio 7 instead of default fixed 25 MHz clock. > + * It allows to set watchdog duration up to the 22 minutes. > + */ > + clrsetbits_32(A385_WDT_GLOBAL_CTRL, > + A385_WDT_GLOBAL_25MHZ | A385_WDT_GLOBAL_RATIO_MASK, > + 7 << A385_WDT_GLOBAL_RATIO_SHIFT); > + > + /* > + * Calculate watchdog clock frequency. It is defined by formula: > + * freq = NBCLK / 2 / (2 ^ ratio) > + * We set ratio to the maximal possible value 7. > + */ > + get_sar_freq(&sar_freq); > + watchdog_freq = sar_freq.nb_clk * 1000000 / 2 / (1 << 7); > + > + /* Set watchdog duration */ > + writel(timeout_minutes * 60 * watchdog_freq, A385_WDT_DURATION); > + > + /* Clear the watchdog expiration bit */ > + clrbits_32(A385_WDT_GLOBAL_STATUS, A385_WDT_GLOBAL_EXPIRED); > + > + /* Enable watchdog timer */ > + setbits_32(A385_WDT_GLOBAL_CTRL, A385_WDT_GLOBAL_ENABLE); > + > + /* Enable reset on watchdog */ > + setbits_32(A385_WD_RSTOUT_UNMASK, A385_WD_RSTOUT_UNMASK_GLOBAL); > + > + /* Unmask reset for watchdog */ > + clrbits_32(SYS_RSTOUT_MASK, SYS_RSTOUT_MASK_WD); > +} > + > static bool disable_mcu_watchdog(void) > { > int ret; > @@ -423,10 +481,13 @@ void spl_board_init(void) > { > /* > * If booting from UART, disable MCU watchdog in SPL, since uploading > - * U-Boot proper can take too much time and trigger it. > + * U-Boot proper can take too much time and trigger it. Instead enable > + * A385 watchdog with very high timeout (10 minutes) to prevent hangup. > */ > - if (get_boot_device() == BOOT_DEVICE_UART) > + if (get_boot_device() == BOOT_DEVICE_UART) { > + enable_a385_watchdog(10); > disable_mcu_watchdog(); > + } > } > > int board_init(void) > Viele Grüße, Stefan Roese
diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c index 36c596efc5..ae24d14b76 100644 --- a/board/CZ.NIC/turris_omnia/turris_omnia.c +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c @@ -43,6 +43,23 @@ DECLARE_GLOBAL_DATA_PTR; #define OMNIA_I2C_EEPROM_CHIP_LEN 2 #define OMNIA_I2C_EEPROM_MAGIC 0x0341a034 +#define SYS_RSTOUT_MASK MVEBU_REGISTER(0x18260) +#define SYS_RSTOUT_MASK_WD BIT(10) + +#define A385_WDT_GLOBAL_CTRL MVEBU_REGISTER(0x20300) +#define A385_WDT_GLOBAL_RATIO_MASK GENMASK(18, 16) +#define A385_WDT_GLOBAL_RATIO_SHIFT 16 +#define A385_WDT_GLOBAL_25MHZ BIT(10) +#define A385_WDT_GLOBAL_ENABLE BIT(8) + +#define A385_WDT_GLOBAL_STATUS MVEBU_REGISTER(0x20304) +#define A385_WDT_GLOBAL_EXPIRED BIT(31) + +#define A385_WDT_DURATION MVEBU_REGISTER(0x20334) + +#define A385_WD_RSTOUT_UNMASK MVEBU_REGISTER(0x20704) +#define A385_WD_RSTOUT_UNMASK_GLOBAL BIT(8) + enum mcu_commands { CMD_GET_STATUS_WORD = 0x01, CMD_GET_RESET = 0x09, @@ -141,6 +158,47 @@ static int omnia_mcu_write(u8 cmd, const void *buf, int len) return dm_i2c_write(chip, cmd, buf, len); } +static void enable_a385_watchdog(unsigned int timeout_minutes) +{ + struct sar_freq_modes sar_freq; + u32 watchdog_freq; + + printf("Enabling A385 watchdog with %u minutes timeout...\n", + timeout_minutes); + + /* + * Use NBCLK clock (a.k.a. L2 clock) as watchdog input clock with + * its maximal ratio 7 instead of default fixed 25 MHz clock. + * It allows to set watchdog duration up to the 22 minutes. + */ + clrsetbits_32(A385_WDT_GLOBAL_CTRL, + A385_WDT_GLOBAL_25MHZ | A385_WDT_GLOBAL_RATIO_MASK, + 7 << A385_WDT_GLOBAL_RATIO_SHIFT); + + /* + * Calculate watchdog clock frequency. It is defined by formula: + * freq = NBCLK / 2 / (2 ^ ratio) + * We set ratio to the maximal possible value 7. + */ + get_sar_freq(&sar_freq); + watchdog_freq = sar_freq.nb_clk * 1000000 / 2 / (1 << 7); + + /* Set watchdog duration */ + writel(timeout_minutes * 60 * watchdog_freq, A385_WDT_DURATION); + + /* Clear the watchdog expiration bit */ + clrbits_32(A385_WDT_GLOBAL_STATUS, A385_WDT_GLOBAL_EXPIRED); + + /* Enable watchdog timer */ + setbits_32(A385_WDT_GLOBAL_CTRL, A385_WDT_GLOBAL_ENABLE); + + /* Enable reset on watchdog */ + setbits_32(A385_WD_RSTOUT_UNMASK, A385_WD_RSTOUT_UNMASK_GLOBAL); + + /* Unmask reset for watchdog */ + clrbits_32(SYS_RSTOUT_MASK, SYS_RSTOUT_MASK_WD); +} + static bool disable_mcu_watchdog(void) { int ret; @@ -423,10 +481,13 @@ void spl_board_init(void) { /* * If booting from UART, disable MCU watchdog in SPL, since uploading - * U-Boot proper can take too much time and trigger it. + * U-Boot proper can take too much time and trigger it. Instead enable + * A385 watchdog with very high timeout (10 minutes) to prevent hangup. */ - if (get_boot_device() == BOOT_DEVICE_UART) + if (get_boot_device() == BOOT_DEVICE_UART) { + enable_a385_watchdog(10); disable_mcu_watchdog(); + } } int board_init(void)