Re: [PATCH 2/3] mfd: lpc_ich: Add support for SPI serial flash host controller

[Lee Jones <lee.jones@linaro.org>]

On Tue, 14 Jun 2016, Mika Westerberg wrote:

> Many Intel CPUs including Haswell, Broadwell and Baytrail have SPI serial
> flash host controller as part of the LPC device. This will populate an MFD
> cell suitable for the SPI host controller driver if we know that the LPC
> device has one.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/mfd/lpc_ich.c       | 97 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/lpc_ich.h |  3 ++
>  2 files changed, 100 insertions(+)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index bd3aa4578346..39b7731d769a 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -83,6 +83,13 @@
>  #define ACPIBASE_GCS_OFF	0x3410
>  #define ACPIBASE_GCS_END	0x3414
>  
> +#define SPIBASE_BYT		0x54
> +#define SPIBASE_BYT_EN		BIT(1)
> +
> +#define SPIBASE_LPT		0x3800
> +#define BCR			0xdc
> +#define BCR_WPD			BIT(0)
> +
>  #define GPIOBASE_ICH0		0x58
>  #define GPIOCTRL_ICH0		0x5C
>  #define GPIOBASE_ICH6		0x48
> @@ -133,6 +140,12 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> +static struct resource intel_spi_res[] = {
> +	{
> +		.flags = IORESOURCE_MEM,
> +	}
> +};
> +
>  static struct mfd_cell lpc_ich_wdt_cell = {
>  	.name = "iTCO_wdt",
>  	.num_resources = ARRAY_SIZE(wdt_ich_res),
> @@ -147,6 +160,14 @@ static struct mfd_cell lpc_ich_gpio_cell = {
>  	.ignore_resource_conflicts = true,
>  };
>  
> +
> +static struct mfd_cell lpc_ich_spi_cell = {
> +	.name = "intel-spi",
> +	.num_resources = ARRAY_SIZE(intel_spi_res),
> +	.resources = intel_spi_res,
> +	.ignore_resource_conflicts = true,
> +};
> +
>  /* chipset related info */
>  enum lpc_chipsets {
>  	LPC_ICH = 0,	/* ICH */
> @@ -493,10 +514,12 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  	[LPC_LPT] = {
>  		.name = "Lynx Point",
>  		.iTCO_version = 2,
> +		.spi_type = INTEL_SPI_LPT,
>  	},
>  	[LPC_LPT_LP] = {
>  		.name = "Lynx Point_LP",
>  		.iTCO_version = 2,
> +		.spi_type = INTEL_SPI_LPT,
>  	},
>  	[LPC_WBG] = {
>  		.name = "Wellsburg",
> @@ -510,6 +533,7 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  	[LPC_BAYTRAIL] = {
>  		.name = "Bay Trail SoC",
>  		.iTCO_version = 3,
> +		.spi_type = INTEL_SPI_BYT,
>  	},
>  	[LPC_COLETO] = {
>  		.name = "Coleto Creek",
> @@ -518,10 +542,12 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  	[LPC_WPT_LP] = {
>  		.name = "Wildcat Point_LP",
>  		.iTCO_version = 2,
> +		.spi_type = INTEL_SPI_LPT,
>  	},
>  	[LPC_BRASWELL] = {
>  		.name = "Braswell SoC",
>  		.iTCO_version = 3,
> +		.spi_type = INTEL_SPI_BYT,
>  	},
>  	[LPC_LEWISBURG] = {
>  		.name = "Lewisburg",
> @@ -875,6 +901,15 @@ static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
>  	cell->pdata_size = sizeof(struct lpc_ich_info);
>  }
>  
> +static void lpc_ich_finalize_spi_cell(struct pci_dev *dev,
> +				     struct intel_spi_boardinfo *info)
> +{
> +	struct mfd_cell *cell = &lpc_ich_spi_cell;
> +
> +	cell->platform_data = info;
> +	cell->pdata_size = sizeof(*info);
> +}

This call doesn't appear to offer anything.  In fact, it looks like it
adds more lines than is required.

>  /*
>   * We don't check for resource conflict globally. There are 2 or 3 independent
>   * GPIO groups and it's enough to have access to one of these to instantiate
> @@ -1050,6 +1085,62 @@ wdt_done:
>  	return ret;
>  }
>  
> +static int lpc_ich_init_spi(struct pci_dev *dev)
> +{
> +	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +	struct resource *res = &intel_spi_res[0];
> +	struct intel_spi_boardinfo *info;
> +	u32 spi_base, rcba, bcr;
> +
> +	info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->type = lpc_chipset_info[priv->chipset].spi_type;
> +
> +	switch (lpc_chipset_info[priv->chipset].spi_type) {

The type now exists in info->type.  I suggest you use that here
instead.

> +	case INTEL_SPI_BYT:
> +		pci_read_config_dword(dev, SPIBASE_BYT, &spi_base);
> +		if (spi_base & SPIBASE_BYT_EN) {
> +			res->start = spi_base &= 0xfffffe00;
> +			res->end = res->start + 512 - 1;

Define all of these magic numbers.

> +		}
> +		break;
> +
> +	case INTEL_SPI_LPT:
> +		pci_read_config_dword(dev, RCBABASE, &rcba);
> +		if (rcba & 1) {
> +			spi_base = rcba & 0xfffffe00;
> +			res->start = spi_base + SPIBASE_LPT;
> +			res->end = res->start + 512 - 1;

And here.

> +			/*
> +			 * Try to make the flash chip writeable now by
> +			 * setting BCR_WPD. It it fails we tell the driver
> +			 * that it can only read the chip.
> +			 */
> +			pci_read_config_dword(dev, BCR, &bcr);
> +			if (!(bcr & BCR_WPD)) {
> +				bcr |= BCR_WPD;
> +				pci_write_config_dword(dev, BCR, bcr);
> +				pci_read_config_dword(dev, BCR, &bcr);
> +			}
> +			info->writeable = !!(bcr & BCR_WPD);

I'd prefer if you didn't do that here.  In fact, is there any
technical reason why you can't move the entirety of this function into
the SPI NOR driver?

> +		}
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!res->start)
> +		return -ENODEV;
> +
> +	lpc_ich_finalize_spi_cell(dev, info);
> +	return mfd_add_devices(&dev->dev, -1, &lpc_ich_spi_cell, 1, NULL, 0,

Use the includes provided.  Look at other drivers for examples.

> +			       NULL);
> +}
> +
>  static int lpc_ich_probe(struct pci_dev *dev,
>  				const struct pci_device_id *id)
>  {
> @@ -1093,6 +1184,12 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  			cell_added = true;
>  	}
>  
> +	if (lpc_chipset_info[priv->chipset].spi_type) {
> +		ret = lpc_ich_init_spi(dev);
> +		if (!ret)
> +			cell_added = true;
> +	}
> +
>  	/*
>  	 * We only care if at least one or none of the cells registered
>  	 * successfully.
> diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
> index 2b300b44f994..fba8fcb54f8c 100644
> --- a/include/linux/mfd/lpc_ich.h
> +++ b/include/linux/mfd/lpc_ich.h
> @@ -20,6 +20,8 @@
>  #ifndef LPC_ICH_H
>  #define LPC_ICH_H
>  
> +#include <linux/platform_data/intel-spi.h>
> +
>  /* GPIO resources */
>  #define ICH_RES_GPIO	0
>  #define ICH_RES_GPE0	1
> @@ -40,6 +42,7 @@ struct lpc_ich_info {
>  	char name[32];
>  	unsigned int iTCO_version;
>  	unsigned int gpio_version;
> +	enum intel_spi_type spi_type;
>  	u8 use_gpio;
>  };
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog