> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Wednesday, April 29, 2020 3:30 PM
> To: Michael Shych <michaelsh@xxxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx;
> andy@xxxxxxxxxxxxx; dvhart@xxxxxxxxxxxxx
> Cc: linux-watchdog@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx wrote:
> > From: Michael Shych <michaelsh@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Reviewed-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> > ---
> > 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, ®val);
> > 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, ®val);
> > + if (wdt->wdt_type == MLX_WDT_TYPE2) {
> > + rc = regmap_read(wdt->regmap, reg_data->reg, ®val);
> > + } 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,
> ®val);
> > + }
> > + }
> > +
> > /* 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))
> >
