[v3,3/3] mfd: lpc_ich: Add support for Intel Apollo Lake SoC

Hello Mika,

In line 1239 you specify "struct pci_bus *bus = dev->bus;"
}, @@ -1122,6 +1131,36 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 		}
 		break;
 
+	case INTEL_SPI_BXT: {
+		unsigned int p2sb = PCI_DEVFN(13, 0);
+		unsigned int spi = PCI_DEVFN(13, 2);
+		struct pci_bus *bus = dev->bus;

but in line 1262 you are using dev->bus again. 
+
+		pci_bus_write_config_byte(dev->bus, p2sb, 0xe1, 0x1);

Sure this is not a runtime issue, but it would be nice to keep the coding-style consistent. 
We could just use dev->bus for BXT SOCs, like the remaining parts of this driver do. 

Kind regards
Matthias Kraemer

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Mika Westerberg
Sent: Montag, 15. August 2016 09:11
To: linux-mtd@lists.infradead.org
Cc: Brian Norris; David Woodhouse; Lee Jones; Peter Tyser; key.seong.lim@intel.com; Mika Westerberg; linux-kernel@vger.kernel.org
Subject: [PATCH v3 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake SoC

Intel Apollo Lake SoC exposes serial SPI flash through the LPC device. The SPI flash host controller is not discoverable through PCI config cycles because P2SB (function 0 of the device 13) is hidden by the BIOS. We unhide the device briefly in order to read BAR 0 of the SPI host controller.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mfd/lpc_ich.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

 #define BCR			0xdc
 #define BCR_WPD			BIT(0)
 
+#define SPIBASE_APL_SZ		4096
+
 #define GPIOBASE_ICH0		0x58
 #define GPIOCTRL_ICH0		0x5C
 #define GPIOBASE_ICH6		0x48
@@ -239,6 +242,7 @@ enum lpc_chipsets {
 	LPC_BRASWELL,	/* Braswell SoC */
 	LPC_LEWISBURG,	/* Lewisburg */
 	LPC_9S,		/* 9 Series */
+	LPC_APL,	/* Apollo Lake SoC */
 };
 
 static struct lpc_ich_info lpc_chipset_info[] = { @@ -559,6 +563,10 @@ static struct lpc_ich_info lpc_chipset_info[] = {
 		.name = "9 Series",
 		.iTCO_version = 2,
 	},
+	[LPC_APL] = {
+		.name = "Apollo Lake SoC",
+		.spi_type = INTEL_SPI_BXT,
+	},
 };
 
 /*
@@ -707,6 +715,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
 	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
 	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
+	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
 	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
 	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
 	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT}, @@ -1122,6 +1131,36 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 		}
 		break;
 
+	case INTEL_SPI_BXT: {
+		unsigned int p2sb = PCI_DEVFN(13, 0);
+		unsigned int spi = PCI_DEVFN(13, 2);
+		struct pci_bus *bus = dev->bus;
+
+		/*
+		 * The P2SB is hidden by BIOS and we need to unhide it in
+		 * order to read BAR of the SPI flash device. Once that is
+		 * done we hide it again.
+		 */
+		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x0);
+		pci_bus_read_config_dword(bus, spi, PCI_BASE_ADDRESS_0,
+					  &spi_base);
+		if (spi_base != ~0) {
+			res->start = spi_base & 0xfffffff0;
+			res->end = res->start + SPIBASE_APL_SZ - 1;
+
+			pci_bus_read_config_dword(bus, spi, BCR, &bcr);
+			if (!(bcr & BCR_WPD)) {
+				bcr |= BCR_WPD;
+				pci_bus_write_config_dword(bus, spi, BCR, bcr);
+				pci_bus_read_config_dword(bus, spi, BCR, &bcr);
+			}
+			info->writeable = !!(bcr & BCR_WPD);
+		}
+
+		pci_bus_write_config_byte(dev->bus, p2sb, 0xe1, 0x1);
+		break;
+	}
+
 	default:
 		return -EINVAL;
 	}
--
2.8.1

On Thu, Sep 29, 2016 at 01:19:30PM +0000, Kraemer, Matthias (Ferchau; ADITG/SW1) wrote:
> Hello Mika,
> 
> In line 1239 you specify "struct pci_bus *bus = dev->bus;"
> }, @@ -1122,6 +1131,36 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>  		}
>  		break;
>  
> +	case INTEL_SPI_BXT: {
> +		unsigned int p2sb = PCI_DEVFN(13, 0);
> +		unsigned int spi = PCI_DEVFN(13, 2);
> +		struct pci_bus *bus = dev->bus;
> 
> but in line 1262 you are using dev->bus again. 
> +
> +		pci_bus_write_config_byte(dev->bus, p2sb, 0xe1, 0x1);

Good point. It is a typo, although works.

> Sure this is not a runtime issue, but it would be nice to keep the
> coding-style consistent.  We could just use dev->bus for BXT SOCs,
> like the remaining parts of this driver do. 

Or "bus". But yeah I agree it should be consistent.

Since it looks like this driver is not going to make it for v4.9, I'll
fix this up and resend the whole thing once v4.9-rc1 is released.

On Thu, 29 Sep 2016, Kraemer, Matthias (Ferchau; ADITG/SW1) wrote:

> Hello Mika,
> 
> In line 1239 you specify "struct pci_bus *bus = dev->bus;"
> }, @@ -1122,6 +1131,36 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>  		}
>  		break;
>  
> +	case INTEL_SPI_BXT: {
> +		unsigned int p2sb = PCI_DEVFN(13, 0);
> +		unsigned int spi = PCI_DEVFN(13, 2);
> +		struct pci_bus *bus = dev->bus;
> 
> but in line 1262 you are using dev->bus again. 
> +
> +		pci_bus_write_config_byte(dev->bus, p2sb, 0xe1, 0x1);
> 
> Sure this is not a runtime issue, but it would be nice to keep the coding-style consistent. 
> We could just use dev->bus for BXT SOCs, like the remaining parts of this driver do. 
> 
> Kind regards
> Matthias Kraemer

Please reply in-line.  It is considered bad etiquette to top-post like this.

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Mika Westerberg
> Sent: Montag, 15. August 2016 09:11
> To: linux-mtd@lists.infradead.org
> Cc: Brian Norris; David Woodhouse; Lee Jones; Peter Tyser; key.seong.lim@intel.com; Mika Westerberg; linux-kernel@vger.kernel.org
> Subject: [PATCH v3 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake SoC

Please fix your mailer.  This is crud and should not be quoted.