Hi Guenter, On 2021/5/10 12:25, Guenter Roeck wrote: > On 5/9/21 8:41 PM, Shaokun Zhang wrote: >> Arm Base System Architecture 1.0[1] has introduced watchdog >> revision 1 that increases the length the watchdog offset > > Is that how they call the watchdog count register ? > I think yes. > Also, doesn't that mean that the maximum timeout supported > by the hardware is now larger ? No, maximum timeout is the same. But the clock can be higher than before. For Armv8.6, The frequency of CNTFRQ_EL0 is standardized to a frequency of 1GHz which will set gwdt->clk. If the timeout is greater than 4(second), the 32-bit counter(WOR) is not enough. > >> regiter to 48 bit, while other operation of the watchdog remains > > register Ok, will fix it. > >> the same. >> Let's support the feature infered it from the architecture version > > I can't parse this sentence. > Apologies for sentence, I mean that we can read or write the WOR using readl/writel or readq/writeq depending on the architecture version. If architecture version is 0, readl/writel are used. Otherwise, we use readq/writeq. >> of watchdog in W_IID register. If the version is 0x1, the watchdog > > W_IIDR ? > Yes >> offset register will be 48 bit, otherwise it will be 32 bit. > > 48 or 64 ? The code says 64. > The whole WOR is 64-bits: WOR_L and WOR_H. WOR_L[31:0] contains the lower 32 bits; WOR_H[63:32] comprises two parts, Bits[15:0] of WOR_H contains the upper 16 bits; Bits[31:16] of WOR_H is reserved that Read all zero and write has no effect. So the real use is 48-bit. >> >> [1] https://developer.arm.com/documentation/den0094/latest >> > > There is no download link at that location. Someone with access > to the documentation will have to confirm this. Can you access this link? If yes, there is a 'Download' label and you can upload the document and check page 47 of 96. > >> Cc: Wim Van Sebroeck <wim@xxxxxxxxxxxxxxxxxx> >> Cc: Guenter Roeck <linux@xxxxxxxxxxxx> >> Cc: Fu Wei <fu.wei@xxxxxxxxxx> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx> >> Cc: Al Stone <al.stone@xxxxxxxxxx> >> Cc: Timur Tabi <timur@xxxxxxxxxxxxxx> >> Cc: Jianchao Hu <hujianchao@xxxxxxxxxxxxx> >> Cc: Huiqiang Wang <wanghuiqiang@xxxxxxxxxx> >> Signed-off-by: Shaokun Zhang <zhangshaokun@xxxxxxxxxxxxx> >> --- >> drivers/watchdog/sbsa_gwdt.c | 46 +++++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 41 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c >> index f0f1e3b2e463..ca4f7c416f1e 100644 >> --- a/drivers/watchdog/sbsa_gwdt.c >> +++ b/drivers/watchdog/sbsa_gwdt.c >> @@ -73,16 +73,21 @@ >> #define SBSA_GWDT_WCS_WS0 BIT(1) >> #define SBSA_GWDT_WCS_WS1 BIT(2) >> +#define SBSA_GWDT_VERSION_MASK 0xF >> +#define SBSA_GWDT_VERSION_SHIFT 16 >> + >> /** >> * struct sbsa_gwdt - Internal representation of the SBSA GWDT >> * @wdd: kernel watchdog_device structure >> * @clk: store the System Counter clock frequency, in Hz. >> + * @version: store the architecture version >> * @refresh_base: Virtual address of the watchdog refresh frame >> * @control_base: Virtual address of the watchdog control frame >> */ >> struct sbsa_gwdt { >> struct watchdog_device wdd; >> u32 clk; >> + int version; >> void __iomem *refresh_base; >> void __iomem *control_base; >> }; >> @@ -113,6 +118,27 @@ MODULE_PARM_DESC(nowayout, >> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); >> /* >> + * Read and write are 32 or 64 bits depending on watchdog architecture >> + * version: if version is equal 0, its 32-bits operation; otherwise 64-bits >> + * operation is chosen. >> + */ >> +static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt) >> +{ >> + if (gwdt->version == 0) >> + return (u64)readl(gwdt->control_base + SBSA_GWDT_WOR); > > Unnecessary typecast. > Ok. >> + else >> + return readq(gwdt->control_base + SBSA_GWDT_WOR); >> +} >> + >> +static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt) > > What is the point of making val an u64 variable ? Without changing Oops, unsigned int is enough. > the maximum timeout it will never be larger than 0xffffffff. > No, the reason that I have explained that the clock can be 1GHz now. Thanks, Shaokun >> +{ >> + if (gwdt->version == 0) >> + writel((u32)val, gwdt->control_base + SBSA_GWDT_WOR); >> + else >> + writeq(val, gwdt->control_base + SBSA_GWDT_WOR); >> +} >> + >> +/* >> * watchdog operation functions >> */ >> static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, >> @@ -123,16 +149,14 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, >> wdd->timeout = timeout; >> if (action) >> - writel(gwdt->clk * timeout, >> - gwdt->control_base + SBSA_GWDT_WOR); >> + sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt); >> else >> /* >> * In the single stage mode, The first signal (WS0) is ignored, >> * the timeout is (WOR * 2), so the WOR should be configured >> * to half value of timeout. >> */ >> - writel(gwdt->clk / 2 * timeout, >> - gwdt->control_base + SBSA_GWDT_WOR); >> + sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt); >> return 0; >> } >> @@ -149,7 +173,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) >> */ >> if (!action && >> !(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0)) >> - timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR); >> + timeleft += sbsa_gwdt_reg_read(gwdt); >> timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) - >> arch_timer_read_counter(); >> @@ -172,6 +196,17 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) >> return 0; >> } >> +static void sbsa_gwdt_get_version(struct watchdog_device *wdd) >> +{ >> + struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd); >> + int ver; >> + >> + ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR); >> + ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK; >> + >> + gwdt->version = ver; >> +} >> + >> static int sbsa_gwdt_start(struct watchdog_device *wdd) >> { >> struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd); >> @@ -300,6 +335,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev) >> * it's also a ping, if watchdog is enabled. >> */ >> sbsa_gwdt_set_timeout(wdd, wdd->timeout); >> + sbsa_gwdt_get_version(wdd); >> watchdog_stop_on_reboot(wdd); >> ret = devm_watchdog_register_device(dev, wdd); >> > > .
