Re: [U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support
On 12/08/2011 02:56 PM, Simon Glass wrote: Hi Stephen, On Wed, Dec 7, 2011 at 2:02 PM, Stephen Warren swar...@nvidia.com wrote: On 12/02/2011 07:57 PM, Simon Glass wrote: Add support for internal matrix keyboard controller for Nvidia Tegra platforms. This driver uses the fdt decode function to obtain its key codes. ... From a purely HW perspective, the only thing that exists is a single mapping from (row, col) to keycode. I believe that's all the KBC driver's binding should define. I'll amend that slightly: keyboards commonly implement the Fn key internally in HW, since it causes keys to emit completely different raw codes, so I can see this driver wanting both keycode-plain and keycode-fn. However, keycode-shift and keycode-ctrl are purely SW concepts. As such, they shouldn't be part of the KBC's own mapping table. Witness how the kernel's Tegra KBC driver only contains the plain and fn tables (albeit merged into a single array for ease of use), and the input core is what interpets e.g. KEY_LEFTSHIFT as a modifier. Yes, certainly for ctrl I agree that we can simply calculate it. Although interestingly the other two keyboard drivers in U-Boot use the same approach as here, so maybe there is a fish hook somewhere (I have not written this code before). So specifically what I'd like to see changed in this binding is: a) We need binding documentation for the Tegra KBC binding, in the same style as found in the kernel's Documentation/devicetree/bindings. OK will do - should I just put that in the cover letter? There is no such file in U-Boot. Right now, the canonical location for binding documentation appears to be the kernel's Documentation/devicetree/bindings/ directory. Perhaps we should add the documentation there first? At least, we should post the binding document in the standard kernel style to the linux-input and devicetree-discuss mailing lists even if it doesn't get immediately checked in. I recall from the kernel summit that Grant Likely was thinking of creating an outside-the-kernel repository for either/both of binding documentation and .dts/.dtsi. I'm not sure if any progress has been made there yet. b) The Tegra KBC binding should include only the keycode-plain and keycode-fn properties; nothing to do with shift/ctrl/alt/ (Well, also any standardized properties such as compatible, reg, interrupts). OK. I'm not sure how I specify what happens when you press shift... c) We need binding documentation for the data that is/could-be common across multiple keyboards: i.e. what does each key code value represent; which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common across (1) all keyboards (2) some standardized meaning for DT that can be used by U-Boot, the Linux kernel, etc. Perhaps there is already such a standard? I'm not aware of it. I looked around for any kind of existing keyboard mapping binding, and I couldn't find anything. I did find a Samsung matrix KBC binding in the kernel (Documentation/devicetree/bindings/input/samsung-keypad.txt) which is conceptually at the same level as having a plain table, although no fn table in their case. I prefer the proposed Tegra binding's table approach rather than having a separate node per key myself. d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a separate binding that can be common to any/all keyboards (probably the same document as (b) above). The input to this table should be the raw codes from keycode-plain/keycode-fn. The output would be the values sent to whatever consumes keyboard input. The naming of these properties should make it obvious that it's something not specific to Tegra's KBC, and SW oriented. Perhaps something semantically similar to loadkeys' or xkbcomp's input, although I haven't looked at those in detail. While I agree this would be nice, it involves adding a layer of software into U-Boot which doesn't currently exist (converting key codes + modifies into ASCII codes). It doesn't have to be a separate lyaer in U-Boot; the binding just has to be defined in a way that doesn't tie it to the Tegra KBC driver, so it can be re-used. In other words: Tegra KBC's binding defines: nvidia,keycode-plain nvidia,keycode-fn (these should output shift and ctrl keycodes for later processing) Generic key mapping binding defines: modifier-mapping-shift modifier-mapping-ctrl ... which take as input the values generated by keycode-plain/keycode-fn and output the rewritten keycodes. (the difference here is that the input to those tables is the output from the nvidia,keycode-foo tables, rather than the raw HW keycodes) As far as implementation goes, all of that can be handled inside the Tegra KBC driver for now, so no new layer is required. I think we should run this plan, and both binding definitions past the linux-input mailing list; people there will be far more aware of any previous work in this area, whether this plan makes
Re: [U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support
Hi Stephen, On Mon, Dec 12, 2011 at 10:00 AM, Stephen Warren swar...@nvidia.com wrote: On 12/08/2011 02:56 PM, Simon Glass wrote: Hi Stephen, On Wed, Dec 7, 2011 at 2:02 PM, Stephen Warren swar...@nvidia.com wrote: On 12/02/2011 07:57 PM, Simon Glass wrote: Add support for internal matrix keyboard controller for Nvidia Tegra platforms. This driver uses the fdt decode function to obtain its key codes. ... From a purely HW perspective, the only thing that exists is a single mapping from (row, col) to keycode. I believe that's all the KBC driver's binding should define. I'll amend that slightly: keyboards commonly implement the Fn key internally in HW, since it causes keys to emit completely different raw codes, so I can see this driver wanting both keycode-plain and keycode-fn. However, keycode-shift and keycode-ctrl are purely SW concepts. As such, they shouldn't be part of the KBC's own mapping table. Witness how the kernel's Tegra KBC driver only contains the plain and fn tables (albeit merged into a single array for ease of use), and the input core is what interpets e.g. KEY_LEFTSHIFT as a modifier. Yes, certainly for ctrl I agree that we can simply calculate it. Although interestingly the other two keyboard drivers in U-Boot use the same approach as here, so maybe there is a fish hook somewhere (I have not written this code before). So specifically what I'd like to see changed in this binding is: a) We need binding documentation for the Tegra KBC binding, in the same style as found in the kernel's Documentation/devicetree/bindings. OK will do - should I just put that in the cover letter? There is no such file in U-Boot. Right now, the canonical location for binding documentation appears to be the kernel's Documentation/devicetree/bindings/ directory. Perhaps we should add the documentation there first? At least, we should post the binding document in the standard kernel style to the linux-input and devicetree-discuss mailing lists even if it doesn't get immediately checked in. I recall from the kernel summit that Grant Likely was thinking of creating an outside-the-kernel repository for either/both of binding documentation and .dts/.dtsi. I'm not sure if any progress has been made there yet. Not enough that it is done, but I believe it is happening. b) The Tegra KBC binding should include only the keycode-plain and keycode-fn properties; nothing to do with shift/ctrl/alt/ (Well, also any standardized properties such as compatible, reg, interrupts). OK. I'm not sure how I specify what happens when you press shift... c) We need binding documentation for the data that is/could-be common across multiple keyboards: i.e. what does each key code value represent; which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common across (1) all keyboards (2) some standardized meaning for DT that can be used by U-Boot, the Linux kernel, etc. Perhaps there is already such a standard? I'm not aware of it. I looked around for any kind of existing keyboard mapping binding, and I couldn't find anything. I did find a Samsung matrix KBC binding in the kernel (Documentation/devicetree/bindings/input/samsung-keypad.txt) which is conceptually at the same level as having a plain table, although no fn table in their case. I prefer the proposed Tegra binding's table approach rather than having a separate node per key myself. d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a separate binding that can be common to any/all keyboards (probably the same document as (b) above). The input to this table should be the raw codes from keycode-plain/keycode-fn. The output would be the values sent to whatever consumes keyboard input. The naming of these properties should make it obvious that it's something not specific to Tegra's KBC, and SW oriented. Perhaps something semantically similar to loadkeys' or xkbcomp's input, although I haven't looked at those in detail. While I agree this would be nice, it involves adding a layer of software into U-Boot which doesn't currently exist (converting key codes + modifies into ASCII codes). It doesn't have to be a separate lyaer in U-Boot; the binding just has to be defined in a way that doesn't tie it to the Tegra KBC driver, so it can be re-used. In other words: Tegra KBC's binding defines: nvidia,keycode-plain nvidia,keycode-fn (these should output shift and ctrl keycodes for later processing) Generic key mapping binding defines: modifier-mapping-shift modifier-mapping-ctrl ... which take as input the values generated by keycode-plain/keycode-fn and output the rewritten keycodes. (the difference here is that the input to those tables is the output from the nvidia,keycode-foo tables, rather than the raw HW keycodes) I'm not sure this passes the 'simple' test. But OK. If we limit it to characters 128 then we can avoid doubling the size of each mapping. As
Re: [U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support
On 12/12/2011 11:10 AM, Simon Glass wrote: On Mon, Dec 12, 2011 at 10:00 AM, Stephen Warren swar...@nvidia.com wrote: On 12/08/2011 02:56 PM, Simon Glass wrote: ... I'd like to ask for comments in linux-input in case they have any other ideas, e.g. whether a set of separately documented modifier mapping properties makes sense. If not, I suppose the following set of properties would suffice: nvidia,keycode-plain nvidia,keycode-fn u-boot,keycode-shift u-boot,keycode-ctrl (although the last two seem to want both nvidia, and u-boot, prefixes) OK. Since this seems to be a kernel issue and Nvidia-specific can I ask if you can please send this email? I will wait for confirmation that this is OK before going further. OK, I'll send the email. I would like to point out though that this is neither a kernel issue nor NVIDIA-specific: It's a device tree issue dealing with how to best represent certain HW features, and it will apply equally to any HW that has a keyboard with modifier keys; essentially all of them. -- nvpublic ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support
Hi Stephen, On Mon, Dec 12, 2011 at 10:31 AM, Stephen Warren swar...@nvidia.com wrote: On 12/12/2011 11:10 AM, Simon Glass wrote: On Mon, Dec 12, 2011 at 10:00 AM, Stephen Warren swar...@nvidia.com wrote: On 12/08/2011 02:56 PM, Simon Glass wrote: ... I'd like to ask for comments in linux-input in case they have any other ideas, e.g. whether a set of separately documented modifier mapping properties makes sense. If not, I suppose the following set of properties would suffice: nvidia,keycode-plain nvidia,keycode-fn u-boot,keycode-shift u-boot,keycode-ctrl (although the last two seem to want both nvidia, and u-boot, prefixes) OK. Since this seems to be a kernel issue and Nvidia-specific can I ask if you can please send this email? I will wait for confirmation that this is OK before going further. OK, I'll send the email. Thanks. I would like to point out though that this is neither a kernel issue nor NVIDIA-specific: It's a device tree issue dealing with how to best represent certain HW features, and it will apply equally to any HW that has a keyboard with modifier keys; essentially all of them. Sorry I don't quite understand what you mean by that, given your comments that we should ask on linux-input. While it could apply to the two existing keyboard drivers, they each do their own thing at present. Regards, Simon -- nvpublic ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support
Hi Stephen, On Wed, Dec 7, 2011 at 2:02 PM, Stephen Warren swar...@nvidia.com wrote: On 12/02/2011 07:57 PM, Simon Glass wrote: Add support for internal matrix keyboard controller for Nvidia Tegra platforms. This driver uses the fdt decode function to obtain its key codes. +static int fdt_decode_kbc(const void *blob, int node, struct keyb *config) +{ + config-kbc = (struct kbc_tegra *)fdtdec_get_addr(blob, node, reg); + config-plain_keycode = fdtdec_locate_byte_array(blob, node, + keycode-plain, KBC_KEY_COUNT); + config-shift_keycode = fdtdec_locate_byte_array(blob, node, + keycode-shift, KBC_KEY_COUNT); + config-fn_keycode = fdtdec_locate_byte_array(blob, node, + keycode-fn, KBC_KEY_COUNT); + config-ctrl_keycode = fdtdec_locate_byte_array(blob, node, + keycode-ctrl, KBC_KEY_COUNT); + if (!config-plain_keycode) { + debug(%s: cannot find keycode-plain map\n, __func__); + return -1; + } + return 0; +} Simon, Sorry to keep pushing back on everything so much, but I don't believe the structure of this binding is correct. If I didn't want your feedback I would not have copied you on the patches :-) It is very useful to know how things are being done in the kernel and we need to try to keep U-Boot and the kernel as close as possible on the fdt side (although perhaps not in any other way - U-Boot is a boot loader not an OS). I'm starting to form the view that U-Boot will need a few more tweaks on top of the kernel file in some cases, but let's see. Thanks very much for taking the time to provide this very useful feedback. If nothing else these discussions might tease out issues to address later. From a purely HW perspective, the only thing that exists is a single mapping from (row, col) to keycode. I believe that's all the KBC driver's binding should define. I'll amend that slightly: keyboards commonly implement the Fn key internally in HW, since it causes keys to emit completely different raw codes, so I can see this driver wanting both keycode-plain and keycode-fn. However, keycode-shift and keycode-ctrl are purely SW concepts. As such, they shouldn't be part of the KBC's own mapping table. Witness how the kernel's Tegra KBC driver only contains the plain and fn tables (albeit merged into a single array for ease of use), and the input core is what interpets e.g. KEY_LEFTSHIFT as a modifier. Yes, certainly for ctrl I agree that we can simply calculate it. Although interestingly the other two keyboard drivers in U-Boot use the same approach as here, so maybe there is a fish hook somewhere (I have not written this code before). So specifically what I'd like to see changed in this binding is: a) We need binding documentation for the Tegra KBC binding, in the same style as found in the kernel's Documentation/devicetree/bindings. OK will do - should I just put that in the cover letter? There is no such file in U-Boot. b) The Tegra KBC binding should include only the keycode-plain and keycode-fn properties; nothing to do with shift/ctrl/alt/ (Well, also any standardized properties such as compatible, reg, interrupts). OK. I'm not sure how I specify what happens when you press shift... c) We need binding documentation for the data that is/could-be common across multiple keyboards: i.e. what does each key code value represent; which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common across (1) all keyboards (2) some standardized meaning for DT that can be used by U-Boot, the Linux kernel, etc. Perhaps there is already such a standard? I'm not aware of it. d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a separate binding that can be common to any/all keyboards (probably the same document as (b) above). The input to this table should be the raw codes from keycode-plain/keycode-fn. The output would be the values sent to whatever consumes keyboard input. The naming of these properties should make it obvious that it's something not specific to Tegra's KBC, and SW oriented. Perhaps something semantically similar to loadkeys' or xkbcomp's input, although I haven't looked at those in detail. While I agree this would be nice, it involves adding a layer of software into U-Boot which doesn't currently exist (converting key codes + modifies into ASCII codes). Linux probably wouldn't use (d), since it already has its own SW-oriented methods of setting up such mapping tables. Although perhaps in the future the default mappings could be set up from these properties. Well, and X already has its own, etc. U-Boot would need (d), since AIUI, there is no handling of such a higher layer of mapping tables there right now. Initially, the code to parse and implement the mappings in (d) could be part of the Tegra KBC driver,
Re: [U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support
On 12/02/2011 07:57 PM, Simon Glass wrote: Add support for internal matrix keyboard controller for Nvidia Tegra platforms. This driver uses the fdt decode function to obtain its key codes. +static int fdt_decode_kbc(const void *blob, int node, struct keyb *config) +{ + config-kbc = (struct kbc_tegra *)fdtdec_get_addr(blob, node, reg); + config-plain_keycode = fdtdec_locate_byte_array(blob, node, + keycode-plain, KBC_KEY_COUNT); + config-shift_keycode = fdtdec_locate_byte_array(blob, node, + keycode-shift, KBC_KEY_COUNT); + config-fn_keycode = fdtdec_locate_byte_array(blob, node, + keycode-fn, KBC_KEY_COUNT); + config-ctrl_keycode = fdtdec_locate_byte_array(blob, node, + keycode-ctrl, KBC_KEY_COUNT); + if (!config-plain_keycode) { + debug(%s: cannot find keycode-plain map\n, __func__); + return -1; + } + return 0; +} Simon, Sorry to keep pushing back on everything so much, but I don't believe the structure of this binding is correct. From a purely HW perspective, the only thing that exists is a single mapping from (row, col) to keycode. I believe that's all the KBC driver's binding should define. I'll amend that slightly: keyboards commonly implement the Fn key internally in HW, since it causes keys to emit completely different raw codes, so I can see this driver wanting both keycode-plain and keycode-fn. However, keycode-shift and keycode-ctrl are purely SW concepts. As such, they shouldn't be part of the KBC's own mapping table. Witness how the kernel's Tegra KBC driver only contains the plain and fn tables (albeit merged into a single array for ease of use), and the input core is what interpets e.g. KEY_LEFTSHIFT as a modifier. So specifically what I'd like to see changed in this binding is: a) We need binding documentation for the Tegra KBC binding, in the same style as found in the kernel's Documentation/devicetree/bindings. b) The Tegra KBC binding should include only the keycode-plain and keycode-fn properties; nothing to do with shift/ctrl/alt/ (Well, also any standardized properties such as compatible, reg, interrupts). c) We need binding documentation for the data that is/could-be common across multiple keyboards: i.e. what does each key code value represent; which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common across (1) all keyboards (2) some standardized meaning for DT that can be used by U-Boot, the Linux kernel, etc. Perhaps there is already such a standard? d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a separate binding that can be common to any/all keyboards (probably the same document as (b) above). The input to this table should be the raw codes from keycode-plain/keycode-fn. The output would be the values sent to whatever consumes keyboard input. The naming of these properties should make it obvious that it's something not specific to Tegra's KBC, and SW oriented. Perhaps something semantically similar to loadkeys' or xkbcomp's input, although I haven't looked at those in detail. Linux probably wouldn't use (d), since it already has its own SW-oriented methods of setting up such mapping tables. Although perhaps in the future the default mappings could be set up from these properties. U-Boot would need (d), since AIUI, there is no handling of such a higher layer of mapping tables there right now. Initially, the code to parse and implement the mappings in (d) could be part of the Tegra KBC driver, if there's nowhere else to put it. I simply want to ensure that the structure of the mapping table bindings doesn't require them to be specific to Tegra KBC, or perpetually implemented by the Tegra KBC driver even when/if U-Boot does acquire a higher layer that deals with this kind of thing. That's because they're SW concepts not part of the HW/HW-binding. -- nvpublic ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support
From: Rakesh Iyer ri...@nvidia.com Add support for internal matrix keyboard controller for Nvidia Tegra platforms. This driver uses the fdt decode function to obtain its key codes. Support for the Ctrl modifier is provided. The left and right ctrl keys are dealt with in the same way. When the user presses Ctrl+D, the top of the keyboard controller FIFO is usually filled with several Ctrl only keycodes. The modifier keys handling code would remove the keycode and return 0. So, the tstc will indicate that no key is pressed. The correct behaviour for tstc is to check all the entries of the FIFO until we find a significant one or return 0 if there is none. This driver adds a small FIFO that is used to store conversions of keycodes to escape sequences that U-Boot expects to see from its input device drivers. The fifo also replaces the testc/getc last keypress management code. Key detection before the driver is loaded is supported. This key will be picked up with the keyboard driver is initialized. Signed-off-by: Simon Glass s...@chromium.org --- drivers/input/Makefile|1 + drivers/input/tegra-kbc.c | 626 + include/fdtdec.h |1 + include/tegra-kbc.h | 33 +++ lib/fdtdec.c |1 + 5 files changed, 662 insertions(+), 0 deletions(-) create mode 100644 drivers/input/tegra-kbc.c create mode 100644 include/tegra-kbc.h diff --git a/drivers/input/Makefile b/drivers/input/Makefile index 1f4dad3..4684e55 100644 --- a/drivers/input/Makefile +++ b/drivers/input/Makefile @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB:= $(obj)libinput.o COBJS-$(CONFIG_I8042_KBD) += i8042.o +COBJS-$(CONFIG_TEGRA2_KEYBOARD) += tegra-kbc.o ifdef CONFIG_PS2KBD COBJS-y += keyboard.o pc_keyb.o COBJS-$(CONFIG_PS2MULT) += ps2mult.o ps2ser.o diff --git a/drivers/input/tegra-kbc.c b/drivers/input/tegra-kbc.c new file mode 100644 index 000..7a45440 --- /dev/null +++ b/drivers/input/tegra-kbc.c @@ -0,0 +1,626 @@ +/* + * (C) Copyright 2011 + * NVIDIA Corporation www.nvidia.com + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ +#define DEBUG +#include common.h +#include malloc.h +#include stdio_dev.h +#include asm/io.h +#include asm/arch/clock.h +#include asm/arch/pinmux.h +#include asm/arch/timer.h +#include tegra-kbc.h +#include fdtdec.h + +DECLARE_GLOBAL_DATA_PTR; + +#define DEVNAME tegra-kbc + +enum { + KBC_MAX_ROW = 16, + KBC_MAX_COL = 8, + KBC_KEY_COUNT = KBC_MAX_ROW * KBC_MAX_COL, + KBC_MAX_GPIO= 24, + KBC_MAX_KPENT = 8,/* size of keypress entry queue */ +}; + +enum KEYS { + KEY_FIRST_MODIFIER = 220, + KEY_RIGHT_CTRL = KEY_FIRST_MODIFIER, + KEY_LEFT_CTRL, + KEY_FN, + KEY_SHIFT, +}; + +/* KBC row scan time and delay for beginning the row scan (in usecs) */ +#define KBC_ROW_SCAN_TIME 16 +#define KBC_ROW_SCAN_DLY 5 + +/* uses a 32KHz clock so a cycle = 1/32Khz */ +#define KBC_CYCLE_IN_USEC DIV_ROUND_UP(1000, 32) + +#define KBC_FIFO_TH_CNT_SHIFT(cnt) (cnt 14) +#define KBC_DEBOUNCE_CNT_SHIFT(cnt)(cnt 4) +#define KBC_CONTROL_FIFO_CNT_INT_EN(1 3) +#define KBC_CONTROL_KBC_EN (1 0) +#define KBC_INT_FIFO_CNT_INT_STATUS(1 2) +#define KBC_KPENT_VALID(1 7) + +#define KBC_RPT_DLY20 +#define KBC_RPT_RATE 4 + +/* + * Use a simple FIFO to convert some keys into escape sequences and to handle + * testc vs getc. The FIFO length must be a power of two. Currently, four + * characters is the smallest power of two that is large enough to contain all + * generated escape sequences. + */ +#define KBC_FIFO_LENGTH(1 2) + +static struct keyb { + /* +* Information about keycode mappings. The plain_keycode array must +* exist but the others may be NULL in which case they are not +* decoded. +*/ + const u8 *plain_keycode;/* key code for each row / column */ + const u8 *shift_keycode;/* ...when shift held down */ + const u8 *fn_keycode; /* ...when Fn held down */ + const u8 *ctrl_keycode; /* ...when ctrl held down */ + +