[U-Boot] unassigned-patches/144: Re: [PATCH 3/4] x86: gpio: add pinctrl support from the device tree

u-boot at bugs.denx.de u-boot at bugs.denx.de
Fri Apr 24 06:40:01 CEST 2015


Hi,

On 23 April 2015 at 10:16, Gabriel Huau <contact at huau-gabriel.fr> wrote:
> A set of properties has been defined for the device tree to select for
> each pin the pull/func/default output configuration.
>
> The offset for the PAD needs to be provided and if a GPIO needs to be
> configured, his offset needs to be provided as well.
>
> Here is an example:
> pin_usb_host_en0 at 0 {
>     gpio-offset = <0x80 8>;
>     pad-offset = <0x260>;
>     mode-gpio;
>     output-value = <1>;
>     direction = <PIN_OUTPUT>;
> };
>
> Signed-off-by: Gabriel Huau <contact at huau-gabriel.fr>
> ---
>  arch/x86/dts/minnowmax.dts                |  21 +++
>  arch/x86/include/asm/arch-baytrail/gpio.h |   1 +
>  arch/x86/include/asm/gpio.h               |   1 +
>  drivers/gpio/intel_ich6_gpio.c            | 222 ++++++++++++++++++++++++++----
>  include/dt-bindings/gpio/gpio.h           |  20 +++
>  5 files changed, 239 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts
> index c73e421..3936e21 100644
> --- a/arch/x86/dts/minnowmax.dts
> +++ b/arch/x86/dts/minnowmax.dts
> @@ -6,6 +6,8 @@
>
>  /dts-v1/;
>
> +#include <dt-bindings/gpio/gpio.h>
> +
>  /include/ "skeleton.dtsi"
>  /include/ "serial.dtsi"
>
> @@ -21,6 +23,25 @@
>                 silent_console = <0>;
>         };
>
> +       pch_pinctrl {
> +               compatible = "intel,ich6-pinctrl";

Make sure you use tabs for indenting here.

You should create a binding file to describe your binding - in
doc/device-tree-bindings.

> +               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
>
>  #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/drivers/gpio/intel_ich6_gpio.c b/drivers/gpio/intel_ich6_gpio.c
> index 7e679a0..a110d5b 100644
> --- a/drivers/gpio/intel_ich6_gpio.c
> +++ b/drivers/gpio/intel_ich6_gpio.c
> @@ -44,21 +44,32 @@ 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)

Comments on the above

> +
> +#define IOPAD_MODE_MASK                                0x7
> +#define IOPAD_PULL_ASSIGN_MASK         0x3
> +#define IOPAD_PULL_ASSIGN_SHIFT                7

Can you make the mask value an actual valid mask, like:

 +#define IOPAD_PULL_ASSIGN_MASK         (0x3 << IOPAD_PULL_ASSIGN_SHIFT)

> +#define IOPAD_PULL_STRENGTH_MASK       0x3
> +#define IOPAD_PULL_STRENGTH_SHIFT      9
> +
> +static int __ich6_gpio_set_value(uint16_t base, unsigned offset, int value);

Can you reorder the functions to avoid the need for these forward
declarations? Also only one underscore prefix please.

> +static int __ich6_gpio_set_direction(uint16_t base, unsigned offset, int dir);
> +static int __ich6_gpio_set_function(uint16_t base, unsigned offset, int func);
> +
>  /* 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 +134,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);

Can the base come from the device tree somewhere? E.g.

pch {
    gpio {
       reg = <some value>;

There is something like this in chromebook_link.dts.

> +       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 +146,138 @@ 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;
> +}
> +
> +int gpio_ich6_pinctrl_init(void)
> +{
> +       int pin_node;
> +       int node;
> +       u32 iobase;
> +       u32 gpiobase;
> +
> +       /*
> +        * 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;
> +       }
> +
> +       gpiobase = gpio_ich6_get_base(PCI_CFG_GPIOBASE);
> +       if (iobase < 0) {
> +               debug("%s: invalid IOBASE address (%08x)\n", __func__, iobase);
> +               return -EINVAL;
> +       }
> +
> +       /* This is not an error to not have a pinctrl node */
> +       node = fdt_node_offset_by_compatible(gd->fdt_blob, -1,
> +                                            "intel,ich6-pinctrl");
> +       if (node < 0)
> +               return 0;

Probably this should move to driver model, but let's worry about that later.
> +
> +       for (pin_node = fdt_first_subnode(gd->fdt_blob, node);
> +            pin_node > 0;
> +            pin_node = fdt_next_subnode(gd->fdt_blob, pin_node)) {
> +               u32 gpio_offset[2] = { -1, -1 };
> +               u32 pad_offset;
> +               u32 tmplong;
> +               const void *tmpnode;
> +
> +               /*
> +                * 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;
> +               }
> +
> +               /*
> +                * 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) {

Can you move this block into its own function without too much trouble?

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

But tmplong is unsigned.

> +                               __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);
> +               }
> +
> +               /*
> +                * 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 <<
> +                                       IOPAD_PULL_ASSIGN_SHIFT,
> +                                       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 <<
> +                                       IOPAD_PULL_STRENGTH_SHIFT,
> +                                       tmplong << IOPAD_PULL_STRENGTH_SHIFT);
> +
> +               debug("%s: pad cfg [0x%x]: %08x\n", __func__, pad_offset,
> +                     readl(iobase + pad_offset));
> +       }
> +
> +       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);
> @@ -189,33 +331,56 @@ static int ich6_gpio_request(struct udevice *dev, unsigned offset,
>         return 0;
>  }
>
> -static int ich6_gpio_direction_input(struct udevice *dev, unsigned offset)
> +static int __ich6_gpio_set_function(uint16_t base, unsigned offset, int func)
>  {
> -       struct ich6_bank_priv *bank = dev_get_priv(dev);
>         u32 tmplong;
>
> -       tmplong = inl(bank->io_sel);
> -       tmplong |= (1UL << offset);
> -       outl(bank->io_sel, 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_direction_output(struct udevice *dev, unsigned offset,
> -                                      int value)
> +static int __ich6_gpio_set_direction(uint16_t base, unsigned offset, int dir)
>  {
> -       struct ich6_bank_priv *bank = dev_get_priv(dev);
>         u32 tmplong;
>
> -       gpio_set_value(offset, value);
> +       if (!dir) {
> +               tmplong = inl(base);
> +               tmplong |= (1UL << offset);
> +               outl(tmplong, base);
> +       } else {
> +               tmplong = inl(base);
> +               tmplong &= ~(1UL << offset);
> +               outl(tmplong, base);
> +       }
>
> -       tmplong = inl(bank->io_sel);
> -       tmplong &= ~(1UL << offset);
> -       outl(bank->io_sel, tmplong);
>         return 0;
>  }
>
> -static int ich6_gpio_get_value(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);
> +
> +       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);
> +
> +       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;
> @@ -226,21 +391,26 @@ static int ich6_gpio_get_value(struct udevice *dev, unsigned offset)
>         return r;
>  }
>
> -static int ich6_gpio_set_value(struct udevice *dev, unsigned offset,
> -                              int value)
> +static int __ich6_gpio_set_value(uint16_t base, unsigned offset, int value)
>  {
> -       struct ich6_bank_priv *bank = dev_get_priv(dev);
>         u32 tmplong;
> -
> -       tmplong = inl(bank->lvl);
> +       tmplong = inl(base);
>         if (value)
>                 tmplong |= (1UL << offset);
>         else
>                 tmplong &= ~(1UL << offset);
> -       outl(bank->lvl, tmplong);
> +       outl(tmplong, base);
> +
>         return 0;
>  }
>
> +static int ich6_gpio_set_value(struct udevice *dev, unsigned offset,
> +               int value)
> +{
> +       struct ich6_bank_priv *bank = dev_get_priv(dev);
> +       return __ich6_gpio_set_value(bank->lvl, offset, value);
> +}
> +
>  static int ich6_gpio_get_function(struct udevice *dev, unsigned offset)
>  {
>         struct ich6_bank_priv *bank = dev_get_priv(dev);
> diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
> index e6b1e0a..49d437c 100644
> --- a/include/dt-bindings/gpio/gpio.h
> +++ b/include/dt-bindings/gpio/gpio.h
> @@ -12,4 +12,24 @@
>  #define GPIO_ACTIVE_HIGH 0
>  #define GPIO_ACTIVE_LOW 1
>
> +#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_FUNC4        4
> +#define GPIO_MODE_FUNC5        5
> +#define GPIO_MODE_FUNC6        6

Can we use an enum for these? They mostly count up.

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

---
Added to GNATS database as unassigned-patches/144
>Responsible:    patch-coord
>Message-Id:     <CAPnjgZ38cPE1h6EBg-ffdtOA4FhTdgTx8u1VS5yMpv9Ynik-KA at mail.gmail.com>
>In-Reply-To:    <1429805775-1809-4-git-send-email-contact at huau-gabriel.fr>
>References:     <1429805775-1809-1-git-send-email-contact at huau-gabriel.fr>	<1429805775-1809-4-git-send-email-contact at huau-gabriel.fr>
>Patch-Date:     Fri Apr 24 05:35:00 +0200 2015



More information about the U-Boot mailing list