Re: [PATCH] gpio: add Intel BayTrail gpio driver

From: Mathias Nyman
Date: Mon Jun 03 2013 - 05:53:03 EST


On 05/30/2013 10:26 PM, Linus Walleij wrote:
On Wed, May 29, 2013 at 10:01 AM, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> wrote:

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<mathias.nyman@xxxxxxxxxxxxxxx>

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.


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

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

(...)

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


+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);
+


+/* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/