[PATCH 0/8] MinnowBoard support

[PATCH 0/8] MinnowBoard support

[Darren Hart]

The following patches add support for the MinnowBoard:

http://www.minnowboard.org

The MinnowBoard is an E6xx Atom CPU paired with an EG20T PCH on a small
(4.25" x 4.25") board with USB, MMC, SATA, Ethernet, audio, GPIO, GPIO
buttons, GPIO LEDs, and EMGD graphics. It has an expansion connector
allowing for daughter cards (Lures) to access things like I2C, SPI, CAN,
UARTs, GPIO, LVDS, SATA and USB. The Lure specification is available
here:

http://www.elinux.org/Minnowboard:Lures_Specifications

This is a rather unique x86 board and the platform drivers are
admittedly atypical for x86. They are intended to provide board-specifc
support where no discovery is possible as well as example drivers and
convenient access to the available GPIO. Future improvements may include
ACPI versions of these drivers, which would at least make them
discoverable.

The following changes since commit 1e876e3b1a9df25bb04682b0d48aaa7e8ae1fc82:

  Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux (2013-06-25 09:08:07 -1000)

are available in the git repository at:

  git://git.infradead.org/users/dvhart/linux-2.6.git minnow/platform-v1
  ssh://git.infradead.org/srv/git/users/dvhart/linux-2.6.git minnow/platform-v1
  http://git.infradead.org/users/dvhart/linux-2.6.git/shortlog/refs/heads/minnow/platform-v1

Darren Hart (8):
  pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32
  pch_uart: Add uart_clk selection for the MinnowBoard
  gpio-sch: Add sch_gpio_resume_set_enable()
  minnowboard: Add base platform driver for the MinnowBoard
  minnowboard-gpio: Export MinnowBoard expansion GPIO
  minnowboard-keys: Bind MinnowBoard buttons to arrow keys
  pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID
  pch_gbe: Add MinnowBoard support

 drivers/gpio/gpio-sch.c                            |  24 +++
 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   |  34 +++-
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    |  89 ++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |   2 +
 drivers/platform/x86/Kconfig                       |  52 ++++++
 drivers/platform/x86/Makefile                      |   3 +
 drivers/platform/x86/minnowboard-gpio.c            | 115 ++++++++++++
 drivers/platform/x86/minnowboard-gpio.h            |  60 +++++++
 drivers/platform/x86/minnowboard-keys.c            | 108 ++++++++++++
 drivers/platform/x86/minnowboard.c                 | 193 +++++++++++++++++++++
 drivers/tty/serial/pch_uart.c                      |   5 +
 include/linux/gpio-sch.h                           |   6 +
 include/linux/minnowboard.h                        |  37 ++++
 include/linux/pci_ids.h                            |   3 +
 16 files changed, 733 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/x86/minnowboard-gpio.c
 create mode 100644 drivers/platform/x86/minnowboard-gpio.h
 create mode 100644 drivers/platform/x86/minnowboard-keys.c
 create mode 100644 drivers/platform/x86/minnowboard.c
 create mode 100644 include/linux/gpio-sch.h
 create mode 100644 include/linux/minnowboard.h

-- 
1.8.1.2

[PATCH 1/8] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32

[Darren Hart]

Avoid using magic numbers when we have perfectly good defines just lying
around.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 0c1c65a..6667a6b 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -670,7 +670,7 @@ static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter)
 	}
 	adapter->hw.phy.addr = adapter->mii.phy_id;
 	pr_debug("phy_addr = %d\n", adapter->mii.phy_id);
-	if (addr == 32)
+	if (addr == PCH_GBE_PHY_REGS_LEN)
 		return -EAGAIN;
 	/* Selected the phy and isolate the rest */
 	for (addr = 0; addr < PCH_GBE_PHY_REGS_LEN; addr++) {
-- 
1.8.1.2

[PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Darren Hart]

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 <dvhart@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/pch_uart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 21a7e17..572d481 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -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;
@@ -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;
+
 	return DEFAULT_UARTCLK;
 }
 
-- 
1.8.1.2

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Greg Kroah-Hartman]

On Tue, Jun 25, 2013 at 06:53:22PM -0700, Darren Hart wrote:
> 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 <dvhart@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/pch_uart.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
> index 21a7e17..572d481 100644
> --- a/drivers/tty/serial/pch_uart.c
> +++ b/drivers/tty/serial/pch_uart.c
> @@ -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;
> @@ -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...

greg k-h

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Darren Hart]

On Tue, 2013-06-25 at 19:31 -0700, Greg Kroah-Hartman wrote:
> On Tue, Jun 25, 2013 at 06:53:22PM -0700, Darren Hart wrote:
>   
> >  struct pch_uart_buffer {
> >  	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...

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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Greg Kroah-Hartman]

On Tue, Jun 25, 2013 at 08:16:18PM -0700, Darren Hart wrote:
> On Tue, 2013-06-25 at 19:31 -0700, Greg Kroah-Hartman wrote:
> > On Tue, Jun 25, 2013 at 06:53:22PM -0700, Darren Hart wrote:
> >   
> > >  struct pch_uart_buffer {
> > >  	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...
> 
> 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...

> 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?

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

thanks,

greg k-h

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Darren Hart]

On Tue, 2013-06-25 at 20:39 -0700, Greg Kroah-Hartman wrote:
> On Tue, Jun 25, 2013 at 08:16:18PM -0700, Darren Hart wrote:
> > On Tue, 2013-06-25 at 19:31 -0700, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 25, 2013 at 06:53:22PM -0700, Darren Hart wrote:
> > >   
> > > >  struct pch_uart_buffer {
> > > >  	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...
> > 
> > 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...

I had expected there to be some discussion on this series as a whole
and didn't want to confuse things... I suppose that was silly as the
patch will just whither and die if it never makes it into mainline.
Apologies.

> 
> > 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?
> 
> 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:

>From 22cb21e2421ffcd439f58843422cff0e2579902f Mon Sep 17 00:00:00 2001
Message-Id:
<22cb21e2421ffcd439f58843422cff0e2579902f.1372219079.git.dvhart@linux.intel.com>
From: Darren Hart <dvhart@linux.intel.com>
Date: Tue, 25 Jun 2013 20:54:29 -0700
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 <dvhart@linux.intel.com>
---
 drivers/tty/serial/pch_uart.c | 71
+++++++++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/pch_uart.c
b/drivers/tty/serial/pch_uart.c
index 572d481..271cc73 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -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);
-	if (cmp && strstr(cmp, "MinnowBoard"))
-		return MINNOW_UARTCLK;
+	d = dmi_first_match(pch_uart_dmi_table);
+	if (d)
+		return (int)d->driver_data;
 
 	return DEFAULT_UARTCLK;
 }
-- 
1.8.1.2



-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Greg Kroah-Hartman]

On Tue, Jun 25, 2013 at 08:58:45PM -0700, Darren Hart wrote:
> > 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:

Yes, that patch looks good to me, and it makes it easier to add new
devices later as well.

thanks,

greg k-h

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Jiri Slaby]

On 06/26/2013 05:58 AM, Darren Hart wrote:
> 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 <dvhart@linux.intel.com>
> ---
>  drivers/tty/serial/pch_uart.c | 71
> +++++++++++++++++++++++++++++--------------
>  1 file changed, 49 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/pch_uart.c
> b/drivers/tty/serial/pch_uart.c
> index 572d481..271cc73 100644
> --- a/drivers/tty/serial/pch_uart.c
> +++ b/drivers/tty/serial/pch_uart.c
> @@ -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);
> -	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...

-- 
js
suse labs

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 08:43 +0200, Jiri Slaby wrote:
> On 06/26/2013 05:58 AM, Darren Hart wrote:
> > 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 <dvhart@linux.intel.com>
> > ---
> >  drivers/tty/serial/pch_uart.c | 71
> > +++++++++++++++++++++++++++++--------------
> >  1 file changed, 49 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/pch_uart.c
> > b/drivers/tty/serial/pch_uart.c
> > index 572d481..271cc73 100644
> > --- a/drivers/tty/serial/pch_uart.c
> > +++ b/drivers/tty/serial/pch_uart.c
> > @@ -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);
> > -	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,
but yes, I can clean that up just on principle. Thank you for pointing
that out.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Jiri Slaby]

On 06/26/2013 09:19 AM, Darren Hart wrote:
> 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?

-- 
js
suse labs

Re: [PATCH 2/8] pch_uart: Add uart_clk selection for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 09:25 +0200, Jiri Slaby wrote:
> On 06/26/2013 09:19 AM, Darren Hart wrote:
> > 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?

As far as I know it isn't that you couldn't use it with a 64b CPU, it
just hasn't been. I'm happy to use a long as you suggest.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

[PATCH 3/8] gpio-sch: Add sch_gpio_resume_set_enable()

[Darren Hart]

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.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-sch.c  | 24 ++++++++++++++++++++++++
 include/linux/gpio-sch.h |  6 ++++++
 2 files changed, 30 insertions(+)
 create mode 100644 include/linux/gpio-sch.h

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 5af6571..90785f8 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -41,6 +41,30 @@ static DEFINE_SPINLOCK(gpio_lock);
 
 static unsigned short gpio_ba;
 
+void sch_gpio_resume_set_enable(unsigned gpio_num, int val)
+{
+	u8 curr_en;
+	unsigned short offset, bit;
+
+	spin_lock(&gpio_lock);
+
+	offset = RGEN + gpio_num / 8;
+	bit = gpio_num % 8;
+
+	curr_en = inb(gpio_ba + offset);
+
+	if (val) {
+		if (!(curr_en & (1 << bit)))
+			outb(curr_en | (1 << bit), gpio_ba + offset);
+	} else {
+		if ((curr_en & (1 << bit)))
+			outb(curr_en & ~(1 << bit), gpio_ba + offset);
+	}
+
+	spin_unlock(&gpio_lock);
+}
+EXPORT_SYMBOL_GPL(sch_gpio_resume_set_enable);
+
 static int sch_gpio_core_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
 {
 	u8 curr_dirs;
diff --git a/include/linux/gpio-sch.h b/include/linux/gpio-sch.h
new file mode 100644
index 0000000..79e042f
--- /dev/null
+++ b/include/linux/gpio-sch.h
@@ -0,0 +1,6 @@
+#ifndef _LINUX_GPIO_SCH_
+#define _LINUX_GPIO_SCH_
+
+void sch_gpio_resume_set_enable(unsigned gpio_num, int val);
+
+#endif /* _LINUX_GPIO_SCH_ */
-- 
1.8.1.2

Re: [PATCH 3/8] gpio-sch: Add sch_gpio_resume_set_enable()

[Darren Hart]

On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote:
> 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.

Given the discussion on the platform/x86 minnowboard drivers, I believe
this patch can be dropped. Please do not pull.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

[PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

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 <dvhart@linux.intel.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org
---
 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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 8577261..154dbf6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -15,6 +15,26 @@ menuconfig X86_PLATFORM_DEVICES
 
 if X86_PLATFORM_DEVICES
 
+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
+	---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
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index ef0ec74..45ede1c 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -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
diff --git a/drivers/platform/x86/minnowboard-gpio.h b/drivers/platform/x86/minnowboard-gpio.h
new file mode 100644
index 0000000..ccc8361
--- /dev/null
+++ b/drivers/platform/x86/minnowboard-gpio.h
@@ -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 <dvhart@linux.intel.com>
+ */
+
+/* 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
+#define GPIO_PCH4 248
+#define GPIO_PCH5 249
+#define GPIO_PCH6 250
+#define GPIO_PCH7 251
+
+#define GPIO_HWID0 252
+#define GPIO_HWID1 253
+#define GPIO_HWID2 254
+
+#define GPIO_LVDS_DETECT 255
diff --git a/drivers/platform/x86/minnowboard.c b/drivers/platform/x86/minnowboard.c
new file mode 100644
index 0000000..73d59c1
--- /dev/null
+++ b/drivers/platform/x86/minnowboard.c
@@ -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 <dvhart@linux.intel.com>
+ */
+
+#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" },
+	{ GPIO_HWID2, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid2" },
+};
+
+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);
+
+bool minnow_detect(void)
+{
+	const char *cmp;
+
+	cmp = dmi_get_system_info(DMI_BOARD_NAME);
+	if (cmp && strstr(cmp, "MinnowBoard"))
+		return true;
+
+	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);
+
+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);
+
+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"))
+		goto out;
+#endif
+#ifdef CONFIG_GPIO_SCH_MODULE
+	if (request_module("gpio-sch"))
+		goto out;
+#endif
+#ifdef CONFIG_GPIO_PCH_MODULE
+	if (request_module("gpio-pch"))
+		goto out;
+#endif
+#endif
+
+	/* 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)
+{
+	gpio_free(GPIO_LVDS_DETECT);
+	gpio_free(GPIO_PHY_RESET);
+	platform_device_unregister(&minnow_gpio_leds);
+}
+
+module_init(minnow_module_init);
+module_exit(minnow_module_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/minnowboard.h b/include/linux/minnowboard.h
new file mode 100644
index 0000000..d3608b8
--- /dev/null
+++ b/include/linux/minnowboard.h
@@ -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 <dvhart@linux.intel.com>
+ */
+
+#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)
+#define minnow_hwid() (-1)
+#define minnow_phy_reset() do { } while (0)
+#endif /* MINNOWBOARD */
+
+#endif /* _LINUX_MINNOWBOARD_H */
-- 
1.8.1.2

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Olof Johansson]

Hi,

On Tue, Jun 25, 2013 at 06:53:24PM -0700, Darren Hart wrote:
> 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 <dvhart@linux.intel.com>
> Cc: Matthew Garrett <matthew.garrett@nebula.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: platform-driver-x86@vger.kernel.org
> ---
>  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.

> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 8577261..154dbf6 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -15,6 +15,26 @@ menuconfig X86_PLATFORM_DEVICES
>  
>  if X86_PLATFORM_DEVICES
>  
> +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. :)

> +	---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
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index ef0ec74..45ede1c 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -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
> diff --git a/drivers/platform/x86/minnowboard-gpio.h b/drivers/platform/x86/minnowboard-gpio.h
> new file mode 100644
> index 0000000..ccc8361
> --- /dev/null
> +++ b/drivers/platform/x86/minnowboard-gpio.h
> @@ -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 <dvhart@linux.intel.com>
> + */
> +
> +/* 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
> +#define GPIO_PCH4 248
> +#define GPIO_PCH5 249
> +#define GPIO_PCH6 250
> +#define GPIO_PCH7 251
> +
> +#define GPIO_HWID0 252
> +#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.

Looks like gpio-sch has hardcoded base 0, so the low-numbered ones might be OK
for now.

> diff --git a/drivers/platform/x86/minnowboard.c b/drivers/platform/x86/minnowboard.c
> new file mode 100644
> index 0000000..73d59c1
> --- /dev/null
> +++ b/drivers/platform/x86/minnowboard.c
> @@ -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 <dvhart@linux.intel.com>
> + */
> +
> +#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" },
> +	{ GPIO_HWID2, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid2" },
> +};
> +
> +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.

> +bool minnow_detect(void)
> +{
> +	const char *cmp;
> +
> +	cmp = dmi_get_system_info(DMI_BOARD_NAME);
> +	if (cmp && strstr(cmp, "MinnowBoard"))
> +		return true;
> +
> +	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.

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

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

> +	/* 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)
> +{
> +	gpio_free(GPIO_LVDS_DETECT);
> +	gpio_free(GPIO_PHY_RESET);
> +	platform_device_unregister(&minnow_gpio_leds);
> +}
> +
> +module_init(minnow_module_init);
> +module_exit(minnow_module_exit);
> +
> +MODULE_LICENSE("GPL");

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

> diff --git a/include/linux/minnowboard.h b/include/linux/minnowboard.h
> new file mode 100644
> index 0000000..d3608b8
> --- /dev/null
> +++ b/include/linux/minnowboard.h
> @@ -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 <dvhart@linux.intel.com>
> + */
> +
> +#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)
> +#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.


-Olof

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Tue, 2013-06-25 at 21:00 -0700, Olof Johansson wrote:
> Hi,

Hey Olof, thanks for the review!

David M, search for "minnow_phy_reset" for your bit :-)

> 
> On Tue, Jun 25, 2013 at 06:53:24PM -0700, Darren Hart wrote:
> > 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 <dvhart@linux.intel.com>
> > Cc: Matthew Garrett <matthew.garrett@nebula.com>
> > Cc: Grant Likely <grant.likely@linaro.org>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: platform-driver-x86@vger.kernel.org
> > ---
> >  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. 


Indeed, I hate myself for this :-)

Here was my rationale, feel free to pick it apart:

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.

2) I felt the value of getting something upstream, even if it isn't
perfect, before the board ships was preferable to only having it in
what is effectively a vendor tree (linux-yocto_3.8 standard/minnow) and
risking various other implementations popping up and confusing the
situation.

3) I at least wanted to fix the pch_gbe support which is currently tied
up with these platform drivers. I considered pushing the
minnow_phy_reset into pch_gbe, but I previously was scolded for putting
too much board-specific knowledge into that driver.


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


+100000


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


Nod, see above. If this trumps the rationale above, it might just make
sense to push the phy reset into the pch_gbe driver and hold off on
these. Alternatively, we can choose to accept that this is transitional
with the understanding that drivers/platform/x86/minnow* *will* go
away.


> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 8577261..154dbf6 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -15,6 +15,26 @@ menuconfig X86_PLATFORM_DEVICES
> >  
> >  if X86_PLATFORM_DEVICES
> >  
> > +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. :)


Check. Thanks.

...

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


Oh dear, not something I've experienced. OK, I'll go look into how to
properly handle that case. Any pointers?

...

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


OK, can do.


> > +bool minnow_detect(void)
> > +{
> > +	const char *cmp;
> > +
> > +	cmp = dmi_get_system_info(DMI_BOARD_NAME);
> > +	if (cmp && strstr(cmp, "MinnowBoard"))
> > +		return true;
> > +
> > +	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.


I had them together initially. The reason I broke them up was to
separate fixed functionality from example code. minnowboard.c sets up
the API and any fixed things, like the GPIO LEDs. Those can be easily
configured through LED triggers, so no need to allow for a different
kind of driver.

minnowboard-gpio.c provides easy user access to the GPIO, but would
conflict with a properly written driver for a device driven with the
GPIO. This one is for prototyping and experimentation, but shouldn't be
considered as a "permanent" driver like minnowboard.c is.

minnowboard-keys.c is example code and could possibly be considered a
"permanent" driver once the ACPI version makes it a bit more
configurable by specifying the key codes and such outside the kernel
driver.

minnow_detect() can be replaced with DMI_MATCH() anywhere, so perhaps
this should just go away once the drivers are ACPI'ified.

minnow_lvds_detect().... that's a long story. I only really need it in
minnowboard-gpio.c to make sure I only export the GPIO if they are not
being used for LVDS. I could move it to that driver and remove the
export altogether.


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

Ethernet, yes.

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


I bike-shed'ed this until I was blue and decided I should leave it up
to David Miller as he was likely to have an opinion on consolidation of
functionality versus board-agnostic code.

David, would you prefer I set the GPIO from the PCI Subsystem ID and
move the reset function into the pch_gbe driver?


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


Shiny. Can do. Thanks.


> > +
> > +module_init(minnow_module_init);
> > +module_exit(minnow_module_exit);
> > +
> > +MODULE_LICENSE("GPL");
> 
> You probably want a MODULE_DEVICE_TABLE(dmi, ...) here?


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?

I think there is precious piece of learning to be had here (for me I
mean)...


> > diff --git a/include/linux/minnowboard.h b/include/linux/minnowboard.h
...
> > +#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)
> > +#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.

Good point. I'll address in V2 after we bat around some of the above a
bit.

Thank you for taking the time Olof!

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1706 bytes --]

On Tue, 2013-06-25 at 21:43 -0700, Darren Hart wrote:

> 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?

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

-- 
Matthew Garrett | mjg59@srcf.ucam.org
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Matthew Garrett]

On Tue, 2013-06-25 at 21:43 -0700, Darren Hart wrote:

> 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?

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

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 04:52 +0000, Matthew Garrett wrote:
> On Tue, 2013-06-25 at 21:43 -0700, Darren Hart wrote:
> 
> > 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.


My biggest concern is fragmentation after the boards start to ship. I
recognize this comes under the "lack of planning on your part doesn't
constitute an emergency on my part" heading (although it really wasn't
a lack of planning). Now that this is out here and people can see where
it's going, if we choose to nack these and wait for a better
implementation, that alone may accomplish the same goal.


> Do these boards currently boot any other OSes?


Currently the linux-yocto_3.8 standard/minnow Linux kernel is the only
thing known to boot on it. This is what will ship with the device.


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

What are you referring to with "programming model" here?

The three drivers in question:
minnowboard.c
minnowboard-gpio.c
minnowboard-keys.c

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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 04:52 +0000, Matthew Garrett wrote:
> On Tue, 2013-06-25 at 21:43 -0700, Darren Hart wrote:
> 
> > 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.


My biggest concern is fragmentation after the boards start to ship. I
recognize this comes under the "lack of planning on your part doesn't
constitute an emergency on my part" heading (although it really wasn't
a lack of planning). Now that this is out here and people can see where
it's going, if we choose to nack these and wait for a better
implementation, that alone may accomplish the same goal.


> Do these boards currently boot any other OSes?


Currently the linux-yocto_3.8 standard/minnow Linux kernel is the only
thing known to boot on it. This is what will ship with the device.


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

What are you referring to with "programming model" here?

The three drivers in question:
minnowboard.c
minnowboard-gpio.c
minnowboard-keys.c

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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 759 bytes --]

On Tue, 2013-06-25 at 22:32 -0700, Darren Hart wrote:

> 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?

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Matthew Garrett]

On Tue, 2013-06-25 at 22:32 -0700, Darren Hart wrote:

> 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?

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 05:36 +0000, Matthew Garrett wrote:
> On Tue, 2013-06-25 at 22:32 -0700, Darren Hart wrote:
> 
> > 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?
> 
> 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.

It does not currently. However, the firmware is being developed along
with the hardware and the OS. We have the opportunity to do whatever
makes the most sense.

My next-steps here were to get a basic ACPI ID setup for each of the
three drivers just so they could be enumerated over ACPI rather than
just loaded manually. Beyond that I'll be looking to Len and Rafael for
guidance.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 05:36 +0000, Matthew Garrett wrote:
> On Tue, 2013-06-25 at 22:32 -0700, Darren Hart wrote:
> 
> > 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?
> 
> 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.

It does not currently. However, the firmware is being developed along
with the hardware and the OS. We have the opportunity to do whatever
makes the most sense.

My next-steps here were to get a basic ACPI ID setup for each of the
three drivers just so they could be enumerated over ACPI rather than
just loaded manually. Beyond that I'll be looking to Len and Rafael for
guidance.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Linus Walleij]

On Wed, Jun 26, 2013 at 3:53 AM, Darren Hart <dvhart@linux.intel.com> wrote:

> 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?

Yours,
Linus Walleij

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Thu, 2013-06-27 at 11:14 +0200, Linus Walleij wrote:
> On Wed, Jun 26, 2013 at 3:53 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> 
> > 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.

I'll fix this for minnow_phy_reset() by moving the reset routine into
the pch_gbe driver and have it request the GPIO, using the pci_id
structure to determine the GPIO line.

I'll drop minnow_lvds_detect() and work toward the firmware managing
this aspect instead.

minnow_detect() doesn't access GPIO.

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?

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

Understood. Since minnow_hwid() is not currently used anywhere anyway,
I'll take Olof's advice and just drop it. We can always add it back if
it becomes necessary.

> 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?

The only one that remains is pch_gbe and we have a PCI Subsystem ID
there that we can use. Once we determine it is a MinnowBoard, we can
look the GPIO up by chip and offset. If this changes in future board
revs, we'll need minnow_hwid(), or we'll have to figure something out
either in the PCI config space or via ACPI as you suggested.

Thanks Linus, appreciate all the feedback.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Linus Walleij]

On Thu, Jul 4, 2013 at 6:26 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:
>
>> 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.

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 {
     };
};

Yours,
Linus Walleij

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On Sat, Jul 20, 2013 at 07:37:54PM +0200, Linus Walleij wrote:
> On Thu, Jul 4, 2013 at 6:26 PM, Mark Brown <broonie@kernel.org> wrote:

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

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

I'd expect us to end up with devices for the modules doing the mapping.
I'm not sure what the accessors you're thinking about would be, this
should hopefully be transparent to devices sitting on the boards
otherwise it seems like there's not that much point really.

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

> board {
>      soc {
>      };
> };

I'm not 100% sure what this means, sorry?  If you're saying it'd be
something that sits within the master board for the system (whatever
that happens to be) then yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Mon, 2013-07-22 at 00:41 +0100, Mark Brown wrote:
> On Sat, Jul 20, 2013 at 07:37:54PM +0200, Linus Walleij wrote:
> > On Thu, Jul 4, 2013 at 6:26 PM, Mark Brown <broonie@kernel.org> wrote:
> 
> > > 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.

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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On Wed, Oct 30, 2013 at 02:18:00PM +0000, Darren Hart wrote:
> On Mon, 2013-07-22 at 00:41 +0100, Mark Brown wrote:

> > 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?

For the identifier space.  We ought to be able to register handling for
plugin boards separately to notifying the system of their existance in a
similar fashion to how we register drivers for anything else.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Grant Likely]

On Thu, Jul 4, 2013 at 5:26 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:
>
>> 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.

g.

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Mon, 2013-07-22 at 01:09 +0100, Grant Likely wrote:
> On Thu, Jul 4, 2013 at 5:26 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:
> >
> >> 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.

Yes, that is the current plan. I have isolated the necessary patches for
bug fixes (merged, serial support (merged) and networking support (under
review). I am exploring different options with ACPI for how to best
handle the GPIO (LEDs and Keys) and the Lure device descriptions.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

[PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

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

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/Kconfig            |  18 +++++
 drivers/platform/x86/Makefile           |   1 +
 drivers/platform/x86/minnowboard-gpio.c | 115 ++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+)
 create mode 100644 drivers/platform/x86/minnowboard-gpio.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 154dbf6..c8755cb 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -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
+	---help---
+	  Export the EG20T (gpio-pch) lines on the expansion connector to sysfs
+	  for easy manipulation from userspace. These will be named
+	  "minnow_gpio_pch[0-7]". If LVDS is not in use, export the E6XX
+	  (gpio-sch) lines on the expansion connector to sysfs, these will be
+	  named "minnow_gpio_aux[0-4]".
+
+	  If you have a MinnowBoard, and want to experiment with the GPIO,
+	  say Y or M here.
+
+endif # MINNOWBOARD
+
+
 config ACER_WMI
 	tristate "Acer WMI Laptop Extras"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 45ede1c..4dac9f0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -3,6 +3,7 @@
 # x86 Platform-Specific Drivers
 #
 obj-$(CONFIG_MINNOWBOARD)	+= minnowboard.o
+obj-$(CONFIG_MINNOWBOARD_GPIO)	+= minnowboard-gpio.o
 obj-$(CONFIG_ASUS_LAPTOP)	+= asus-laptop.o
 obj-$(CONFIG_ASUS_WMI)		+= asus-wmi.o
 obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o
diff --git a/drivers/platform/x86/minnowboard-gpio.c b/drivers/platform/x86/minnowboard-gpio.c
new file mode 100644
index 0000000..c33c438
--- /dev/null
+++ b/drivers/platform/x86/minnowboard-gpio.c
@@ -0,0 +1,115 @@
+/*
+ * 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 <dvhart@linux.intel.com>
+ */
+
+#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>
+#include "minnowboard-gpio.h"
+
+static struct gpio expansion_gpios[] = {
+	{ GPIO_PCH0, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch0" },
+	{ GPIO_PCH1, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch1" },
+	{ GPIO_PCH2, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch2" },
+	{ GPIO_PCH3, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch3" },
+	{ GPIO_PCH4, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch4" },
+	{ GPIO_PCH5, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch5" },
+	{ GPIO_PCH6, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch6" },
+	{ GPIO_PCH7, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch7" },
+};
+
+static struct gpio expansion_aux_gpios[] = {
+	{ GPIO_AUX0, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux0" },
+	{ GPIO_AUX1, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux1" },
+	{ GPIO_AUX2, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux2" },
+	{ GPIO_AUX3, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux3" },
+	{ GPIO_AUX4, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux4" },
+};
+
+static int __init minnow_gpio_module_init(void)
+{
+	int err;
+
+	err = -ENODEV;
+	if (!minnow_detect())
+		goto out;
+
+#ifdef MODULE
+#ifdef CONFIG_MINNOWBOARD_MODULE
+	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;
+	}
+
+ out:
+	return err;
+}
+
+static void __exit minnow_gpio_module_exit(void)
+{
+	if (minnow_lvds_detect())
+		gpio_free_array(expansion_aux_gpios,
+				ARRAY_SIZE(expansion_aux_gpios));
+	gpio_free_array(expansion_gpios, ARRAY_SIZE(expansion_gpios));
+}
+
+module_init(minnow_gpio_module_init);
+module_exit(minnow_gpio_module_exit);
+
+MODULE_LICENSE("GPL");
-- 
1.8.1.2

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Andy Shevchenko]

On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> 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.

> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -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.

> --- /dev/null
> +++ b/drivers/platform/x86/minnowboard-gpio.c

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

> +#include "minnowboard-gpio.h"

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

> +	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;
> +	}
> +
> + 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.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Darren Hart]

On Wed, 2013-06-26 at 10:55 +0300, Andy Shevchenko wrote:
> On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > 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.
> 
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -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.
> 
> > --- /dev/null
> > +++ b/drivers/platform/x86/minnowboard-gpio.c
> 
> > +#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?
> 
> > +#include "minnowboard-gpio.h"
> 
> > +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().
> 

All good to here and consistent with Olof's comments. Thanks, I will
include in V2.

> > +	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;
> > +	}
> > +
> > + 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.

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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Andy Shevchenko]

On Wed, 2013-06-26 at 09:21 -0700, Darren Hart wrote: 
> On Wed, 2013-06-26 at 10:55 +0300, Andy Shevchenko wrote:

> > > + 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.
> 
> 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..."

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Darren Hart]

On Thu, 2013-06-27 at 11:18 +0300, Andy Shevchenko wrote:
> On Wed, 2013-06-26 at 09:21 -0700, Darren Hart wrote: 
> > On Wed, 2013-06-26 at 10:55 +0300, Andy Shevchenko wrote:
> 
> > > > + 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.
> > 
> > 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..."

OK, I'll fix that up in V2 and use that model in the future. Thank you
for being persistent.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

[PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

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

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/Kconfig            |  14 +++++
 drivers/platform/x86/Makefile           |   1 +
 drivers/platform/x86/minnowboard-keys.c | 108 ++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)
 create mode 100644 drivers/platform/x86/minnowboard-keys.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index c8755cb..b9ff98c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -50,6 +50,20 @@ config MINNOWBOARD_GPIO
 	  If you have a MinnowBoard, and want to experiment with the GPIO,
 	  say Y or M here.
 
+config MINNOWBOARD_KEYS
+	tristate "MinnowBoard GPIO Keys"
+	depends on MINNOWBOARD
+	depends on KEYBOARD_GPIO_POLLED
+	default n
+	---help---
+	  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.
+
+	  If you have a MinnowBoard and want to use the buttons as arrow keys,
+	  say Y or M here.
+
 endif # MINNOWBOARD
 
 
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 4dac9f0..2f55903 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -4,6 +4,7 @@
 #
 obj-$(CONFIG_MINNOWBOARD)	+= minnowboard.o
 obj-$(CONFIG_MINNOWBOARD_GPIO)	+= minnowboard-gpio.o
+obj-$(CONFIG_MINNOWBOARD_KEYS)	+= minnowboard-keys.o
 obj-$(CONFIG_ASUS_LAPTOP)	+= asus-laptop.o
 obj-$(CONFIG_ASUS_WMI)		+= asus-wmi.o
 obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o
diff --git a/drivers/platform/x86/minnowboard-keys.c b/drivers/platform/x86/minnowboard-keys.c
new file mode 100644
index 0000000..de96df1
--- /dev/null
+++ b/drivers/platform/x86/minnowboard-keys.c
@@ -0,0 +1,108 @@
+/*
+ * 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 <dvhart@linux.intel.com>
+ */
+
+#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>
+#include "minnowboard-gpio.h"
+
+/* VI-style direction keys seem like as good as anything */
+#define GPIO_BTN0_KEY KEY_LEFT
+#define GPIO_BTN1_KEY KEY_DOWN
+#define GPIO_BTN2_KEY KEY_UP
+#define GPIO_BTN3_KEY KEY_RIGHT
+
+/* Timing in milliseconds */
+#define GPIO_DEBOUNCE 1
+#define BUTTON_POLL_INTERVAL 300
+
+/* gpio-keys platform device structures */
+static struct gpio_keys_button minnow_buttons[] = {
+	{ .code = GPIO_BTN0_KEY, .gpio = GPIO_BTN0, .active_low = 1,
+	  .desc = "minnow_btn0", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+	{ .code = GPIO_BTN1_KEY, .gpio = GPIO_BTN1, .active_low = 1,
+	  .desc = "minnow_btn1", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+	{ .code = GPIO_BTN2_KEY, .gpio = GPIO_BTN2, .active_low = 1,
+	  .desc = "minnow_btn2", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+	{ .code = GPIO_BTN3_KEY, .gpio = GPIO_BTN3, .active_low = 1,
+	  .desc = "minnow_btn3", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+};
+
+static const struct gpio_keys_platform_data minnow_buttons_platform_data = {
+	.buttons = minnow_buttons,
+	.nbuttons = ARRAY_SIZE(minnow_buttons),
+	.poll_interval = BUTTON_POLL_INTERVAL,
+	.rep = 1,
+	.enable = NULL,
+	.disable = NULL,
+	.name = "minnow_buttons",
+};
+
+static struct platform_device minnow_gpio_buttons = {
+	.name = "gpio-keys-polled",
+	.id = -1,
+	.dev = {
+		.platform_data = (void *) &minnow_buttons_platform_data,
+	},
+};
+
+static int __init minnow_keys_module_init(void)
+{
+	int err;
+
+	err = -ENODEV;
+	if (!minnow_detect())
+		goto out;
+
+#ifdef MODULE
+#ifdef CONFIG_MINNOWBOARD_MODULE
+	if (request_module("minnowboard"))
+		goto out;
+#endif
+#endif
+
+	/* Export GPIO buttons to sysfs */
+	err = platform_device_register(&minnow_gpio_buttons);
+	if (err) {
+		pr_err("Failed to register gpio-keys-polled platform device\n");
+		goto out;
+	}
+
+ out:
+	return err;
+}
+
+static void __exit minnow_keys_module_exit(void)
+{
+	platform_device_unregister(&minnow_gpio_buttons);
+}
+
+module_init(minnow_keys_module_init);
+module_exit(minnow_keys_module_exit);
+
+MODULE_LICENSE("GPL");
-- 
1.8.1.2

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Andy Shevchenko]

On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> 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/...


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Darren Hart]

On Wed, 2013-06-26 at 11:46 +0300, Andy Shevchenko wrote:
> On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > 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/...


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.

Does that address your concern or do you still feel they should be
merged?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Greg Kroah-Hartman]

On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:
> On Wed, 2013-06-26 at 11:46 +0300, Andy Shevchenko wrote:
> > On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > > 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/...
> 
> 
> 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.

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

thanks,

greg k-h

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Darren Hart]

On Wed, 2013-06-26 at 10:16 -0700, Greg Kroah-Hartman wrote:
> On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:
> > On Wed, 2013-06-26 at 11:46 +0300, Andy Shevchenko wrote:
> > > On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > > > 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/...
> > 
> > 
> > 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.
> 
> 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.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Linus Walleij]

On Wed, Jun 26, 2013 at 7:23 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> On Wed, 2013-06-26 at 10:16 -0700, Greg Kroah-Hartman wrote:
>> On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:

>> > 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.
>>
>> 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;
                };
        };

Yours,
Linus Walleij

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Darren Hart]

On Wed, 2013-06-26 at 21:57 +0200, Linus Walleij wrote:
> On Wed, Jun 26, 2013 at 7:23 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> > On Wed, 2013-06-26 at 10:16 -0700, Greg Kroah-Hartman wrote:
> >> On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:
> 
> >> > 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.
> >>
> >> 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";
>                 };
>         };

This is something we should be able to do with ACPI 5 and SSDTs.

>         /* 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;
>                 };
>         };

And this too actually. I was thinking along the lines of the tables in
the firmware, but using SSDTs passed in (optionally) at boot, the user
can easily decide not to use the buttons with gpio-keys simply by not
passing in that SSDT. This had been the direction we were going with the
Lures (daughter cards), but we can also apply this to the board itself
as a way to provide a proper example of how to do this.

So the answer seems to be to put any fixed functionality into the board
firmware ACPI tables and leave the optional experimenter config to SSDTs
which the user can choose to use or not.

The rest of what minnowboard.c does (LVDS/GPIO setup) can (and should)
be done in the firmware directly, also eliminating the need for the 3/8
(sch_gpio_resume_set_enable()).

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

[PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[Darren Hart]

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

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: David Anders <danders@circuitco.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-pci@vger.kernel.org
---
 include/linux/pci_ids.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index c129162..b2879ce 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -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
-- 
1.8.1.2

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[Bjorn Helgaas]

On Tue, Jun 25, 2013 at 06:53:27PM -0700, Darren Hart wrote:
> Add CircuitCo's newly created VENDOR ID and their first board subsystem
> ID for the MinnowBoard.
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: David Anders <danders@circuitco.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  include/linux/pci_ids.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index c129162..b2879ce 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -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
> -- 
> 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 <dvhart@linux.intel.com>
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 <dvhart@linux.intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index c129162..8b1aa7f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2476,6 +2476,8 @@
 
 #define PCI_VENDOR_ID_ASMEDIA		0x1b21
 
+#define PCI_VENDOR_ID_CIRCUITCO		0x1cc8
+
 #define PCI_VENDOR_ID_TEKRAM		0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
 

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[Darren Hart]

On Wed, 2013-06-26 at 10:32 -0600, Bjorn Helgaas wrote:
> On Tue, Jun 25, 2013 at 06:53:27PM -0700, Darren Hart wrote:
> > Add CircuitCo's newly created VENDOR ID and their first board subsystem
> > ID for the MinnowBoard.
> > 
> > Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: David Anders <danders@circuitco.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: linux-pci@vger.kernel.org
> > ---
> >  include/linux/pci_ids.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > index c129162..b2879ce 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -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
> > -- 
> > 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 <dvhart@linux.intel.com>
> 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 <dvhart@linux.intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index c129162..8b1aa7f 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2476,6 +2476,8 @@
>  
>  #define PCI_VENDOR_ID_ASMEDIA		0x1b21
>  
> +#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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[Bjorn Helgaas]

On Wed, Jun 26, 2013 at 11:15 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> On Wed, 2013-06-26 at 10:32 -0600, Bjorn Helgaas wrote:

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

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[Darren Hart]

On Wed, 2013-06-26 at 13:37 -0600, Bjorn Helgaas wrote:
> On Wed, Jun 26, 2013 at 11:15 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> > On Wed, 2013-06-26 at 10:32 -0600, Bjorn Helgaas wrote:
> 
> >> +#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


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

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

Would you like me to send another patch?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[Bjorn Helgaas]

On Wed, Jun 26, 2013 at 02:16:56PM -0700, Darren Hart wrote:
> On Wed, 2013-06-26 at 13:37 -0600, Bjorn Helgaas wrote:
> > On Wed, Jun 26, 2013 at 11:15 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> > > On Wed, 2013-06-26 at 10:32 -0600, Bjorn Helgaas wrote:
> > 
> > >> +#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
> 
> 
> I would be happy to change DEVICE to SUBSYSTEM in the the pci_ids.h:
> 
> +#define PCI_VENDOR_ID_CIRCUITCO		0x1cc8
> +#define PCI_SUBSYSTEM_ID_CITCUITCO_MINNOWBOARD	0x0001
> 
> Would you like me to send another patch?

No need.  I'll just apply the following patch.  Obviously your follow-on
pch_gbe patch will need a corresponding change.

Let me know if it needs tweaking, or if you decide I was talking nonsense
above.


commit c2efb42ab6033388f3bbed2dbe0fb76f2402e04a
Author: Darren Hart <dvhart@linux.intel.com>
Date:   Tue Jun 25 20:08:46 2013 -0600

    PCI: Add CircuitCo vendor ID and subsystem ID
    
    Add CircuitCo's newly created VENDOR ID and their first board subsystem
    ID for the MinnowBoard.
    
    [bhelgaas: sort, change DEVICE_ID to SUBSYSTEM_ID]
    Signed-off-by: Darren Hart <dvhart@linux.intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index c129162..4c7b349 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2476,6 +2476,9 @@
 
 #define PCI_VENDOR_ID_ASMEDIA		0x1b21
 
+#define PCI_VENDOR_ID_CIRCUITCO		0x1cc8
+#define PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD	0x0001
+
 #define PCI_VENDOR_ID_TEKRAM		0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
 

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[H. Peter Anvin]

On 06/26/2013 12:37 PM, Bjorn Helgaas wrote:
> 
> 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.
> 

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

	-hpa

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[Bjorn Helgaas]

On Wed, Jun 26, 2013 at 3:30 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 06/26/2013 12:37 PM, Bjorn Helgaas wrote:
>>
>> 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.
>>
>
> 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

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.

Bjorn

Re: [PATCH 7/8] pci: Add CircuitCo VENDOR ID and MinnowBoard DEVICE ID

[H. Peter Anvin]

On 06/26/2013 02:46 PM, Bjorn Helgaas wrote:
>>
>> 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
> 
> 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.
> 

It could be interpreted that literally, yes.  This is part of why a
"subsystem" isn't necessarily forced to be a "board".

	-hpa

[PATCH 8/8] pch_gbe: Add MinnowBoard support

[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
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 <dvhart@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: netdev@vger.kernel.org
---
 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(+)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
index 34d05bf..a93fa6b 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
@@ -5,6 +5,7 @@
 config PCH_GBE
 	tristate "OKI SEMICONDUCTOR IOH(ML7223/ML7831) GbE"
 	depends on PCI
+	depends on !MINNOWBOARD || (MINNOWBOARD=m && m) || MINNOWBOARD=y
 	select NET_CORE
 	select MII
 	select PTP_1588_CLOCK_PCH
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index 7fb7e17..feacf05 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -384,6 +384,7 @@ struct pch_gbe_mac_info {
  * @revision:		PHY's revision
  * @reset_delay_us:	HW reset delay time[us]
  * @autoneg_advertised:	Autoneg advertised
+ * @tx_clk_delay:	Setup TX clock delay in the PHY
  */
 struct pch_gbe_phy_info {
 	u32 addr;
@@ -391,6 +392,7 @@ struct pch_gbe_phy_info {
 	u32 revision;
 	u32 reset_delay_us;
 	u16 autoneg_advertised;
+	int(*tx_clk_delay)(struct pch_gbe_hw *);
 };
 
 /*!
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 6667a6b..6f0b9e3 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/net_tstamp.h>
 #include <linux/ptp_classify.h>
+#include <linux/minnowboard.h>
 
 #define DRV_VERSION     "1.01"
 const char pch_driver_version[] = DRV_VERSION;
@@ -111,6 +112,11 @@ const char pch_driver_version[] = DRV_VERSION;
 #define PTP_L4_MULTICAST_SA "01:00:5e:00:01:81"
 #define PTP_L2_MULTICAST_SA "01:1b:19:00:00:00"
 
+struct pch_gbe_privdata {
+	void(*phy_reset)(void);
+	int(*phy_tx_clk_delay)(struct pch_gbe_hw *hw);
+};
+
 static unsigned int copybreak __read_mostly = PCH_GBE_COPYBREAK_DEFAULT;
 
 static int pch_gbe_mdio_read(struct net_device *netdev, int addr, int reg);
@@ -2559,6 +2565,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 {
 	struct net_device *netdev;
 	struct pch_gbe_adapter *adapter;
+	struct pch_gbe_privdata *pdata = NULL;
 	int ret;
 
 	ret = pci_enable_device(pdev);
@@ -2596,6 +2603,15 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, netdev);
 	adapter = netdev_priv(netdev);
+
+	adapter->hw.phy.tx_clk_delay = NULL;
+	if (pci_id->driver_data) {
+		pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
+		adapter->hw.phy.tx_clk_delay = pdata->phy_tx_clk_delay;
+		if (pdata->phy_reset)
+			pdata->phy_reset();
+	}
+
 	adapter->netdev = netdev;
 	adapter->pdev = pdev;
 	adapter->hw.back = adapter;
@@ -2679,6 +2695,9 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 
 	dev_dbg(&pdev->dev, "PCH Network Connection\n");
 
+	/* Disable hibernation on certain PHYs */
+	pch_gbe_phy_disable_hibernate(&adapter->hw);
+
 	device_set_wakeup_enable(&pdev->dev, 1);
 	return 0;
 
@@ -2697,9 +2716,22 @@ err_disable_device:
 	return ret;
 }
 
+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,
+	 .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),
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
index 28bb960..ab0e0e7 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
@@ -20,6 +20,7 @@
 
 #include "pch_gbe.h"
 #include "pch_gbe_phy.h"
+#include <linux/gpio.h>
 
 #define PHY_MAX_REG_ADDRESS   0x1F	/* 5 bit address bus (0-0x1F) */
 
@@ -74,6 +75,15 @@
 #define MII_SR_100X_FD_CAPS      0x4000	/* 100X  Full Duplex Capable */
 #define MII_SR_100T4_CAPS        0x8000	/* 100T4 Capable */
 
+/* AR8031 PHY Debug Registers */
+#define PHY_AR803X_ID           0x00001374
+#define PHY_AR8031_DBG_OFF      0x1D
+#define PHY_AR8031_DBG_DAT      0x1E
+#define PHY_AR8031_SERDES       0x05
+#define PHY_AR8031_HIBERNATE    0x0B
+#define PHY_AR8031_SERDES_TX_CLK_DLY   0x0100 /* TX clock delay of 2.0ns */
+#define PHY_AR8031_PS_HIB_EN           0x8000 /* Hibernate enable */
+
 /* Phy Id Register (word 2) */
 #define PHY_REVISION_MASK        0x000F
 
@@ -271,4 +281,83 @@ void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw)
 	mii_reg |= PHYSP_CTRL_ASSERT_CRS_TX;
 	pch_gbe_phy_write_reg_miic(hw, PHY_PHYSP_CONTROL, mii_reg);
 
+	 /* Setup a TX clock delay for certain boards */
+	 if (hw->phy.tx_clk_delay)
+		hw->phy.tx_clk_delay(hw);
+}
+
+/**
+ * pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
+ * @hw:	            Pointer to the HW structure
+ * Returns
+ *	0:		Successful.
+ *	-EINVAL:	Invalid argument.
+ */
+int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw)
+{
+	/*
+	 * The RGMII interface requires a ~2ns TX clock delay. This is typically
+	 * done in layout with a longer trace or via PHY strapping, but can also
+	 * be done via PHY configuration registers.
+	 */
+	u16 mii_reg;
+	int ret = 0;
+
+	switch (hw->phy.id) {
+	case PHY_AR803X_ID:
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_OFF, &mii_reg);
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_OFF,
+						 PHY_AR8031_SERDES);
+		if (ret)
+			break;
+
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_DAT, &mii_reg);
+		mii_reg |= PHY_AR8031_SERDES_TX_CLK_DLY;
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_DAT,
+						 mii_reg);
+		break;
+	default:
+		pr_err("Unknown PHY (%x), could not set TX clock delay.\n",
+		       hw->phy.id);
+		return -EINVAL;
+	}
+
+	if (ret)
+		pr_err("Could not configure tx clock delay for PHY.\n");
+	return ret;
+}
+
+/**
+ * pch_gbe_phy_disable_hibernate - Disable the PHY low power state
+ * @hw:	            Pointer to the HW structure
+ * Returns
+ *	0:		Successful.
+ *	-EINVAL:	Invalid argument.
+ */
+int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw)
+{
+	u16 mii_reg;
+	int ret = 0;
+
+	switch (hw->phy.id) {
+	case PHY_AR803X_ID:
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_OFF,
+						 PHY_AR8031_HIBERNATE);
+		if (ret)
+			break;
+
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_DAT, &mii_reg);
+		mii_reg &= ~PHY_AR8031_PS_HIB_EN;
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_DAT,
+						 mii_reg);
+		break;
+	default:
+		pr_err("Unknown PHY (%x), could not disable hibernation\n",
+		       hw->phy.id);
+		return -EINVAL;
+	}
+
+	if (ret)
+		pr_err("Could not disable PHY hibernation.\n");
+	return ret;
 }
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
index 03264dc..e3e4bc9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
@@ -33,5 +33,7 @@ void pch_gbe_phy_power_up(struct pch_gbe_hw *hw);
 void pch_gbe_phy_power_down(struct pch_gbe_hw *hw);
 void pch_gbe_phy_set_rgmii(struct pch_gbe_hw *hw);
 void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw);
+int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw);
+int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw);
 
 #endif /* _PCH_GBE_PHY_H_ */
-- 
1.8.1.2

Re: [PATCH 8/8] pch_gbe: Add MinnowBoard support

[Bjorn Helgaas]

On Tue, Jun 25, 2013 at 7:53 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> 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 <dvhart@linux.intel.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: netdev@vger.kernel.org
> ---
>  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(+)
> ...

> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> index 6667a6b..6f0b9e3 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>...
> +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).

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

Re: [PATCH 8/8] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Tue, 2013-06-25 at 20:35 -0600, Bjorn Helgaas wrote:
> On Tue, Jun 25, 2013 at 7:53 PM, Darren Hart <dvhart@linux.intel.com> wrote:

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

The firmware populates the subsystem vendor and device ID for every
device on the PCH with PCI_VENDOR_ID_CIRCUITCO and
PCI_DEVICE_ID_CIRCUITCO_MINNOWBOARD, respectively. Currently I have
only made changes to pch_gbe to make use of it, but there is the
possibility of using this ID elsewhere. I don't currently have plans to
do so however. So if it is preferable, I can leave
PCI_VENDOR_ID_CIRCUITCO in pci_ids.h and use 0x0001 here for the
subsystem device ID.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 8/8] pch_gbe: Add MinnowBoard support

[H. Peter Anvin]

The same subsystem (not subdevice!) ID is used for all devices on the Minnowboard itself.

Bjorn Helgaas <bhelgaas@google.com> wrote:

>On Tue, Jun 25, 2013 at 7:53 PM, Darren Hart <dvhart@linux.intel.com>
>wrote:
>> 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 <dvhart@linux.intel.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: netdev@vger.kernel.org
>> ---
>>  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(+)
>> ...
>
>> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>> index 6667a6b..6f0b9e3 100644
>> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
>>...
>> +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).
>
>> +        .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),

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH 8/8] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote:
> 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.
> 


Per the discussion on the minnowboard platform drivers in this series
(which provide the minnow_phy_reset() function), I will be rewriting
this patch to provide its own phy_physical_reset() or similar which will
look like the other two phy functions I've added and will take a GPIO
line as an argument, determined by pci_id.

Please disregard this patch for now.

Thanks,


-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel