| Message ID | 1515503383-8909-1-git-send-email-jassisinghbrar@gmail.com |
|---|---|
| Headers | show |
| Series | spi: support for Socionext Synquacer platform | expand |
On Tue, Jan 09, 2018 at 06:40:48PM +0530, jassisinghbrar@gmail.com wrote: This is basically fine, a couple of style things plus the question I had on the bindings: > +++ b/drivers/spi/spi-synquacer.c > @@ -0,0 +1,664 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Synquacer HSSPI controller driver Mixing C and C++ style comments on adjacent lines looks messy, just make the whole thing C++. > + speed = xfer->speed_hz ? : spi->max_speed_hz; > + bpw = xfer->bits_per_word ? : spi->bits_per_word; I'm not a big fan of all the ternery operator use in the driver, it's not super helpful for legibility. > + if (sspi->bpw == 8) > + words = xfer->len / 1; > + else if (sspi->bpw == 16) > + words = xfer->len / 2; > + else > + words = xfer->len / 4; This should be a switch statement; a safety check for invalid values wouldn't go amiss either.
On Tue, 2018-01-09 at 18:40 +0530, jassisinghbrar@gmail.com wrote: > From: Jassi Brar <jaswinder.singh@linaro.org> > > This patch adds support for controller found on synquacer platforms. > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > > +static int synquacer_spi_config(struct spi_master *master, > + struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + struct synquacer_spi *sspi = spi_master_get_devdata(master); > + unsigned int speed, mode, bpw, cs, bus_width; > + unsigned long rate, transf_mode; > + u32 val, div; > + > + /* Full Duplex only on 1bit wide bus */ > + if (xfer->rx_buf && xfer->tx_buf && > + (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) { > + dev_err(sspi->dev, "RX and TX bus widths must match!\n"); This looks like the wrong error message is being printed. It does not match the comment nor the code. > + speed = xfer->speed_hz ? : spi->max_speed_hz; > + bpw = xfer->bits_per_word ? : spi->bits_per_word; You shouldn't need to do this checking. __spi_validate() should have already set these fields in xfer to their defaults if they weren't set, checked xfer speed is less than controller max speed, checked bus width is supported, etc. > + /* return if nothing to change */ > + if (speed == sspi->speed && > + bus_width == sspi->bus_width && bpw == sspi->bpw && > + mode == sspi->mode && cs == sspi->cs && > + transf_mode == sspi->old_transf_mode) { > + return 0; > + } Good optimization to not reprogram on every xfer, since usually speed, bits, etc. stays the same for a series of xfers. However, often SPI clients generate a number write, read pairs. In this case transf_mode will alternate and the optimization will fail to work, even through it is very likely that nothing (especially bus speed, which can be slow to change) else changed. Consider allowing changing between read and write without unnecessarily reprogramming everything else. > + > + if (xfer->tx_buf) > + sspi->old_transf_mode = TXBIT; > + else > + sspi->old_transf_mode = RXBIT; Why not just: sspi->old_transf_mode = transf_mode; Also, why is this "old_" when all the other ones, sspi->speed, etc., don't have "old_" in front. Seems inconsistent unless there is something special about transf_mode I have missed. > > + > + if (xfer->tx_buf && xfer->rx_buf) > + val |= (DATA_TXRX << DATA_SHIFT); > + else if (xfer->rx_buf) > + val |= (DATA_RX << DATA_SHIFT); > + else > + val |= (DATA_TX << DATA_SHIFT); Looks like one needs to set three different modes for bi, rx, and tx. But your transf_mode variable only has two settings, rx and tx. It won't detect change between tx and bi. > > + > + val = readl_relaxed(sspi->regs + FIFOCFG); > + val |= RX_FLUSH; > + val |= TX_FLUSH; > + writel_relaxed(val, sspi->regs + FIFOCFG); If this causes the master to pulse the chipselect it will mess up messages that have more than one xfer. > + > + /* See if we can transfer 4-bytes as 1 word even if not asked */ > + bpw = xfer->bits_per_word ? : spi->bits_per_word; Don't neeed this as xfer->bits_per_word is already normalized. > + if (bpw == 8 && !(xfer->len % 4) && !(spi->mode & SPI_LSB_FIRST)) { > + bpw = xfer->bits_per_word; > + xfer->bits_per_word = 32; > + } else { > + bpw = xfer->bits_per_word; > + } What's the point of the bpw = xfer->bits_per_word above? It would already be set to that value. > + > + ret = synquacer_spi_config(master, spi, xfer); > + if (ret) { > + xfer->bits_per_word = bpw; Why not restore xfer->bits_per_word immediately after call to synquacer_spi_config, before if statement. It's not used again. Then you don't have to restore it also at the end of the function. > > + > + spin_lock_irqsave(&sspi->lock, flags); What's this lock for? I see no other use than here. > + master->min_speed_hz = master->max_speed_hz / 254; The configure function rejects divisors larger than 127. Seems like these should match. I notice your transfer function does not use interrupts. It will spin in a tight loop polling DMSTATUS waiting for data to arrive. Not very friendly for a multitasking operating system. It would be much better if there was a way wait for in an interrupt at the start of your while loop and have the controller trigger one when the tx and/or rx fifo has some amount of data in it (like half full). You'll likely also find that in an interruptable kernel that your transfer function will eventually stop running and another process will run. Multitasking. When this happens, it might be many milliseconds before it is scheduled to run again. Hope that fifo is large... Usually when a device has a fifo that must be emptied before it overflows, it is either emptied from an interrupt handler or the interrupt handler will queue a work function or tasklet that will empty it. Doing it from user context has a maximum latency that is too high.
On Tue, Jan 09, 2018 at 07:59:05PM +0000, Trent Piepho wrote: > Usually when a device has a fifo that must be emptied before it > overflows, it is either emptied from an interrupt handler or the > interrupt handler will queue a work function or tasklet that will empty > it. Doing it from user context has a maximum latency that is too high. It wouldn't be the first SPI controller that lacked interrupt support, this is depressingly common.
On 9 January 2018 at 23:11, Mark Brown <broonie@kernel.org> wrote: > On Tue, Jan 09, 2018 at 06:40:48PM +0530, jassisinghbrar@gmail.com wrote: > > This is basically fine, a couple of style things plus the question I had > on the bindings: > >> +++ b/drivers/spi/spi-synquacer.c >> @@ -0,0 +1,664 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Synquacer HSSPI controller driver > > Mixing C and C++ style comments on adjacent lines looks messy, just make > the whole thing C++. > Fixed. >> + speed = xfer->speed_hz ? : spi->max_speed_hz; >> + bpw = xfer->bits_per_word ? : spi->bits_per_word; > > I'm not a big fan of all the ternery operator use in the driver, it's > not super helpful for legibility. > This actually goes away now. >> + if (sspi->bpw == 8) >> + words = xfer->len / 1; >> + else if (sspi->bpw == 16) >> + words = xfer->len / 2; >> + else >> + words = xfer->len / 4; > > This should be a switch statement; a safety check for invalid values > wouldn't go amiss either. > ok Thank you. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 January 2018 at 01:29, Trent Piepho <tpiepho@impinj.com> wrote: > On Tue, 2018-01-09 at 18:40 +0530, jassisinghbrar@gmail.com wrote: >> From: Jassi Brar <jaswinder.singh@linaro.org> >> >> This patch adds support for controller found on synquacer platforms. >> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> >> >> +static int synquacer_spi_config(struct spi_master *master, >> + struct spi_device *spi, >> + struct spi_transfer *xfer) >> +{ >> + struct synquacer_spi *sspi = spi_master_get_devdata(master); >> + unsigned int speed, mode, bpw, cs, bus_width; >> + unsigned long rate, transf_mode; >> + u32 val, div; >> + >> + /* Full Duplex only on 1bit wide bus */ >> + if (xfer->rx_buf && xfer->tx_buf && >> + (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) { >> + dev_err(sspi->dev, "RX and TX bus widths must match!\n"); > > This looks like the wrong error message is being printed. It does not > match the comment nor the code. > Well, TX and TX bus widths should both be 1 for Full-Duplex mode. I have anyway tried to made it clearer. >> + speed = xfer->speed_hz ? : spi->max_speed_hz; >> + bpw = xfer->bits_per_word ? : spi->bits_per_word; > > You shouldn't need to do this checking. __spi_validate() should have > already set these fields in xfer to their defaults if they weren't set, > checked xfer speed is less than controller max speed, checked bus width > is supported, etc. > Cool. Thanks >> + /* return if nothing to change */ >> + if (speed == sspi->speed && >> + bus_width == sspi->bus_width && bpw == sspi->bpw && >> + mode == sspi->mode && cs == sspi->cs && >> + transf_mode == sspi->old_transf_mode) { >> + return 0; >> + } > > Good optimization to not reprogram on every xfer, since usually speed, > bits, etc. stays the same for a series of xfers. However, often SPI > clients generate a number write, read pairs. In this case transf_mode > will alternate and the optimization will fail to work, even through it > is very likely that nothing (especially bus speed, which can be slow to > change) else changed. > > Consider allowing changing between read and write without unnecessarily > reprogramming everything else. > Tracking transf_mode is actually redundant. I have removed it. >> + >> + if (xfer->tx_buf) >> + sspi->old_transf_mode = TXBIT; >> + else >> + sspi->old_transf_mode = RXBIT; > Why not just: > > sspi->old_transf_mode = transf_mode; > > Also, why is this "old_" when all the other ones, sspi->speed, etc., > don't have "old_" in front. Seems inconsistent unless there is > something special about transf_mode I have missed. > >> >> + >> + if (xfer->tx_buf && xfer->rx_buf) >> + val |= (DATA_TXRX << DATA_SHIFT); >> + else if (xfer->rx_buf) >> + val |= (DATA_RX << DATA_SHIFT); >> + else >> + val |= (DATA_TX << DATA_SHIFT); > > Looks like one needs to set three different modes for bi, rx, and tx. > But your transf_mode variable only has two settings, rx and tx. It > won't detect change between tx and bi. > >> >> + >> + val = readl_relaxed(sspi->regs + FIFOCFG); >> + val |= RX_FLUSH; >> + val |= TX_FLUSH; >> + writel_relaxed(val, sspi->regs + FIFOCFG); > > If this causes the master to pulse the chipselect it will mess up > messages that have more than one xfer. > No, that just resets the fifos. >> + >> + /* See if we can transfer 4-bytes as 1 word even if not asked */ >> + bpw = xfer->bits_per_word ? : spi->bits_per_word; > > Don't neeed this as xfer->bits_per_word is already normalized. > OK >> + if (bpw == 8 && !(xfer->len % 4) && !(spi->mode & SPI_LSB_FIRST)) { >> + bpw = xfer->bits_per_word; >> + xfer->bits_per_word = 32; >> + } else { >> + bpw = xfer->bits_per_word; >> + } > > What's the point of the bpw = xfer->bits_per_word above? It would > already be set to that value. > Cleaned. >> + >> + ret = synquacer_spi_config(master, spi, xfer); >> + if (ret) { >> + xfer->bits_per_word = bpw; > > Why not restore xfer->bits_per_word immediately after call to > synquacer_spi_config, before if statement. It's not used again. Then > you don't have to restore it also at the end of the function. > Done. >> >> + >> + spin_lock_irqsave(&sspi->lock, flags); > > What's this lock for? I see no other use than here. > Remnant of old driver. Dropped. >> + master->min_speed_hz = master->max_speed_hz / 254; > > The configure function rejects divisors larger than 127. Seems like > these should match. > Fixed. > I notice your transfer function does not use interrupts. It will spin > in a tight loop polling DMSTATUS waiting for data to arrive. Not very > friendly for a multitasking operating system. It would be much better > if there was a way wait for in an interrupt at the start of your while > loop and have the controller trigger one when the tx and/or rx fifo has > some amount of data in it (like half full). > > You'll likely also find that in an interruptable kernel that your > transfer function will eventually stop running and another process will > run. Multitasking. When this happens, it might be many milliseconds > before it is scheduled to run again. Hope that fifo is large... > > Usually when a device has a fifo that must be emptied before it > overflows, it is either emptied from an interrupt handler or the > interrupt handler will queue a work function or tasklet that will empty > it. Doing it from user context has a maximum latency that is too high. > I am aware of the benefits of interrupts, unfortunately the h/w doesn't seem to support. Thank you. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jassi Brar <jaswinder.singh@linaro.org> Hello, Support for Socionext's FIP controller intended for flash device interfacing. The controller can operate in 'direct' or 'command' mode. One mode directly talks and provide a read/write i/f to the flash device. Other works as plain SPI mode. This driver runs the controller as a SPI controller. Jassi Brar (3): dt-bindings: spi: Add DT bindings for Synquacer spi: Add spi driver for Socionext Synquacer platform MAINTAINERS: Add entry for Synquacer SPI driver .../devicetree/bindings/spi/spi-synquacer.txt | 24 + MAINTAINERS | 7 + drivers/spi/Kconfig | 11 + drivers/spi/Makefile | 1 + drivers/spi/spi-synquacer.c | 664 +++++++++++++++++++++ 5 files changed, 707 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-synquacer.txt create mode 100644 drivers/spi/spi-synquacer.c