[U-Boot] [PATCH v2 0/9] x86: gpio: Tidy up GPIO driver
The ICH GPIO driver does not work correctly at present. This series tidies things up and fixes a few bugs.
One issue remaining is that reading from output GPIOs does not work - it always returns 0.
Changes in v2: - Adjust pci_get_bus() to probe bus 0 so that the required bus is found - Fix 'micros' typo
Simon Glass (9): x86: minnowmax: Add access to GPIOs E0, E1, E2 dm: pci: Allow a PCI bus to be found without an alias dm: pci: Add a comment to help find pci_hose_read_config_byte, etc. x86: minnowmax: Correct pad-offset value for host_en1 gpio: Report errors when GPIOs cannot be read x86: gpio: Correct calls to _ich6_gpio_set_direction() x86: gpio: Tidy up gpio_ich6_get_base() and callers WIP: x86: gpio: Handle the case where reading a GPIO fails WIP: x86: gpio: Make sure that output GPIOs can be read
arch/x86/dts/minnowmax.dts | 29 ++++++++++++++++++++++++++++- common/cmd_gpio.c | 34 +++++++++++++++++++++++++++------- drivers/gpio/intel_ich6_gpio.c | 38 +++++++++++++++++++++++++++++++------- drivers/pci/pci-uclass.c | 23 ++++++++++++++++++++--- include/pci.h | 1 + 5 files changed, 107 insertions(+), 18 deletions(-)
These GPIOs are accessible on the pin header. Add pinctrl settings for them so that we they can be adjusted using the 'gpio' command.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
arch/x86/dts/minnowmax.dts | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts index f4e0a35..a8ecf0d 100644 --- a/arch/x86/dts/minnowmax.dts +++ b/arch/x86/dts/minnowmax.dts @@ -30,6 +30,33 @@ compatible = "intel,x86-pinctrl"; io-base = <0x4c>;
+ /* GPIO E0 */ + soc_gpio_s5_0@0 { + gpio-offset = <0x80 0>; + pad-offset = <0x1d0>; + mode-gpio; + output-value = <0>; + direction = <PIN_OUTPUT>; + }; + + /* GPIO E1 */ + soc_gpio_s5_1@0 { + gpio-offset = <0x80 1>; + pad-offset = <0x210>; + mode-gpio; + output-value = <0>; + direction = <PIN_OUTPUT>; + }; + + /* GPIO E2 */ + soc_gpio_s5_2@0 { + gpio-offset = <0x80 2>; + pad-offset = <0x1e0>; + mode-gpio; + output-value = <0>; + direction = <PIN_OUTPUT>; + }; + pin_usb_host_en0@0 { gpio-offset = <0x80 8>; pad-offset = <0x260>;
On 22 August 2015 at 14:58, Simon Glass sjg@chromium.org wrote:
These GPIOs are accessible on the pin header. Add pinctrl settings for them so that we they can be adjusted using the 'gpio' command.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
arch/x86/dts/minnowmax.dts | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
Applied to u-boot-x86.
At present, until a PCI bus is probed, it cannot be found by its sequence number unless it has an alias. This is the same with any device.
However with PCI this is more annoying than usual, since bus 0 is always the same device.
Add a function that tries a little harder to locate PCI bus 0. This means that PCI enumeration will happen automatically on the first access.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Adjust pci_get_bus() to probe bus 0 so that the required bus is found
drivers/pci/pci-uclass.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index c90e7ac..ca47dc3 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -20,6 +20,23 @@
DECLARE_GLOBAL_DATA_PTR;
+static int pci_get_bus(int busnum, struct udevice **busp) +{ + int ret; + + ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, busp); + + /* Since buses may not be numbered yet try a little harder with bus 0 */ + if (ret == -ENODEV) { + ret = uclass_first_device(UCLASS_PCI, busp); + if (!ret) + return -ENODEV; + ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, busp); + } + + return ret; +} + struct pci_controller *pci_bus_to_hose(int busnum) { struct udevice *bus; @@ -128,7 +145,7 @@ int pci_bus_find_bdf(pci_dev_t bdf, struct udevice **devp) struct udevice *bus; int ret;
- ret = uclass_get_device_by_seq(UCLASS_PCI, PCI_BUS(bdf), &bus); + ret = pci_get_bus(PCI_BUS(bdf), &bus); if (ret) return ret; return pci_bus_find_devfn(bus, PCI_MASK_BUS(bdf), devp); @@ -206,7 +223,7 @@ int pci_write_config(pci_dev_t bdf, int offset, unsigned long value, struct udevice *bus; int ret;
- ret = uclass_get_device_by_seq(UCLASS_PCI, PCI_BUS(bdf), &bus); + ret = pci_get_bus(PCI_BUS(bdf), &bus); if (ret) return ret;
@@ -271,7 +288,7 @@ int pci_read_config(pci_dev_t bdf, int offset, unsigned long *valuep, struct udevice *bus; int ret;
- ret = uclass_get_device_by_seq(UCLASS_PCI, PCI_BUS(bdf), &bus); + ret = pci_get_bus(PCI_BUS(bdf), &bus); if (ret) return ret;
Hi Simon,
On Sun, Aug 23, 2015 at 5:58 AM, Simon Glass sjg@chromium.org wrote:
At present, until a PCI bus is probed, it cannot be found by its sequence number unless it has an alias. This is the same with any device.
However with PCI this is more annoying than usual, since bus 0 is always the same device.
Add a function that tries a little harder to locate PCI bus 0. This means that PCI enumeration will happen automatically on the first access.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Adjust pci_get_bus() to probe bus 0 so that the required bus is found
drivers/pci/pci-uclass.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index c90e7ac..ca47dc3 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -20,6 +20,23 @@
DECLARE_GLOBAL_DATA_PTR;
+static int pci_get_bus(int busnum, struct udevice **busp) +{
int ret;ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, busp);/* Since buses may not be numbered yet try a little harder with bus 0 */if (ret == -ENODEV) {ret = uclass_first_device(UCLASS_PCI, busp);if (!ret)
By looking at the API comments of uclass_first_device(), I think this logic should be:
if (ret || ((!ret) && (!*busp)))
return -ENODEV;ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, busp);}return ret;+}
struct pci_controller *pci_bus_to_hose(int busnum) { struct udevice *bus; @@ -128,7 +145,7 @@ int pci_bus_find_bdf(pci_dev_t bdf, struct udevice **devp) struct udevice *bus; int ret;
ret = uclass_get_device_by_seq(UCLASS_PCI, PCI_BUS(bdf), &bus);
ret = pci_get_bus(PCI_BUS(bdf), &bus); if (ret) return ret; return pci_bus_find_devfn(bus, PCI_MASK_BUS(bdf), devp);@@ -206,7 +223,7 @@ int pci_write_config(pci_dev_t bdf, int offset, unsigned long value, struct udevice *bus; int ret;
ret = uclass_get_device_by_seq(UCLASS_PCI, PCI_BUS(bdf), &bus);
ret = pci_get_bus(PCI_BUS(bdf), &bus); if (ret) return ret;@@ -271,7 +288,7 @@ int pci_read_config(pci_dev_t bdf, int offset, unsigned long *valuep, struct udevice *bus; int ret;
ret = uclass_get_device_by_seq(UCLASS_PCI, PCI_BUS(bdf), &bus);
ret = pci_get_bus(PCI_BUS(bdf), &bus); if (ret) return ret;--
I think we need also update pci_bus_to_hose() to use pci_get_bus() instead of calling uclass_get_device_by_seq().
Regards, Bin
These functions are defined by macros so do not show up with grep. Add a comment to help.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com ---
Changes in v2: - Fix 'micros' typo
include/pci.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/pci.h b/include/pci.h index 488ff44..e24c970 100644 --- a/include/pci.h +++ b/include/pci.h @@ -653,6 +653,7 @@ extern pci_addr_t pci_hose_phys_to_bus(struct pci_controller* hose, #define pci_io_to_virt(dev, addr, len, map_flags) \ pci_bus_to_virt((dev), (addr), PCI_REGION_IO, (len), (map_flags))
+/* For driver model these are defined in macros in pci_compat.c */ extern int pci_hose_read_config_byte(struct pci_controller *hose, pci_dev_t dev, int where, u8 *val); extern int pci_hose_read_config_word(struct pci_controller *hose,
On 22 August 2015 at 14:58, Simon Glass sjg@chromium.org wrote:
These functions are defined by macros so do not show up with grep. Add a comment to help.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com
Changes in v2:
- Fix 'micros' typo
include/pci.h | 1 + 1 file changed, 1 insertion(+)
Applied to u-boot-x86.
This should be 0x250, not 0x258. Fix it.
Reported-by: Andrew Bradford andrew.bradford@kodakalaris.com Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
arch/x86/dts/minnowmax.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts index a8ecf0d..e917f0f 100644 --- a/arch/x86/dts/minnowmax.dts +++ b/arch/x86/dts/minnowmax.dts @@ -67,7 +67,7 @@
pin_usb_host_en1@0 { gpio-offset = <0x80 9>; - pad-offset = <0x258>; + pad-offset = <0x250>; mode-gpio; output-value = <1>; direction = <PIN_OUTPUT>;
On 22 August 2015 at 14:58, Simon Glass sjg@chromium.org wrote:
This should be 0x250, not 0x258. Fix it.
Reported-by: Andrew Bradford andrew.bradford@kodakalaris.com Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
arch/x86/dts/minnowmax.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to u-boot-x86.
Some controllers do not allow the output value to be read. Detect this and report the error in that case.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/cmd_gpio.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c index 65d6df4..7cd1ba1 100644 --- a/common/cmd_gpio.c +++ b/common/cmd_gpio.c @@ -119,7 +119,7 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { unsigned int gpio; enum gpio_cmd sub_cmd; - ulong value; + int value; const char *str_cmd, *str_gpio = NULL; int ret; #ifdef CONFIG_DM_GPIO @@ -195,15 +195,35 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) value = gpio_get_value(gpio); } else { switch (sub_cmd) { - case GPIO_SET: value = 1; break; - case GPIO_CLEAR: value = 0; break; - case GPIO_TOGGLE: value = !gpio_get_value(gpio); break; - default: goto show_usage; + case GPIO_SET: + value = 1; + break; + case GPIO_CLEAR: + value = 0; + break; + case GPIO_TOGGLE: + value = gpio_get_value(gpio); + if (!IS_ERR_VALUE(value)) + value = !value; + break; + default: + goto show_usage; } gpio_direction_output(gpio, value); } - printf("gpio: pin %s (gpio %i) value is %lu\n", - str_gpio, gpio, value); + printf("gpio: pin %s (gpio %i) value is ", str_gpio, gpio); + if (IS_ERR_VALUE(value)) + printf("unknown (ret=%d)\n", value); + else + printf("%d\n", value); + if (sub_cmd != GPIO_INPUT && !IS_ERR_VALUE(value)) { + int nval = gpio_get_value(gpio); + + if (IS_ERR_VALUE(nval)) + printf(" Warning: no access to GPIO output value\n"); + else if (nval != value) + printf(" Warning: value of pin is still %d\n", nval); + }
if (ret != -EBUSY) gpio_free(gpio);
These calls seem to be incorrect. The function expects an I/O address but the existing callers pass the value at an I/O address. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
drivers/gpio/intel_ich6_gpio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/intel_ich6_gpio.c b/drivers/gpio/intel_ich6_gpio.c index cb408a4..fd1f287 100644 --- a/drivers/gpio/intel_ich6_gpio.c +++ b/drivers/gpio/intel_ich6_gpio.c @@ -410,7 +410,7 @@ static int ich6_gpio_direction_input(struct udevice *dev, unsigned offset) { struct ich6_bank_priv *bank = dev_get_priv(dev);
- return _ich6_gpio_set_direction(inl(bank->io_sel), offset, 0); + return _ich6_gpio_set_direction(bank->io_sel, offset, 0); }
static int ich6_gpio_direction_output(struct udevice *dev, unsigned offset, @@ -419,7 +419,7 @@ static int ich6_gpio_direction_output(struct udevice *dev, unsigned offset, int ret; struct ich6_bank_priv *bank = dev_get_priv(dev);
- ret = _ich6_gpio_set_direction(inl(bank->io_sel), offset, 1); + ret = _ich6_gpio_set_direction(bank->io_sel, offset, 1); if (ret) return ret;
On 22 August 2015 at 14:58, Simon Glass sjg@chromium.org wrote:
These calls seem to be incorrect. The function expects an I/O address but the existing callers pass the value at an I/O address. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
drivers/gpio/intel_ich6_gpio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Applied to u-boot-x86.
This function can return an error. Correct the detection of this error so that it works even with large 32-bit addresses.
The return value is set up for returning an I/O address but the function is also used to return a memory-mapped address. Adjust the return code to make this work.
Also add a bit more debugging.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
drivers/gpio/intel_ich6_gpio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/intel_ich6_gpio.c b/drivers/gpio/intel_ich6_gpio.c index fd1f287..67bf0a2 100644 --- a/drivers/gpio/intel_ich6_gpio.c +++ b/drivers/gpio/intel_ich6_gpio.c @@ -75,7 +75,7 @@ static int gpio_ich6_get_base(unsigned long base) /* Is the device present? */ tmpword = x86_pci_read_config16(pci_dev, PCI_VENDOR_ID); if (tmpword != PCI_VENDOR_ID_INTEL) { - debug("%s: wrong VendorID\n", __func__); + debug("%s: wrong VendorID %x\n", __func__, tmpword); return -ENODEV; }
@@ -144,7 +144,7 @@ static int gpio_ich6_get_base(unsigned long base) * at the offset that we just read. Bit 0 indicates that it's * an I/O address, not a memory address, so mask that off. */ - return tmplong & 0xfffc; + return tmplong & 1 ? tmplong & ~3 : tmplong & ~15; }
static int _ich6_gpio_set_value(uint16_t base, unsigned offset, int value) @@ -324,7 +324,7 @@ int gpio_ich6_pinctrl_init(void) debug("%s: io-base offset not present\n", __func__); } else { iobase = gpio_ich6_get_base(iobase_offset); - if (iobase < 0) { + if (IS_ERR_VALUE(iobase)) { debug("%s: invalid IOBASE address (%08x)\n", __func__, iobase); return -EINVAL;
On 22 August 2015 at 14:58, Simon Glass sjg@chromium.org wrote:
This function can return an error. Correct the detection of this error so that it works even with large 32-bit addresses.
The return value is set up for returning an I/O address but the function is also used to return a memory-mapped address. Adjust the return code to make this work.
Also add a bit more debugging.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
drivers/gpio/intel_ich6_gpio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Applied to u-boot-x86.
It should be possible to read an output GPIO. For now it is not clear how to do this. Perhaps the driver should be adjusted to use memory-mapped access throughout. For now, return an error to avoid confusion.
Not to apply.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
drivers/gpio/intel_ich6_gpio.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpio/intel_ich6_gpio.c b/drivers/gpio/intel_ich6_gpio.c index 67bf0a2..5cc86d7 100644 --- a/drivers/gpio/intel_ich6_gpio.c +++ b/drivers/gpio/intel_ich6_gpio.c @@ -195,6 +195,14 @@ static int _ich6_gpio_set_direction(uint16_t base, unsigned offset, int dir) return 0; }
+static int _ich6_gpio_get_direction(uint16_t base, unsigned offset) +{ + u32 val; + + val = inl(base); + return !(val & (1UL << offset)); +} + static int _gpio_ich6_pinctrl_cfg_pin(s32 gpiobase, s32 iobase, int pin_node) { u32 gpio_offset[2]; @@ -432,6 +440,13 @@ static int ich6_gpio_get_value(struct udevice *dev, unsigned offset) u32 tmplong; int r;
+ /* + * TODO: Mirror this register since reading only works when it is an + * input, Alternatively figure out how to make the hardware work with + * this. + */ + if (_ich6_gpio_get_direction(bank->io_sel, offset)) + return -EIO; tmplong = inl(bank->lvl); r = (tmplong & (1UL << offset)) ? 1 : 0; return r;
The datasheet suggests that the state of an output pin can be read. In fact this does not seem to work, perhaps because we are also accessing the GPIO state through I/O instead of just memory-mapped.
Not to apply.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
drivers/gpio/intel_ich6_gpio.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/intel_ich6_gpio.c b/drivers/gpio/intel_ich6_gpio.c index 5cc86d7..204a4b4 100644 --- a/drivers/gpio/intel_ich6_gpio.c +++ b/drivers/gpio/intel_ich6_gpio.c @@ -49,6 +49,8 @@ struct ich6_bank_priv { #define GPIO_USESEL_OFFSET(x) (x) #define GPIO_IOSEL_OFFSET(x) (x + 4) #define GPIO_LVL_OFFSET(x) (x + 8) +#define IOPAD_VAL_OFFSET(x) (x + 8) +#define IINENB_SHIFT 2
#define IOPAD_MODE_MASK 0x7 #define IOPAD_PULL_ASSIGN_SHIFT 7 @@ -210,6 +212,7 @@ static int _gpio_ich6_pinctrl_cfg_pin(s32 gpiobase, s32 iobase, int pin_node) int val; int ret; const void *prop; + bool mode_gpio = false;
/* * GPIO node is not mandatory, so we only do the @@ -221,11 +224,13 @@ static int _gpio_ich6_pinctrl_cfg_pin(s32 gpiobase, s32 iobase, int pin_node) /* Do we want to force the GPIO mode? */ prop = fdt_getprop(gd->fdt_blob, pin_node, "mode-gpio", NULL); - if (prop) + if (prop) { _ich6_gpio_set_function(GPIO_USESEL_OFFSET (gpiobase) + gpio_offset[0], gpio_offset[1], 1); + mode_gpio = true; + }
val = fdtdec_get_int(gd->fdt_blob, pin_node, "direction", -1); @@ -270,8 +275,12 @@ static int _gpio_ich6_pinctrl_cfg_pin(s32 gpiobase, s32 iobase, int pin_node) * be just ignored by the controller */ val = fdtdec_get_int(gd->fdt_blob, pin_node, "mode-func", -1); - if (val != -1) + if (val != -1) { clrsetbits_le32(iobase_addr, IOPAD_MODE_MASK, val); + } else if (mode_gpio) { + clrbits_le32(IOPAD_VAL_OFFSET(iobase_addr), + 1 << IINENB_SHIFT); + }
/* Configure the pull-up/down if needed */ val = fdtdec_get_int(gd->fdt_blob, pin_node, "pull-assign", -1);
participants (2)
-
Bin Meng -
Simon Glass