lkml.org 
[lkml]   [2019]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3] platform/chrome: wilco_ec: Add h1_gpio status to debugfs
Hi Nick,

Some comments below ...

Missatge de Nick Crews <ncrews@chromium.org> del dia dj., 11 d’abr.
2019 a les 0:09:
>
> As part of Chrome OS's FAFT (Fully Automated Firmware Testing)
> tests, we need to ensure that the H1 chip is properly setting
> some GPIO lines. The h1_gpio attribute exposes the state
> of the lines:
> - ENTRY_TO_FACT_MODE in BIT(0)
> - SPI_CHROME_SEL in BIT(1)
>
> There are two reasons that I am exposing this in debugfs,
> and not as a GPIO:
> 1. This is only useful for testing, so end users shouldn't ever
> care about this. In fact, if it passes the tests, then the value of
> h1_gpio will always be 2, so it would be really uninteresting for users.
> 2. This GPIO is not connected to, controlled by, or really even related
> to the AP. The GPIO runs between the EC and the H1 security chip.
>
> Changes in v3:
> - Fix documentation to correspond with formatting change in v2.
> Changes in v2:
> - Zero out the unused fields in the request.
> - Format result as "%02x\n" instead of as a decimal.

I don't really mind, but wouldn't be more clear prefix with 0x (0x%02x)?

>
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
> drivers/platform/chrome/wilco_ec/debugfs.c | 64 +++++++++++++++++++++-

ABI documentation missing.

> 1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/wilco_ec/debugfs.c b/drivers/platform/chrome/wilco_ec/debugfs.c
> index 17c4c9068aaf..1b93243c8954 100644
> --- a/drivers/platform/chrome/wilco_ec/debugfs.c
> +++ b/drivers/platform/chrome/wilco_ec/debugfs.c
> @@ -4,7 +4,7 @@
> *
> * Copyright 2019 Google LLC
> *
> - * There is only one attribute used for debugging, called raw.
> + * The raw attribute is very useful for EC debugging.
> * You can write a hexadecimal sentence to raw, and that series of bytes
> * will be sent to the EC. Then, you can read the bytes of response
> * by reading from raw.
> @@ -30,6 +30,16 @@
> * Note that the first 32 bytes of the received MBOX[] will be printed,
> * even if some of the data is junk. It is up to you to know how many of
> * the first bytes of data are the actual response.
> + *
> + * There is also another debugfs attribute, called h1_gpio.
> + * As part of Chrome OS's FAFT (Fully Automated Firmware Testing)
> + * tests, we need to ensure that the H1 chip is properly setting
> + * some GPIO lines. The h1_gpio attribute exposes the state
> + * of the lines:
> + * - ENTRY_TO_FACT_MODE in BIT(0)
> + * - SPI_CHROME_SEL in BIT(1)
> +
> + * Output will formatted with "%02x\n".
> */
>
> #include <linux/ctype.h>
> @@ -189,6 +199,56 @@ static const struct file_operations fops_raw = {
> .llseek = no_llseek,
> };
>
> +#define CMD_KB_CHROME 0x88
> +#define SUB_CMD_H1_GPIO 0x0A
> +
> +struct h1_gpio_status_request {
> + u8 cmd; /* Always CMD_KB_CHROME */
> + u8 reserved;
> + u8 sub_cmd; /* Always SUB_CMD_H1_GPIO */
> +} __packed;
> +
> +struct hi_gpio_status_response {
> + u8 status; /* 0 if allowed */
> + u8 val; /* BIT(0)=ENTRY_TO_FACT_MODE, BIT(1)=SPI_CHROME_SEL */
> +} __packed;
> +
> +static ssize_t h1_gpio_read(struct file *file, char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct h1_gpio_status_request rq;
> + struct hi_gpio_status_response rs;
> + struct wilco_ec_message msg;
> + char buf[4];
> + int ret;
> +
> + memset(&rq, 0, sizeof(rq));
> + rq.cmd = CMD_KB_CHROME;
> + rq.sub_cmd = SUB_CMD_H1_GPIO;
> +
> + memset(&msg, 0, sizeof(msg));
> + msg.type = WILCO_EC_MSG_LEGACY;
> + msg.request_data = &rq;
> + msg.request_size = sizeof(rq);
> + msg.response_data = &rs;
> + msg.response_size = sizeof(rs);
> + ret = wilco_ec_mailbox(debug_info->ec, &msg);
> + if (ret < 0)
> + return ret;
> + if (rs.status)
> + return -EIO;
> +
> + sprintf(buf, "%02x\n", rs.val);
> +
> + return simple_read_from_buffer(user_buf, count, ppos, buf, sizeof(buf));
> +}
> +
> +static const struct file_operations fops_h1_gpio = {
> + .owner = THIS_MODULE,
> + .read = h1_gpio_read,
> + .llseek = no_llseek,
> +};
> +

I think that I'd use the DEFINE_DEBUGFS_ATTRIBUTE here.

> /**
> * wilco_ec_debugfs_probe() - Create the debugfs node
> * @pdev: The platform device, probably created in core.c
> @@ -210,6 +270,8 @@ static int wilco_ec_debugfs_probe(struct platform_device *pdev)
> if (!debug_info->dir)
> return 0;
> debugfs_create_file("raw", 0644, debug_info->dir, NULL, &fops_raw);
> + debugfs_create_file("h1_gpio", 0444, debug_info->dir, NULL,
> + &fops_h1_gpio);
>
> return 0;
> }
> --
> 2.20.1
>

Thanks,
Enric

\
 
 \ /
  Last update: 2019-04-11 23:44    [W:0.048 / U:50.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site