Hi.
On Tue, Jun 11, 2019 at 10:08 AM Kunihiko Hayashi hayashi.kunihiko@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@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;elseval &= ~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;elseval &= ~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;elseval |= 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