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

[PATCH v2 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
  docs: watchdog: mlx-wdt: Add description of new watchdog type 3

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

-- 
2.11.0

[PATCH v2 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 v2 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 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 v2 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>
---
v1-v2:
Make changes pointed out by Guenter:
-Simplify bit operations
-Consistency in registers access
-Don't check irrelevant return code
---
 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..4e76d4a1af5c 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 & 0xff;
+			rc = regmap_write(wdt->regmap, reg_data->reg, regval);
+			if (!rc) {
+				regval = (timeout & 0xff00) >> 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;
 	int rc;
 
-	rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
+	if (wdt->wdt_type == MLX_WDT_TYPE2) {
+		rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
+	} else {
+		/* WD_TYPE3 has 2 byte timeleft register */
+		if (wdt->regmap_val_sz == 1) {
+			rc = regmap_read(wdt->regmap, reg_data->reg, &lsb);
+			if (!rc) {
+				rc = regmap_read(wdt->regmap,
+						reg_data->reg + 1, &msb);
+				regval = (msb & 0xff) << 8 | (lsb & 0xff);
+				regval = le16_to_cpu(regval);
+			}
+		} else {
+			rc = regmap_read(wdt->regmap, reg_data->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 v2 4/4] docs: watchdog: mlx-wdt: 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>
---
v1-v2:
Add explanation about device registers order
---
 Documentation/watchdog/mlx-wdt.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/watchdog/mlx-wdt.rst b/Documentation/watchdog/mlx-wdt.rst
index bf5bafac47f0..241768b885a5 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.
@@ -54,3 +63,6 @@ The driver checks during initialization if the previous system reset
 was done by the watchdog. If yes, it makes a notification about this event.
 
 Access to HW registers is performed through a generic regmap interface.
+Programmable logic device registers have little-endian order.
+Watchdog type 3, 2-byte width fields should be converted from LE to CPU order
+and vice versa.
-- 
2.11.0

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

[Andy Shevchenko]

On Tue, Apr 28, 2020 at 4:09 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.
>

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

for PDx86 bits.

> 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
>   docs: watchdog: mlx-wdt: Add description of new watchdog type 3
>
>  Documentation/watchdog/mlx-wdt.rst   |  12 ++++
>  drivers/platform/x86/mlx-platform.c  | 106 ++++++++++++++++++++++++++++++
>  drivers/watchdog/mlx_wdt.c           |  75 +++++++++++++++++++++----
>  include/linux/platform_data/mlxreg.h |   5 +-
>  4 files changed, 186 insertions(+), 12 deletions(-)
>
> --
> 2.11.0
>


-- 
With Best Regards,
Andy Shevchenko

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

[Guenter Roeck]

On 4/28/20 6:08 AM, 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>
> Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
> ---
> v1-v2:
> Make changes pointed out by Guenter:
> -Simplify bit operations
> -Consistency in registers access
> -Don't check irrelevant return code
> ---
>  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..4e76d4a1af5c 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 & 0xff;
> +			rc = regmap_write(wdt->regmap, reg_data->reg, regval);
> +			if (!rc) {
> +				regval = (timeout & 0xff00) >> 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;
>  	int rc;
>  
> -	rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
> +	if (wdt->wdt_type == MLX_WDT_TYPE2) {
> +		rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
> +	} else {
> +		/* WD_TYPE3 has 2 byte timeleft register */
> +		if (wdt->regmap_val_sz == 1) {
> +			rc = regmap_read(wdt->regmap, reg_data->reg, &lsb);
> +			if (!rc) {
> +				rc = regmap_read(wdt->regmap,
> +						reg_data->reg + 1, &msb);
> +				regval = (msb & 0xff) << 8 | (lsb & 0xff);
> +				regval = le16_to_cpu(regval);

I don't think my concerns regarding the use of le16_to_cpu() have been
addressed, and I still think this results in bad data on a big endian
system.

Guenter

> +			}
> +		} else {
> +			rc = regmap_read(wdt->regmap, reg_data->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))
> 

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

[Michael Shych]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Wednesday, April 29, 2020 3:30 PM
> To: Michael Shych <michaelsh@mellanox.com>; wim@linux-watchdog.org;
> andy@infradead.org; dvhart@infradead.org
> Cc: linux-watchdog@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> Vadim Pasternak <vadimp@mellanox.com>
> Subject: Re: [PATCH v2 3/4] watchdog: mlx-wdt: support new watchdog type
> with longer timeout period
> 
> On 4/28/20 6:08 AM, 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>
> > Reviewed-by: Vadim Pasternak <vadimp@mellanox.com>
> > ---
> > v1-v2:
> > Make changes pointed out by Guenter:
> > -Simplify bit operations
> > -Consistency in registers access
> > -Don't check irrelevant return code
> > ---
> >  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..4e76d4a1af5c 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 & 0xff;
> > +			rc = regmap_write(wdt->regmap, reg_data->reg, regval);
> > +			if (!rc) {
> > +				regval = (timeout & 0xff00) >> 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;
> >  	int rc;
> >
> > -	rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
> > +	if (wdt->wdt_type == MLX_WDT_TYPE2) {
> > +		rc = regmap_read(wdt->regmap, reg_data->reg, &regval);
> > +	} else {
> > +		/* WD_TYPE3 has 2 byte timeleft register */
> > +		if (wdt->regmap_val_sz == 1) {
> > +			rc = regmap_read(wdt->regmap, reg_data->reg, &lsb);
> > +			if (!rc) {
> > +				rc = regmap_read(wdt->regmap,
> > +						reg_data->reg + 1, &msb);
> > +				regval = (msb & 0xff) << 8 | (lsb & 0xff);
> > +				regval = le16_to_cpu(regval);
> 
> I don't think my concerns regarding the use of le16_to_cpu() have been
> addressed, and I still think this results in bad data on a big endian
> system.
> 
> Guenter

You are right. These are unnecessary conversions. I removed them as well as reference
To them in the document. Resending patchset v3.

> 
> > +			}
> > +		} else {
> > +			rc = regmap_read(wdt->regmap, reg_data->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))
> >