Hi Trent, > -----Original Message----- > From: Trent Piepho [mailto:tpiepho@xxxxxxxxxx] > Sent: Friday, July 20, 2018 4:46 AM > Subject: Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC > > On Thu, 2018-07-19 at 15:51 +0900, Keiji Hayashibara wrote: > > > > +config SPI_UNIPHIER > > + tristate "Socionext UniPhier SPI Controller" > > + depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF > > + help > > + This driver supports the SPI controller on Socionext > > + UniPhier SoCs. > > Perhaps add the bit that this is for the SCSSI and not MCSSI here? OK. I will add it. > > > > + > > +#define BYTES_PER_WORD(x) \ > > +({ \ > > + int __x; \ > > + __x = (x <= 8) ? 1 : \ > > + (x <= 16) ? 2 : 4; \ > > + __x; \ > > +}) > > Or: > > static inline bytes_per_word(unsigned int bits) { > return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4); } I will modify. > > > + > > +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_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->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->speed_hz != t->speed_hz) { > > + ret = uniphier_spi_set_baudrate(spi, t->speed_hz); > > + if (ret) > > + return ret; > > + priv->speed_hz = t->speed_hz; > > + } > > + > > + /* 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 i, loop; > > + u32 val = 0; > > + > > + loop = BYTES_PER_WORD(priv->bits_per_word); > > + if (priv->tx_bytes < loop) > > + loop = priv->tx_bytes; > > + > > + priv->tx_bytes -= loop; > > + > > + if (priv->tx_buf) > > + for (i = 0; i < loop; i++) { > > + val |= (*(const u8 *)priv->tx_buf) > > + << (BITS_PER_BYTE * i); > > priv->tx_buf is already a const u8*, no need to cast it. Also in recv, > no need to cast the pointer. It'll just hide errors if someone changes the type of the field. I agree. > > + (const u8 *)priv->tx_buf++; > > + } > > + > > + writel(val, priv->base + SSI_TXDR); > > +} > > The loop to read the data will likely be somewhat slow. It might be faster to use: > > val = get_unaligned_le32(priv->tx_buf); > > To support different sizes a switch can be used: > > switch (MIN(BYTES_PER_WORD(priv->bits_per_word), priv->tx_bytes)) { > 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; > } > > However, I don't think either the existing code or this code is correctly handling word sizes that are not an > even number of bytes. I think it needs to left shift the data, but of course it also depends on what the uniphier > hardware expected in the TXDR register. I agree. This code is simple for no loop. I will modify to this code. > > > +static void uniphier_spi_recv(struct uniphier_spi_priv *priv) { > > + int i, loop; > > + u32 val; > > + > > + loop = BYTES_PER_WORD(priv->bits_per_word); > > + if (priv->rx_bytes < loop) > > + loop = priv->rx_bytes; > > + > > + priv->rx_bytes -= loop; > > + > > + val = readl(priv->base + SSI_RXDR); > > + > > + if (priv->rx_buf) > > + for (i = 0; i < loop; i++) { > > + val = val >> (BITS_PER_BYTE * i); > > + *(u8 *)priv->rx_buf = val & GENMASK(7, 0); > > + (u8 *)priv->rx_buf++; > > + } > > > > > +}+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv > > +*priv) { > > + unsigned int tx_count; > > + int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word); > > + u32 val; > > + > > + tx_count = priv->tx_bytes / bytes_per_word; > > + if (tx_count > SSI_FIFO_DEPTH) > > + 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); > > +} > > If you have 24 bits per word, 3 words, that's 9 bytes. > BYTES_PER_WORD(24) is 4. tx_count = 9/4 = 2. Looks like your tx_count rounds incorrectly, as it will only send > 8 of the 9 bytes. Oh, I will fix this bug. > > > +static int uniphier_spi_setup(struct spi_device *spi) { > > > + > > + 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); > > Did you see this in the spi docs? > > Unless each SPI slave has its own configuration registers, don't > change them right away ... otherwise drivers could corrupt I/O > that's in progress for other SPI devices. > > ** BUG ALERT: for some reason the first version of > ** many spi_master drivers seems to get this wrong. > ** When you code setup(), ASSUME that the controller > ** is actively processing transfers for another device. > > You have one chipselect, so maybe this is ok. Until you want to support more than one chipselect. > > With gpio lines as chip selects, there's really no reason any spi master can't support multiple slaves. I agree. I will re-think about this. Thank you for your advice. ----------------- Best Regards, Keiji Hayashibara -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
