<div dir="ltr"><div dir="ltr">Sorry Guenter probebly I didnt explain it well.<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 1 Mar 2020 at 12:48, Guenter Roeck <<a href="mailto:linux@roeck-us.net">linux@roeck-us.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 3/1/20 1:40 AM, Tomer Maimon wrote:<br>
> During probe NPCM watchdog sets the following bootstatus flags:<br>
>       - WDIOF_CARDRESET represent power and core reset.<br>
>       - WDIOF_EXTERN1 represent watchdog 0-2 reset.<br>
>       - WDIOF_EXTERN2 represent software 1-4 reset.<br>
> <br>
> Each flag is representing a group of bootstatus.<br>
> The user can configure through the device treethe exact reset<br>
> to each flag group.<br>
> <br>
<br>
Sorry, this doesn't make sense to me. I could understand reporting<br>
the above, but it looks to me like devicetree is used to associate<br>
a reset bit from the controller with one of the above.<br>
Devicetree only seems to be used to associate reset status bits<br>
from the controller with WDIOF_CARDRESET, WDIOF_EXTERN1, or<br>
WDIOF_EXTERN2. That adds a lot of complexity for little if any<br>
gain. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It would make sense to set the bootstatus bits as suggested above,<br>
but that doesn't require devicetree properties.<br>
<br>
More comments inline.<br>
<br>
Guenter <br></blockquote><div> </div><div><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif" style="">In the NPCM750 we have the following
reset types:</font></p>

<ol start="1" type="1" style="margin-bottom:0cm">
 <li class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">board
     reset (Power on reset, Core reset)</font></li>
 <li class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">WD
     reset (0-2 WD reset).</font></li>
 <li class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">SW
     reset (1-4 SW reset).</font></li>
</ol>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"><br></font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Each board can use different reset
types, because in the WD status bit there is not enough bits to represent the
entire NPCM750 resets.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">The NPCM750 reset groups are
represent as follow:</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"> - WDIOF_CARDRESET represent
power and core reset.<br>
 - WDIOF_EXTERN1 represent watchdog 0-2 reset.<br>
 - WDIOF_EXTERN2 represent software 1-4 reset.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"> </font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif" style="">To have maximum </font><span style="font-family:Arial,sans-serif">flexibility</span> <font face="arial, sans-serif" style="">(with three boot status bits), In the device tree the user can
configure each reset group type according the one the board is using.</font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif" style=""><br></font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif" style="">Hope I explian it better :-)</font></p></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Signed-off-by: Tomer Maimon <<a href="mailto:tmaimon77@gmail.com" target="_blank">tmaimon77@gmail.com</a>><br>
> ---<br>
>  drivers/watchdog/npcm_wdt.c | 132 ++++++++++++++++++++++++++++++++----<br>
>  1 file changed, 119 insertions(+), 13 deletions(-)<br>
> <br>
> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c<br>
> index 8609c7acf17d..dba9a73249c9 100644<br>
> --- a/drivers/watchdog/npcm_wdt.c<br>
> +++ b/drivers/watchdog/npcm_wdt.c<br>
> @@ -11,7 +11,24 @@<br>
>  #include <linux/platform_device.h><br>
>  #include <linux/slab.h><br>
>  #include <linux/watchdog.h><br>
> -<br>
> +#include <linux/regmap.h><br>
> +#include <linux/mfd/syscon.h><br>
> +<br>
New include files in alphabetic order merged with existing ones, please.<br>
<br>
> +/* NPCM7xx GCR module */<br>
> +#define NPCM7XX_RESSR_OFFSET         0x6C<br>
> +#define NPCM7XX_INTCR2_OFFSET                0x60<br>
> +<br>
> +#define NPCM7XX_PORST                        BIT(31)<br>
> +#define NPCM7XX_CORST                        BIT(30)<br>
> +#define NPCM7XX_WD0RST                       BIT(29)<br>
> +#define NPCM7XX_WD1RST                       BIT(24)<br>
> +#define NPCM7XX_WD2RST                       BIT(23)<br>
> +#define NPCM7XX_SWR1RST                      BIT(28)<br>
> +#define NPCM7XX_SWR2RST                      BIT(27)<br>
> +#define NPCM7XX_SWR3RST                      BIT(26)<br>
> +#define NPCM7XX_SWR4RST                      BIT(25)<br>
> +<br>
> + /* WD register */<br>
>  #define NPCM_WTCR    0x1C<br>
>  <br>
>  #define NPCM_WTCLK   (BIT(10) | BIT(11))     /* Clock divider */<br>
> @@ -43,6 +60,9 @@<br>
>  struct npcm_wdt {<br>
>       struct watchdog_device  wdd;<br>
>       void __iomem            *reg;<br>
> +     u32                     card_reset;<br>
> +     u32                     ext1_reset;<br>
> +     u32                     ext2_reset;<br>
>  };<br>
>  <br>
>  static inline struct npcm_wdt *to_npcm_wdt(struct watchdog_device *wdd)<br>
> @@ -103,30 +123,29 @@ static int npcm_wdt_stop(struct watchdog_device *wdd)<br>
>       return 0;<br>
>  }<br>
>  <br>
> -<br>
>  static int npcm_wdt_set_timeout(struct watchdog_device *wdd,<br>
>                               unsigned int timeout)<br>
>  {<br>
>       if (timeout < 2)<br>
>               wdd->timeout = 1;<br>
>       else if (timeout < 3)<br>
> -           wdd->timeout = 2;<br>
> +             wdd->timeout = 2;<br>
>       else if (timeout < 6)<br>
> -           wdd->timeout = 5;<br>
> +             wdd->timeout = 5;<br>
>       else if (timeout < 11)<br>
> -           wdd->timeout = 10;<br>
> +             wdd->timeout = 10;<br>
>       else if (timeout < 22)<br>
> -           wdd->timeout = 21;<br>
> +             wdd->timeout = 21;<br>
>       else if (timeout < 44)<br>
> -           wdd->timeout = 43;<br>
> +             wdd->timeout = 43;<br>
>       else if (timeout < 87)<br>
> -           wdd->timeout = 86;<br>
> +             wdd->timeout = 86;<br>
>       else if (timeout < 173)<br>
> -           wdd->timeout = 172;<br>
> +             wdd->timeout = 172;<br>
>       else if (timeout < 688)<br>
> -           wdd->timeout = 687;<br>
> +             wdd->timeout = 687;<br>
>       else<br>
> -           wdd->timeout = 2750;<br>
> +             wdd->timeout = 2750;<br>
>  <br>
<br>
Whitespace changes in a separate patch, please.<br>
<br>
>       if (watchdog_active(wdd))<br>
>               npcm_wdt_start(wdd);<br>
> @@ -177,9 +196,61 @@ static const struct watchdog_ops npcm_wdt_ops = {<br>
>       .restart = npcm_wdt_restart,<br>
>  };<br>
>  <br>
> +static void npcm_get_reset_status(struct npcm_wdt *wdt, struct device *dev)<br>
> +{<br>
> +     struct regmap *gcr_regmap;<br>
> +     u32 rstval;<br>
> +<br>
> +     if (of_device_is_compatible(dev->of_node, "nuvoton,npcm750-wdt")) {<br>
> +             gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");<br>
> +             if (IS_ERR(gcr_regmap))<br>
> +                     dev_warn(dev, "Failed to find nuvoton,npcm750-gcr WD reset status not supported\n");<br>
> +<br>
> +             regmap_read(gcr_regmap, NPCM7XX_RESSR_OFFSET, &rstval);<br>
> +             if (!rstval) {<br>
> +                     regmap_read(gcr_regmap, NPCM7XX_INTCR2_OFFSET, &rstval);<br>
> +                     rstval = ~rstval;<br>
> +             }<br>
<br>
The second register reports the same as the first only negated if<br>
bits in the first register are not set ? That seems unlikely.<br>
Please point to the datasheet, or at least provide a reference to the<br>
two registers.<br></blockquote><div> </div><div>If NPCM750 is power on the NPCM7XX_RESSR_OFFSET register is cleaned and the reset copied to the NPCM7XX_INTCR2_OFFSET.<br></div><div>I will add a note to the code</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> +             if (rstval & wdt->card_reset)<br>
> +                     wdt->wdd.bootstatus |= WDIOF_CARDRESET;<br>
> +             if (rstval & wdt->ext1_reset)<br>
> +                     wdt->wdd.bootstatus |= WDIOF_EXTERN1;<br>
> +             if (rstval & wdt->ext2_reset)<br>
> +                     wdt->wdd.bootstatus |= WDIOF_EXTERN2;<br>
> +     }<br>
> +}<br>
> +<br>
> +static u32 npcm_wdt_reset_type(const char *reset_type)<br>
> +{<br>
> +     if (!strcmp(reset_type, "porst"))<br>
> +             return NPCM7XX_PORST;<br>
> +     else if (!strcmp(reset_type, "corst"))<br>
> +             return NPCM7XX_CORST;<br>
> +     else if (!strcmp(reset_type, "wd0"))<br>
> +             return NPCM7XX_WD0RST;<br>
> +     else if (!strcmp(reset_type, "wd1"))<br>
> +             return NPCM7XX_WD1RST;<br>
> +     else if (!strcmp(reset_type, "wd2"))<br>
> +             return NPCM7XX_WD2RST;<br>
> +     else if (!strcmp(reset_type, "sw1"))<br>
> +             return NPCM7XX_SWR1RST;<br>
> +     else if (!strcmp(reset_type, "sw2"))<br>
> +             return NPCM7XX_SWR2RST;<br>
> +     else if (!strcmp(reset_type, "sw3"))<br>
> +             return NPCM7XX_SWR3RST;<br>
> +     else if (!strcmp(reset_type, "sw4"))<br>
> +             return NPCM7XX_SWR4RST;<br>
> +<br>
> +     return 0;<br>
> +}<br>
> +<br>
>  static int npcm_wdt_probe(struct platform_device *pdev)<br>
>  {<br>
>       struct device *dev = &pdev->dev;<br>
> +     const char *card_reset_type;<br>
> +     const char *ext1_reset_type;<br>
> +     const char *ext2_reset_type;<br>
>       struct npcm_wdt *wdt;<br>
>       u32 priority;<br>
>       int irq;<br>
> @@ -202,6 +273,39 @@ static int npcm_wdt_probe(struct platform_device *pdev)<br>
>       else<br>
>               watchdog_set_restart_priority(&wdt->wdd, priority);<br>
>  <br>
> +     ret = of_property_read_string(pdev->dev.of_node,<br>
> +                                   "nuvoton,card-reset-type",<br>
> +                                   &card_reset_type);<br>
> +     if (ret) {<br>
> +             wdt->card_reset = NPCM7XX_PORST;<br>
> +     } else {<br>
> +             wdt->card_reset = npcm_wdt_reset_type(card_reset_type);<br>
> +             if (!wdt->card_reset)<br>
> +                     wdt->card_reset = NPCM7XX_PORST;<br>
> +     }<br>
> +<br>
> +     ret = of_property_read_string(pdev->dev.of_node,<br>
> +                                   "nuvoton,ext1-reset-type",<br>
> +                                   &ext1_reset_type);<br>
> +     if (ret) {<br>
> +             wdt->ext1_reset = NPCM7XX_WD0RST;<br>
> +     } else {<br>
> +             wdt->ext1_reset = npcm_wdt_reset_type(ext1_reset_type);<br>
> +             if (!wdt->ext1_reset)<br>
> +                     wdt->ext1_reset = NPCM7XX_WD0RST;<br>
> +     }<br>
> +<br>
> +     ret = of_property_read_string(pdev->dev.of_node,<br>
> +                                   "nuvoton,ext2-reset-type",<br>
> +                                   &ext2_reset_type);<br>
> +     if (ret) {<br>
> +             wdt->ext2_reset = NPCM7XX_SWR1RST;<br>
> +     } else {<br>
> +             wdt->ext2_reset = npcm_wdt_reset_type(ext2_reset_type);<br>
> +             if (!wdt->ext2_reset)<br>
> +                     wdt->ext2_reset = NPCM7XX_SWR1RST;<br>
> +     }<br>
> +<br>
>       wdt-><a href="http://wdd.info" rel="noreferrer" target="_blank">wdd.info</a> = &npcm_wdt_info;<br>
>       wdt->wdd.ops = &npcm_wdt_ops;<br>
>       wdt->wdd.min_timeout = 1;<br>
> @@ -220,8 +324,10 @@ static int npcm_wdt_probe(struct platform_device *pdev)<br>
>               set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);<br>
>       }<br>
>  <br>
> -     ret = devm_request_irq(dev, irq, npcm_wdt_interrupt, 0, "watchdog",<br>
> -                            wdt);<br>
> +     npcm_get_reset_status(wdt, dev);<br>
> +<br>
> +     ret = devm_request_irq(dev, irq, npcm_wdt_interrupt, 0,<br>
> +                            "watchdog", wdt);<br>
>       if (ret)<br>
>               return ret;<br>
>  <br>
> <br>
<br></blockquote><div><br></div><div>Thanks a lot,</div><div><br></div><div>Tomer </div></div></div>