[PATCH v1 0/4] support watchdog with longer timeout period

[PATCH v1 0/4] support watchdog with longer timeout period

[michaelsh]

From: Michael Shych <michaelsh@mellanox.com>

This patchset adds support of extended new watchdog type 3 of Mellanox
Ethernet and Infiniband switch systems.
This type of watchdog can have a timeout period longer than 255 or 32 sec.
as it was before.

Michael Shych (4):
  platform_data/mlxreg: support new watchdog type with longer timeout
    period
  platform/x86: mlx-platform: support new watchdog type with longer
    timeout
  watchdog: mlx-wdt: support new watchdog type with longer timeout
    period
  Documentation/watchdog: Add description of new watchdog type 3

 Documentation/watchdog/mlx-wdt.rst   |   9 +++
 drivers/platform/x86/mlx-platform.c  | 106 +++++++++++++++++++++++++++++++
 drivers/watchdog/mlx_wdt.c           |  75 +++++++++++++++++++++----
 include/linux/platform_data/mlxreg.h |   5 +-
 4 files changed, 183 insertions(+), 12 deletions(-)

-- 
2.11.0

[PATCH v1 1/4] platform_data/mlxreg: support new watchdog type with longer timeout period

[michaelsh]

From: Michael Shych <michaelsh@mellanox.com>

Add new watchdog type 3 with longer timeout period.
Extend size of health_cntr field that that can be used to init watchdog
timeout period.

Signed-off-by: Michael Shych <michaelsh@mellanox.com>

Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
---
 include/linux/platform_data/mlxreg.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/platform_data/mlxreg.h b/include/linux/platform_data/mlxreg.h
index b8da8aef2446..2c5e58d1d77b 100644
--- a/include/linux/platform_data/mlxreg.h
+++ b/include/linux/platform_data/mlxreg.h
@@ -43,10 +43,13 @@
  *
  * TYPE1 HW watchdog implementation exist in old systems.
  * All new systems have TYPE2 HW watchdog.
+ * TYPE3 HW watchdog can exist on all systems with new CPLD.
+ * TYPE3 is selected by WD capability bit.
  */
 enum mlxreg_wdt_type {
 	MLX_WDT_TYPE1,
 	MLX_WDT_TYPE2,
+	MLX_WDT_TYPE3,
 };
 
 /**
@@ -90,7 +93,7 @@ struct mlxreg_core_data {
 	umode_t	mode;
 	struct device_node *np;
 	struct mlxreg_hotplug_device hpdev;
-	u8 health_cntr;
+	u32 health_cntr;
 	bool attached;
 };
 
-- 
2.11.0

[PATCH v1 2/4] platform/x86: mlx-platform: support new watchdog type with longer timeout

[michaelsh]

From: Michael Shych <michaelsh@mellanox.com>

Add verification of WD capability in order to distinguish between the
x existing WD types and new type, implemented in CPLD.
Add configuration for a new WD type.
Change access mode for watchdog registers.

Signed-off-by: Michael Shych <michaelsh@mellanox.com>

Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/platform/x86/mlx-platform.c | 106 ++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/platform/x86/mlx-platform.c b/drivers/platform/x86/mlx-platform.c
index c27548fd386a..9d3371cd58d5 100644
--- a/drivers/platform/x86/mlx-platform.c
+++ b/drivers/platform/x86/mlx-platform.c
@@ -178,7 +178,9 @@
 #define MLXPLAT_CPLD_WD_RESET_ACT_MASK	GENMASK(7, 1)
 #define MLXPLAT_CPLD_WD_FAN_ACT_MASK	(GENMASK(7, 0) & ~BIT(4))
 #define MLXPLAT_CPLD_WD_COUNT_ACT_MASK	(GENMASK(7, 0) & ~BIT(7))
+#define MLXPLAT_CPLD_WD_CPBLTY_MASK	(GENMASK(7, 0) & ~BIT(6))
 #define MLXPLAT_CPLD_WD_DFLT_TIMEOUT	30
+#define MLXPLAT_CPLD_WD3_DFLT_TIMEOUT	600
 #define MLXPLAT_CPLD_WD_MAX_DEVS	2
 
 /* mlxplat_priv - platform private data
@@ -1959,6 +1961,84 @@ static struct mlxreg_core_platform_data mlxplat_mlxcpld_wd_set_type2[] = {
 	},
 };
 
+/* Watchdog type3: hardware implementation version 3
+ * Can be on all systems. It's differentiated by WD capability bit.
+ * Old systems (MSN2700, MSN2410, MSN2740, MSN2100 and MSN2140)
+ * still have only one main watchdog.
+ */
+static struct mlxreg_core_data mlxplat_mlxcpld_wd_main_regs_type3[] = {
+	{
+		.label = "action",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD2_ACT_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_RESET_ACT_MASK,
+		.bit = 0,
+	},
+	{
+		.label = "timeout",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD2_TMR_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_TYPE2_TO_MASK,
+		.health_cntr = MLXPLAT_CPLD_WD3_DFLT_TIMEOUT,
+	},
+	{
+		.label = "timeleft",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD2_TMR_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_TYPE2_TO_MASK,
+	},
+	{
+		.label = "ping",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD2_ACT_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_RESET_ACT_MASK,
+		.bit = 0,
+	},
+	{
+		.label = "reset",
+		.reg = MLXPLAT_CPLD_LPC_REG_RESET_CAUSE_OFFSET,
+		.mask = GENMASK(7, 0) & ~BIT(6),
+		.bit = 6,
+	},
+};
+
+static struct mlxreg_core_data mlxplat_mlxcpld_wd_aux_regs_type3[] = {
+	{
+		.label = "action",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD3_ACT_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_FAN_ACT_MASK,
+		.bit = 4,
+	},
+	{
+		.label = "timeout",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD3_TMR_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_TYPE2_TO_MASK,
+		.health_cntr = MLXPLAT_CPLD_WD3_DFLT_TIMEOUT,
+	},
+	{
+		.label = "timeleft",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD3_TMR_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_TYPE2_TO_MASK,
+	},
+	{
+		.label = "ping",
+		.reg = MLXPLAT_CPLD_LPC_REG_WD3_ACT_OFFSET,
+		.mask = MLXPLAT_CPLD_WD_FAN_ACT_MASK,
+		.bit = 4,
+	},
+};
+
+static struct mlxreg_core_platform_data mlxplat_mlxcpld_wd_set_type3[] = {
+	{
+		.data = mlxplat_mlxcpld_wd_main_regs_type3,
+		.counter = ARRAY_SIZE(mlxplat_mlxcpld_wd_main_regs_type3),
+		.version = MLX_WDT_TYPE3,
+		.identity = "mlx-wdt-main",
+	},
+	{
+		.data = mlxplat_mlxcpld_wd_aux_regs_type3,
+		.counter = ARRAY_SIZE(mlxplat_mlxcpld_wd_aux_regs_type3),
+		.version = MLX_WDT_TYPE3,
+		.identity = "mlx-wdt-aux",
+	},
+};
+
 static bool mlxplat_mlxcpld_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -1989,8 +2069,10 @@ static bool mlxplat_mlxcpld_writeable_reg(struct device *dev, unsigned int reg)
 	case MLXPLAT_CPLD_LPC_REG_WD1_TMR_OFFSET:
 	case MLXPLAT_CPLD_LPC_REG_WD1_ACT_OFFSET:
 	case MLXPLAT_CPLD_LPC_REG_WD2_TMR_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_WD2_TLEFT_OFFSET:
 	case MLXPLAT_CPLD_LPC_REG_WD2_ACT_OFFSET:
 	case MLXPLAT_CPLD_LPC_REG_WD3_TMR_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_WD3_TLEFT_OFFSET:
 	case MLXPLAT_CPLD_LPC_REG_WD3_ACT_OFFSET:
 	case MLXPLAT_CPLD_LPC_REG_PWM1_OFFSET:
 	case MLXPLAT_CPLD_LPC_REG_PWM_CONTROL_OFFSET:
@@ -2601,6 +2683,27 @@ static int mlxplat_mlxcpld_verify_bus_topology(int *nr)
 	return 0;
 }
 
+static int mlxplat_mlxcpld_check_wd_capability(void *regmap)
+{
+	u32 regval;
+	int i, rc;
+
+	rc = regmap_read(regmap, MLXPLAT_CPLD_LPC_REG_PSU_I2C_CAP_OFFSET,
+			 &regval);
+	if (rc)
+		return rc;
+
+	if (!(regval & ~MLXPLAT_CPLD_WD_CPBLTY_MASK)) {
+		for (i = 0; i < ARRAY_SIZE(mlxplat_mlxcpld_wd_set_type3); i++) {
+			if (mlxplat_wd_data[i])
+				mlxplat_wd_data[i] =
+					&mlxplat_mlxcpld_wd_set_type3[i];
+		}
+	}
+
+	return 0;
+}
+
 static int __init mlxplat_init(void)
 {
 	struct mlxplat_priv *priv;
@@ -2733,6 +2836,9 @@ static int __init mlxplat_init(void)
 	}
 
 	/* Add WD drivers. */
+	err = mlxplat_mlxcpld_check_wd_capability(priv->regmap);
+	if (err)
+		goto fail_platform_wd_register;
 	for (j = 0; j < MLXPLAT_CPLD_WD_MAX_DEVS; j++) {
 		if (mlxplat_wd_data[j]) {
 			mlxplat_wd_data[j]->regmap = priv->regmap;
-- 
2.11.0

[PATCH v1 3/4] watchdog: mlx-wdt: support new watchdog type with longer timeout period

[michaelsh]

From: Michael Shych <michaelsh@mellanox.com>

New programmable logic device can have watchdog type 3 implementation.
It's same as Type 2 with extended maximum timeout period.
Maximum timeout is up-to 65535 sec.
Type 3 HW watchdog implementation can exist on all Mellanox systems.
It is differentiated by WD capability bit.

Signed-off-by: Michael Shych <michaelsh@mellanox.com>

Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/watchdog/mlx_wdt.c | 75 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/mlx_wdt.c b/drivers/watchdog/mlx_wdt.c
index 03b9ac4b99af..c6d81a3d4587 100644
--- a/drivers/watchdog/mlx_wdt.c
+++ b/drivers/watchdog/mlx_wdt.c
@@ -21,6 +21,7 @@
 #define MLXREG_WDT_CLOCK_SCALE		1000
 #define MLXREG_WDT_MAX_TIMEOUT_TYPE1	32
 #define MLXREG_WDT_MAX_TIMEOUT_TYPE2	255
+#define MLXREG_WDT_MAX_TIMEOUT_TYPE3	65535
 #define MLXREG_WDT_MIN_TIMEOUT		1
 #define MLXREG_WDT_OPTIONS_BASE (WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | \
 				 WDIOF_SETTIMEOUT)
@@ -49,6 +50,7 @@ struct mlxreg_wdt {
 	int tleft_idx;
 	int ping_idx;
 	int reset_idx;
+	int regmap_val_sz;
 	enum mlxreg_wdt_type wdt_type;
 };
 
@@ -111,7 +113,8 @@ static int mlxreg_wdt_set_timeout(struct watchdog_device *wdd,
 	u32 regval, set_time, hw_timeout;
 	int rc;
 
-	if (wdt->wdt_type == MLX_WDT_TYPE1) {
+	switch (wdt->wdt_type) {
+	case MLX_WDT_TYPE1:
 		rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
 		if (rc)
 			return rc;
@@ -120,14 +123,33 @@ static int mlxreg_wdt_set_timeout(struct watchdog_device *wdd,
 		regval = (regval & reg_data->mask) | hw_timeout;
 		/* Rowndown to actual closest number of sec. */
 		set_time = BIT(hw_timeout) / MLXREG_WDT_CLOCK_SCALE;
-	} else {
+		rc = regmap_write(wdt->regmap, reg_data->reg, regval);
+		break;
+	case MLX_WDT_TYPE2:
+		set_time = timeout;
+		rc = regmap_write(wdt->regmap, reg_data->reg, timeout);
+		break;
+	case MLX_WDT_TYPE3:
+		/* WD_TYPE3 has 2B set time register */
 		set_time = timeout;
-		regval = timeout;
+		if (wdt->regmap_val_sz == 1) {
+			timeout = cpu_to_le16(timeout);
+			regval = timeout & GENMASK(7, 0);
+			rc = regmap_write(wdt->regmap, reg_data->reg, regval);
+			if (!rc) {
+				regval = ror32((timeout & GENMASK(15, 8)), 8);
+				rc = regmap_write(wdt->regmap,
+						reg_data->reg + 1, regval);
+			}
+		} else {
+			rc = regmap_write(wdt->regmap, reg_data->reg, timeout);
+		}
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	wdd->timeout = set_time;
-	rc = regmap_write(wdt->regmap, reg_data->reg, regval);
-
 	if (!rc) {
 		/*
 		 * Restart watchdog with new timeout period
@@ -147,10 +169,26 @@ static unsigned int mlxreg_wdt_get_timeleft(struct watchdog_device *wdd)
 {
 	struct mlxreg_wdt *wdt = watchdog_get_drvdata(wdd);
 	struct mlxreg_core_data *reg_data = &wdt->pdata->data[wdt->tleft_idx];
-	u32 regval;
+	u32 regval, msb, lsb, reg = reg_data->reg;
 	int rc;
 
-	rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
+	if (wdt->wdt_type == MLX_WDT_TYPE2) {
+		rc = regmap_read(wdt->regmap, reg, &regval);
+	} else {
+		/* WD_TYPE3 has 2 byte timeleft register */
+		if (wdt->regmap_val_sz == 1) {
+			rc = regmap_read(wdt->regmap, reg, &lsb);
+			if (!rc) {
+				rc = regmap_read(wdt->regmap, ++reg, &msb);
+				regval = rol32((msb & GENMASK(7, 0)), 8) |
+						(lsb & GENMASK(7, 0));
+				regval = le16_to_cpu(regval);
+			}
+		} else {
+			rc = regmap_read(wdt->regmap, reg, &regval);
+		}
+	}
+
 	/* Return 0 timeleft in case of failure register read. */
 	return rc == 0 ? regval : 0;
 }
@@ -212,13 +250,23 @@ static void mlxreg_wdt_config(struct mlxreg_wdt *wdt,
 		wdt->wdd.info = &mlxreg_wdt_aux_info;
 
 	wdt->wdt_type = pdata->version;
-	if (wdt->wdt_type == MLX_WDT_TYPE2) {
-		wdt->wdd.ops = &mlxreg_wdt_ops_type2;
-		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE2;
-	} else {
+	switch (wdt->wdt_type) {
+	case MLX_WDT_TYPE1:
 		wdt->wdd.ops = &mlxreg_wdt_ops_type1;
 		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE1;
+		break;
+	case MLX_WDT_TYPE2:
+		wdt->wdd.ops = &mlxreg_wdt_ops_type2;
+		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE2;
+		break;
+	case MLX_WDT_TYPE3:
+		wdt->wdd.ops = &mlxreg_wdt_ops_type2;
+		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE3;
+		break;
+	default:
+		break;
 	}
+
 	wdt->wdd.min_timeout = MLXREG_WDT_MIN_TIMEOUT;
 }
 
@@ -249,6 +297,11 @@ static int mlxreg_wdt_probe(struct platform_device *pdev)
 
 	wdt->wdd.parent = dev;
 	wdt->regmap = pdata->regmap;
+	rc = regmap_get_val_bytes(wdt->regmap);
+	if (rc <= 0)
+		return -EINVAL;
+
+	wdt->regmap_val_sz = rc;
 	mlxreg_wdt_config(wdt, pdata);
 
 	if ((pdata->features & MLXREG_CORE_WD_FEATURE_NOWAYOUT))
-- 
2.11.0

[PATCH v1 4/4] Documentation/watchdog: Add description of new watchdog type 3

[michaelsh]

From: Michael Shych <michaelsh@mellanox.com>

Add documentation with details of new type of Mellanox watchdog driver.

Signed-off-by: Michael Shych <michaelsh@mellanox.com>

Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
---
 Documentation/watchdog/mlx-wdt.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/watchdog/mlx-wdt.rst b/Documentation/watchdog/mlx-wdt.rst
index bf5bafac47f0..fa0a94e5c82e 100644
--- a/Documentation/watchdog/mlx-wdt.rst
+++ b/Documentation/watchdog/mlx-wdt.rst
@@ -24,10 +24,19 @@ Type 2:
   Maximum timeout is 255 sec.
   Get time-left is supported.
 
+Type 3:
+  Same as Type 2 with extended maximum timeout period.
+  Maximum timeout is 65535 sec.
+
 Type 1 HW watchdog implementation exist in old systems and
 all new systems have type 2 HW watchdog.
 Two types of HW implementation have also different register map.
 
+Type 3 HW watchdog implementation can exist on all Mellanox systems
+with new programmer logic device.
+It's differentiated by WD capability bit.
+Old systems still have only one main watchdog.
+
 Mellanox system can have 2 watchdogs: main and auxiliary.
 Main and auxiliary watchdog devices can be enabled together
 on the same system.
-- 
2.11.0

Re: [PATCH v1 0/4] support watchdog with longer timeout period

[Andy Shevchenko]

On Thu, Apr 16, 2020 at 7:35 PM <michaelsh@mellanox.com> wrote:
>
> From: Michael Shych <michaelsh@mellanox.com>
>
> This patchset adds support of extended new watchdog type 3 of Mellanox
> Ethernet and Infiniband switch systems.
> This type of watchdog can have a timeout period longer than 255 or 32 sec.
> as it was before.
>

Pushed to my review and testing queue, thanks!


> Michael Shych (4):
>   platform_data/mlxreg: support new watchdog type with longer timeout
>     period
>   platform/x86: mlx-platform: support new watchdog type with longer
>     timeout
>   watchdog: mlx-wdt: support new watchdog type with longer timeout
>     period
>   Documentation/watchdog: Add description of new watchdog type 3
>
>  Documentation/watchdog/mlx-wdt.rst   |   9 +++
>  drivers/platform/x86/mlx-platform.c  | 106 +++++++++++++++++++++++++++++++
>  drivers/watchdog/mlx_wdt.c           |  75 +++++++++++++++++++++----
>  include/linux/platform_data/mlxreg.h |   5 +-
>  4 files changed, 183 insertions(+), 12 deletions(-)
>
> --
> 2.11.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] watchdog: mlx-wdt: support new watchdog type with longer timeout period

[Guenter Roeck]

On Thu, Apr 16, 2020 at 07:35:13PM +0300, michaelsh@mellanox.com wrote:
> From: Michael Shych <michaelsh@mellanox.com>
> 
> New programmable logic device can have watchdog type 3 implementation.
> It's same as Type 2 with extended maximum timeout period.
> Maximum timeout is up-to 65535 sec.
> Type 3 HW watchdog implementation can exist on all Mellanox systems.
> It is differentiated by WD capability bit.
> 
> Signed-off-by: Michael Shych <michaelsh@mellanox.com>
> 
Stray empty line.

> Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
> ---
>  drivers/watchdog/mlx_wdt.c | 75 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/watchdog/mlx_wdt.c b/drivers/watchdog/mlx_wdt.c
> index 03b9ac4b99af..c6d81a3d4587 100644
> --- a/drivers/watchdog/mlx_wdt.c
> +++ b/drivers/watchdog/mlx_wdt.c
> @@ -21,6 +21,7 @@
>  #define MLXREG_WDT_CLOCK_SCALE		1000
>  #define MLXREG_WDT_MAX_TIMEOUT_TYPE1	32
>  #define MLXREG_WDT_MAX_TIMEOUT_TYPE2	255
> +#define MLXREG_WDT_MAX_TIMEOUT_TYPE3	65535
>  #define MLXREG_WDT_MIN_TIMEOUT		1
>  #define MLXREG_WDT_OPTIONS_BASE (WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | \
>  				 WDIOF_SETTIMEOUT)
> @@ -49,6 +50,7 @@ struct mlxreg_wdt {
>  	int tleft_idx;
>  	int ping_idx;
>  	int reset_idx;
> +	int regmap_val_sz;
>  	enum mlxreg_wdt_type wdt_type;
>  };
>  
> @@ -111,7 +113,8 @@ static int mlxreg_wdt_set_timeout(struct watchdog_device *wdd,
>  	u32 regval, set_time, hw_timeout;
>  	int rc;
>  
> -	if (wdt->wdt_type == MLX_WDT_TYPE1) {
> +	switch (wdt->wdt_type) {
> +	case MLX_WDT_TYPE1:
>  		rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
>  		if (rc)
>  			return rc;
> @@ -120,14 +123,33 @@ static int mlxreg_wdt_set_timeout(struct watchdog_device *wdd,
>  		regval = (regval & reg_data->mask) | hw_timeout;
>  		/* Rowndown to actual closest number of sec. */
>  		set_time = BIT(hw_timeout) / MLXREG_WDT_CLOCK_SCALE;
> -	} else {
> +		rc = regmap_write(wdt->regmap, reg_data->reg, regval);
> +		break;
> +	case MLX_WDT_TYPE2:
> +		set_time = timeout;
> +		rc = regmap_write(wdt->regmap, reg_data->reg, timeout);
> +		break;
> +	case MLX_WDT_TYPE3:
> +		/* WD_TYPE3 has 2B set time register */
>  		set_time = timeout;
> -		regval = timeout;
> +		if (wdt->regmap_val_sz == 1) {
> +			timeout = cpu_to_le16(timeout);
> +			regval = timeout & GENMASK(7, 0);
> +			rc = regmap_write(wdt->regmap, reg_data->reg, regval);
> +			if (!rc) {
> +				regval = ror32((timeout & GENMASK(15, 8)), 8);
> +				rc = regmap_write(wdt->regmap,
> +						reg_data->reg + 1, regval);
> +			}

This code seems odd. I can understand if the destination is of fixed endianness,
ie either little or big ended. If that was the case, writing timeout & 0xff into
data->reg and then writing timeout >> 8 into reg_data->reg+1 would make sense.
But that isn't what is done here. On a big endian system, data->reg will get
the upper 8 bit and data->reg+1 will get the lower 8 bit. This implies that
the target would in that case be big endian. Is this really what is intended ?
Is it guaranteed that the host byte order always matches the device byte order ?

Also, why use something as complicated as ror32 instead of a simple >> 8 ?
And using "GENMASK(7,0)" for 0xff is for sure possible but seems overly
complicated to me.

> +		} else {
> +			rc = regmap_write(wdt->regmap, reg_data->reg, timeout);
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
>  	wdd->timeout = set_time;
> -	rc = regmap_write(wdt->regmap, reg_data->reg, regval);
> -
>  	if (!rc) {
>  		/*
>  		 * Restart watchdog with new timeout period
> @@ -147,10 +169,26 @@ static unsigned int mlxreg_wdt_get_timeleft(struct watchdog_device *wdd)
>  {
>  	struct mlxreg_wdt *wdt = watchdog_get_drvdata(wdd);
>  	struct mlxreg_core_data *reg_data = &wdt->pdata->data[wdt->tleft_idx];
> -	u32 regval;
> +	u32 regval, msb, lsb, reg = reg_data->reg;
>  	int rc;
>  
> -	rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
> +	if (wdt->wdt_type == MLX_WDT_TYPE2) {
> +		rc = regmap_read(wdt->regmap, reg, &regval);
> +	} else {
> +		/* WD_TYPE3 has 2 byte timeleft register */
> +		if (wdt->regmap_val_sz == 1) {
> +			rc = regmap_read(wdt->regmap, reg, &lsb);
> +			if (!rc) {
> +				rc = regmap_read(wdt->regmap, ++reg, &msb);

FWIW, incrementing reg only serves a purpose if the incremented value is
actually used. That is not the case here, so this only makes the code
more complicated and risky. Besides, it is inconsistent with the code
above which directly uses reg_data->reg.

> +				regval = rol32((msb & GENMASK(7, 0)), 8) |
> +						(lsb & GENMASK(7, 0));
> +				regval = le16_to_cpu(regval);

Same question as above. Worse, this code explicitly puts "lsb" into bit 0..7
of regval, and msb into bit 8..15. Then it takes that value as little endian
value (even though, at least according to lsb/msb variable names, it is
already in host byte order) and swaps the two bytes.

I'd really like to see this code tested on a big endian system.

> +			}
> +		} else {
> +			rc = regmap_read(wdt->regmap, reg, &regval);

... or maybe someone can point me to a regmap definition with 16-bit value
size for comparison/validation.

> +		}
> +	}
> +
>  	/* Return 0 timeleft in case of failure register read. */
>  	return rc == 0 ? regval : 0;
>  }
> @@ -212,13 +250,23 @@ static void mlxreg_wdt_config(struct mlxreg_wdt *wdt,
>  		wdt->wdd.info = &mlxreg_wdt_aux_info;
>  
>  	wdt->wdt_type = pdata->version;
> -	if (wdt->wdt_type == MLX_WDT_TYPE2) {
> -		wdt->wdd.ops = &mlxreg_wdt_ops_type2;
> -		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE2;
> -	} else {
> +	switch (wdt->wdt_type) {
> +	case MLX_WDT_TYPE1:
>  		wdt->wdd.ops = &mlxreg_wdt_ops_type1;
>  		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE1;
> +		break;
> +	case MLX_WDT_TYPE2:
> +		wdt->wdd.ops = &mlxreg_wdt_ops_type2;
> +		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE2;
> +		break;
> +	case MLX_WDT_TYPE3:
> +		wdt->wdd.ops = &mlxreg_wdt_ops_type2;
> +		wdt->wdd.max_timeout = MLXREG_WDT_MAX_TIMEOUT_TYPE3;
> +		break;
> +	default:
> +		break;
>  	}
> +
>  	wdt->wdd.min_timeout = MLXREG_WDT_MIN_TIMEOUT;
>  }
>  
> @@ -249,6 +297,11 @@ static int mlxreg_wdt_probe(struct platform_device *pdev)
>  
>  	wdt->wdd.parent = dev;
>  	wdt->regmap = pdata->regmap;
> +	rc = regmap_get_val_bytes(wdt->regmap);
> +	if (rc <= 0)
> +		return -EINVAL;

It seems to me that val_bytes == 0 does not really make any sense.
Does it really add value to check for that instead of just returning
any errors ?

> +
> +	wdt->regmap_val_sz = rc;
>  	mlxreg_wdt_config(wdt, pdata);
>  
>  	if ((pdata->features & MLXREG_CORE_WD_FEATURE_NOWAYOUT))
> -- 
> 2.11.0
> 

Re: [PATCH v1 4/4] Documentation/watchdog: Add description of new watchdog type 3

[Guenter Roeck]

On Thu, Apr 16, 2020 at 07:35:14PM +0300, michaelsh@mellanox.com wrote:
> From: Michael Shych <michaelsh@mellanox.com>
> 
> Add documentation with details of new type of Mellanox watchdog driver.
> 
> Signed-off-by: Michael Shych <michaelsh@mellanox.com>
> 

Stray empty line.

Also, the subject should reflect the driver name.

Guenter

> Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
> ---
>  Documentation/watchdog/mlx-wdt.rst | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/watchdog/mlx-wdt.rst b/Documentation/watchdog/mlx-wdt.rst
> index bf5bafac47f0..fa0a94e5c82e 100644
> --- a/Documentation/watchdog/mlx-wdt.rst
> +++ b/Documentation/watchdog/mlx-wdt.rst
> @@ -24,10 +24,19 @@ Type 2:
>    Maximum timeout is 255 sec.
>    Get time-left is supported.
>  
> +Type 3:
> +  Same as Type 2 with extended maximum timeout period.
> +  Maximum timeout is 65535 sec.
> +
>  Type 1 HW watchdog implementation exist in old systems and
>  all new systems have type 2 HW watchdog.
>  Two types of HW implementation have also different register map.
>  
> +Type 3 HW watchdog implementation can exist on all Mellanox systems
> +with new programmer logic device.
> +It's differentiated by WD capability bit.
> +Old systems still have only one main watchdog.
> +
>  Mellanox system can have 2 watchdogs: main and auxiliary.
>  Main and auxiliary watchdog devices can be enabled together
>  on the same system.
> -- 
> 2.11.0
> 

Re: [PATCH v1 0/4] support watchdog with longer timeout period

[Andy Shevchenko]

On Fri, Apr 17, 2020 at 4:41 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Apr 16, 2020 at 7:35 PM <michaelsh@mellanox.com> wrote:
> >
> > From: Michael Shych <michaelsh@mellanox.com>
> >
> > This patchset adds support of extended new watchdog type 3 of Mellanox
> > Ethernet and Infiniband switch systems.
> > This type of watchdog can have a timeout period longer than 255 or 32 sec.
> > as it was before.
> >
>
> Pushed to my review and testing queue, thanks!

Withdrawn due to changes requested.
Please, address and resend.

>
>
> > Michael Shych (4):
> >   platform_data/mlxreg: support new watchdog type with longer timeout
> >     period
> >   platform/x86: mlx-platform: support new watchdog type with longer
> >     timeout
> >   watchdog: mlx-wdt: support new watchdog type with longer timeout
> >     period
> >   Documentation/watchdog: Add description of new watchdog type 3
> >
> >  Documentation/watchdog/mlx-wdt.rst   |   9 +++
> >  drivers/platform/x86/mlx-platform.c  | 106 +++++++++++++++++++++++++++++++
> >  drivers/watchdog/mlx_wdt.c           |  75 +++++++++++++++++++++----
> >  include/linux/platform_data/mlxreg.h |   5 +-
> >  4 files changed, 183 insertions(+), 12 deletions(-)
> >
> > --
> > 2.11.0
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 4/4] Documentation/watchdog: Add description of new watchdog type 3

[Michael Shych]

Hi Guenter,

By some reason, I obtained only this your mail as a reply to sent patchset V1
Later on, I discovered your comments at 
https://www.spinics.net/lists/platform-driver-x86/msg21412.html
And saw that you also have comments regarding [PATCH v1 3/4].
I'm resending patchset v2 that address your comments.

Thanks,
    Michael.

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Saturday, April 18, 2020 2:47 PM
> To: Michael Shych <michaelsh@mellanox.com>
> Cc: wim@linux-watchdog.org; andy@infradead.org; dvhart@infradead.org; linux-
> watchdog@vger.kernel.org; platform-driver-x86@vger.kernel.org; Vadim
> Pasternak <vadimp@mellanox.com>
> Subject: Re: [PATCH v1 4/4] Documentation/watchdog: Add description of new
> watchdog type 3
> 
> On Thu, Apr 16, 2020 at 07:35:14PM +0300, michaelsh@mellanox.com wrote:
> > From: Michael Shych <michaelsh@mellanox.com>
> >
> > Add documentation with details of new type of Mellanox watchdog driver.
> >
> > Signed-off-by: Michael Shych <michaelsh@mellanox.com>
> >
> 
> Stray empty line.
> 
> Also, the subject should reflect the driver name.
> 
> Guenter
> 
> > Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
> > ---
> >  Documentation/watchdog/mlx-wdt.rst | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/Documentation/watchdog/mlx-wdt.rst
> b/Documentation/watchdog/mlx-wdt.rst
> > index bf5bafac47f0..fa0a94e5c82e 100644
> > --- a/Documentation/watchdog/mlx-wdt.rst
> > +++ b/Documentation/watchdog/mlx-wdt.rst
> > @@ -24,10 +24,19 @@ Type 2:
> >    Maximum timeout is 255 sec.
> >    Get time-left is supported.
> >
> > +Type 3:
> > +  Same as Type 2 with extended maximum timeout period.
> > +  Maximum timeout is 65535 sec.
> > +
> >  Type 1 HW watchdog implementation exist in old systems and
> >  all new systems have type 2 HW watchdog.
> >  Two types of HW implementation have also different register map.
> >
> > +Type 3 HW watchdog implementation can exist on all Mellanox systems
> > +with new programmer logic device.
> > +It's differentiated by WD capability bit.
> > +Old systems still have only one main watchdog.
> > +
> >  Mellanox system can have 2 watchdogs: main and auxiliary.
> >  Main and auxiliary watchdog devices can be enabled together
> >  on the same system.
> > --
> > 2.11.0
> >