[U-Boot] [PATCH 3/4 v2] x86: gpio: add pinctrl support from the device tree
gabriel huau
contact at huau-gabriel.fr
Tue May 12 05:33:14 CEST 2015
Hi Simon,
Sorry for the delay, I'm gonna provide a new version in the next few
days but here is some answers to your question:
On 04/28/2015 06:53 AM, Simon Glass wrote:
> Hi Gabriel,
>
> On 25 April 2015 at 14:17, Gabriel Huau <contact at huau-gabriel.fr> wrote:
>> Every pin can be configured now from the device tree. A dt-bindings
>> has been added to describe the different property available.
>>
>> Signed-off-by: Gabriel Huau <contact at huau-gabriel.fr>
>> ---
>> Changes for v2:
>> - Clean commit message
>> - Rename compatible string 'ich6' to 'x86'
>> - Fix coding style
>> - Create a dt-bindinds documentation
>> - Move x86-gpio defines to a specific header
>> - Reorder the functions to avoid the need for forward declarations
>> - Rename double underscore functions to only one
>> - Create a specific function to configure one pin
>> - Use a define to prevent build/support issues with other x86 CPU that
>> doesn't have a IOBASE.
> I have a few minor comments below. Do you know how to access the GPIO
> pings on the top connector of the Minnowboard MAX? I'd like to figure
> out the pin names for those in U-Boot and that would allow me to test
> a few things.
You should be able to access SOC_GPIO_S5_0, SOC_GPIO_S5_1, SOC_GPIO_S5_2
GPIO_BASE should be 0x80 (respecting bit 0 1 and 2).
IO_BASE should be 0x1D0, 0x210, 0x1E0 (respect GPIO0, 1 and 2).
>> arch/x86/dts/minnowmax.dts | 21 ++
>> arch/x86/include/asm/arch-baytrail/gpio.h | 1 +
>> arch/x86/include/asm/gpio.h | 1 +
>> .../gpio/intel,x86-pinctrl.txt | 31 +++
>> drivers/gpio/intel_ich6_gpio.c | 234 ++++++++++++++++++---
>> include/dt-bindings/gpio/x86-gpio.h | 36 ++++
>> 6 files changed, 295 insertions(+), 29 deletions(-)
>> create mode 100644 doc/device-tree-bindings/gpio/intel,x86-pinctrl.txt
>> create mode 100644 include/dt-bindings/gpio/x86-gpio.h
>>
>> diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts
>> index c73e421..ea10963 100644
>> --- a/arch/x86/dts/minnowmax.dts
>> +++ b/arch/x86/dts/minnowmax.dts
>> @@ -6,6 +6,8 @@
>>
>> /dts-v1/;
>>
>> +#include <dt-bindings/gpio/x86-gpio.h>
>> +
>> /include/ "skeleton.dtsi"
>> /include/ "serial.dtsi"
>>
>> @@ -21,6 +23,25 @@
>> silent_console = <0>;
>> };
>>
>> + pch_pinctrl {
>> + compatible = "intel,x86-pinctrl";
>> + pin_usb_host_en0 at 0 {
>> + gpio-offset = <0x80 8>;
>> + pad-offset = <0x260>;
>> + mode-gpio;
>> + output-value = <1>;
>> + direction = <PIN_OUTPUT>;
>> + };
>> +
>> + pin_usb_host_en1 at 0 {
>> + gpio-offset = <0x80 9>;
>> + pad-offset = <0x258>;
>> + mode-gpio;
>> + output-value = <1>;
>> + direction = <PIN_OUTPUT>;
>> + };
>> + };
>> +
>> gpioa {
>> compatible = "intel,ich6-gpio";
>> u-boot,dm-pre-reloc;
>> diff --git a/arch/x86/include/asm/arch-baytrail/gpio.h b/arch/x86/include/asm/arch-baytrail/gpio.h
>> index 4e8987c..85a65a8 100644
>> --- a/arch/x86/include/asm/arch-baytrail/gpio.h
>> +++ b/arch/x86/include/asm/arch-baytrail/gpio.h
>> @@ -9,5 +9,6 @@
>>
>> /* Where in config space is the register that points to the GPIO registers? */
>> #define PCI_CFG_GPIOBASE 0x48
>> +#define PCI_CFG_IOBASE 0x4c
> Can we put this in the device tree as a property of the pch_pinctrl
> node? If you like we could do it later.
Yes, I will do the modification, I thought as a first version it would
be easier to use a define but actually, a property is cleaner and also
easy to implement.
>> #endif /* _X86_ARCH_GPIO_H_ */
>> diff --git a/arch/x86/include/asm/gpio.h b/arch/x86/include/asm/gpio.h
>> index 1099427..ed85b08 100644
>> --- a/arch/x86/include/asm/gpio.h
>> +++ b/arch/x86/include/asm/gpio.h
>> @@ -147,6 +147,7 @@ struct pch_gpio_map {
>> } set3;
>> };
>>
>> +int gpio_ich6_pinctrl_init(void);
>> void setup_pch_gpios(u16 gpiobase, const struct pch_gpio_map *gpio);
>> void ich_gpio_set_gpio_map(const struct pch_gpio_map *map);
>>
>> diff --git a/doc/device-tree-bindings/gpio/intel,x86-pinctrl.txt b/doc/device-tree-bindings/gpio/intel,x86-pinctrl.txt
>> new file mode 100644
>> index 0000000..45ab1af
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/gpio/intel,x86-pinctrl.txt
>> @@ -0,0 +1,31 @@
>> +Intel x86 PINCTRL/GPIO controller
>> +
>> +Pin-muxing on x86 can be described with a node for the PINCTRL master
>> +node and a set of child nodes for each pin on the SoC.
>> +
>> +The PINCTRL master node requires the following properties:
>> +- compatible : "intel,x86-pinctrl"
>> +
>> +Pin nodes must be children of the pinctrl master node and can
>> +contain the following properties:
>> +- pad-offset - (required) offset in the IOBASE for the pin to configured.
>> +- gpio-offset - (required) offset in the GPIOBASE for the pin to configured and
>> + also the bit shift in this register.
>> +- mode-gpio - (optional) standalone property to force the pin into GPIO mode.
>> +- mode-func - (optional) function number to assign to the pin. if 'mode-gpio'
>> + is set, this property will be ignored.
>> +in case of 'mode-gpio' property set:
>> +- output-value - (optional) this set the default output value of the GPIO.
>> +- direction - (optional) this set the direction of the gpio.
>> +- pull-str - (optional) this set the pull strength of the pin.
>> +- pull-assign - (optional) this set the pull assignement (up/down) of the pin.
>> +
>> +Example:
>> +
>> +pin_usb_host_en0 at 0 {
>> + gpio-offset = <0x80 8>;
>> + pad-offset = <0x260>;
>> + mode-gpio;
>> + output-value = <1>;
>> + direction = <PIN_OUTPUT>;
>> +};
>> diff --git a/drivers/gpio/intel_ich6_gpio.c b/drivers/gpio/intel_ich6_gpio.c
>> index 7e679a0..c18c60f 100644
>> --- a/drivers/gpio/intel_ich6_gpio.c
>> +++ b/drivers/gpio/intel_ich6_gpio.c
>> @@ -44,21 +44,28 @@ struct ich6_bank_priv {
>> uint16_t lvl;
>> };
>>
>> +#define GPIO_USESEL_OFFSET(x) (x)
>> +#define GPIO_IOSEL_OFFSET(x) (x + 4)
>> +#define GPIO_LVL_OFFSET(x) (x + 8)
> Please align all of these values to the same tab column.
>
I will do.
>> +
>> +#define IOPAD_MODE_MASK 0x7
>> +#define IOPAD_PULL_ASSIGN_SHIFT 7
>> +#define IOPAD_PULL_ASSIGN_MASK (0x3 << IOPAD_PULL_ASSIGN_SHIFT)
>> +#define IOPAD_PULL_STRENGTH_SHIFT 9
>> +#define IOPAD_PULL_STRENGTH_MASK (0x3 << IOPAD_PULL_STRENGTH_SHIFT)
>> +
>> /* TODO: Move this to device tree, or platform data */
>> void ich_gpio_set_gpio_map(const struct pch_gpio_map *map)
>> {
>> gd->arch.gpio_map = map;
>> }
>>
>> -static int gpio_ich6_ofdata_to_platdata(struct udevice *dev)
>> +static int gpio_ich6_get_base(unsigned long base)
>> {
>> - struct ich6_bank_platdata *plat = dev_get_platdata(dev);
>> pci_dev_t pci_dev; /* handle for 0:1f:0 */
>> u8 tmpbyte;
>> u16 tmpword;
>> u32 tmplong;
>> - u16 gpiobase;
>> - int offset;
>>
>> /* Where should it be? */
>> pci_dev = PCI_BDF(0, 0x1f, 0);
>> @@ -123,9 +130,9 @@ static int gpio_ich6_ofdata_to_platdata(struct udevice *dev)
>> * while on the Ivybridge the bit0 is used to indicate it is an
>> * I/O space.
>> */
>> - tmplong = x86_pci_read_config32(pci_dev, PCI_CFG_GPIOBASE);
>> + tmplong = x86_pci_read_config32(pci_dev, base);
>> if (tmplong == 0x00000000 || tmplong == 0xffffffff) {
>> - debug("%s: unexpected GPIOBASE value\n", __func__);
>> + debug("%s: unexpected BASE value\n", __func__);
>> return -ENODEV;
>> }
>>
>> @@ -135,7 +142,195 @@ static int gpio_ich6_ofdata_to_platdata(struct udevice *dev)
>> * 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.
>> */
>> - gpiobase = tmplong & 0xfffe;
>> + return tmplong & 0xfffc;
>> +}
>> +
>> +static int _ich6_gpio_set_value(uint16_t base, unsigned offset, int value)
>> +{
>> + u32 tmplong;
> blank line here. Also can we use something like 'val' instead of
> tmplong? I think calling a u32 'long' is confusing.
Agreed, I was just trying to reuse the variable for some other property,
but it was a bad idea as it also introduced some errors due to the sign
of the type.
>> + tmplong = inl(base);
>> + if (value)
>> + tmplong |= (1UL << offset);
>> + else
>> + tmplong &= ~(1UL << offset);
>> + outl(tmplong, base);
>> +
>> + return 0;
>> +}
>> +
>> +static int _ich6_gpio_set_function(uint16_t base, unsigned offset, int func)
>> +{
>> + u32 tmplong;
>> +
>> + if (func) {
>> + tmplong = inl(base);
>> + tmplong |= (1UL << offset);
>> + outl(tmplong, base);
>> + } else {
>> + tmplong = inl(base);
>> + tmplong &= ~(1UL << offset);
>> + outl(tmplong, base);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int _ich6_gpio_set_direction(uint16_t base, unsigned offset, int dir)
>> +{
>> + u32 tmplong;
>> +
>> + if (!dir) {
>> + tmplong = inl(base);
>> + tmplong |= (1UL << offset);
>> + outl(tmplong, base);
>> + } else {
>> + tmplong = inl(base);
>> + tmplong &= ~(1UL << offset);
>> + outl(tmplong, base);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int _gpio_ich6_pinctrl_cfg_pin(u32 gpiobase, u32 iobase, int pin_node)
>> +{
>> + u32 gpio_offset[2] = { -1, -1 };
>> + u32 pad_offset;
>> + int tmplong;
>> + const void *tmpnode;
> Perhaps 'prop' is a better name. It doesn't point to a node, but a
> property of a node.
Agreed.
>> +
>> + /*
>> + * GPIO node is not mandatory, so we only do the
>> + * pinmuxing if the node exist.
>> + */
>> + fdtdec_get_int_array(gd->fdt_blob, pin_node, "gpio-offset",
>> + gpio_offset, 2);
>> + if (gpio_offset[0] != -1) {
>> + /* Do we want to force the GPIO mode? */
>> + tmpnode = fdt_getprop(gd->fdt_blob, pin_node, "mode-gpio",
>> + NULL);
>> + if (tmpnode)
> {} around these multi-line blocks. If you use patman it will run
> checkpatch for you.
>
Actually, I'm running checkpatch on all the patches all the time, I'm
pretty surprised to have miss it. I will definitely try to use patman,
>> + _ich6_gpio_set_function(GPIO_USESEL_OFFSET
>> + (gpiobase) +
>> + gpio_offset[0],
>> + gpio_offset[1], 1);
>> +
>> + tmplong =
>> + fdtdec_get_int(gd->fdt_blob, pin_node, "direction", -1);
>> + if (tmplong != -1)
>> + _ich6_gpio_set_direction(GPIO_IOSEL_OFFSET
>> + (gpiobase) +
>> + gpio_offset[0],
>> + gpio_offset[1], tmplong);
>> +
>> + tmplong =
>> + fdtdec_get_int(gd->fdt_blob, pin_node, "output-value", -1);
>> + if (tmplong != -1)
>> + _ich6_gpio_set_value(GPIO_LVL_OFFSET(gpiobase)
>> + + gpio_offset[0],
>> + gpio_offset[1], tmplong);
>> + }
>> +#ifdef PCI_CFG_IOBASE
> We should avoid adding #ifdefs like this to the driver. Perhaps the
> device tree can provide this property, or not? OK if you want to
> tackle this in a follow-on patch.
No I think you are right, I can do the modification for the v3.
>> + /*
>> + * The offset for the same pin for the IOBASE and GPIOBASE are
>> + * different, so instead of maintaining a lookup table,
>> + * the device tree should provide directly the correct
>> + * value for both mapping.
>> + */
>> + pad_offset = fdtdec_get_int(gd->fdt_blob, pin_node, "pad-offset", -1);
>> + if (pad_offset == -1) {
>> + debug("%s: Invalid register io offset %d\n",
>> + __func__, pad_offset);
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Do we need to set a specific function mode?
>> + * If someone put also 'mode-gpio', this option will
>> + * be just ignored by the controller
>> + */
>> + tmplong = fdtdec_get_int(gd->fdt_blob, pin_node, "mode-func", -1);
>> + if (tmplong != -1)
>> + clrsetbits_le32(iobase + pad_offset, IOPAD_MODE_MASK, tmplong);
>> +
>> + /* Configure the pull-up/down if needed */
>> + tmplong = fdtdec_get_int(gd->fdt_blob, pin_node, "pull-assign", -1);
>> + if (tmplong != -1)
>> + clrsetbits_le32(iobase + pad_offset,
> Can you create a local variable for (iobase + pad_offset) ?
I will do.
>> + IOPAD_PULL_ASSIGN_MASK,
>> + tmplong << IOPAD_PULL_ASSIGN_SHIFT);
>> +
>> + tmplong = fdtdec_get_int(gd->fdt_blob, pin_node, "pull-strength", -1);
>> + if (tmplong != -1)
>> + clrsetbits_le32(iobase + pad_offset,
>> + IOPAD_PULL_STRENGTH_MASK,
>> + tmplong << IOPAD_PULL_STRENGTH_SHIFT);
>> +
>> + debug("%s: pad cfg [0x%x]: %08x\n", __func__, pad_offset,
>> + readl(iobase + pad_offset));
>> +#endif
>> +
>> + return 0;
>> +}
>> +
>> +int gpio_ich6_pinctrl_init(void)
>> +{
>> + int pin_node;
>> + int node;
>> + int ret;
>> + u32 gpiobase;
>> + u32 iobase;
>> +
>> +#ifdef PCI_CFG_IOBASE
> If this block is #ifdef'd out then you will have an uninited variable
> iobase, used below.
Right, as I'm going to move to a property, I will fix this error as well.
>> + /*
>> + * Get the memory/io base address to configure every pins.
>> + * IOBASE is used to configure the mode/pads
>> + * GPIOBASE is used to configure the direction and default value
>> + */
>> + iobase = gpio_ich6_get_base(PCI_CFG_IOBASE);
>> + if (iobase < 0) {
>> + debug("%s: invalid IOBASE address (%08x)\n", __func__, iobase);
>> + return -EINVAL;
>> + }
>> +#endif
>> +
>> + gpiobase = gpio_ich6_get_base(PCI_CFG_GPIOBASE);
>> + if (gpiobase < 0) {
>> + debug("%s: invalid GPIOBASE address (%08x)\n", __func__,
>> + gpiobase);
>> + return -EINVAL;
>> + }
>> +
>> + /* This is not an error to not have a pinctrl node */
>> + node = fdt_node_offset_by_compatible(gd->fdt_blob, -1,
>> + "intel,x86-pinctrl");
> This should be added to the tables in fdtdec.c/h. Then use
> fdtdec_next_compatible(). That file contains a list of all the
> compatible strings we have in U-Boot and is an indicator of what needs
> to be converted to driver model.
Ok, make sense.
>> + if (node < 0) {
>> + debug("%s: no pinctrl node\n", __func__);
>> + return 0;
>> + }
>> +
>> + for (pin_node = fdt_first_subnode(gd->fdt_blob, node);
>> + pin_node > 0;
>> + pin_node = fdt_next_subnode(gd->fdt_blob, pin_node)) {
>> + /* Configure the pin */
>> + ret = _gpio_ich6_pinctrl_cfg_pin(gpiobase, iobase, pin_node);
>> + if (ret != 0) {
>> + debug("%s: invalid configuration for the pin %d\n",
>> + __func__, pin_node);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int gpio_ich6_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + struct ich6_bank_platdata *plat = dev_get_platdata(dev);
>> + u16 gpiobase;
>> + int offset;
>> +
>> + gpiobase = gpio_ich6_get_base(PCI_CFG_GPIOBASE);
>> offset = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "reg", -1);
>> if (offset == -1) {
>> debug("%s: Invalid register offset %d\n", __func__, offset);
>> @@ -192,30 +387,19 @@ static int ich6_gpio_request(struct udevice *dev, unsigned offset,
>> static int ich6_gpio_direction_input(struct udevice *dev, unsigned offset)
>> {
>> struct ich6_bank_priv *bank = dev_get_priv(dev);
>> - u32 tmplong;
>>
>> - tmplong = inl(bank->io_sel);
>> - tmplong |= (1UL << offset);
>> - outl(bank->io_sel, tmplong);
>> - return 0;
>> + return _ich6_gpio_set_direction(inl(bank->io_sel), offset, 0);
>> }
>>
>> static int ich6_gpio_direction_output(struct udevice *dev, unsigned offset,
>> int value)
>> {
>> struct ich6_bank_priv *bank = dev_get_priv(dev);
>> - u32 tmplong;
>>
>> - gpio_set_value(offset, value);
>> -
>> - tmplong = inl(bank->io_sel);
>> - tmplong &= ~(1UL << offset);
>> - outl(bank->io_sel, tmplong);
>> - return 0;
>> + return _ich6_gpio_set_direction(inl(bank->io_sel), offset, 1);
>> }
>>
>> static int ich6_gpio_get_value(struct udevice *dev, unsigned offset)
>> -
>> {
>> struct ich6_bank_priv *bank = dev_get_priv(dev);
>> u32 tmplong;
>> @@ -230,15 +414,7 @@ static int ich6_gpio_set_value(struct udevice *dev, unsigned offset,
>> int value)
>> {
>> struct ich6_bank_priv *bank = dev_get_priv(dev);
>> - u32 tmplong;
>> -
>> - tmplong = inl(bank->lvl);
>> - if (value)
>> - tmplong |= (1UL << offset);
>> - else
>> - tmplong &= ~(1UL << offset);
>> - outl(bank->lvl, tmplong);
>> - return 0;
>> + return _ich6_gpio_set_value(bank->lvl, offset, value);
>> }
>>
>> static int ich6_gpio_get_function(struct udevice *dev, unsigned offset)
>> diff --git a/include/dt-bindings/gpio/x86-gpio.h b/include/dt-bindings/gpio/x86-gpio.h
>> new file mode 100644
>> index 0000000..103bd04
>> --- /dev/null
>> +++ b/include/dt-bindings/gpio/x86-gpio.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * This header provides constants for binding nvidia,tegra*-gpio.
>> + *
>> + * The first cell in Tegra's GPIO specifier is the GPIO ID. The macros below
>> + * provide names for this.
>> + *
>> + * The second cell contains standard flag values specified in gpio.h.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_GPIO_X86_GPIO_H
>> +#define _DT_BINDINGS_GPIO_X86_GPIO_H
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +
>> +#define GPIO_MODE_NATIVE 0
>> +#define GPIO_MODE_GPIO 1
>> +
>> +#define GPIO_MODE_FUNC0 0
>> +#define GPIO_MODE_FUNC1 1
>> +#define GPIO_MODE_FUNC2 2
>> +#define GPIO_MODE_FUNC3 3
>> +#define GPIO_MODE_FUNC4 4
>> +#define GPIO_MODE_FUNC5 5
>> +#define GPIO_MODE_FUNC6 6
> Please can you tab the numbers out to the same column?
I will do.
>> +
>> +#define PIN_INPUT 0
>> +#define PIN_OUTPUT 1
>> +
>> +#define PIN_INPUT_NOPULL 0
>> +#define PIN_INPUT_PULLUP 1
>> +#define PIN_INPUT_PULLDOWN 2
>> +
>> +#define PULL_STR_2K 0
>> +#define PULL_STR_20K 2
>> +
>> +#endif
>> --
>> 2.1.4
>>
> Regards,
> Simon
Regards,
Gabriel
More information about the U-Boot
mailing list