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@xxxxxxxxxxxx>
> ---
> 
>  .../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