Hi Guenter, On 2021/5/10 16:25, Shaokun Zhang wrote: > 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. > Sorry, it shall be 'u64', because it is the value that clock * timeout and will be written to WOR register which is 64-bit register in architecture version 1. Thanks, Shaokun >> 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); >>> >> >> .
