| Message ID | 1620618117-20135-1-git-send-email-zhangshaokun@hisilicon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: sbsa: Support architecture version 1 | expand |
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 ? Also, doesn't that mean that the maximum timeout supported by the hardware is now larger ? > regiter to 48 bit, while other operation of the watchdog remains register > the same. > Let's support the feature infered it from the architecture version I can't parse this sentence. > of watchdog in W_IID register. If the version is 0x1, the watchdog W_IIDR ? > offset register will be 48 bit, otherwise it will be 32 bit. 48 or 64 ? The code says 64. > > [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. > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Fu Wei <fu.wei@linaro.org> > Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Cc: Al Stone <al.stone@linaro.org> > Cc: Timur Tabi <timur@codeaurora.org> > Cc: Jianchao Hu <hujianchao@hisilicon.com> > Cc: Huiqiang Wang <wanghuiqiang@huawei.com> > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > --- > 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. > + 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 the maximum timeout it will never be larger than 0xffffffff. > +{ > + 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); >
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@linux-watchdog.org> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: Fu Wei <fu.wei@linaro.org> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> Cc: Al Stone <al.stone@linaro.org> >> Cc: Timur Tabi <timur@codeaurora.org> >> Cc: Jianchao Hu <hujianchao@hisilicon.com> >> Cc: Huiqiang Wang <wanghuiqiang@huawei.com> >> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >> --- >> 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); >> > > .
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@linux-watchdog.org> >>> Cc: Guenter Roeck <linux@roeck-us.net> >>> Cc: Fu Wei <fu.wei@linaro.org> >>> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >>> Cc: Al Stone <al.stone@linaro.org> >>> Cc: Timur Tabi <timur@codeaurora.org> >>> Cc: Jianchao Hu <hujianchao@hisilicon.com> >>> Cc: Huiqiang Wang <wanghuiqiang@huawei.com> >>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>> --- >>> 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); >>> >> >> .
On 5/10/21 1:25 AM, 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. > The maximuma timeout is limited with wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000; You did not update that calculation. That means that the maximuma timeout is still U32_MAX / gwdt->clk * 1000, which still fits into 32 bit. Please correct me if I am missing something. Guenter
On Mon, May 10, 2021 at 04:44:21PM +0800, Shaokun Zhang wrote: > >>> +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. > As I have been trying to point out, that won't really happen unless max_hw_heartbeat_ms is adjusted. The register may be a 64 bit register, and the variable may be a 64-bit variable, but that doesn't make the value passed in that variable larger than 32 bit unless the code is in place to actually do that. Guenter
Hi Guenter, On 2021/5/10 21:16, Guenter Roeck wrote: > On 5/10/21 1:25 AM, 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. >> > > The maximuma timeout is limited with > > wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000; > > You did not update that calculation. That means that the maximuma > timeout is still U32_MAX / gwdt->clk * 1000, which still fits > into 32 bit. Correct, I will fix this in next version. > > Please correct me if I am missing something. > My bad, you are right. The maximum timeout shall be 0xFFFF.FFFF.FFFF / 1000.000.000(1GHz) which is larger than before 0xFFFF.FFFF / 100.000.000(100MHz) by the hardware. Can I do like this, after the version is got and check the version? sbsa_gwdt_set_timeout(wdd, wdd->timeout); + sbsa_gwdt_get_version(wdd); + if (wdd->version > 0) + wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000; Thanks, Shaokun > Guenter > .
On 5/10/21 7:49 PM, Shaokun Zhang wrote: > Hi Guenter, > > On 2021/5/10 21:16, Guenter Roeck wrote: >> On 5/10/21 1:25 AM, 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. >>> >> >> The maximuma timeout is limited with >> >> wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000; >>> You did not update that calculation. That means that the maximuma >> timeout is still U32_MAX / gwdt->clk * 1000, which still fits >> into 32 bit. > > Correct, I will fix this in next version. > >> >> Please correct me if I am missing something. >> > > My bad, you are right. The maximum timeout shall be > 0xFFFF.FFFF.FFFF / 1000.000.000(1GHz) which is larger than before > 0xFFFF.FFFF / 100.000.000(100MHz) by the hardware. > > Can I do like this, after the version is got and check the version? > sbsa_gwdt_set_timeout(wdd, wdd->timeout); > + sbsa_gwdt_get_version(wdd); > + if (wdd->version > 0) > + wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000; > I would suggest to set max_hw_heartbeat_ms in one place to avoid confusion. Either check the version earlier, or move setting both max_hw_heartbeat_ms values after the call to sbsa_gwdt_get_version(). Thanks, Guenter
Hi Guenter, On 2021/5/11 11:52, Guenter Roeck wrote: > On 5/10/21 7:49 PM, Shaokun Zhang wrote: >> Hi Guenter, >> >> On 2021/5/10 21:16, Guenter Roeck wrote: >>> On 5/10/21 1:25 AM, 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. >>>> >>> >>> The maximuma timeout is limited with >>> >>> wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000; >>>> You did not update that calculation. That means that the maximuma >>> timeout is still U32_MAX / gwdt->clk * 1000, which still fits >>> into 32 bit. >> >> Correct, I will fix this in next version. >> >>> >>> Please correct me if I am missing something. >>> >> >> My bad, you are right. The maximum timeout shall be >> 0xFFFF.FFFF.FFFF / 1000.000.000(1GHz) which is larger than before >> 0xFFFF.FFFF / 100.000.000(100MHz) by the hardware. >> >> Can I do like this, after the version is got and check the version? >> sbsa_gwdt_set_timeout(wdd, wdd->timeout); >> + sbsa_gwdt_get_version(wdd); >> + if (wdd->version > 0) >> + wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000; >> > > I would suggest to set max_hw_heartbeat_ms in one place > to avoid confusion. Either check the version earlier, > or move setting both max_hw_heartbeat_ms values > after the call to sbsa_gwdt_get_version(). > Got it, I will follow the former that many members in @wdd are initialized closely. Thanks again, Shaokun > Thanks, > Guenter > > .
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); + else + return readq(gwdt->control_base + SBSA_GWDT_WOR); +} + +static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt) +{ + 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);
Arm Base System Architecture 1.0[1] has introduced watchdog revision 1 that increases the length the watchdog offset regiter to 48 bit, while other operation of the watchdog remains the same. Let's support the feature infered it from the architecture version of watchdog in W_IID register. If the version is 0x1, the watchdog offset register will be 48 bit, otherwise it will be 32 bit. [1] https://developer.arm.com/documentation/den0094/latest Cc: Wim Van Sebroeck <wim@linux-watchdog.org> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Fu Wei <fu.wei@linaro.org> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> Cc: Al Stone <al.stone@linaro.org> Cc: Timur Tabi <timur@codeaurora.org> Cc: Jianchao Hu <hujianchao@hisilicon.com> Cc: Huiqiang Wang <wanghuiqiang@huawei.com> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> --- drivers/watchdog/sbsa_gwdt.c | 46 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-)