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

Message ID 1397751967-20250-3-git-send-email-chiau.ee.chew@intel.com
State New, archived
Headers show

Commit Message

Chew Chiau Ee April 17, 2014, 4:26 p.m. UTC
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(-)

Comments

Mark Brown April 18, 2014, 4:30 p.m. UTC | #1
On Fri, Apr 18, 2014 at 12:26:07AM +0800, Chew Chiau Ee wrote:
> 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.

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.
Mika Westerberg April 22, 2014, 11:57 a.m. UTC | #2
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@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.
> 
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 22, 2014, 12:09 p.m. UTC | #3
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.
Mika Westerberg April 22, 2014, 12:37 p.m. UTC | #4
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox April 22, 2014, 2:27 p.m. UTC | #5
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 22, 2014, 6:21 p.m. UTC | #6
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.

Patch
diff mbox

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;