gpio: add Intel BayTrail gpio driver
PATCH 1/1 gpio: add Intel BayTrail gpio driver
Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins. Supports gpio interrupts
Pins may be muxed to alternate function instead of gpio by firmware. This driver does not touch the pin muxing and expect firmare to set pin muxing and pullup/down properties properly.
--- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-baytrail.c | 539 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 552 insertions(+), 0 deletions(-) create mode 100644 drivers/gpio/gpio-baytrail.c
drivers/gpio/Kconfig +12 -0
@@ -108,6 +108,18 @@ config GPIO_MAX730X comment "Memory mapped GPIO drivers:" +config GPIO_BAYTRAIL + bool "Intel Baytrail GPIO support" + depends on ACPI && X86 + select IRQ_DOMAIN + help + driver for memory mapped GPIO functionality on Intel Baytrail + platforms. Supports 3 banks with 102, 28 and 43 gpios. + Most pins are usually muxed to some other functionality by firmware, + so only a small amount is available for gpio use. + + Requires ACPI device enumeration code to set up a platform device. + config GPIO_CLPS711X def_bool y depends on ARCH_CLPS711X
drivers/gpio/Makefile +1 -0
@@ -16,6 +16,7 @@ obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o +obj-$(CONFIG_GPIO_BAYTRAIL) += gpio-baytrail.o obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
drivers/gpio/gpio-baytrail.c new file +539 -0
539 lines not on this page · raw message
Signature · 1 line
1.7.4.1
Quoted 7 lines · Mathias Nyman
Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins. Supports gpio interrupts
Pins may be muxed to alternate function instead of gpio by firmware. This driver does not touch the pin muxing and expect firmare to set pin muxing and pullup/down properties properly.
Few comments below (it looks like you sent previous version of patch than that we discussed early)
Quoted 5 lines
drivers/gpio/gpio-baytrail.c new file +1
-0@@ -0,0 +1,539 @@ +static int byt_gpio_direction_input (struct gpio_chip *chip,unsigned offset)
drivers/gpio/gpio-baytrail.c line 257
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +10
-0+{ + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip); +void __iomem *reg =byt_gpio_reg (chip, offset,BYT_VAL_REG ); +unsigned long flags; +u32 value; + +spin_lock_irqsave (&vg->lock, flags); + + value =readl (reg) |BYT_DIR_MASK ; + value = value & (~BYT_INPUT_EN );/* active low */
Just value &= ~BYT_INPUT_EN;
drivers/gpio/gpio-baytrail.c line 289
Quoted 6 lines
drivers/gpio/gpio-baytrail.c new file +6
-0+static void byt_gpio_dbg_show (struct seq_file *s,struct gpio_chip *chip) +{ + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip); +int i; +unsigned long flags; +u32 conf0, val, offs;
Two spaces instead of one.
drivers/gpio/gpio-baytrail.c line 294
Quoted 5 lines
drivers/gpio/gpio-baytrail.c new file +5
-0+ +spin_lock_irqsave (&vg->lock, flags); + +for (i =0 ; i < vg->chip.ngpio; i++) { +
Seems redundant empty line.
Quoted 6 lines
drivers/gpio/gpio-baytrail.c new file +6
-0+ offs = vg->gpio_to_pad[i] *16 ; + conf0 =readl (vg->reg_base + offs +BYT_CONF0_REG ); + val =readl (vg->reg_base + offs +BYT_VAL_REG ); + + seq_printf(s, + " gpio-%-3d %s %s %s pad-%-3d offset:0x%03x mux:%d %s %s %s\n",
drivers/gpio/gpio-baytrail.c line 310
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +10
-0+ i, + val &BYT_INPUT_EN ?" " :"in" , + val &BYT_OUTPUT_EN ?" " :"out" , + val &BYT_LEVEL ?"hi" :"lo" , + vg->gpio_to_pad[i], offs, + conf0 &0x7 , + conf0 &BYT_TRIG_NEG ?"fall" :"" , + conf0 &BYT_TRIG_POS ?"rise" :"" , + conf0 & BYT_TRIG_LVL ? "lvl " : ""); +
Perhaps this one will go after curle brace.
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +9
-0+ } +spin_unlock_irqrestore (&vg->lock, flags); +} +static int byt_gpio_probe (struct platform_device *pdev) +{ +struct byt_gpio *vg; +struct gpio_chip *gc; +struct resource *mem_rc, *irq_rc; +struct device *dev = &pdev->dev;
drivers/gpio/gpio-baytrail.c line 429
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +8
-0+struct acpi_device *acpi_dev; +struct gpio_bank *bank; +acpi_handle handle =ACPI_HANDLE (dev); +unsigned hwirq; +int ret; + + mem_rc =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + irq_rc =platform_get_resource (pdev,IORESOURCE_IRQ ,0 );
Perhaps this one should go close to place where it is used
drivers/gpio/gpio-baytrail.c line 435
Quoted 5 lines
drivers/gpio/gpio-baytrail.c new file +5
-0+if (!mem_rc) { +dev_err (&pdev->dev,"missing MEM resource\n" ); +return -EINVAL ; + } +
Just put
mem_rc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
here and remove above check.
drivers/gpio/gpio-baytrail.c line 437
Quoted 2 lines
drivers/gpio/gpio-baytrail.c new file +2
-0+ vg->reg_base =devm_ioremap_resource (dev, mem_rc); +
No need to have this empty line.
drivers/gpio/gpio-baytrail.c line 439
Quoted 2 lines
drivers/gpio/gpio-baytrail.c new file +2
-0+if (IS_ERR (vg->reg_base)) { +dev_err (&pdev->dev,"error mapping resource\n" );
No need to have this message.
Quoted 7 lines
drivers/gpio/gpio-baytrail.c new file +6
-0+return PTR_ERR (vg->reg_base); + } + ret =gpiochip_add (gc); +if (ret) { +dev_err (&pdev->dev,"failed adding byt-gpio chip\n" ); +return ret;
drivers/gpio/gpio-baytrail.c line 472
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +10
-0+ } + +/* set up interrupts */ +if (irq_rc && irq_rc->start) { + hwirq = irq_rc->start; + gc->to_irq = byt_gpio_to_irq; + + vg->domain =irq_domain_add_linear (NULL , gc->ngpio, + &byt_gpio_irq_ops, vg); +if (!vg->domain)
Have you to call gpiochip_remove() or something like this here?
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +10
-0+return -ENXIO ; + +byt_gpio_irq_init_hw (vg); + +irq_set_handler_data (hwirq, vg); +irq_set_chained_handler (hwirq, byt_gpio_irq_handler); + } + +/* Register interrupt handlers for gpio signaled acpi events */ +acpi_gpiochip_request_interrupts (gc);
drivers/gpio/gpio-baytrail.c line 513
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +9
-0+ +pm_runtime_enable (dev); + +return 0 ; +} +static int byt_gpio_remove (struct platform_device *pdev) +{ +struct byt_gpio *vg =platform_get_drvdata (pdev); +int err;
+ empty line here
drivers/gpio/gpio-baytrail.c line 519
Quoted 6 lines
drivers/gpio/gpio-baytrail.c new file +6
-0+pm_runtime_disable (&pdev->dev); + err =gpiochip_remove (&vg->chip); +if (err) +dev_warn (&pdev->dev,"failed to remove gpio_chip.\n" ); + +platform_set_drvdata (pdev,NULL );
No need to set this.
--
With Best Regards,
Andy Shevchenko
Quoted 9 lines · Mathias Nyman
Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins. Supports gpio interrupts
Pins may be muxed to alternate function instead of gpio by firmware. This driver does not touch the pin muxing and expect firmare to set pin muxing and pullup/down properties properly.
Signed-off-by Mathias Nyman <[email protected]>
Even though it is not talking to the firmware about changing the state of pins etc, can we expect it to do so at a later point? Some comments in the code make me pretty sure that this is the case.
I think there may be a good incentive to put this driver into drivers/pinctrl/* instead and implement parts of the pinctrl interfaces.
Pls consult Documentation/pinctrl.txt.
Quoted 27 lines
drivers/gpio/gpio-baytrail.c new file +27
-0+/* +* Baytrail gpio controller consist of three separate sub-controllers called +* SCORE, NCORE and SUS. The sub-controllers are identified by their acpi UID. +* +* GPIO numbering is _not_ ordered meaning that gpio # 0 in ACPI namespace does +* _not_ correspond to the first gpio register at controller's gpio base. +* There is no logic or pattern in mapping gpio numbers to registers (pads) so +* each sub-controller needs to have its own mapping table +*/ + +static unsigned score_gpio_to_pad[BYT_NGPIO_SCORE ] = { +85 ,89 ,93 ,96 ,99 ,102 ,98 ,101 ,34 ,37 , +36 ,38 ,39 ,35 ,40 ,84 ,62 ,61 ,64 ,59 , +54 ,56 ,60 ,55 ,63 ,57 ,51 ,50 ,53 ,47 , +52 ,49 ,48 ,43 ,46 ,41 ,45 ,42 ,58 ,44 , +95 ,105 ,70 ,68 ,67 ,66 ,69 ,71 ,65 ,72 , +86 ,90 ,88 ,92 ,103 ,77 ,79 ,83 ,78 ,81 , +80 ,82 ,13 ,12 ,15 ,14 ,17 ,18 ,19 ,16 , +2 ,1 ,0 ,4 ,6 ,7 ,9 ,8 ,33 ,32 , +31 ,30 ,29 ,27 ,25 ,28 ,26 ,23 ,21 ,20 , +24 ,22 ,5 ,3 ,10 ,11 ,106 ,87 ,91 ,104 , +97 ,100 , +}; + +static unsigned ncore_gpio_to_pad[BYT_NGPIO_NCORE ] = { +19 ,18 ,17 ,20 ,21 ,22 ,24 ,25 ,23 ,16 , +14 ,15 ,12 ,26 ,27 ,1 ,4 ,8 ,11 ,0 ,
drivers/gpio/gpio-baytrail.c line 99
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +10
-0+ 3, 6, 10, 13, 2, 5, 9, 7, +}; + +static unsigned sus_gpio_to_pad[BYT_NGPIO_SUS ] = { +29 ,33 ,30 ,31 ,32 ,34 ,36 ,35 ,38 ,37 , +18 ,11 ,20 ,17 ,1 ,8 ,10 ,19 ,12 ,0 , +2 ,23 ,39 ,28 ,27 ,22 ,21 ,24 ,25 ,26 , +51 ,56 ,54 ,49 ,55 ,48 ,57 ,50 ,58 ,52 , +53 ,59 ,40 , +};
This is duplicating the functionality of mapping pins to GPIO ranges which already exist in the pinctrl subsystem.
gpiochip_add_pin_range(chip, "foo", offset, pin_base, N);
This can be reused by instatiating a shallow pinctrl driver which only populates the .name, .pins, .owner, .get_groups_count(), .get_group_name(), and . get_group_pins() of a struct pinctrl_desc (we can discuss about making the group functions optional) and then using the GPIO ranges to cross reference pins to GPIOs.
Then you could use from <linux/pinctrl/pinctrl.h> pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
unsigned int pin);
And other helpers to cross-reference pins and GPIOs.
(...)
Quoted 14 lines
drivers/gpio/gpio-baytrail.c new file +14
-0+static void __iomem *byt_gpio_reg (struct gpio_chip *chip,unsigned offset, +int reg) +{ + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip); +u32 reg_offset; +void __iomem *ptr; + +if (reg ==BYT_INT_STAT_REG ) + reg_offset = (offset /32 ) *4 ; +else + reg_offset = vg->gpio_to_pad[offset] *16 ; + ptr = (void __iomem *) (vg->reg_base + reg_offset + reg); +return ptr; +}
drivers/gpio/gpio-baytrail.c line 158
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +8
-0+ +static int byt_gpio_request (struct gpio_chip *chip,unsigned offset) +{ + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip); + +/* Policy about what should be done when requesting a gpio is unclear. +* In most cases PIN MUX 000 means gpio function, with the exception of SUS +* core pins 11-21 where gpio is mux 001.
This also looks suspicious. If it is "unclear" I am reading here:
"we will sooner or later have to deal with pin control somekindof".
Quoted 19 lines
+ * Some pins are set by bios to a non-gpio mux, but still marked as gpio
+ * resource in acpi tables, and they work just as they should when not touching
+ * the pin muxing. (For example mmc card detect switch)
+ *
+ * option 1, check pin mux is "gpio", else fail (FIXME gpio SUS pins 11-21):+ * void __iomem *reg = byt_gpio_reg(chip, offset, BYT_CONF0_REG); + * u32 value; + * value = readl(reg) & BYT_PIN_MUX; + * if (value) + * return -EINVAL; + * + * option 2, force pin mux to gpio mode (FIXME gpio SUS pins 11-21): + * void __iomem *reg = byt_gpio_reg(chip, offset, BYT_CONF0_REG); + * u32 value; + * value = readl(reg) & BYT_PIN_MUX; + * writel(value & ~BYT_PIN_MUX, reg);+ *
+ * option 3: don't touch the pinmuxing at all, let BIOS handle it
+ */
drivers/gpio/gpio-baytrail.c line 182
Quoted 4 lines
drivers/gpio/gpio-baytrail.c new file +4
-0+pm_runtime_get (&vg->pdev->dev); + +return 0 ; +}
If you ever want to do option 1 or 2, you should use pinctrl.
It also has (when implemented) the upside of exposing all these gory details through debugfs for nice debugging, so developers can see the state of all pads here.
I would recommend putting this in drivers/pinctrl, and use a single probe() function, but call both gpiochip_add() and pinctrl_register() then use gpiochip_add_pin_range() to map pins to GPIOs. Compare e.g. drivers/pinctrl/pinctrl-abx500.c
Yours,
Linus Walleij
Quoted 17 lines · Linus Walleij
Quoted 9 lines · Mathias Nyman
Add support for gpio on Intel BayTrail platforms. BayTrail supports 3 banks of gpios called SCORE, NCORE ans SUS with 102, 28 and 43 gpio pins. Supports gpio interrupts
Pins may be muxed to alternate function instead of gpio by firmware. This driver does not touch the pin muxing and expect firmare to set pin muxing and pullup/down properties properly.
Signed-off-by Mathias Nyman<[email protected]>Even though it is not talking to the firmware about changing the state of pins etc, can we expect it to do so at a later point? Some comments in the code make me pretty sure that this is the case.
Quoted 6 lines
I think there may be a good incentive to put this driver into
drivers/pinctrl/*instead and implement parts of the pinctrl interfaces.Pls consult
Documentation/pinctrl.txt.
I'll have a look at pinctrl and see if it fits.
My understanding at the moment is that this driver shouldn't need to modify pin muxing. BIOS is responsible for adding GPIO resource entries to ACPI tables for devices that use gpios, and BIOS should also set all muxings correctly for those pins(also pull-ups/downs etc.)
Quoted 50 lines
Quoted 27 lines
drivers/gpio/gpio-baytrail.c new file +27
-0+/* +* Baytrail gpio controller consist of three separate sub-controllers called +* SCORE, NCORE and SUS. The sub-controllers are identified by their acpi UID. +* +* GPIO numbering is _not_ ordered meaning that gpio # 0 in ACPI namespace does +* _not_ correspond to the first gpio register at controller's gpio base. +* There is no logic or pattern in mapping gpio numbers to registers (pads) so +* each sub-controller needs to have its own mapping table +*/ + +static unsigned score_gpio_to_pad[BYT_NGPIO_SCORE ] = { +85 ,89 ,93 ,96 ,99 ,102 ,98 ,101 ,34 ,37 , +36 ,38 ,39 ,35 ,40 ,84 ,62 ,61 ,64 ,59 , +54 ,56 ,60 ,55 ,63 ,57 ,51 ,50 ,53 ,47 , +52 ,49 ,48 ,43 ,46 ,41 ,45 ,42 ,58 ,44 , +95 ,105 ,70 ,68 ,67 ,66 ,69 ,71 ,65 ,72 , +86 ,90 ,88 ,92 ,103 ,77 ,79 ,83 ,78 ,81 , +80 ,82 ,13 ,12 ,15 ,14 ,17 ,18 ,19 ,16 , +2 ,1 ,0 ,4 ,6 ,7 ,9 ,8 ,33 ,32 , +31 ,30 ,29 ,27 ,25 ,28 ,26 ,23 ,21 ,20 , +24 ,22 ,5 ,3 ,10 ,11 ,106 ,87 ,91 ,104 , +97 ,100 , +}; + +static unsigned ncore_gpio_to_pad[BYT_NGPIO_NCORE ] = { +19 ,18 ,17 ,20 ,21 ,22 ,24 ,25 ,23 ,16 , +14 ,15 ,12 ,26 ,27 ,1 ,4 ,8 ,11 ,0 ,drivers/gpio/gpio-baytrail.c line 99
Quoted 10 lines
drivers/gpio/gpio-baytrail.c new file +10
-0+ 3, 6, 10, 13, 2, 5, 9, 7, +}; + +static unsigned sus_gpio_to_pad[BYT_NGPIO_SUS ] = { +29 ,33 ,30 ,31 ,32 ,34 ,36 ,35 ,38 ,37 , +18 ,11 ,20 ,17 ,1 ,8 ,10 ,19 ,12 ,0 , +2 ,23 ,39 ,28 ,27 ,22 ,21 ,24 ,25 ,26 , +51 ,56 ,54 ,49 ,55 ,48 ,57 ,50 ,58 ,52 , +53 ,59 ,40 , +};This is duplicating the functionality of mapping pins to GPIO ranges which already exist in the pinctrl subsystem.
gpiochip_add_pin_range(chip, "foo", offset, pin_base, N);This can be reused by instatiating a shallow pinctrl driver which only populates the .name, .pins, .owner, .get_groups_count(), .get_group_name(), and .
get_group_pins()of a structpinctrl_desc(we can discuss about making the group functions optional) and then using the GPIO ranges to cross reference pins to GPIOs.
Quoted 7 lines
Then you could use from<linux/pinctrl/pinctrl.h> pinctrl_find_gpio_range_from_pin(struct
pinctrl_dev*pctldev,unsigned int pin);And other helpers to cross-reference pins and GPIOs.
(...)
The pin-to-gpio mapping is quite random here, there are no long consecutive ranges, I tried to find some pattern but it seems to be just random.
I haven't yet looked into pinctrl yet and see what it offers
Quoted 21 lines
Quoted 19 lines
drivers/gpio/gpio-baytrail.c new file +19
-0+static void __iomem *byt_gpio_reg (struct gpio_chip *chip,unsigned offset, +int reg) +{ + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip); +u32 reg_offset; +void __iomem *ptr; + +if (reg ==BYT_INT_STAT_REG ) + reg_offset = (offset /32 ) *4 ; +else + reg_offset = vg->gpio_to_pad[offset] *16 ; + ptr = (void __iomem *) (vg->reg_base + reg_offset + reg); +return ptr; +} + +static int byt_gpio_request (struct gpio_chip *chip,unsigned offset) +{ + struct byt_gpio *vg = container_of(chip, struct byt_gpio, chip); +
Quoted 7 lines
Quoted 3 lines
+/* Policy about what should be done when requesting a gpio is unclear.
+ * In most cases PIN MUX 000 means gpio function, with the exception of SUS
+ * core pins 11-21 where gpio is mux 001.This also looks suspicious. If it is "unclear" I am reading here:
"we will sooner or later have to deal with pin control somekindof".
It should evolve in the other direction with BIOS configuring the pins, but while writing the driver the BIOSes were not mature enough and could not be trusted.
So I think I'll either remove the muxing options and keep it as gpio driver, or then if it turns out we need to do do pinmuxing I'll use pinctrl.
Thanks for reviewing and for the pinctrl explanations
-Mathias