| Message ID | cover.1642494310.git.lhjeff911@gmail.com |
|---|---|
| Headers | show |
| Series | Add spi control driver for Sunplus SP7021 SoC | expand |
On Tue, Jan 18, 2022 at 04:42:38PM +0800, Li-hao Kuo wrote: Looks mostly good - a couple of small nits below but nothing major. > +static int sp7021_spi_master_transfer_one(struct spi_controller *ctlr, struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + for (i = 0; i <= xfer_cnt; i++) { > + mutex_lock(&pspim->buf_lock); This lock is redundant: it is only ever held in this function which is guaranteed by the core to never be called twice concurrently. > + ret = devm_request_irq(dev, pspim->m_irq, sp7021_spi_master_irq, > + IRQF_TRIGGER_RISING, pdev->name, pspim); > + if (ret) > + return ret; > + > + ret = devm_request_irq(dev, pspim->s_irq, sp7021_spi_slave_irq, > + IRQF_TRIGGER_RISING, pdev->name, pspim); > + if (ret) > + return ret; Are you sure the driver is ready to handle interrupts without any of the other resources? Normally interrupts are one of the last things to be requested.
On Tue, Jan 18, 2022 at 10:42 AM Li-hao Kuo <lhjeff911@gmail.com> wrote: > > Add spi driver for Sunplus SP7021. ... > Changes in v6: Thanks for update, my comments below. ... > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/dma-mapping.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/reset.h> > +#include <linux/spi/spi.h> ... > + data_status = readl(pspim->s_base + SP7021_DATA_RDY_REG); > + writel(data_status | SP7021_SLAVE_CLR_INT, pspim->s_base + SP7021_DATA_RDY_REG); Wouldn't be data_status = readl(pspim->s_base + SP7021_DATA_RDY_REG); data_status |= SP7021_SLAVE_CLR_INT; writel(data_status, pspim->s_base + SP7021_DATA_RDY_REG); better to read? Same question to other places like this. ... > + writel(SP7021_SLAVE_DMA_EN | SP7021_SLAVE_DMA_RW | FIELD_PREP(SP7021_SLAVE_DMA_CMD, 3), > + pspim->s_base + SP7021_SLAVE_DMA_CTRL_REG); Temporary variable? var = ... ... > + writel(readl(pspim->s_base + SP7021_DATA_RDY_REG) | SP7021_SLAVE_DATA_RDY, > + pspim->s_base + SP7021_DATA_RDY_REG); Ditto. ... > +int sp7021_spi_slave_rx(struct spi_device *spi, struct spi_transfer *xfer) > +{ > + struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(spi->controller); > + int ret = 0; Unused. ... > + writel(SP7021_SLAVE_DMA_EN | FIELD_PREP(SP7021_SLAVE_DMA_CMD, 3), > + pspim->s_base + SP7021_SLAVE_DMA_CTRL_REG); Temporary variable? ... > +static irqreturn_t sp7021_spi_master_irq(int irq, void *dev) > +{ > + struct sp7021_spi_ctlr *pspim = dev; > + unsigned int tx_cnt, total_len; > + unsigned int tx_len, rx_cnt; > + unsigned int fd_status; > + unsigned long flags; Why do you need this? > + bool isrdone = false; > + u32 value; > + return IRQ_HANDLED; > +} ... > + div = clk_rate / xfer->speed_hz; > + if (div < 2) > + div = 2; div = max(2U, clk_rate / xfer->speed_hz); ? ... > +static int sp7021_spi_master_transfer_one(struct spi_controller *ctlr, struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr); > + unsigned long timeout = msecs_to_jiffies(1000); > + unsigned int xfer_cnt, xfer_len, last_len; > + unsigned int i, len_temp; > + u32 reg_temp; > + int ret; Seems redundant. > + xfer_cnt = xfer->len / SP7021_SPI_DATA_SIZE; > + last_len = xfer->len % SP7021_SPI_DATA_SIZE; > + > + for (i = 0; i <= xfer_cnt; i++) { > + mutex_lock(&pspim->buf_lock); > + sp7021_prep_transfer(ctlr, spi); > + sp7021_spi_setup_clk(ctlr, xfer); > + reinit_completion(&pspim->isr_done); > + if (i == xfer_cnt) > + xfer_len = last_len; > + else > + xfer_len = SP7021_SPI_DATA_SIZE; > + > + pspim->tx_buf = xfer->tx_buf + i * SP7021_SPI_DATA_SIZE; > + pspim->rx_buf = xfer->rx_buf + i * SP7021_SPI_DATA_SIZE; > + > + if (pspim->tx_cur_len < xfer_len) { > + len_temp = min(pspim->data_unit, xfer_len); > + sp7021_spi_master_wb(pspim, len_temp); > + } > + reg_temp = readl(pspim->m_base + SP7021_SPI_CONFIG_REG); > + reg_temp &= ~SP7021_CLEAN_RW_BYTE; > + reg_temp &= ~SP7021_CLEAN_FLUG_MASK; > + reg_temp |= SP7021_FD_SEL | SP7021_FINISH_FLAG_MASK | > + SP7021_TX_EMP_FLAG_MASK | SP7021_RX_FULL_FLAG_MASK | > + FIELD_PREP(SP7021_TX_UNIT, 0) | FIELD_PREP(SP7021_RX_UNIT, 0); > + writel(reg_temp, pspim->m_base + SP7021_SPI_CONFIG_REG); > + > + reg_temp = FIELD_PREP(SP7021_SET_TX_LEN, xfer_len) | > + FIELD_PREP(SP7021_SET_XFER_LEN, xfer_len) | > + SP7021_SPI_START_FD; > + writel(reg_temp, pspim->m_base + SP7021_SPI_STATUS_REG); > + > + if (!wait_for_completion_interruptible_timeout(&pspim->isr_done, timeout)) { > + dev_err(&spi->dev, "wait_for_completion err\n"); > + return -ETIMEDOUT; > + } > + > + reg_temp = readl(pspim->m_base + SP7021_SPI_STATUS_REG); > + if (reg_temp & SP7021_FINISH_FLAG) { > + writel(SP7021_FINISH_FLAG, pspim->m_base + SP7021_SPI_STATUS_REG); > + writel(readl(pspim->m_base + SP7021_SPI_CONFIG_REG) & > + SP7021_CLEAN_FLUG_MASK, pspim->m_base + SP7021_SPI_CONFIG_REG); > + } > + > + if (pspim->xfer_conf & SP7021_CPOL_FD) > + writel(pspim->xfer_conf, pspim->m_base + SP7021_SPI_CONFIG_REG); > + > + mutex_unlock(&pspim->buf_lock); > + ret = 0; > + } > + return ret; > +} ... > +static int sp7021_spi_slave_transfer_one(struct spi_controller *ctlr, struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr); > + struct device *dev = pspim->dev; > + int mode, ret = 0; ret assignment can be moved to the default case below, where it will naturally fit. > + mode = SP7021_SPI_IDLE; > + if (xfer->tx_buf && xfer->rx_buf) { > + dev_dbg(&ctlr->dev, "%s() wrong command\n", __func__); > + ret = -EINVAL; Do you need this check if you properly set the capabilities of the controller? If still needed, why not return here? > + } else if (xfer->tx_buf) { > + xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf, > + xfer->len, DMA_TO_DEVICE); > + if (dma_mapping_error(dev, xfer->tx_dma)) > + return -ENOMEM; > + mode = SP7021_SLAVE_WRITE; > + } else if (xfer->rx_buf) { > + xfer->rx_dma = dma_map_single(dev, xfer->rx_buf, xfer->len, > + DMA_FROM_DEVICE); > + if (dma_mapping_error(dev, xfer->rx_dma)) > + return -ENOMEM; > + mode = SP7021_SLAVE_READ; > + } > + > + switch (mode) { > + case SP7021_SLAVE_WRITE: > + ret = sp7021_spi_slave_tx(spi, xfer); > + break; > + case SP7021_SLAVE_READ: > + ret = sp7021_spi_slave_rx(spi, xfer); > + break; > + default: > + break; > + } > + if (xfer->tx_buf) > + dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE); > + if (xfer->rx_buf) > + dma_unmap_single(dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE); Why can't you use SPI core DMA mapping code? > + spi_finalize_current_transfer(ctlr); > + return ret; > +} ... > + device_set_node(&ctlr->dev, pdev->dev.fwnode); Use dev_fwnode() in the second argument.
Hi Mr. Mark Brown : Thanks for your comment, I will modify it in next next submission > > > + for (i = 0; i <= xfer_cnt; i++) { > > + mutex_lock(&pspim->buf_lock); > > This lock is redundant: it is only ever held in this function which is guaranteed by the core to never be > called twice concurrently. I will modify it in next next submission > > + ret = devm_request_irq(dev, pspim->m_irq, sp7021_spi_master_irq, > > + IRQF_TRIGGER_RISING, pdev->name, pspim); > > + if (ret) > > + return ret; > > + > > + ret = devm_request_irq(dev, pspim->s_irq, sp7021_spi_slave_irq, > > + IRQF_TRIGGER_RISING, pdev->name, pspim); > > + if (ret) > > + return ret; > > Are you sure the driver is ready to handle interrupts without any of the other resources? Normally > interrupts are one of the last things to be requested. I will modify it in next next submission
Hi Andy Shevchenko : Thanks for your comment, I have some questions as below. > > + if (xfer->tx_buf) > > + dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE); > > + if (xfer->rx_buf) > > + dma_unmap_single(dev, xfer->rx_dma, xfer->len, > > + DMA_FROM_DEVICE); > > Why can't you use SPI core DMA mapping code? I didn't find the SPI core DMA mapping code for single maping. The method currently used is the general DMA single-map code usage method. > > > + device_set_node(&ctlr->dev, pdev->dev.fwnode); > > Use dev_fwnode() in the second argument. > You mean as below ? device_set_node(&ctlr->dev, dev_fwnode(dev)); Best Regards, Li-hao Kuo
On Thu, Jan 20, 2022 at 11:22 AM Lh Kuo 郭力豪 <lh.Kuo@sunplus.com> wrote: ... > > > + if (xfer->tx_buf) > > > + dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE); > > > + if (xfer->rx_buf) > > > + dma_unmap_single(dev, xfer->rx_dma, xfer->len, > > > + DMA_FROM_DEVICE); > > > > Why can't you use SPI core DMA mapping code? > > I didn't find the SPI core DMA mapping code for single maping. > The method currently used is the general DMA single-map code usage method. Why do you need single page mapping? What's wrong with SG mapping that SPI core provides? ... > > > + device_set_node(&ctlr->dev, pdev->dev.fwnode); > > > > Use dev_fwnode() in the second argument. > > You mean as below ? > > device_set_node(&ctlr->dev, dev_fwnode(dev)); Yes.
> > > > + if (xfer->tx_buf) > > > > + dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE); > > > > + if (xfer->rx_buf) > > > > + dma_unmap_single(dev, xfer->rx_dma, xfer->len, > > > > + DMA_FROM_DEVICE); > > > > > > Why can't you use SPI core DMA mapping code? > > > > I didn't find the SPI core DMA mapping code for single maping. > > The method currently used is the general DMA single-map code usage method. > > Why do you need single page mapping? > What's wrong with SG mapping that SPI core provides? SP7021 SPI slave dma only supports single dma in one trigger. It is not suitable for using SG mapping. If the length of the transfer is larger than the length of the SG-mapping, Slave-mode will get error in the transfer.
On Fri, Jan 21, 2022 at 8:54 AM Lh Kuo 郭力豪 <lh.Kuo@sunplus.com> wrote: > > > > > > + if (xfer->tx_buf) > > > > > + dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE); > > > > > + if (xfer->rx_buf) > > > > > + dma_unmap_single(dev, xfer->rx_dma, xfer->len, > > > > > + DMA_FROM_DEVICE); > > > > > > > > Why can't you use SPI core DMA mapping code? > > > > > > I didn't find the SPI core DMA mapping code for single maping. > > > The method currently used is the general DMA single-map code usage method. > > > > Why do you need single page mapping? > > What's wrong with SG mapping that SPI core provides? > > SP7021 SPI slave dma only supports single dma in one trigger. > It is not suitable for using SG mapping. > If the length of the transfer is larger than the length of the SG-mapping, > Slave-mode will get error in the transfer. Same story for SPI DesignWare on Intel Medfield, where no SG transfers are supported by hardware. Nevertheless, the DMA driver takes care of this and on each interrupt recharges a channel to continue. Why can't the same be implemented here?
> > > > > > + if (xfer->tx_buf) > > > > > > + dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE); > > > > > > + if (xfer->rx_buf) > > > > > > + dma_unmap_single(dev, xfer->rx_dma, xfer->len, > > > > > > + DMA_FROM_DEVICE); > > > > > > > > > > Why can't you use SPI core DMA mapping code? > > > > > > > > I didn't find the SPI core DMA mapping code for single maping. > > > > The method currently used is the general DMA single-map code usage method. > > > > > > Why do you need single page mapping? > > > What's wrong with SG mapping that SPI core provides? > > > > SP7021 SPI slave dma only supports single dma in one trigger. > > It is not suitable for using SG mapping. > > If the length of the transfer is larger than the length of the > > SG-mapping, Slave-mode will get error in the transfer. > > Same story for SPI DesignWare on Intel Medfield, where no SG transfers are supported by hardware. > Nevertheless, the DMA driver takes care of this and on each interrupt recharges a channel to continue. > Why can't the same be implemented here? > > I think it should work in master. spi master must actively send clk and date to slave device. And yes, in the "master" mode it can handle SG-DMA on each interrupt. But if working in "slave" mode, the master will not know the state of the slave. Slaves work on interrupt and recharge channels. When master send clk and date in the same time. It may lose data and errors occur
On Tue, 18 Jan 2022 16:42:37 +0800, Li-hao Kuo wrote: > This is a patch series for SPI driver for Sunplus SP7021 SoC. > > Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates > many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and > etc.) into a single chip. It is designed for industrial control. > > Refer to: > https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview > https://tibbo.com/store/plus1.html > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: Add spi driver for Sunplus SP7021 commit: f62ca4e2a863033d9b3b5a00a0d897557c9da6c5 [2/2] dt-bindings:spi: Add Sunplus SP7021 schema (no commit info) All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark