| Message ID | f87d7a5ef8a713fb6e64c5d9471e7e5bf2051d18.1469174814.git.jglauber@cavium.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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: > > > +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)? I agree that it looks weird, the reasoning is that we would like to avoid making the driver depend on something like ARCH_THUNDER. 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). That said, I can remove the !CAVIUM_OCTEON_SOC, it compiles without errors on MIPS too. Would that be ok? > > +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. OK, I'll merge it into the probe function. > > + 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. I find it more readable instead of PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xa00b), or did I miss your point? thanks, Jan -- 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
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.
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.
On 07/25/2016 09:16 AM, Mark Brown wrote: > 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). 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. > >> That said, I can remove the !CAVIUM_OCTEON_SOC, it compiles without >> errors on MIPS too. Would that be ok? > > Sure. > -- 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
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.
On 07/27/2016 11:12 AM, Mark Brown wrote: > 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? 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. Better to just directly select the devices and remove this middle ARCH_* layer. Also who is responsible for making sure the proper ARCH_* constraints are maintained? If we remove ARCH_THUNDER, no need to worry about this. > 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. > -- 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
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.
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)
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