[PATCH 0/2] Add PCI mode support for BayTrail LPSS SPI

[PATCH 0/2] Add PCI mode support for BayTrail LPSS SPI

[Chew Chiau Ee]

From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

Hi,

BayTrail LPSS subsystem consists of one SPI host which can be PCI
enumerated. PXA2XX PCI layer used to support only CE4100's SPI. Thus,
we convert it into a generic PCI layer to add support for LPSS
SPI as well.

Since PCI mode LPSS SPI does not rely on common clock framework, we
need a mechanism to pass in the host supported clock rate to core layer.
Thus, we introduced a new member known as "max_clk_rate" under struct
pxa2xx_spi_master which can be used in PCI glue layer to pass in
host supported clock rate info.

Chew, Chiau Ee (2):
  spi/pxa2xx-pci: Add PCI mode support for BayTrail LPSS SPI
  spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

 Documentation/spi/pxa2xx       |    3 ++
 drivers/spi/spi-pxa2xx-pci.c   |   79 ++++++++++++++++++++++++++++++++--------
 drivers/spi/spi-pxa2xx.c       |    2 +
 include/linux/spi/pxa2xx_spi.h |    1 +
 4 files changed, 70 insertions(+), 15 deletions(-)

-- 
1.7.4.4

[PATCH 1/2] spi/pxa2xx-pci: Add PCI mode support for BayTrail LPSS SPI

[Chew Chiau Ee]

From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

Similar to CE4100, BayTrail LPSS SPI can be PCI enumerated
as well. Thus, the functions are renamed from ce4100_xxx
to pxa2xx_spi_pci_xxx to clarify that this is a generic
PCI glue layer. Also, added required infrastructure to
support SPI hosts with different configurations.

This patch is based on Mika Westerberg's previous work.

Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
---
 drivers/spi/spi-pxa2xx-pci.c |   76 +++++++++++++++++++++++++++++++++--------
 1 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index 3f006d3..c1865c9 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -8,7 +8,43 @@
 #include <linux/module.h>
 #include <linux/spi/pxa2xx_spi.h>
 
-static int ce4100_spi_probe(struct pci_dev *dev,
+enum {
+	PORT_CE4100,
+	PORT_BYT,
+};
+
+struct pxa_spi_info {
+	enum pxa_ssp_type type;
+	int port_id;
+	int num_chipselect;
+	int tx_slave_id;
+	int tx_chan_id;
+	int rx_slave_id;
+	int rx_chan_id;
+};
+
+static struct pxa_spi_info spi_info_configs[] = {
+	[PORT_CE4100] = {
+		.type = PXA25x_SSP,
+		.port_id =  -1,
+		.num_chipselect = -1,
+		.tx_slave_id = -1,
+		.tx_chan_id = -1,
+		.rx_slave_id = -1,
+		.rx_chan_id = -1,
+	},
+	[PORT_BYT] = {
+		.type = LPSS_SSP,
+		.port_id = 0,
+		.num_chipselect = 1,
+		.tx_slave_id = 0,
+		.tx_chan_id = 0,
+		.rx_slave_id = 1,
+		.rx_chan_id = 1,
+	},
+};
+
+static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 		const struct pci_device_id *ent)
 {
 	struct platform_device_info pi;
@@ -16,6 +52,7 @@ static int ce4100_spi_probe(struct pci_dev *dev,
 	struct platform_device *pdev;
 	struct pxa2xx_spi_master spi_pdata;
 	struct ssp_device *ssp;
+	struct pxa_spi_info *c;
 
 	ret = pcim_enable_device(dev);
 	if (ret)
@@ -25,8 +62,16 @@ static int ce4100_spi_probe(struct pci_dev *dev,
 	if (ret)
 		return ret;
 
+	c = &spi_info_configs[ent->driver_data];
+
 	memset(&spi_pdata, 0, sizeof(spi_pdata));
-	spi_pdata.num_chipselect = dev->devfn;
+	spi_pdata.num_chipselect = (c->num_chipselect > 0) ?
+					c->num_chipselect : dev->devfn;
+	spi_pdata.tx_slave_id = c->tx_slave_id;
+	spi_pdata.tx_chan_id = c->tx_chan_id;
+	spi_pdata.rx_slave_id = c->rx_slave_id;
+	spi_pdata.rx_chan_id = c->rx_chan_id;
+	spi_pdata.enable_dma = c->rx_slave_id >= 0 && c->tx_slave_id >= 0;
 
 	ssp = &spi_pdata.ssp;
 	ssp->phys_base = pci_resource_start(dev, 0);
@@ -36,8 +81,8 @@ static int ce4100_spi_probe(struct pci_dev *dev,
 		return -EIO;
 	}
 	ssp->irq = dev->irq;
-	ssp->port_id = dev->devfn;
-	ssp->type = PXA25x_SSP;
+	ssp->port_id = (c->port_id >= 0) ? c->port_id : dev->devfn;
+	ssp->type = c->type;
 
 	memset(&pi, 0, sizeof(pi));
 	pi.parent = &dev->dev;
@@ -55,28 +100,29 @@ static int ce4100_spi_probe(struct pci_dev *dev,
 	return 0;
 }
 
-static void ce4100_spi_remove(struct pci_dev *dev)
+static void pxa2xx_spi_pci_remove(struct pci_dev *dev)
 {
 	struct platform_device *pdev = pci_get_drvdata(dev);
 
 	platform_device_unregister(pdev);
 }
 
-static const struct pci_device_id ce4100_spi_devices[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x2e6a) },
+static const struct pci_device_id pxa2xx_spi_pci_devices[] = {
+	{ PCI_VDEVICE(INTEL, 0x2e6a), PORT_CE4100 },
+	{ PCI_VDEVICE(INTEL, 0x0f0e), PORT_BYT },
 	{ },
 };
-MODULE_DEVICE_TABLE(pci, ce4100_spi_devices);
+MODULE_DEVICE_TABLE(pci, pxa2xx_spi_pci_devices);
 
-static struct pci_driver ce4100_spi_driver = {
-	.name           = "ce4100_spi",
-	.id_table       = ce4100_spi_devices,
-	.probe          = ce4100_spi_probe,
-	.remove         = ce4100_spi_remove,
+static struct pci_driver pxa2xx_spi_pci_driver = {
+	.name           = "pxa2xx_spi_pci",
+	.id_table       = pxa2xx_spi_pci_devices,
+	.probe          = pxa2xx_spi_pci_probe,
+	.remove         = pxa2xx_spi_pci_remove,
 };
 
-module_pci_driver(ce4100_spi_driver);
+module_pci_driver(pxa2xx_spi_pci_driver);
 
-MODULE_DESCRIPTION("CE4100 PCI-SPI glue code for PXA's driver");
+MODULE_DESCRIPTION("CE4100/LPSS PCI-SPI glue code for PXA's driver");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Sebastian Andrzej Siewior <bigeasy@linutronix.de>");
-- 
1.7.4.4

Re: [PATCH 1/2] spi/pxa2xx-pci: Add PCI mode support for BayTrail LPSS SPI

[Mark Brown]

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

On Fri, Apr 18, 2014 at 12:26:06AM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> Similar to CE4100, BayTrail LPSS SPI can be PCI enumerated
> as well. Thus, the functions are renamed from ce4100_xxx

Applied, thanks.

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

[PATCH 2/2] spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

[Chew Chiau Ee]

From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

Intel BayTrail PCI mode LPSS devices inclusive of SPI do not rely
on common clock framework. Thus, this patch allows the PCI mode
SPI host to pass the supported clock rate info to the core layer
which eventually used for speed calculation.

Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
---
 Documentation/spi/pxa2xx       |    3 +++
 drivers/spi/spi-pxa2xx-pci.c   |    3 +++
 drivers/spi/spi-pxa2xx.c       |    2 ++
 include/linux/spi/pxa2xx_spi.h |    1 +
 4 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/Documentation/spi/pxa2xx b/Documentation/spi/pxa2xx
index 3352f97..3cde429 100644
--- a/Documentation/spi/pxa2xx
+++ b/Documentation/spi/pxa2xx
@@ -31,6 +31,9 @@ The "pxa2xx_spi_master.clock_enable" field is used to enable/disable the
 corresponding SSP peripheral block in the "Clock Enable Register (CKEN"). See
 the "PXA2xx Developer Manual" section "Clocks and Power Management".
 
+The "pxa2xx_spi_master.max_clk_rate" field is used to determine the maximum
+clock rate supported by the SPI master.
+
 The "pxa2xx_spi_master.num_chipselect" field is used to determine the number of
 slave device (chips) attached to this SPI master.
 
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index c1865c9..24cd938 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -17,6 +17,7 @@ struct pxa_spi_info {
 	enum pxa_ssp_type type;
 	int port_id;
 	int num_chipselect;
+	unsigned long max_clk_rate;
 	int tx_slave_id;
 	int tx_chan_id;
 	int rx_slave_id;
@@ -37,6 +38,7 @@ static struct pxa_spi_info spi_info_configs[] = {
 		.type = LPSS_SSP,
 		.port_id = 0,
 		.num_chipselect = 1,
+		.max_clk_rate = 50000000,
 		.tx_slave_id = 0,
 		.tx_chan_id = 0,
 		.rx_slave_id = 1,
@@ -72,6 +74,7 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 	spi_pdata.rx_slave_id = c->rx_slave_id;
 	spi_pdata.rx_chan_id = c->rx_chan_id;
 	spi_pdata.enable_dma = c->rx_slave_id >= 0 && c->tx_slave_id >= 0;
+	spi_pdata.max_clk_rate = c->max_clk_rate;
 
 	ssp = &spi_pdata.ssp;
 	ssp->phys_base = pci_resource_start(dev, 0);
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index d0fc8e0..b670371 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1182,6 +1182,8 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	clk_prepare_enable(ssp->clk);
 
 	drv_data->max_clk_rate = clk_get_rate(ssp->clk);
+	if (!drv_data->max_clk_rate)
+		drv_data->max_clk_rate = platform_info->max_clk_rate;
 
 	/* Load default SSP configuration */
 	write_SSCR0(0, drv_data->ioaddr);
diff --git a/include/linux/spi/pxa2xx_spi.h b/include/linux/spi/pxa2xx_spi.h
index 82d5111..967903f 100644
--- a/include/linux/spi/pxa2xx_spi.h
+++ b/include/linux/spi/pxa2xx_spi.h
@@ -28,6 +28,7 @@ struct pxa2xx_spi_master {
 	u32 clock_enable;
 	u16 num_chipselect;
 	u8 enable_dma;
+	unsigned long max_clk_rate;
 
 	/* DMA engine specific config */
 	int rx_chan_id;
-- 
1.7.4.4

Re: [PATCH 2/2] spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

[Mark Brown]

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

On Fri, Apr 18, 2014 at 12:26:07AM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Intel BayTrail PCI mode LPSS devices inclusive of SPI do not rely
> on common clock framework. Thus, this patch allows the PCI mode
> SPI host to pass the supported clock rate info to the core layer
> which eventually used for speed calculation.

As you mention we already have a clock API for passing around
information about clocks, it would be much better to fix the fact that
this isn't enabled on x86 than to add custom code to deal with clocks
(even simple things like this) - this is far from the only place where
it causes issues.

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

Re: [PATCH 2/2] spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

[Mika Westerberg]

On Fri, Apr 18, 2014 at 05:30:54PM +0100, Mark Brown wrote:
> On Fri, Apr 18, 2014 at 12:26:07AM +0800, Chew Chiau Ee wrote:
> > From: Chew, Chiau Ee <chiau.ee.chew-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 
> > Intel BayTrail PCI mode LPSS devices inclusive of SPI do not rely
> > on common clock framework. Thus, this patch allows the PCI mode
> > SPI host to pass the supported clock rate info to the core layer
> > which eventually used for speed calculation.
> 
> As you mention we already have a clock API for passing around
> information about clocks, it would be much better to fix the fact that
> this isn't enabled on x86 than to add custom code to deal with clocks
> (even simple things like this) - this is far from the only place where
> it causes issues.

We actually have clock API enabled on x86 if CONFIG_X86_INTEL_LPSS is
selected.

In this case the question is more like, where do we create clocks for PCI
enumerated devices? Surely we don't want to have board files to do this.

In ACPI mode, we do it in drivers/acpi/acpi_lpss.c but not sure where we
could do that in PCI code.
--
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 2/2] spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

[Mark Brown]

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

On Tue, Apr 22, 2014 at 02:57:17PM +0300, Mika Westerberg wrote:
> On Fri, Apr 18, 2014 at 05:30:54PM +0100, Mark Brown wrote:

> > As you mention we already have a clock API for passing around
> > information about clocks, it would be much better to fix the fact that
> > this isn't enabled on x86 than to add custom code to deal with clocks
> > (even simple things like this) - this is far from the only place where
> > it causes issues.

> We actually have clock API enabled on x86 if CONFIG_X86_INTEL_LPSS is
> selected.

Why is this conditional?  It's really unhelpful for writing generic code
handling clocks that the API isn't available as standard.  Allowing the
user to disable it if it's not required makes sense but not making it
available at all is just unhelpful.

> In this case the question is more like, where do we create clocks for PCI
> enumerated devices? Surely we don't want to have board files to do this.

> In ACPI mode, we do it in drivers/acpi/acpi_lpss.c but not sure where we
> could do that in PCI code.

As far as I can tell the clock here is part of the same glue that allows
the generic pxa2xx support to be instantiated from PCI.  I would
therefore expect the glue to also instantiate the clock.  Or if it's a
standard PCI clock then perhaps the PCI subsystem ought to be providing
it.

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

Re: [PATCH 2/2] spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

[Mika Westerberg]

On Tue, Apr 22, 2014 at 01:09:48PM +0100, Mark Brown wrote:
> On Tue, Apr 22, 2014 at 02:57:17PM +0300, Mika Westerberg wrote:
> > On Fri, Apr 18, 2014 at 05:30:54PM +0100, Mark Brown wrote:
> 
> > > As you mention we already have a clock API for passing around
> > > information about clocks, it would be much better to fix the fact that
> > > this isn't enabled on x86 than to add custom code to deal with clocks
> > > (even simple things like this) - this is far from the only place where
> > > it causes issues.
> 
> > We actually have clock API enabled on x86 if CONFIG_X86_INTEL_LPSS is
> > selected.
> 
> Why is this conditional?  It's really unhelpful for writing generic code
> handling clocks that the API isn't available as standard.  Allowing the
> user to disable it if it's not required makes sense but not making it
> available at all is just unhelpful.

IIRC, it was decided at the time that it was the "safest" option. Anyway,
distros seem to have it enabled already. Just checked my Fedora
installation and it's there.

> > In this case the question is more like, where do we create clocks for PCI
> > enumerated devices? Surely we don't want to have board files to do this.
> 
> > In ACPI mode, we do it in drivers/acpi/acpi_lpss.c but not sure where we
> > could do that in PCI code.
> 
> As far as I can tell the clock here is part of the same glue that allows
> the generic pxa2xx support to be instantiated from PCI.  I would
> therefore expect the glue to also instantiate the clock.  Or if it's a
> standard PCI clock then perhaps the PCI subsystem ought to be providing
> it.

Instantiating the clock in the PCI glue driver makes sense (as it's not a
standard PCI clock). Thanks.

Chiau Ee, do you see any problems with that solution?
--
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 2/2] spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

[One Thousand Gnomes]

> Why is this conditional?  It's really unhelpful for writing generic code
> handling clocks that the API isn't available as standard.  Allowing the
> user to disable it if it's not required makes sense but not making it
> available at all is just unhelpful.

KConfig allows drivers to select features they need - this seems to be a
non issue ?

> As far as I can tell the clock here is part of the same glue that allows
> the generic pxa2xx support to be instantiated from PCI.  I would
> therefore expect the glue to also instantiate the clock.  Or if it's a
> standard PCI clock then perhaps the PCI subsystem ought to be providing
> it.

For most cases it probably makes sense to do something of that form,
although as clk_enable/clk_disable allow NULL it's much less of an issue
than it might otherwise be for some of the other drivers.

Alan

--
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 2/2] spi/pxa2xx-pci: Pass host clock rate info from PCI glue layer

[Mark Brown]

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

On Tue, Apr 22, 2014 at 03:27:23PM +0100, One Thousand Gnomes wrote:
> > Why is this conditional?  It's really unhelpful for writing generic code
> > handling clocks that the API isn't available as standard.  Allowing the
> > user to disable it if it's not required makes sense but not making it
> > available at all is just unhelpful.

> KConfig allows drivers to select features they need - this seems to be a
> non issue ?

No, not for the clock API - the platform has to provide a clock API
since currently platforms still have the option of providing their own
implementation.  We should fix that (just making the common clock API
available on all platforms which don't have a custom one would help a
lot) but getting anyone to take any interest has been hard.

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