Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: [WIP] ec/kontron/kempld: Add code to GPIOs control ......................................................................
[WIP] ec/kontron/kempld: Add code to GPIOs control
This is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [1].
[1] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/kempld.h A src/ec/kontron/kempld/kempld_gpio.c A src/ec/kontron/kempld/kempld_gpio.h 4 files changed, 118 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/1
diff --git a/src/ec/kontron/kempld/Makefile.inc b/src/ec/kontron/kempld/Makefile.inc index 0d59886..e7bafa3 100644 --- a/src/ec/kontron/kempld/Makefile.inc +++ b/src/ec/kontron/kempld/Makefile.inc @@ -2,3 +2,4 @@ ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += early_kempld.c ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += kempld.c ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += kempld_i2c.c +ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += kempld_gpio.c diff --git a/src/ec/kontron/kempld/kempld.h b/src/ec/kontron/kempld/kempld.h index 6f624aa..7bd6ba3 100644 --- a/src/ec/kontron/kempld/kempld.h +++ b/src/ec/kontron/kempld/kempld.h @@ -13,4 +13,9 @@
void kempld_enable_uart_for_console(void);
+int kempld_gpio_get(unsigned int offset); +void kempld_gpio_set(unsigned int offset, int value); +void kempld_gpio_direction_input(unsigned int offset); +void kempld_gpio_direction_output(unsigned int offset, int value); + #endif /* EC_KONTRON_KEMPLD_H */ diff --git a/src/ec/kontron/kempld/kempld_gpio.c b/src/ec/kontron/kempld/kempld_gpio.c new file mode 100644 index 0000000..b3307ae --- /dev/null +++ b/src/ec/kontron/kempld/kempld_gpio.c @@ -0,0 +1,83 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Kontron PLD driver definitions + * + * Copyright (c) 2010-2012 Kontron Europe GmbH + * Author: Michael Brunner michael.brunner@kontron.com + */ + +#include <stdint.h> +#include <arch/io.h> +#include <delay.h> +#include "chip.h" +#include "kempld.h" +#include "kempld_gpio.h" +#include "kempld_internal.h" + +int kempld_gpio_get(unsigned int offset) +{ + int status; + + if (kempld_get_mutex(100) < 0) + return 0; + + status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); + status &= KEMPLD_GPIO_MASK(offset); + + kempld_release_mutex(); + + return status; +} + +void kempld_gpio_set(unsigned int offset, int value) +{ + int status; + + if (kempld_get_mutex(100) < 0) + return; + + status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); + if (value) + status |= KEMPLD_GPIO_MASK(offset); + else + status &= ~KEMPLD_GPIO_MASK(offset); + kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status); + + kempld_release_mutex(); +} + + +void kempld_gpio_direction_input(unsigned int offset) +{ + int status; + + if (kempld_get_mutex(100) < 0) + return; + + status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); + status &= ~KEMPLD_GPIO_MASK(offset); + kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); + + kempld_release_mutex(); +} + +void kempld_gpio_direction_output(unsigned int offset, int value) +{ + int status; + + if (kempld_get_mutex(100) < 0) + return; + + status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); + if (value) + status |= KEMPLD_GPIO_MASK(offset); + else + status &= ~KEMPLD_GPIO_MASK(offset); + kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status); + + status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); + status |= KEMPLD_GPIO_MASK(offset); + kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); + + kempld_release_mutex(); +} diff --git a/src/ec/kontron/kempld/kempld_gpio.h b/src/ec/kontron/kempld/kempld_gpio.h new file mode 100644 index 0000000..b918bd0 --- /dev/null +++ b/src/ec/kontron/kempld/kempld_gpio.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Kontron PLD driver definitions + * + * Copyright (c) 2010-2012 Kontron Europe GmbH + * Author: Michael Brunner michael.brunner@kontron.com + */ + +#ifndef _KEMPLD_GPIO_H_ +#define _KEMPLD_GPIO_H_ + +#define KEMPLD_GPIO_MAX_NUM 16 +#define KEMPLD_GPIO_MASK(x) (1 << ((x) % 8)) +#define KEMPLD_GPIO_DIR 0x40 +#define KEMPLD_GPIO_DIR_NUM(x) (0x40 + (x)/8) +#define KEMPLD_GPIO_LVL 0x42 +#define KEMPLD_GPIO_LVL_NUM(x) (0x42 + (x)/8) +#define KEMPLD_GPIO_STS 0x44 +#define KEMPLD_GPIO_STS_NUM(x) (0x44 + (x)/8) +#define KEMPLD_GPIO_EVT_LVL_EDGE 0x46 +#define KEMPLD_GPIO_EVT_LVL_EDGE_NUM(x) (0x46 + (x)/8) +#define KEMPLD_GPIO_EVT_LOW_HIGH 0x48 +#define KEMPLD_GPIO_EVT_LOW_HIGH_NUM(x) (0x48 + (x)/8) +#define KEMPLD_GPIO_IEN 0x4A +#define KEMPLD_GPIO_IEN_NUM(x) (0x4A + (x)/8) +#define KEMPLD_GPIO_NMIEN 0x4C +#define KEMPLD_GPIO_NMIEN_NUM(x) (0x4C + (x)/8) + +#endif /* _KEMPLD_GPIO_H_ */
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#2).
Change subject: [WIP] ec/kontron/kempld: add nimimal GPIO driver ......................................................................
[WIP] ec/kontron/kempld: add nimimal GPIO driver
This is based on code from the drivers/gpio/gpio-kempld.c linux driver.
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld.h A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 6 files changed, 146 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: [WIP] ec/kontron/kempld: add nimimal GPIO driver ......................................................................
Patch Set 2:
(2 comments)
File src/ec/kontron/kempld/kempld_gpio.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119481): https://review.coreboot.org/c/coreboot/+/47595/comment/ae4f6013_59386acc PS2, Line 95: for(i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) { space required before the open parenthesis '('
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119481): https://review.coreboot.org/c/coreboot/+/47595/comment/6aebb0fa_59f4829a PS2, Line 96: if(config->gpio_pad[i] & KEMPLD_PAD_GPIO_CFG_DIR(OUT)) space required before the open parenthesis '('
Attention is currently required from: Maxim Polyakov. Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#3).
Change subject: [WIP] ec/kontron/kempld: add nimimal GPIO driver ......................................................................
[WIP] ec/kontron/kempld: add nimimal GPIO driver
This is based on code from the drivers/gpio/gpio-kempld.c linux driver.
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld.h A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 6 files changed, 145 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/3
Attention is currently required from: Maxim Polyakov. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: [WIP] ec/kontron/kempld: add nimimal GPIO driver ......................................................................
Patch Set 3:
(2 comments)
File src/ec/kontron/kempld/kempld_gpio.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119489): https://review.coreboot.org/c/coreboot/+/47595/comment/381f74c1_231d157a PS3, Line 94: for(i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) { space required before the open parenthesis '('
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-119489): https://review.coreboot.org/c/coreboot/+/47595/comment/c42468f7_9c8fb314 PS3, Line 95: if(config->gpio_pad[i] & KEMPLD_PAD_GPIO_CFG_DIR(OUT)) space required before the open parenthesis '('
Attention is currently required from: Maxim Polyakov. Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#4).
Change subject: [WIP] ec/kontron/kempld: add nimimal GPIO driver ......................................................................
[WIP] ec/kontron/kempld: add nimimal GPIO driver
This is based on code from the drivers/gpio/gpio-kempld.c linux driver.
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld.h A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 6 files changed, 143 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/4
Attention is currently required from: Maxim Polyakov. Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#5).
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
ec/kontron/kempld: Add minimal GPIO driver
This patch adds an interface for configuring GPIOs inside the Kontron CPLD/EC. There are several ways to do this: you can enable <ec/kontron/ kempel/kempeld.h> in the source codes with board initialization and configure/manage each pad using the API or statically define the GPIO configuration in devicetree.cb. For example:
chip ec/kontron/kempld device gpio 0 on register "gpio_pad[0]" = "KEMPLD_PAD_GPIO_CFG_IN" register "gpio_pad[4]" = "KEMPLD_PAD_GPIO_CFG_OUT(1)" register "gpio_pad[5]" = "KEMPLD_PAD_GPIO_CFG_OUT(0)" end end
KEMPLD_PAD_GPIO_CFG_IN - configure the pad as an input; KEMPLD_PAD_GPIO_CFG_OUT(1) - configure the pad as an output, set to 1; KEMPLD_PAD_GPIO_CFG_OUT(0) - configure the pad as an output, set to 0.
The pad configuration is set using the options gpio_pad[], however, if this option is not set, the corresponding pad will be configured as an input. In this case, <device gpio 0>, like all other devices, is not a real device inside the EC. These definitions are used to understand the EC resources and systematize configuration options, but if mark this as <off>, the initialization step will be skipped in the driver code.
This work is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL-10 COMe module CB:54380 .
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld.h A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 6 files changed, 133 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/5
Attention is currently required from: Felix Singer, Nico Huber, Maxim Polyakov, Werner Zeh. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 5:
(16 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/47595/comment/9b888f68_57f5494f PS5, Line 11: kempel/kempeld typos? `kempld/kempld`
https://review.coreboot.org/c/coreboot/+/47595/comment/c32cf311_b621ce9b PS5, Line 12: configure/manage each pad using the API Are there plans to use this API?
File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/2df06114_f525a113 PS5, Line 7: KEMPLD_NUM_GPIO_PADS Why not `KEMPLD_NUM_GPIOS`?
https://review.coreboot.org/c/coreboot/+/47595/comment/89fbd0cb_36820d73 PS5, Line 9: #define KEMPLD_PAD_GPIO_DIR_IN 0 : #define KEMPLD_PAD_GPIO_DIR_OUT 1 : #define KEMPLD_PAD_GPIO_CFG_DIR(in_out) ((KEMPLD_PAD_GPIO_DIR_##in_out) << 7) : #define KEMPLD_PAD_GPIO_CFG_VAL(v) (v) : #define KEMPLD_PAD_GPIO_CFG_IN KEMPLD_PAD_GPIO_CFG_DIR(IN) : #define KEMPLD_PAD_GPIO_CFG_OUT(v) \ : (KEMPLD_PAD_GPIO_CFG_DIR(OUT) | KEMPLD_PAD_GPIO_CFG_VAL(v)) Do we really need to use macros for this? Why not go with enums, like the rest of the code does?
enum kempld_gpio_mode { KEMPLD_GPIO_DEFAULT, KEMPLD_GPIO_INPUT, KEMPLD_GPIO_OUTPUT_LOW, KEMPLD_GPIO_OUTPUT_HIGH, };
The `KEMPLD_GPIO_DEFAULT` value allows skipping config for unspecified GPIOs, which preserves current behavior.
https://review.coreboot.org/c/coreboot/+/47595/comment/e6a79fdf_91a0fd11 PS5, Line 38: gpio_pad I'd call this `gpios`. Also, the type should be `enum kempld_gpio_mode`.
File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/015fe73b_c8d95b76 PS5, Line 99: GPIOs configuration nit: either `GPIO configuration` or `Configuring GPIOs`. I prefer the former.
https://review.coreboot.org/c/coreboot/+/47595/comment/0a3d3fa7_841e5f77 PS5, Line 100: } else, warn? it would be consistent with the code above:
} else { printk(BIOS_WARNING, "KEMPLD: Spurious GPIO device %s.\n", dev_path(dev)); }
File src/ec/kontron/kempld/kempld_gpio.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/2681c9d1_717c1b08 PS5, Line 6: * Author: Michael Brunner michael.brunner@kontron.com Is this comment because the code was taken from the Linux kernel? Do we need it?
https://review.coreboot.org/c/coreboot/+/47595/comment/4b8629ed_b0f424ff PS5, Line 16: offset Why is this called `offset`? Unless I'm missing something, it's just the GPIO index?
https://review.coreboot.org/c/coreboot/+/47595/comment/2404e3b8_f87450b0 PS5, Line 18: int status Why is this an int (should be u8)? Why is it called `status`?
https://review.coreboot.org/c/coreboot/+/47595/comment/98fd12fc_4092af41 PS5, Line 23: (u8) Are these casts needed? I guess they might be because `offset` is an `unsigned int`.
https://review.coreboot.org/c/coreboot/+/47595/comment/1f4a7d17_666913af PS5, Line 33: int status Why is this an int (should be u8)? Why is it called `status`?
https://review.coreboot.org/c/coreboot/+/47595/comment/2504c7de_b5f720fd PS5, Line 49: int kempld_gpio_direction_input(unsigned int offset) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : } : : int kempld_gpio_direction_output(unsigned int offset, int value) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); : if (value) : status |= KEMPLD_GPIO_MASK(offset); : else : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status); : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status |= KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : } This would need to be adapted to use the enum type I suggested in another comment:
enum kempld_gpio_mode { KEMPLD_GPIO_DEFAULT, KEMPLD_GPIO_INPUT, KEMPLD_GPIO_OUTPUT_LOW, KEMPLD_GPIO_OUTPUT_HIGH, };
With the enum, one function is enough:
int kempld_configure_gpio(unsigned int gpio, enum kempld_gpio_mode mode) { const u8 mask = KEMPLD_GPIO_MASK(gpio); u8 dir, lvl, val;
switch (mode) { case KEMPLD_GPIO_DEFAULT: return 0; case KEMPLD_GPIO_INPUT: dir = 0; lvl = 0; break; case KEMPLD_GPIO_OUTPUT_LOW: dir = mask; lvl = 0; break; case KEMPLD_GPIO_OUTPUT_HIGH: dir = mask; lvl = mask; break; default: return -1; } if (kempld_get_mutex(100) < 0) return -1;
/* For outputs, set level before direction to avoid glitches */ if (dir) { val = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(gpio)); val &= ~mask; val |= lvl; kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(gpio), val); }
val = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(gpio)); val &= ~mask; val |= dir; kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(gpio), val);
kempld_release_mutex(); return 0; }
I know the switch-block is rather verbose. I did it on purpose, I feel it's clearer this way.
https://review.coreboot.org/c/coreboot/+/47595/comment/1f9ee1f1_d5ef6881 PS5, Line 86: int kempld_gpio_pad_config(struct device *const dev) : { : const struct ec_kontron_kempld_config *const config = dev->chip_info; : int i, status; : : if (!config) : return -1; : : for (i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) { : if (config->gpio_pad[i] & KEMPLD_PAD_GPIO_CFG_DIR(OUT)) : status = kempld_gpio_direction_output(i, config->gpio_pad[i] & 0x1); : else : status = kempld_gpio_direction_input(i); : : if (status < 0) : return status; : } : return 0; : } With the changes suggested above, I'd rewrite this as follows:
int kempld_gpio_pad_config(struct device *const dev) { const struct ec_kontron_kempld_config *const config = dev->chip_info; if (!config) return -1;
for (unsigned int i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) { if (kempld_configure_gpio(i, config->gpios[i]) < 0) return -1; } return 0; }
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/15acf943_0770c581 PS5, Line 33: 8 Given that `KEMPLD_NUM_GPIO_PADS` is 8, the parametrization seems pointless. Or do other kempld's support more than 8 GPIOs? If so, I wouldn't define two macros for each register:
#define KEMPLD_GPIO_DIR(gpio) (0x40 + (gpio) / 8) #define KEMPLD_GPIO_LVL(gpio) (0x42 + (gpio) / 8)
https://review.coreboot.org/c/coreboot/+/47595/comment/628e83dc_47a06a0f PS5, Line 33: / nit: spaces around `/`
Attention is currently required from: Angel Pons. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 8:
(12 comments)
This change is ready for review.
Commit Message:
https://review.coreboot.org/c/coreboot/+/47595/comment/91bab639_c3566075 PS5, Line 11: kempel/kempeld
typos? `kempld/kempld`
It was removed from the message
https://review.coreboot.org/c/coreboot/+/47595/comment/ef8b9753_c46d805b PS5, Line 12: configure/manage each pad using the API
Are there plans to use this API?
Yes, but I decided to do it in a separate patch.
Sometimes carrier board manufacturers need to read the ID, and the easiest way is to use GPIO. In this case, the COMe board uses only GPIO from CPLD.
File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/50235183_87d98a6d PS5, Line 7: KEMPLD_NUM_GPIO_PADS
Why not `KEMPLD_NUM_GPIOS`?
KEMPLD_NUM_GPIOS Done
Also, I have specified the number of pads - 16. But on my board, I can only check the 8 that are used in the connectors of the COMe module.
https://review.coreboot.org/c/coreboot/+/47595/comment/8357328a_3adb137e PS5, Line 9: #define KEMPLD_PAD_GPIO_DIR_IN 0 : #define KEMPLD_PAD_GPIO_DIR_OUT 1 : #define KEMPLD_PAD_GPIO_CFG_DIR(in_out) ((KEMPLD_PAD_GPIO_DIR_##in_out) << 7) : #define KEMPLD_PAD_GPIO_CFG_VAL(v) (v) : #define KEMPLD_PAD_GPIO_CFG_IN KEMPLD_PAD_GPIO_CFG_DIR(IN) : #define KEMPLD_PAD_GPIO_CFG_OUT(v) \ : (KEMPLD_PAD_GPIO_CFG_DIR(OUT) | KEMPLD_PAD_GPIO_CFG_VAL(v))
Do we really need to use macros for this? Why not go with enums, like the rest of the code does? […]
Ok, done. Thanks
https://review.coreboot.org/c/coreboot/+/47595/comment/d5d49011_9065ff07 PS5, Line 38: gpio_pad
I'd call this `gpios`. Also, the type should be `enum kempld_gpio_mode`.
I would prefer gpio in the option name (array), if you don't mind?
File src/ec/kontron/kempld/kempld_gpio.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/d7b9381f_14017866 PS5, Line 6: * Author: Michael Brunner michael.brunner@kontron.com
Is this comment because the code was taken from the Linux kernel? Do we need it?
Yes, from the kernel. In the beginning I tried to keep this code unchanged (the following comments are related to this reason). After the fixes, this is no longer required.
https://review.coreboot.org/c/coreboot/+/47595/comment/d2fe048d_38a19d28 PS5, Line 16: offset
Why is this called `offset`? Unless I'm missing something, it's just the GPIO index?
I would prefer pin_num as the variable name.
https://review.coreboot.org/c/coreboot/+/47595/comment/f9e02f8e_4d1630c9 PS5, Line 18: int status
Why is this an int (should be u8)? Why is it called `status`?
Reworked.
https://review.coreboot.org/c/coreboot/+/47595/comment/90471879_e71918fe PS5, Line 23: (u8)
Are these casts needed? I guess they might be because `offset` is an `unsigned int`.
Reworked. At the moment, this is not needed, since the variables already have the type u8.
https://review.coreboot.org/c/coreboot/+/47595/comment/30730d6d_691d43e6 PS5, Line 33: int status
Why is this an int (should be u8)? Why is it called `status`?
Reworked
https://review.coreboot.org/c/coreboot/+/47595/comment/ec4bd459_254fa7b0 PS5, Line 49: int kempld_gpio_direction_input(unsigned int offset) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : } : : int kempld_gpio_direction_output(unsigned int offset, int value) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); : if (value) : status |= KEMPLD_GPIO_MASK(offset); : else : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status); : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status |= KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : }
This would need to be adapted to use the enum type I suggested in another comment: […]
Thanks for your time on this work, but I would like to separate operations and use functions, if you don't mind? Perhaps this is only the opinion of the author, but in this case, the code will be easier to read if it is divided into functions )
https://review.coreboot.org/c/coreboot/+/47595/comment/3b8a9650_105f9ff8 PS5, Line 86: int kempld_gpio_pad_config(struct device *const dev) : { : const struct ec_kontron_kempld_config *const config = dev->chip_info; : int i, status; : : if (!config) : return -1; : : for (i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) { : if (config->gpio_pad[i] & KEMPLD_PAD_GPIO_CFG_DIR(OUT)) : status = kempld_gpio_direction_output(i, config->gpio_pad[i] & 0x1); : else : status = kempld_gpio_direction_input(i); : : if (status < 0) : return status; : } : return 0; : }
With the changes suggested above, I'd rewrite this as follows: […]
Done. Thanks for your help on this.
Attention is currently required from: Angel Pons. Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Angel Pons, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#9).
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
ec/kontron/kempld: Add minimal GPIO driver
The patch adds an interface for configuring GPIOs inside the Kontron CPLD/EC. This allows to statically define the mode for each pad in devicetree.cb of the motherboard or carrier board. For example:
chip ec/kontron/kempld device gpio 0 on register "gpio[0]" = "KEMPLD_GPIO_INPUT" register "gpio[4]" = "KEMPLD_GPIO_OUTPUT_LOW" register "gpio[5]" = "KEMPLD_GPIO_OUTPUT_HIGH" register "gpio[11]" = "KEMPLD_GPIO_DEFAULT" end end
In this case, <device gpio 0>, like all other devices, is not a real device inside the EC. These definitions are used to understand the EC resources and systematize configuration options, but if mark this as <off>, the initialization step will be skipped in the driver code.
Use KEMPLD_GPIO_DEFAULT or skip it in devicetree.cb to not configure the pad and keep the default mode after CPLD reset.
This work is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL-10 COMe module CB:54380 .
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 5 files changed, 95 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/9
Attention is currently required from: Angel Pons. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 9:
(4 comments)
File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/a479367b_1c4b5ad4 PS5, Line 99: GPIOs configuration
nit: either `GPIO configuration` or `Configuring GPIOs`. I prefer the former.
Done
https://review.coreboot.org/c/coreboot/+/47595/comment/7bc19adb_30c7e042 PS5, Line 100: }
else, warn? it would be consistent with the code above: […]
Done
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/e41511db_17a2e973 PS5, Line 33: /
nit: spaces around `/`
Done
https://review.coreboot.org/c/coreboot/+/47595/comment/0c519f74_b9fd933b PS5, Line 33: 8
Given that `KEMPLD_NUM_GPIO_PADS` is 8, the parametrization seems pointless. […]
Done
Attention is currently required from: Angel Pons. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 9:
(1 comment)
Patchset:
PS9: Please review again. Please notice that some issues aren't resolved, I need your approval. Thanks.
Attention is currently required from: Angel Pons. Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Angel Pons, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#10).
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
ec/kontron/kempld: Add minimal GPIO driver
The patch adds an interface for configuring GPIOs inside the Kontron CPLD/EC. This allows to statically define the mode for each pad in devicetree.cb of the motherboard or carrier board. For example:
chip ec/kontron/kempld device gpio 0 on register "gpio[0]" = "KEMPLD_GPIO_INPUT" register "gpio[4]" = "KEMPLD_GPIO_OUTPUT_LOW" register "gpio[5]" = "KEMPLD_GPIO_OUTPUT_HIGH" register "gpio[11]" = "KEMPLD_GPIO_DEFAULT" end end
In this case, <device gpio 0>, like all other devices, is not a real device inside the EC. These definitions are used to understand the EC resources and systematize configuration options, but if mark this as <off>, the initialization step will be skipped in the driver code.
Use KEMPLD_GPIO_DEFAULT or skip it in devicetree.cb to not configure the pad and keep the default mode after CPLD reset.
This work is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL-10 COMe module CB:54380 .
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 5 files changed, 95 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/10
Attention is currently required from: Maxim Polyakov, Angel Pons. Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Angel Pons, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#11).
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
ec/kontron/kempld: Add minimal GPIO driver
The patch adds an interface for configuring GPIOs inside the Kontron CPLD/EC. This allows to statically define the mode for each pad in devicetree.cb of the motherboard or carrier board. For example:
chip ec/kontron/kempld device gpio 0 on register "gpio[0]" = "KEMPLD_GPIO_INPUT" register "gpio[4]" = "KEMPLD_GPIO_OUTPUT_LOW" register "gpio[5]" = "KEMPLD_GPIO_OUTPUT_HIGH" register "gpio[11]" = "KEMPLD_GPIO_DEFAULT" end end
In this case, <device gpio 0>, like all other devices, is not a real device inside the EC. These definitions are used to understand the EC resources and systematize configuration options, but if mark this as <off>, the initialization step will be skipped in the driver code.
Use KEMPLD_GPIO_DEFAULT or skip it in devicetree.cb to not configure the pad and keep the default mode after CPLD reset.
This work is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL-10 COMe module CB:54380 .
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 5 files changed, 95 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/11
Attention is currently required from: Maxim Polyakov, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 12: Code-Review+2
(5 comments)
Patchset:
PS12: Angel, I marked some of your comments as resolved, hope you don't mind.
One remark: Sometimes it's called `pin` sometimes `pad` that's a bit irritating (however I only noticed after going through the earlier comments).
File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/5b5e2427_63f64233 PS5, Line 38: gpio_pad
I would prefer gpio in the option name (array), if you don't mind?
SGTM.
File src/ec/kontron/kempld/kempld_gpio.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/6e520f59_77213350 PS5, Line 16: offset
I would prefer pin_num as the variable name.
LGTM.
https://review.coreboot.org/c/coreboot/+/47595/comment/5eb0e2ef_302c3dd1 PS5, Line 49: int kempld_gpio_direction_input(unsigned int offset) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : } : : int kempld_gpio_direction_output(unsigned int offset, int value) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); : if (value) : status |= KEMPLD_GPIO_MASK(offset); : else : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status); : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status |= KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : }
Thanks for your time on this work, but I would like to separate operations and use functions, if you […]
New code looks good to me.
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/cd8ff1c9_a883b1e7 PS12, Line 35: const Did I write this? It's not meaningful in a forward declaration.
Attention is currently required from: Maxim Polyakov. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 12: Code-Review+1
(4 comments)
File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/ae0eaf1b_bc4558f5 PS5, Line 38: gpio_pad
SGTM.
Ack
File src/ec/kontron/kempld/kempld_gpio.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/ee016130_ef24ae3c PS5, Line 16: offset
LGTM.
Ack
https://review.coreboot.org/c/coreboot/+/47595/comment/e3972214_b0d3b827 PS5, Line 49: int kempld_gpio_direction_input(unsigned int offset) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : } : : int kempld_gpio_direction_output(unsigned int offset, int value) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); : if (value) : status |= KEMPLD_GPIO_MASK(offset); : else : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status); : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status |= KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : }
New code looks good to me.
Looks good to me as well. While my approach skips acquiring the kempld mutex for `KEMPLD_GPIO_DEFAULT` (a micro-optimisation), your solution is much more elegant and easier to understand. And I prefer easy-to-maintain code over code that's sliiiiightly faster but considerably messier.
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/e05b2232_c01c1597 PS12, Line 31: #define KEMPLD_GPIO_MASK(x) (1 << ((x) % 8)) : #define KEMPLD_GPIO_DIR(pad_num) (0x40 + (pad_num) / 8) : #define KEMPLD_GPIO_LVL(pad_num) (0x42 + (pad_num) / 8) nit: Use the same name for the macro parameters, e.g. `pin_num`?
Attention is currently required from: Maxim Polyakov. Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Angel Pons, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#13).
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
ec/kontron/kempld: Add minimal GPIO driver
The patch adds an interface for configuring GPIOs inside the Kontron CPLD/EC. This allows to statically define the mode for each GPIO pin in devicetree.cb of the motherboard or carrier board. For example:
chip ec/kontron/kempld device gpio 0 on register "gpio[0]" = "KEMPLD_GPIO_INPUT" register "gpio[4]" = "KEMPLD_GPIO_OUTPUT_LOW" register "gpio[5]" = "KEMPLD_GPIO_OUTPUT_HIGH" register "gpio[11]" = "KEMPLD_GPIO_DEFAULT" end end
In this case, <device gpio 0>, like all other devices, is not a real device inside the EC. These definitions are used to understand the EC resources and systematize configuration options, but if mark this as <off>, the initialization step will be skipped in the driver code.
Use KEMPLD_GPIO_DEFAULT or skip it in devicetree.cb to not configure the GPIO and keep the default mode after CPLD reset.
This work is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL-10 COMe module [1].
[1] CB:54380 , Change-Id: I7d354aa32ac8c64f54b2bcbdb4f1b8915f55264e
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 5 files changed, 95 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/13
Attention is currently required from: Angel Pons. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 13:
(2 comments)
Patchset:
PS13: Hi! Thanks for the review again and sorry for the confusion with the "pin/pad" (I fixed this to use "pin" only).
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/a3daf0a2_0173e593 PS12, Line 31: #define KEMPLD_GPIO_MASK(x) (1 << ((x) % 8)) : #define KEMPLD_GPIO_DIR(pad_num) (0x40 + (pad_num) / 8) : #define KEMPLD_GPIO_LVL(pad_num) (0x42 + (pad_num) / 8)
nit: Use the same name for the macro parameters, e.g. […]
Done
Attention is currently required from: Maxim Polyakov. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 13:
(1 comment)
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/456f487f_b606e826 PS12, Line 31: #define KEMPLD_GPIO_MASK(x) (1 << ((x) % 8)) : #define KEMPLD_GPIO_DIR(pad_num) (0x40 + (pad_num) / 8) : #define KEMPLD_GPIO_LVL(pad_num) (0x42 + (pad_num) / 8)
Done
Missed KEMPLD_GPIO_MASK
Attention is currently required from: Maxim Polyakov. Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Angel Pons, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47595
to look at the new patch set (#14).
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
ec/kontron/kempld: Add minimal GPIO driver
The patch adds an interface for configuring GPIOs inside the Kontron CPLD/EC. This allows to statically define the mode for each GPIO pin in devicetree.cb of the motherboard or carrier board. For example:
chip ec/kontron/kempld device gpio 0 on register "gpio[0]" = "KEMPLD_GPIO_INPUT" register "gpio[4]" = "KEMPLD_GPIO_OUTPUT_LOW" register "gpio[5]" = "KEMPLD_GPIO_OUTPUT_HIGH" register "gpio[11]" = "KEMPLD_GPIO_DEFAULT" end end
In this case, <device gpio 0>, like all other devices, is not a real device inside the EC. These definitions are used to understand the EC resources and systematize configuration options, but if mark this as <off>, the initialization step will be skipped in the driver code.
Use KEMPLD_GPIO_DEFAULT or skip it in devicetree.cb to not configure the GPIO and keep the default mode after CPLD reset.
This work is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL-10 COMe module [1].
[1] CB:54380 , Change-Id: I7d354aa32ac8c64f54b2bcbdb4f1b8915f55264e
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 5 files changed, 95 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/47595/14
Attention is currently required from: Maxim Polyakov. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 14: Code-Review+2
Attention is currently required from: Maxim Polyakov. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 14: Code-Review+2
(1 comment)
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/41a157fb_656f5bee PS12, Line 31: #define KEMPLD_GPIO_MASK(x) (1 << ((x) % 8)) : #define KEMPLD_GPIO_DIR(pad_num) (0x40 + (pad_num) / 8) : #define KEMPLD_GPIO_LVL(pad_num) (0x42 + (pad_num) / 8)
Missed KEMPLD_GPIO_MASK
Done
Werner Zeh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
ec/kontron/kempld: Add minimal GPIO driver
The patch adds an interface for configuring GPIOs inside the Kontron CPLD/EC. This allows to statically define the mode for each GPIO pin in devicetree.cb of the motherboard or carrier board. For example:
chip ec/kontron/kempld device gpio 0 on register "gpio[0]" = "KEMPLD_GPIO_INPUT" register "gpio[4]" = "KEMPLD_GPIO_OUTPUT_LOW" register "gpio[5]" = "KEMPLD_GPIO_OUTPUT_HIGH" register "gpio[11]" = "KEMPLD_GPIO_DEFAULT" end end
In this case, <device gpio 0>, like all other devices, is not a real device inside the EC. These definitions are used to understand the EC resources and systematize configuration options, but if mark this as <off>, the initialization step will be skipped in the driver code.
Use KEMPLD_GPIO_DEFAULT or skip it in devicetree.cb to not configure the GPIO and keep the default mode after CPLD reset.
This work is based on code from the drivers/gpio/gpio-kempld.c linux driver. Tested on Kontron mAL-10 COMe module [1].
[1] CB:54380 , Change-Id: I7d354aa32ac8c64f54b2bcbdb4f1b8915f55264e
Change-Id: Id767aa451fbf2ca1c0dccfc9aa2c024c6f37c1bb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47595 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/ec/kontron/kempld/Makefile.inc M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld.c A src/ec/kontron/kempld/kempld_gpio.c M src/ec/kontron/kempld/kempld_internal.h 5 files changed, 95 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/ec/kontron/kempld/Makefile.inc b/src/ec/kontron/kempld/Makefile.inc index 0d59886..e7bafa3 100644 --- a/src/ec/kontron/kempld/Makefile.inc +++ b/src/ec/kontron/kempld/Makefile.inc @@ -2,3 +2,4 @@ ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += early_kempld.c ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += kempld.c ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += kempld_i2c.c +ramstage-$(CONFIG_EC_KONTRON_KEMPLD) += kempld_gpio.c diff --git a/src/ec/kontron/kempld/chip.h b/src/ec/kontron/kempld/chip.h index 30f40fe..c44a103 100644 --- a/src/ec/kontron/kempld/chip.h +++ b/src/ec/kontron/kempld/chip.h @@ -3,7 +3,15 @@ #ifndef EC_KONTRON_KEMPLD_CHIP_H #define EC_KONTRON_KEMPLD_CHIP_H
-#define KEMPLD_NUM_UARTS 2 +#define KEMPLD_NUM_UARTS 2 +#define KEMPLD_NUM_GPIOS 16 + +enum kempld_gpio_mode { + KEMPLD_GPIO_DEFAULT = 0, + KEMPLD_GPIO_INPUT, + KEMPLD_GPIO_OUTPUT_LOW, + KEMPLD_GPIO_OUTPUT_HIGH, +};
enum kempld_uart_io { KEMPLD_UART_3F8 = 0, @@ -26,6 +34,7 @@
struct ec_kontron_kempld_config { struct kempld_uart uart[KEMPLD_NUM_UARTS]; + enum kempld_gpio_mode gpio[KEMPLD_NUM_GPIOS]; unsigned short i2c_frequency; };
diff --git a/src/ec/kontron/kempld/kempld.c b/src/ec/kontron/kempld/kempld.c index f8371a8..0489bac 100644 --- a/src/ec/kontron/kempld/kempld.c +++ b/src/ec/kontron/kempld/kempld.c @@ -93,6 +93,14 @@ printk(BIOS_WARNING, "KEMPLD: Spurious device %s.\n", dev_path(dev)); break; } + } else if (dev->path.type == DEVICE_PATH_GPIO) { + if (dev->path.gpio.id == 0) { + if (kempld_gpio_pads_config(dev) < 0) + printk(BIOS_ERR, "KEMPLD: GPIO configuration failed!\n"); + } else { + printk(BIOS_WARNING, "KEMPLD: Spurious GPIO device %s.\n", + dev_path(dev)); + } } }
diff --git a/src/ec/kontron/kempld/kempld_gpio.c b/src/ec/kontron/kempld/kempld_gpio.c new file mode 100644 index 0000000..8699c1f --- /dev/null +++ b/src/ec/kontron/kempld/kempld_gpio.c @@ -0,0 +1,71 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <stdint.h> +#include <arch/io.h> +#include <delay.h> +#include "chip.h" +#include "kempld.h" +#include "kempld_internal.h" + +enum kempld_gpio_direction { + KEMPLD_GPIO_DIR_IN = 0, + KEMPLD_GPIO_DIR_OUT = 1 +}; + +static void kempld_gpio_value_set(u8 pin_num, u8 value) +{ + const u8 mask = KEMPLD_GPIO_MASK(pin_num); + u8 reg_val = kempld_read8(KEMPLD_GPIO_LVL(pin_num)); + reg_val = value ? reg_val | mask : reg_val & ~mask; + kempld_write8(KEMPLD_GPIO_LVL(pin_num), reg_val); +} + +static void kempld_gpio_direction_set(u8 pin_num, enum kempld_gpio_direction dir) +{ + const u8 mask = KEMPLD_GPIO_MASK(pin_num); + u8 reg_val = kempld_read8(KEMPLD_GPIO_DIR(pin_num)); + reg_val = dir == KEMPLD_GPIO_DIR_OUT ? reg_val | mask : reg_val & ~mask; + kempld_write8(KEMPLD_GPIO_DIR(pin_num), reg_val); +} + +static int kempld_configure_gpio(u8 pin_num, enum kempld_gpio_mode mode) +{ + if (kempld_get_mutex(100) < 0) + return -1; + + switch (mode) { + case KEMPLD_GPIO_DEFAULT: + break; + + case KEMPLD_GPIO_INPUT: + kempld_gpio_direction_set(pin_num, KEMPLD_GPIO_DIR_IN); + break; + + case KEMPLD_GPIO_OUTPUT_LOW: + kempld_gpio_value_set(pin_num, 0); + kempld_gpio_direction_set(pin_num, KEMPLD_GPIO_DIR_OUT); + break; + + case KEMPLD_GPIO_OUTPUT_HIGH: + kempld_gpio_value_set(pin_num, 1); + kempld_gpio_direction_set(pin_num, KEMPLD_GPIO_DIR_OUT); + break; + } + + kempld_release_mutex(); + return 0; +} + +int kempld_gpio_pads_config(struct device *dev) +{ + const struct ec_kontron_kempld_config *config = dev->chip_info; + + if (!config) + return -1; + + for (u8 i = 0; i < KEMPLD_NUM_GPIOS; ++i) { + if (kempld_configure_gpio(i, config->gpio[i]) < 0) + return -1; + } + return 0; +} diff --git a/src/ec/kontron/kempld/kempld_internal.h b/src/ec/kontron/kempld/kempld_internal.h index 993fe8e..2c64f79 100644 --- a/src/ec/kontron/kempld/kempld_internal.h +++ b/src/ec/kontron/kempld/kempld_internal.h @@ -28,6 +28,11 @@
#define KEMPLD_CLK 33333333 /* 33MHz */
+#define KEMPLD_GPIO_MASK(pin_num) (1 << ((pin_num) % 8)) +#define KEMPLD_GPIO_DIR(pin_num) (0x40 + (pin_num) / 8) +#define KEMPLD_GPIO_LVL(pin_num) (0x42 + (pin_num) / 8) + void kempld_i2c_device_init(struct device *const dev); +int kempld_gpio_pads_config(struct device *dev);
#endif /* EC_KONTRON_KEMPLD_INTERNAL_H */
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Maxim Polyakov (Code Review)
-
Nico Huber (Code Review)
-
Werner Zeh (Code Review)