[PATCH] power: supply: PCHG: Use MKBP for device event handling

[PATCH] power: supply: PCHG: Use MKBP for device event handling

[Daisuke Nojiri]

Currently, the peripheral device charger driver receives PCHG
events as device events (EC_HOST_EVENT_DEVICE=10) except ones
related to firmware updates. This is delivered via the Chrome OS
host event protocol.

On X86 systems, the Chrome OS host event protocol is implemented on
ACPI and a ACPI handler for EC_HOST_EVENT_DEVICE (a.k.a. _Q0A)
doesn't exist.

Instead of adding a device event ACPI handler, this patch makes the
PCHG driver receive a device event through the MKBP protocol.

The MKBP protocol is already used for firmware update for PCHG
devices.  So, this allows us to unite all PCHG event handling via
MKBP on both x86 and ARM platforms.

Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
---

 .../power/supply/cros_peripheral_charger.c    | 35 ++--------
 .../linux/platform_data/cros_ec_commands.h    | 64 +++++++++++++++++++
 2 files changed, 70 insertions(+), 29 deletions(-)

diff --git a/drivers/power/supply/cros_peripheral_charger.c b/drivers/power/supply/cros_peripheral_charger.c
index 305f10dfc06d1b..d1c45af2c50638 100644
--- a/drivers/power/supply/cros_peripheral_charger.c
+++ b/drivers/power/supply/cros_peripheral_charger.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/stringify.h>
 #include <linux/types.h>
+#include <asm/unaligned.h>
 
 #define DRV_NAME		"cros-ec-pchg"
 #define PCHG_DIR_PREFIX		"peripheral"
@@ -237,46 +238,22 @@ static int cros_pchg_event(const struct charger_data *charger,
 	return NOTIFY_OK;
 }
 
-static u32 cros_get_device_event(const struct charger_data *charger)
-{
-	struct ec_params_device_event req;
-	struct ec_response_device_event rsp;
-	struct device *dev = charger->dev;
-	int ret;
-
-	req.param = EC_DEVICE_EVENT_PARAM_GET_CURRENT_EVENTS;
-	ret = cros_pchg_ec_command(charger, 0, EC_CMD_DEVICE_EVENT,
-				   &req, sizeof(req), &rsp, sizeof(rsp));
-	if (ret < 0) {
-		dev_warn(dev, "Unable to get device events (err:%d)\n", ret);
-		return 0;
-	}
-
-	return rsp.event_mask;
-}
-
 static int cros_ec_notify(struct notifier_block *nb,
 			  unsigned long queued_during_suspend,
 			  void *data)
 {
 	struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
-	u32 host_event = cros_ec_get_host_event(ec_dev);
 	struct charger_data *charger =
 			container_of(nb, struct charger_data, notifier);
-	u32 device_event_mask;
+	u32 host_event;
 
-	if (!host_event)
+	if (ec_dev->event_size != sizeof(host_event))
 		return NOTIFY_DONE;
 
-	if (!(host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_DEVICE)))
-		return NOTIFY_DONE;
+	host_event = get_unaligned_le32(&ec_dev->event_data.data.host_event);
 
-	/*
-	 * todo: Retrieve device event mask in common place
-	 * (e.g. cros_ec_proto.c).
-	 */
-	device_event_mask = cros_get_device_event(charger);
-	if (!(device_event_mask & EC_DEVICE_EVENT_MASK(EC_DEVICE_EVENT_WLC)))
+	if (ec_dev->event_data.event_type != EC_MKBP_EVENT_PCHG
+			|| !(host_event & EC_MKBP_PCHG_DEVICE_EVENT))
 		return NOTIFY_DONE;
 
 	return cros_pchg_event(charger, host_event);
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 271bd87bff0a25..c784bed3388865 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -3386,6 +3386,9 @@ enum ec_mkbp_event {
 	/* Send an incoming CEC message to the AP */
 	EC_MKBP_EVENT_CEC_MESSAGE = 9,
 
+	/* Peripheral device charger event */
+	EC_MKBP_EVENT_PCHG = 12,
+
 	/* Number of MKBP events */
 	EC_MKBP_EVENT_COUNT,
 };
@@ -5527,6 +5530,67 @@ enum pchg_state {
 	[PCHG_STATE_CONNECTED] = "CONNECTED", \
 	}
 
+/**
+ * Update firmware of peripheral chip
+ */
+#define EC_CMD_PCHG_UPDATE 0x0136
+
+/* Port number is encoded in bit[28:31]. */
+#define EC_MKBP_PCHG_PORT_SHIFT		28
+/* Utility macro for converting MKBP event to port number. */
+#define EC_MKBP_PCHG_EVENT_TO_PORT(e)	(((e) >> EC_MKBP_PCHG_PORT_SHIFT) & 0xf)
+/* Utility macro for extracting event bits. */
+#define EC_MKBP_PCHG_EVENT_MASK(e)	((e) \
+					& GENMASK(EC_MKBP_PCHG_PORT_SHIFT-1, 0))
+
+#define EC_MKBP_PCHG_UPDATE_OPENED	BIT(0)
+#define EC_MKBP_PCHG_WRITE_COMPLETE	BIT(1)
+#define EC_MKBP_PCHG_UPDATE_CLOSED	BIT(2)
+#define EC_MKBP_PCHG_UPDATE_ERROR	BIT(3)
+#define EC_MKBP_PCHG_DEVICE_EVENT	BIT(4)
+
+enum ec_pchg_update_cmd {
+	/* Reset chip to normal mode. */
+	EC_PCHG_UPDATE_CMD_RESET_TO_NORMAL = 0,
+	/* Reset and put a chip in update (a.k.a. download) mode. */
+	EC_PCHG_UPDATE_CMD_OPEN,
+	/* Write a block of data containing FW image. */
+	EC_PCHG_UPDATE_CMD_WRITE,
+	/* Close update session. */
+	EC_PCHG_UPDATE_CMD_CLOSE,
+	/* End of commands */
+	EC_PCHG_UPDATE_CMD_COUNT,
+};
+
+struct ec_params_pchg_update {
+	/* PCHG port number */
+	uint8_t port;
+	/* enum ec_pchg_update_cmd */
+	uint8_t cmd;
+	/* Padding */
+	uint8_t reserved0;
+	uint8_t reserved1;
+	/* Version of new firmware */
+	uint32_t version;
+	/* CRC32 of new firmware */
+	uint32_t crc32;
+	/* Address in chip memory where <data> is written to */
+	uint32_t addr;
+	/* Size of <data> */
+	uint32_t size;
+	/* Partial data of new firmware */
+	uint8_t data[];
+} __ec_align4;
+
+BUILD_ASSERT(EC_PCHG_UPDATE_CMD_COUNT
+	     < BIT(sizeof(((struct ec_params_pchg_update *)0)->cmd)*8));
+
+struct ec_response_pchg_update {
+	/* Block size */
+	uint32_t block_size;
+} __ec_align4;
+
+
 /*****************************************************************************/
 /* Voltage regulator controls */
 
-- 
2.31.0

Re: [PATCH] power: supply: PCHG: Use MKBP for device event handling

[Dmitry Torokhov]

Hi Daisuke,

On Sun, Jan 23, 2022 at 11:43:13AM -0800, Daisuke Nojiri wrote:
> Currently, the peripheral device charger driver receives PCHG
> events as device events (EC_HOST_EVENT_DEVICE=10) except ones
> related to firmware updates. This is delivered via the Chrome OS
> host event protocol.
> 
> On X86 systems, the Chrome OS host event protocol is implemented on
> ACPI and a ACPI handler for EC_HOST_EVENT_DEVICE (a.k.a. _Q0A)
> doesn't exist.
> 
> Instead of adding a device event ACPI handler, this patch makes the
> PCHG driver receive a device event through the MKBP protocol.
> 
> The MKBP protocol is already used for firmware update for PCHG
> devices.  So, this allows us to unite all PCHG event handling via
> MKBP on both x86 and ARM platforms.

I wonder if we could simply say that we have switched EC to deliver all
peripheral change communication/events to MKBP protocol so that the
handling is unified. We can simply drop the EC_CMD_DEVICE_EVENT handling
because we have not shipped any devices using this mechanism (correct?).

> 
> Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
> ---
> 
>  .../power/supply/cros_peripheral_charger.c    | 35 ++--------
>  .../linux/platform_data/cros_ec_commands.h    | 64 +++++++++++++++++++
>  2 files changed, 70 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/power/supply/cros_peripheral_charger.c b/drivers/power/supply/cros_peripheral_charger.c
> index 305f10dfc06d1b..d1c45af2c50638 100644
> --- a/drivers/power/supply/cros_peripheral_charger.c
> +++ b/drivers/power/supply/cros_peripheral_charger.c
> @@ -14,6 +14,7 @@
>  #include <linux/slab.h>
>  #include <linux/stringify.h>
>  #include <linux/types.h>
> +#include <asm/unaligned.h>
>  
>  #define DRV_NAME		"cros-ec-pchg"
>  #define PCHG_DIR_PREFIX		"peripheral"
> @@ -237,46 +238,22 @@ static int cros_pchg_event(const struct charger_data *charger,
>  	return NOTIFY_OK;
>  }
>  
> -static u32 cros_get_device_event(const struct charger_data *charger)
> -{
> -	struct ec_params_device_event req;
> -	struct ec_response_device_event rsp;
> -	struct device *dev = charger->dev;
> -	int ret;
> -
> -	req.param = EC_DEVICE_EVENT_PARAM_GET_CURRENT_EVENTS;
> -	ret = cros_pchg_ec_command(charger, 0, EC_CMD_DEVICE_EVENT,
> -				   &req, sizeof(req), &rsp, sizeof(rsp));
> -	if (ret < 0) {
> -		dev_warn(dev, "Unable to get device events (err:%d)\n", ret);
> -		return 0;
> -	}
> -
> -	return rsp.event_mask;
> -}
> -
>  static int cros_ec_notify(struct notifier_block *nb,
>  			  unsigned long queued_during_suspend,
>  			  void *data)
>  {
>  	struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
> -	u32 host_event = cros_ec_get_host_event(ec_dev);
>  	struct charger_data *charger =
>  			container_of(nb, struct charger_data, notifier);
> -	u32 device_event_mask;
> +	u32 host_event;
>  
> -	if (!host_event)
> +	if (ec_dev->event_size != sizeof(host_event))
>  		return NOTIFY_DONE;
>  
> -	if (!(host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_DEVICE)))
> -		return NOTIFY_DONE;
> +	host_event = get_unaligned_le32(&ec_dev->event_data.data.host_event);
>  
> -	/*
> -	 * todo: Retrieve device event mask in common place
> -	 * (e.g. cros_ec_proto.c).
> -	 */
> -	device_event_mask = cros_get_device_event(charger);
> -	if (!(device_event_mask & EC_DEVICE_EVENT_MASK(EC_DEVICE_EVENT_WLC)))
> +	if (ec_dev->event_data.event_type != EC_MKBP_EVENT_PCHG

Bikeshedding: I'd check this condition as the very first thing in the
driver, then checked the size as we are doing now, fetched the
host_event data and then checked event against EC_MKBP_PCHG_DEVICE_EVENT
mask.

> +			|| !(host_event & EC_MKBP_PCHG_DEVICE_EVENT))
>  		return NOTIFY_DONE;
>  
>  	return cros_pchg_event(charger, host_event);
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 271bd87bff0a25..c784bed3388865 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -3386,6 +3386,9 @@ enum ec_mkbp_event {
>  	/* Send an incoming CEC message to the AP */
>  	EC_MKBP_EVENT_CEC_MESSAGE = 9,
>  
> +	/* Peripheral device charger event */
> +	EC_MKBP_EVENT_PCHG = 12,
> +
>  	/* Number of MKBP events */
>  	EC_MKBP_EVENT_COUNT,
>  };
> @@ -5527,6 +5530,67 @@ enum pchg_state {
>  	[PCHG_STATE_CONNECTED] = "CONNECTED", \
>  	}
>  
> +/**
> + * Update firmware of peripheral chip
> + */
> +#define EC_CMD_PCHG_UPDATE 0x0136
> +
> +/* Port number is encoded in bit[28:31]. */
> +#define EC_MKBP_PCHG_PORT_SHIFT		28
> +/* Utility macro for converting MKBP event to port number. */
> +#define EC_MKBP_PCHG_EVENT_TO_PORT(e)	(((e) >> EC_MKBP_PCHG_PORT_SHIFT) & 0xf)
> +/* Utility macro for extracting event bits. */
> +#define EC_MKBP_PCHG_EVENT_MASK(e)	((e) \
> +					& GENMASK(EC_MKBP_PCHG_PORT_SHIFT-1, 0))
> +
> +#define EC_MKBP_PCHG_UPDATE_OPENED	BIT(0)
> +#define EC_MKBP_PCHG_WRITE_COMPLETE	BIT(1)
> +#define EC_MKBP_PCHG_UPDATE_CLOSED	BIT(2)
> +#define EC_MKBP_PCHG_UPDATE_ERROR	BIT(3)
> +#define EC_MKBP_PCHG_DEVICE_EVENT	BIT(4)
> +
> +enum ec_pchg_update_cmd {
> +	/* Reset chip to normal mode. */
> +	EC_PCHG_UPDATE_CMD_RESET_TO_NORMAL = 0,
> +	/* Reset and put a chip in update (a.k.a. download) mode. */
> +	EC_PCHG_UPDATE_CMD_OPEN,
> +	/* Write a block of data containing FW image. */
> +	EC_PCHG_UPDATE_CMD_WRITE,
> +	/* Close update session. */
> +	EC_PCHG_UPDATE_CMD_CLOSE,
> +	/* End of commands */
> +	EC_PCHG_UPDATE_CMD_COUNT,
> +};
> +
> +struct ec_params_pchg_update {
> +	/* PCHG port number */
> +	uint8_t port;
> +	/* enum ec_pchg_update_cmd */
> +	uint8_t cmd;
> +	/* Padding */
> +	uint8_t reserved0;
> +	uint8_t reserved1;
> +	/* Version of new firmware */
> +	uint32_t version;
> +	/* CRC32 of new firmware */
> +	uint32_t crc32;
> +	/* Address in chip memory where <data> is written to */
> +	uint32_t addr;
> +	/* Size of <data> */
> +	uint32_t size;
> +	/* Partial data of new firmware */
> +	uint8_t data[];
> +} __ec_align4;
> +
> +BUILD_ASSERT(EC_PCHG_UPDATE_CMD_COUNT
> +	     < BIT(sizeof(((struct ec_params_pchg_update *)0)->cmd)*8));
> +
> +struct ec_response_pchg_update {
> +	/* Block size */
> +	uint32_t block_size;
> +} __ec_align4;
> +
> +
>  /*****************************************************************************/
>  /* Voltage regulator controls */
>  
> -- 
> 2.31.0
> 

Thanks.

-- 
Dmitry

[PATCH] power: supply: PCHG: Use MKBP for device event handling

[Daisuke Nojiri]

This patch makes the PCHG driver receive device events through
MKBP protocol since CrOS EC switched to deliver all peripheral
charge events to the MKBP protocol. This will unify PCHG event
handling on X86 and ARM.

Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
---
 .../power/supply/cros_peripheral_charger.c    | 35 ++--------
 .../linux/platform_data/cros_ec_commands.h    | 64 +++++++++++++++++++
 2 files changed, 70 insertions(+), 29 deletions(-)

diff --git a/drivers/power/supply/cros_peripheral_charger.c b/drivers/power/supply/cros_peripheral_charger.c
index 305f10dfc06d1b..cb402f48087ddf 100644
--- a/drivers/power/supply/cros_peripheral_charger.c
+++ b/drivers/power/supply/cros_peripheral_charger.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/stringify.h>
 #include <linux/types.h>
+#include <asm/unaligned.h>
 
 #define DRV_NAME		"cros-ec-pchg"
 #define PCHG_DIR_PREFIX		"peripheral"
@@ -237,46 +238,22 @@ static int cros_pchg_event(const struct charger_data *charger,
 	return NOTIFY_OK;
 }
 
-static u32 cros_get_device_event(const struct charger_data *charger)
-{
-	struct ec_params_device_event req;
-	struct ec_response_device_event rsp;
-	struct device *dev = charger->dev;
-	int ret;
-
-	req.param = EC_DEVICE_EVENT_PARAM_GET_CURRENT_EVENTS;
-	ret = cros_pchg_ec_command(charger, 0, EC_CMD_DEVICE_EVENT,
-				   &req, sizeof(req), &rsp, sizeof(rsp));
-	if (ret < 0) {
-		dev_warn(dev, "Unable to get device events (err:%d)\n", ret);
-		return 0;
-	}
-
-	return rsp.event_mask;
-}
-
 static int cros_ec_notify(struct notifier_block *nb,
 			  unsigned long queued_during_suspend,
 			  void *data)
 {
 	struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
-	u32 host_event = cros_ec_get_host_event(ec_dev);
 	struct charger_data *charger =
 			container_of(nb, struct charger_data, notifier);
-	u32 device_event_mask;
+	u32 host_event;
 
-	if (!host_event)
+	if (ec_dev->event_data.event_type != EC_MKBP_EVENT_PCHG
+			|| ec_dev->event_size != sizeof(host_event))
 		return NOTIFY_DONE;
 
-	if (!(host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_DEVICE)))
-		return NOTIFY_DONE;
+	host_event = get_unaligned_le32(&ec_dev->event_data.data.host_event);
 
-	/*
-	 * todo: Retrieve device event mask in common place
-	 * (e.g. cros_ec_proto.c).
-	 */
-	device_event_mask = cros_get_device_event(charger);
-	if (!(device_event_mask & EC_DEVICE_EVENT_MASK(EC_DEVICE_EVENT_WLC)))
+	if (!(host_event & EC_MKBP_PCHG_DEVICE_EVENT))
 		return NOTIFY_DONE;
 
 	return cros_pchg_event(charger, host_event);
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 271bd87bff0a25..c784bed3388865 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -3386,6 +3386,9 @@ enum ec_mkbp_event {
 	/* Send an incoming CEC message to the AP */
 	EC_MKBP_EVENT_CEC_MESSAGE = 9,
 
+	/* Peripheral device charger event */
+	EC_MKBP_EVENT_PCHG = 12,
+
 	/* Number of MKBP events */
 	EC_MKBP_EVENT_COUNT,
 };
@@ -5527,6 +5530,67 @@ enum pchg_state {
 	[PCHG_STATE_CONNECTED] = "CONNECTED", \
 	}
 
+/**
+ * Update firmware of peripheral chip
+ */
+#define EC_CMD_PCHG_UPDATE 0x0136
+
+/* Port number is encoded in bit[28:31]. */
+#define EC_MKBP_PCHG_PORT_SHIFT		28
+/* Utility macro for converting MKBP event to port number. */
+#define EC_MKBP_PCHG_EVENT_TO_PORT(e)	(((e) >> EC_MKBP_PCHG_PORT_SHIFT) & 0xf)
+/* Utility macro for extracting event bits. */
+#define EC_MKBP_PCHG_EVENT_MASK(e)	((e) \
+					& GENMASK(EC_MKBP_PCHG_PORT_SHIFT-1, 0))
+
+#define EC_MKBP_PCHG_UPDATE_OPENED	BIT(0)
+#define EC_MKBP_PCHG_WRITE_COMPLETE	BIT(1)
+#define EC_MKBP_PCHG_UPDATE_CLOSED	BIT(2)
+#define EC_MKBP_PCHG_UPDATE_ERROR	BIT(3)
+#define EC_MKBP_PCHG_DEVICE_EVENT	BIT(4)
+
+enum ec_pchg_update_cmd {
+	/* Reset chip to normal mode. */
+	EC_PCHG_UPDATE_CMD_RESET_TO_NORMAL = 0,
+	/* Reset and put a chip in update (a.k.a. download) mode. */
+	EC_PCHG_UPDATE_CMD_OPEN,
+	/* Write a block of data containing FW image. */
+	EC_PCHG_UPDATE_CMD_WRITE,
+	/* Close update session. */
+	EC_PCHG_UPDATE_CMD_CLOSE,
+	/* End of commands */
+	EC_PCHG_UPDATE_CMD_COUNT,
+};
+
+struct ec_params_pchg_update {
+	/* PCHG port number */
+	uint8_t port;
+	/* enum ec_pchg_update_cmd */
+	uint8_t cmd;
+	/* Padding */
+	uint8_t reserved0;
+	uint8_t reserved1;
+	/* Version of new firmware */
+	uint32_t version;
+	/* CRC32 of new firmware */
+	uint32_t crc32;
+	/* Address in chip memory where <data> is written to */
+	uint32_t addr;
+	/* Size of <data> */
+	uint32_t size;
+	/* Partial data of new firmware */
+	uint8_t data[];
+} __ec_align4;
+
+BUILD_ASSERT(EC_PCHG_UPDATE_CMD_COUNT
+	     < BIT(sizeof(((struct ec_params_pchg_update *)0)->cmd)*8));
+
+struct ec_response_pchg_update {
+	/* Block size */
+	uint32_t block_size;
+} __ec_align4;
+
+
 /*****************************************************************************/
 /* Voltage regulator controls */
 
-- 
2.31.0