diff mbox

[U-Boot,3/4,v2] x86: gpio: add pinctrl support from the device tree

Message ID 1429993061-10513-1-git-send-email-contact@huau-gabriel.fr
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

Gabriel Huau April 25, 2015, 8:17 p.m. UTC 
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@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.

 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

Comments

Simon Glass April 28, 2015, 1:53 p.m. UTC | #1 
Hi Gabriel,

On 25 April 2015 at 14:17, Gabriel Huau <contact@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@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.

>
>  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@0 {
> +                       gpio-offset = <0x80 8>;
> +                       pad-offset = <0x260>;
> +                       mode-gpio;
> +                       output-value = <1>;
> +                       direction = <PIN_OUTPUT>;
> +               };
> +
> +               pin_usb_host_en1@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.

>
>  #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@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.

> +
> +#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.

> +       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.

> +
> +       /*
> +        * 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.

> +                       _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.

> +       /*
> +        * 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) ?

> +                               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.

> +       /*
> +        * 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.

> +       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?

> +
> +#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
Stephen Warren April 28, 2015, 3:22 p.m. UTC | #2 
On 04/28/2015 07:53 AM, Simon Glass wrote:
> Hi Gabriel,
>
> On 25 April 2015 at 14:17, Gabriel Huau <contact@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.

>> diff --git a/include/dt-bindings/gpio/x86-gpio.h b/include/dt-bindings/gpio/x86-gpio.h

>> +/*
>> + * 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.

I think this comment needs updating.
Gabriel Huau May 12, 2015, 3:33 a.m. UTC | #3 
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@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@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@0 {
>> +                       gpio-offset = <0x80 8>;
>> +                       pad-offset = <0x260>;
>> +                       mode-gpio;
>> +                       output-value = <1>;
>> +                       direction = <PIN_OUTPUT>;
>> +               };
>> +
>> +               pin_usb_host_en1@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@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
Gabriel Huau May 12, 2015, 3:34 a.m. UTC | #4 
Hi Stephen,

Thanks for the feedback, I completely miss it, I will do the 
modification for the v3.

On 04/28/2015 08:22 AM, Stephen Warren wrote:
> On 04/28/2015 07:53 AM, Simon Glass wrote:
>> Hi Gabriel,
>>
>> On 25 April 2015 at 14:17, Gabriel Huau <contact@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.
>
>>> diff --git a/include/dt-bindings/gpio/x86-gpio.h 
>>> b/include/dt-bindings/gpio/x86-gpio.h
>
>>> +/*
>>> + * 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.
>
> I think this comment needs updating.

Regards,
Gabriel
diff mbox

Patch

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@0 {
+			gpio-offset = <0x80 8>;
+			pad-offset = <0x260>;
+			mode-gpio;
+			output-value = <1>;
+			direction = <PIN_OUTPUT>;
+		};
+
+		pin_usb_host_en1@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
 
 #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@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)
+
+#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;
+	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;
+
+	/*
+	 * 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)
+			_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
+	/*
+	 * 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,
+				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
+	/*
+	 * 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");
+	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
+
+#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