| Message ID | 1395057936-8643-1-git-send-email-harinik@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> wrote: > Add driver for Cadence SPI controller. This is used in Xilinx Zynq. > > Signed-off-by: Harini Katakam <harinik@xilinx.com> > --- > .../devicetree/bindings/spi/spi-cadence.txt | 25 + We prefer binding docs in separate patch. > drivers/spi/Kconfig | 7 + > drivers/spi/Makefile | 1 + > drivers/spi/spi-cadence.c | 804 ++++++++++++++++++++ > 4 files changed, 837 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/spi-cadence.txt > create mode 100644 drivers/spi/spi-cadence.c > > diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.txt b/Documentation/devicetree/bindings/spi/spi-cadence.txt > new file mode 100644 > index 0000000..d25bc2d > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/spi-cadence.txt > @@ -0,0 +1,25 @@ > +Cadence SPI controller Device Tree Bindings > +------------------------------------------- > + > +Required properties: > +- compatible : Should be "cdns,spi-r1p6". One problem with using IP vendor info in the compatible string is an IP block typically has a variety of configuration options so the actual implementations may be very different. I would recommend adding a Xilinx compatible string as well even if you don't use it now. > +- reg : Physical base address and size of SPI registers map. > +- interrupts : Property with a value describing the interrupt > + number. > +- interrupt-parent : Must be core interrupt controller > +- clock-names : List of input clock names - "ref_clk", "pclk" > + (See clock bindings for details). > +- clocks : Clock phandles (see clock bindings for details). > +- num-chip-select : Number of chip selects used. See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here. > + > +Example: > + > + spi@e0007000 { > + clock-names = "ref_clk", "pclk"; > + clocks = <&clkc 26>, <&clkc 35>; > + compatible = "cdns,spi-r1p6"; Nit. We usually put compatible first in the node. > + interrupt-parent = <&intc>; > + interrupts = <0 49 4>; > + num-chip-select = /bits/ 16 <4>; > + reg = <0xe0007000 0x1000>; > + } ; > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > index 581ee2a..aeae44a 100644 > --- a/drivers/spi/Kconfig > +++ b/drivers/spi/Kconfig > @@ -148,6 +148,13 @@ config SPI_BUTTERFLY > inexpensive battery powered microcontroller evaluation board. > This same cable can be used to flash new firmware. > > +config SPI_CADENCE > + tristate "Cadence SPI controller" > + depends on SPI_MASTER > + help > + This selects the Cadence SPI controller master driver > + used by Xilinx Zynq. > + > config SPI_CLPS711X > tristate "CLPS711X host SPI controller" > depends on ARCH_CLPS711X > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > index 95af48d..1be2ed7 100644 > --- a/drivers/spi/Makefile > +++ b/drivers/spi/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_BFIN_V3) += spi-bfin-v3.o > obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o > obj-$(CONFIG_SPI_BITBANG) += spi-bitbang.o > obj-$(CONFIG_SPI_BUTTERFLY) += spi-butterfly.o > +obj-$(CONFIG_SPI_CADENCE) += spi-cadence.o > obj-$(CONFIG_SPI_CLPS711X) += spi-clps711x.o > obj-$(CONFIG_SPI_COLDFIRE_QSPI) += spi-coldfire-qspi.o > obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o > diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c > new file mode 100644 > index 0000000..7344411 > --- /dev/null > +++ b/drivers/spi/spi-cadence.c > @@ -0,0 +1,804 @@ > +/* > + * Cadence SPI controller driver (master mode only) > + * > + * Copyright (C) 2008 - 2014 Xilinx, Inc. > + * > + * based on Blackfin On-Chip SPI Driver (spi_bfin5xx.c) > + * > + * This program is free software; you can redistribute it and/or modify it under > + * the terms of the GNU General Public License version 2 as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/spi/spi.h> > + > +/* Name of this driver */ > +#define CDNS_SPI_NAME "cdns-spi" > + > +/* Register offset definitions */ > +#define CDNS_SPI_CR_OFFSET 0x00 /* Configuration Register, RW */ > +#define CDNS_SPI_ISR_OFFSET 0x04 /* Interrupt Status Register, RO */ > +#define CDNS_SPI_IER_OFFSET 0x08 /* Interrupt Enable Register, WO */ > +#define CDNS_SPI_IDR_OFFSET 0x0c /* Interrupt Disable Register, WO */ > +#define CDNS_SPI_IMR_OFFSET 0x10 /* Interrupt Enabled Mask Register, RO */ > +#define CDNS_SPI_ER_OFFSET 0x14 /* Enable/Disable Register, RW */ > +#define CDNS_SPI_DR_OFFSET 0x18 /* Delay Register, RW */ > +#define CDNS_SPI_TXD_OFFSET 0x1C /* Data Transmit Register, WO */ > +#define CDNS_SPI_RXD_OFFSET 0x20 /* Data Receive Register, RO */ > +#define CDNS_SPI_SICR_OFFSET 0x24 /* Slave Idle Count Register, RW */ > +#define CDNS_SPI_THLD_OFFSET 0x28 /* Transmit FIFO Watermark Register,RW */ > + > +/* > + * SPI Configuration Register bit Masks > + * > + * This register contains various control bits that affect the operation > + * of the SPI controller > + */ > +#define CDNS_SPI_CR_MANSTRT_MASK 0x00010000 /* Manual TX Start */ > +#define CDNS_SPI_CR_CPHA_MASK 0x00000004 /* Clock Phase Control */ > +#define CDNS_SPI_CR_CPOL_MASK 0x00000002 /* Clock Polarity Control */ > +#define CDNS_SPI_CR_SSCTRL_MASK 0x00003C00 /* Slave Select Mask */ > +#define CDNS_SPI_CR_BAUD_DIV_MASK 0x00000038 /* Baud Rate Divisor Mask */ > +#define CDNS_SPI_CR_MSTREN_MASK 0x00000001 /* Master Enable Mask */ > +#define CDNS_SPI_CR_MANSTRTEN_MASK 0x00008000 /* Manual TX Enable Mask */ > +#define CDNS_SPI_CR_SSFORCE_MASK 0x00004000 /* Manual SS Enable Mask */ > +#define CDNS_SPI_CR_BAUD_DIV_4_MASK 0x00000008 /* Default Baud Div Mask */ > +#define CDNS_SPI_CR_DEFAULT_MASK (CDNS_SPI_CR_MSTREN_MASK | \ > + CDNS_SPI_CR_SSCTRL_MASK | \ > + CDNS_SPI_CR_SSFORCE_MASK | \ > + CDNS_SPI_CR_BAUD_DIV_4_MASK) > + > +/* > + * SPI Configuration Register - Baud rate and slave select > + * > + * These are the values used in the calculation of baud rate divisor and > + * setting the slave select. > + */ > + > +#define CDNS_SPI_BAUD_DIV_MAX 7 /* Baud rate divisor maximum */ > +#define CDNS_SPI_BAUD_DIV_MIN 1 /* Baud rate divisor minimum */ > +#define CDNS_SPI_BAUD_DIV_SHIFT 3 /* Baud rate divisor shift in CR */ > +#define CDNS_SPI_SS_SHIFT 10 /* Slave Select field shift in CR */ > +#define CDNS_SPI_SS0 0x1 /* Slave Select zero */ > + > +/* > + * SPI Interrupt Registers bit Masks > + * > + * All the four interrupt registers (Status/Mask/Enable/Disable) have the same > + * bit definitions. > + */ > +#define CDNS_SPI_IXR_TXOW_MASK 0x00000004 /* SPI TX FIFO Overwater */ > +#define CDNS_SPI_IXR_MODF_MASK 0x00000002 /* SPI Mode Fault */ > +#define CDNS_SPI_IXR_RXNEMTY_MASK 0x00000010 /* SPI RX FIFO Not Empty */ > +#define CDNS_SPI_IXR_DEFAULT_MASK (CDNS_SPI_IXR_TXOW_MASK | \ > + CDNS_SPI_IXR_MODF_MASK) > +#define CDNS_SPI_IXR_TXFULL_MASK 0x00000008 /* SPI TX Full */ > +#define CDNS_SPI_IXR_ALL_MASK 0x0000007F /* SPI all interrupts */ > + > +/* > + * SPI Enable Register bit Masks > + * > + * This register is used to enable or disable the SPI controller > + */ > +#define CDNS_SPI_ER_ENABLE_MASK 0x00000001 /* SPI Enable Bit Mask */ > +#define CDNS_SPI_ER_DISABLE_MASK 0x0 /* SPI Disable Bit Mask */ > + > +/* SPI timeout value */ > +#define CDNS_SPI_TIMEOUT (5 * HZ) > + > +/* SPI FIFO depth in bytes */ > +#define CDNS_SPI_FIFO_DEPTH 128 > + > +/* Macros for the SPI controller read/write */ > +#define cdns_spi_read(addr) readl_relaxed(addr) > +#define cdns_spi_write(addr, val) writel_relaxed((val), (addr)) Just use readl/writel directly. > + > +/* Driver state - suspend/ready */ > +enum driver_state_val { > + CDNS_SPI_DRIVER_STATE_READY = 0, > + CDNS_SPI_DRIVER_STATE_SUSPEND > +}; > + > +/** > + * struct cdns_spi - This definition defines spi driver instance > + * @regs: Virtual address of the SPI controller registers > + * @ref_clk: Pointer to the peripheral clock > + * @pclk: Pointer to the APB clock > + * @speed_hz: Current SPI bus clock speed in Hz > + * @txbuf: Pointer to the TX buffer > + * @rxbuf: Pointer to the RX buffer > + * @remaining_bytes: Number of bytes left to transfer > + * @requested_bytes: Number of bytes requested > + * @dev_busy: Device busy flag > + * @done: Transfer complete status > + * @driver_state: Describes driver state - ready/suspended > + */ > +struct cdns_spi { > + void __iomem *regs; > + struct clk *ref_clk; > + struct clk *pclk; > + u32 speed_hz; > + const u8 *txbuf; > + u8 *rxbuf; > + int remaining_bytes; > + int requested_bytes; > + u8 dev_busy; > + struct completion done; > + enum driver_state_val driver_state; > +}; > + > +/** > + * cdns_spi_init_hw - Initialize the hardware and configure the SPI controller > + * @regs_base: Base address of SPI controller > + * > + * On reset the SPI controller is configured to be in master mode, baud rate > + * divisor is set to 4, threshold value for TX FIFO not full interrupt is set > + * to 1 and size of the word to be transferred as 8 bit. > + * This function initializes the SPI controller to disable and clear all the > + * interrupts, enable manual slave select and manual start, deselect all the > + * chip select lines, and enable the SPI controller. > + */ > +static void cdns_spi_init_hw(void __iomem *regs_base) > +{ > + cdns_spi_write(regs_base + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_DISABLE_MASK); > + cdns_spi_write(regs_base + CDNS_SPI_IDR_OFFSET, CDNS_SPI_IXR_ALL_MASK); > + > + /* Clear the RX FIFO */ > + while (cdns_spi_read(regs_base + CDNS_SPI_ISR_OFFSET) & > + CDNS_SPI_IXR_RXNEMTY_MASK) > + cdns_spi_read(regs_base + CDNS_SPI_RXD_OFFSET); > + > + cdns_spi_write(regs_base + CDNS_SPI_ISR_OFFSET, CDNS_SPI_IXR_ALL_MASK); > + cdns_spi_write(regs_base + CDNS_SPI_CR_OFFSET, > + CDNS_SPI_CR_DEFAULT_MASK); > + cdns_spi_write(regs_base + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_ENABLE_MASK); > +} > + > +/** > + * cdns_spi_chipselect - Select or deselect the chip select line > + * @spi: Pointer to the spi_device structure > + * @is_on: Select(1) or deselect (0) the chip select line > + */ > +static void cdns_spi_chipselect(struct spi_device *spi, int is_on) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); > + u32 ctrl_reg; > + > + ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET); > + > + if (is_on) { > + /* Select the slave */ > + ctrl_reg &= ~CDNS_SPI_CR_SSCTRL_MASK; > + ctrl_reg |= ((~(CDNS_SPI_SS0 << spi->chip_select)) << > + CDNS_SPI_SS_SHIFT) & CDNS_SPI_CR_SSCTRL_MASK; > + } else { > + /* Deselect the slave */ > + ctrl_reg |= CDNS_SPI_CR_SSCTRL_MASK; > + } > + > + cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg); > +} > + > +/** > + * cdns_spi_config_clock - Sets clock polarity, phase and frequency > + * @spi: Pointer to the spi_device structure > + * @transfer: Pointer to the spi_transfer structure which provides > + * information about next transfer setup parameters > + * > + * Sets the requested clock polarity, phase and frequency. > + * Note: If the requested frequency is not an exact match with what can be > + * obtained using the prescalar value the driver sets the clock frequency which > + * is lower than the requested frequency (maximum lower) for the transfer. If > + * the requested frequency is higher or lower than that is supported by the SPI > + * controller the driver will set the highest or lowest frequency supported by > + * controller. > + */ > +static void cdns_spi_config_clock(struct spi_device *spi, > + struct spi_transfer *transfer) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); > + u32 ctrl_reg, req_hz, baud_rate_val; > + unsigned long frequency; > + > + if (transfer && transfer->speed_hz) > + req_hz = transfer->speed_hz; > + else > + req_hz = spi->max_speed_hz; > + > + frequency = clk_get_rate(xspi->ref_clk); > + > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_DISABLE_MASK); > + ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET); > + > + /* Set the SPI clock phase and clock polarity */ > + ctrl_reg &= ~(CDNS_SPI_CR_CPHA_MASK | CDNS_SPI_CR_CPOL_MASK); > + if (spi->mode & SPI_CPHA) > + ctrl_reg |= CDNS_SPI_CR_CPHA_MASK; > + if (spi->mode & SPI_CPOL) > + ctrl_reg |= CDNS_SPI_CR_CPOL_MASK; > + > + /* Set the clock frequency */ > + if (xspi->speed_hz != req_hz) { > + /* first valid value is 1 */ > + baud_rate_val = CDNS_SPI_BAUD_DIV_MIN; > + while ((baud_rate_val < CDNS_SPI_BAUD_DIV_MAX) && > + (frequency / (2 << baud_rate_val)) > req_hz) > + baud_rate_val++; > + > + ctrl_reg &= ~CDNS_SPI_CR_BAUD_DIV_MASK; > + ctrl_reg |= baud_rate_val << CDNS_SPI_BAUD_DIV_SHIFT; > + > + xspi->speed_hz = frequency / (2 << baud_rate_val); > + } > + > + cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg); > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_ENABLE_MASK); > +} > + > +/** > + * cdns_spi_setup_transfer - Configure SPI controller for specified transfer > + * @spi: Pointer to the spi_device structure > + * @transfer: Pointer to the spi_transfer structure which provides > + * information about next transfer setup parameters > + * > + * Sets the operational mode of SPI controller for the next SPI transfer and > + * sets the requested clock frequency. > + * > + * Return: 0 on success and error value on error > + */ > +static int cdns_spi_setup_transfer(struct spi_device *spi, > + struct spi_transfer *transfer) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); > + u8 bits_per_word; > + > + bits_per_word = transfer ? > + transfer->bits_per_word : spi->bits_per_word; > + > + if (bits_per_word != 8) { > + dev_err(&spi->dev, "%s, unsupported bits per word %x\n", > + __func__, spi->bits_per_word); > + return -EINVAL; > + } > + > + cdns_spi_config_clock(spi, transfer); > + > + dev_dbg(&spi->dev, "%s, mode %d, %u bits/w, %u clock speed\n", > + __func__, spi->mode, spi->bits_per_word, > + xspi->speed_hz); > + > + return 0; > +} > + > +/** > + * cdns_spi_setup - Configure the SPI controller > + * @spi: Pointer to the spi_device structure > + * > + * Sets the operational mode of SPI controller for the next SPI transfer, sets > + * the baud rate and divisor value to setup the requested spi clock. > + * > + * Return: 0 on success and error value on error > + */ > +static int cdns_spi_setup(struct spi_device *spi) > +{ > + if (!spi->max_speed_hz) > + return -EINVAL; > + > + if (!spi->bits_per_word) > + spi->bits_per_word = 8; > + > + return cdns_spi_setup_transfer(spi, NULL); > +} > + > +/** > + * cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible > + * @xspi: Pointer to the cdns_spi structure > + */ > +static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi) > +{ > + unsigned long trans_cnt = 0; > + > + while ((trans_cnt < CDNS_SPI_FIFO_DEPTH) && > + (xspi->remaining_bytes > 0)) { > + if (xspi->txbuf) > + cdns_spi_write(xspi->regs + CDNS_SPI_TXD_OFFSET, > + *xspi->txbuf++); > + else > + cdns_spi_write(xspi->regs + CDNS_SPI_TXD_OFFSET, 0); > + > + xspi->remaining_bytes--; > + trans_cnt++; > + } > +} > + > +/** > + * cdns_spi_irq - Interrupt service routine of the SPI controller > + * @irq: IRQ number > + * @dev_id: Pointer to the xspi structure > + * > + * This function handles TX empty and Mode Fault interrupts only. > + * On TX empty interrupt this function reads the received data from RX FIFO and > + * fills the TX FIFO if there is any data remaining to be transferred. > + * On Mode Fault interrupt this function indicates that transfer is completed, > + * the SPI subsystem will identify the error as the remaining bytes to be > + * transferred is non-zero. > + * > + * Return: IRQ_HANDLED always > + */ > +static irqreturn_t cdns_spi_irq(int irq, void *dev_id) > +{ > + struct cdns_spi *xspi = dev_id; > + u32 intr_status; > + > + intr_status = cdns_spi_read(xspi->regs + CDNS_SPI_ISR_OFFSET); > + cdns_spi_write(xspi->regs + CDNS_SPI_ISR_OFFSET, intr_status); > + > + if (intr_status & CDNS_SPI_IXR_MODF_MASK) { > + /* Indicate that transfer is completed, the SPI subsystem will > + * identify the error as the remaining bytes to be > + * transferred is non-zero > + */ > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + complete(&xspi->done); > + } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) { > + unsigned long trans_cnt; > + > + trans_cnt = xspi->requested_bytes - xspi->remaining_bytes; > + > + /* Read out the data from the RX FIFO */ > + while (trans_cnt) { > + u8 data; > + > + data = cdns_spi_read(xspi->regs + CDNS_SPI_RXD_OFFSET); > + if (xspi->rxbuf) > + *xspi->rxbuf++ = data; > + > + xspi->requested_bytes--; > + trans_cnt--; > + } > + > + if (xspi->remaining_bytes) { > + /* There is more data to send */ > + cdns_spi_fill_tx_fifo(xspi); > + } else { > + /* Transfer is completed */ > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + complete(&xspi->done); > + } > + } > + > + return IRQ_HANDLED; > +} > + > +/** > + * cdns_spi_reset_controller - Resets SPI controller > + * @spi: Pointer to the spi_device structure > + * > + * This function disables the interrupts, de-asserts the chip select and > + * disables the SPI controller. > + */ > +static void cdns_spi_reset_controller(struct spi_device *spi) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); > + > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + cdns_spi_chipselect(spi, 0); > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_DISABLE_MASK); > +} > + > +/** > + * cdns_spi_start_transfer - Initiates the SPI transfer > + * @spi: Pointer to the spi_device structure > + * @transfer: Pointer to the spi_transfer structure which provides > + * information about next transfer parameters > + * > + * This function fills the TX FIFO, starts the SPI transfer, and waits for the > + * transfer to be completed. > + * > + * Return: Number of bytes transferred in the last transfer > + */ > +static int cdns_spi_start_transfer(struct spi_device *spi, > + struct spi_transfer *transfer) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); > + int ret; > + > + xspi->txbuf = transfer->tx_buf; > + xspi->rxbuf = transfer->rx_buf; > + xspi->remaining_bytes = transfer->len; > + xspi->requested_bytes = transfer->len; > + reinit_completion(&xspi->done); > + > + cdns_spi_fill_tx_fifo(xspi); > + > + cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + > + ret = wait_for_completion_interruptible_timeout(&xspi->done, > + CDNS_SPI_TIMEOUT); > + if (ret < 1) { > + cdns_spi_reset_controller(spi); > + if (!ret) > + return -ETIMEDOUT; > + > + return ret; > + } > + > + return transfer->len - xspi->remaining_bytes; > +} > + > +/** > + * cdns_prepare_transfer_hardware - Prepares hardware for transfer. > + * @master: Pointer to the spi_master structure which provides > + * information about the controller. > + * > + * This function enables SPI master controller. > + * > + * Return: 0 on success and error value on error > + */ > +static int cdns_prepare_transfer_hardware(struct spi_master *master) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(master); > + > + if (xspi->driver_state != CDNS_SPI_DRIVER_STATE_READY) > + return -EINVAL; > + > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_ENABLE_MASK); > + > + return 0; > +} > + > +/** > + * cdns_transfer_one_message - Sets up and transfer a message. > + * @master: Pointer to the spi_master structure which provides > + * information about the controller. > + * @msg: Pointer to the spi_message which contains the > + * data to be transferred. > + * > + * This function calls the necessary functions to setup operational mode, > + * clock, control chip select and completes the transfer. > + * > + * Return: 0 on success and error value on error. > + */ > +static int cdns_transfer_one_message(struct spi_master *master, > + struct spi_message *msg) > +{ > + struct spi_device *spi; > + unsigned cs_change = 1; > + int status = 0, length = 0; > + struct spi_transfer *transfer; > + > + spi = msg->spi; > + > + list_for_each_entry(transfer, &msg->transfers, transfer_list) { > + if ((transfer->bits_per_word || transfer->speed_hz) && > + cs_change) { > + status = cdns_spi_setup_transfer(spi, transfer); > + if (status < 0) > + break; > + } > + > + if (cs_change) > + cdns_spi_chipselect(spi, 1); > + > + cs_change = transfer->cs_change; > + > + if (!transfer->tx_buf && !transfer->rx_buf && > + transfer->len) { > + status = -EINVAL; > + break; > + } > + > + if (transfer->len) > + length = cdns_spi_start_transfer(spi, transfer); > + > + if (length != transfer->len) { > + if (length > 0) > + status = -EMSGSIZE; > + else > + status = length; > + break; > + } > + msg->actual_length += length; > + status = 0; > + > + if (transfer->delay_usecs) > + udelay(transfer->delay_usecs); > + > + if (!cs_change) > + continue; > + > + if (transfer->transfer_list.next == &msg->transfers) > + break; > + > + cdns_spi_chipselect(spi, 0); > + } > + > + if (status || !cs_change) > + cdns_spi_chipselect(spi, 0); > + > + msg->status = status; > + spi_finalize_current_message(master); > + > + return status; > +} > + > +/** > + * cdns_unprepare_transfer_hardware - Relaxes hardware after transfer > + * @master: Pointer to the spi_master structure which provides > + * information about the controller. > + * > + * This function disables the SPI master controller. > + * > + * Return: 0 always > + */ > +static int cdns_unprepare_transfer_hardware(struct spi_master *master) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(master); > + > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_DISABLE_MASK); > + > + return 0; > +} > + > +/** > + * cdns_spi_probe - Probe method for the SPI driver > + * @pdev: Pointer to the platform_device structure > + * > + * This function initializes the driver data structures and the hardware. > + * > + * Return: 0 on success and error value on error > + */ > +static int cdns_spi_probe(struct platform_device *pdev) > +{ > + int ret = 0, irq; > + struct spi_master *master; > + struct cdns_spi *xspi; > + struct resource *res; > + > + master = spi_alloc_master(&pdev->dev, sizeof(*xspi)); > + if (master == NULL) > + return -ENOMEM; > + > + xspi = spi_master_get_devdata(master); > + master->dev.of_node = pdev->dev.of_node; > + platform_set_drvdata(pdev, master); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + xspi->regs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(xspi->regs)) { > + ret = PTR_ERR(xspi->regs); > + goto remove_master; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { I believe this returns NO_IRQ which could be 0. s/</<=/ > + ret = -ENXIO; > + dev_err(&pdev->dev, "irq number is negative\n"); > + goto remove_master; > + } > + > + ret = devm_request_irq(&pdev->dev, irq, cdns_spi_irq, > + 0, pdev->name, xspi); > + if (ret != 0) { > + ret = -ENXIO; > + dev_err(&pdev->dev, "request_irq failed\n"); > + goto remove_master; > + } > + > + xspi->pclk = devm_clk_get(&pdev->dev, "pclk"); > + if (IS_ERR(xspi->pclk)) { > + dev_err(&pdev->dev, "pclk clock not found.\n"); > + ret = PTR_ERR(xspi->pclk); > + goto remove_master; > + } > + > + xspi->ref_clk = devm_clk_get(&pdev->dev, "ref_clk"); > + if (IS_ERR(xspi->ref_clk)) { > + dev_err(&pdev->dev, "ref_clk clock not found.\n"); > + ret = PTR_ERR(xspi->ref_clk); > + goto remove_master; > + } > + > + ret = clk_prepare_enable(xspi->pclk); > + if (ret) { > + dev_err(&pdev->dev, "Unable to enable APB clock.\n"); > + goto remove_master; > + } > + > + ret = clk_prepare_enable(xspi->ref_clk); > + if (ret) { > + dev_err(&pdev->dev, "Unable to enable device clock.\n"); > + goto clk_dis_apb; > + } > + > + /* SPI controller initializations */ > + cdns_spi_init_hw(xspi->regs); > + > + init_completion(&xspi->done); > + > + ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select", > + &master->num_chipselect); > + if (ret < 0) { > + dev_err(&pdev->dev, "couldn't determine num-chip-select\n"); > + goto clk_dis_all; > + } > + master->setup = cdns_spi_setup; > + master->prepare_transfer_hardware = cdns_prepare_transfer_hardware; > + master->transfer_one_message = cdns_transfer_one_message; > + master->unprepare_transfer_hardware = cdns_unprepare_transfer_hardware; > + master->mode_bits = SPI_CPOL | SPI_CPHA; > + > + /* Set to default valid value */ > + xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4; > + > + xspi->driver_state = CDNS_SPI_DRIVER_STATE_READY; > + > + ret = spi_register_master(master); > + if (ret) { > + dev_err(&pdev->dev, "spi_register_master failed\n"); > + goto clk_dis_all; > + } > + > + dev_info(&pdev->dev, "at 0x%08X mapped to 0x%08X, irq=%d\n", > + res->start, (u32 __force)xspi->regs, irq); > + > + return ret; > + > +clk_dis_all: > + clk_disable_unprepare(xspi->ref_clk); > +clk_dis_apb: > + clk_disable_unprepare(xspi->pclk); > +remove_master: > + spi_master_put(master); > + return ret; > +} > + > +/** > + * cdns_spi_remove - Remove method for the SPI driver > + * @pdev: Pointer to the platform_device structure > + * > + * This function is called if a device is physically removed from the system or > + * if the driver module is being unloaded. It frees all resources allocated to > + * the device. > + * > + * Return: 0 on success and error value on error > + */ > +static int cdns_spi_remove(struct platform_device *pdev) > +{ > + struct spi_master *master = platform_get_drvdata(pdev); > + struct cdns_spi *xspi = spi_master_get_devdata(master); > + > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_DISABLE_MASK); > + > + clk_disable_unprepare(xspi->ref_clk); > + clk_disable_unprepare(xspi->pclk); > + > + spi_unregister_master(master); > + spi_master_put(master); > + > + dev_dbg(&pdev->dev, "remove succeeded\n"); > + return 0; > +} > + > +/** > + * cdns_spi_suspend - Suspend method for the SPI driver > + * @dev: Address of the platform_device structure > + * > + * This function disables the SPI controller and > + * changes the driver state to "suspend" > + * > + * Return: 0 on success and error value on error > + */ > +static int __maybe_unused cdns_spi_suspend(struct device *dev) > +{ > + struct platform_device *pdev = container_of(dev, > + struct platform_device, dev); > + struct spi_master *master = platform_get_drvdata(pdev); > + struct cdns_spi *xspi = spi_master_get_devdata(master); > + u32 ctrl_reg; > + > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + complete(&xspi->done); > + > + ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET); > + ctrl_reg |= CDNS_SPI_CR_SSCTRL_MASK; > + cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg); > + > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_DISABLE_MASK); > + > + xspi->driver_state = CDNS_SPI_DRIVER_STATE_SUSPEND; > + > + clk_disable(xspi->ref_clk); > + clk_disable(xspi->pclk); > + > + dev_dbg(&pdev->dev, "suspend succeeded\n"); Not needed. The kernel already has suspend/resume tracing printks. > + return 0; > +} > + > +/** > + * cdns_spi_resume - Resume method for the SPI driver > + * @dev: Address of the platform_device structure > + * > + * This function changes the driver state to "ready" > + * > + * Return: 0 on success and error value on error > + */ > +static int __maybe_unused cdns_spi_resume(struct device *dev) > +{ > + struct platform_device *pdev = container_of(dev, > + struct platform_device, dev); > + struct spi_master *master = platform_get_drvdata(pdev); > + struct cdns_spi *xspi = spi_master_get_devdata(master); > + int ret = 0; > + > + ret = clk_enable(xspi->pclk); > + if (ret) { > + dev_err(dev, "Cannot enable APB clock.\n"); > + return ret; > + } > + > + ret = clk_enable(xspi->ref_clk); > + if (ret) { > + dev_err(dev, "Cannot enable device clock.\n"); > + clk_disable(xspi->pclk); > + return ret; > + } > + > + xspi->driver_state = CDNS_SPI_DRIVER_STATE_READY; > + > + dev_dbg(&pdev->dev, "resume succeeded\n"); Not needed. The kernel already has suspend/resume tracing printks. > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend, > + cdns_spi_resume); > + > +/* Work with hotplug and coldplug */ > +MODULE_ALIAS("platform:" CDNS_SPI_NAME); Not sure, but I don't think this should be needed. > +static struct of_device_id cdns_spi_of_match[] = { > + { .compatible = "cdns,spi-r1p6" }, > + { /* end of table */ } > +}; > +MODULE_DEVICE_TABLE(of, cdns_spi_of_match); > + > +/* cdns_spi_driver - This structure defines the SPI subsystem platform driver */ > +static struct platform_driver cdns_spi_driver = { > + .probe = cdns_spi_probe, > + .remove = cdns_spi_remove, > + .driver = { > + .name = CDNS_SPI_NAME, > + .owner = THIS_MODULE, > + .of_match_table = cdns_spi_of_match, > + .pm = &cdns_spi_dev_pm_ops, > + }, > +}; > + > +module_platform_driver(cdns_spi_driver); > + > +MODULE_AUTHOR("Xilinx, Inc."); > +MODULE_DESCRIPTION("Cadence SPI driver"); > +MODULE_LICENSE("GPL"); > -- > 1.7.9.5 > -- 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, Mar 17, 2014 at 07:47:24AM -0500, Rob Herring wrote: Please delete irrelevant context from your replies, it makes it easier to find the new content. > On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> wrote: > > +/* Macros for the SPI controller read/write */ > > +#define cdns_spi_read(addr) readl_relaxed(addr) > > +#define cdns_spi_write(addr, val) writel_relaxed((val), (addr)) > Just use readl/writel directly. Or make them static inline structures which take the driver data structure and an offset within the register map for the device and do the maths to resolve the actual address. > > +static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend, > > + cdns_spi_resume); > > +/* Work with hotplug and coldplug */ > > +MODULE_ALIAS("platform:" CDNS_SPI_NAME); > Not sure, but I don't think this should be needed. It won't be used by DT systems but it is good practice, especially with something like this that clearly is a generic IP which could be used on non-DT systems too (it's not like it costs anything meaningful).
Hi Rob, On 03/17/2014 01:47 PM, Rob Herring wrote: > On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> wrote: >> Add driver for Cadence SPI controller. This is used in Xilinx Zynq. >> >> Signed-off-by: Harini Katakam <harinik@xilinx.com> >> --- >> .../devicetree/bindings/spi/spi-cadence.txt | 25 + > > We prefer binding docs in separate patch. I have heard several times that also for binding review you need driver to look if this binding make sense that's why Harini sent this together. It means 2 emails instead of one. But if you wish to have this in two files no problem to split it but then I believe both should be copied to DT mailing list. >> drivers/spi/Kconfig | 7 + >> drivers/spi/Makefile | 1 + >> drivers/spi/spi-cadence.c | 804 ++++++++++++++++++++ >> 4 files changed, 837 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/spi/spi-cadence.txt >> create mode 100644 drivers/spi/spi-cadence.c >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.txt b/Documentation/devicetree/bindings/spi/spi-cadence.txt >> new file mode 100644 >> index 0000000..d25bc2d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/spi/spi-cadence.txt >> @@ -0,0 +1,25 @@ >> +Cadence SPI controller Device Tree Bindings >> +------------------------------------------- >> + >> +Required properties: >> +- compatible : Should be "cdns,spi-r1p6". > > One problem with using IP vendor info in the compatible string is an > IP block typically has a variety of configuration options so the > actual implementations may be very different. I would recommend adding > a Xilinx compatible string as well even if you don't use it now. It means xlnx,zynq-spi-r1p6 should be fine. > >> +- reg : Physical base address and size of SPI registers map. >> +- interrupts : Property with a value describing the interrupt >> + number. >> +- interrupt-parent : Must be core interrupt controller >> +- clock-names : List of input clock names - "ref_clk", "pclk" >> + (See clock bindings for details). >> +- clocks : Clock phandles (see clock bindings for details). >> +- num-chip-select : Number of chip selects used. > > See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here. > >> + >> +Example: >> + >> + spi@e0007000 { >> + clock-names = "ref_clk", "pclk"; >> + clocks = <&clkc 26>, <&clkc 35>; >> + compatible = "cdns,spi-r1p6"; > > Nit. We usually put compatible first in the node. Our device-tree generator sorts them that's why it is just like this but not a problem to fix just in documentation. >> + interrupt-parent = <&intc>; >> + interrupts = <0 49 4>; >> + num-chip-select = /bits/ 16 <4>; I was expecting you will comment this a little bit. :-) Because all just reading this num-cs as 32bit and then assigning this value to master->num_chipselect which is 16bit. <snip> >> +/* Macros for the SPI controller read/write */ >> +#define cdns_spi_read(addr) readl_relaxed(addr) >> +#define cdns_spi_write(addr, val) writel_relaxed((val), (addr)) > > Just use readl/writel directly. There shouldn't be any problem to use these helper macros and even I think this is better than using readl/writel directly because you are more flexible if you want to change it to different IO. <snip> >> +/** >> + * cdns_spi_probe - Probe method for the SPI driver >> + * @pdev: Pointer to the platform_device structure >> + * >> + * This function initializes the driver data structures and the hardware. >> + * >> + * Return: 0 on success and error value on error >> + */ >> +static int cdns_spi_probe(struct platform_device *pdev) >> +{ >> + int ret = 0, irq; >> + struct spi_master *master; >> + struct cdns_spi *xspi; >> + struct resource *res; >> + >> + master = spi_alloc_master(&pdev->dev, sizeof(*xspi)); >> + if (master == NULL) >> + return -ENOMEM; >> + >> + xspi = spi_master_get_devdata(master); >> + master->dev.of_node = pdev->dev.of_node; >> + platform_set_drvdata(pdev, master); >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + xspi->regs = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(xspi->regs)) { >> + ret = PTR_ERR(xspi->regs); >> + goto remove_master; >> + } >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { > > I believe this returns NO_IRQ which could be 0. > > s/</<=/ Are you sure regarding this? NO_IRQ on ARM is -1. And IRC irq = 0 should be just valid number. But if you are right then others drivers have to fixed too. >> + return 0; >> +} >> + >> +static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend, >> + cdns_spi_resume); >> + >> +/* Work with hotplug and coldplug */ >> +MODULE_ALIAS("platform:" CDNS_SPI_NAME); > > Not sure, but I don't think this should be needed. I don't know too. Thanks for review, Michal
On Mon, Mar 17, 2014 at 2:22 PM, Michal Simek <monstr@monstr.eu> wrote: >>> + return 0; >>> +} >>> + >>> +static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend, >>> + cdns_spi_resume); >>> + >>> +/* Work with hotplug and coldplug */ >>> +MODULE_ALIAS("platform:" CDNS_SPI_NAME); >> >> Not sure, but I don't think this should be needed. > > I don't know too. A plain platform device driver needs the MODULE_ALIAS, unless there's a "MODULE_DEVICE_TABLE(platform, ...)", which doesn't exist in this driver. However, as the driver fails in the absence of DT: ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select", &master->num_chipselect); if (ret < 0) { dev_err(&pdev->dev, "couldn't determine num-chip-select\n"); goto clk_dis_all; } it won't work (yet) as a plain platform device driver. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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
Hi Rob/Mark, > -----Original Message----- > From: Michal Simek [mailto:monstr@monstr.eu] > Sent: Monday, March 17, 2014 6:53 PM > To: Rob Herring > Cc: Harini Katakam; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; > Kumar Gala; Rob Landley; Mark Brown; Grant Likely; > devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-spi@vger.kernel.org > Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller > > Hi Rob, > > On 03/17/2014 01:47 PM, Rob Herring wrote: > > On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> > wrote: > >> Add driver for Cadence SPI controller. This is used in Xilinx Zynq. > >> > >> Signed-off-by: Harini Katakam <harinik@xilinx.com> > >> --- > >> .../devicetree/bindings/spi/spi-cadence.txt | 25 + > > > > We prefer binding docs in separate patch. > > I have heard several times that also for binding review you need driver to > look if this binding make sense that's why Harini sent this together. > It means 2 emails instead of one. > But if you wish to have this in two files no problem to split it but then I > believe both should be copied to DT mailing list. > I can split this. > > >> drivers/spi/Kconfig | 7 + > >> drivers/spi/Makefile | 1 + > >> drivers/spi/spi-cadence.c | 804 ++++++++++++++++++++ > >> 4 files changed, 837 insertions(+) > >> create mode 100644 > >> Documentation/devicetree/bindings/spi/spi-cadence.txt > >> create mode 100644 drivers/spi/spi-cadence.c > >> > >> diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.txt > >> b/Documentation/devicetree/bindings/spi/spi-cadence.txt > >> new file mode 100644 > >> index 0000000..d25bc2d > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/spi/spi-cadence.txt > >> @@ -0,0 +1,25 @@ > >> +Cadence SPI controller Device Tree Bindings > >> +------------------------------------------- > >> + > >> +Required properties: > >> +- compatible : Should be "cdns,spi-r1p6". > > > > One problem with using IP vendor info in the compatible string is an > > IP block typically has a variety of configuration options so the > > actual implementations may be very different. I would recommend adding > > a Xilinx compatible string as well even if you don't use it now. > > It means xlnx,zynq-spi-r1p6 should be fine. I can add this string. "r1p6" is specific to the cadence IP used. Is that OK? > > > > >> +- reg : Physical base address and size of SPI registers map. > >> +- interrupts : Property with a value describing the interrupt > >> + number. > >> +- interrupt-parent : Must be core interrupt controller > >> +- clock-names : List of input clock names - "ref_clk", "pclk" > >> + (See clock bindings for details). > >> +- clocks : Clock phandles (see clock bindings for details). > >> +- num-chip-select : Number of chip selects used. > > > > See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" > here. > > > >> + > >> +Example: > >> + > >> + spi@e0007000 { > >> + clock-names = "ref_clk", "pclk"; > >> + clocks = <&clkc 26>, <&clkc 35>; > >> + compatible = "cdns,spi-r1p6"; > > > > Nit. We usually put compatible first in the node. > > Our device-tree generator sorts them that's why it is just like this but not a > problem to fix just in documentation. > Will fix. > > >> + interrupt-parent = <&intc>; > >> + interrupts = <0 49 4>; > >> + num-chip-select = /bits/ 16 <4>; > > I was expecting you will comment this a little bit. :-) Because all just reading > this num-cs as 32bit and then assigning this value to master- > >num_chipselect which is 16bit. > > <snip> > > >> +/* Macros for the SPI controller read/write */ > >> +#define cdns_spi_read(addr) readl_relaxed(addr) > >> +#define cdns_spi_write(addr, val) writel_relaxed((val), (addr)) > > > > Just use readl/writel directly. > > There shouldn't be any problem to use these helper macros and even I think > this is better than using readl/writel directly because you are more flexible > if you want to change it to different IO. > Like Michal pointed out, this is more flexible. Can try what Mark suggested: "Or make them static inline structures which take the driver data structure and an offset within the register map for the device and do the maths to resolve the actual address." Thanks for the review. Regards, Harini This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. -- 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
Hi Rob, > -----Original Message----- > From: Rob Herring [mailto:robherring2@gmail.com] > Sent: Monday, March 17, 2014 6:17 PM > To: Harini Katakam > Cc: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Rob > Landley; Mark Brown; Grant Likely; devicetree@vger.kernel.org; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > spi@vger.kernel.org; Michal Simek > Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller > > On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> > wrote: > > Add driver for Cadence SPI controller. This is used in Xilinx Zynq. > > > > Signed-off-by: Harini Katakam <harinik@xilinx.com> > > --- > > > +- reg : Physical base address and size of SPI registers map. > > +- interrupts : Property with a value describing the interrupt > > + number. > > +- interrupt-parent : Must be core interrupt controller > > +- clock-names : List of input clock names - "ref_clk", "pclk" > > + (See clock bindings for details). > > +- clocks : Clock phandles (see clock bindings for details). > > +- num-chip-select : Number of chip selects used. > > See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" > here. I'll check that. > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > I believe this returns NO_IRQ which could be 0. > > s/</<=/ > > > + dev_dbg(&pdev->dev, "suspend succeeded\n"); > > Not needed. The kernel already has suspend/resume tracing printks. Will remove. > > + dev_dbg(&pdev->dev, "resume succeeded\n"); > > Not needed. The kernel already has suspend/resume tracing printks. Will remove. Regards, Harini This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. -- 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, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote: > + bits_per_word = transfer ? > + transfer->bits_per_word : spi->bits_per_word; This would be a lot more legible without the ternery operator. > + if (bits_per_word != 8) { > + dev_err(&spi->dev, "%s, unsupported bits per word %x\n", > + __func__, spi->bits_per_word); > + return -EINVAL; > + } The core will check this for you. > +static int cdns_spi_setup(struct spi_device *spi) > +{ > + if (!spi->max_speed_hz) > + return -EINVAL; > + > + if (!spi->bits_per_word) > + spi->bits_per_word = 8; The core does this for you. > + return cdns_spi_setup_transfer(spi, NULL); > +} It's not clear to me why this has been split into a separate function and the function will write to the hardware which you're not allowed to do in setup() if it might affect an ongoing transfer. > + intr_status = cdns_spi_read(xspi->regs + CDNS_SPI_ISR_OFFSET); > + cdns_spi_write(xspi->regs + CDNS_SPI_ISR_OFFSET, intr_status); > + > + if (intr_status & CDNS_SPI_IXR_MODF_MASK) { > + /* Indicate that transfer is completed, the SPI subsystem will > + * identify the error as the remaining bytes to be > + * transferred is non-zero > + */ > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + complete(&xspi->done); > + } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) { What happens if both interrupts are flagged at the same time? > + if (xspi->remaining_bytes) { > + /* There is more data to send */ > + cdns_spi_fill_tx_fifo(xspi); > + } else { > + /* Transfer is completed */ > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + complete(&xspi->done); > + } > + } I see from further up the file that there are error interrupt states which might be flagged but these are just being ignored. > + > + return IRQ_HANDLED; This should only be returned if an interrupt was actually handled - if the line is shared in some system this will break. > + cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + > + ret = wait_for_completion_interruptible_timeout(&xspi->done, > + CDNS_SPI_TIMEOUT); > + if (ret < 1) { If you return a positive value from transfer_one() the core will wait for you. > +static int cdns_prepare_transfer_hardware(struct spi_master *master) > +{ > + struct cdns_spi *xspi = spi_master_get_devdata(master); > + > + if (xspi->driver_state != CDNS_SPI_DRIVER_STATE_READY) > + return -EINVAL; > + > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > + CDNS_SPI_ER_ENABLE_MASK); > + > + return 0; > +} You probably want to enable the clocks here (and disable them when unpreparing too). > +static int cdns_transfer_one_message(struct spi_master *master, > + struct spi_message *msg) > +{ Just implement transfer_one() and provide a set_cs() operation and most of this code will go away. > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + ret = -ENXIO; > + dev_err(&pdev->dev, "irq number is negative\n"); > + goto remove_master; > + } > + > + ret = devm_request_irq(&pdev->dev, irq, cdns_spi_irq, > + 0, pdev->name, xspi); > + if (ret != 0) { > + ret = -ENXIO; > + dev_err(&pdev->dev, "request_irq failed\n"); > + goto remove_master; > + } I'd expect to see something that initialises the hardware prior to requesting the interrupt, you don't know what state the hardware is in when the driver takes control. > + init_completion(&xspi->done); This needs to be done prior to the interrupt as well. > + ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select", > + &master->num_chipselect); > + if (ret < 0) { > + dev_err(&pdev->dev, "couldn't determine num-chip-select\n"); > + goto clk_dis_all; > + } Is there no reasonable default? > + /* Set to default valid value */ > + xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4; The driver should set max_speed_hz as well. > + dev_info(&pdev->dev, "at 0x%08X mapped to 0x%08X, irq=%d\n", > + res->start, (u32 __force)xspi->regs, irq); No need for this, it's just noisy. > +static int __maybe_unused cdns_spi_suspend(struct device *dev) > +{ This needs to call spi_master_suspend() as well (and similarly on resume). > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > + CDNS_SPI_IXR_DEFAULT_MASK); > + complete(&xspi->done); > + > + ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET); > + ctrl_reg |= CDNS_SPI_CR_SSCTRL_MASK; > + cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg); Don't do this, the spi_master_suspend() call will quiesce transfers (or if open coding it should be doing something similar rather than just breaking any ongoing transfer). > + clk_disable(xspi->ref_clk); > + clk_disable(xspi->pclk); Why not unprepare?
On Mon, Mar 17, 2014 at 05:30:17PM +0000, Mark Brown wrote: > On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote: [..] > > +static int __maybe_unused cdns_spi_suspend(struct device *dev) > > +{ > > This needs to call spi_master_suspend() as well (and similarly on > resume). I'm not that familiar with the SPI core, but this seems like an inversion. Is there a reason why the SPI master class doesn't implement suspend/resume() callbacks which handle stopping/starting the queue automatically for all masters?
On Mon, Mar 17, 2014 at 12:59:11PM -0500, Josh Cartwright wrote: > On Mon, Mar 17, 2014 at 05:30:17PM +0000, Mark Brown wrote: > > On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote: > > > +static int __maybe_unused cdns_spi_suspend(struct device *dev) > > > +{ > > This needs to call spi_master_suspend() as well (and similarly on > > resume). > I'm not that familiar with the SPI core, but this seems like an > inversion. Is there a reason why the SPI master class doesn't implement > suspend/resume() callbacks which handle stopping/starting the queue > automatically for all masters? This is for users of an optional feature of the infrastructure. We probably should just call it anyway since it does have checks for the feature being used (but given all the open coding around this stuff I'd need to verify that the class callbacks would reliably get called). In any case that's not happening now and the driver as it stands is buggy since it's trampling all over the hardware without syncing with anything that isn't ongoing.
On Mon, Mar 17, 2014 at 8:22 AM, Michal Simek <monstr@monstr.eu> wrote: > Hi Rob, > > On 03/17/2014 01:47 PM, Rob Herring wrote: >> On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> wrote: >>> Add driver for Cadence SPI controller. This is used in Xilinx Zynq. >>> >>> Signed-off-by: Harini Katakam <harinik@xilinx.com> >>> --- >>> .../devicetree/bindings/spi/spi-cadence.txt | 25 + >> >> We prefer binding docs in separate patch. > > I have heard several times that also for binding review you need driver > to look if this binding make sense that's why Harini sent this together. > It means 2 emails instead of one. > But if you wish to have this in two files no problem to split it > but then I believe both should be copied to DT mailing list. Yes, for 2 reasons: - To prepare for DT bindings to get merged to separate repo if we ever get there. - So DT maintainers can review/ack just the bindings. >>> +- reg : Physical base address and size of SPI registers map. >>> +- interrupts : Property with a value describing the interrupt >>> + number. >>> +- interrupt-parent : Must be core interrupt controller >>> +- clock-names : List of input clock names - "ref_clk", "pclk" >>> + (See clock bindings for details). >>> +- clocks : Clock phandles (see clock bindings for details). >>> +- num-chip-select : Number of chip selects used. >> >> See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here. >> >>> + >>> +Example: >>> + >>> + spi@e0007000 { >>> + clock-names = "ref_clk", "pclk"; >>> + clocks = <&clkc 26>, <&clkc 35>; >>> + compatible = "cdns,spi-r1p6"; >> >> Nit. We usually put compatible first in the node. > > Our device-tree generator sorts them that's why it is just like this > but not a problem to fix just in documentation. > > >>> + interrupt-parent = <&intc>; >>> + interrupts = <0 49 4>; >>> + num-chip-select = /bits/ 16 <4>; > > I was expecting you will comment this a little bit. :-) > Because all just reading this num-cs as 32bit and then > assigning this value to master->num_chipselect which is 16bit. Well, everyone else has that problem then. Obviously it takes a bit more care than just reading into a u32, but that is a kernel problem and not a problem of the binding. >>> +/* Macros for the SPI controller read/write */ >>> +#define cdns_spi_read(addr) readl_relaxed(addr) >>> +#define cdns_spi_write(addr, val) writel_relaxed((val), (addr)) >> >> Just use readl/writel directly. > > There shouldn't be any problem to use these helper macros > and even I think this is better than using readl/writel directly > because you are more flexible if you want to change it to different > IO. Then when I read the code I have to go lookup what they do while I know exactly what writel/readl do already. It is really the same reasons as why the kernel doesn't have register set and clear bits accessors. >>> + irq = platform_get_irq(pdev, 0); >>> + if (irq < 0) { >> >> I believe this returns NO_IRQ which could be 0. >> >> s/</<=/ > > Are you sure regarding this? > NO_IRQ on ARM is -1. > And IRC irq = 0 should be just valid number. > > But if you are right then others drivers have to fixed too. The definition varies by arch being 0 or -1, so drivers need to deal with both. The preference is 0 is NO_IRQ. It has been decreed by Linus. ARM is actually pretty close to being able to change NO_IRQ to 0. Rob -- 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
Hi Mark, > -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Monday, March 17, 2014 11:00 PM > To: Harini Katakam > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com; > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; rob@landley.net; > grant.likely@linaro.org; devicetree@vger.kernel.org; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > spi@vger.kernel.org; Michal Simek > Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller > > On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote: > > > + bits_per_word = transfer ? > > + transfer->bits_per_word : spi->bits_per_word; > > This would be a lot more legible without the ternery operator. > > > + if (bits_per_word != 8) { > > + dev_err(&spi->dev, "%s, unsupported bits per word %x\n", > > + __func__, spi->bits_per_word); > > + return -EINVAL; > > + } > > The core will check this for you. I dint find that the core does this. > > > +static int cdns_spi_setup(struct spi_device *spi) { > > + if (!spi->max_speed_hz) > > + return -EINVAL; > > + > > + if (!spi->bits_per_word) > > + spi->bits_per_word = 8; > > The core does this for you. I understand that the core does this. > > > + return cdns_spi_setup_transfer(spi, NULL); } > > It's not clear to me why this has been split into a separate function and the > function will write to the hardware which you're not allowed to do in > setup() if it might affect an ongoing transfer. Are you saying that there's a chance cdns_spi_setup() will be called when there's an ongoing transfer? In that case I have to remove the cdns_setup_transfer() call from here but then there's nothing left to do in setup. The function was split into two because cdns_spi_setup_transfer() is called internally by transfer_one_message() everytime. Is it always possible that the spi_transfer paramters will remain the same; In that case this call is probably not necessary in transfer_one_message. > > > + intr_status = cdns_spi_read(xspi->regs + CDNS_SPI_ISR_OFFSET); > > + cdns_spi_write(xspi->regs + CDNS_SPI_ISR_OFFSET, intr_status); > > + > > + if (intr_status & CDNS_SPI_IXR_MODF_MASK) { > > + /* Indicate that transfer is completed, the SPI subsystem will > > + * identify the error as the remaining bytes to be > > + * transferred is non-zero > > + */ > > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > > + CDNS_SPI_IXR_DEFAULT_MASK); > > + complete(&xspi->done); > > + } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) { > > What happens if both interrupts are flagged at the same time? The MODF is an error interrupt and so if TXOW is raised along with it, TXOW will be ignored (it will be cleared but no data is read). Completion is indicated and since the remaining bytes is non-zero, The transfer function will understand that it is an error. > > > + if (xspi->remaining_bytes) { > > + /* There is more data to send */ > > + cdns_spi_fill_tx_fifo(xspi); > > + } else { > > + /* Transfer is completed */ > > + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, > > + CDNS_SPI_IXR_DEFAULT_MASK); > > + complete(&xspi->done); > > + } > > + } > > I see from further up the file that there are error interrupt states which > might be flagged but these are just being ignored. I'm not sure I understand what you are referring to - The only interrupts enabled (See CNDS_IXR_DEFAULT_MASK) are TXOW and MODF. > > > + > > + return IRQ_HANDLED; > > This should only be returned if an interrupt was actually handled - if the line > is shared in some system this will break. In this case both possible interrupt conditions are handled. > > > + cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET, > > + CDNS_SPI_IXR_DEFAULT_MASK); > > + > > + ret = wait_for_completion_interruptible_timeout(&xspi->done, > > + CDNS_SPI_TIMEOUT); > > + if (ret < 1) { > > If you return a positive value from transfer_one() the core will wait for you. > I don't understand. Here, if the ret from wait_for_completion_interruptible_timeout is positive, then this function (spi_start_transfer) will return (transfer->len) - (xspi->remaining_bytes) to cdns_transfer_one_message(the calling funtion). Which will just use this return as information on how many bytes were transferred (see variable length). cdns_transfer_one_messag() only returns 0 or error value (-ve) (see variable status). It doesn't return positive value to core. > > +static int cdns_prepare_transfer_hardware(struct spi_master *master) > > +{ > > + struct cdns_spi *xspi = spi_master_get_devdata(master); > > + > > + if (xspi->driver_state != CDNS_SPI_DRIVER_STATE_READY) > > + return -EINVAL; > > + > > + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, > > + CDNS_SPI_ER_ENABLE_MASK); > > + > > + return 0; > > +} > > You probably want to enable the clocks here (and disable them when > unpreparing too). OK > > > +static int cdns_transfer_one_message(struct spi_master *master, > > + struct spi_message *msg) > > +{ > > Just implement transfer_one() and provide a set_cs() operation and most of > this code will go away. I dint realise there was a hook for set_cs(). Yeah I will simplify this. > > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + ret = -ENXIO; > > + dev_err(&pdev->dev, "irq number is negative\n"); > > + goto remove_master; > > + } > > + > > + ret = devm_request_irq(&pdev->dev, irq, cdns_spi_irq, > > + 0, pdev->name, xspi); > > + if (ret != 0) { > > + ret = -ENXIO; > > + dev_err(&pdev->dev, "request_irq failed\n"); > > + goto remove_master; > > + } > > I'd expect to see something that initialises the hardware prior to requesting > the interrupt, you don't know what state the hardware is in when the driver > takes control. > > > + init_completion(&xspi->done); > > This needs to be done prior to the interrupt as well. I will move the devm_request_irq here. > > > + ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select", > > + &master->num_chipselect); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "couldn't determine num-chip- > select\n"); > > + goto clk_dis_all; > > + } > > Is there no reasonable default? Are you saying if I cant read num-chipselect from dts, I should set it to a default (that will be 4) and proceed? > > > + /* Set to default valid value */ > > + xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4; > > The driver should set max_speed_hz as well. This is the max_speed_hz - 4 is the lowest divisor possible. 2 is invalid. It is set in init_hw (see config register default mask). > > > + clk_disable(xspi->ref_clk); > > + clk_disable(xspi->pclk); > > Why not unprepare? I can do that. Regards, Harini -- 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
Hi Mark, > -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Monday, March 17, 2014 11:45 PM > To: Josh Cartwright > Cc: Harini Katakam; robh+dt@kernel.org; pawel.moll@arm.com; > mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; > galak@codeaurora.org; rob@landley.net; grant.likely@linaro.org; > devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-spi@vger.kernel.org; Michal Simek > Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller > > On Mon, Mar 17, 2014 at 12:59:11PM -0500, Josh Cartwright wrote: > > On Mon, Mar 17, 2014 at 05:30:17PM +0000, Mark Brown wrote: > > > On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote: > > > > > +static int __maybe_unused cdns_spi_suspend(struct device *dev) { > > > > This needs to call spi_master_suspend() as well (and similarly on > > > resume). > > > I'm not that familiar with the SPI core, but this seems like an > > inversion. Is there a reason why the SPI master class doesn't > > implement > > suspend/resume() callbacks which handle stopping/starting the queue > > automatically for all masters? > > This is for users of an optional feature of the infrastructure. We probably > should just call it anyway since it does have checks for the feature being > used (but given all the open coding around this stuff I'd need to verify that > the class callbacks would reliably get called). > > In any case that's not happening now and the driver as it stands is buggy > since it's trampling all over the hardware without syncing with anything that > isn't ongoing. In case of a suspend, we are stopping an ongoing transfer and disabling the interface. In case I add clock disable and anything else to unprepared too, it will be a cleaner exit but it will still stop the transfer right? What do you suggest? Should we wait for transfer to complete or a timeout to occur? Regards, Harini -- 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 Tue, Mar 18, 2014 at 05:16:26AM +0000, Harini Katakam wrote: Please fix your mailer to word wrap within paragraphs, this will make your mail much more legible. > > > + if (bits_per_word != 8) { > > > + dev_err(&spi->dev, "%s, unsupported bits per word %x\n", > > > + __func__, spi->bits_per_word); > > > + return -EINVAL; > > > + } > > The core will check this for you. > I dint find that the core does this. bpw_mask. > > It's not clear to me why this has been split into a separate function and the > > function will write to the hardware which you're not allowed to do in > > setup() if it might affect an ongoing transfer. > Are you saying that there's a chance cdns_spi_setup() will be called > when there's an ongoing transfer? In that case I have to remove the > cdns_setup_transfer() call from here but then there's nothing left to > do in setup. yes. > > I see from further up the file that there are error interrupt states which > > might be flagged but these are just being ignored. > I'm not sure I understand what you are referring to - > The only interrupts enabled (See CNDS_IXR_DEFAULT_MASK) are TXOW and MODF. The code had: > +#define CDNS_SPI_IXR_TXOW_MASK 0x00000004 /* SPI TX FIFO Overwater */ > +#define CDNS_SPI_IXR_MODF_MASK 0x00000002 /* SPI Mode Fault */ > +#define CDNS_SPI_IXR_RXNEMTY_MASK 0x00000010 /* SPI RX FIFO Not Empty */ > +#define CDNS_SPI_IXR_TXFULL_MASK 0x00000008 /* SPI TX Full */ and defined: > +#define CDNS_SPI_IXR_ALL_MASK 0x0000007F /* SPI all interrupts */ > > > + return IRQ_HANDLED; > > This should only be returned if an interrupt was actually handled - if the line > > is shared in some system this will break. > In this case both possible interrupt conditions are handled. Are you sure that's the case, and even if you are that's still not handling the case where the device isn't flagging an interrupt at all. > > > + cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET, > > > + CDNS_SPI_IXR_DEFAULT_MASK); > > > + > > > + ret = wait_for_completion_interruptible_timeout(&xspi->done, > > > + CDNS_SPI_TIMEOUT); > > > + if (ret < 1) { > > If you return a positive value from transfer_one() the core will wait for you. > I don't understand. Here, if the ret from > wait_for_completion_interruptible_timeout is positive, then this > function (spi_start_transfer) will return (transfer->len) - > (xspi->remaining_bytes) to cdns_transfer_one_message(the calling > funtion). Which will just use this return as information on how many > bytes were transferred (see variable length). > cdns_transfer_one_messag() only returns 0 or error value (-ve) (see > variable status). It doesn't return positive value to core. I'm saying you should be implementing a transfer_one() operation and returning a positive value from it so the core can do the delay for you. > > > + ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select", > > > + &master->num_chipselect); > > > + if (ret < 0) { > > > + dev_err(&pdev->dev, "couldn't determine num-chip- > > select\n"); > > > + goto clk_dis_all; > > > + } > > Is there no reasonable default? > Are you saying if I cant read num-chipselect from dts, > I should set it to a default (that will be 4) and proceed? Yes. > > > + /* Set to default valid value */ > > > + xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4; > > The driver should set max_speed_hz as well. > This is the max_speed_hz - 4 is the lowest divisor possible. 2 is invalid. > It is set in init_hw (see config register default mask). I'm saying you should set max_speed_hz, not set speed_hz to the maximum value. This tells the core what the maximum speed is so it can error check the driver operations.
On Tue, Mar 18, 2014 at 05:22:37AM +0000, Harini Katakam wrote: > > > > This needs to call spi_master_suspend() as well (and similarly on > > > > resume). ... > In case of a suspend, we are stopping an ongoing transfer and > disabling the interface. In case I add clock disable and anything > else to unprepared too, it will be a cleaner exit but it will still > stop the transfer right? What do you suggest? Should we wait for > transfer to complete or a timeout to occur? I suggest doing what I said above and calling spi_master_suspend(). Right now what the driver is doing is just halting the hardware if a transfer is in progress which will break any ongoing transfers.
Hi Mark, > -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Tuesday, March 18, 2014 4:34 PM > To: Harini Katakam > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com; > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; rob@landley.net; > grant.likely@linaro.org; devicetree@vger.kernel.org; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > spi@vger.kernel.org; Michal Simek > Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller > > On Tue, Mar 18, 2014 at 05:16:26AM +0000, Harini Katakam wrote: > > Please fix your mailer to word wrap within paragraphs, this will make > your mail much more legible. > > > > > + if (bits_per_word != 8) { > > > > + dev_err(&spi->dev, "%s, unsupported bits per word %x\n", > > > > + __func__, spi->bits_per_word); > > > > + return -EINVAL; > > > > + } > > > > The core will check this for you. > > > I dint find that the core does this. > > bpw_mask. > OK. Will set master->bits_per_word_mask. > > > It's not clear to me why this has been split into a separate function and > the > > > function will write to the hardware which you're not allowed to do in > > > setup() if it might affect an ongoing transfer. > > > Are you saying that there's a chance cdns_spi_setup() will be called > > when there's an ongoing transfer? In that case I have to remove the > > cdns_setup_transfer() call from here but then there's nothing left to > > do in setup. > > yes. > I'm going to remove the bits_per_word check anyway. But the clock configuration still needs to be done. Where should it be done spi_setup() or transfer? Can you please comment on this? "The function was split into two because cdns_spi_setup_transfer() is called internally by transfer_one_message() everytime. Is it always possible that the spi_transfer paramters will remain the same? In that case this call is probably not necessary in transfer_one_message." > > > I see from further up the file that there are error interrupt states which > > > might be flagged but these are just being ignored. > > > I'm not sure I understand what you are referring to - > > The only interrupts enabled (See CNDS_IXR_DEFAULT_MASK) are TXOW > and MODF. > > The code had: > > > +#define CDNS_SPI_IXR_TXOW_MASK 0x00000004 /* SPI TX FIFO > Overwater */ > > +#define CDNS_SPI_IXR_MODF_MASK 0x00000002 /* SPI Mode Fault */ > > +#define CDNS_SPI_IXR_RXNEMTY_MASK 0x00000010 /* SPI RX FIFO Not > Empty */ > > > +#define CDNS_SPI_IXR_TXFULL_MASK 0x00000008 /* SPI TX Full */ > > and defined: > > > +#define CDNS_SPI_IXR_ALL_MASK 0x0000007F /* SPI all interrupts */ > > > > > + return IRQ_HANDLED; > > > > This should only be returned if an interrupt was actually handled - if the > line > > > is shared in some system this will break. > > > In this case both possible interrupt conditions are handled. > > Are you sure that's the case, and even if you are that's still not > handling the case where the device isn't flagging an interrupt at all. > The IXR_ALL mask is only used to disable all the interrupts in the beginning. These two are the only interrupts enabled. And RXNEMPTY status is just polled. That interrupt is not enabled either Regards, Harini This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. -- 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 Tue, Mar 18, 2014 at 12:13:45PM +0000, Harini Katakam wrote: > I'm going to remove the bits_per_word check anyway. > But the clock configuration still needs to be done. > Where should it be done spi_setup() or transfer? It needs to be done on the transfer - that is required anyway since the clock rate is specified per transfer. > > > In this case both possible interrupt conditions are handled. > > Are you sure that's the case, and even if you are that's still not > > handling the case where the device isn't flagging an interrupt at all. > The IXR_ALL mask is only used to disable all the interrupts in the beginning. > These two are the only interrupts enabled. > And RXNEMPTY status is just polled. That interrupt is not enabled either This is all going to be fragile in the face of bugs or changes in the code though and like I keep saying it doesn't handle interrupt sharing.
HI Mark, > -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Tuesday, March 18, 2014 6:04 PM > To: Harini Katakam > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com; > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; rob@landley.net; > grant.likely@linaro.org; devicetree@vger.kernel.org; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > spi@vger.kernel.org; Michal Simek > Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller > > On Tue, Mar 18, 2014 at 12:13:45PM +0000, Harini Katakam wrote: > > > > > > In this case both possible interrupt conditions are handled. > > > > Are you sure that's the case, and even if you are that's still not > > > handling the case where the device isn't flagging an interrupt at all. > > > The IXR_ALL mask is only used to disable all the interrupts in the > beginning. > > These two are the only interrupts enabled. > > And RXNEMPTY status is just polled. That interrupt is not enabled either > > This is all going to be fragile in the face of bugs or changes in the > code though and like I keep saying it doesn't handle interrupt sharing. OK. I dint consider interrupt sharing. Do you think the following implementation would be better? status = IRQ_NONE; if (intr_status & CDNS_SPI_IXR_MODF_MASK) { /* Handle this interrupt here */ status = IRQ_HANDLED; } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) { /* Handle this interrupt here */ status = IRQ_HANDLED; } return status; Regards, Harini -- 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 Tue, Mar 18, 2014 at 02:45:09PM +0000, Harini Katakam wrote: > OK. I dint consider interrupt sharing. > Do you think the following implementation would be better? > status = IRQ_NONE; > if (intr_status & CDNS_SPI_IXR_MODF_MASK) { > /* Handle this interrupt here */ > status = IRQ_HANDLED; > } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) { > /* Handle this interrupt here */ > status = IRQ_HANDLED; > } > return status; Yes, that looks good.
On 03/17/2014 08:00 PM, Rob Herring wrote: > On Mon, Mar 17, 2014 at 8:22 AM, Michal Simek <monstr@monstr.eu> wrote: >> Hi Rob, >> >> On 03/17/2014 01:47 PM, Rob Herring wrote: >>> On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> wrote: >>>> Add driver for Cadence SPI controller. This is used in Xilinx Zynq. >>>> >>>> Signed-off-by: Harini Katakam <harinik@xilinx.com> >>>> --- >>>> .../devicetree/bindings/spi/spi-cadence.txt | 25 + >>> >>> We prefer binding docs in separate patch. >> >> I have heard several times that also for binding review you need driver >> to look if this binding make sense that's why Harini sent this together. >> It means 2 emails instead of one. >> But if you wish to have this in two files no problem to split it >> but then I believe both should be copied to DT mailing list. > > Yes, for 2 reasons: > - To prepare for DT bindings to get merged to separate repo if we ever > get there. > - So DT maintainers can review/ack just the bindings. ok. No problem to divide it. >>>> +- reg : Physical base address and size of SPI registers map. >>>> +- interrupts : Property with a value describing the interrupt >>>> + number. >>>> +- interrupt-parent : Must be core interrupt controller >>>> +- clock-names : List of input clock names - "ref_clk", "pclk" >>>> + (See clock bindings for details). >>>> +- clocks : Clock phandles (see clock bindings for details). >>>> +- num-chip-select : Number of chip selects used. >>> >>> See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here. >>> >>>> + >>>> +Example: >>>> + >>>> + spi@e0007000 { >>>> + clock-names = "ref_clk", "pclk"; >>>> + clocks = <&clkc 26>, <&clkc 35>; >>>> + compatible = "cdns,spi-r1p6"; >>> >>> Nit. We usually put compatible first in the node. >> >> Our device-tree generator sorts them that's why it is just like this >> but not a problem to fix just in documentation. >> >> >>>> + interrupt-parent = <&intc>; >>>> + interrupts = <0 49 4>; >>>> + num-chip-select = /bits/ 16 <4>; >> >> I was expecting you will comment this a little bit. :-) >> Because all just reading this num-cs as 32bit and then >> assigning this value to master->num_chipselect which is 16bit. > > Well, everyone else has that problem then. Obviously it takes a bit > more care than just reading into a u32, but that is a kernel problem > and not a problem of the binding. They are not reading it directly with read_u32 but they are using intermediate u32 value which is assigned to u16 which is fine. This pattern is in most drivers(maybe all). The point is if binding should or can't simplify driver code. And from your reaction above I expect that it is up to driver owner and binding doc how you want to do it. >>>> +/* Macros for the SPI controller read/write */ >>>> +#define cdns_spi_read(addr) readl_relaxed(addr) >>>> +#define cdns_spi_write(addr, val) writel_relaxed((val), (addr)) >>> >>> Just use readl/writel directly. >> >> There shouldn't be any problem to use these helper macros >> and even I think this is better than using readl/writel directly >> because you are more flexible if you want to change it to different >> IO. > > Then when I read the code I have to go lookup what they do while I > know exactly what writel/readl do already. It is really the same > reasons as why the kernel doesn't have register set and clear bits > accessors. I understand your reasons but on the other hand if we want to run ARM-BE then using direct readl/writel functions is also not correct. Mark proposed one solution and I will see how it looks like. >>>> + irq = platform_get_irq(pdev, 0); >>>> + if (irq < 0) { >>> >>> I believe this returns NO_IRQ which could be 0. >>> >>> s/</<=/ >> >> Are you sure regarding this? >> NO_IRQ on ARM is -1. >> And IRC irq = 0 should be just valid number. >> >> But if you are right then others drivers have to fixed too. > > The definition varies by arch being 0 or -1, so drivers need to deal > with both. The preference is 0 is NO_IRQ. It has been decreed by > Linus. ARM is actually pretty close to being able to change NO_IRQ to > 0. I think that resolution was to completely remove NO_IRQ. Thanks, Michal
diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.txt b/Documentation/devicetree/bindings/spi/spi-cadence.txt new file mode 100644 index 0000000..d25bc2d --- /dev/null +++ b/Documentation/devicetree/bindings/spi/spi-cadence.txt @@ -0,0 +1,25 @@ +Cadence SPI controller Device Tree Bindings +------------------------------------------- + +Required properties: +- compatible : Should be "cdns,spi-r1p6". +- reg : Physical base address and size of SPI registers map. +- interrupts : Property with a value describing the interrupt + number. +- interrupt-parent : Must be core interrupt controller +- clock-names : List of input clock names - "ref_clk", "pclk" + (See clock bindings for details). +- clocks : Clock phandles (see clock bindings for details). +- num-chip-select : Number of chip selects used. + +Example: + + spi@e0007000 { + clock-names = "ref_clk", "pclk"; + clocks = <&clkc 26>, <&clkc 35>; + compatible = "cdns,spi-r1p6"; + interrupt-parent = <&intc>; + interrupts = <0 49 4>; + num-chip-select = /bits/ 16 <4>; + reg = <0xe0007000 0x1000>; + } ; diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 581ee2a..aeae44a 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -148,6 +148,13 @@ config SPI_BUTTERFLY inexpensive battery powered microcontroller evaluation board. This same cable can be used to flash new firmware. +config SPI_CADENCE + tristate "Cadence SPI controller" + depends on SPI_MASTER + help + This selects the Cadence SPI controller master driver + used by Xilinx Zynq. + config SPI_CLPS711X tristate "CLPS711X host SPI controller" depends on ARCH_CLPS711X diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 95af48d..1be2ed7 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_BFIN_V3) += spi-bfin-v3.o obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o obj-$(CONFIG_SPI_BITBANG) += spi-bitbang.o obj-$(CONFIG_SPI_BUTTERFLY) += spi-butterfly.o +obj-$(CONFIG_SPI_CADENCE) += spi-cadence.o obj-$(CONFIG_SPI_CLPS711X) += spi-clps711x.o obj-$(CONFIG_SPI_COLDFIRE_QSPI) += spi-coldfire-qspi.o obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c new file mode 100644 index 0000000..7344411 --- /dev/null +++ b/drivers/spi/spi-cadence.c @@ -0,0 +1,804 @@ +/* + * Cadence SPI controller driver (master mode only) + * + * Copyright (C) 2008 - 2014 Xilinx, Inc. + * + * based on Blackfin On-Chip SPI Driver (spi_bfin5xx.c) + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/spi/spi.h> + +/* Name of this driver */ +#define CDNS_SPI_NAME "cdns-spi" + +/* Register offset definitions */ +#define CDNS_SPI_CR_OFFSET 0x00 /* Configuration Register, RW */ +#define CDNS_SPI_ISR_OFFSET 0x04 /* Interrupt Status Register, RO */ +#define CDNS_SPI_IER_OFFSET 0x08 /* Interrupt Enable Register, WO */ +#define CDNS_SPI_IDR_OFFSET 0x0c /* Interrupt Disable Register, WO */ +#define CDNS_SPI_IMR_OFFSET 0x10 /* Interrupt Enabled Mask Register, RO */ +#define CDNS_SPI_ER_OFFSET 0x14 /* Enable/Disable Register, RW */ +#define CDNS_SPI_DR_OFFSET 0x18 /* Delay Register, RW */ +#define CDNS_SPI_TXD_OFFSET 0x1C /* Data Transmit Register, WO */ +#define CDNS_SPI_RXD_OFFSET 0x20 /* Data Receive Register, RO */ +#define CDNS_SPI_SICR_OFFSET 0x24 /* Slave Idle Count Register, RW */ +#define CDNS_SPI_THLD_OFFSET 0x28 /* Transmit FIFO Watermark Register,RW */ + +/* + * SPI Configuration Register bit Masks + * + * This register contains various control bits that affect the operation + * of the SPI controller + */ +#define CDNS_SPI_CR_MANSTRT_MASK 0x00010000 /* Manual TX Start */ +#define CDNS_SPI_CR_CPHA_MASK 0x00000004 /* Clock Phase Control */ +#define CDNS_SPI_CR_CPOL_MASK 0x00000002 /* Clock Polarity Control */ +#define CDNS_SPI_CR_SSCTRL_MASK 0x00003C00 /* Slave Select Mask */ +#define CDNS_SPI_CR_BAUD_DIV_MASK 0x00000038 /* Baud Rate Divisor Mask */ +#define CDNS_SPI_CR_MSTREN_MASK 0x00000001 /* Master Enable Mask */ +#define CDNS_SPI_CR_MANSTRTEN_MASK 0x00008000 /* Manual TX Enable Mask */ +#define CDNS_SPI_CR_SSFORCE_MASK 0x00004000 /* Manual SS Enable Mask */ +#define CDNS_SPI_CR_BAUD_DIV_4_MASK 0x00000008 /* Default Baud Div Mask */ +#define CDNS_SPI_CR_DEFAULT_MASK (CDNS_SPI_CR_MSTREN_MASK | \ + CDNS_SPI_CR_SSCTRL_MASK | \ + CDNS_SPI_CR_SSFORCE_MASK | \ + CDNS_SPI_CR_BAUD_DIV_4_MASK) + +/* + * SPI Configuration Register - Baud rate and slave select + * + * These are the values used in the calculation of baud rate divisor and + * setting the slave select. + */ + +#define CDNS_SPI_BAUD_DIV_MAX 7 /* Baud rate divisor maximum */ +#define CDNS_SPI_BAUD_DIV_MIN 1 /* Baud rate divisor minimum */ +#define CDNS_SPI_BAUD_DIV_SHIFT 3 /* Baud rate divisor shift in CR */ +#define CDNS_SPI_SS_SHIFT 10 /* Slave Select field shift in CR */ +#define CDNS_SPI_SS0 0x1 /* Slave Select zero */ + +/* + * SPI Interrupt Registers bit Masks + * + * All the four interrupt registers (Status/Mask/Enable/Disable) have the same + * bit definitions. + */ +#define CDNS_SPI_IXR_TXOW_MASK 0x00000004 /* SPI TX FIFO Overwater */ +#define CDNS_SPI_IXR_MODF_MASK 0x00000002 /* SPI Mode Fault */ +#define CDNS_SPI_IXR_RXNEMTY_MASK 0x00000010 /* SPI RX FIFO Not Empty */ +#define CDNS_SPI_IXR_DEFAULT_MASK (CDNS_SPI_IXR_TXOW_MASK | \ + CDNS_SPI_IXR_MODF_MASK) +#define CDNS_SPI_IXR_TXFULL_MASK 0x00000008 /* SPI TX Full */ +#define CDNS_SPI_IXR_ALL_MASK 0x0000007F /* SPI all interrupts */ + +/* + * SPI Enable Register bit Masks + * + * This register is used to enable or disable the SPI controller + */ +#define CDNS_SPI_ER_ENABLE_MASK 0x00000001 /* SPI Enable Bit Mask */ +#define CDNS_SPI_ER_DISABLE_MASK 0x0 /* SPI Disable Bit Mask */ + +/* SPI timeout value */ +#define CDNS_SPI_TIMEOUT (5 * HZ) + +/* SPI FIFO depth in bytes */ +#define CDNS_SPI_FIFO_DEPTH 128 + +/* Macros for the SPI controller read/write */ +#define cdns_spi_read(addr) readl_relaxed(addr) +#define cdns_spi_write(addr, val) writel_relaxed((val), (addr)) + +/* Driver state - suspend/ready */ +enum driver_state_val { + CDNS_SPI_DRIVER_STATE_READY = 0, + CDNS_SPI_DRIVER_STATE_SUSPEND +}; + +/** + * struct cdns_spi - This definition defines spi driver instance + * @regs: Virtual address of the SPI controller registers + * @ref_clk: Pointer to the peripheral clock + * @pclk: Pointer to the APB clock + * @speed_hz: Current SPI bus clock speed in Hz + * @txbuf: Pointer to the TX buffer + * @rxbuf: Pointer to the RX buffer + * @remaining_bytes: Number of bytes left to transfer + * @requested_bytes: Number of bytes requested + * @dev_busy: Device busy flag + * @done: Transfer complete status + * @driver_state: Describes driver state - ready/suspended + */ +struct cdns_spi { + void __iomem *regs; + struct clk *ref_clk; + struct clk *pclk; + u32 speed_hz; + const u8 *txbuf; + u8 *rxbuf; + int remaining_bytes; + int requested_bytes; + u8 dev_busy; + struct completion done; + enum driver_state_val driver_state; +}; + +/** + * cdns_spi_init_hw - Initialize the hardware and configure the SPI controller + * @regs_base: Base address of SPI controller + * + * On reset the SPI controller is configured to be in master mode, baud rate + * divisor is set to 4, threshold value for TX FIFO not full interrupt is set + * to 1 and size of the word to be transferred as 8 bit. + * This function initializes the SPI controller to disable and clear all the + * interrupts, enable manual slave select and manual start, deselect all the + * chip select lines, and enable the SPI controller. + */ +static void cdns_spi_init_hw(void __iomem *regs_base) +{ + cdns_spi_write(regs_base + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_DISABLE_MASK); + cdns_spi_write(regs_base + CDNS_SPI_IDR_OFFSET, CDNS_SPI_IXR_ALL_MASK); + + /* Clear the RX FIFO */ + while (cdns_spi_read(regs_base + CDNS_SPI_ISR_OFFSET) & + CDNS_SPI_IXR_RXNEMTY_MASK) + cdns_spi_read(regs_base + CDNS_SPI_RXD_OFFSET); + + cdns_spi_write(regs_base + CDNS_SPI_ISR_OFFSET, CDNS_SPI_IXR_ALL_MASK); + cdns_spi_write(regs_base + CDNS_SPI_CR_OFFSET, + CDNS_SPI_CR_DEFAULT_MASK); + cdns_spi_write(regs_base + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_ENABLE_MASK); +} + +/** + * cdns_spi_chipselect - Select or deselect the chip select line + * @spi: Pointer to the spi_device structure + * @is_on: Select(1) or deselect (0) the chip select line + */ +static void cdns_spi_chipselect(struct spi_device *spi, int is_on) +{ + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); + u32 ctrl_reg; + + ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET); + + if (is_on) { + /* Select the slave */ + ctrl_reg &= ~CDNS_SPI_CR_SSCTRL_MASK; + ctrl_reg |= ((~(CDNS_SPI_SS0 << spi->chip_select)) << + CDNS_SPI_SS_SHIFT) & CDNS_SPI_CR_SSCTRL_MASK; + } else { + /* Deselect the slave */ + ctrl_reg |= CDNS_SPI_CR_SSCTRL_MASK; + } + + cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg); +} + +/** + * cdns_spi_config_clock - Sets clock polarity, phase and frequency + * @spi: Pointer to the spi_device structure + * @transfer: Pointer to the spi_transfer structure which provides + * information about next transfer setup parameters + * + * Sets the requested clock polarity, phase and frequency. + * Note: If the requested frequency is not an exact match with what can be + * obtained using the prescalar value the driver sets the clock frequency which + * is lower than the requested frequency (maximum lower) for the transfer. If + * the requested frequency is higher or lower than that is supported by the SPI + * controller the driver will set the highest or lowest frequency supported by + * controller. + */ +static void cdns_spi_config_clock(struct spi_device *spi, + struct spi_transfer *transfer) +{ + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); + u32 ctrl_reg, req_hz, baud_rate_val; + unsigned long frequency; + + if (transfer && transfer->speed_hz) + req_hz = transfer->speed_hz; + else + req_hz = spi->max_speed_hz; + + frequency = clk_get_rate(xspi->ref_clk); + + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_DISABLE_MASK); + ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET); + + /* Set the SPI clock phase and clock polarity */ + ctrl_reg &= ~(CDNS_SPI_CR_CPHA_MASK | CDNS_SPI_CR_CPOL_MASK); + if (spi->mode & SPI_CPHA) + ctrl_reg |= CDNS_SPI_CR_CPHA_MASK; + if (spi->mode & SPI_CPOL) + ctrl_reg |= CDNS_SPI_CR_CPOL_MASK; + + /* Set the clock frequency */ + if (xspi->speed_hz != req_hz) { + /* first valid value is 1 */ + baud_rate_val = CDNS_SPI_BAUD_DIV_MIN; + while ((baud_rate_val < CDNS_SPI_BAUD_DIV_MAX) && + (frequency / (2 << baud_rate_val)) > req_hz) + baud_rate_val++; + + ctrl_reg &= ~CDNS_SPI_CR_BAUD_DIV_MASK; + ctrl_reg |= baud_rate_val << CDNS_SPI_BAUD_DIV_SHIFT; + + xspi->speed_hz = frequency / (2 << baud_rate_val); + } + + cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg); + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_ENABLE_MASK); +} + +/** + * cdns_spi_setup_transfer - Configure SPI controller for specified transfer + * @spi: Pointer to the spi_device structure + * @transfer: Pointer to the spi_transfer structure which provides + * information about next transfer setup parameters + * + * Sets the operational mode of SPI controller for the next SPI transfer and + * sets the requested clock frequency. + * + * Return: 0 on success and error value on error + */ +static int cdns_spi_setup_transfer(struct spi_device *spi, + struct spi_transfer *transfer) +{ + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); + u8 bits_per_word; + + bits_per_word = transfer ? + transfer->bits_per_word : spi->bits_per_word; + + if (bits_per_word != 8) { + dev_err(&spi->dev, "%s, unsupported bits per word %x\n", + __func__, spi->bits_per_word); + return -EINVAL; + } + + cdns_spi_config_clock(spi, transfer); + + dev_dbg(&spi->dev, "%s, mode %d, %u bits/w, %u clock speed\n", + __func__, spi->mode, spi->bits_per_word, + xspi->speed_hz); + + return 0; +} + +/** + * cdns_spi_setup - Configure the SPI controller + * @spi: Pointer to the spi_device structure + * + * Sets the operational mode of SPI controller for the next SPI transfer, sets + * the baud rate and divisor value to setup the requested spi clock. + * + * Return: 0 on success and error value on error + */ +static int cdns_spi_setup(struct spi_device *spi) +{ + if (!spi->max_speed_hz) + return -EINVAL; + + if (!spi->bits_per_word) + spi->bits_per_word = 8; + + return cdns_spi_setup_transfer(spi, NULL); +} + +/** + * cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible + * @xspi: Pointer to the cdns_spi structure + */ +static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi) +{ + unsigned long trans_cnt = 0; + + while ((trans_cnt < CDNS_SPI_FIFO_DEPTH) && + (xspi->remaining_bytes > 0)) { + if (xspi->txbuf) + cdns_spi_write(xspi->regs + CDNS_SPI_TXD_OFFSET, + *xspi->txbuf++); + else + cdns_spi_write(xspi->regs + CDNS_SPI_TXD_OFFSET, 0); + + xspi->remaining_bytes--; + trans_cnt++; + } +} + +/** + * cdns_spi_irq - Interrupt service routine of the SPI controller + * @irq: IRQ number + * @dev_id: Pointer to the xspi structure + * + * This function handles TX empty and Mode Fault interrupts only. + * On TX empty interrupt this function reads the received data from RX FIFO and + * fills the TX FIFO if there is any data remaining to be transferred. + * On Mode Fault interrupt this function indicates that transfer is completed, + * the SPI subsystem will identify the error as the remaining bytes to be + * transferred is non-zero. + * + * Return: IRQ_HANDLED always + */ +static irqreturn_t cdns_spi_irq(int irq, void *dev_id) +{ + struct cdns_spi *xspi = dev_id; + u32 intr_status; + + intr_status = cdns_spi_read(xspi->regs + CDNS_SPI_ISR_OFFSET); + cdns_spi_write(xspi->regs + CDNS_SPI_ISR_OFFSET, intr_status); + + if (intr_status & CDNS_SPI_IXR_MODF_MASK) { + /* Indicate that transfer is completed, the SPI subsystem will + * identify the error as the remaining bytes to be + * transferred is non-zero + */ + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, + CDNS_SPI_IXR_DEFAULT_MASK); + complete(&xspi->done); + } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) { + unsigned long trans_cnt; + + trans_cnt = xspi->requested_bytes - xspi->remaining_bytes; + + /* Read out the data from the RX FIFO */ + while (trans_cnt) { + u8 data; + + data = cdns_spi_read(xspi->regs + CDNS_SPI_RXD_OFFSET); + if (xspi->rxbuf) + *xspi->rxbuf++ = data; + + xspi->requested_bytes--; + trans_cnt--; + } + + if (xspi->remaining_bytes) { + /* There is more data to send */ + cdns_spi_fill_tx_fifo(xspi); + } else { + /* Transfer is completed */ + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, + CDNS_SPI_IXR_DEFAULT_MASK); + complete(&xspi->done); + } + } + + return IRQ_HANDLED; +} + +/** + * cdns_spi_reset_controller - Resets SPI controller + * @spi: Pointer to the spi_device structure + * + * This function disables the interrupts, de-asserts the chip select and + * disables the SPI controller. + */ +static void cdns_spi_reset_controller(struct spi_device *spi) +{ + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); + + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, + CDNS_SPI_IXR_DEFAULT_MASK); + cdns_spi_chipselect(spi, 0); + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_DISABLE_MASK); +} + +/** + * cdns_spi_start_transfer - Initiates the SPI transfer + * @spi: Pointer to the spi_device structure + * @transfer: Pointer to the spi_transfer structure which provides + * information about next transfer parameters + * + * This function fills the TX FIFO, starts the SPI transfer, and waits for the + * transfer to be completed. + * + * Return: Number of bytes transferred in the last transfer + */ +static int cdns_spi_start_transfer(struct spi_device *spi, + struct spi_transfer *transfer) +{ + struct cdns_spi *xspi = spi_master_get_devdata(spi->master); + int ret; + + xspi->txbuf = transfer->tx_buf; + xspi->rxbuf = transfer->rx_buf; + xspi->remaining_bytes = transfer->len; + xspi->requested_bytes = transfer->len; + reinit_completion(&xspi->done); + + cdns_spi_fill_tx_fifo(xspi); + + cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET, + CDNS_SPI_IXR_DEFAULT_MASK); + + ret = wait_for_completion_interruptible_timeout(&xspi->done, + CDNS_SPI_TIMEOUT); + if (ret < 1) { + cdns_spi_reset_controller(spi); + if (!ret) + return -ETIMEDOUT; + + return ret; + } + + return transfer->len - xspi->remaining_bytes; +} + +/** + * cdns_prepare_transfer_hardware - Prepares hardware for transfer. + * @master: Pointer to the spi_master structure which provides + * information about the controller. + * + * This function enables SPI master controller. + * + * Return: 0 on success and error value on error + */ +static int cdns_prepare_transfer_hardware(struct spi_master *master) +{ + struct cdns_spi *xspi = spi_master_get_devdata(master); + + if (xspi->driver_state != CDNS_SPI_DRIVER_STATE_READY) + return -EINVAL; + + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_ENABLE_MASK); + + return 0; +} + +/** + * cdns_transfer_one_message - Sets up and transfer a message. + * @master: Pointer to the spi_master structure which provides + * information about the controller. + * @msg: Pointer to the spi_message which contains the + * data to be transferred. + * + * This function calls the necessary functions to setup operational mode, + * clock, control chip select and completes the transfer. + * + * Return: 0 on success and error value on error. + */ +static int cdns_transfer_one_message(struct spi_master *master, + struct spi_message *msg) +{ + struct spi_device *spi; + unsigned cs_change = 1; + int status = 0, length = 0; + struct spi_transfer *transfer; + + spi = msg->spi; + + list_for_each_entry(transfer, &msg->transfers, transfer_list) { + if ((transfer->bits_per_word || transfer->speed_hz) && + cs_change) { + status = cdns_spi_setup_transfer(spi, transfer); + if (status < 0) + break; + } + + if (cs_change) + cdns_spi_chipselect(spi, 1); + + cs_change = transfer->cs_change; + + if (!transfer->tx_buf && !transfer->rx_buf && + transfer->len) { + status = -EINVAL; + break; + } + + if (transfer->len) + length = cdns_spi_start_transfer(spi, transfer); + + if (length != transfer->len) { + if (length > 0) + status = -EMSGSIZE; + else + status = length; + break; + } + msg->actual_length += length; + status = 0; + + if (transfer->delay_usecs) + udelay(transfer->delay_usecs); + + if (!cs_change) + continue; + + if (transfer->transfer_list.next == &msg->transfers) + break; + + cdns_spi_chipselect(spi, 0); + } + + if (status || !cs_change) + cdns_spi_chipselect(spi, 0); + + msg->status = status; + spi_finalize_current_message(master); + + return status; +} + +/** + * cdns_unprepare_transfer_hardware - Relaxes hardware after transfer + * @master: Pointer to the spi_master structure which provides + * information about the controller. + * + * This function disables the SPI master controller. + * + * Return: 0 always + */ +static int cdns_unprepare_transfer_hardware(struct spi_master *master) +{ + struct cdns_spi *xspi = spi_master_get_devdata(master); + + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_DISABLE_MASK); + + return 0; +} + +/** + * cdns_spi_probe - Probe method for the SPI driver + * @pdev: Pointer to the platform_device structure + * + * This function initializes the driver data structures and the hardware. + * + * Return: 0 on success and error value on error + */ +static int cdns_spi_probe(struct platform_device *pdev) +{ + int ret = 0, irq; + struct spi_master *master; + struct cdns_spi *xspi; + struct resource *res; + + master = spi_alloc_master(&pdev->dev, sizeof(*xspi)); + if (master == NULL) + return -ENOMEM; + + xspi = spi_master_get_devdata(master); + master->dev.of_node = pdev->dev.of_node; + platform_set_drvdata(pdev, master); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + xspi->regs = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(xspi->regs)) { + ret = PTR_ERR(xspi->regs); + goto remove_master; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + ret = -ENXIO; + dev_err(&pdev->dev, "irq number is negative\n"); + goto remove_master; + } + + ret = devm_request_irq(&pdev->dev, irq, cdns_spi_irq, + 0, pdev->name, xspi); + if (ret != 0) { + ret = -ENXIO; + dev_err(&pdev->dev, "request_irq failed\n"); + goto remove_master; + } + + xspi->pclk = devm_clk_get(&pdev->dev, "pclk"); + if (IS_ERR(xspi->pclk)) { + dev_err(&pdev->dev, "pclk clock not found.\n"); + ret = PTR_ERR(xspi->pclk); + goto remove_master; + } + + xspi->ref_clk = devm_clk_get(&pdev->dev, "ref_clk"); + if (IS_ERR(xspi->ref_clk)) { + dev_err(&pdev->dev, "ref_clk clock not found.\n"); + ret = PTR_ERR(xspi->ref_clk); + goto remove_master; + } + + ret = clk_prepare_enable(xspi->pclk); + if (ret) { + dev_err(&pdev->dev, "Unable to enable APB clock.\n"); + goto remove_master; + } + + ret = clk_prepare_enable(xspi->ref_clk); + if (ret) { + dev_err(&pdev->dev, "Unable to enable device clock.\n"); + goto clk_dis_apb; + } + + /* SPI controller initializations */ + cdns_spi_init_hw(xspi->regs); + + init_completion(&xspi->done); + + ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select", + &master->num_chipselect); + if (ret < 0) { + dev_err(&pdev->dev, "couldn't determine num-chip-select\n"); + goto clk_dis_all; + } + master->setup = cdns_spi_setup; + master->prepare_transfer_hardware = cdns_prepare_transfer_hardware; + master->transfer_one_message = cdns_transfer_one_message; + master->unprepare_transfer_hardware = cdns_unprepare_transfer_hardware; + master->mode_bits = SPI_CPOL | SPI_CPHA; + + /* Set to default valid value */ + xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4; + + xspi->driver_state = CDNS_SPI_DRIVER_STATE_READY; + + ret = spi_register_master(master); + if (ret) { + dev_err(&pdev->dev, "spi_register_master failed\n"); + goto clk_dis_all; + } + + dev_info(&pdev->dev, "at 0x%08X mapped to 0x%08X, irq=%d\n", + res->start, (u32 __force)xspi->regs, irq); + + return ret; + +clk_dis_all: + clk_disable_unprepare(xspi->ref_clk); +clk_dis_apb: + clk_disable_unprepare(xspi->pclk); +remove_master: + spi_master_put(master); + return ret; +} + +/** + * cdns_spi_remove - Remove method for the SPI driver + * @pdev: Pointer to the platform_device structure + * + * This function is called if a device is physically removed from the system or + * if the driver module is being unloaded. It frees all resources allocated to + * the device. + * + * Return: 0 on success and error value on error + */ +static int cdns_spi_remove(struct platform_device *pdev) +{ + struct spi_master *master = platform_get_drvdata(pdev); + struct cdns_spi *xspi = spi_master_get_devdata(master); + + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_DISABLE_MASK); + + clk_disable_unprepare(xspi->ref_clk); + clk_disable_unprepare(xspi->pclk); + + spi_unregister_master(master); + spi_master_put(master); + + dev_dbg(&pdev->dev, "remove succeeded\n"); + return 0; +} + +/** + * cdns_spi_suspend - Suspend method for the SPI driver + * @dev: Address of the platform_device structure + * + * This function disables the SPI controller and + * changes the driver state to "suspend" + * + * Return: 0 on success and error value on error + */ +static int __maybe_unused cdns_spi_suspend(struct device *dev) +{ + struct platform_device *pdev = container_of(dev, + struct platform_device, dev); + struct spi_master *master = platform_get_drvdata(pdev); + struct cdns_spi *xspi = spi_master_get_devdata(master); + u32 ctrl_reg; + + cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET, + CDNS_SPI_IXR_DEFAULT_MASK); + complete(&xspi->done); + + ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET); + ctrl_reg |= CDNS_SPI_CR_SSCTRL_MASK; + cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg); + + cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET, + CDNS_SPI_ER_DISABLE_MASK); + + xspi->driver_state = CDNS_SPI_DRIVER_STATE_SUSPEND; + + clk_disable(xspi->ref_clk); + clk_disable(xspi->pclk); + + dev_dbg(&pdev->dev, "suspend succeeded\n"); + return 0; +} + +/** + * cdns_spi_resume - Resume method for the SPI driver + * @dev: Address of the platform_device structure + * + * This function changes the driver state to "ready" + * + * Return: 0 on success and error value on error + */ +static int __maybe_unused cdns_spi_resume(struct device *dev) +{ + struct platform_device *pdev = container_of(dev, + struct platform_device, dev); + struct spi_master *master = platform_get_drvdata(pdev); + struct cdns_spi *xspi = spi_master_get_devdata(master); + int ret = 0; + + ret = clk_enable(xspi->pclk); + if (ret) { + dev_err(dev, "Cannot enable APB clock.\n"); + return ret; + } + + ret = clk_enable(xspi->ref_clk); + if (ret) { + dev_err(dev, "Cannot enable device clock.\n"); + clk_disable(xspi->pclk); + return ret; + } + + xspi->driver_state = CDNS_SPI_DRIVER_STATE_READY; + + dev_dbg(&pdev->dev, "resume succeeded\n"); + return 0; +} + +static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend, + cdns_spi_resume); + +/* Work with hotplug and coldplug */ +MODULE_ALIAS("platform:" CDNS_SPI_NAME); + +static struct of_device_id cdns_spi_of_match[] = { + { .compatible = "cdns,spi-r1p6" }, + { /* end of table */ } +}; +MODULE_DEVICE_TABLE(of, cdns_spi_of_match); + +/* cdns_spi_driver - This structure defines the SPI subsystem platform driver */ +static struct platform_driver cdns_spi_driver = { + .probe = cdns_spi_probe, + .remove = cdns_spi_remove, + .driver = { + .name = CDNS_SPI_NAME, + .owner = THIS_MODULE, + .of_match_table = cdns_spi_of_match, + .pm = &cdns_spi_dev_pm_ops, + }, +}; + +module_platform_driver(cdns_spi_driver); + +MODULE_AUTHOR("Xilinx, Inc."); +MODULE_DESCRIPTION("Cadence SPI driver"); +MODULE_LICENSE("GPL");
Add driver for Cadence SPI controller. This is used in Xilinx Zynq. Signed-off-by: Harini Katakam <harinik@xilinx.com> --- .../devicetree/bindings/spi/spi-cadence.txt | 25 + drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi-cadence.c | 804 ++++++++++++++++++++ 4 files changed, 837 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-cadence.txt create mode 100644 drivers/spi/spi-cadence.c