Re: [PATCH] cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT
Hi Alper,
On Mon, 9 Nov 2020 at 17:55, Alper Nebi Yasak wrote:
>
> On 09/11/2020 22:37, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 9 Nov 2020 at 12:34, Heinrich Schuchardt wrote:
> >>
> >> On 10/30/20 6:25 PM, Alper Nebi Yasak wrote:
> >>> The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the
> >>> keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT
> >>
> >> This patch has been applied to origin/master.
> >>
> >> Now when I compile sandbox_defconfig and run './u-boot -D' it spits out
> >> zillions of
> >>
> >>** Unknown EC command 0x67
> >>** Unknown EC command 0x67
> >>** Unknown EC command 0x67
> >>** Unknown EC command 0x67
> >>** Unknown EC command 0x67
> >>** Unknown EC command 0x67
> >>** Unknown EC command 0x67
> >>** Unknown EC command 0x67
> >>
> >> When I revert the patch the messages are gone.
> >>
> >> So something is really missing in this patch.
> >
> > Test coverage, for a start!
> >
> > I think the EC emulator needs to be updated for the new command.
> >
> > Alper, can you please take a look?
> >
>
> I can get the messages to stop with the following, does it look good to
> you?
>
> case EC_CMD_ENTERING_MODE:
> len = 0;
> break;
> + case EC_CMD_GET_NEXT_EVENT: {
> + struct ec_response_get_next_event *resp = resp_data;
> + resp->event_type = EC_MKBP_EVENT_KEY_MATRIX;
> + cros_ec_keyscan(ec, resp->data.key_matrix);
> + len = sizeof(*resp);
> + break;
> + }
> default:
> printf(" ** Unknown EC command %#02x\n", req_hdr->command);
> return -1;
>
> That's more or less only what the cros-ec-keyb counterpart expects. But
> it doesn't test the -EC_RES_UNAVAILABLE thing or the fallback to the old
> method.
Yes that looks good. We don't need to test the fallback for now as
this is just an emulator anyway.
Regards,
Simon
Re: [PATCH] cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT
On 09/11/2020 22:37, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 9 Nov 2020 at 12:34, Heinrich Schuchardt wrote:
>>
>> On 10/30/20 6:25 PM, Alper Nebi Yasak wrote:
>>> The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the
>>> keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT
>>
>> This patch has been applied to origin/master.
>>
>> Now when I compile sandbox_defconfig and run './u-boot -D' it spits out
>> zillions of
>>
>>** Unknown EC command 0x67
>>** Unknown EC command 0x67
>>** Unknown EC command 0x67
>>** Unknown EC command 0x67
>>** Unknown EC command 0x67
>>** Unknown EC command 0x67
>>** Unknown EC command 0x67
>>** Unknown EC command 0x67
>>
>> When I revert the patch the messages are gone.
>>
>> So something is really missing in this patch.
>
> Test coverage, for a start!
>
> I think the EC emulator needs to be updated for the new command.
>
> Alper, can you please take a look?
>
I can get the messages to stop with the following, does it look good to
you?
case EC_CMD_ENTERING_MODE:
len = 0;
break;
+ case EC_CMD_GET_NEXT_EVENT: {
+ struct ec_response_get_next_event *resp = resp_data;
+ resp->event_type = EC_MKBP_EVENT_KEY_MATRIX;
+ cros_ec_keyscan(ec, resp->data.key_matrix);
+ len = sizeof(*resp);
+ break;
+ }
default:
printf(" ** Unknown EC command %#02x\n", req_hdr->command);
return -1;
That's more or less only what the cros-ec-keyb counterpart expects. But
it doesn't test the -EC_RES_UNAVAILABLE thing or the fallback to the old
method.
Re: [PATCH] cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT
Hi Heinrich,
On Mon, 9 Nov 2020 at 12:34, Heinrich Schuchardt wrote:
>
> On 10/30/20 6:25 PM, Alper Nebi Yasak wrote:
> > The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the
> > keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT
> > and unavailable on more recent devices (including gru-kevin), as it was
> > removed in cros-ec commit 87a071941b89 ("mkbp: Add support for buttons
> > and switches.") dated 2016-07-06.
> >
> > The EC_CMD_GET_NEXT_EVENT has been available since cros-ec commit
> > d1ed75815efe ("MKBP event signalling implementation") dated 2014-10-20,
> > but it looks like it isn't included in firmware-* branches for at least
> > link, nyan-big, samus, snow, spring, panther and peach-pit which have
> > defconfigs in U-Boot. So this patch falls back to the old method if the
> > EC doesn't recognize the newer command.
> >
> > The implementation is mostly adapted from Depthcharge commit
> > f88af26b44fc ("cros_ec: Change keyboard scanning method.").
> >
> > On a gru-kevin, the current driver before this patch fails to read the
> > pressed keys with:
> >
> > out: cmd=0x60: 03 9d 60 00 00 00 00 00
> > in-header: 03 fc 01 00 00 00 00 00
> > in-data:
> > ec_command_inptr: len=-1, din=
> > check_for_keys: keyboard scan failed
> >
> > However the keyboard works fine with the newer command:
> >
> > out: cmd=0x67: 03 96 67 00 00 00 00 00
> > in-header: 03 ef 00 00 0e 00 00 00
> > in-data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ec_command_inptr: len=14, din=f412df30
> > key_matrix_decode: num_keys = 0
> > 0 valid keycodes found
> > out: cmd=0x67: 03 96 67 00 00 00 00 00
> > in-header: 03 df 00 00 0e 00 00 00
> > in-data: 00 00 00 00 00 00 00 00 00 00 00 00 10 00
> > ec_command_inptr: len=14, din=f412df30
> > key_matrix_decode: num_keys = 1
> > valid=1, row=4, col=11
> > keycode=28
> > 1 valid keycodes found
> > {0d}
> >
> > Signed-off-by: Alper Nebi Yasak
> > Reviewed-by: Simon Glass
>
> This patch has been applied to origin/master.
>
> Now when I compile sandbox_defconfig and run './u-boot -D' it spits out
> zillions of
>
>** Unknown EC command 0x67
>** Unknown EC command 0x67
>** Unknown EC command 0x67
>** Unknown EC command 0x67
>** Unknown EC command 0x67
>** Unknown EC command 0x67
>** Unknown EC command 0x67
>** Unknown EC command 0x67
>
> When I revert the patch the messages are gone.
>
> So something is really missing in this patch.
Test coverage, for a start!
I think the EC emulator needs to be updated for the new command.
Alper, can you please take a look?
Regards,
SImon
>
> Best regards
>
> Heinrich
>
> > ---
> >
> > drivers/input/cros_ec_keyb.c | 32 ++--
> > drivers/misc/cros_ec.c | 15 +++
> > include/cros_ec.h| 11 +++
> > 3 files changed, 52 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/input/cros_ec_keyb.c b/drivers/input/cros_ec_keyb.c
> > index 00bf58f2b5d2..0c0f52205b28 100644
> > --- a/drivers/input/cros_ec_keyb.c
> > +++ b/drivers/input/cros_ec_keyb.c
> > @@ -47,15 +47,35 @@ static int check_for_keys(struct udevice *dev, struct
> > key_matrix_key *keys,
> > struct key_matrix_key *key;
> > static struct mbkp_keyscan last_scan;
> > static bool last_scan_valid;
> > - struct mbkp_keyscan scan;
> > + struct ec_response_get_next_event event;
> > + struct mbkp_keyscan *scan = (struct mbkp_keyscan *)
> > + _matrix;
> > unsigned int row, col, bit, data;
> > int num_keys;
> > + int ret;
> >
> > - if (cros_ec_scan_keyboard(dev->parent, )) {
> > - debug("%s: keyboard scan failed\n", __func__);
> > + /* Get pending MKBP event. It may not be a key matrix event. */
> > + do {
> > + ret = cros_ec_get_next_event(dev->parent, );
> > + /* The EC has no events for us at this time. */
> > + if (ret == -EC_RES_UNAVAILABLE)
> > + return -EIO;
> > + else if (ret)
> > + break;
> > + } while (event.event_type != EC_MKBP_EVENT_KEY_MATRIX);
> > +
> > + /* Try the old command if the EC doesn't support the above. */
> > + if (ret == -EC_RES_INVALID_COMMAND) {
> > + if (cros_ec_scan_keyboard(dev->parent, scan)) {
> > + debug("%s: keyboard scan failed\n", __func__);
> > + return -EIO;
> > + }
> > + } else if (ret) {
> > + debug("%s: Error getting next MKBP event. (%d)\n",
> > + __func__, ret);
> > return -EIO;
> > }
> > - *samep = last_scan_valid && !memcmp(_scan, , sizeof(scan));
> > + *samep = last_scan_valid && !memcmp(_scan, scan, sizeof(*scan));
> >
> > /*
> >* This is a bit odd. The EC has
Re: [PATCH] cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT
On 10/30/20 6:25 PM, Alper Nebi Yasak wrote:
> The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the
> keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT
> and unavailable on more recent devices (including gru-kevin), as it was
> removed in cros-ec commit 87a071941b89 ("mkbp: Add support for buttons
> and switches.") dated 2016-07-06.
>
> The EC_CMD_GET_NEXT_EVENT has been available since cros-ec commit
> d1ed75815efe ("MKBP event signalling implementation") dated 2014-10-20,
> but it looks like it isn't included in firmware-* branches for at least
> link, nyan-big, samus, snow, spring, panther and peach-pit which have
> defconfigs in U-Boot. So this patch falls back to the old method if the
> EC doesn't recognize the newer command.
>
> The implementation is mostly adapted from Depthcharge commit
> f88af26b44fc ("cros_ec: Change keyboard scanning method.").
>
> On a gru-kevin, the current driver before this patch fails to read the
> pressed keys with:
>
> out: cmd=0x60: 03 9d 60 00 00 00 00 00
> in-header: 03 fc 01 00 00 00 00 00
> in-data:
> ec_command_inptr: len=-1, din=
> check_for_keys: keyboard scan failed
>
> However the keyboard works fine with the newer command:
>
> out: cmd=0x67: 03 96 67 00 00 00 00 00
> in-header: 03 ef 00 00 0e 00 00 00
> in-data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ec_command_inptr: len=14, din=f412df30
> key_matrix_decode: num_keys = 0
> 0 valid keycodes found
> out: cmd=0x67: 03 96 67 00 00 00 00 00
> in-header: 03 df 00 00 0e 00 00 00
> in-data: 00 00 00 00 00 00 00 00 00 00 00 00 10 00
> ec_command_inptr: len=14, din=f412df30
> key_matrix_decode: num_keys = 1
> valid=1, row=4, col=11
> keycode=28
> 1 valid keycodes found
> {0d}
>
> Signed-off-by: Alper Nebi Yasak
> Reviewed-by: Simon Glass
This patch has been applied to origin/master.
Now when I compile sandbox_defconfig and run './u-boot -D' it spits out
zillions of
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
When I revert the patch the messages are gone.
So something is really missing in this patch.
Best regards
Heinrich
> ---
>
> drivers/input/cros_ec_keyb.c | 32 ++--
> drivers/misc/cros_ec.c | 15 +++
> include/cros_ec.h| 11 +++
> 3 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/cros_ec_keyb.c b/drivers/input/cros_ec_keyb.c
> index 00bf58f2b5d2..0c0f52205b28 100644
> --- a/drivers/input/cros_ec_keyb.c
> +++ b/drivers/input/cros_ec_keyb.c
> @@ -47,15 +47,35 @@ static int check_for_keys(struct udevice *dev, struct
> key_matrix_key *keys,
> struct key_matrix_key *key;
> static struct mbkp_keyscan last_scan;
> static bool last_scan_valid;
> - struct mbkp_keyscan scan;
> + struct ec_response_get_next_event event;
> + struct mbkp_keyscan *scan = (struct mbkp_keyscan *)
> + _matrix;
> unsigned int row, col, bit, data;
> int num_keys;
> + int ret;
>
> - if (cros_ec_scan_keyboard(dev->parent, )) {
> - debug("%s: keyboard scan failed\n", __func__);
> + /* Get pending MKBP event. It may not be a key matrix event. */
> + do {
> + ret = cros_ec_get_next_event(dev->parent, );
> + /* The EC has no events for us at this time. */
> + if (ret == -EC_RES_UNAVAILABLE)
> + return -EIO;
> + else if (ret)
> + break;
> + } while (event.event_type != EC_MKBP_EVENT_KEY_MATRIX);
> +
> + /* Try the old command if the EC doesn't support the above. */
> + if (ret == -EC_RES_INVALID_COMMAND) {
> + if (cros_ec_scan_keyboard(dev->parent, scan)) {
> + debug("%s: keyboard scan failed\n", __func__);
> + return -EIO;
> + }
> + } else if (ret) {
> + debug("%s: Error getting next MKBP event. (%d)\n",
> + __func__, ret);
> return -EIO;
> }
> - *samep = last_scan_valid && !memcmp(_scan, , sizeof(scan));
> + *samep = last_scan_valid && !memcmp(_scan, scan, sizeof(*scan));
>
> /*
>* This is a bit odd. The EC has no way to tell us that it has run
> @@ -64,14 +84,14 @@ static int check_for_keys(struct udevice *dev, struct
> key_matrix_key *keys,
>* that this scan is the same as the last.
>*/
> last_scan_valid = true;
> - memcpy(_scan, , sizeof(last_scan));
> + memcpy(_scan, scan, sizeof(last_scan));
>
> for (col = num_keys = bit = 0; col < priv->matrix.num_cols;
> col++) {
> for (row = 0;
Re: [PATCH] cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT
On Fri, 30 Oct 2020 at 11:25, Alper Nebi Yasak wrote:
>
> The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the
> keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT
> and unavailable on more recent devices (including gru-kevin), as it was
> removed in cros-ec commit 87a071941b89 ("mkbp: Add support for buttons
> and switches.") dated 2016-07-06.
>
> The EC_CMD_GET_NEXT_EVENT has been available since cros-ec commit
> d1ed75815efe ("MKBP event signalling implementation") dated 2014-10-20,
> but it looks like it isn't included in firmware-* branches for at least
> link, nyan-big, samus, snow, spring, panther and peach-pit which have
> defconfigs in U-Boot. So this patch falls back to the old method if the
> EC doesn't recognize the newer command.
>
> The implementation is mostly adapted from Depthcharge commit
> f88af26b44fc ("cros_ec: Change keyboard scanning method.").
>
> On a gru-kevin, the current driver before this patch fails to read the
> pressed keys with:
>
> out: cmd=0x60: 03 9d 60 00 00 00 00 00
> in-header: 03 fc 01 00 00 00 00 00
> in-data:
> ec_command_inptr: len=-1, din=
> check_for_keys: keyboard scan failed
>
> However the keyboard works fine with the newer command:
>
> out: cmd=0x67: 03 96 67 00 00 00 00 00
> in-header: 03 ef 00 00 0e 00 00 00
> in-data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ec_command_inptr: len=14, din=f412df30
> key_matrix_decode: num_keys = 0
> 0 valid keycodes found
> out: cmd=0x67: 03 96 67 00 00 00 00 00
> in-header: 03 df 00 00 0e 00 00 00
> in-data: 00 00 00 00 00 00 00 00 00 00 00 00 10 00
> ec_command_inptr: len=14, din=f412df30
> key_matrix_decode: num_keys = 1
> valid=1, row=4, col=11
> keycode=28
> 1 valid keycodes found
> {0d}
>
> Signed-off-by: Alper Nebi Yasak
> ---
>
> drivers/input/cros_ec_keyb.c | 32 ++--
> drivers/misc/cros_ec.c | 15 +++
> include/cros_ec.h| 11 +++
> 3 files changed, 52 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass
[PATCH] cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT
The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the
keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT
and unavailable on more recent devices (including gru-kevin), as it was
removed in cros-ec commit 87a071941b89 ("mkbp: Add support for buttons
and switches.") dated 2016-07-06.
The EC_CMD_GET_NEXT_EVENT has been available since cros-ec commit
d1ed75815efe ("MKBP event signalling implementation") dated 2014-10-20,
but it looks like it isn't included in firmware-* branches for at least
link, nyan-big, samus, snow, spring, panther and peach-pit which have
defconfigs in U-Boot. So this patch falls back to the old method if the
EC doesn't recognize the newer command.
The implementation is mostly adapted from Depthcharge commit
f88af26b44fc ("cros_ec: Change keyboard scanning method.").
On a gru-kevin, the current driver before this patch fails to read the
pressed keys with:
out: cmd=0x60: 03 9d 60 00 00 00 00 00
in-header: 03 fc 01 00 00 00 00 00
in-data:
ec_command_inptr: len=-1, din=
check_for_keys: keyboard scan failed
However the keyboard works fine with the newer command:
out: cmd=0x67: 03 96 67 00 00 00 00 00
in-header: 03 ef 00 00 0e 00 00 00
in-data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ec_command_inptr: len=14, din=f412df30
key_matrix_decode: num_keys = 0
0 valid keycodes found
out: cmd=0x67: 03 96 67 00 00 00 00 00
in-header: 03 df 00 00 0e 00 00 00
in-data: 00 00 00 00 00 00 00 00 00 00 00 00 10 00
ec_command_inptr: len=14, din=f412df30
key_matrix_decode: num_keys = 1
valid=1, row=4, col=11
keycode=28
1 valid keycodes found
{0d}
Signed-off-by: Alper Nebi Yasak
---
drivers/input/cros_ec_keyb.c | 32 ++--
drivers/misc/cros_ec.c | 15 +++
include/cros_ec.h| 11 +++
3 files changed, 52 insertions(+), 6 deletions(-)
diff --git a/drivers/input/cros_ec_keyb.c b/drivers/input/cros_ec_keyb.c
index 00bf58f2b5d2..0c0f52205b28 100644
--- a/drivers/input/cros_ec_keyb.c
+++ b/drivers/input/cros_ec_keyb.c
@@ -47,15 +47,35 @@ static int check_for_keys(struct udevice *dev, struct
key_matrix_key *keys,
struct key_matrix_key *key;
static struct mbkp_keyscan last_scan;
static bool last_scan_valid;
- struct mbkp_keyscan scan;
+ struct ec_response_get_next_event event;
+ struct mbkp_keyscan *scan = (struct mbkp_keyscan *)
+ _matrix;
unsigned int row, col, bit, data;
int num_keys;
+ int ret;
- if (cros_ec_scan_keyboard(dev->parent, )) {
- debug("%s: keyboard scan failed\n", __func__);
+ /* Get pending MKBP event. It may not be a key matrix event. */
+ do {
+ ret = cros_ec_get_next_event(dev->parent, );
+ /* The EC has no events for us at this time. */
+ if (ret == -EC_RES_UNAVAILABLE)
+ return -EIO;
+ else if (ret)
+ break;
+ } while (event.event_type != EC_MKBP_EVENT_KEY_MATRIX);
+
+ /* Try the old command if the EC doesn't support the above. */
+ if (ret == -EC_RES_INVALID_COMMAND) {
+ if (cros_ec_scan_keyboard(dev->parent, scan)) {
+ debug("%s: keyboard scan failed\n", __func__);
+ return -EIO;
+ }
+ } else if (ret) {
+ debug("%s: Error getting next MKBP event. (%d)\n",
+ __func__, ret);
return -EIO;
}
- *samep = last_scan_valid && !memcmp(_scan, , sizeof(scan));
+ *samep = last_scan_valid && !memcmp(_scan, scan, sizeof(*scan));
/*
* This is a bit odd. The EC has no way to tell us that it has run
@@ -64,14 +84,14 @@ static int check_for_keys(struct udevice *dev, struct
key_matrix_key *keys,
* that this scan is the same as the last.
*/
last_scan_valid = true;
- memcpy(_scan, , sizeof(last_scan));
+ memcpy(_scan, scan, sizeof(last_scan));
for (col = num_keys = bit = 0; col < priv->matrix.num_cols;
col++) {
for (row = 0; row < priv->matrix.num_rows; row++) {
unsigned int mask = 1 << (bit & 7);
- data = scan.data[bit / 8];
+ data = scan->data[bit / 8];
if ((data & mask) && num_keys < max_count) {
key = keys + num_keys++;
key->row = row;
diff --git a/drivers/misc/cros_ec.c b/drivers/misc/cros_ec.c
index a5534b16673b..c3674908ee82 100644
--- a/drivers/misc/cros_ec.c
+++ b/drivers/misc/cros_ec.c
@@ -415,6 +415,21 @@ int cros_ec_scan_keyboard(struct udevice *dev, struct
mbkp_keyscan *scan)
return 0;
}
+int
