[U-Boot] [PATCH 2/2] spi: Add SPI controller driver for UniPhier SoCs
Kunihiko Hayashi
hayashi.kunihiko at socionext.com
Fri Jun 28 01:15:34 UTC 2019
Hi Yamada-san,
Thank you for reviewing.
On Tue, 25 Jun 2019 23:55:42 +0900 <yamada.masahiro at socionext.com> wrote:
> Hi.
>
> On Tue, Jun 11, 2019 at 10:08 AM Kunihiko Hayashi
> <hayashi.kunihiko at socionext.com> wrote:
> >
> > Add SPI controller driver implemented in Socionext UniPhier SoCs.
> > This controller has the SPI master mode only.
> >
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko at socionext.com>
>
> I see warnings even with GCC 6, which is the minimal GCC
> version for ARM.
> I doubt you compile tested this before the submission.
>
>
> CC drivers/spi/spi-uclass.o
> CC drivers/spi/uniphier_spi.o
> drivers/spi/uniphier_spi.c: In function ‘uniphier_spi_ofdata_to_platdata’:
> drivers/spi/uniphier_spi.c:392:28: warning: unused variable ‘priv’
> [-Wunused-variable]
> struct uniphier_spi_priv *priv = dev_get_priv(bus);
> ^~~~
> drivers/spi/uniphier_spi.c: In function ‘uniphier_spi_xfer’:
> drivers/spi/uniphier_spi.c:267:4: warning: ‘status’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> printf("spi_uniphier: access timeout %08x\n", status);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> I request a patch to enable this driver
> so that I (and you) can easily compile test it.
I missed compiling with minimal GCC version for arm.
I'll test with that, and these warnings should be fixed.
>
> Cosmetic review follows...
>
>
> > new file mode 100644
> > index 0000000..6840726
> > --- /dev/null
> > +++ b/drivers/spi/uniphier_spi.c
> > @@ -0,0 +1,443 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * uniphier_spi.c - Socionext UniPhier SPI driver
> > + * Copyright 2019 Socionext, Inc.
> > + */
> > +
> > +#include <common.h>
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <spi.h>
> > +#include <wait_bit.h>
> > +#include <linux/bitfield.h>
> > +#include <asm/io.h>
>
> I prefer <linux/io.h> line in Linux.
Indeed. <linux/io.h> is preferable.
>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +#define SSI_CTL 0x00
> > +#define SSI_CTL_EN BIT(0)
> > +
> > +#define SSI_CKS 0x04
> > +#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 0x08
> > +#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 0x0c
> > +#define SSI_RXWDS_RDTF_MASK GENMASK(7, 6)
> > +#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_BUSY BIT(7)
> > +#define SSI_SR_TNF BIT(5)
> > +#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
> > +
> > +#define SSI_REG_TIMEOUT (CONFIG_SYS_HZ / 100) /* 10 ms */
> > +#define SSI_XFER_TIMEOUT (CONFIG_SYS_HZ) /* 1 sec */
> > +
> > +#define SSI_CLK 50000000 /* internal I/O clock: 50MHz */
> > +
> > +/* uniphier spi register set */
> > +struct uniphier_spi_regs {
> > + u32 ctl; /* 0x00 */
> > + u32 cks; /* 0x04 */
> > + u32 txwds; /* 0x08 */
> > + u32 rxwds; /* 0x0C */
> > + u32 fps; /* 0x10 */
> > + u32 sr; /* 0x14 */
> > + u32 ie; /* 0x18 */
> > + u32 ic; /* 0x1C */
> > + u32 fc; /* 0x20 */
> > + u32 xdr; /* 0x24 */
> > +};
>
>
> You defined SSI_CTL, SSI_CKS, SSI_TXWDS, etc.
> all of which are unused.
> Then, you define this struct.
> You do not need both.
>
> Personally, I am not a big fan of a struct
> representing the register, since this
> potentially has __packed problem.
>
> I leave it up to you, though.
This has been affected by other spi drivers' implementations.
However, the struct description of registers contains some risks.
If there is no other reason to write with this, I'll rewrite this
with macros.
>
> > +/* uniphier spi platform data */
>
> Pointless comment.
Including event below here, I'll remove it.
>
> > +struct uniphier_spi_platdata {
> > + struct uniphier_spi_regs *regs;
> > + u32 frequency; /* input frequency */
> > + u32 speed_hz;
> > + uint deactivate_delay_us; /* Delay to wait after deactivate */
> > + uint activate_delay_us; /* Delay to wait after activate */
> > +};
> > +
> > +/* uniphier spi priv */
>
> Same here.
>
>
> > +struct uniphier_spi_priv {
> > + struct uniphier_spi_regs *regs;
> > + u8 mode;
> > + u8 fifo_depth;
> > + u8 bits_per_word;
> > + ulong last_transaction_us; /* Time of last transaction end */
> > +};
> > +
> > +static void uniphier_spi_enable(struct uniphier_spi_regs *regs, int enable)
> > +{
> > + u32 val;
> > +
> > + val = readl(®s->ctl);
> > + if (enable)
> > + val |= SSI_CTL_EN;
> > + else
> > + val &= ~SSI_CTL_EN;
> > + writel(val, ®s->ctl);
> > +}
> > +
> > +static void uniphier_spi_regdump(struct uniphier_spi_regs *regs)
> > +{
> > + debug("CTL %08x\n", readl(®s->ctl));
> > + debug("CKS %08x\n", readl(®s->cks));
> > + debug("TXWDS %08x\n", readl(®s->txwds));
> > + debug("RXWDS %08x\n", readl(®s->rxwds));
> > + debug("FPS %08x\n", readl(®s->fps));
> > + debug("SR %08x\n", readl(®s->sr));
> > + debug("IE %08x\n", readl(®s->ie));
> > + debug("IC %08x\n", readl(®s->ic));
> > + debug("FC %08x\n", readl(®s->fc));
> > + debug("XDR %08x\n", readl(®s->xdr));
>
> You can use Linux-like pr_debug() if you like.
It's available here, so I'll replace with it.
>
> > +}
> > +
> > +static void spi_cs_activate(struct udevice *dev)
> > +{
> > + struct udevice *bus = dev->parent;
> > + struct uniphier_spi_platdata *plat = bus->platdata;
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + struct uniphier_spi_regs *regs = priv->regs;
> > + ulong delay_us; /* The delay completed so far */
> > + u32 val;
> > +
> > + /* If it's too soon to do another transaction, wait */
> > + if (plat->deactivate_delay_us && priv->last_transaction_us) {
> > + delay_us = timer_get_us() - priv->last_transaction_us;
> > + if (delay_us < plat->deactivate_delay_us)
> > + udelay(plat->deactivate_delay_us - delay_us);
> > + }
> > +
> > + val = readl(®s->fps);
> > + if (priv->mode & SPI_CS_HIGH)
> > + val |= SSI_FPS_FSPOL;
> > + else
> > + val &= ~SSI_FPS_FSPOL;
> > + writel(val, ®s->fps);
> > +
> > + if (plat->activate_delay_us)
> > + udelay(plat->activate_delay_us);
> > +}
> > +
> > +static void spi_cs_deactivate(struct udevice *dev)
> > +{
> > + struct udevice *bus = dev->parent;
> > + struct uniphier_spi_platdata *plat = bus->platdata;
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + struct uniphier_spi_regs *regs = priv->regs;
> > + u32 val;
> > +
> > + val = readl(®s->fps);
> > + if (priv->mode & SPI_CS_HIGH)
> > + val &= ~SSI_FPS_FSPOL;
> > + else
> > + val |= SSI_FPS_FSPOL;
> > + writel(val, ®s->fps);
> > +
> > + /* Remember time of this transaction so we can honour the bus delay */
> > + if (plat->deactivate_delay_us)
> > + priv->last_transaction_us = timer_get_us();
> > +}
> > +
> > +static int uniphier_spi_claim_bus(struct udevice *dev)
> > +{
> > + struct udevice *bus = dev->parent;
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + struct uniphier_spi_regs *regs = priv->regs;
> > + u32 val, size;
> > +
> > + uniphier_spi_enable(regs, false);
> > +
> > + /* disable interrupts */
> > + writel(0, ®s->ie);
> > +
> > + /* bits_per_word */
> > + size = priv->bits_per_word;
> > + val = readl(®s->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, ®s->txwds);
> > +
> > + val = readl(®s->rxwds);
> > + val &= ~SSI_RXWDS_DTLEN_MASK;
> > + val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> > + writel(val, ®s->rxwds);
> > +
> > + /* reset FIFOs */
> > + val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> > + writel(val, ®s->fc);
> > +
> > + /* FIFO threthold */
> > + val = readl(®s->fc);
> > + val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> > + val |= FIELD_PREP(SSI_FC_TXFTH_MASK, priv->fifo_depth);
> > + val |= FIELD_PREP(SSI_FC_RXFTH_MASK, priv->fifo_depth);
> > + writel(val, ®s->fc);
> > +
> > + /* clear interrupts */
> > + writel(SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC, ®s->ic);
> > +
> > + uniphier_spi_enable(regs, true);
> > +
> > + return 0;
> > +}
> > +
> > +static int uniphier_spi_release_bus(struct udevice *dev)
> > +{
> > + struct udevice *bus = dev->parent;
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + struct uniphier_spi_regs *regs = priv->regs;
> > +
> > + uniphier_spi_enable(regs, false);
> > +
> > + return 0;
> > +}
> > +
> > +static int uniphier_spi_xfer(struct udevice *dev, unsigned int bitlen,
> > + const void *dout, void *din, unsigned long flags)
> > +{
> > + struct udevice *bus = dev->parent;
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + struct uniphier_spi_regs *regs = priv->regs;
> > + const u8 *tx_buf = dout;
> > + u8 *rx_buf = din, buf;
> > + u32 len = bitlen / 8;
> > + u32 tx_len, rx_len;
> > + u32 ts, status;
> > + int ret = 0;
> > +
> > + if (bitlen % 8) {
> > + debug("spi_xfer: Non byte aligned SPI transfer\n");
> > + return -EINVAL;
>
>
> You return error code.
> Is this the debug log? Why not dev_err()?
This code is derived from other drivers.
Since this is an error, should replace with dev_err().
>
> > + }
> > +
> > + if (flags & SPI_XFER_BEGIN)
> > + spi_cs_activate(dev);
> > +
> > + uniphier_spi_enable(regs, true);
> > +
> > + ts = get_timer(0);
> > + tx_len = len;
> > + rx_len = len;
> > +
> > + uniphier_spi_regdump(regs);
> > +
> > + while (tx_len || rx_len) {
> > + ret = wait_for_bit_le32(®s->sr, SSI_SR_BUSY, false,
> > + SSI_REG_TIMEOUT * 1000, false);
> > + if (ret) {
> > + printf("spi_uniphier: access timeout %08x\n", status);
> > + break;
>
>
> Is this an error? ( I just saw it from the message)
>
> If so, is it OK to return 0 successfully?
>
> You can use kernel-like dev_err() etc. depending on the log-level.
This code waits until data can be sent. If this returns 0, the driver
can send some data. Or, it's an error.
The error is timeout or abort, so this message should be replaced with
dev_err() in case of timeout.
>
> > + }
> > +
> > + status = readl(®s->sr);
> > + /* write the data into TX */
> > + if (tx_len && (status & SSI_SR_TNF)) {
> > + buf = tx_buf ? *tx_buf++ : 0;
> > + writel(buf, ®s->xdr);
> > + tx_len--;
> > + }
> > +
> > + /* read the data from RX */
> > + if (rx_len && (status & SSI_SR_RNE)) {
> > + buf = readl(®s->xdr);
> > + if (rx_buf)
> > + *rx_buf++ = buf;
> > + rx_len--;
> > + }
> > +
> > + if (get_timer(ts) >= SSI_XFER_TIMEOUT) {
> > + printf("spi_uniphier: xfer timeout\n");
>
> Please dev_err().
I'll replace with dev_err().
>
> > + ret = -ETIMEDOUT;
> > + break;
> > + }
> > + }
> > +
> > + if (flags & SPI_XFER_END)
> > + spi_cs_deactivate(dev);
> > +
> > + uniphier_spi_enable(regs, false);
> > +
> > + return ret;
> > +}
> > +
> > +static int uniphier_spi_set_speed(struct udevice *bus, uint speed)
> > +{
> > + struct uniphier_spi_platdata *plat = bus->platdata;
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + struct uniphier_spi_regs *regs = priv->regs;
> > + u32 val, ckdiv;
> > +
> > + if (speed > plat->frequency)
> > + speed = plat->frequency;
> > +
> > + /* baudrate */
> > + ckdiv = DIV_ROUND_UP(SSI_CLK, speed);
> > + ckdiv = round_up(ckdiv, 2);
> > +
> > + val = readl(®s->cks);
> > + val &= ~SSI_CKS_CKRAT_MASK;
> > + val |= ckdiv & SSI_CKS_CKRAT_MASK;
> > + writel(val, ®s->cks);
> > +
> > + return 0;
> > +}
> > +
> > +static int uniphier_spi_set_mode(struct udevice *bus, uint mode)
> > +{
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + struct uniphier_spi_regs *regs = priv->regs;
> > + 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
> > + */
> > + val1 = readl(®s->cks);
> > + val2 = readl(®s->fps);
> > +
> > + switch (mode & (SPI_CPOL | SPI_CPHA)) {
> > + case SPI_MODE_0:
> > + /* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
> > + val1 |= SSI_CKS_CKPHS | SSI_CKS_CKDLY;
> > + val1 &= ~SSI_CKS_CKINIT;
> > + val2 &= ~SSI_FPS_FSTRT;
> > + break;
> > + case SPI_MODE_1:
> > + /* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
> > + val1 &= ~(SSI_CKS_CKPHS | SSI_CKS_CKINIT | SSI_CKS_CKDLY);
> > + val2 |= SSI_FPS_FSTRT;
> > + break;
> > + case SPI_MODE_2:
> > + /* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
> > + val1 |= SSI_CKS_CKINIT | SSI_CKS_CKDLY;
> > + val1 &= ~SSI_CKS_CKPHS;
> > + val2 |= SSI_FPS_FSTRT;
> > + break;
> > + case SPI_MODE_3:
> > + /* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
> > + val1 |= SSI_CKS_CKPHS | SSI_CKS_CKINIT;
> > + val1 &= ~SSI_CKS_CKDLY;
> > + val2 &= ~SSI_FPS_FSTRT;
> > + break;
> > + }
> > +
> > + writel(val1, ®s->cks);
> > + writel(val2, ®s->fps);
> > +
> > + /* format */
> > + val1 = readl(®s->txwds);
> > + val2 = readl(®s->rxwds);
> > + if (mode & SPI_LSB_FIRST) {
> > + val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> > + val2 |= FIELD_PREP(SSI_RXWDS_RDTF_MASK, 1);
> > + }
> > + writel(val1, ®s->txwds);
> > + writel(val2, ®s->rxwds);
> > +
> > + priv->mode = mode;
> > +
> > + return 0;
> > +}
> > +
> > +static int uniphier_spi_ofdata_to_platdata(struct udevice *bus)
> > +{
> > + struct uniphier_spi_platdata *plat = bus->platdata;
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > + const void *blob = gd->fdt_blob;
> > + int node = dev_of_offset(bus);
> > +
> > + plat->regs = (struct uniphier_spi_regs *)devfdt_get_addr(bus);
>
> This casting is fragile because you never know the primitive type of fdt_addr_t.
> Use devfdt_get_addr_ptr().
The devdt_get_addr_ptr() is reasonable. I'll replace with it.
>
> > +
> > + plat->frequency =
> > + fdtdec_get_int(blob, node, "spi-max-frequency", 12500000);
> > + plat->deactivate_delay_us =
> > + fdtdec_get_int(blob, node, "spi-deactivate-delay", 0);
> > + plat->activate_delay_us =
> > + fdtdec_get_int(blob, node, "spi-activate-delay", 0);
> > + plat->speed_hz = plat->frequency / 2;
> > +
> > + return 0;
> > +}
> > +
> > +static int uniphier_spi_probe(struct udevice *bus)
> > +{
> > + struct uniphier_spi_platdata *plat = dev_get_platdata(bus);
> > + struct uniphier_spi_priv *priv = dev_get_priv(bus);
> > +
> > + priv->regs = plat->regs;
> > + priv->fifo_depth = SSI_FIFO_DEPTH;
> > + priv->bits_per_word = 8;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dm_spi_ops uniphier_spi_ops = {
> > + .claim_bus = uniphier_spi_claim_bus,
> > + .release_bus = uniphier_spi_release_bus,
> > + .xfer = uniphier_spi_xfer,
> > + .set_speed = uniphier_spi_set_speed,
> > + .set_mode = uniphier_spi_set_mode,
> > +};
> > +
> > +static const struct udevice_id uniphier_spi_ids[] = {
> > + { .compatible = "socionext,uniphier-scssi" },
> > + { /* Sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(uniphier_spi) = {
> > + .name = "uniphier_spi",
> > + .id = UCLASS_SPI,
> > + .of_match = uniphier_spi_ids,
> > + .ops = &uniphier_spi_ops,
> > + .ofdata_to_platdata = uniphier_spi_ofdata_to_platdata,
> > + .platdata_auto_alloc_size = sizeof(struct uniphier_spi_platdata),
> > + .priv_auto_alloc_size = sizeof(struct uniphier_spi_priv),
> > + .probe = uniphier_spi_probe,
> > +};
> > --
> > 2.7.4
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
>
>
>
> --
> Best Regards
> Masahiro Yamada
Thank you,
---
Best Regards,
Kunihiko Hayashi
More information about the U-Boot
mailing list