Hi Simon,
On 04/23/2015 08:35 PM, Simon Glass wrote:
Hi,
On 23 April 2015 at 10:16, Gabriel Huau contact@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@0 { gpio-offset = <0x80 8>; pad-offset = <0x260>; mode-gpio; output-value = <1>; direction = <PIN_OUTPUT>; };
Signed-off-by: Gabriel Huau contact@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@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/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.
Sounds good and will answer to another issue to was breaking actually the other board ...
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.
Agreed, for the first version I would like to have an easy implementation that we can move to driver model 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?
Agreed.
/* 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.
This is not how we do to never have an error? ;)
I'll do the modification.
__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.
Agreed.
+#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