Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448920 > unrolled thread

[PATCH 6/6] spi: octeon: Add thunderx driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 6/6] spi: octeon: Add thunderx driver Jan Glauber <jglauber@cavium.com> - 2016-07-23 12:50 +0200
    Re: [PATCH 6/6] spi: octeon: Add thunderx driver Mark Brown <broonie@kernel.org> - 2016-07-24 23:10 +0200
      Re: [PATCH 6/6] spi: octeon: Add thunderx driver Mark Brown <broonie@kernel.org> - 2016-07-25 18:20 +0200
        Re: [PATCH 6/6] spi: octeon: Add thunderx driver Mark Brown <broonie@kernel.org> - 2016-07-27 20:20 +0200
          Re: [PATCH 6/6] spi: octeon: Add thunderx driver Mark Brown <broonie@kernel.org> - 2016-07-27 21:10 +0200
      Re: [PATCH 6/6] spi: octeon: Add thunderx driver Mark Brown <broonie@kernel.org> - 2016-07-25 18:30 +0200

#1448920 — [PATCH 6/6] spi: octeon: Add thunderx driver

Add ThunderX SPI driver using the shared part from the Octeon
driver. The main difference of the ThunderX driver is that it
is a PCI device so probing is different. The system clock settings
can be specified in device tree.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/spi/Kconfig               |   7 ++
 drivers/spi/Makefile              |   2 +
 drivers/spi/spi-cavium-thunderx.c | 158 ++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-cavium.h          |   3 +
 4 files changed, 170 insertions(+)
 create mode 100644 drivers/spi/spi-cavium-thunderx.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 4b931ec..db02ba7 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -630,6 +630,13 @@ config SPI_TEGRA20_SLINK
 	help
 	  SPI driver for Nvidia Tegra20/Tegra30 SLINK Controller interface.
 
+config SPI_THUNDERX
+	tristate "Cavium ThunderX SPI controller"
+	depends on 64BIT && PCI && !CAVIUM_OCTEON_SOC
+	help
+	  SPI host driver for the hardware found on Cavium ThunderX
+	  SOCs.
+
 config SPI_TOPCLIFF_PCH
 	tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) SPI"
 	depends on PCI && (X86_32 || MIPS || COMPILE_TEST)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 185367e..133364b 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -91,6 +91,8 @@ obj-$(CONFIG_SPI_TEGRA114)		+= spi-tegra114.o
 obj-$(CONFIG_SPI_TEGRA20_SFLASH)	+= spi-tegra20-sflash.o
 obj-$(CONFIG_SPI_TEGRA20_SLINK)		+= spi-tegra20-slink.o
 obj-$(CONFIG_SPI_TLE62X0)		+= spi-tle62x0.o
+spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
+obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
 obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
 obj-$(CONFIG_SPI_TXX9)			+= spi-txx9.o
 obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
diff --git a/drivers/spi/spi-cavium-thunderx.c b/drivers/spi/spi-cavium-thunderx.c
new file mode 100644
index 0000000..7eb9141
--- /dev/null
+++ b/drivers/spi/spi-cavium-thunderx.c
@@ -0,0 +1,158 @@
+/*
+ * Cavium ThunderX SPI driver.
+ *
+ * Copyright (C) 2016 Cavium Inc.
+ * Authors: Jan Glauber <jglauber@cavium.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/spi/spi.h>
+
+#include "spi-cavium.h"
+
+#define DRV_NAME "spi-thunderx"
+
+#define SYS_FREQ_DEFAULT		700000000
+
+static void thunderx_spi_clock_enable(struct device *dev, struct octeon_spi *p)
+{
+	int ret;
+
+	p->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(p->clk)) {
+		p->clk = NULL;
+		goto skip;
+	}
+
+	ret = clk_prepare_enable(p->clk);
+	if (ret)
+		goto skip;
+	p->sys_freq = clk_get_rate(p->clk);
+
+skip:
+	if (!p->sys_freq)
+		p->sys_freq = SYS_FREQ_DEFAULT;
+
+	dev_info(dev, "Set system clock to %u\n", p->sys_freq);
+}
+
+static void thunderx_spi_clock_disable(struct device *dev, struct clk *clk)
+{
+	if (!clk)
+		return;
+	clk_disable_unprepare(clk);
+	devm_clk_put(dev, clk);
+}
+
+static int thunderx_spi_probe(struct pci_dev *pdev,
+			      const struct pci_device_id *ent)
+{
+	struct device *dev = &pdev->dev;
+	struct spi_master *master;
+	struct octeon_spi *p;
+	int ret = -ENOENT;
+
+	master = spi_alloc_master(dev, sizeof(struct octeon_spi));
+	if (!master)
+		return -ENOMEM;
+	p = spi_master_get_devdata(master);
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		dev_err(dev, "Failed to enable PCI device\n");
+		goto out_free;
+	}
+
+	ret = pci_request_regions(pdev, DRV_NAME);
+	if (ret) {
+		dev_err(dev, "PCI request regions failed 0x%x\n", ret);
+		goto out_disable;
+	}
+
+	p->register_base = pci_ioremap_bar(pdev, 0);
+	if (!p->register_base) {
+		dev_err(dev, "Cannot map reg base\n");
+		ret = -EINVAL;
+		goto out_region;
+	}
+
+	p->regs.config = 0x1000;
+	p->regs.status = 0x1008;
+	p->regs.tx = 0x1010;
+	p->regs.data = 0x1080;
+
+	thunderx_spi_clock_enable(dev, p);
+
+	master->num_chipselect = 4;
+	master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH |
+			    SPI_LSB_FIRST | SPI_3WIRE;
+	master->transfer_one_message = octeon_spi_transfer_one_message;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->max_speed_hz = OCTEON_SPI_MAX_CLOCK_HZ;
+	master->dev.of_node = pdev->dev.of_node;
+
+	pci_set_drvdata(pdev, master);
+	ret = devm_spi_register_master(dev, master);
+	if (ret) {
+		dev_err(&pdev->dev, "Register master failed: %d\n", ret);
+		goto out_unmap;
+	}
+
+	dev_info(&pdev->dev, "Cavium SPI bus driver probed\n");
+	return 0;
+
+out_unmap:
+	thunderx_spi_clock_disable(dev, p->clk);
+	iounmap(p->register_base);
+out_region:
+	pci_release_regions(pdev);
+out_disable:
+	pci_disable_device(pdev);
+out_free:
+	spi_master_put(master);
+	return ret;
+}
+
+static void thunderx_spi_remove(struct pci_dev *pdev)
+{
+	struct spi_master *master = pci_get_drvdata(pdev);
+	struct octeon_spi *p;
+
+	p = spi_master_get_devdata(master);
+	if (!p)
+		return;
+
+	/* Put everything in a known state. */
+	writeq(0, p->register_base + OCTEON_SPI_CFG(p));
+
+	thunderx_spi_clock_disable(&pdev->dev, p->clk);
+	iounmap(p->register_base);
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+	pci_set_drvdata(pdev, NULL);
+}
+
+#define PCI_DEVICE_ID_THUNDERX_SPI      0xa00b
+
+static const struct pci_device_id thunderx_spi_pci_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDERX_SPI) },
+	{ 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, thunderx_spi_pci_id_table);
+
+static struct pci_driver thunderx_spi_driver = {
+	.name		= DRV_NAME,
+	.id_table	= thunderx_spi_pci_id_table,
+	.probe		= thunderx_spi_probe,
+	.remove		= thunderx_spi_remove,
+};
+
+module_pci_driver(thunderx_spi_driver);
+
+MODULE_DESCRIPTION("Cavium, Inc. ThunderX SPI bus driver");
+MODULE_AUTHOR("Jan Glauber");
+MODULE_LICENSE("GPL");
diff --git a/drivers/spi/spi-cavium.h b/drivers/spi/spi-cavium.h
index 88c5f36..1f91d61 100644
--- a/drivers/spi/spi-cavium.h
+++ b/drivers/spi/spi-cavium.h
@@ -1,6 +1,8 @@
 #ifndef __SPI_CAVIUM_H
 #define __SPI_CAVIUM_H
 
+#include <linux/clk.h>
+
 #define OCTEON_SPI_MAX_BYTES 9
 #define OCTEON_SPI_MAX_CLOCK_HZ 16000000
 
@@ -17,6 +19,7 @@ struct octeon_spi {
 	u64 cs_enax;
 	int sys_freq;
 	struct octeon_spi_regs regs;
+	struct clk *clk;
 };
 
 #define OCTEON_SPI_CFG(x)	(x->regs.config)
-- 
2.9.0.rc0.21.g7777322

[toc] | [next] | [standalone]

#1449209

[Multipart message — attachments visible in raw view] — view raw

On Sat, Jul 23, 2016 at 12:42:55PM +0200, Jan Glauber wrote:

> +config SPI_THUNDERX
> +	tristate "Cavium ThunderX SPI controller"
> +	depends on 64BIT && PCI && !CAVIUM_OCTEON_SOC

This is a *weird* and most likely broken set of dependencies - why
exclude this if we're on Octeon (or Octeon happens to have been enabled
in a config)?

> +static void thunderx_spi_clock_enable(struct device *dev, struct octeon_spi *p)
> +{
> +	int ret;
> +
> +	p->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(p->clk)) {
> +		p->clk = NULL;
> +		goto skip;
> +	}

This is really not clever - we should be requesting clocks on probe, not
only when we're trying to enable them, and using devm_ outside of probe
paths is usually a warning sign too.  Now, this is actually called from
probe so it works out fine but obviously it'd be better to improve the
power management to only enable the clock when needed and at that point
this function will be used and we'll fall into a bad pattern.

Given how tiny this function is and that we've not bothered splitting
out any of the other resource acquisition it's probably better to just
inline it into probe.

> +	dev_info(&pdev->dev, "Cavium SPI bus driver probed\n");

Again, this is just adding noise to the boot log.

> +#define PCI_DEVICE_ID_THUNDERX_SPI      0xa00b
> +
> +static const struct pci_device_id thunderx_spi_pci_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDERX_SPI) },
> +	{ 0, }
> +};

The define for the device ID doesn't seem to be adding much here.

[toc] | [prev] | [next] | [standalone]

#1449633

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jul 25, 2016 at 05:51:22PM +0200, Jan Glauber wrote:
> On Sun, Jul 24, 2016 at 10:04:52PM +0100, Mark Brown wrote:
> > On Sat, Jul 23, 2016 at 12:42:55PM +0200, Jan Glauber wrote:

> > > +	depends on 64BIT && PCI && !CAVIUM_OCTEON_SOC

> > This is a *weird* and most likely broken set of dependencies - why
> > exclude this if we're on Octeon (or Octeon happens to have been enabled
> > in a config)?

> I agree that it looks weird, the reasoning is that we would like
> to avoid making the driver depend on something like ARCH_THUNDER.

Why?

> So I made the driver depend on the things it actually uses
> (PCI for probing and 64BIT because of readq/writeq) and don't care if it
> compiles on other platforms too (like x86).

The usual pattern would be something like (ARCH_THUNDER || COMPILE_TEST)
&& PCI && 64BIT (so that people on other platforms where the device will
never actually appear don't get bothered by the prompt).

> That said, I can remove the !CAVIUM_OCTEON_SOC, it compiles without
> errors on MIPS too. Would that be ok?

Sure.

[toc] | [prev] | [next] | [standalone]

#1451450

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jul 25, 2016 at 09:31:15AM -0700, David Daney wrote:
> On 07/25/2016 09:16 AM, Mark Brown wrote:

> > The usual pattern would be something like (ARCH_THUNDER || COMPILE_TEST)
> > && PCI && 64BIT (so that people on other platforms where the device will
> > never actually appear don't get bothered by the prompt).

> ARCH_THUNDER needs to die, so perhaps it should be (ARM64 || COMPILE_TEST)
> && PCI && 64BIT if you really want to hide it from non-arm64 kernel configs.

It does?  Why?  One of the functions of the vendor specific Kconfig
options is to improve UX when configuring the kernel, if you're building
for a particular SoC or set of SoCs then we can avoid showing you
drivers that can never possibly appear in your system which makes life
a bit easier.  We shouldn't be using them in the code itself but they do
help people in Kconfig.

[toc] | [prev] | [next] | [standalone]

#1451471

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jul 27, 2016 at 11:25:10AM -0700, David Daney wrote:
> On 07/27/2016 11:12 AM, Mark Brown wrote:
> > On Mon, Jul 25, 2016 at 09:31:15AM -0700, David Daney wrote:

> > > ARCH_THUNDER needs to die, so perhaps it should be (ARM64 || COMPILE_TEST)
> > > && PCI && 64BIT if you really want to hide it from non-arm64 kernel configs.

> > It does?  Why?

> It adds clutter.  If we build a generic kernel, we first must select all the
> ARCH_*, then go back and select the devices we want.  Not much of a value
> add.

The value comes when moving to a new kernel version and updating your
config or doing a new board - if you're building for a specific system
then you get fewer new driver prompts (especially if you've got a
smaller number of hotpluggable buses enabled).  This is the much more
common case for people doing kernel configuration, it did bother people
a lot in the past.

> Better to just directly select the devices and remove this middle ARCH_*
> layer.

It's one option every time a new vendor appears rather than every time a
new driver appears - it's a useful quality of life improvement for
people who are doing system customization that is fairly painless for
those doing generic kernels (who can just enable everything).

> Also who is responsible for making sure the proper ARCH_* constraints are
> maintained?  If we remove ARCH_THUNDER, no need to worry about this.

People using the devices or writing drivers...  this really isn't
particularly challenging and it's not like it's hard to fix if people
notice issues.

[toc] | [prev] | [next] | [standalone]

#1449638

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jul 25, 2016 at 05:51:22PM +0200, Jan Glauber wrote:
> On Sun, Jul 24, 2016 at 10:04:52PM +0100, Mark Brown wrote:
> > On Sat, Jul 23, 2016 at 12:42:55PM +0200, Jan Glauber wrote:

> > > +#define PCI_DEVICE_ID_THUNDERX_SPI      0xa00b

> > > +static const struct pci_device_id thunderx_spi_pci_id_table[] = {
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDERX_SPI) },
> > > +	{ 0, }
> > > +};

> > The define for the device ID doesn't seem to be adding much here.

> I find it more readable instead of PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xa00b),
> or did I miss your point?

No, that's my point - I find myself wondering why there's a define half
way down the file and what else is looking at the define other than the
location a few lines below.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web