Message-ID: <8afd7bda-a600-4abb-95fc-ee70c6f89749@kernel.org>
Date: Wed, 17 Jan 2024 16:27:42 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Lukasz Majczak <lma@...omium.org>, Gwendal Grignou
 <gwendal@...omium.org>, Lee Jones <lee@...nel.org>,
 Benson Leung <bleung@...omium.org>, Wim Van Sebroeck
 <wim@...ux-watchdog.org>, Tzung-Bi Shih <tzungbi@...nel.org>,
 Radoslaw Biernacki <biernacki@...gle.com>
Cc: linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev,
 linux-watchdog@...r.kernel.org
Subject: Re: [PATCH] drivers: watchdog: Add ChromeOS EC watchdog

On 17/01/2024 11:24, Lukasz Majczak wrote:
> This adds EC-based watchdog support for ChromeOS
> based devices equipped with embedded controller (EC).
> 
> Signed-off-by: Lukasz Majczak <lma@...omium.org>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/mfd/cros_ec_dev.c                     |   9 +
>  drivers/watchdog/Kconfig                      |  15 +
>  drivers/watchdog/Makefile                     |   3 +
>  drivers/watchdog/cros_ec_wdt.c                | 303 ++++++++++++++++++
>  .../linux/platform_data/cros_ec_commands.h    |  78 ++---
>  6 files changed, 370 insertions(+), 44 deletions(-)
>  create mode 100644 drivers/watchdog/cros_ec_wdt.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 391bbb855cbe..55cd626a525f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4952,6 +4952,12 @@ R:	Sami Kyöstilä <skyostil@...omium.org>
>  S:	Maintained
>  F:	drivers/platform/chrome/cros_hps_i2c.c
>  
> +CHROMEOS EC WATCHDOG
> +M:	Lukasz Majczak <lma@...omium.org>
> +L:	chrome-platform@...ts.linux.dev
> +S:	Maintained
> +F:	drivers/watchdog/cros_ec_wdt.c
> +
>  CHRONTEL CH7322 CEC DRIVER
>  M:	Joe Tessler <jrt@...gle.com>
>  L:	linux-media@...r.kernel.org
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 79d393b602bf..60fe831cf30a 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -91,6 +91,10 @@ static const struct mfd_cell cros_usbpd_notify_cells[] = {
>  	{ .name = "cros-usbpd-notify", },
>  };
>  
> +static const struct mfd_cell cros_ec_wdt_cells[] = {
> +	{ .name = "cros-ec-wdt-drv", }
> +};
> +
>  static const struct cros_feature_to_cells cros_subdevices[] = {
>  	{
>  		.id		= EC_FEATURE_CEC,
> @@ -107,6 +111,11 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
>  		.mfd_cells	= cros_usbpd_charger_cells,
>  		.num_cells	= ARRAY_SIZE(cros_usbpd_charger_cells),
>  	},
> +	{
> +		.id		= EC_FEATURE_HANG_DETECT,
> +		.mfd_cells	= cros_ec_wdt_cells,
> +		.num_cells	= ARRAY_SIZE(cros_ec_wdt_cells),
> +	},
>  };
>  
>  static const struct mfd_cell cros_ec_platform_cells[] = {
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..1da4be661be8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2251,4 +2251,19 @@ config KEEMBAY_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called keembay_wdt.
>  
> +#
> +# ChromeOS EC-based Watchdog
> +#

Drop comment, useless, copies what's below.

> +
> +config CROS_EC_WATCHDOG
> +	tristate "ChromeOS EC-based watchdog driver"
> +	select WATCHDOG_CORE
> +	depends on CROS_EC
> +	help
> +	  This is the watchdog driver for ChromeOS devices equipped with EC.
> +	  The EC watchdog - when enabled - expects to be kicked within a given
> +	  time (default set to 30 seconds) otherwise it will simple reboot
> +	  the AP. Priori to that it can also send an event (configurable timeout)
> +	  about upcoming reboot.

Instead you could say what will be the name of the module.

> +
>  endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 7cbc34514ec1..8295c209ddb0 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -234,3 +234,6 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
>  obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
>  obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
>  obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> +
> +# Cros EC watchdog

Drop comment.

Also, are you sure you placed it in appropriate place, not just at the
end of both files?

> +obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o
> diff --git a/drivers/watchdog/cros_ec_wdt.c b/drivers/watchdog/cros_ec_wdt.c
> new file mode 100644
> index 000000000000..b461c0613269
> --- /dev/null
> +++ b/drivers/watchdog/cros_ec_wdt.c
> @@ -0,0 +1,303 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/uaccess.h>
> +
> +#define CROS_EC_WATCHDOG_DEFAULT_TIME 30 /* seconds */
> +
> +#define DEV_NAME "cros-ec-wdt-dev"

Drop unused defines.

> +#define DRV_NAME "cros-ec-wdt-drv"
> +
> +static int cros_ec_wdt_ping(struct watchdog_device *wdd);
> +static int cros_ec_wdt_start(struct watchdog_device *wdd);
> +static int cros_ec_wdt_stop(struct watchdog_device *wdd);
> +static int cros_ec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int t);
> +

..

> +
> +static int cros_ec_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	/* We need to get a reference to cros_ec_devices
> +	 * (provides communication layer) which is a parent of
> +	 * the cros-ec-dev (our parent)
> +	 */
> +	struct cros_ec_device *cros_ec = dev_get_drvdata(dev->parent->parent);
> +	int ret = 0;
> +	uint32_t bootstatus;
> +
> +	if (!cros_ec) {
> +		ret = -ENODEV;
> +		dev_err_probe(dev, ret, "There is no coresponding EC device!");

Syntax is return dev_err_probe

> +		return ret;
> +	}
> +
> +	cros_ec_wdd.parent = &pdev->dev;
> +	wd_data.cros_ec = cros_ec;
> +	wd_data.wdd = &cros_ec_wdd;
> +	wd_data.start_on_resume = false;
> +	wd_data.keepalive_on = false;
> +	wd_data.wdd->timeout = CROS_EC_WATCHDOG_DEFAULT_TIME;
> +
> +	watchdog_stop_on_reboot(&cros_ec_wdd);
> +	watchdog_stop_on_unregister(&cros_ec_wdd);
> +	watchdog_set_drvdata(&cros_ec_wdd, &wd_data);
> +	platform_set_drvdata(pdev, &wd_data);
> +
> +	/* Get current AP boot status */
> +	ret = cros_ec_wdt_send_hang_detect(&wd_data, EC_HANG_DETECT_CMD_GET_STATUS, 0,
> +					   &bootstatus);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Couldn't get AP boot status from EC");

Syntax is return dev_err_probe

> +		return ret;
> +	}
> +
> +	/*
> +	 * If bootstatus is not EC_HANG_DETECT_AP_BOOT_NORMAL
> +	 * it mens EC has rebooted the AP due to watchdog timeout.
> +	 * Lets translate it to watchdog core status code.
> +	 */
> +	if (bootstatus != EC_HANG_DETECT_AP_BOOT_NORMAL)
> +		wd_data.wdd->bootstatus = WDIOF_CARDRESET;
> +
> +	ret = watchdog_register_device(&cros_ec_wdd);
> +	if (ret < 0)
> +		dev_err_probe(dev, ret, "Couldn't get AP boot status from EC");

Syntax is return dev_err_probe

> +
> +	return ret;

return 0

> +}
> +
> +static int cros_ec_wdt_remove(struct platform_device *pdev)
> +{
> +	struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(wd_data->wdd);
> +
> +	return 0;
> +}
> +
> +static void cros_ec_wdt_shutdown(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	/*
> +	 * Clean only bootstatus flag.
> +	 * EC wdt is are already stopped by watchdog framework.
> +	 */
> +	ret = cros_ec_wdt_send_hang_detect(wd_data,
> +					   EC_HANG_DETECT_CMD_CLEAR_STATUS, 0, NULL);
> +	if (ret < 0)
> +		dev_err(dev, "%s failed (%d)", __func__, ret);
> +
> +	watchdog_unregister_device(wd_data->wdd);
> +}
> +
> +static int __maybe_unused cros_ec_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	if (watchdog_active(wd_data->wdd)) {
> +		ret = cros_ec_wdt_send_hang_detect(wd_data,
> +						   EC_HANG_DETECT_CMD_CANCEL, 0, NULL);
> +		if (ret < 0)
> +			dev_err(dev, "%s failed (%d)", __func__, ret);
> +		wd_data->start_on_resume = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused cros_ec_wdt_resume(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	/* start_on_resume is only set if watchdog was active
> +	 * when device was going to sleep
> +	 */
> +	if (wd_data->start_on_resume) {
> +		/* On resume we just need to setup a EC watchdog the same way as

Use comment style from Linux coding style.

> +		 * in cros_ec_wdt_start(). When userspace resumes from suspend
> +		 * the watchdog app should just start petting the watchdog again.
> +		 */
> +		ret = cros_ec_wdt_start(wd_data->wdd);
> +		if (ret < 0)
> +			dev_err(dev, "%s failed (%d)", __func__, ret);

That's not really helpful. This applies everywhere. You have all over
the place debugging prints with __func__. This is not useful pattern.
Print something useful, not function name.

> +
> +		wd_data->start_on_resume = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cros_ec_wdt_driver = {
> +	.probe		= cros_ec_wdt_probe,
> +	.remove		= cros_ec_wdt_remove,
> +	.shutdown	= cros_ec_wdt_shutdown,
> +	.suspend	= pm_ptr(cros_ec_wdt_suspend),
> +	.resume		= pm_ptr(cros_ec_wdt_resume),
> +	.driver		= {
> +		.name	= DRV_NAME,
> +	},
> +};
> +
> +module_platform_driver(cros_ec_wdt_driver);
> +
> +MODULE_ALIAS("platform:" DRV_NAME);

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


> +MODULE_DESCRIPTION("Cros EC Watchdog Device Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 7dae17b62a4d..35a7a2d32819 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -3961,60 +3961,50 @@ struct ec_response_i2c_passthru {
>  } __ec_align1;
>  
>  /*****************************************************************************/
> -/* Power button hang detect */
> -
> +/* AP hang detect */

I don't understand how this change is related to the new driver. This
entire hunk is confusing.


Best regards,
Krzysztof

Powered by blists - more mailing lists