| 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: <Ye8Ym9Ie7UOp0/jY@chromium.org>
Date: Mon, 24 Jan 2022 21:22:35 +0000
From: Prashant Malani <pmalani@...omium.org>
To: Daisuke Nojiri <dnojiri@...omium.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Sebastian Reichel <sre@...nel.org>,
Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Stephen Boyd <swboyd@...omium.org>, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2] power: supply: PCHG: Use MKBP for device event
handling
Hi Daisuke,
On Jan 23 17:11, Daisuke Nojiri wrote:
> 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@...omium.org>
> ---
> v1 -> v2
> * Make the patch description concise.
> * Change the order of if-conditions in cros_ec_notify.
> ---
> .../power/supply/cros_peripheral_charger.c | 35 ++--------
> .../linux/platform_data/cros_ec_commands.h | 64 +++++++++++++++++++
In the past, the maintainer here has requested the header update to be
in a separate patch. Perhaps we should follow that format.
> 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;
Do we need to introduce these structs if they are not being used anywhere?
This header isn't auto-generated AFAIK.
If they *are* being used, could you kindly point to where they are being
used?
Thanks,
Powered by blists - more mailing lists