| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a24155a3-281b-433d-9964-eedc40ae2bf8@roeck-us.net>
Date: Thu, 10 Apr 2025 05:25:00 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Yen-Chi Huang <jesse.huang@...twell.com.tw>
Cc: Hans de Goede <hdegoede@...hat.com>, linus.walleij@...aro.org,
brgl@...ev.pl, wim@...ux-watchdog.org, LKML <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-watchdog@...r.kernel.org, jay.chen@...onical.com
Subject: Re: [PATCH v3] platform/x86: portwell-ec: Add GPIO and WDT driver for
Portwell EC
On 4/10/25 05:07, Ilpo Järvinen wrote:
> On Thu, 10 Apr 2025, Yen-Chi Huang wrote:
>
>> Adds a driver for the ITE Embedded Controller (EC) on Portwell boards.
>> It integrates with the Linux GPIO and watchdog subsystems to provide:
>>
>> - Control/monitoring of up to 8 EC GPIO pins.
>> - Hardware watchdog timer with 1-255 second timeouts.
>>
>> The driver communicates with the EC via I/O port 0xe300 and identifies
>> the hardware by the "PWG" firmware signature. This enables enhanced
>> system management for Portwell embedded/industrial platforms.
>>
>> Signed-off-by: Yen-Chi Huang <jesse.huang@...twell.com.tw>
>> ---
>> V2->V3:
>> - Reworked based on review from Bartosz Golaszewski
>> - Changed to use platform_driver and platform_device
>> - Updated GPIO to use .set_rv() instead of .set()
>> - Used devm_ helpers for request_region, GPIO and watchdog registration
>>
>> V1->V2:
>> - Addressed review comments from Ilpo Jarvinen
>> - Add DMI system check to avoid running on unsupported platforms
>> - Add 'force' module parameter to override DMI matching
>> - Use request_region() to claim I/O port access
>> - Extend WDT timeout handling to use both minute and second registers
>> - Increase WDT max timeout from 255s to 15300s
>> - Use named defines for WDT enable/disable
>> - Remove dummy pr_info() messages
>> - Fix several coding style issues (comments, alignment, spacing)
>> ---
>> MAINTAINERS | 6 +
>> drivers/platform/x86/Kconfig | 14 ++
>> drivers/platform/x86/Makefile | 3 +
>> drivers/platform/x86/portwell-ec.c | 292 +++++++++++++++++++++++++++++
>> 4 files changed, 315 insertions(+)
>> create mode 100644 drivers/platform/x86/portwell-ec.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d5dfb9186962..c52f819786dc 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19132,6 +19132,12 @@ F: kernel/time/itimer.c
>> F: kernel/time/posix-*
>> F: kernel/time/namespace.c
>>
>> +PORTWELL EC DRIVER
>> +M: Yen-Chi Huang <jesse.huang@...twell.com.tw>
>> +L: platform-driver-x86@...r.kernel.org
>> +S: Maintained
>> +F: drivers/platform/x86/portwell-ec.c
>> +
>> POWER MANAGEMENT CORE
>> M: "Rafael J. Wysocki" <rafael@...nel.org>
>> L: linux-pm@...r.kernel.org
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 43407e76476b..2f26d1bf0a75 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -779,6 +779,20 @@ config PCENGINES_APU2
>> To compile this driver as a module, choose M here: the module
>> will be called pcengines-apuv2.
>>
>> +config PORTWELL_EC
>> + tristate "Portwell Embedded Controller driver"
>> + depends on X86 && HAS_IOPORT && WATCHDOG && GPIOLIB
>> + help
>> + This driver provides support for the GPIO pins and watchdog timer
>> + embedded in Portwell's EC.
>> +
>> + Theoretically, this driver should work on multiple Portwell platforms,
>> + but it has only been tested on the Portwell NANO-6064 board.
>> + If you encounter any issues on other boards, please report them.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called portwell-ec.
>> +
>> config BARCO_P50_GPIO
>> tristate "Barco P50 GPIO driver for identify LED/button"
>> depends on GPIOLIB
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index 650dfbebb6c8..83dd82e04457 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -92,6 +92,9 @@ obj-$(CONFIG_XO1_RFKILL) += xo1-rfkill.o
>> # PC Engines
>> obj-$(CONFIG_PCENGINES_APU2) += pcengines-apuv2.o
>>
>> +# Portwell
>> +obj-$(CONFIG_PORTWELL_EC) += portwell-ec.o
>> +
>> # Barco
>> obj-$(CONFIG_BARCO_P50_GPIO) += barco-p50-gpio.o
>>
>> diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
>> new file mode 100644
>> index 000000000000..7a60ced0c984
>> --- /dev/null
>> +++ b/drivers/platform/x86/portwell-ec.c
>> @@ -0,0 +1,292 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * portwell-ec.c: Portwell embedded controller driver.
>> + *
>> + * Tested on:
>> + * - Portwell NANO-6064
>> + *
>> + * This driver provides support for GPIO and Watchdog Timer
>> + * functionalities of the Portwell boards with ITE embedded controller (EC).
>> + * The EC is accessed through I/O ports and provides:
>> + * - 8 GPIO pins for control and monitoring
>> + * - Hardware watchdog with 1-15300 second timeout range
>> + *
>> + * It integrates with the Linux GPIO and Watchdog subsystems, allowing
>> + * userspace interaction with EC GPIO pins and watchdog control,
>> + * ensuring system stability and configurability.
>> + *
>> + * (C) Copyright 2025 Portwell, Inc.
>> + * Author: Yen-Chi Huang (jesse.huang@...twell.com.tw)
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/dmi.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/ioport.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/string.h>
>> +#include <linux/watchdog.h>
>> +
>> +#define PORTWELL_EC_IOSPACE 0xe300
>> +#define PORTWELL_EC_IOSPACE_LEN 0x100
>
> SZ_256 + add #include for it.
>
>> +
>> +#define PORTWELL_GPIO_PINS 8
>> +#define PORTWELL_GPIO_DIR_REG 0x2b
>> +#define PORTWELL_GPIO_VAL_REG 0x2c
>> +
>> +#define PORTWELL_WDT_EC_CONFIG_ADDR 0x06
>> +#define PORTWELL_WDT_CONFIG_ENABLE 0x1
>> +#define PORTWELL_WDT_CONFIG_DISABLE 0x0
>
> Align values.
>
>> +#define PORTWELL_WDT_EC_COUNT_MIN_ADDR 0x07
>> +#define PORTWELL_WDT_EC_COUNT_SEC_ADDR 0x08
>> +#define PORTWELL_WDT_EC_MAX_COUNT_SECOND 15300 //255*60secs
>
> Move the formula from the comment to the define itself. While doing so,
> you need to add () around it and add spaces around *.
>
>> +
>> +#define PORTWELL_EC_FW_VENDOR_ADDRESS 0x4d
>> +#define PORTWELL_EC_FW_VENDOR_LENGTH 3
>> +#define PORTWELL_EC_FW_VENDOR_NAME "PWG"
>> +
>> +static bool force;
>> +module_param(force, bool, 0444);
>> +MODULE_PARM_DESC(force, "Force loading ec driver without checking DMI boardname");
>
> EC
>
>> +static const struct dmi_system_id pwec_dmi_table[] = {
>> + {
>> + .ident = "NANO-6064 series",
>> + .matches = {
>> + DMI_MATCH(DMI_BOARD_NAME, "NANO-6064"),
>> + },
>> + },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(dmi, pwec_dmi_table);
>> +
>> +/* Functions for access EC via IOSPACE*/
>> +
>> +static void pwec_write(u8 index, u8 data)
>> +{
>> + outb(data, PORTWELL_EC_IOSPACE + index);
>> +}
>> +
>> +static u8 pwec_read(u8 address)
>> +{
>> + return inb(PORTWELL_EC_IOSPACE + address);
>> +}
>> +
>> +/* GPIO functions*/
>
> Missing space. Please check all your comments as the one above seems to
> have the same lack of space at the end.
>
>> +
>> +static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> + return (pwec_read(PORTWELL_GPIO_VAL_REG) & (1 << offset)) ? 1 : 0;
>> +}
>> +
>> +static int pwec_gpio_set_rv(struct gpio_chip *chip, unsigned int offset, int val)
>> +{
>> + u8 tmp = pwec_read(PORTWELL_GPIO_VAL_REG);
>> +
>> + if (val)
>> + tmp |= (1 << offset);
>> + else
>> + tmp &= ~(1 << offset);
>> + pwec_write(PORTWELL_GPIO_VAL_REG, tmp);
>
> Add empty line here.
>
>> + return 0;
>> +}
>> +
>> +static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>> +{
>> + u8 direction = pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset);
>> +
>> + if (direction)
>> + return GPIO_LINE_DIRECTION_IN;
>> + else
>> + return GPIO_LINE_DIRECTION_OUT;
>> +}
>> +
>> +/*
>> + * Changing direction causes issues on some boards,
>> + * so direction_input and direction_output are disabled for now.
>> + */
>> +
>> +static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +static struct gpio_chip pwec_gpio_chip = {
>> + .label = "portwell-ec-gpio",
>> + .get_direction = pwec_gpio_get_direction,
>> + .direction_input = pwec_gpio_direction_input,
>> + .direction_output = pwec_gpio_direction_output,
>> + .get = pwec_gpio_get,
>> + .set_rv = pwec_gpio_set_rv,
>> + .base = -1,
>> + .ngpio = PORTWELL_GPIO_PINS,
>> +};
>> +
>> +/* Watchdog functions*/
>> +
>> +static int pwec_wdt_trigger(struct watchdog_device *wdd)
>> +{
>> + int retry = 10;
>> + u8 min, sec;
>> + unsigned int timeout;
>> +
>> + do {
>> + pwec_write(PORTWELL_WDT_EC_COUNT_MIN_ADDR, wdd->timeout / 60);
>> + pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout % 60);
>> + pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, PORTWELL_WDT_CONFIG_ENABLE);
>> + min = pwec_read(PORTWELL_WDT_EC_COUNT_MIN_ADDR);
>> + sec = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
>> + timeout = min * 60 + sec;
>
> This readback could reuse pwec_wdt_get_timeleft().
>
>> + retry--;
>
> Decrementing could be done within the condition that checks it.
>
>> + } while (timeout != wdd->timeout && retry >= 0);
>
> Is this write until timeout matches the one written typical thing for
> watchdog drivers, or is there something specific to this HW you should
> comment + note in the changelog so it is recorded for future readers of
> this code?
>
No, this is absolutely not typical, and it has nothing to do with watchdog
drivers in the first place. If the code was in drivers/watchdog/ I'd
request a detailed comment explaining why it is needed.
Guenter
Powered by blists - more mailing lists