Thanks for the comments Enric! I'll resend in a day or two. On Thu, Apr 11, 2019 at 3:43 PM Enric Balletbo Serra <[email protected]> wrote: > > Hi Nick, > > Some comments below ... > > Missatge de Nick Crews <[email protected]> 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)?
I figured this would be easier for a program to parse, but you're right it would be more clear. I can change it. > > > > > Signed-off-by: Nick Crews <[email protected]> > > --- > > drivers/platform/chrome/wilco_ec/debugfs.c | 64 +++++++++++++++++++++- > > ABI documentation missing. Whoops, will do. The Documentation is out of date with the raw attribute as well, so I'll fix that too. > > > 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. Will do. > > > /** > > * 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
