[v2,2/2] spi: add SPI controller driver for UniPhier SoC

Commit Message

Keiji Hayashibara July 26, 2018, 7:09 a.m. UTC

Add SPI controller driver implemented in Socionext UniPhier SoCs.

UniPhier SoCs have two types SPI controllers; SCSSI supports a
single channel, and MCSSI supports multiple channels.
This driver supports SCSSI only.

This controller has 32bit TX/RX FIFO with depth of eight entry,
and supports the SPI master mode only.

This commit is implemented in PIO transfer mode, not DMA transfer.

Signed-off-by: Kunihiko Hayashi <[e-mail]>
Signed-off-by: Keiji Hayashibara <[e-mail]>
---
 drivers/spi/Kconfig        |  13 ++
 drivers/spi/Makefile       |   1 +
 drivers/spi/spi-uniphier.c | 539 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 553 insertions(+)
 create mode 100644 drivers/spi/spi-uniphier.c

Comments

Andy Shevchenko July 26, 2018, 8:46 a.m. UTC | #1

On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara
<[e-mail]> wrote:
> Add SPI controller driver implemented in Socionext UniPhier SoCs.
>
> UniPhier SoCs have two types SPI controllers; SCSSI supports a
> single channel, and MCSSI supports multiple channels.
> This driver supports SCSSI only.
>
> This controller has 32bit TX/RX FIFO with depth of eight entry,
> and supports the SPI master mode only.
>
> This commit is implemented in PIO transfer mode, not DMA transfer.

Few style realted comments.

> +#include <asm/unaligned.h>
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spi/spi.h>

Slightly better to keep them in order and put asm/* at the last.

> +#define SSI_TIMEOUT            2000    /* ms */

SSI_TIMEOUT_MS ?

> +#define SSI_CTL                        0x0

Slightly better to keep same width for the addresses, like 0x00 here.

> +#define SSI_CKS                        0x4

> +#define SSI_TXWDS              0x8

> +#define SSI_RXWDS              0xc

Ditto.


> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val, ckrat;
> +
> +       /*
> +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> +        * round up as we look for equal or less speed
> +        */
> +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);

> +       ckrat = roundup(ckrat, 2);

ckrat += ckrat & 1;

?

> +       /* check if requested speed is too small */
> +       if (ckrat > SSI_MAX_CLK_DIVIDER)

> +               return -EINVAL;

So, does this critical?

> +
> +       if (ckrat < SSI_MIN_CLK_DIVIDER)
> +               ckrat = SSI_MIN_CLK_DIVIDER;

clamp_val()  / max() ?

> +       val = readl(priv->base + SSI_CKS);
> +       val &= ~SSI_CKS_CKRAT_MASK;
> +       val |= ckrat & SSI_CKS_CKRAT_MASK;
> +       writel(val, priv->base + SSI_CKS);
> +
> +       return 0;
> +}

> +       priv->irq = platform_get_irq(pdev, 0);
> +       if (priv->irq < 0) {
> +               dev_err(&pdev->dev, "failed to get IRQ\n");

> +               ret = -ENXIO;

What's wrong with

ret = priv->irq;

?

> +               goto out_disable_clk;
> +       }

> +static const struct of_device_id uniphier_spi_match[] = {
> +       { .compatible = "socionext,uniphier-scssi", },

> +       { /* sentinel */ },

Slightly better without comma.

> +};
> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);

Masahiro Yamada July 26, 2018, 9:12 a.m. UTC | #2

Hi.


2018-07-26 16:09 GMT+09:00 Keiji Hayashibara <[e-mail]>:
> Add SPI controller driver implemented in Socionext UniPhier SoCs.
>
> UniPhier SoCs have two types SPI controllers; SCSSI supports a
> single channel, and MCSSI supports multiple channels.
> This driver supports SCSSI only.
>
> This controller has 32bit TX/RX FIFO with depth of eight entry,
> and supports the SPI master mode only.
>
> This commit is implemented in PIO transfer mode, not DMA transfer.
>
> Signed-off-by: Kunihiko Hayashi <[e-mail]>
> Signed-off-by: Keiji Hayashibara <[e-mail]>
> ---
>  drivers/spi/Kconfig        |  13 ++
>  drivers/spi/Makefile       |   1 +
>  drivers/spi/spi-uniphier.c | 539 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 553 insertions(+)
>  create mode 100644 drivers/spi/spi-uniphier.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ad5d68e..671d078 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -688,6 +688,19 @@ config SPI_TXX9
>         help
>           SPI driver for Toshiba TXx9 MIPS SoCs
>
> +config SPI_UNIPHIER
> +       tristate "Socionext UniPhier SPI Controller"
> +       depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> +       help
> +         This enables a driver for the Socionext UniPhier SoC SCSSI SPI controller.
> +
> +         UniPhier SoCs have SCSSI and MCSSI SPI controllers.
> +         Every UniPhier SoC has SCSSI which supports single channel.
> +         Older UniPhier Pro4/Pro5 also has MCSSI which support multiple channels.
> +         This driver supports SCSSI only.
> +
> +         If your SoC supports SCSSI, say Y here.
> +
>  config SPI_XCOMM
>         tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
>         depends on I2C
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index cb1f437..a90d559 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -101,6 +101,7 @@ spi-thunderx-objs                   := spi-cavium.o spi-cavium-thunderx.o
>  obj-$(CONFIG_SPI_THUNDERX)             += spi-thunderx.o
>  obj-$(CONFIG_SPI_TOPCLIFF_PCH)         += spi-topcliff-pch.o
>  obj-$(CONFIG_SPI_TXX9)                 += spi-txx9.o
> +obj-$(CONFIG_SPI_UNIPHIER)             += spi-uniphier.o
>  obj-$(CONFIG_SPI_XCOMM)                += spi-xcomm.o
>  obj-$(CONFIG_SPI_XILINX)               += spi-xilinx.o
>  obj-$(CONFIG_SPI_XLP)                  += spi-xlp.o
> diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
> new file mode 100644
> index 0000000..2e80bb0
> --- /dev/null
> +++ b/drivers/spi/spi-uniphier.c
> @@ -0,0 +1,539 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// spi-uniphier.c - Socionext UniPhier SPI controller driver
> +// Copyright 2012      Panasonic Corporation
> +// Copyright 2016-2018 Socionext Inc.
> +
> +#include <asm/unaligned.h>
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spi/spi.h>
> +
> +#define SSI_TIMEOUT            2000    /* ms */
> +#define SSI_MAX_CLK_DIVIDER    254
> +#define SSI_MIN_CLK_DIVIDER    4
> +
> +struct uniphier_spi_priv {
> +       void __iomem *base;
> +       int irq;


irq is unnecessary because it is only used in probe().

Use a stack variable.



> +       struct clk *clk;
> +       struct spi_master *master;
> +       struct completion xfer_done;
> +
> +       int error;
> +       unsigned int tx_bytes;
> +       unsigned int rx_bytes;
> +       const u8 *tx_buf;
> +       u8 *rx_buf;
> +
> +       bool is_save_param;
> +       u8 bits_per_word;
> +       u16 mode;
> +       u32 speed_hz;
> +};
> +
> +#define SSI_CTL                        0x0
> +#define   SSI_CTL_EN           BIT(0)
> +
> +#define SSI_CKS                        0x4
> +#define   SSI_CKS_CKRAT_MASK   GENMASK(7, 0)
> +#define   SSI_CKS_CKPHS                BIT(14)
> +#define   SSI_CKS_CKINIT       BIT(13)
> +#define   SSI_CKS_CKDLY                BIT(12)
> +
> +#define SSI_TXWDS              0x8
> +#define   SSI_TXWDS_WDLEN_MASK GENMASK(13, 8)
> +#define   SSI_TXWDS_TDTF_MASK  GENMASK(7, 6)
> +#define   SSI_TXWDS_DTLEN_MASK GENMASK(5, 0)
> +
> +#define SSI_RXWDS              0xc
> +#define   SSI_RXWDS_DTLEN_MASK GENMASK(5, 0)
> +
> +#define SSI_FPS                        0x10
> +#define   SSI_FPS_FSPOL                BIT(15)
> +#define   SSI_FPS_FSTRT                BIT(14)
> +
> +#define SSI_SR                 0x14
> +#define   SSI_SR_RNE           BIT(0)
> +
> +#define SSI_IE                 0x18
> +#define   SSI_IE_RCIE          BIT(3)
> +#define   SSI_IE_RORIE         BIT(0)
> +
> +#define SSI_IS                 0x1c
> +#define   SSI_IS_RXRS          BIT(9)
> +#define   SSI_IS_RCID          BIT(3)
> +#define   SSI_IS_RORID         BIT(0)
> +
> +#define SSI_IC                 0x1c
> +#define   SSI_IC_TCIC          BIT(4)
> +#define   SSI_IC_RCIC          BIT(3)
> +#define   SSI_IC_RORIC         BIT(0)
> +
> +#define SSI_FC                 0x20
> +#define   SSI_FC_TXFFL         BIT(12)
> +#define   SSI_FC_TXFTH_MASK    GENMASK(11, 8)
> +#define   SSI_FC_RXFFL         BIT(4)
> +#define   SSI_FC_RXFTH_MASK    GENMASK(3, 0)
> +
> +#define SSI_TXDR               0x24
> +#define SSI_RXDR               0x24
> +
> +#define SSI_FIFO_DEPTH         8U
> +
> +static inline unsigned int bytes_per_word(unsigned int bits)
> +{
> +       return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4);
> +}
> +
> +static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val;
> +
> +       val = readl(priv->base + SSI_IE);
> +       val |= mask;
> +       writel(val, priv->base + SSI_IE);
> +}
> +
> +static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val;
> +
> +       val = readl(priv->base + SSI_IE);
> +       val &= ~mask;
> +       writel(val, priv->base + SSI_IE);
> +}
> +
> +static void uniphier_spi_set_mode(struct spi_device *spi)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val1, val2;
> +
> +       /*
> +        * clock setting
> +        * CKPHS    capture timing. 0:rising edge, 1:falling edge
> +        * CKINIT   clock initial level. 0:low, 1:high
> +        * CKDLY    clock delay. 0:no delay, 1:delay depending on FSTRT
> +        *          (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
> +        *
> +        * frame setting
> +        * FSPOL    frame signal porarity. 0: low, 1: high
> +        * FSTRT    start frame timing
> +        *          0: rising edge of clock, 1: falling edge of clock
> +        */
> +       switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
> +       case SPI_MODE_0:
> +               /* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
> +               val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
> +               val2 = 0;
> +               break;
> +       case SPI_MODE_1:
> +               /* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
> +               val1 = 0;
> +               val2 = SSI_FPS_FSTRT;
> +               break;
> +       case SPI_MODE_2:
> +               /* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
> +               val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
> +               val2 = SSI_FPS_FSTRT;
> +               break;
> +       case SPI_MODE_3:
> +               /* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
> +               val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
> +               val2 = 0;
> +               break;
> +       }
> +
> +       if (!(spi->mode & SPI_CS_HIGH))
> +               val2 |= SSI_FPS_FSPOL;
> +
> +       writel(val1, priv->base + SSI_CKS);
> +       writel(val2, priv->base + SSI_FPS);
> +
> +       val1 = 0;
> +       if (spi->mode & SPI_LSB_FIRST)
> +               val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> +       writel(val1, priv->base + SSI_TXWDS);
> +       writel(val1, priv->base + SSI_RXWDS);
> +}
> +
> +static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val;
> +
> +       val = readl(priv->base + SSI_TXWDS);
> +       val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> +       val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> +       val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> +       writel(val, priv->base + SSI_TXWDS);
> +
> +       val = readl(priv->base + SSI_RXWDS);
> +       val &= ~SSI_RXWDS_DTLEN_MASK;
> +       val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> +       writel(val, priv->base + SSI_RXWDS);
> +}
> +
> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val, ckrat;
> +
> +       /*
> +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> +        * round up as we look for equal or less speed
> +        */
> +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> +       ckrat = roundup(ckrat, 2);
> +
> +       /* check if requested speed is too small */
> +       if (ckrat > SSI_MAX_CLK_DIVIDER)
> +               return -EINVAL;
> +
> +       if (ckrat < SSI_MIN_CLK_DIVIDER)
> +               ckrat = SSI_MIN_CLK_DIVIDER;
> +
> +       val = readl(priv->base + SSI_CKS);
> +       val &= ~SSI_CKS_CKRAT_MASK;
> +       val |= ckrat & SSI_CKS_CKRAT_MASK;
> +       writel(val, priv->base + SSI_CKS);
> +
> +       return 0;
> +}
> +
> +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> +                                      struct spi_transfer *t)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val;
> +       int ret;
> +
> +       priv->error = 0;
> +       priv->tx_buf = t->tx_buf;
> +       priv->rx_buf = t->rx_buf;
> +       priv->tx_bytes = priv->rx_bytes = t->len;
> +
> +       if (!priv->is_save_param || priv->mode != spi->mode) {
> +               uniphier_spi_set_mode(spi);
> +               priv->mode = spi->mode;
> +       }
> +
> +       if (!priv->is_save_param || priv->bits_per_word != t->bits_per_word) {
> +               uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> +               priv->bits_per_word = t->bits_per_word;
> +       }
> +
> +       if (!priv->is_save_param || priv->speed_hz != t->speed_hz) {
> +               ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> +               if (ret)
> +                       return ret;
> +               priv->speed_hz = t->speed_hz;
> +       }
> +
> +       if (!priv->is_save_param)
> +               priv->is_save_param = true;
> +
> +       /* reset FIFOs */
> +       val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> +       writel(val, priv->base + SSI_FC);
> +
> +       return 0;
> +}
> +
> +static void uniphier_spi_send(struct uniphier_spi_priv *priv)
> +{
> +       int wsize;
> +       u32 val = 0;
> +
> +       wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
> +       priv->tx_bytes -= wsize;
> +
> +       if (priv->tx_buf) {
> +               switch (wsize) {
> +               case 1:
> +                       val = *priv->tx_buf;
> +                       break;
> +               case 2:
> +                       val = get_unaligned_le16(priv->tx_buf);
> +                       break;
> +               case 4:
> +                       val = get_unaligned_le32(priv->tx_buf);
> +                       break;
> +               }
> +
> +               priv->tx_buf += wsize;
> +       }
> +
> +       writel(val, priv->base + SSI_TXDR);
> +}
> +
> +static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
> +{
> +       int rsize;
> +       u32 val;
> +
> +       rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
> +       priv->rx_bytes -= rsize;
> +
> +       val = readl(priv->base + SSI_RXDR);
> +
> +       if (priv->rx_buf) {
> +               switch (rsize) {
> +               case 1:
> +                       *priv->rx_buf = (u8)val;

Is this cast necessary?




> +                       break;
> +               case 2:
> +                       put_unaligned_le16(val, priv->rx_buf);
> +                       break;
> +               case 4:
> +                       put_unaligned_le32(val, priv->rx_buf);
> +                       break;
> +               }
> +
> +               priv->rx_buf += rsize;
> +       }
> +}
> +
> +static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
> +{
> +       unsigned int tx_count;
> +       u32 val;
> +
> +       tx_count = DIV_ROUND_UP(priv->tx_bytes,
> +                               bytes_per_word(priv->bits_per_word));
> +       tx_count = min(tx_count, SSI_FIFO_DEPTH);
> +
> +       /* set fifo threthold */


Typo.  threshold



> +       val = readl(priv->base + SSI_FC);
> +       val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> +       val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> +       val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> +       writel(val, priv->base + SSI_FC);
> +
> +       while (tx_count--)
> +               uniphier_spi_send(priv);
> +}
> +
> +static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +       u32 val;
> +
> +       val = readl(priv->base + SSI_FPS);
> +
> +       if (enable)
> +               val |= SSI_FPS_FSPOL;
> +       else
> +               val &= ~SSI_FPS_FSPOL;
> +
> +       writel(val, priv->base + SSI_FPS);
> +}
> +
> +static int uniphier_spi_transfer_one(struct spi_master *master,
> +                                    struct spi_device *spi,
> +                                    struct spi_transfer *t)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> +       int status;
> +
> +       status = uniphier_spi_setup_transfer(spi, t);
> +       if (status < 0)
> +               return status;
> +
> +       reinit_completion(&priv->xfer_done);
> +
> +       uniphier_spi_fill_tx_fifo(priv);
> +
> +       uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> +
> +       status = wait_for_completion_timeout(&priv->xfer_done,
> +                                            msecs_to_jiffies(SSI_TIMEOUT));
> +
> +       uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> +
> +       if (status < 0)
> +               return status;
> +
> +       return priv->error;
> +}
> +
> +static int uniphier_spi_prepare_transfer_hardware(struct spi_master *master)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> +
> +       writel(SSI_CTL_EN, priv->base + SSI_CTL);
> +
> +       return 0;
> +}
> +
> +static int uniphier_spi_unprepare_transfer_hardware(struct spi_master *master)
> +{
> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> +
> +       writel(0, priv->base + SSI_CTL);
> +
> +       return 0;
> +}
> +
> +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
> +{
> +       struct uniphier_spi_priv *priv = dev_id;
> +       u32 val, stat;
> +
> +       stat = readl(priv->base + SSI_IS);
> +       val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> +       writel(val, priv->base + SSI_IC);
> +
> +       /* rx fifo overrun */
> +       if (stat & SSI_IS_RORID) {
> +               priv->error = -EIO;
> +               goto done;
> +       }
> +
> +       /* rx complete */
> +       if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
> +               while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
> +                               (priv->rx_bytes - priv->tx_bytes) > 0)
> +                       uniphier_spi_recv(priv);
> +
> +               if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
> +                               (priv->rx_bytes != priv->tx_bytes)) {
> +                       priv->error = -EIO;
> +                       goto done;
> +               } else if (priv->rx_bytes == 0)
> +                       goto done;
> +
> +               /* next tx transfer */
> +               uniphier_spi_fill_tx_fifo(priv);
> +
> +               return IRQ_HANDLED;
> +       }
> +
> +       return IRQ_NONE;
> +
> +done:
> +       complete(&priv->xfer_done);
> +       return IRQ_HANDLED;
> +}
> +
> +static int uniphier_spi_probe(struct platform_device *pdev)
> +{
> +       struct uniphier_spi_priv *priv;
> +       struct spi_master *master;
> +       struct resource *res;
> +       unsigned long clksrc;
> +       int ret;
> +
> +       master = spi_alloc_master(&pdev->dev, sizeof(*priv));
> +       if (!master)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, master);
> +
> +       priv = spi_master_get_devdata(master);
> +       priv->master = master;
> +       priv->is_save_param = false;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(priv->base)) {
> +               ret = PTR_ERR(priv->base);
> +               goto out_master_put;
> +       }
> +
> +       priv->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(priv->clk)) {
> +               dev_err(&pdev->dev, "failed to get clock\n");
> +               ret = PTR_ERR(priv->clk);
> +               goto out_master_put;
> +       }
> +
> +       ret = clk_prepare_enable(priv->clk);
> +       if (ret)
> +               goto out_master_put;
> +
> +       priv->irq = platform_get_irq(pdev, 0);
> +       if (priv->irq < 0) {
> +               dev_err(&pdev->dev, "failed to get IRQ\n");
> +               ret = -ENXIO;
> +               goto out_disable_clk;
> +       }
> +
> +       ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
> +                              0, "uniphier-spi", priv);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to request IRQ\n");
> +               goto out_disable_clk;
> +       }
> +
> +       init_completion(&priv->xfer_done);
> +
> +       clksrc = clk_get_rate(priv->clk);


How about 'clk_rate' ?



> +       master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
> +       master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
> +       master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
> +       master->dev.of_node = pdev->dev.of_node;
> +       master->bus_num = pdev->id;
> +       master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> +
> +       master->set_cs = uniphier_spi_set_cs;
> +       master->transfer_one = uniphier_spi_transfer_one;
> +       master->prepare_transfer_hardware
> +                               = uniphier_spi_prepare_transfer_hardware;
> +       master->unprepare_transfer_hardware
> +                               = uniphier_spi_unprepare_transfer_hardware;
> +       master->num_chipselect = 1;
> +
> +       ret = devm_spi_register_master(&pdev->dev, master);
> +       if (ret)
> +               goto out_disable_clk;
> +
> +       return ret;


I think

          return 0;

will be clearer.


> +
> +out_disable_clk:
> +       clk_disable_unprepare(priv->clk);
> +
> +out_master_put:
> +       spi_master_put(master);
> +       return ret;
> +}
> +
> +static int uniphier_spi_remove(struct platform_device *pdev)
> +{
> +       struct uniphier_spi_priv *priv = platform_get_drvdata(pdev);
> +
> +       clk_disable_unprepare(priv->clk);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id uniphier_spi_match[] = {
> +       { .compatible = "socionext,uniphier-scssi", },
> +       { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
> +
> +static struct platform_driver uniphier_spi_driver = {
> +       .probe = uniphier_spi_probe,
> +       .remove = uniphier_spi_remove,
> +       .driver = {
> +               .name = "uniphier-spi",
> +               .of_match_table = uniphier_spi_match,
> +       },
> +};
> +module_platform_driver(uniphier_spi_driver);
> +
> +MODULE_AUTHOR("Kunihiko Hayashi <[e-mail]>");
> +MODULE_AUTHOR("Keiji Hayashibara <[e-mail]>");
> +MODULE_DESCRIPTION("Socionext UniPhier SPI controller driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Keiji Hayashibara July 26, 2018, 9:38 a.m. UTC | #3

Hello Andy,

Thank you for your check!


> From: Andy Shevchenko [mailto:[e-mail]]
> Sent: Thursday, July 26, 2018 5:46 PM
> To: Hayashibara, Keiji/林原 啓二 <[e-mail]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara <[e-mail]> wrote:
> > Add SPI controller driver implemented in Socionext UniPhier SoCs.
> >
> > UniPhier SoCs have two types SPI controllers; SCSSI supports a single
> > channel, and MCSSI supports multiple channels.
> > This driver supports SCSSI only.
> >
> > This controller has 32bit TX/RX FIFO with depth of eight entry, and
> > supports the SPI master mode only.
> >
> > This commit is implemented in PIO transfer mode, not DMA transfer.
> 
> Few style realted comments.
> 
> > +#include <asm/unaligned.h>
> > +#include <linux/kernel.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spi/spi.h>
> 
> Slightly better to keep them in order and put asm/* at the last.

I see. I will modify this.


> > +#define SSI_TIMEOUT            2000    /* ms */
> 
> SSI_TIMEOUT_MS ?
> 
> > +#define SSI_CTL                        0x0
> 
> Slightly better to keep same width for the addresses, like 0x00 here.
> 
> > +#define SSI_CKS                        0x4
> 
> > +#define SSI_TXWDS              0x8
> 
> > +#define SSI_RXWDS              0xc
> 
> Ditto.

I will modify about above.

> 
> > +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
> > +int speed) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val, ckrat;
> > +
> > +       /*
> > +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > +        * round up as we look for equal or less speed
> > +        */
> > +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> 
> > +       ckrat = roundup(ckrat, 2);
> 
> ckrat += ckrat & 1;
> 
> ?

It's simple. I will modify.


> > +       /* check if requested speed is too small */
> > +       if (ckrat > SSI_MAX_CLK_DIVIDER)
> 
> > +               return -EINVAL;
> 
> So, does this critical?

If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
I don't change it to high frequency, and it is daringly an error.
On the other hand, when changing to low frequency, I will change it automatically.

> > +
> > +       if (ckrat < SSI_MIN_CLK_DIVIDER)
> > +               ckrat = SSI_MIN_CLK_DIVIDER;
> 
> clamp_val()  / max() ?

I will modify it to use max().

> 
> > +       val = readl(priv->base + SSI_CKS);
> > +       val &= ~SSI_CKS_CKRAT_MASK;
> > +       val |= ckrat & SSI_CKS_CKRAT_MASK;
> > +       writel(val, priv->base + SSI_CKS);
> > +
> > +       return 0;
> > +}
> 
> > +       priv->irq = platform_get_irq(pdev, 0);
> > +       if (priv->irq < 0) {
> > +               dev_err(&pdev->dev, "failed to get IRQ\n");
> 
> > +               ret = -ENXIO;
> 
> What's wrong with
> 
> ret = priv->irq;
> 
> ?

I will modify it.

> > +               goto out_disable_clk;
> > +       }
> 
> > +static const struct of_device_id uniphier_spi_match[] = {
> > +       { .compatible = "socionext,uniphier-scssi", },
> 
> > +       { /* sentinel */ },
> 
> Slightly better without comma.

OK. I will modify this.

-----------------
Best Regards,
Keiji Hayashibara



> > +};
> > +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
> 
> --
> With Best Regards,
> Andy Shevchenko

Keiji Hayashibara July 26, 2018, 9:49 a.m. UTC | #4

Hello Yamada-san,


> From: Masahiro Yamada [mailto:[e-mail]]
> Sent: Thursday, July 26, 2018 6:13 PM
> To: Hayashibara, Keiji/林原 啓二 <[e-mail]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> Hi.
> 
> 
> 2018-07-26 16:09 GMT+09:00 Keiji Hayashibara <[e-mail]>:
> > Add SPI controller driver implemented in Socionext UniPhier SoCs.
> >
> > UniPhier SoCs have two types SPI controllers; SCSSI supports a single
> > channel, and MCSSI supports multiple channels.
> > This driver supports SCSSI only.
> >
> > This controller has 32bit TX/RX FIFO with depth of eight entry, and
> > supports the SPI master mode only.
> >
> > This commit is implemented in PIO transfer mode, not DMA transfer.
> >
> > Signed-off-by: Kunihiko Hayashi <[e-mail]>
> > Signed-off-by: Keiji Hayashibara <[e-mail]>
> > ---
> >  drivers/spi/Kconfig        |  13 ++
> >  drivers/spi/Makefile       |   1 +
> >  drivers/spi/spi-uniphier.c | 539
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 553 insertions(+)
> >  create mode 100644 drivers/spi/spi-uniphier.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> > ad5d68e..671d078 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -688,6 +688,19 @@ config SPI_TXX9
> >         help
> >           SPI driver for Toshiba TXx9 MIPS SoCs
> >
> > +config SPI_UNIPHIER
> > +       tristate "Socionext UniPhier SPI Controller"
> > +       depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> > +       help
> > +         This enables a driver for the Socionext UniPhier SoC SCSSI SPI controller.
> > +
> > +         UniPhier SoCs have SCSSI and MCSSI SPI controllers.
> > +         Every UniPhier SoC has SCSSI which supports single channel.
> > +         Older UniPhier Pro4/Pro5 also has MCSSI which support multiple channels.
> > +         This driver supports SCSSI only.
> > +
> > +         If your SoC supports SCSSI, say Y here.
> > +
> >  config SPI_XCOMM
> >         tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
> >         depends on I2C
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
> > cb1f437..a90d559 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -101,6 +101,7 @@ spi-thunderx-objs                   := spi-cavium.o spi-cavium-thunderx.o
> >  obj-$(CONFIG_SPI_THUNDERX)             += spi-thunderx.o
> >  obj-$(CONFIG_SPI_TOPCLIFF_PCH)         += spi-topcliff-pch.o
> >  obj-$(CONFIG_SPI_TXX9)                 += spi-txx9.o
> > +obj-$(CONFIG_SPI_UNIPHIER)             += spi-uniphier.o
> >  obj-$(CONFIG_SPI_XCOMM)                += spi-xcomm.o
> >  obj-$(CONFIG_SPI_XILINX)               += spi-xilinx.o
> >  obj-$(CONFIG_SPI_XLP)                  += spi-xlp.o
> > diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
> > new file mode 100644 index 0000000..2e80bb0
> > --- /dev/null
> > +++ b/drivers/spi/spi-uniphier.c
> > @@ -0,0 +1,539 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// spi-uniphier.c - Socionext UniPhier SPI controller driver
> > +// Copyright 2012      Panasonic Corporation
> > +// Copyright 2016-2018 Socionext Inc.
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/kernel.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define SSI_TIMEOUT            2000    /* ms */
> > +#define SSI_MAX_CLK_DIVIDER    254
> > +#define SSI_MIN_CLK_DIVIDER    4
> > +
> > +struct uniphier_spi_priv {
> > +       void __iomem *base;
> > +       int irq;
> 
> 
> irq is unnecessary because it is only used in probe().
> 
> Use a stack variable.

I see. I will modify it.

> 
> 
> > +       struct clk *clk;
> > +       struct spi_master *master;
> > +       struct completion xfer_done;
> > +
> > +       int error;
> > +       unsigned int tx_bytes;
> > +       unsigned int rx_bytes;
> > +       const u8 *tx_buf;
> > +       u8 *rx_buf;
> > +
> > +       bool is_save_param;
> > +       u8 bits_per_word;
> > +       u16 mode;
> > +       u32 speed_hz;
> > +};
> > +
> > +#define SSI_CTL                        0x0
> > +#define   SSI_CTL_EN           BIT(0)
> > +
> > +#define SSI_CKS                        0x4
> > +#define   SSI_CKS_CKRAT_MASK   GENMASK(7, 0)
> > +#define   SSI_CKS_CKPHS                BIT(14)
> > +#define   SSI_CKS_CKINIT       BIT(13)
> > +#define   SSI_CKS_CKDLY                BIT(12)
> > +
> > +#define SSI_TXWDS              0x8
> > +#define   SSI_TXWDS_WDLEN_MASK GENMASK(13, 8)
> > +#define   SSI_TXWDS_TDTF_MASK  GENMASK(7, 6)
> > +#define   SSI_TXWDS_DTLEN_MASK GENMASK(5, 0)
> > +
> > +#define SSI_RXWDS              0xc
> > +#define   SSI_RXWDS_DTLEN_MASK GENMASK(5, 0)
> > +
> > +#define SSI_FPS                        0x10
> > +#define   SSI_FPS_FSPOL                BIT(15)
> > +#define   SSI_FPS_FSTRT                BIT(14)
> > +
> > +#define SSI_SR                 0x14
> > +#define   SSI_SR_RNE           BIT(0)
> > +
> > +#define SSI_IE                 0x18
> > +#define   SSI_IE_RCIE          BIT(3)
> > +#define   SSI_IE_RORIE         BIT(0)
> > +
> > +#define SSI_IS                 0x1c
> > +#define   SSI_IS_RXRS          BIT(9)
> > +#define   SSI_IS_RCID          BIT(3)
> > +#define   SSI_IS_RORID         BIT(0)
> > +
> > +#define SSI_IC                 0x1c
> > +#define   SSI_IC_TCIC          BIT(4)
> > +#define   SSI_IC_RCIC          BIT(3)
> > +#define   SSI_IC_RORIC         BIT(0)
> > +
> > +#define SSI_FC                 0x20
> > +#define   SSI_FC_TXFFL         BIT(12)
> > +#define   SSI_FC_TXFTH_MASK    GENMASK(11, 8)
> > +#define   SSI_FC_RXFFL         BIT(4)
> > +#define   SSI_FC_RXFTH_MASK    GENMASK(3, 0)
> > +
> > +#define SSI_TXDR               0x24
> > +#define SSI_RXDR               0x24
> > +
> > +#define SSI_FIFO_DEPTH         8U
> > +
> > +static inline unsigned int bytes_per_word(unsigned int bits) {
> > +       return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4); }
> > +
> > +static inline void uniphier_spi_irq_enable(struct spi_device *spi,
> > +u32 mask) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val;
> > +
> > +       val = readl(priv->base + SSI_IE);
> > +       val |= mask;
> > +       writel(val, priv->base + SSI_IE); }
> > +
> > +static inline void uniphier_spi_irq_disable(struct spi_device *spi,
> > +u32 mask) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val;
> > +
> > +       val = readl(priv->base + SSI_IE);
> > +       val &= ~mask;
> > +       writel(val, priv->base + SSI_IE); }
> > +
> > +static void uniphier_spi_set_mode(struct spi_device *spi) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val1, val2;
> > +
> > +       /*
> > +        * clock setting
> > +        * CKPHS    capture timing. 0:rising edge, 1:falling edge
> > +        * CKINIT   clock initial level. 0:low, 1:high
> > +        * CKDLY    clock delay. 0:no delay, 1:delay depending on FSTRT
> > +        *          (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
> > +        *
> > +        * frame setting
> > +        * FSPOL    frame signal porarity. 0: low, 1: high
> > +        * FSTRT    start frame timing
> > +        *          0: rising edge of clock, 1: falling edge of clock
> > +        */
> > +       switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
> > +       case SPI_MODE_0:
> > +               /* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
> > +               val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
> > +               val2 = 0;
> > +               break;
> > +       case SPI_MODE_1:
> > +               /* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
> > +               val1 = 0;
> > +               val2 = SSI_FPS_FSTRT;
> > +               break;
> > +       case SPI_MODE_2:
> > +               /* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
> > +               val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
> > +               val2 = SSI_FPS_FSTRT;
> > +               break;
> > +       case SPI_MODE_3:
> > +               /* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
> > +               val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
> > +               val2 = 0;
> > +               break;
> > +       }
> > +
> > +       if (!(spi->mode & SPI_CS_HIGH))
> > +               val2 |= SSI_FPS_FSPOL;
> > +
> > +       writel(val1, priv->base + SSI_CKS);
> > +       writel(val2, priv->base + SSI_FPS);
> > +
> > +       val1 = 0;
> > +       if (spi->mode & SPI_LSB_FIRST)
> > +               val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> > +       writel(val1, priv->base + SSI_TXWDS);
> > +       writel(val1, priv->base + SSI_RXWDS); }
> > +
> > +static void uniphier_spi_set_transfer_size(struct spi_device *spi,
> > +int size) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val;
> > +
> > +       val = readl(priv->base + SSI_TXWDS);
> > +       val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> > +       val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> > +       val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> > +       writel(val, priv->base + SSI_TXWDS);
> > +
> > +       val = readl(priv->base + SSI_RXWDS);
> > +       val &= ~SSI_RXWDS_DTLEN_MASK;
> > +       val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> > +       writel(val, priv->base + SSI_RXWDS); }
> > +
> > +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
> > +int speed) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val, ckrat;
> > +
> > +       /*
> > +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > +        * round up as we look for equal or less speed
> > +        */
> > +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > +       ckrat = roundup(ckrat, 2);
> > +
> > +       /* check if requested speed is too small */
> > +       if (ckrat > SSI_MAX_CLK_DIVIDER)
> > +               return -EINVAL;
> > +
> > +       if (ckrat < SSI_MIN_CLK_DIVIDER)
> > +               ckrat = SSI_MIN_CLK_DIVIDER;
> > +
> > +       val = readl(priv->base + SSI_CKS);
> > +       val &= ~SSI_CKS_CKRAT_MASK;
> > +       val |= ckrat & SSI_CKS_CKRAT_MASK;
> > +       writel(val, priv->base + SSI_CKS);
> > +
> > +       return 0;
> > +}
> > +
> > +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> > +                                      struct spi_transfer *t) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val;
> > +       int ret;
> > +
> > +       priv->error = 0;
> > +       priv->tx_buf = t->tx_buf;
> > +       priv->rx_buf = t->rx_buf;
> > +       priv->tx_bytes = priv->rx_bytes = t->len;
> > +
> > +       if (!priv->is_save_param || priv->mode != spi->mode) {
> > +               uniphier_spi_set_mode(spi);
> > +               priv->mode = spi->mode;
> > +       }
> > +
> > +       if (!priv->is_save_param || priv->bits_per_word != t->bits_per_word) {
> > +               uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> > +               priv->bits_per_word = t->bits_per_word;
> > +       }
> > +
> > +       if (!priv->is_save_param || priv->speed_hz != t->speed_hz) {
> > +               ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> > +               if (ret)
> > +                       return ret;
> > +               priv->speed_hz = t->speed_hz;
> > +       }
> > +
> > +       if (!priv->is_save_param)
> > +               priv->is_save_param = true;
> > +
> > +       /* reset FIFOs */
> > +       val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> > +       writel(val, priv->base + SSI_FC);
> > +
> > +       return 0;
> > +}
> > +
> > +static void uniphier_spi_send(struct uniphier_spi_priv *priv) {
> > +       int wsize;
> > +       u32 val = 0;
> > +
> > +       wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
> > +       priv->tx_bytes -= wsize;
> > +
> > +       if (priv->tx_buf) {
> > +               switch (wsize) {
> > +               case 1:
> > +                       val = *priv->tx_buf;
> > +                       break;
> > +               case 2:
> > +                       val = get_unaligned_le16(priv->tx_buf);
> > +                       break;
> > +               case 4:
> > +                       val = get_unaligned_le32(priv->tx_buf);
> > +                       break;
> > +               }
> > +
> > +               priv->tx_buf += wsize;
> > +       }
> > +
> > +       writel(val, priv->base + SSI_TXDR); }
> > +
> > +static void uniphier_spi_recv(struct uniphier_spi_priv *priv) {
> > +       int rsize;
> > +       u32 val;
> > +
> > +       rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
> > +       priv->rx_bytes -= rsize;
> > +
> > +       val = readl(priv->base + SSI_RXDR);
> > +
> > +       if (priv->rx_buf) {
> > +               switch (rsize) {
> > +               case 1:
> > +                       *priv->rx_buf = (u8)val;
> 
> Is this cast necessary?

I added it explicitly, but I will modify it.


> 
> > +                       break;
> > +               case 2:
> > +                       put_unaligned_le16(val, priv->rx_buf);
> > +                       break;
> > +               case 4:
> > +                       put_unaligned_le32(val, priv->rx_buf);
> > +                       break;
> > +               }
> > +
> > +               priv->rx_buf += rsize;
> > +       }
> > +}
> > +
> > +static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
> > +{
> > +       unsigned int tx_count;
> > +       u32 val;
> > +
> > +       tx_count = DIV_ROUND_UP(priv->tx_bytes,
> > +                               bytes_per_word(priv->bits_per_word));
> > +       tx_count = min(tx_count, SSI_FIFO_DEPTH);
> > +
> > +       /* set fifo threthold */
> 
> 
> Typo.  threshold

I will modify it.

> 
> 
> > +       val = readl(priv->base + SSI_FC);
> > +       val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> > +       val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> > +       val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> > +       writel(val, priv->base + SSI_FC);
> > +
> > +       while (tx_count--)
> > +               uniphier_spi_send(priv); }
> > +
> > +static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
> > +{
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +       u32 val;
> > +
> > +       val = readl(priv->base + SSI_FPS);
> > +
> > +       if (enable)
> > +               val |= SSI_FPS_FSPOL;
> > +       else
> > +               val &= ~SSI_FPS_FSPOL;
> > +
> > +       writel(val, priv->base + SSI_FPS); }
> > +
> > +static int uniphier_spi_transfer_one(struct spi_master *master,
> > +                                    struct spi_device *spi,
> > +                                    struct spi_transfer *t) {
> > +       struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
> > +       int status;
> > +
> > +       status = uniphier_spi_setup_transfer(spi, t);
> > +       if (status < 0)
> > +               return status;
> > +
> > +       reinit_completion(&priv->xfer_done);
> > +
> > +       uniphier_spi_fill_tx_fifo(priv);
> > +
> > +       uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> > +
> > +       status = wait_for_completion_timeout(&priv->xfer_done,
> > +
> > + msecs_to_jiffies(SSI_TIMEOUT));
> > +
> > +       uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
> > +
> > +       if (status < 0)
> > +               return status;
> > +
> > +       return priv->error;
> > +}
> > +
> > +static int uniphier_spi_prepare_transfer_hardware(struct spi_master
> > +*master) {
> > +       struct uniphier_spi_priv *priv =
> > +spi_master_get_devdata(master);
> > +
> > +       writel(SSI_CTL_EN, priv->base + SSI_CTL);
> > +
> > +       return 0;
> > +}
> > +
> > +static int uniphier_spi_unprepare_transfer_hardware(struct spi_master
> > +*master) {
> > +       struct uniphier_spi_priv *priv =
> > +spi_master_get_devdata(master);
> > +
> > +       writel(0, priv->base + SSI_CTL);
> > +
> > +       return 0;
> > +}
> > +
> > +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id) {
> > +       struct uniphier_spi_priv *priv = dev_id;
> > +       u32 val, stat;
> > +
> > +       stat = readl(priv->base + SSI_IS);
> > +       val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> > +       writel(val, priv->base + SSI_IC);
> > +
> > +       /* rx fifo overrun */
> > +       if (stat & SSI_IS_RORID) {
> > +               priv->error = -EIO;
> > +               goto done;
> > +       }
> > +
> > +       /* rx complete */
> > +       if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
> > +               while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
> > +                               (priv->rx_bytes - priv->tx_bytes) > 0)
> > +                       uniphier_spi_recv(priv);
> > +
> > +               if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
> > +                               (priv->rx_bytes != priv->tx_bytes)) {
> > +                       priv->error = -EIO;
> > +                       goto done;
> > +               } else if (priv->rx_bytes == 0)
> > +                       goto done;
> > +
> > +               /* next tx transfer */
> > +               uniphier_spi_fill_tx_fifo(priv);
> > +
> > +               return IRQ_HANDLED;
> > +       }
> > +
> > +       return IRQ_NONE;
> > +
> > +done:
> > +       complete(&priv->xfer_done);
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int uniphier_spi_probe(struct platform_device *pdev) {
> > +       struct uniphier_spi_priv *priv;
> > +       struct spi_master *master;
> > +       struct resource *res;
> > +       unsigned long clksrc;
> > +       int ret;
> > +
> > +       master = spi_alloc_master(&pdev->dev, sizeof(*priv));
> > +       if (!master)
> > +               return -ENOMEM;
> > +
> > +       platform_set_drvdata(pdev, master);
> > +
> > +       priv = spi_master_get_devdata(master);
> > +       priv->master = master;
> > +       priv->is_save_param = false;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       priv->base = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(priv->base)) {
> > +               ret = PTR_ERR(priv->base);
> > +               goto out_master_put;
> > +       }
> > +
> > +       priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(priv->clk)) {
> > +               dev_err(&pdev->dev, "failed to get clock\n");
> > +               ret = PTR_ERR(priv->clk);
> > +               goto out_master_put;
> > +       }
> > +
> > +       ret = clk_prepare_enable(priv->clk);
> > +       if (ret)
> > +               goto out_master_put;
> > +
> > +       priv->irq = platform_get_irq(pdev, 0);
> > +       if (priv->irq < 0) {
> > +               dev_err(&pdev->dev, "failed to get IRQ\n");
> > +               ret = -ENXIO;
> > +               goto out_disable_clk;
> > +       }
> > +
> > +       ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
> > +                              0, "uniphier-spi", priv);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "failed to request IRQ\n");
> > +               goto out_disable_clk;
> > +       }
> > +
> > +       init_completion(&priv->xfer_done);
> > +
> > +       clksrc = clk_get_rate(priv->clk);
> 
> 
> How about 'clk_rate' ?

OK. I will replace clksrc to clk_rate.

> 
> 
> > +       master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
> > +       master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
> > +       master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
> > +       master->dev.of_node = pdev->dev.of_node;
> > +       master->bus_num = pdev->id;
> > +       master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> > +
> > +       master->set_cs = uniphier_spi_set_cs;
> > +       master->transfer_one = uniphier_spi_transfer_one;
> > +       master->prepare_transfer_hardware
> > +                               = uniphier_spi_prepare_transfer_hardware;
> > +       master->unprepare_transfer_hardware
> > +                               = uniphier_spi_unprepare_transfer_hardware;
> > +       master->num_chipselect = 1;
> > +
> > +       ret = devm_spi_register_master(&pdev->dev, master);
> > +       if (ret)
> > +               goto out_disable_clk;
> > +
> > +       return ret;
> 
> 
> I think
> 
>           return 0;
> 
> will be clearer.

I think so. I will modify it.

Thank you.

-----------------
Best Regards,
Keiji Hayashibara

Radu Pirea July 26, 2018, 10:57 a.m. UTC | #5

On 07/26/2018 12:38 PM, Keiji Hayashibara wrote:
> Hello Andy,
> 
> Thank you for your check!
> 
> 
>> From: Andy Shevchenko [mailto:[e-mail]]
>> Sent: Thursday, July 26, 2018 5:46 PM
>> To: Hayashibara, Keiji/林原 啓二 <[e-mail]>
>> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>>
>> On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara <[e-mail]> wrote:
>>> Add SPI controller driver implemented in Socionext UniPhier SoCs.
>>>
>>> UniPhier SoCs have two types SPI controllers; SCSSI supports a single
>>> channel, and MCSSI supports multiple channels.
>>> This driver supports SCSSI only.
>>>
>>> This controller has 32bit TX/RX FIFO with depth of eight entry, and
>>> supports the SPI master mode only.
>>>
>>> This commit is implemented in PIO transfer mode, not DMA transfer.
>>
>> Few style realted comments.
>>
>>> +#include <asm/unaligned.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/spi/spi.h>
>>
>> Slightly better to keep them in order and put asm/* at the last.
> 
> I see. I will modify this.
> 
> 
>>> +#define SSI_TIMEOUT            2000    /* ms */
>>
>> SSI_TIMEOUT_MS ?
>>
>>> +#define SSI_CTL                        0x0
>>
>> Slightly better to keep same width for the addresses, like 0x00 here.
>>
>>> +#define SSI_CKS                        0x4
>>
>>> +#define SSI_TXWDS              0x8
>>
>>> +#define SSI_RXWDS              0xc
>>
>> Ditto.
> 
> I will modify about above.
> 
>>
>>> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
>>> +int speed) {
>>> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
>>> +       u32 val, ckrat;
>>> +
>>> +       /*
>>> +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
>>> +        * round up as we look for equal or less speed
>>> +        */
>>> +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
>>
>>> +       ckrat = roundup(ckrat, 2);
>>
>> ckrat += ckrat & 1;
>>
>> ?
> 
> It's simple. I will modify.
> 
> 
>>> +       /* check if requested speed is too small */
>>> +       if (ckrat > SSI_MAX_CLK_DIVIDER)
>>
>>> +               return -EINVAL;
>>
>> So, does this critical?
> 
> If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> I don't change it to high frequency, and it is daringly an error.
> On the other hand, when changing to low frequency, I will change it automatically.
> 
>>> +
>>> +       if (ckrat < SSI_MIN_CLK_DIVIDER)
>>> +               ckrat = SSI_MIN_CLK_DIVIDER;

In fact you don't need this checks. You already set in probe function this.
master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);

The SPI core will check if transfer speed is higher than controller 
speed and if is, will set the transfer speed to master->max_speed_hz. In 
case of master->min_speed_hz, if transfer speed is lower than 
master->min_speed_hz __spi_validate(drivers/spi/spi.c) will return -EINVAL.

>>
>> clamp_val()  / max() ?
> 
> I will modify it to use max().
> 
>>
>>> +       val = readl(priv->base + SSI_CKS);
>>> +       val &= ~SSI_CKS_CKRAT_MASK;
>>> +       val |= ckrat & SSI_CKS_CKRAT_MASK;
>>> +       writel(val, priv->base + SSI_CKS);
>>> +
>>> +       return 0;
>>> +}
>>
>>> +       priv->irq = platform_get_irq(pdev, 0);
>>> +       if (priv->irq < 0) {
>>> +               dev_err(&pdev->dev, "failed to get IRQ\n");
>>
>>> +               ret = -ENXIO;
>>
>> What's wrong with
>>
>> ret = priv->irq;
>>
>> ?
> 
> I will modify it.
> 
>>> +               goto out_disable_clk;
>>> +       }
>>
>>> +static const struct of_device_id uniphier_spi_match[] = {
>>> +       { .compatible = "socionext,uniphier-scssi", },
>>
>>> +       { /* sentinel */ },
>>
>> Slightly better without comma.
> 
> OK. I will modify this.
> 
> -----------------
> Best Regards,
> Keiji Hayashibara
> 
> 
> 
>>> +};
>>> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Andy Shevchenko July 26, 2018, 1:44 p.m. UTC | #6

On Thu, Jul 26, 2018 at 12:38 PM, Keiji Hayashibara
<[e-mail]> wrote:

>> > +       /* check if requested speed is too small */
>> > +       if (ckrat > SSI_MAX_CLK_DIVIDER)
>>
>> > +               return -EINVAL;
>>
>> So, does this critical?
>
> If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> I don't change it to high frequency, and it is daringly an error.
> On the other hand, when changing to low frequency, I will change it automatically.

No. My point is, if somehow user asks for that condition to be
happened you bail out, while when using clamp_val() you may continue
to work at maximum limit, though issue warning to user, for example.

>> > +
>> > +       if (ckrat < SSI_MIN_CLK_DIVIDER)
>> > +               ckrat = SSI_MIN_CLK_DIVIDER;
>>
>> clamp_val()  / max() ?
>
> I will modify it to use max().

See above.

Trent Piepho July 26, 2018, 5:01 p.m. UTC | #7

On Thu, 2018-07-26 at 11:46 +0300, Andy Shevchenko wrote:
> > +
> > +       /*
> > +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > +        * round up as we look for equal or less speed
> > +        */
> > +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > +       ckrat = roundup(ckrat, 2);
> 
> ckrat += ckrat & 1;

Either way, the compiler produces the same code:

 add r0, r0, #1
 bic r0, r0, #1

I.e., ckrat = (ckrat + 1) & ~1, one "add" and one "and".

Might as well use the macro so it's clear. There is also round_up(x,y),
which I believe is intended to be used only with power of 2 values of
y.

Keiji Hayashibara July 30, 2018, 1:15 a.m. UTC | #8

Hello Radu,

> From: Radu Pirea [mailto:[e-mail]]
> Sent: Thursday, July 26, 2018 7:58 PM
> To: Hayashibara, Keiji/林原 啓二 <[e-mail]>; 'Andy Shevchenko'
> <[e-mail]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> 
> 
> On 07/26/2018 12:38 PM, Keiji Hayashibara wrote:
> > Hello Andy,
> >
> > Thank you for your check!
> >
> >
> >> From: Andy Shevchenko [mailto:[e-mail]]
> >> Sent: Thursday, July 26, 2018 5:46 PM
> >> To: Hayashibara, Keiji/林原 啓二 <[e-mail]>
> >> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for
> >> UniPhier SoC
> >>
> >> On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara <[e-mail]> wrote:
> >>> Add SPI controller driver implemented in Socionext UniPhier SoCs.
> >>>
> >>> UniPhier SoCs have two types SPI controllers; SCSSI supports a
> >>> single channel, and MCSSI supports multiple channels.
> >>> This driver supports SCSSI only.
> >>>
> >>> This controller has 32bit TX/RX FIFO with depth of eight entry, and
> >>> supports the SPI master mode only.
> >>>
> >>> This commit is implemented in PIO transfer mode, not DMA transfer.
> >>
> >> Few style realted comments.
> >>
> >>> +#include <asm/unaligned.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/bitfield.h>
> >>> +#include <linux/bitops.h>
> >>> +#include <linux/clk.h>
> >>> +#include <linux/interrupt.h>
> >>> +#include <linux/io.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/of.h>
> >>> +#include <linux/of_platform.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/spi/spi.h>
> >>
> >> Slightly better to keep them in order and put asm/* at the last.
> >
> > I see. I will modify this.
> >
> >
> >>> +#define SSI_TIMEOUT            2000    /* ms */
> >>
> >> SSI_TIMEOUT_MS ?
> >>
> >>> +#define SSI_CTL                        0x0
> >>
> >> Slightly better to keep same width for the addresses, like 0x00 here.
> >>
> >>> +#define SSI_CKS                        0x4
> >>
> >>> +#define SSI_TXWDS              0x8
> >>
> >>> +#define SSI_RXWDS              0xc
> >>
> >> Ditto.
> >
> > I will modify about above.
> >
> >>
> >>> +static int uniphier_spi_set_baudrate(struct spi_device *spi,
> >>> +unsigned int speed) {
> >>> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> >>> +       u32 val, ckrat;
> >>> +
> >>> +       /*
> >>> +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> >>> +        * round up as we look for equal or less speed
> >>> +        */
> >>> +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> >>
> >>> +       ckrat = roundup(ckrat, 2);
> >>
> >> ckrat += ckrat & 1;
> >>
> >> ?
> >
> > It's simple. I will modify.
> >
> >
> >>> +       /* check if requested speed is too small */
> >>> +       if (ckrat > SSI_MAX_CLK_DIVIDER)
> >>
> >>> +               return -EINVAL;
> >>
> >> So, does this critical?
> >
> > If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> > I don't change it to high frequency, and it is daringly an error.
> > On the other hand, when changing to low frequency, I will change it automatically.
> >
> >>> +
> >>> +       if (ckrat < SSI_MIN_CLK_DIVIDER)
> >>> +               ckrat = SSI_MIN_CLK_DIVIDER;
> 
> In fact you don't need this checks. You already set in probe function this.
> master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
> master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
> 
> The SPI core will check if transfer speed is higher than controller speed and if is, will set the transfer speed
> to master->max_speed_hz. In case of master->min_speed_hz, if transfer speed is lower than
> master->min_speed_hz __spi_validate(drivers/spi/spi.c) will return -EINVAL.

I see.
I confirmed __spi_validate() and understood that this check code is unnecessary.
I will remove this check code.

Thank you.

> >>
> >> clamp_val()  / max() ?
> >
> > I will modify it to use max().
> >
> >>
> >>> +       val = readl(priv->base + SSI_CKS);
> >>> +       val &= ~SSI_CKS_CKRAT_MASK;
> >>> +       val |= ckrat & SSI_CKS_CKRAT_MASK;
> >>> +       writel(val, priv->base + SSI_CKS);
> >>> +
> >>> +       return 0;
> >>> +}
> >>
> >>> +       priv->irq = platform_get_irq(pdev, 0);
> >>> +       if (priv->irq < 0) {
> >>> +               dev_err(&pdev->dev, "failed to get IRQ\n");
> >>
> >>> +               ret = -ENXIO;
> >>
> >> What's wrong with
> >>
> >> ret = priv->irq;
> >>
> >> ?
> >
> > I will modify it.
> >
> >>> +               goto out_disable_clk;
> >>> +       }
> >>
> >>> +static const struct of_device_id uniphier_spi_match[] = {
> >>> +       { .compatible = "socionext,uniphier-scssi", },
> >>
> >>> +       { /* sentinel */ },
> >>
> >> Slightly better without comma.
> >
> > OK. I will modify this.
> >
> > -----------------
> > Best Regards,
> > Keiji Hayashibara
> >
> >
> >
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
> >>
> >> --
> >> With Best Regards,
> >> Andy Shevchenko
> >


-----------------
Best Regards,
Keiji Hayashibara

Keiji Hayashibara July 30, 2018, 1:46 a.m. UTC | #9

Hello Andy,

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf Of Andy
> Shevchenko
> Sent: Thursday, July 26, 2018 10:44 PM
> To: Hayashibara, Keiji/林原 啓二 <[e-mail]>
> Cc: Mark Brown <[e-mail]>; Rob Herring <[e-mail]>; Mark Rutland <[e-mail]>;
> Yamada, Masahiro/山田 真弘 <[e-mail]>; linux-spi <[e-mail]>; linux-arm
> Mailing List <[e-mail]>; devicetree <[e-mail]>; Masami Hiramatsu
> <[e-mail]>; Jassi Brar <[e-mail]>; Linux Kernel Mailing List
> <[e-mail]>; Hayashi, Kunihiko/林 邦彦 <[e-mail]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, Jul 26, 2018 at 12:38 PM, Keiji Hayashibara <[e-mail]> wrote:
> 
> >> > +       /* check if requested speed is too small */
> >> > +       if (ckrat > SSI_MAX_CLK_DIVIDER)
> >>
> >> > +               return -EINVAL;
> >>
> >> So, does this critical?
> >
> > If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> > I don't change it to high frequency, and it is daringly an error.
> > On the other hand, when changing to low frequency, I will change it automatically.
> 
> No. My point is, if somehow user asks for that condition to be happened you bail out, while when using clamp_val()
> you may continue to work at maximum limit, though issue warning to user, for example.

Thank you. I understood what you pointed out. 
However, I found this code unnecessary as pointed out by Radu, 
so I will delete it.

> >> > +
> >> > +       if (ckrat < SSI_MIN_CLK_DIVIDER)
> >> > +               ckrat = SSI_MIN_CLK_DIVIDER;
> >>
> >> clamp_val()  / max() ?
> >
> > I will modify it to use max().
> 
> See above.

This is also the same as above.

> --
> With Best Regards,
> Andy Shevchenko

-----------------
Best Regards,
Keiji Hayashibara

Keiji Hayashibara July 30, 2018, 5:30 a.m. UTC | #10

Hello Trent and Andy,

> From: Trent Piepho [mailto:[e-mail]]
> Sent: Friday, July 27, 2018 2:02 AM
> To: [e-mail]; Hayashibara, Keiji/林原 啓二 <[e-mail]>
> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, 2018-07-26 at 11:46 +0300, Andy Shevchenko wrote:
> > > +
> > > +       /*
> > > +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > > +        * round up as we look for equal or less speed
> > > +        */
> > > +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > > +       ckrat = roundup(ckrat, 2);
> >
> > ckrat += ckrat & 1;
> 
> Either way, the compiler produces the same code:
> 
>  add r0, r0, #1 
>  bic r0, r0, #1
> 
> I.e., ckrat = (ckrat + 1) & ~1, one "add" and one "and".
> 
> Might as well use the macro so it's clear. There is also round_up(x,y), which I believe is intended to be used
> only with power of 2 values of y.

I confirmed round_up() macro.
This macro is optimized to power of 2 argument,
and it matches to this case and clear.

I will replace to round_up() macro.

Thank you.

-----------------
Best Regards,
Keiji Hayashibara

Andy Shevchenko July 30, 2018, 8:39 a.m. UTC | #11

On Mon, Jul 30, 2018 at 8:30 AM, Keiji Hayashibara
<[e-mail]> wrote:
> Hello Trent and Andy,

>> > > +       ckrat = roundup(ckrat, 2);
>> >
>> > ckrat += ckrat & 1;
>>
>> Either way, the compiler produces the same code:
>>
>>  add r0, r0, #1
>>  bic r0, r0, #1
>>
>> I.e., ckrat = (ckrat + 1) & ~1, one "add" and one "and".
>>
>> Might as well use the macro so it's clear. There is also round_up(x,y), which I believe is intended to be used
>> only with power of 2 values of y.
>
> I confirmed round_up() macro.
> This macro is optimized to power of 2 argument,
> and it matches to this case and clear.
>
> I will replace to round_up() macro.

Good!

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e..671d078 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -688,6 +688,19 @@  config SPI_TXX9
 	help
 	  SPI driver for Toshiba TXx9 MIPS SoCs
 
+config SPI_UNIPHIER
+	tristate "Socionext UniPhier SPI Controller"
+	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
+	help
+	  This enables a driver for the Socionext UniPhier SoC SCSSI SPI controller.
+
+	  UniPhier SoCs have SCSSI and MCSSI SPI controllers.
+	  Every UniPhier SoC has SCSSI which supports single channel.
+	  Older UniPhier Pro4/Pro5 also has MCSSI which support multiple channels.
+	  This driver supports SCSSI only.
+
+	  If your SoC supports SCSSI, say Y here.
+
 config SPI_XCOMM
 	tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
 	depends on I2C
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f437..a90d559 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -101,6 +101,7 @@  spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
 obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
 obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
 obj-$(CONFIG_SPI_TXX9)			+= spi-txx9.o
+obj-$(CONFIG_SPI_UNIPHIER)		+= spi-uniphier.o
 obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
 obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
 obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
new file mode 100644
index 0000000..2e80bb0
--- /dev/null
+++ b/drivers/spi/spi-uniphier.c
@@ -0,0 +1,539 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// spi-uniphier.c - Socionext UniPhier SPI controller driver
+// Copyright 2012      Panasonic Corporation
+// Copyright 2016-2018 Socionext Inc.
+
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+
+#define SSI_TIMEOUT		2000	/* ms */
+#define SSI_MAX_CLK_DIVIDER	254
+#define SSI_MIN_CLK_DIVIDER	4
+
+struct uniphier_spi_priv {
+	void __iomem *base;
+	int irq;
+	struct clk *clk;
+	struct spi_master *master;
+	struct completion xfer_done;
+
+	int error;
+	unsigned int tx_bytes;
+	unsigned int rx_bytes;
+	const u8 *tx_buf;
+	u8 *rx_buf;
+
+	bool is_save_param;
+	u8 bits_per_word;
+	u16 mode;
+	u32 speed_hz;
+};
+
+#define SSI_CTL			0x0
+#define   SSI_CTL_EN		BIT(0)
+
+#define SSI_CKS			0x4
+#define   SSI_CKS_CKRAT_MASK	GENMASK(7, 0)
+#define   SSI_CKS_CKPHS		BIT(14)
+#define   SSI_CKS_CKINIT	BIT(13)
+#define   SSI_CKS_CKDLY		BIT(12)
+
+#define SSI_TXWDS		0x8
+#define   SSI_TXWDS_WDLEN_MASK	GENMASK(13, 8)
+#define   SSI_TXWDS_TDTF_MASK	GENMASK(7, 6)
+#define   SSI_TXWDS_DTLEN_MASK	GENMASK(5, 0)
+
+#define SSI_RXWDS		0xc
+#define   SSI_RXWDS_DTLEN_MASK	GENMASK(5, 0)
+
+#define SSI_FPS			0x10
+#define   SSI_FPS_FSPOL		BIT(15)
+#define   SSI_FPS_FSTRT		BIT(14)
+
+#define SSI_SR			0x14
+#define   SSI_SR_RNE		BIT(0)
+
+#define SSI_IE			0x18
+#define   SSI_IE_RCIE		BIT(3)
+#define   SSI_IE_RORIE		BIT(0)
+
+#define SSI_IS			0x1c
+#define   SSI_IS_RXRS		BIT(9)
+#define   SSI_IS_RCID		BIT(3)
+#define   SSI_IS_RORID		BIT(0)
+
+#define SSI_IC			0x1c
+#define   SSI_IC_TCIC		BIT(4)
+#define   SSI_IC_RCIC		BIT(3)
+#define   SSI_IC_RORIC		BIT(0)
+
+#define SSI_FC			0x20
+#define   SSI_FC_TXFFL		BIT(12)
+#define   SSI_FC_TXFTH_MASK	GENMASK(11, 8)
+#define   SSI_FC_RXFFL		BIT(4)
+#define   SSI_FC_RXFTH_MASK	GENMASK(3, 0)
+
+#define SSI_TXDR		0x24
+#define SSI_RXDR		0x24
+
+#define SSI_FIFO_DEPTH		8U
+
+static inline unsigned int bytes_per_word(unsigned int bits)
+{
+	return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4);
+}
+
+static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_IE);
+	val |= mask;
+	writel(val, priv->base + SSI_IE);
+}
+
+static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_IE);
+	val &= ~mask;
+	writel(val, priv->base + SSI_IE);
+}
+
+static void uniphier_spi_set_mode(struct spi_device *spi)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val1, val2;
+
+	/*
+	 * clock setting
+	 * CKPHS    capture timing. 0:rising edge, 1:falling edge
+	 * CKINIT   clock initial level. 0:low, 1:high
+	 * CKDLY    clock delay. 0:no delay, 1:delay depending on FSTRT
+	 *          (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
+	 *
+	 * frame setting
+	 * FSPOL    frame signal porarity. 0: low, 1: high
+	 * FSTRT    start frame timing
+	 *          0: rising edge of clock, 1: falling edge of clock
+	 */
+	switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
+	case SPI_MODE_0:
+		/* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
+		val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
+		val2 = 0;
+		break;
+	case SPI_MODE_1:
+		/* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
+		val1 = 0;
+		val2 = SSI_FPS_FSTRT;
+		break;
+	case SPI_MODE_2:
+		/* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
+		val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
+		val2 = SSI_FPS_FSTRT;
+		break;
+	case SPI_MODE_3:
+		/* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
+		val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
+		val2 = 0;
+		break;
+	}
+
+	if (!(spi->mode & SPI_CS_HIGH))
+		val2 |= SSI_FPS_FSPOL;
+
+	writel(val1, priv->base + SSI_CKS);
+	writel(val2, priv->base + SSI_FPS);
+
+	val1 = 0;
+	if (spi->mode & SPI_LSB_FIRST)
+		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
+	writel(val1, priv->base + SSI_TXWDS);
+	writel(val1, priv->base + SSI_RXWDS);
+}
+
+static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_TXWDS);
+	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
+	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
+	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
+	writel(val, priv->base + SSI_TXWDS);
+
+	val = readl(priv->base + SSI_RXWDS);
+	val &= ~SSI_RXWDS_DTLEN_MASK;
+	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
+	writel(val, priv->base + SSI_RXWDS);
+}
+
+static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val, ckrat;
+
+	/*
+	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
+	 * round up as we look for equal or less speed
+	 */
+	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
+	ckrat = roundup(ckrat, 2);
+
+	/* check if requested speed is too small */
+	if (ckrat > SSI_MAX_CLK_DIVIDER)
+		return -EINVAL;
+
+	if (ckrat < SSI_MIN_CLK_DIVIDER)
+		ckrat = SSI_MIN_CLK_DIVIDER;
+
+	val = readl(priv->base + SSI_CKS);
+	val &= ~SSI_CKS_CKRAT_MASK;
+	val |= ckrat & SSI_CKS_CKRAT_MASK;
+	writel(val, priv->base + SSI_CKS);
+
+	return 0;
+}
+
+static int uniphier_spi_setup_transfer(struct spi_device *spi,
+				       struct spi_transfer *t)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+	int ret;
+
+	priv->error = 0;
+	priv->tx_buf = t->tx_buf;
+	priv->rx_buf = t->rx_buf;
+	priv->tx_bytes = priv->rx_bytes = t->len;
+
+	if (!priv->is_save_param || priv->mode != spi->mode) {
+		uniphier_spi_set_mode(spi);
+		priv->mode = spi->mode;
+	}
+
+	if (!priv->is_save_param || priv->bits_per_word != t->bits_per_word) {
+		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
+		priv->bits_per_word = t->bits_per_word;
+	}
+
+	if (!priv->is_save_param || priv->speed_hz != t->speed_hz) {
+		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
+		if (ret)
+			return ret;
+		priv->speed_hz = t->speed_hz;
+	}
+
+	if (!priv->is_save_param)
+		priv->is_save_param = true;
+
+	/* reset FIFOs */
+	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
+	writel(val, priv->base + SSI_FC);
+
+	return 0;
+}
+
+static void uniphier_spi_send(struct uniphier_spi_priv *priv)
+{
+	int wsize;
+	u32 val = 0;
+
+	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
+	priv->tx_bytes -= wsize;
+
+	if (priv->tx_buf) {
+		switch (wsize) {
+		case 1:
+			val = *priv->tx_buf;
+			break;
+		case 2:
+			val = get_unaligned_le16(priv->tx_buf);
+			break;
+		case 4:
+			val = get_unaligned_le32(priv->tx_buf);
+			break;
+		}
+
+		priv->tx_buf += wsize;
+	}
+
+	writel(val, priv->base + SSI_TXDR);
+}
+
+static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
+{
+	int rsize;
+	u32 val;
+
+	rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
+	priv->rx_bytes -= rsize;
+
+	val = readl(priv->base + SSI_RXDR);
+
+	if (priv->rx_buf) {
+		switch (rsize) {
+		case 1:
+			*priv->rx_buf = (u8)val;
+			break;
+		case 2:
+			put_unaligned_le16(val, priv->rx_buf);
+			break;
+		case 4:
+			put_unaligned_le32(val, priv->rx_buf);
+			break;
+		}
+
+		priv->rx_buf += rsize;
+	}
+}
+
+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
+{
+	unsigned int tx_count;
+	u32 val;
+
+	tx_count = DIV_ROUND_UP(priv->tx_bytes,
+				bytes_per_word(priv->bits_per_word));
+	tx_count = min(tx_count, SSI_FIFO_DEPTH);
+
+	/* set fifo threthold */
+	val = readl(priv->base + SSI_FC);
+	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
+	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
+	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
+	writel(val, priv->base + SSI_FC);
+
+	while (tx_count--)
+		uniphier_spi_send(priv);
+}
+
+static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_FPS);
+
+	if (enable)
+		val |= SSI_FPS_FSPOL;
+	else
+		val &= ~SSI_FPS_FSPOL;
+
+	writel(val, priv->base + SSI_FPS);
+}
+
+static int uniphier_spi_transfer_one(struct spi_master *master,
+				     struct spi_device *spi,
+				     struct spi_transfer *t)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+	int status;
+
+	status = uniphier_spi_setup_transfer(spi, t);
+	if (status < 0)
+		return status;
+
+	reinit_completion(&priv->xfer_done);
+
+	uniphier_spi_fill_tx_fifo(priv);
+
+	uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+	status = wait_for_completion_timeout(&priv->xfer_done,
+					     msecs_to_jiffies(SSI_TIMEOUT));
+
+	uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+	if (status < 0)
+		return status;
+
+	return priv->error;
+}
+
+static int uniphier_spi_prepare_transfer_hardware(struct spi_master *master)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+	writel(SSI_CTL_EN, priv->base + SSI_CTL);
+
+	return 0;
+}
+
+static int uniphier_spi_unprepare_transfer_hardware(struct spi_master *master)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+	writel(0, priv->base + SSI_CTL);
+
+	return 0;
+}
+
+static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
+{
+	struct uniphier_spi_priv *priv = dev_id;
+	u32 val, stat;
+
+	stat = readl(priv->base + SSI_IS);
+	val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
+	writel(val, priv->base + SSI_IC);
+
+	/* rx fifo overrun */
+	if (stat & SSI_IS_RORID) {
+		priv->error = -EIO;
+		goto done;
+	}
+
+	/* rx complete */
+	if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
+		while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
+				(priv->rx_bytes - priv->tx_bytes) > 0)
+			uniphier_spi_recv(priv);
+
+		if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
+				(priv->rx_bytes != priv->tx_bytes)) {
+			priv->error = -EIO;
+			goto done;
+		} else if (priv->rx_bytes == 0)
+			goto done;
+
+		/* next tx transfer */
+		uniphier_spi_fill_tx_fifo(priv);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+
+done:
+	complete(&priv->xfer_done);
+	return IRQ_HANDLED;
+}
+
+static int uniphier_spi_probe(struct platform_device *pdev)
+{
+	struct uniphier_spi_priv *priv;
+	struct spi_master *master;
+	struct resource *res;
+	unsigned long clksrc;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*priv));
+	if (!master)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, master);
+
+	priv = spi_master_get_devdata(master);
+	priv->master = master;
+	priv->is_save_param = false;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base)) {
+		ret = PTR_ERR(priv->base);
+		goto out_master_put;
+	}
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(&pdev->dev, "failed to get clock\n");
+		ret = PTR_ERR(priv->clk);
+		goto out_master_put;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		goto out_master_put;
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ\n");
+		ret = -ENXIO;
+		goto out_disable_clk;
+	}
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
+			       0, "uniphier-spi", priv);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request IRQ\n");
+		goto out_disable_clk;
+	}
+
+	init_completion(&priv->xfer_done);
+
+	clksrc = clk_get_rate(priv->clk);
+
+	master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
+	master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = pdev->id;
+	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
+
+	master->set_cs = uniphier_spi_set_cs;
+	master->transfer_one = uniphier_spi_transfer_one;
+	master->prepare_transfer_hardware
+				= uniphier_spi_prepare_transfer_hardware;
+	master->unprepare_transfer_hardware
+				= uniphier_spi_unprepare_transfer_hardware;
+	master->num_chipselect = 1;
+
+	ret = devm_spi_register_master(&pdev->dev, master);
+	if (ret)
+		goto out_disable_clk;
+
+	return ret;
+
+out_disable_clk:
+	clk_disable_unprepare(priv->clk);
+
+out_master_put:
+	spi_master_put(master);
+	return ret;
+}
+
+static int uniphier_spi_remove(struct platform_device *pdev)
+{
+	struct uniphier_spi_priv *priv = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id uniphier_spi_match[] = {
+	{ .compatible = "socionext,uniphier-scssi", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, uniphier_spi_match);
+
+static struct platform_driver uniphier_spi_driver = {
+	.probe = uniphier_spi_probe,
+	.remove = uniphier_spi_remove,
+	.driver = {
+		.name = "uniphier-spi",
+		.of_match_table = uniphier_spi_match,
+	},
+};
+module_platform_driver(uniphier_spi_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <[e-mail]>");
+MODULE_AUTHOR("Keiji Hayashibara <[e-mail]>");
+MODULE_DESCRIPTION("Socionext UniPhier SPI controller driver");
+MODULE_LICENSE("GPL v2");