Hi. On Tue, Jun 11, 2019 at 10:08 AM Kunihiko Hayashi <hayashi.kunih...@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.kunih...@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. 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. > + > +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. > +/* uniphier spi platform data */ Pointless comment. > +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. > +} > + > +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()? > + } > + > + 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. > + } > + > + 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(). > + 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(). > + > + 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@lists.denx.de > https://lists.denx.de/listinfo/u-boot -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot