[PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types

[PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types

[Jarkko Nikula]

Intel LPSS SPI properties differ between between platforms. Now private
registers offset 0x400 or 0x800 is autodetected but there is need to
support also other offset and handle a few other differences.

Prepare for that by splitting the LPSS_SSP type into compatible hardware
types and set it now based on PCI or ACPI ID. That type will be used to set
properties that differ between current and upcoming platforms.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spi-pxa2xx-pci.c |  8 ++++----
 drivers/spi/spi-pxa2xx.c     | 44 ++++++++++++++++++++++++--------------------
 include/linux/pxa2xx_ssp.h   |  3 ++-
 3 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index fa7399e84bbb..3cfd4357489a 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -62,7 +62,7 @@ static struct pxa_spi_info spi_info_configs[] = {
 		.max_clk_rate = 3686400,
 	},
 	[PORT_BYT] = {
-		.type = LPSS_SSP,
+		.type = LPSS_BYT_SSP,
 		.port_id = 0,
 		.num_chipselect = 1,
 		.max_clk_rate = 50000000,
@@ -70,7 +70,7 @@ static struct pxa_spi_info spi_info_configs[] = {
 		.rx_param = &byt_rx_param,
 	},
 	[PORT_BSW0] = {
-		.type = LPSS_SSP,
+		.type = LPSS_BYT_SSP,
 		.port_id = 0,
 		.num_chipselect = 1,
 		.max_clk_rate = 50000000,
@@ -78,7 +78,7 @@ static struct pxa_spi_info spi_info_configs[] = {
 		.rx_param = &bsw0_rx_param,
 	},
 	[PORT_BSW1] = {
-		.type = LPSS_SSP,
+		.type = LPSS_BYT_SSP,
 		.port_id = 1,
 		.num_chipselect = 1,
 		.max_clk_rate = 50000000,
@@ -86,7 +86,7 @@ static struct pxa_spi_info spi_info_configs[] = {
 		.rx_param = &bsw1_rx_param,
 	},
 	[PORT_BSW2] = {
-		.type = LPSS_SSP,
+		.type = LPSS_BYT_SSP,
 		.port_id = 2,
 		.num_chipselect = 1,
 		.max_clk_rate = 50000000,
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index e3223ac75a7c..13ec36b1c9aa 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -74,7 +74,8 @@ MODULE_ALIAS("platform:pxa2xx-spi");
 
 static bool is_lpss_ssp(const struct driver_data *drv_data)
 {
-	return drv_data->ssp_type == LPSS_SSP;
+	return drv_data->ssp_type == LPSS_LPT_SSP ||
+	       drv_data->ssp_type == LPSS_BYT_SSP;
 }
 
 static bool is_quark_x1000_ssp(const struct driver_data *drv_data)
@@ -1079,22 +1080,18 @@ static int setup(struct spi_device *spi)
 	unsigned int clk_div;
 	uint tx_thres, tx_hi_thres, rx_thres;
 
-	switch (drv_data->ssp_type) {
-	case QUARK_X1000_SSP:
+	if (is_quark_x1000_ssp(drv_data)) {
 		tx_thres = TX_THRESH_QUARK_X1000_DFLT;
 		tx_hi_thres = 0;
 		rx_thres = RX_THRESH_QUARK_X1000_DFLT;
-		break;
-	case LPSS_SSP:
+	} else if (is_lpss_ssp(drv_data)) {
 		tx_thres = LPSS_TX_LOTHRESH_DFLT;
 		tx_hi_thres = LPSS_TX_HITHRESH_DFLT;
 		rx_thres = LPSS_RX_THRESH_DFLT;
-		break;
-	default:
+	} else {
 		tx_thres = TX_THRESH_DFLT;
 		tx_hi_thres = 0;
 		rx_thres = RX_THRESH_DFLT;
-		break;
 	}
 
 	/* Only alloc on first setup */
@@ -1242,6 +1239,18 @@ static void cleanup(struct spi_device *spi)
 }
 
 #ifdef CONFIG_ACPI
+
+static struct acpi_device_id pxa2xx_spi_acpi_match[] = {
+	{ "INT33C0", LPSS_LPT_SSP },
+	{ "INT33C1", LPSS_LPT_SSP },
+	{ "INT3430", LPSS_LPT_SSP },
+	{ "INT3431", LPSS_LPT_SSP },
+	{ "80860F0E", LPSS_BYT_SSP },
+	{ "8086228E", LPSS_BYT_SSP },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
+
 static struct pxa2xx_spi_master *
 pxa2xx_spi_acpi_get_pdata(struct platform_device *pdev)
 {
@@ -1249,12 +1258,17 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device *pdev)
 	struct acpi_device *adev;
 	struct ssp_device *ssp;
 	struct resource *res;
-	int devid;
+	const struct acpi_device_id *id;
+	int devid, type = SSP_UNDEFINED;
 
 	if (!ACPI_HANDLE(&pdev->dev) ||
 	    acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
 		return NULL;
 
+	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+	if (id)
+		type = (int)id->driver_data;
+
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return NULL;
@@ -1272,7 +1286,7 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device *pdev)
 
 	ssp->clk = devm_clk_get(&pdev->dev, NULL);
 	ssp->irq = platform_get_irq(pdev, 0);
-	ssp->type = LPSS_SSP;
+	ssp->type = type;
 	ssp->pdev = pdev;
 
 	ssp->port_id = -1;
@@ -1285,16 +1299,6 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device *pdev)
 	return pdata;
 }
 
-static struct acpi_device_id pxa2xx_spi_acpi_match[] = {
-	{ "INT33C0", 0 },
-	{ "INT33C1", 0 },
-	{ "INT3430", 0 },
-	{ "INT3431", 0 },
-	{ "80860F0E", 0 },
-	{ "8086228E", 0 },
-	{ },
-};
-MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
 #else
 static inline struct pxa2xx_spi_master *
 pxa2xx_spi_acpi_get_pdata(struct platform_device *pdev)
diff --git a/include/linux/pxa2xx_ssp.h b/include/linux/pxa2xx_ssp.h
index dab545bb66b3..95a4b3bd7a5c 100644
--- a/include/linux/pxa2xx_ssp.h
+++ b/include/linux/pxa2xx_ssp.h
@@ -194,8 +194,9 @@ enum pxa_ssp_type {
 	PXA168_SSP,
 	PXA910_SSP,
 	CE4100_SSP,
-	LPSS_SSP,
 	QUARK_X1000_SSP,
+	LPSS_LPT_SSP,
+	LPSS_BYT_SSP,
 };
 
 struct ssp_device {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] spi: pxa2xx: Prepare for new Intel LPSS SPI type

[Jarkko Nikula]

Some of the Intel LPSS SPI properties will be different in upcoming
platforms compared to existing Lynxpoint and BayTrail/Braswell. LPSS SPI
private registers will be at different offset and there will be changes in
individual registers and default FIFO thresholds too.

Add configuration for these differences and use them in runtime based on
LPSS SSP type. With this change private registers offset autodetection
becomes needless.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spi-pxa2xx.c   | 107 +++++++++++++++++++++++++--------------------
 include/linux/pxa2xx_ssp.h |   2 +-
 2 files changed, 60 insertions(+), 49 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 13ec36b1c9aa..174b88e44987 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -60,18 +60,51 @@ MODULE_ALIAS("platform:pxa2xx-spi");
 				| QUARK_X1000_SSCR1_TFT		\
 				| SSCR1_SPH | SSCR1_SPO | SSCR1_LBM)
 
-#define LPSS_RX_THRESH_DFLT	64
-#define LPSS_TX_LOTHRESH_DFLT	160
-#define LPSS_TX_HITHRESH_DFLT	224
-
-/* Offset from drv_data->lpss_base */
-#define GENERAL_REG		0x08
 #define GENERAL_REG_RXTO_HOLDOFF_DISABLE BIT(24)
-#define SSP_REG			0x0c
-#define SPI_CS_CONTROL		0x18
 #define SPI_CS_CONTROL_SW_MODE	BIT(0)
 #define SPI_CS_CONTROL_CS_HIGH	BIT(1)
 
+struct lpss_config {
+	/* LPSS offset from drv_data->ioaddr */
+	unsigned offset;
+	/* Register offsets from drv_data->lpss_base or -1 */
+	int reg_general;
+	int reg_ssp;
+	int reg_cs_ctrl;
+	/* FIFO thresholds */
+	u32 rx_threshold;
+	u32 tx_threshold_lo;
+	u32 tx_threshold_hi;
+};
+
+/* Keep these sorted with enum pxa_ssp_type */
+static const struct lpss_config lpss_platforms[] = {
+	{	/* LPSS_LPT_SSP */
+		.offset = 0x800,
+		.reg_general = 0x08,
+		.reg_ssp = 0x0c,
+		.reg_cs_ctrl = 0x18,
+		.rx_threshold = 64,
+		.tx_threshold_lo = 160,
+		.tx_threshold_hi = 224,
+	},
+	{	/* LPSS_BYT_SSP */
+		.offset = 0x400,
+		.reg_general = 0x08,
+		.reg_ssp = 0x0c,
+		.reg_cs_ctrl = 0x18,
+		.rx_threshold = 64,
+		.tx_threshold_lo = 160,
+		.tx_threshold_hi = 224,
+	},
+};
+
+static inline const struct lpss_config
+*lpss_get_config(const struct driver_data *drv_data)
+{
+	return &lpss_platforms[drv_data->ssp_type - LPSS_LPT_SSP];
+}
+
 static bool is_lpss_ssp(const struct driver_data *drv_data)
 {
 	return drv_data->ssp_type == LPSS_LPT_SSP ||
@@ -193,63 +226,39 @@ static void __lpss_ssp_write_priv(struct driver_data *drv_data,
  */
 static void lpss_ssp_setup(struct driver_data *drv_data)
 {
-	unsigned offset = 0x400;
-	u32 value, orig;
-
-	/*
-	 * Perform auto-detection of the LPSS SSP private registers. They
-	 * can be either at 1k or 2k offset from the base address.
-	 */
-	orig = readl(drv_data->ioaddr + offset + SPI_CS_CONTROL);
-
-	/* Test SPI_CS_CONTROL_SW_MODE bit enabling */
-	value = orig | SPI_CS_CONTROL_SW_MODE;
-	writel(value, drv_data->ioaddr + offset + SPI_CS_CONTROL);
-	value = readl(drv_data->ioaddr + offset + SPI_CS_CONTROL);
-	if (value != (orig | SPI_CS_CONTROL_SW_MODE)) {
-		offset = 0x800;
-		goto detection_done;
-	}
-
-	orig = readl(drv_data->ioaddr + offset + SPI_CS_CONTROL);
-
-	/* Test SPI_CS_CONTROL_SW_MODE bit disabling */
-	value = orig & ~SPI_CS_CONTROL_SW_MODE;
-	writel(value, drv_data->ioaddr + offset + SPI_CS_CONTROL);
-	value = readl(drv_data->ioaddr + offset + SPI_CS_CONTROL);
-	if (value != (orig & ~SPI_CS_CONTROL_SW_MODE)) {
-		offset = 0x800;
-		goto detection_done;
-	}
+	const struct lpss_config *config;
+	u32 value;
 
-detection_done:
-	/* Now set the LPSS base */
-	drv_data->lpss_base = drv_data->ioaddr + offset;
+	config = lpss_get_config(drv_data);
+	drv_data->lpss_base = drv_data->ioaddr + config->offset;
 
 	/* Enable software chip select control */
 	value = SPI_CS_CONTROL_SW_MODE | SPI_CS_CONTROL_CS_HIGH;
-	__lpss_ssp_write_priv(drv_data, SPI_CS_CONTROL, value);
+	__lpss_ssp_write_priv(drv_data, config->reg_cs_ctrl, value);
 
 	/* Enable multiblock DMA transfers */
 	if (drv_data->master_info->enable_dma) {
-		__lpss_ssp_write_priv(drv_data, SSP_REG, 1);
+		__lpss_ssp_write_priv(drv_data, config->reg_ssp, 1);
 
-		value = __lpss_ssp_read_priv(drv_data, GENERAL_REG);
+		value = __lpss_ssp_read_priv(drv_data, config->reg_general);
 		value |= GENERAL_REG_RXTO_HOLDOFF_DISABLE;
-		__lpss_ssp_write_priv(drv_data, GENERAL_REG, value);
+		__lpss_ssp_write_priv(drv_data, config->reg_general, value);
 	}
 }
 
 static void lpss_ssp_cs_control(struct driver_data *drv_data, bool enable)
 {
+	const struct lpss_config *config;
 	u32 value;
 
-	value = __lpss_ssp_read_priv(drv_data, SPI_CS_CONTROL);
+	config = lpss_get_config(drv_data);
+
+	value = __lpss_ssp_read_priv(drv_data, config->reg_cs_ctrl);
 	if (enable)
 		value &= ~SPI_CS_CONTROL_CS_HIGH;
 	else
 		value |= SPI_CS_CONTROL_CS_HIGH;
-	__lpss_ssp_write_priv(drv_data, SPI_CS_CONTROL, value);
+	__lpss_ssp_write_priv(drv_data, config->reg_cs_ctrl, value);
 }
 
 static void cs_assert(struct driver_data *drv_data)
@@ -1076,6 +1085,7 @@ static int setup(struct spi_device *spi)
 {
 	struct pxa2xx_spi_chip *chip_info = NULL;
 	struct chip_data *chip;
+	const struct lpss_config *config;
 	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
 	unsigned int clk_div;
 	uint tx_thres, tx_hi_thres, rx_thres;
@@ -1085,9 +1095,10 @@ static int setup(struct spi_device *spi)
 		tx_hi_thres = 0;
 		rx_thres = RX_THRESH_QUARK_X1000_DFLT;
 	} else if (is_lpss_ssp(drv_data)) {
-		tx_thres = LPSS_TX_LOTHRESH_DFLT;
-		tx_hi_thres = LPSS_TX_HITHRESH_DFLT;
-		rx_thres = LPSS_RX_THRESH_DFLT;
+		config = lpss_get_config(drv_data);
+		tx_thres = config->tx_threshold_lo;
+		tx_hi_thres = config->tx_threshold_hi;
+		rx_thres = config->rx_threshold;
 	} else {
 		tx_thres = TX_THRESH_DFLT;
 		tx_hi_thres = 0;
diff --git a/include/linux/pxa2xx_ssp.h b/include/linux/pxa2xx_ssp.h
index 95a4b3bd7a5c..0485bab061fd 100644
--- a/include/linux/pxa2xx_ssp.h
+++ b/include/linux/pxa2xx_ssp.h
@@ -195,7 +195,7 @@ enum pxa_ssp_type {
 	PXA910_SSP,
 	CE4100_SSP,
 	QUARK_X1000_SSP,
-	LPSS_LPT_SSP,
+	LPSS_LPT_SSP, /* Keep LPSS types sorted with lpss_platforms[] */
 	LPSS_BYT_SSP,
 };
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] spi: pxa2xx: Make LPSS SPI general register optional

[Jarkko Nikula]

General register located in LPSS SPI private register space is not found in
upcoming Intel LPSS platforms. Access it conditionally depending is it
defined in configuration.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spi-pxa2xx.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 174b88e44987..28d22206d6c8 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -240,9 +240,13 @@ static void lpss_ssp_setup(struct driver_data *drv_data)
 	if (drv_data->master_info->enable_dma) {
 		__lpss_ssp_write_priv(drv_data, config->reg_ssp, 1);
 
-		value = __lpss_ssp_read_priv(drv_data, config->reg_general);
-		value |= GENERAL_REG_RXTO_HOLDOFF_DISABLE;
-		__lpss_ssp_write_priv(drv_data, config->reg_general, value);
+		if (config->reg_general >= 0) {
+			value = __lpss_ssp_read_priv(drv_data,
+						     config->reg_general);
+			value |= GENERAL_REG_RXTO_HOLDOFF_DISABLE;
+			__lpss_ssp_write_priv(drv_data,
+					      config->reg_general, value);
+		}
 	}
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types

[Mark Brown]

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

On Wed, Jun 03, 2015 at 01:49:45PM +0300, Jarkko Nikula wrote:

>  static bool is_lpss_ssp(const struct driver_data *drv_data)
>  {
> -	return drv_data->ssp_type == LPSS_SSP;
> +	return drv_data->ssp_type == LPSS_LPT_SSP ||
> +	       drv_data->ssp_type == LPSS_BYT_SSP;
>  }

switch statement please, it helps scale new types.

> -	switch (drv_data->ssp_type) {
> -	case QUARK_X1000_SSP:
> +	if (is_quark_x1000_ssp(drv_data)) {
>  		tx_thres = TX_THRESH_QUARK_X1000_DFLT;
>  		tx_hi_thres = 0;
>  		rx_thres = RX_THRESH_QUARK_X1000_DFLT;
> -		break;
> -	case LPSS_SSP:
> +	} else if (is_lpss_ssp(drv_data)) {

Why are we moving away from a switch statement here?  Half the point of
using them for variant selection is that it makes it easier to adjust
the set of cases later.

> +	const struct acpi_device_id *id;
> +	int devid, type = SSP_UNDEFINED;

Don't mix initialisations with other variable declarations please.

>  	if (!ACPI_HANDLE(&pdev->dev) ||
>  	    acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
>  		return NULL;
>  
> +	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
> +	if (id)
> +		type = (int)id->driver_data;

It'd be a bit clearer if the error case was an else here, though - on
first read I'd expected to return an error if we couldn't identify the
device and the initialisation is far enough away to appear in a
different hunk.

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

Re: [PATCH 2/3] spi: pxa2xx: Prepare for new Intel LPSS SPI type

[Mark Brown]

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

On Wed, Jun 03, 2015 at 01:49:46PM +0300, Jarkko Nikula wrote:
> Some of the Intel LPSS SPI properties will be different in upcoming
> platforms compared to existing Lynxpoint and BayTrail/Braswell. LPSS SPI
> private registers will be at different offset and there will be changes in
> individual registers and default FIFO thresholds too.

This looks good.

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

Re: [PATCH 3/3] spi: pxa2xx: Make LPSS SPI general register optional

[Mark Brown]

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

On Wed, Jun 03, 2015 at 01:49:47PM +0300, Jarkko Nikula wrote:
> General register located in LPSS SPI private register space is not found in
> upcoming Intel LPSS platforms. Access it conditionally depending is it
> defined in configuration.

This too.

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

Re: [PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types

[Jarkko Nikula]

On 06/03/2015 03:11 PM, Mark Brown wrote:
> On Wed, Jun 03, 2015 at 01:49:45PM +0300, Jarkko Nikula wrote:
>
>>   static bool is_lpss_ssp(const struct driver_data *drv_data)
>>   {
>> -	return drv_data->ssp_type == LPSS_SSP;
>> +	return drv_data->ssp_type == LPSS_LPT_SSP ||
>> +	       drv_data->ssp_type == LPSS_BYT_SSP;
>>   }
>
> switch statement please, it helps scale new types.
>
>> -	switch (drv_data->ssp_type) {
>> -	case QUARK_X1000_SSP:
>> +	if (is_quark_x1000_ssp(drv_data)) {
>>   		tx_thres = TX_THRESH_QUARK_X1000_DFLT;
>>   		tx_hi_thres = 0;
>>   		rx_thres = RX_THRESH_QUARK_X1000_DFLT;
>> -		break;
>> -	case LPSS_SSP:
>> +	} else if (is_lpss_ssp(drv_data)) {
>
> Why are we moving away from a switch statement here?  Half the point of
> using them for variant selection is that it makes it easier to adjust
> the set of cases later.
>
It looked clever to reuse is_lpss_ssp() here. Onto another hand I don't 
see we'll expand a lot LPSS types so I'll keep the swich-case.

>> +	const struct acpi_device_id *id;
>> +	int devid, type = SSP_UNDEFINED;
>
> Don't mix initialisations with other variable declarations please.
>
>>   	if (!ACPI_HANDLE(&pdev->dev) ||
>>   	    acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
>>   		return NULL;
>>
>> +	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
>> +	if (id)
>> +		type = (int)id->driver_data;
>
> It'd be a bit clearer if the error case was an else here, though - on
> first read I'd expected to return an error if we couldn't identify the
> device and the initialisation is far enough away to appear in a
> different hunk.
>
Above appears to deserve a comment at least. Code gets here when device 
is either probed using those ACPI IDs in pxa2xx_spi_acpi_match[] or if 
it's registered as an platform device without platform data but with 
ACPI companion being set.

Idea here is to set type from pxa2xx_spi_acpi_match[] only for current 
ACPI probed platforms and let the acpi_match_device() return NULL for 
the latter platform device case.

Upcoming platforms will integrate host controller (SPI, I2C and UART) 
and a dedicated integrated DMA engine into same device with single PCI 
or ACPI ID. Our plan is to register them as a MFD child devices:

http://marc.info/?l=linux-kernel&m=143317013403300&w=2

ACPI companion gets set through the MFD device registration. For 
spi-pxa2xx.c it means probe enters into pxa2xx_spi_acpi_get_pdata() 
because we don't set any platform data and the MMIO, clock and IRQ are 
still set there.

My idea to detect these newer platforms and set the type accordingly is 
to check does the device have named resource "lpss_priv" in a code snip 
below

@@ -1299,6 +1319,13 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device 
*pdev)
  	if (IS_ERR(ssp->mmio_base))
  		return NULL;

+	if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpss_priv")) {
+		type = LPSS_SPT_SSP;
+		pdata->tx_param = pdev->dev.parent;
+		pdata->rx_param = pdev->dev.parent;
+		pdata->dma_filter = pxa2xx_spi_idma_filter;
+	}
+
  	ssp->clk = devm_clk_get(&pdev->dev, NULL);
  	ssp->irq = platform_get_irq(pdev, 0);
  	ssp->type = type;

That made me thinking can I set the type to LPSS_SPT_SSP and needed DMA 
filter directly when acpi_match_device() returns NULL. At quick look it 
seems no any other platform device case than our new MFD registration 
enters here but have to look at more carefully.

-- 
Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types

[Mark Brown]

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

On Wed, Jun 03, 2015 at 05:07:48PM +0300, Jarkko Nikula wrote:
> On 06/03/2015 03:11 PM, Mark Brown wrote:

> My idea to detect these newer platforms and set the type accordingly is to
> check does the device have named resource "lpss_priv" in a code snip below

> @@ -1299,6 +1319,13 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device
> *pdev)
>  	if (IS_ERR(ssp->mmio_base))
>  		return NULL;
> 
> +	if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpss_priv")) {
> +		type = LPSS_SPT_SSP;
> +		pdata->tx_param = pdev->dev.parent;
> +		pdata->rx_param = pdev->dev.parent;
> +		pdata->dma_filter = pxa2xx_spi_idma_filter;
> +	}
> +

Why not just pass some platform data?  The above seems like it's abusing
the API a bit and might upset generic code.

>  	ssp->clk = devm_clk_get(&pdev->dev, NULL);
>  	ssp->irq = platform_get_irq(pdev, 0);
>  	ssp->type = type;

> That made me thinking can I set the type to LPSS_SPT_SSP and needed DMA
> filter directly when acpi_match_device() returns NULL. At quick look it
> seems no any other platform device case than our new MFD registration enters
> here but have to look at more carefully.

That might work too, checking for DT as well would help defensiveness
though you'd have to watch out for board file registraitons too since
I'm not sure all the PXA platforms were DTified.  Or just register the
device with a different ID and use that?

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

Re: [PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types

[Jarkko Nikula]

On 06/03/2015 05:39 PM, Mark Brown wrote:
> On Wed, Jun 03, 2015 at 05:07:48PM +0300, Jarkko Nikula wrote:
>> On 06/03/2015 03:11 PM, Mark Brown wrote:
>
>> My idea to detect these newer platforms and set the type accordingly is to
>> check does the device have named resource "lpss_priv" in a code snip below
>
>> @@ -1299,6 +1319,13 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device
>> *pdev)
>>   	if (IS_ERR(ssp->mmio_base))
>>   		return NULL;
>>
>> +	if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpss_priv")) {
>> +		type = LPSS_SPT_SSP;
>> +		pdata->tx_param = pdev->dev.parent;
>> +		pdata->rx_param = pdev->dev.parent;
>> +		pdata->dma_filter = pxa2xx_spi_idma_filter;
>> +	}
>> +
>
> Why not just pass some platform data?  The above seems like it's abusing
> the API a bit and might upset generic code.
>
I wanted to avoid that in order to keep MFD patch set and SPI changes 
independent from each other from both build and runtime. It would also 
need some code arrangements between pxa2xx_spi_probe() and 
pxa2xx_spi_acpi_get_pdata() but that's a minor reason.

Of course relying on "lpss_priv" is not fully future proof either but it 
looks platform after Sunrisepoint won't need any extra type but just 
looking at differences from device registers.

>>   	ssp->clk = devm_clk_get(&pdev->dev, NULL);
>>   	ssp->irq = platform_get_irq(pdev, 0);
>>   	ssp->type = type;
>
>> That made me thinking can I set the type to LPSS_SPT_SSP and needed DMA
>> filter directly when acpi_match_device() returns NULL. At quick look it
>> seems no any other platform device case than our new MFD registration enters
>> here but have to look at more carefully.
>
> That might work too, checking for DT as well would help defensiveness
> though you'd have to watch out for board file registraitons too since
> I'm not sure all the PXA platforms were DTified.  Or just register the
> device with a different ID and use that?
>
Looks like all users are arch/arm/mach-pxa/ board files (no ACPI 
handle), PCI enumarated that set platform data in pxa2xx_spi_pci_probe() 
and ACPI matched devices in pxa2xx_spi_acpi_match[] (no platform data).

Messing with device ID doesn't look clear either as we need to match the 
clock name and it's more clear to keep the host controller and DMA 
instance IDs the same.

I realized I really need to handle NULL from acpi_match_device() in this 
patch. Assuming our MFD set is in the tree before adding the SPI 
Sunrisepoint support. Then code gets through the ACPI_HANDLE() test in 
pxa2xx_spi_acpi_get_pdata() (no platform data but ACPI companion is set 
by the MFD core) and goes setting the enable_dma flag for instance.

This is not fatal but pxa2xx_spi_dma_setup() picks up then the any first 
available DMA channel because DMA filter function is not in place. It 
obviously won't work since SPI is hard-wired to its integrated DMA so 
transfers will be halted.

I'll send version 2 in a minute.

-- 
Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html