Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Sourav Poddar <sourav.poddar@ti.com>]

On Monday 29 July 2013 03:01 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jul 29, 2013 at 12:52:29PM +0530, Sourav Poddar wrote:
>> diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
>> new file mode 100644
>> index 0000000..51fe95f
>> --- /dev/null
>> +++ b/drivers/spi/spi-ti-qspi.c
> [ snip ]
>
>> +struct ti_qspi {
>> +	spinlock_t              lock;           /* IRQ synchronization */
>> +	struct spi_master	*master;
>> +	void __iomem            *base;
>> +	struct device           *dev;
>> +	struct completion       transfer_complete;
>> +	struct clk *fclk;
>> +	struct ti_qspi_regs     ctx_reg;
>> +	int device_type;
> device_type isn't used
>
My bad. yes, will remove. Was added for some experiments.
>> +	u32 spi_max_frequency;
>> +	u32 cmd;
>> +	u32 dc;
>> +};
> you need to make a choice here ? Are you going to tabify the structure
> or not ? You might also want to reorganize this structure to it looks
> like below:
>
> struct ti_qspi {
> 	struct completion	transfer_complete;
>
> 	/* IRQ synchronization */
> 	spinlock_t		lock;
>
> 	struct spi_master	*master;
> 	void __iomem		*base;
> 	struct clk		*fclk;
> 	struct device		*dev;
>
> 	struct ti_qspi_regs	ctx_reg;
>
> 	u32			spi_max_frequency;
> 	u32			cmd;
> 	u32			dc;
> };
>
hmm..yes will do.
>> +/* Status */
>> +#define QSPI_WC				(1<<  1)
>> +#define QSPI_BUSY			(1<<  0)
>> +#define QSPI_WC_BUSY			(QSPI_WC | QSPI_BUSY)
> WC_BUSY isn't used in this file. It looks unnecessary too.
>
Yes.
>> +#define QSPI_XFER_DONE			QSPI_WC
> so transfer done is word completion ? Why do you need this ? It's not
> even used in this source file.
>
Yes, this was used in ealier versons for polled mode. Will remove.
>> +static inline void ti_qspi_read_data(struct ti_qspi *qspi,
>> +		unsigned long reg, int wlen, u8 **rxbuf)
>> +{
>> +	switch (wlen) {
>> +	case 8:
>> +		**rxbuf = readb(qspi->base + reg);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
> you're incrementing only after printing, do you really need the -1 here?
>
> Also, I would change these into dev_vdbg(), it's quite unlikely someone
> needs to track all reads to the data register and since you're not
> printing the writes either, this looks even more like a case for
> dev_vdbg()
>
Ok.will change.
>> +		*rxbuf += 1;
>> +		break;
>> +	case 16:
>> +		**rxbuf = readw(qspi->base + reg);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
> %04x and no -1 (also, if you needed to decrement, it would have to be
> %-2).
>
Ok.
>> +		*rxbuf += 2;
>> +		break;
>> +	case 32:
>> +		**rxbuf = readl(qspi->base + reg);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
> %04x
>
OK
>> +		*rxbuf += 4;
>> +	default:
>> +		dev_dbg(qspi->dev, "word lenght out of range");
> s/lenght/length
>
hmm..will change.
>> +static int ti_qspi_setup(struct spi_device *spi)
>> +{
>> +	struct ti_qspi	*qspi = spi_master_get_devdata(spi->master);
>> +	struct ti_qspi_regs *ctx_reg =&qspi->ctx_reg;
>> +	int clk_div = 0, ret;
>> +	u32 clk_ctrl_reg, clk_rate, clk_mask;
>> +
>> +	clk_rate = clk_get_rate(qspi->fclk);
>> +
>> +	if (!qspi->spi_max_frequency) {
>> +		dev_err(qspi->dev, "spi max frequency not defined\n");
>> +		return -EINVAL;
> if you're returning here...
>
>> +	} else {
> this else is unneeded
>
hmm..true. Will change.
>> +		clk_div = DIV_ROUND_UP(clk_rate, qspi->spi_max_frequency) - 1;
>> +	}
>> +
>> +	dev_dbg(qspi->dev, "%s: hz: %d, clock divider %d\n", __func__,
>> +			qspi->spi_max_frequency, clk_div);
>> +
>> +	ret = pm_runtime_get_sync(qspi->dev);
>> +	if (ret) {
>> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
>> +		return ret;
>> +	}
> after Mark's patch, is this really needed ?
>
>> +	clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG);
>> +
>> +	clk_ctrl_reg&= ~QSPI_CLK_EN;
>> +
>> +	/* disable SCLK */
>> +	if (!spi->master->busy)
>> +		ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG);
>> +	else
>> +		dev_dbg(qspi->dev, "master busy doing other trasnfers\n");
> if master is busy, shouldn't you return -EBUSY at this point ?
>
Yes. will add.
>> +
>> +	if (clk_div<  0) {
>> +		dev_dbg(qspi->dev, "%s: clock divider<  0, using /1 divider\n",
>> +				__func__);
>> +		return -EINVAL;
> you do a get_sync without a put_sync() here.
>
Hmm..will add put_sync in error path.
>> +	}
>> +
>> +	if (clk_div>  QSPI_CLK_DIV_MAX) {
>> +		dev_dbg(qspi->dev, "%s: clock divider>%d , using /%d divider\n",
>> +			__func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1);
>> +		return -EINVAL;
> no put_sync()
>
Same.
>> +	}
>> +
>> +	/* enable SCLK */
>> +	clk_mask = QSPI_CLK_EN | clk_div;
>> +	ti_qspi_write(qspi, clk_mask, QSPI_SPI_CLOCK_CNTRL_REG);
>> +	ctx_reg->clkctrl = clk_mask;
>> +
>> +	pm_runtime_mark_last_busy(qspi->dev);
>> +	ret = pm_runtime_put_autosuspend(qspi->dev);
>> +	if (ret<  0) {
>> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ti_qspi_restore_ctx(struct ti_qspi *qspi)
>> +{
>> +	struct ti_qspi_regs *ctx_reg =&qspi->ctx_reg;
>> +
>> +	ti_qspi_write(qspi, ctx_reg->clkctrl, QSPI_SPI_CLOCK_CNTRL_REG);
>> +}
>> +
>> +static void qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>> +{
>> +	const u8 *txbuf;
>> +	int wlen, count;
>> +
>> +	count = t->len;
>> +	txbuf = t->tx_buf;
>> +	wlen = t->bits_per_word;
>> +
>> +	while (count--) {
>> +		dev_dbg(qspi->dev, "tx cmd %08x dc %08x data %02x\n",
>> +			qspi->cmd | QSPI_WR_SNGL, qspi->dc, *txbuf);
>> +		ti_qspi_write_data(qspi, *txbuf, QSPI_SPI_DATA_REG,
>> +					wlen,&txbuf);
>> +		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
>> +		ti_qspi_write(qspi, qspi->cmd | QSPI_WR_SNGL,
>> +			QSPI_SPI_CMD_REG);
>> +		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
>> +		wait_for_completion(&qspi->transfer_complete);
> you can't wait forever dude, wait_for_completion_timeout() and give it a
> 2 second timeout, then you can make this and qspi_read_msg() return
> something so you can notify that the controller didn't transfer anything
> in case your wait_for_completion_timeout() actually times out.
>
Ok.
>> +	}
>> +}
>> +
>> +static void qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>> +{
>> +	u8 *rxbuf;
>> +	int wlen, count;
>> +
>> +	count = t->len;
>> +	rxbuf = t->rx_buf;
>> +	wlen = t->bits_per_word;
>> +
>> +	while (count--) {
>> +		dev_dbg(qspi->dev, "rx cmd %08x dc %08x\n",
>> +			qspi->cmd | QSPI_RD_SNGL, qspi->dc);
>> +		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
>> +		ti_qspi_write(qspi, qspi->cmd | QSPI_RD_SNGL,
>> +			QSPI_SPI_CMD_REG);
>> +		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
>> +		wait_for_completion(&qspi->transfer_complete);
> wait_for_completion_timeout()
>
Ok.
>> +		ti_qspi_read_data(qspi, QSPI_SPI_DATA_REG, wlen,&rxbuf);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(rxbuf-1));
>> +	}
>> +}
>> +
>> +static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>> +{
>> +	if (t->tx_buf)
>> +		qspi_write_msg(qspi, t);
>> +	if (t->rx_buf)
>> +		qspi_read_msg(qspi, t);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_qspi_start_transfer_one(struct spi_master *master,
>> +		struct spi_message *m)
>> +{
>> +	struct ti_qspi *qspi = spi_master_get_devdata(master);
>> +	struct spi_device *spi = m->spi;
>> +	struct spi_transfer *t;
>> +	int status = 0, ret;
>> +	int frame_length;
>> +
>> +	/* setup device control reg */
>> +	qspi->dc = 0;
>> +
>> +	if (spi->mode&  SPI_CPHA)
>> +		qspi->dc |= QSPI_CKPHA(spi->chip_select);
>> +	if (spi->mode&  SPI_CPOL)
>> +		qspi->dc |= QSPI_CKPOL(spi->chip_select);
>> +	if (spi->mode&  SPI_CS_HIGH)
>> +		qspi->dc |= QSPI_CSPOL(spi->chip_select);
>> +
>> +	frame_length = (m->frame_length<<  3) / spi->bits_per_word;
> there's another way to optimize this. If you assume bits_per_word to
> always be power of two you can:
>
> 	frame_length = (m->frame_length<<  3)>>  __ffs(spi->bits_per_word);
>
> but that would need a comment stating that you're assuming bits_per_word
> to always be a power of two. Not sure if this is a correct assumption.
>
I dont think it will be a correct assumption, since our controller is 
flexible to
handle anything from 1 to 128 as bits_per_word.
>> +	frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
>> +
>> +	/* setup command reg */
>> +	qspi->cmd = 0;
>> +	qspi->cmd |= QSPI_EN_CS(spi->chip_select);
>> +	qspi->cmd |= QSPI_FLEN(frame_length);
>> +	qspi->cmd |= QSPI_WC_CMD_INT_EN;
>> +
>> +	list_for_each_entry(t,&m->transfers, transfer_list) {
>> +		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
>> +
>> +		ret = qspi_transfer_msg(qspi, t);
>> +		if (ret) {
>> +			dev_dbg(qspi->dev, "transfer message failed\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		m->actual_length += t->len;
>> +	}
>> +
>> +	m->status = status;
>> +	spi_finalize_current_message(master);
>> +
>> +	ti_qspi_write(qspi, qspi->cmd | QSPI_INVAL, QSPI_SPI_CMD_REG);
>> +
>> +	return status;
>> +}
>> +
>> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
>> +{
>> +	struct ti_qspi *qspi = dev_id;
>> +	u16 mask, stat;
>> +
>> +	irqreturn_t ret = IRQ_HANDLED;
>> +
>> +	spin_lock(&qspi->lock);
>> +
>> +	stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);
> since you're not reading the _RAW register, you don't need the mask
> below, right ?
>
I think yes, only status register should be sufficient.
>> +	mask = ti_qspi_read(qspi, QSPI_INTR_ENABLE_SET_REG);
>> +
>> +	if (!(stat&  mask)) {
>> +		dev_dbg(qspi->dev, "No IRQ triggered\n");
>> +		ret = IRQ_NONE;
>> +	}
>> +
>> +	ret = IRQ_WAKE_THREAD;
> look at this code and tell me when will you *EVER* return IRQ_NONE. Dude
> you're waking up the IRQ thread even when there are no IRQs to be
> handled.
>
My bad, should add a return ret before IRQ_NONE.
>> +	spin_unlock(&qspi->lock);
> You should, before releasing the lock, mask your IRQs, so that you can
> get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ
> thread.
>
Sorry, did not get  you here?
  I am reading interrupt status register before and clearing them in the 
threaded irq.
>> +static int ti_qspi_probe(struct platform_device *pdev)
>> +{
>> +	struct  ti_qspi *qspi;
>> +	struct spi_master *master;
>> +	struct resource         *r;
>> +	struct device_node *np = pdev->dev.of_node;
>> +	u32 max_freq;
>> +	int ret = 0, num_cs, irq;
>> +
>> +	master = spi_alloc_master(&pdev->dev, sizeof(*qspi));
>> +	if (!master)
>> +		return -ENOMEM;
>> +
>> +	master->mode_bits = SPI_CPOL | SPI_CPHA;
>> +
>> +	master->num_chipselect = 1;
> again with the num_chipselect = 1 ? IIRC this controller has 4
> chipselects, just read 24.5.1 on your TRM.
>
this is unnecessary. I intended to configure chip selects through dt)as 
done below).
Will remove.
>> +	master->bus_num = -1;
>> +	master->flags = SPI_MASTER_HALF_DUPLEX;
>> +	master->setup = ti_qspi_setup;
>> +	master->auto_runtime_pm = true;
>> +	master->transfer_one_message = ti_qspi_start_transfer_one;
>> +	master->dev.of_node = pdev->dev.of_node;
>> +	master->bits_per_word_mask = BIT(32 - 1) | BIT(16 - 1) | BIT(8 - 1);
>> +
>> +	if (!of_property_read_u32(np, "num-cs",&num_cs))
>> +		master->num_chipselect = num_cs;
> so you reinitialize here num_chipselect, then why do you initialize it
> above ?
>
>> +	platform_set_drvdata(pdev, master);
>> +
>> +	qspi = spi_master_get_devdata(master);
>> +	qspi->master = master;
>> +	qspi->dev =&pdev->dev;
>> +
>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (r == NULL) {
>> +		ret = -ENODEV;
>> +		goto free_master;
>> +	}
> you don't need to check the resource when using devm_ioremap_resource().
>
Ok.
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq<  0) {
>> +		dev_err(&pdev->dev, "no irq resource?\n");
>> +		return irq;
>> +	}
>> +
>> +	spin_lock_init(&qspi->lock);
>> +
>> +	qspi->base = devm_ioremap_resource(&pdev->dev, r);
>> +	if (IS_ERR(qspi->base)) {
>> +		ret = PTR_ERR(qspi->base);
>> +		goto free_master;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
>> +			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> why do you need IRQF_NO_SUSPEND ?
>
I should get away with this.