MinnowBoard support

Use DMI_BOARD_NAME to determine if we are running on a MinnowBoard and set the uart clock to 50MHz if so. This removes the need to pass the user_uartclk to the kernel at boot time.

Signed-off-by Darren Hart <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Jiri Slaby <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Waskiewicz <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc [email protected]

---
 drivers/tty/serial/pch_uart.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/tty/serial/pch_uart.c +1 -0

index 21a7e17..572d481 100644
@@ -217,6 +217,7 @@ enum {
 #define FRI2_64_UARTCLK  64000000 /*  64.0000 MHz */
 #define FRI2_48_UARTCLK  48000000 /*  48.0000 MHz */
 #define NTC1_UARTCLK     64000000 /*  64.0000 MHz */
+#define MINNOW_UARTCLK   50000000 /*  50.0000 MHz */
 
 struct pch_uart_buffer {
 	unsigned char *buf;

Quoted 6 lines · Darren Hart

struct pch_uart_buffer {

drivers/tty/serial/pch_uart.c +0 -0

 	unsigned char *buf;

@@ -398,6 +399,10 @@ static int pch_uart_get_uartclk(void)
 		    strstr(cmp, "nanoETXexpress-TT")))
 		return NTC1_UARTCLK;
 

drivers/tty/serial/pch_uart.c static int pch_uart_get_uartclk(void) · line 405

Quoted 4 lines

drivers/tty/serial/pch_uart.c +4 -0

+	cmp = dmi_get_system_info(DMI_BOARD_NAME);
+	if (cmp && strstr(cmp, "MinnowBoard"))
+		return MINNOW_UARTCLK;
+

You know, we do have the DMI interface to handle this in a much nicer way instead of just randomly trying different strings over and over until we find one that matches...

Quoted 8 lines · Greg Kroah-Hartman

Quoted 6 lines · Darren Hart

struct pch_uart_buffer {

drivers/tty/serial/pch_uart.c +0 -0

 	unsigned char *buf;

@@ -398,6 +399,10 @@ static int pch_uart_get_uartclk(void)
 		    strstr(cmp, "nanoETXexpress-TT")))
 		return NTC1_UARTCLK;
 

Quoted 8 lines

drivers/tty/serial/pch_uart.c static int pch_uart_get_uartclk(void) · line 405

Quoted 4 lines

drivers/tty/serial/pch_uart.c +4 -0

+	cmp = dmi_get_system_info(DMI_BOARD_NAME);
+	if (cmp && strstr(cmp, "MinnowBoard"))
+		return MINNOW_UARTCLK;
+

You know, we do have the DMI interface to handle this in a much nicer way instead of just randomly trying different strings over and over until we find one that matches...

I was aiming for minimal change. Partly because I'm lazy. Partly because I don't have all of the impacted hardware to test. Partly because I wanted to keep it simple so I could push this to 3.8 stable.

I can rewrite this detection to use the DMI interface. Would you allow it as a follow-on, to keep the changes to stable minimal?

Also, I do have a PCI subsystem ID for this particular board which I could trigger on, but since that mechanism didn't exist in the driver already and the other boards don't do it, I just followed what was already there (yeah, so I wrote most of what was already there... but.... anyway) :-)

What do you prefer? Rewrite, then add Minnow, or use this, then rewrite?

Quoted 18 lines · Darren Hart

Quoted 16 lines · Greg Kroah-Hartman

Review static int pch_uart_get_uartclk(void) · line 405

Quoted 10 lines · Darren Hart

struct pch_uart_buffer {

(quoted diff) +4 -0

 	unsigned char *buf;

@@ -398,6 +399,10 @@ static int pch_uart_get_uartclk(void)
 		    strstr(cmp, "nanoETXexpress-TT")))
 		return NTC1_UARTCLK;
 
+	cmp = dmi_get_system_info(DMI_BOARD_NAME);
+	if (cmp && strstr(cmp, "MinnowBoard"))
+		return MINNOW_UARTCLK;
+

You know, we do have the DMI interface to handle this in a much nicer way instead of just randomly trying different strings over and over until we find one that matches...

Quoted 3 lines

I was aiming for minimal change. Partly because I'm lazy. Partly because I don't have all of the impacted hardware to test. Partly because I wanted to keep it simple so I could push this to 3.8 stable.

Then you should have marked it for the stable tree by putting the proper
Cc: in the body of the patch...

Quoted 10 lines

I can rewrite this detection to use the DMI interface. Would you allow it as a follow-on, to keep the changes to stable minimal?

Also, I do have a PCI subsystem ID for this particular board which I could trigger on, but since that mechanism didn't exist in the driver already and the other boards don't do it, I just followed what was already there (yeah, so I wrote most of what was already there... but.... anyway) :-)

Quoted 2 lines

What do you prefer? Rewrite, then add Minnow, or use this, then rewrite?

How about this, which makes it easy to backport, then you fix it up properly for 3.12? This comes after my tree is pretty much closed for 3.11, but a simple device id addition like this is acceptable, but I'll not get to it until after 3.11-rc1 is out...

From 22cb21e2421ffcd439f58843422cff0e2579902f Mon Sep 17 00:00:00 2001

Quoted 4 lines

How about this, which makes it easy to backport, then you fix it up properly for 3.12? This comes after my tree is pretty much closed for 3.11, but a simple device id addition like this is acceptable, but I'll not get to it until after 3.11-rc1 is out...

Sure. So something like this is what you have in mind? Build tested only, I'll include it in V2 with proper testing, but just wanted to make sure we're on the same page while I have your attention:

Subject: [PATCH] pch_uart: Use DMI interface for board detection

Use the DMI interface rather than manually matching DMI strings.

Signed-off-by Darren Hart <[email protected]>

---
 drivers/tty/serial/pch_uart.c | 71
+++++++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 22 deletions(-)

drivers/tty/serial/pch_uart.c +46 -15

index 572d481..271cc73 100644
@@ -373,35 +373,62 @@ static const struct file_operations port_regs_ops
 = {
 };
 #endif	/* CONFIG_DEBUG_FS */
 
+static struct dmi_system_id __initdata pch_uart_dmi_table[] = {
+	{
+		.ident = "CM-iTC",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "CM-iTC"),
+		},
+		(void *)CMITC_UARTCLK,
+	},
+	{
+		.ident = "FRI2",
+		{
+			DMI_MATCH(DMI_BIOS_VERSION, "FRI2"),
+		},
+		(void *)FRI2_64_UARTCLK,
+	},
+	{
+		.ident = "Fish River Island II",
+		{
+			DMI_MATCH(DMI_PRODUCT_NAME, "Fish River Island II"),
+		},
+		(void *)FRI2_48_UARTCLK,
+	},
+	{
+		.ident = "COMe-mTT",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "COMe-mTT"),
+		},
+		(void *)NTC1_UARTCLK,
+	},
+	{
+		.ident = "nanoETXexpress-TT",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "nanoETXexpress-TT"),
+		},
+		(void *)NTC1_UARTCLK,
+	},
+	{
+		.ident = "MinnowBoard",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "MinnowBoard"),
+		},
+		(void *)MINNOW_UARTCLK,
+	},
+};
+
 /* Return UART clock, checking for board specific clocks. */
 static int pch_uart_get_uartclk(void)
 {
-	const char *cmp;
+	const struct dmi_system_id *d;
 
 	if (user_uartclk)
 		return user_uartclk;
 
-	cmp = dmi_get_system_info(DMI_BOARD_NAME);
-	if (cmp && strstr(cmp, "CM-iTC"))
-		return CMITC_UARTCLK;
-
-	cmp = dmi_get_system_info(DMI_BIOS_VERSION);
-	if (cmp && strnstr(cmp, "FRI2", 4))
-		return FRI2_64_UARTCLK;
-
-	cmp = dmi_get_system_info(DMI_PRODUCT_NAME);
-	if (cmp && strstr(cmp, "Fish River Island II"))
-		return FRI2_48_UARTCLK;
-
-	/* Kontron COMe-mTT10 (nanoETXexpress-TT) */
-	cmp = dmi_get_system_info(DMI_BOARD_NAME);

Quoted 93 lines · Darren Hart

Subject: [PATCH] pch_uart: Use DMI interface for board detection

Use the DMI interface rather than manually matching DMI strings.

Signed-off-by Darren Hart <[email protected]>

---
 drivers/tty/serial/pch_uart.c | 71
+++++++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 22 deletions(-)

drivers/tty/serial/pch_uart.c +46 -20

index 572d481..271cc73 100644
@@ -373,35 +373,62 @@ static const struct file_operations port_regs_ops
 = {
 };
 #endif	/* CONFIG_DEBUG_FS */
 
+static struct dmi_system_id __initdata pch_uart_dmi_table[] = {
+	{
+		.ident = "CM-iTC",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "CM-iTC"),
+		},
+		(void *)CMITC_UARTCLK,
+	},
+	{
+		.ident = "FRI2",
+		{
+			DMI_MATCH(DMI_BIOS_VERSION, "FRI2"),
+		},
+		(void *)FRI2_64_UARTCLK,
+	},
+	{
+		.ident = "Fish River Island II",
+		{
+			DMI_MATCH(DMI_PRODUCT_NAME, "Fish River Island II"),
+		},
+		(void *)FRI2_48_UARTCLK,
+	},
+	{
+		.ident = "COMe-mTT",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "COMe-mTT"),
+		},
+		(void *)NTC1_UARTCLK,
+	},
+	{
+		.ident = "nanoETXexpress-TT",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "nanoETXexpress-TT"),
+		},
+		(void *)NTC1_UARTCLK,
+	},
+	{
+		.ident = "MinnowBoard",
+		{
+			DMI_MATCH(DMI_BOARD_NAME, "MinnowBoard"),
+		},
+		(void *)MINNOW_UARTCLK,
+	},
+};
+
 /* Return UART clock, checking for board specific clocks. */
 static int pch_uart_get_uartclk(void)
 {
-	const char *cmp;
+	const struct dmi_system_id *d;
 
 	if (user_uartclk)
 		return user_uartclk;
 
-	cmp = dmi_get_system_info(DMI_BOARD_NAME);
-	if (cmp && strstr(cmp, "CM-iTC"))
-		return CMITC_UARTCLK;
-
-	cmp = dmi_get_system_info(DMI_BIOS_VERSION);
-	if (cmp && strnstr(cmp, "FRI2", 4))
-		return FRI2_64_UARTCLK;
-
-	cmp = dmi_get_system_info(DMI_PRODUCT_NAME);
-	if (cmp && strstr(cmp, "Fish River Island II"))
-		return FRI2_48_UARTCLK;
-
-	/* Kontron COMe-mTT10 (nanoETXexpress-TT) */
-	cmp = dmi_get_system_info(DMI_BOARD_NAME);
-	if (cmp && (strstr(cmp, "COMe-mTT") ||
-		    strstr(cmp, "nanoETXexpress-TT")))
-		return NTC1_UARTCLK;
-
-	cmp = dmi_get_system_info(DMI_BOARD_NAME);

drivers/tty/serial/pch_uart.c static const struct file_operations port_regs_ops · line 432

Quoted 5 lines

drivers/tty/serial/pch_uart.c +3 -2

-	if (cmp && strstr(cmp, "MinnowBoard"))
-		return MINNOW_UARTCLK;
+	d = dmi_first_match(pch_uart_dmi_table);
+	if (d)
+		return (int)d->driver_data;

This and the (void *) casts above will generate a warning on 64bit, right? We need to go through long here...

You can't actually use this driver on 64 bit as there is no hardware,

Quoted 1 line · Darren Hart

You can't actually use this driver on 64 bit as there is no hardware,

Oh, so is this another candidate for adding dependson (X86_32 || COMPILE_TEST) as well as the other *_PCH options?

Allow for enabling and disabling of the resume well GPIOs. The E6xx Atom CPUs multiplex the resume GPIO 2:0 lines with LVDS and individual board drivers need to be able to enable or disable the lines appropriately.

Unfortunately, the information regarding if the pins are being used for LVDS or GPIO is board specific and may not be available to the gpio-sch driver at probe time.

The MinnowBoard (http://www.minnowboard.org) is an Intel Atom (E6xx) plus EG20T PCH development board. It uses a few GPIO lines for specific purposes and exposes the rest to the user.

Request the dedicated GPIO lines:
HWID
LVDS_DETECT
PHY_RESET
LED0
LED1

Setup platform drivers for the MinnowBoard LEDs using the leds-gpio driver. Setup led0 and led1 with heartbeat and mmc0 default triggers respectively.

GPIO lines SUS[0-4] are dual purpose, either for LVDS signaling or as user GPIO. Determine which via the LVDS_DETECT signal and enable or disable them accordingly.

Provide a minimal public interface:
minnow_detect()
minnow_lvds_detect()
minnow_hwid()
minnow_phy_reset()

Signed-off-by Darren Hart <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Grant Likely <[email protected]>

Cc Linus Walleij <[email protected]>

Cc Richard Purdie <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Waskiewicz <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc [email protected]

---
 drivers/platform/x86/Kconfig            |  20 ++++
 drivers/platform/x86/Makefile           |   1 +
 drivers/platform/x86/minnowboard-gpio.h |  60 ++++++++++
 drivers/platform/x86/minnowboard.c      | 193 ++++++++++++++++++++++++++++++++
 include/linux/minnowboard.h             |  37 ++++++
 5 files changed, 311 insertions(+)
 create mode 100644 drivers/platform/x86/minnowboard-gpio.h
 create mode 100644 drivers/platform/x86/minnowboard.c
 create mode 100644 include/linux/minnowboard.h

drivers/platform/x86/Kconfig +0 -0

index 8577261..154dbf6 100644

drivers/platform/x86/Kconfig +13 -0

+	---help---
+	  This driver configures the MinnowBoard fixed functionality GPIO lines.
+
+	  It ensures that the E6XX SUS GPIOs muxed with LVDS signals (SUS[0:2])
+	  are disabled if the LVDS_DETECT signal is asserted.
+
+	  If LED_TRIGGER* are enabled, LED0 will use the heartbeat trigger and
+	  LED1 will use the mmc0 trigger.
+
+	  The Minnow Hardware ID is read from the GPIO HWID lines and logged.
+
+	  If you have a MinnowBoard, say Y or M here.
+
 config ACER_WMI
 	tristate "Acer WMI Laptop Extras"
 	depends on ACPI

drivers/platform/x86/Makefile +1 -0

index ef0ec74..45ede1c 100644
@@ -2,6 +2,7 @@
 # Makefile for linux/drivers/platform/x86
 # x86 Platform-Specific Drivers
 #
+obj-$(CONFIG_MINNOWBOARD)	+= minnowboard.o
 obj-$(CONFIG_ASUS_LAPTOP)	+= asus-laptop.o
 obj-$(CONFIG_ASUS_WMI)		+= asus-wmi.o
 obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o

drivers/platform/x86/minnowboard-gpio.h new file +50 -0

new file mode 100644
index 0000000..ccc8361
@@ -0,0 +1,60 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <[email protected]>
+ */
+
+/* MinnowBoard GPIO definitions */
+#define GPIO_BTN0 0
+#define GPIO_BTN1 1
+#define GPIO_BTN2 2
+#define GPIO_BTN3 3
+
+#define GPIO_PROG_VOLTAGE 4
+
+/*
+ * If !LVDS_DETECT, the AUX lines are available as GPIO,
+ * otherwise they are used for LVDS signals.
+ */
+#define GPIO_AUX0 5
+#define GPIO_AUX1 6
+#define GPIO_AUX2 7
+#define GPIO_AUX3 8
+#define GPIO_AUX4 9
+
+#define GPIO_LED0 10
+#define GPIO_LED1 11
+
+#define GPIO_USB_VBUS_DETECT 12
+
+#define GPIO_PHY_RESET 13
+
+#define GPIO_PCH0 244
+#define GPIO_PCH1 245
+#define GPIO_PCH2 246
+#define GPIO_PCH3 247

drivers/platform/x86/minnowboard.c new file +62 -0

new file mode 100644
index 0000000..73d59c1
@@ -0,0 +1,193 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <[email protected]>
+ */
+
+#define pr_fmt(fmt) "MinnowBoard: " fmt
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/dmi.h>
+#include <linux/input.h>
+#include <linux/gpio.h>
+#include <linux/leds.h>
+#include <linux/gpio-sch.h>
+#include <linux/delay.h>
+#include <linux/minnowboard.h>
+#include "minnowboard-gpio.h"
+
+static int minnow_hwid_val = -1;
+
+/* leds-gpio platform device structures */
+static const struct gpio_led minnow_leds[] = {
+	{ .name = "minnow_led0", .gpio = GPIO_LED0, .active_low = 0,
+	  .retain_state_suspended = 1, .default_state = LEDS_GPIO_DEFSTATE_ON,
+	  .default_trigger = "heartbeat"},
+	{ .name = "minnow_led1", .gpio = GPIO_LED1, .active_low = 0,
+	  .retain_state_suspended = 1, .default_state = LEDS_GPIO_DEFSTATE_ON,
+	  .default_trigger = "mmc0"},
+};
+
+static struct gpio_led_platform_data minnow_leds_platform_data = {
+	.num_leds = ARRAY_SIZE(minnow_leds),
+	.leds = (void *) minnow_leds,
+};
+
+static struct platform_device minnow_gpio_leds = {
+	.name =	"leds-gpio",
+	.id = -1,
+	.dev = {
+		.platform_data = &minnow_leds_platform_data,
+	},
+};
+
+static struct gpio hwid_gpios[] = {
+	{ GPIO_HWID0, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid0" },
+	{ GPIO_HWID1, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid1" },

drivers/platform/x86/minnowboard.c new file +7 -0

+bool minnow_detect(void)
+{
+	const char *cmp;
+
+	cmp = dmi_get_system_info(DMI_BOARD_NAME);
+	if (cmp && strstr(cmp, "MinnowBoard"))
+		return true;

drivers/platform/x86/minnowboard.c new file +55 -0

+	/* HWID GPIOs */
+	err = gpio_request_array(hwid_gpios, ARRAY_SIZE(hwid_gpios));
+	if (err) {
+		pr_err("Failed to request hwid GPIO lines\n");
+		goto out;
+	}
+	minnow_hwid_val = (!!gpio_get_value(GPIO_HWID0)) |
+			  (!!gpio_get_value(GPIO_HWID1) << 1) |
+			  (!!gpio_get_value(GPIO_HWID2) << 2);
+
+	pr_info("Hardware ID: %d\n", minnow_hwid_val);
+
+	err = gpio_request_one(GPIO_LVDS_DETECT, GPIOF_DIR_IN | GPIOF_EXPORT,
+			       "minnow_lvds_detect");
+	if (err) {
+		pr_err("Failed to request LVDS_DETECT GPIO line (%d)\n",
+		       GPIO_LVDS_DETECT);
+		goto out;
+	}
+
+	/* Disable the GPIO lines if LVDS is detected */
+	val = minnow_lvds_detect() ? 1 : 0;
+	pr_info("Aux GPIO lines %s\n", val ? "Disabled" : "Enabled");
+	for (i = 0; i < 5; i++)
+		sch_gpio_resume_set_enable(i, !val);
+
+	/* Reserve the AR8031 PHY's ETH_RESET GPIO line */
+	err = gpio_request_one(GPIO_PHY_RESET,
+			       GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT,
+			       "minnow_phy_reset");
+	if (err) {
+		pr_err("Failed to request PHY_RESET GPIO line (%d)\n",
+		       GPIO_PHY_RESET);
+		goto out_lvds;
+	}
+
+	/* GPIO LEDs */
+	err = platform_device_register(&minnow_gpio_leds);
+	if (err) {
+		pr_err("Failed to register leds-gpio platform device\n");
+		goto out_phy;
+	}
+	goto out;
+
+ out_phy:
+	gpio_free(GPIO_PHY_RESET);
+
+ out_lvds:
+	gpio_free(GPIO_LVDS_DETECT);
+
+ out:
+	return err;
+}
+
+static void __exit minnow_module_exit(void)

include/linux/minnowboard.h new file +25 -0

new file mode 100644
index 0000000..d3608b8
@@ -0,0 +1,37 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <[email protected]>
+ */
+
+#ifndef _LINUX_MINNOWBOARD_H
+#define _LINUX_MINNOWBOARD_H
+
+#if defined(CONFIG_MINNOWBOARD) || defined(CONFIG_MINNOWBOARD_MODULE)

Hi,

Quoted 34 lines

The MinnowBoard (http://www.minnowboard.org) is an Intel Atom (E6xx) plus EG20T PCH development board. It uses a few GPIO lines for specific purposes and exposes the rest to the user.

Request the dedicated GPIO lines:
HWID
LVDS_DETECT
PHY_RESET
LED0
LED1

Setup platform drivers for the MinnowBoard LEDs using the leds-gpio driver. Setup led0 and led1 with heartbeat and mmc0 default triggers respectively.

GPIO lines SUS[0-4] are dual purpose, either for LVDS signaling or as user GPIO. Determine which via the LVDS_DETECT signal and enable or disable them accordingly.

Provide a minimal public interface:
minnow_detect()
minnow_lvds_detect()
minnow_hwid()
minnow_phy_reset()

Signed-off-by Darren Hart <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Grant Likely <[email protected]>

Cc Linus Walleij <[email protected]>

Cc Richard Purdie <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Waskiewicz <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc [email protected]

Quoted 10 lines

---
 drivers/platform/x86/Kconfig            |  20 ++++
 drivers/platform/x86/Makefile           |   1 +
 drivers/platform/x86/minnowboard-gpio.h |  60 ++++++++++
 drivers/platform/x86/minnowboard.c      | 193 ++++++++++++++++++++++++++++++++
 include/linux/minnowboard.h             |  37 ++++++
 5 files changed, 311 insertions(+)
 create mode 100644 drivers/platform/x86/minnowboard-gpio.h
 create mode 100644 drivers/platform/x86/minnowboard.c
 create mode 100644 include/linux/minnowboard.h

Hmmmm. x86 boardfiles arriving under drivers/platform.

The main concern is that this won't really scale if more vendors add variations of this board -- needing code changes for each and every one of them. Given that it's an open platform encouraging derivatives, that seems like a slippery slope.

It's really unfortunate that this information couldn't be passed in from firmware. We've been working so hard for so long on ARM now to move away from board files, it's such a pity to add them on another major platform.

Quoted 8 lines

drivers/platform/x86/Kconfig +0 -0

index 8577261..154dbf6 100644
@@ -15,6 +15,26 @@ menuconfig X86_PLATFORM_DEVICES
 
 if X86_PLATFORM_DEVICES
 

drivers/platform/x86/Kconfig menuconfig X86_PLATFORM_DEVICES · line 24

Quoted 7 lines

drivers/platform/x86/Kconfig +7 -0

+config MINNOWBOARD
+	tristate "MinnowBoard GPIO and LVDS support"
+	depends on LPC_SCH
+	depends on GPIO_SCH
+	depends on GPIO_PCH
+	depends on LEDS_GPIO
+	default n

No need to default n. 'n' is the default default. :)

Quoted 10 lines

drivers/platform/x86/Kconfig +10 -0

+#define GPIO_PCH0 244
+#define GPIO_PCH1 245
+#define GPIO_PCH2 246
+#define GPIO_PCH3 247
+#define GPIO_PCH4 248
+#define GPIO_PCH5 249
+#define GPIO_PCH6 250
+#define GPIO_PCH7 251
+
+#define GPIO_HWID0 252

drivers/platform/x86/Kconfig menuconfig X86_PLATFORM_DEVICES · line 38

Quoted 4 lines

drivers/platform/x86/Kconfig +4 -0

+#define GPIO_HWID1 253
+#define GPIO_HWID2 254
+
+#define GPIO_LVDS_DETECT 255

It looks like at least gpio-pch.c uses dynamic gpio numbers, which makes it hard to define these as static numbers since they might move around depending on module load order, etc.

drivers/platform/x86/Kconfig menuconfig X86_PLATFORM_DEVICES

Quoted 7 lines

drivers/platform/x86/Kconfig +7 -0

+int minnow_hwid(void)
+{
+	/* This should never be called prior to minnow_init_module() */
+	WARN_ON_ONCE(minnow_hwid_val == -1);
+	return minnow_hwid_val;
+}
+EXPORT_SYMBOL_GPL(minnow_hwid);

I don't see this used anywhere, so it's hard to tell what the expected use is. I'd just remove it until first user comes along.

Quoted 7 lines

drivers/platform/x86/Kconfig +7 -0

+bool minnow_detect(void)
+{
+	const char *cmp;
+
+	cmp = dmi_get_system_info(DMI_BOARD_NAME);
+	if (cmp && strstr(cmp, "MinnowBoard"))
+		return true;

drivers/platform/x86/Kconfig

Quoted 10 lines

drivers/platform/x86/Kconfig +9 -0

+
+	return false;
+}
+EXPORT_SYMBOL_GPL(minnow_detect);
 
+bool minnow_lvds_detect(void)
+{
+	return !!gpio_get_value(GPIO_LVDS_DETECT);
+}
+EXPORT_SYMBOL_GPL(minnow_lvds_detect);

These two are only used in the other subfiles.

The scope of those files are really narrow, and you end up exporting a set of global functions for it. I suggest you just bake them all together -- there's a small amount of flexibility lost w.r.t. keeping some as modules and others built-in, but the code isn't very large.

drivers/platform/x86/Kconfig

Quoted 13 lines

drivers/platform/x86/Kconfig +12 -0

+void minnow_phy_reset(void)
+{
+	/*
+	 * Hold reset for a little over 1ms and allow some time after to ensure
+	 * the PHY has fully woken up.
+	 */
+	gpio_set_value(GPIO_PHY_RESET, 0);
+	usleep_range(1250, 1500);
+	gpio_set_value(GPIO_PHY_RESET, 1);
+	usleep_range(1250, 1500);
+}
+EXPORT_SYMBOL_GPL(minnow_phy_reset);

What phy? USB? SATA? Ethernet?

I wonder if you'd be better off just putting the GPIO line in the table in the driver instead of the reset function pointer, and have this done from there instead of exporting function pointers from board code to drivers.

drivers/platform/x86/Kconfig

Quoted 13 lines

drivers/platform/x86/Kconfig +12 -0

+static int __init minnow_module_init(void)
+{
+	int err, val, i;
+
+	err = -ENODEV;
+	if (!minnow_detect())
+		goto out;
+
+#ifdef MODULE
+/* Load any implicit dependencies that are not built-in */
+#ifdef CONFIG_LPC_SCH_MODULE
+	if (request_module("lpc_sch"))

Less ifdefs with:

if (IS_MODULE(....) && request_module(..))

drivers/platform/x86/Kconfig

Quoted 5 lines

drivers/platform/x86/Kconfig +5 -0

+
+module_init(minnow_module_init);
+module_exit(minnow_module_exit);
+
+MODULE_LICENSE("GPL");

You probably want a MODULE_DEVICE_TABLE(dmi, ...) here?

Quoted 1 line

include/linux/minnowboard.h +0 -0

Quoted 11 lines

include/linux/minnowboard.h +11 -0

+#ifndef _LINUX_MINNOWBOARD_H
+#define _LINUX_MINNOWBOARD_H
+
+#if defined(CONFIG_MINNOWBOARD) || defined(CONFIG_MINNOWBOARD_MODULE)
+bool minnow_detect(void);
+bool minnow_lvds_detect(void);
+int minnow_hwid(void);
+void minnow_phy_reset(void);
+#else
+#define minnow_detect() (false)
+#define minnow_lvds_detect() (false)

include/linux/minnowboard.h line 35

Quoted 3 lines

include/linux/minnowboard.h +3 -0

+#define minnow_hwid() (-1)
+#define minnow_phy_reset() do { } while (0)
+#endif /* MINNOWBOARD */

Besides the phy_reset() export, the rest of these don't leave this subdirectory. It'd be nice not to expose this very board-specific stuff to the rest of the kernel -- temptation tends to be too great to resist adding more and more things over time.

1) I need time, possibly a couple of months, to get proper ACPI support for these drivers into the firmware. Then I can rewrite these as ACPI drivers as is proper for an x86 board. I've already started down this path.

See what happens when core kernel people are allowed to write driver code? How does this relate to converting this over to an ACPI device driver? I guess I would replace the above with MODULE_DEVICE_TABLE(acpi, ...) ? If I do the above.... is this still an evil board-file? What makes the acpi method of discover superior to setting up linux-hotplug via dmi?

Quoted 4 lines · Darren Hart

1) I need time, possibly a couple of months, to get proper ACPI support for these drivers into the firmware. Then I can rewrite these as ACPI drivers as is proper for an x86 board. I've already started down this path.

A couple of months pushes you back one kernel release. It's not a huge deal. I think I side with Olof here - the kernel developers have pushed back against hardcoded and NIHed ARM device descriptors, and given that we have a perfectly reasonable standard in the X86 world (ie, ACPI), I'm not enthusiastic about merging something that's (a) going to be superseded in the near future and (b) may end up serving as an example to others who think this is ok.

Do these boards currently boot any other OSes?

Quoted 6 lines

See what happens when core kernel people are allowed to write driver code? How does this relate to converting this over to an ACPI device driver? I guess I would replace the above with MODULE_DEVICE_TABLE(acpi, ...) ? If I do the above.... is this still an evil board-file? What makes the acpi method of discover superior to setting up linux-hotplug via dmi?

MODULE_DEVICE_TABLE is invisible to the running driver, it just exports metadata that udev will use to decide whether to load the driver. If a driver is intended to deal with a specific board, DMI makes sense. If it's intended to deal with a specific ACPI device (ie, something with a _HID that defines the programming model), ACPI makes sense.

are all board-specific. They map GPIO to their fixed functions and provide an API for board-specific queries (minnowboard.c), they provide example uses (minnowboard-gpio and minnowboard-keys) which aid in experimentation and the development of new drivers.

Which of these make sense as ACPI devices in your opinion?

Quoted 5 lines · Darren Hart

are all board-specific. They map GPIO to their fixed functions and provide an API for board-specific queries (minnowboard.c), they provide example uses (minnowboard-gpio and minnowboard-keys) which aid in experimentation and the development of new drivers.

Quoted 1 line

Which of these make sense as ACPI devices in your opinion?

Does the firmware contain a corresponding ACPI device with a specific _HID that defines the programming model? If so, it should have an ACPI driver. If not, it shouldn't.

Provide a minimal public interface:
minnow_detect()
minnow_lvds_detect()
minnow_hwid()
minnow_phy_reset()

Quoted 5 lines · Darren Hart

Provide a minimal public interface:
minnow_detect()
minnow_lvds_detect()
minnow_hwid()
minnow_phy_reset()

So instead of these calling drivers issueing gpio_request() themselves to obtain a resource, they make a function call to this proxy that issue gpio_request() for them.

This is generally not how we do things. A driver should request its GPIO just as it requests its regulator or clock or IRQ line or anything else.

Centralizing resource handling is not a good idea IMO, it's better
that each driver request it's GPIO pin(s) and do the stuff it needs with them.

This of course creates the problem of associating the GPIOs to a driver and how it should look that up, which I guess ACPI can do, isn't that what acpi_find_gpio() is for?

minnow_hwid() just returns an int that the minnowboard platform driver read from the GPIO. This seems like a proper abstraction to me. Do you object to this one as well?

Quoted 3 lines · Darren Hart

minnow_hwid() just returns an int that the minnowboard platform driver read from the GPIO. This seems like a proper abstraction to me. Do you object to this one as well?

We should really have a subsystem for this too - the general idea idea of identifying boards, fit options and so on by looking at things like GPIOs or numbers in flash is really common.

Quoted 3 lines · Mark Brown

We should really have a subsystem for this too - the general idea idea of identifying boards, fit options and so on by looking at things like GPIOs or numbers in flash is really common.

Would it then be a bus following the pattern we chiseled out for the soc bus? (Greg, Lee & Arnd architectured this.)

There we needed a struct device * on an overarching level to tie in the sysfs entries reading out the SoC properties. But it would be the same thing with in-kernel accessors for these properties.

So it would be the same pattern above with a board bus, in DT syntax:

board {
     soc {
     };
};

Quoted 8 lines · Linus Walleij

Quoted 3 lines · Mark Brown

We should really have a subsystem for this too - the general idea idea of identifying boards, fit options and so on by looking at things like GPIOs or numbers in flash is really common.

Would it then be a bus following the pattern we chiseled out for the soc bus? (Greg, Lee & Arnd architectured this.)

I'd expect it to be a bus, yes.

Quoted 1 line · Mark Brown

I'd expect it to be a bus, yes.

It seems to me the platform bus already provides everything we need. We can just make a platform driver, say platform-id-gpio.c which can get it's platform data from OF or ACPI (_PRP proposal from kernel summit last week for example). Why would a separate bus type need to be defined?

Quoted 3 lines · Darren Hart

minnow_hwid() just returns an int that the minnowboard platform driver read from the GPIO. This seems like a proper abstraction to me. Do you object to this one as well?

We should really have a subsystem for this too - the general idea idea of identifying boards, fit options and so on by looking at things like GPIOs or numbers in flash is really common.

Quoted 9 lines · Mark Brown

Quoted 3 lines · Darren Hart

minnow_hwid() just returns an int that the minnowboard platform driver read from the GPIO. This seems like a proper abstraction to me. Do you object to this one as well?

We should really have a subsystem for this too - the general idea idea of identifying boards, fit options and so on by looking at things like GPIOs or numbers in flash is really common.

And yet this is a platform with ACPI. I would expect the ACPI to identify the board, not a custom driver. The newest ACPI spec adds a lot of nice useful things like GPIO and SPI bindings. Talk to Al Stone about the progress his team has made on adding GPIO support to ACPICA.

This driver shouldn't be merged into mainline. Keep it as an out-of-tree patch until the proper solution is implemented. That shouldn't be too onerous since we now have available not one, but two mechanisms for describing exactly what you want to do; ACPI or FDT.

Request and export the user-configurable GPIO lines to sysfs. This provides a label readable in /debugfs/gpio and a simple interface for experimenting with GPIO on the MinnowBoard.

This is separate from the minnowboard driver to provide users with the flexibility to write kernel drivers for their own devices using these GPIO lines.

drivers/platform/x86/minnowboard-gpio.c new file +20 -0

+	if (request_module("minnowboard"))
+		goto out;
+#endif
+#endif
+
+	/* Auxillary Expansion GPIOs */
+	if (!minnow_lvds_detect()) {
+		pr_debug("LVDS_DETECT not asserted, configuring Aux GPIO lines\n");
+		err = gpio_request_array(expansion_aux_gpios,
+					 ARRAY_SIZE(expansion_aux_gpios));
+		if (err) {
+			pr_err("Failed to request expansion aux GPIO lines\n");
+			goto out;
+		}
+	} else {
+		pr_debug("LVDS_DETECT asserted, ignoring aux GPIO lines\n");
+	}
+
+	/* Expansion GPIOs */
+	err = gpio_request_array(expansion_gpios, ARRAY_SIZE(expansion_gpios));

Quoted 7 lines · Darren Hart

Request and export the user-configurable GPIO lines to sysfs. This provides a label readable in /debugfs/gpio and a simple interface for experimenting with GPIO on the MinnowBoard.

This is separate from the minnowboard driver to provide users with the flexibility to write kernel drivers for their own devices using these GPIO lines.

Few comments below.

drivers/platform/x86/Kconfig config MINNOWBOARD · line 42

Quoted 11 lines

drivers/platform/x86/Kconfig +5 -0

@@ -35,6 +35,24 @@ config MINNOWBOARD
 
 	  If you have a MinnowBoard, say Y or M here.
 
+if MINNOWBOARD
+config MINNOWBOARD_GPIO
+	tristate "MinnowBoard Expansion GPIO"
+	depends on MINNOWBOARD
+	default n

Like you already had been told you don't need to have default n.

drivers/platform/x86/minnowboard-gpio.c line 27

Quoted 9 lines

drivers/platform/x86/minnowboard-gpio.c new file +6 -0

 
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
+#include <linux/input.h>
+#include <linux/minnowboard.h>

+ empty line here?

Quoted 1 line

+#include "minnowboard-gpio.h"

drivers/platform/x86/minnowboard-gpio.c line 71

Quoted 10 lines

drivers/platform/x86/minnowboard-gpio.c new file +10 -0

+static int __init minnow_gpio_module_init(void)
+{
+	int err;
+
+	err = -ENODEV;
+	if (!minnow_detect())
+		goto out;
+
+#ifdef MODULE
+#ifdef CONFIG_MINNOWBOARD_MODULE

And less ifdefs with IS_MODULE().

Quoted 26 lines

drivers/platform/x86/minnowboard-gpio.c new file +26 -0

+	if (request_module("minnowboard"))
+		goto out;
+#endif
+#endif
+
+	/* Auxillary Expansion GPIOs */
+	if (!minnow_lvds_detect()) {
+		pr_debug("LVDS_DETECT not asserted, configuring Aux GPIO lines\n");
+		err = gpio_request_array(expansion_aux_gpios,
+					 ARRAY_SIZE(expansion_aux_gpios));
+		if (err) {
+			pr_err("Failed to request expansion aux GPIO lines\n");
+			goto out;
+		}
+	} else {
+		pr_debug("LVDS_DETECT asserted, ignoring aux GPIO lines\n");
+	}
+
+	/* Expansion GPIOs */
+	err = gpio_request_array(expansion_gpios, ARRAY_SIZE(expansion_gpios));
+	if (err) {
+		pr_err("Failed to request expansion GPIO lines\n");
+		if (minnow_lvds_detect())
+			gpio_free_array(expansion_aux_gpios,
+					ARRAY_SIZE(expansion_aux_gpios));
+		goto out;

drivers/platform/x86/minnowboard-gpio.c line 101

Quoted 4 lines

drivers/platform/x86/minnowboard-gpio.c new file +4 -0

+	}
+
+ out:
+	return err;

Are you planning to add something else to 'out' path? Otherwise I think it will look better if you do return instead of [useless] gotos.

Quoted 6 lines · Andy Shevchenko

drivers/platform/x86/minnowboard-gpio.c line 101

Quoted 2 lines

+ out:

drivers/platform/x86/minnowboard-gpio.c new file +1 -0

+	return err;

Are you planning to add something else to 'out' path? Otherwise I think it will look better if you do return instead of [useless] gotos.

I suppose this is a matter of preference. I am allergic to multiple return points. However, your argument is consistent with CodingStyle Chapter 7 in that it states "and some common work such as cleanup has to be done." If that "and" is a required sort of &&, then I should change it. Do others have a strong opinion here?

Quoted 9 lines · Darren Hart

Quoted 6 lines · Andy Shevchenko

drivers/platform/x86/minnowboard-gpio.c line 101

Quoted 2 lines

+ out:

drivers/platform/x86/minnowboard-gpio.c new file +1 -0

+	return err;

Are you planning to add something else to 'out' path? Otherwise I think it will look better if you do return instead of [useless] gotos.

Quoted 5 lines

I suppose this is a matter of preference. I am allergic to multiple return points. However, your argument is consistent with CodingStyle Chapter 7 in that it states "and some common work such as cleanup has to be done." If that "and" is a required sort of &&, then I should change it. Do others have a strong opinion here?

There was recently similar discussion. Author finally agreed to change:
http://www.spinics.net/lists/arm-kernel/msg252108.html

"I did say in the changelog I opted for goto over return. But since everybody keeps preferring returns..."

Configure the four buttons tied to the E6XX GPIO lines on the MinnowBoard as keys using the gpio-keys-polled platform driver. From left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI directional keys.

This is separate from the minnowboard driver to provide users with the flexibility to write kernel drivers for their own devices using these GPIO lines.

Quoted 8 lines · Darren Hart

Configure the four buttons tied to the E6XX GPIO lines on the MinnowBoard as keys using the gpio-keys-polled platform driver. From left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI directional keys.

This is separate from the minnowboard driver to provide users with the flexibility to write kernel drivers for their own devices using these GPIO lines.

I'm repeating my comment I did early to you.

I think this driver is a wrong approach, since you just use gpio_keys_polling with custom platform data. So, it should go to the platform code / board file under arch/x86/platform/minnow/...

Quoted 10 lines · Andy Shevchenko

Quoted 8 lines · Darren Hart

Configure the four buttons tied to the E6XX GPIO lines on the MinnowBoard as keys using the gpio-keys-polled platform driver. From left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI directional keys.

This is separate from the minnowboard driver to provide users with the flexibility to write kernel drivers for their own devices using these GPIO lines.

Quoted 5 lines

I'm repeating my comment I did early to you.

I think this driver is a wrong approach, since you just use gpio_keys_polling with custom platform data. So, it should go to the platform code / board file under arch/x86/platform/minnow/...

Olof mentinoed something similar. The reason this is separate is that I can easily see someone wanting to use these buttons in a different way when integrating the MinnowBoard into some kind of product. The minnowboard.c driver sets up the fixed functionality GPIO lines, such as the LEDs (which can be easily reconfigured via triggers) while this driver serves as an example of how the GPIO buttons could be used as keys, but if included in the minnowboard driver, users couldn't get the fixed functionality without also tying up these GPIO lines.

I could remedy that with driver command-line options, but I know I've heard Greg KH discourage their use in the past.

Quoted 27 lines · Darren Hart

Quoted 10 lines · Andy Shevchenko

Quoted 8 lines · Darren Hart

Configure the four buttons tied to the E6XX GPIO lines on the MinnowBoard as keys using the gpio-keys-polled platform driver. From left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI directional keys.

This is separate from the minnowboard driver to provide users with the flexibility to write kernel drivers for their own devices using these GPIO lines.

Quoted 5 lines

I'm repeating my comment I did early to you.

I think this driver is a wrong approach, since you just use gpio_keys_polling with custom platform data. So, it should go to the platform code / board file under arch/x86/platform/minnow/...

Olof mentinoed something similar. The reason this is separate is that I can easily see someone wanting to use these buttons in a different way when integrating the MinnowBoard into some kind of product. The minnowboard.c driver sets up the fixed functionality GPIO lines, such as the LEDs (which can be easily reconfigured via triggers) while this driver serves as an example of how the GPIO buttons could be used as keys, but if included in the minnowboard driver, users couldn't get the fixed functionality without also tying up these GPIO lines.

Quoted 2 lines

I could remedy that with driver command-line options, but I know I've heard Greg KH discourage their use in the past.

Ick, yes, never do that type of thing as a command-line option, that's what device-tree is for :)

Quoted 14 lines · Greg Kroah-Hartman

Quoted 11 lines · Darren Hart

The reason this is separate is that I
can easily see someone wanting to use these buttons in a different way when integrating the MinnowBoard into some kind of product. The minnowboard.c driver sets up the fixed functionality GPIO lines, such as the LEDs (which can be easily reconfigured via triggers) while this driver serves as an example of how the GPIO buttons could be used as keys, but if included in the minnowboard driver, users couldn't get the fixed functionality without also tying up these GPIO lines.

I could remedy that with driver command-line options, but I know I've heard Greg KH discourage their use in the past.

Quoted 2 lines

Ick, yes, never do that type of thing as a command-line option, that's what device-tree is for :)

I suppose when I convert to ACPI drivers I could merge them and have the ACPI table include some data that enabled or disabled things like the minnowboard-keys.... but that seems like more work for the user than it should be to disable the example keys driver.

Quoted 15 lines · Darren Hart

Quoted 14 lines · Greg Kroah-Hartman

Quoted 11 lines · Darren Hart

The reason this is separate is that I
can easily see someone wanting to use these buttons in a different way when integrating the MinnowBoard into some kind of product. The minnowboard.c driver sets up the fixed functionality GPIO lines, such as the LEDs (which can be easily reconfigured via triggers) while this driver serves as an example of how the GPIO buttons could be used as keys, but if included in the minnowboard driver, users couldn't get the fixed functionality without also tying up these GPIO lines.

I could remedy that with driver command-line options, but I know I've heard Greg KH discourage their use in the past.

Quoted 7 lines

Quoted 2 lines

Ick, yes, never do that type of thing as a command-line option, that's what device-tree is for :)

I suppose when I convert to ACPI drivers I could merge them and have the ACPI table include some data that enabled or disabled things like the minnowboard-keys.... but that seems like more work for the user than it should be to disable the example keys driver.

So now you make it sound that devicetree is somehow really superior to ACPI because it can actually be used to do some board-specific configs, and ACPI tables are too hard to use?

Device tree was what we came up with for ARM go get *away* from stashing custom config into the kernel, such as boardfiles and even more horrible things like a command-line switch for every key.

Is x86 now not really presenting anything better? I would have a second look at augmented ACPI tables, if that is what all of x86 is going to use.

FYI here is how I set up a heartbeat LED and some GPIO key in a device tree:

        /* The user LED on the board is set up to be used for heartbeat */
        leds {
                compatible = "gpio-leds";
                user-led {
                        label = "user_led";
                        gpios = <&gpio0 2 0x1>;
                        default-state = "off";
                        linux,default-trigger = "heartbeat";
                };
        };

        /* User key mapped in as "escape" */
        gpio-keys {
                compatible = "gpio-keys";
                user-button {
                        label = "user_button";
                        gpios = <&gpio0 3 0x1>;
                        linux,code = <1>; /* KEY_ESC */
                        gpio-key,wakeup;
                };
        };

Add CircuitCo's newly created VENDOR ID and their first board subsystem ID for the MinnowBoard.

Signed-off-by Darren Hart <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc David Anders <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Waskiewicz <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc [email protected]

---
 include/linux/pci_ids.h | 3 +++
 1 file changed, 3 insertions(+)

include/linux/pci_ids.h +3 -0

index c129162..b2879ce 100644
@@ -149,6 +149,9 @@
 #define PCI_DEVICE_ID_BERKOM_A4T		0xffa4
 #define PCI_DEVICE_ID_BERKOM_SCITEL_QUADRO	0xffa8
 
+#define PCI_VENDOR_ID_CIRCUITCO			0x1cc8
+#define PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD	0x0001
+
 #define PCI_VENDOR_ID_COMPAQ		0x0e11
 #define PCI_DEVICE_ID_COMPAQ_TOKENRING	0x0508
 #define PCI_DEVICE_ID_COMPAQ_TACHYON	0xa0fc

Signature · 1 line

1.8.1.2

Quoted 31 lines · Darren Hart

Add CircuitCo's newly created VENDOR ID and their first board subsystem ID for the MinnowBoard.

Signed-off-by Darren Hart <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc David Anders <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Waskiewicz <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc [email protected]

---
 include/linux/pci_ids.h | 3 +++
 1 file changed, 3 insertions(+)

include/linux/pci_ids.h +3 -0

index c129162..b2879ce 100644
@@ -149,6 +149,9 @@
 #define PCI_DEVICE_ID_BERKOM_A4T		0xffa4
 #define PCI_DEVICE_ID_BERKOM_SCITEL_QUADRO	0xffa8
 
+#define PCI_VENDOR_ID_CIRCUITCO			0x1cc8
+#define PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD	0x0001
+
 #define PCI_VENDOR_ID_COMPAQ		0x0e11
 #define PCI_DEVICE_ID_COMPAQ_TOKENRING	0x0508
 #define PCI_DEVICE_ID_COMPAQ_TACHYON	0xa0fc

Signature · 1 line

1.8.1.2

Per conversation at [1], I merged the patch below to my pci/misc branch for v3.11:

[1] https://lkml.kernel.org/r/6c27e79870ec93f7a8c6692d4bcfebaee589fa6b.1372211451.git.dvhart@linux.intel.com

commit 9f4de80a09667538e1162fdecff96ccb5bb354a8

Author: Darren Hart <[email protected]>
Date:   Tue Jun 25 20:08:46 2013 -0600

PCI: Add CircuitCo vendor ID

Add CircuitCo's newly created VENDOR ID

[bhelgaas: sort, drop device ID (only used once)]

Signed-off-by Darren Hart <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

include/linux/pci_ids.h +0 -0

index c129162..8b1aa7f 100644

include/linux/pci_ids.h line 2482

Quoted 4 lines · Bjorn Helgaas

include/linux/pci_ids.h +2 -0

+#define PCI_VENDOR_ID_CIRCUITCO              0x1cc8
+
 #define PCI_VENDOR_ID_TEKRAM         0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290   0xdc29

Thanks Bjorn. When I reuse this Subsystem ID and there is more than one usage, I should send a patch to pci_ids.h adding it and replace the hex value in all drivers with the new define. Is that right?

Quoted 11 lines · Darren Hart

include/linux/pci_ids.h line 2482

Quoted 4 lines · Bjorn Helgaas

include/linux/pci_ids.h +2 -0

+#define PCI_VENDOR_ID_CIRCUITCO              0x1cc8
+
 #define PCI_VENDOR_ID_TEKRAM         0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290   0xdc29

Thanks Bjorn. When I reuse this Subsystem ID and there is more than one usage, I should send a patch to pci_ids.h adding it and replace the hex value in all drivers with the new define. Is that right?

Yeah, that's what I was thinking.

But Peter's comment makes more sense to me now. The spec refers to that config register as "Subsystem ID," not "Subsystem Device ID," but I was confused because most existing usage treats it as a device ID. For example, the field in struct pci_device_id is named "subdevice," and all the existing #defines in pci_ids.h are of the form PCI_SUBDEVICE_ID_*.

Device IDs are pretty specific identifiers, so I was thinking that a "sub-device ID" would be even more specific. Then it would make no sense to have a "sub-device ID" that was as generic as "MINNOWBOARD." But the register is actually *not* a "sub-device ID," and I can see that using the same Subsystem ID for all the devices on a board might make sense.

So I think the name PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD is a bit of a misnomer, and something like PCI_SUBSYSTEM_CIRCUITCO_MINNOWBOARD would make it more clear that it really isn't sharing the device ID space assigned to CircuitCo. It would make perfect sense to have a Device ID, e.g., "PCI_DEVICE_ID_CIRCUIT_CO_xxx 0x0001," that has nothing to do with the Subsystem ID 0x0001.

If you want to do something like that (or even keep your original patch), I can put that in my -next branch. Just let me know.

Bjorn

Quoted 36 lines · Bjorn Helgaas

Quoted 11 lines · Darren Hart

Review

Quoted 4 lines · Bjorn Helgaas

(quoted diff) +2 -0

+#define PCI_VENDOR_ID_CIRCUITCO              0x1cc8
+
 #define PCI_VENDOR_ID_TEKRAM         0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290   0xdc29

Thanks Bjorn. When I reuse this Subsystem ID and there is more than one usage, I should send a patch to pci_ids.h adding it and replace the hex value in all drivers with the new define. Is that right?

Yeah, that's what I was thinking.

But Peter's comment makes more sense to me now. The spec refers to that config register as "Subsystem ID," not "Subsystem Device ID," but I was confused because most existing usage treats it as a device ID. For example, the field in struct pci_device_id is named "subdevice," and all the existing #defines in pci_ids.h are of the form PCI_SUBDEVICE_ID_*.

Device IDs are pretty specific identifiers, so I was thinking that a "sub-device ID" would be even more specific. Then it would make no sense to have a "sub-device ID" that was as generic as "MINNOWBOARD." But the register is actually *not* a "sub-device ID," and I can see that using the same Subsystem ID for all the devices on a board might make sense.

So I think the name PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD is a bit of a misnomer, and something like PCI_SUBSYSTEM_CIRCUITCO_MINNOWBOARD would make it more clear that it really isn't sharing the device ID space assigned to CircuitCo. It would make perfect sense to have a Device ID, e.g., "PCI_DEVICE_ID_CIRCUIT_CO_xxx 0x0001," that has nothing to do with the Subsystem ID 0x0001.

Quoted 4 lines

If you want to do something like that (or even keep your original patch), I can put that in my -next branch. Just let me know.

Bjorn

I would be happy to change DEVICE to SUBSYSTEM in the the pci_ids.h:

(quoted diff) +2 -0

+#define PCI_VENDOR_ID_CIRCUITCO		0x1cc8
+#define PCI_SUBSYSTEM_ID_CITCUITCO_MINNOWBOARD	0x0001

Would you like me to send another patch?

Yeah, that's what I was thinking.

But Peter's comment makes more sense to me now. The spec refers to that config register as "Subsystem ID," not "Subsystem Device ID," but I was confused because most existing usage treats it as a device ID. For example, the field in struct pci_device_id is named "subdevice," and all the existing #defines in pci_ids.h are of the form PCI_SUBDEVICE_ID_*.

Device IDs are pretty specific identifiers, so I was thinking that a "sub-device ID" would be even more specific. Then it would make no sense to have a "sub-device ID" that was as generic as "MINNOWBOARD." But the register is actually *not* a "sub-device ID," and I can see that using the same Subsystem ID for all the devices on a board might make sense.

Quoted 10 lines · Bjorn Helgaas

Yeah, that's what I was thinking.

But Peter's comment makes more sense to me now. The spec refers to that config register as "Subsystem ID," not "Subsystem Device ID," but I was confused because most existing usage treats it as a device ID. For example, the field in struct pci_device_id is named "subdevice," and all the existing #defines in pci_ids.h are of the form PCI_SUBDEVICE_ID_*.

Quoted 7 lines

Device IDs are pretty specific identifiers, so I was thinking that a "sub-device ID" would be even more specific. Then it would make no sense to have a "sub-device ID" that was as generic as "MINNOWBOARD." But the register is actually *not* a "sub-device ID," and I can see that using the same Subsystem ID for all the devices on a board might make sense.

Subsystem IDs is basically a board ID in the traditional PC view, but they didn't call it that because it would have been confusing in other, nontraditional configurations.

Microsoft has a "best practices" document, which may end up becoming basis for a future PCI-SIG document clarifying the standard:

http://msdn.microsoft.com/en-us/library/windows/hardware/gg463287.aspx

Quoted 4 lines

Microsoft has a "best practices" document, which may end up becoming basis for a future PCI-SIG document clarifying the standard:

Quoted 1 line

http://msdn.microsoft.com/en-us/library/windows/hardware/gg463287.aspx

Interesting, thanks for the link. If I read that correctly, the MinnowBoard is basically a motherboard, and any board layout change or component value change will require a new Subsystem ID, which will in turn require a pch_gbe update. That doesn't sound optimal, but maybe people don't actually interpret it that strictly.

The MinnowBoard uses an AR803x PHY with the PCH GBE.

It does not implement the RGMII 2ns TX clock delay in the trace routing nor via strapping. Add a detection method for the board and the PHY and enable the tx clock delay via the registers.

This PHY will hibernate without link for 10 seconds. Ensure the PHY is awake for probe and then disable hibernation. A future improvement would be to convert pch_gbe to using PHYLIB and making sure we can wake the PHY at the necessary times rather than permanently disabling it.

Use the MinnowBoard PCI subsystem ID to identify the board and setup the appropriate callbacks in a new pci_id driver_data structure.

Signed-off-by Darren Hart <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Waskiewicz <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc [email protected]

---
 drivers/net/ethernet/oki-semi/pch_gbe/Kconfig      |  1 +
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |  2 +
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 32 ++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 89 ++++++++++++++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
 5 files changed, 126 insertions(+)

...

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +0 -0

index 6667a6b..6f0b9e3 100644
...

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +6 -0

+        .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
+        .class_mask = (0xFFFF00),
+        .driver_data = (unsigned long) &pch_gbe_minnowboard_privdata
+        },
+       {.vendor = PCI_VENDOR_ID_INTEL,
+        .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
         .subvendor = PCI_ANY_ID,
         .subdevice = PCI_ANY_ID,
         .class = (PCI_CLASS_NETWORK_ETHERNET << 8),

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c err_disable_device: · line 2728

Quoted 5 lines · Darren Hart

static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
{.vendor = PCI_VENDOR_ID_INTEL,
.device = PCI_DEVICE_ID_INTEL_IOH1_GBE,

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +2 -0

+        .subvendor = PCI_VENDOR_ID_CIRCUITCO,
+        .subdevice = PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD,

"MINNOWBOARD" seems like a pretty generic name for something that probably refers only to the gigabit ethernet device in the EG20T. If you expect to use that same subdevice ID on other devices, I guess we can add PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD to pci_ids.h. If it will only be used for the gigabit ethernet device, we would normally not add a #define for it and would just use the hex constant here (see the comment at the top of pci_ids.h).

Quoted 3 lines · Darren Hart

The MinnowBoard uses an AR803x PHY with the PCH GBE.

It does not implement the RGMII 2ns TX clock delay in the trace

routing

Quoted 1 line

nor via strapping. Add a detection method for the board and the PHY

and

Quoted 3 lines

enable the tx clock delay via the registers.

This PHY will hibernate without link for 10 seconds. Ensure the PHY

is

Quoted 1 line

awake for probe and then disable hibernation. A future improvement

would

Quoted 4 lines

be to convert pch_gbe to using PHYLIB and making sure we can wake the PHY at the necessary times rather than permanently disabling it.

Use the MinnowBoard PCI subsystem ID to identify the board and setup

the

Quoted 8 lines

appropriate callbacks in a new pci_id driver_data structure.

Signed-off-by Darren Hart <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Peter Waskiewicz <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc [email protected]

Quoted 5 lines

---
 drivers/net/ethernet/oki-semi/pch_gbe/Kconfig      |  1 +
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |  2 +
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 32 ++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 89

++++++++++++++++++++++

Review

Quoted 5 lines

 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
 5 files changed, 126 insertions(+)

...

(quoted diff) +0 -0

b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c

Quoted 4 lines

index 6667a6b..6f0b9e3 100644

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +0 -0

...

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c err_disable_device: · line 2728

Quoted 10 lines

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +7 -0

+static struct pch_gbe_privdata pch_gbe_minnowboard_privdata = {
+       .phy_reset = minnow_phy_reset,
+       .phy_tx_clk_delay = pch_gbe_phy_tx_clk_delay,
+};
+
 static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
        {.vendor = PCI_VENDOR_ID_INTEL,
         .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
+        .subvendor = PCI_VENDOR_ID_CIRCUITCO,
+        .subdevice = PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD,

"MINNOWBOARD" seems like a pretty generic name for something that probably refers only to the gigabit ethernet device in the EG20T. If you expect to use that same subdevice ID on other devices, I guess we can add PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD to pci_ids.h. If it will only be used for the gigabit ethernet device, we would normally not add a #define for it and would just use the hex constant here (see the comment at the top of pci_ids.h).

Quoted 9 lines

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +6 -0

+        .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
+        .class_mask = (0xFFFF00),
+        .driver_data = (unsigned long) &pch_gbe_minnowboard_privdata
+        },
+       {.vendor = PCI_VENDOR_ID_INTEL,
+        .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
         .subvendor = PCI_ANY_ID,
         .subdevice = PCI_ANY_ID,
         .class = (PCI_CLASS_NETWORK_ETHERNET << 8),

The MinnowBoard uses an AR803x PHY with the PCH GBE.

It does not implement the RGMII 2ns TX clock delay in the trace routing nor via strapping. Add a detection method for the board and the PHY and enable the tx clock delay via the registers.

This PHY will hibernate without link for 10 seconds. Ensure the PHY is awake for probe and then disable hibernation. A future improvement would be to convert pch_gbe to using PHYLIB and making sure we can wake the PHY at the necessary times rather than permanently disabling it.

Use the MinnowBoard PCI subsystem ID to identify the board and setup the appropriate callbacks in a new pci_id driver_data structure.