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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 8264 bytes --]

Hi,

On Wed, Jul 31, 2013 at 11:17:52AM +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..3d10b69
> --- /dev/null
> +++ b/drivers/spi/spi-ti-qspi.c
> @@ -0,0 +1,545 @@

<snip>

> +/* Device Control */
> +#define QSPI_DD(m, n)			(m << (3 + n*8))
> +#define QSPI_CKPHA(n)			(1 << (2 + n*8))
> +#define QSPI_CSPOL(n)			(1 << (1 + n*8))
> +#define QSPI_CKPOL(n)			(1 << (n*8))

add spaces around the * operator

> +#define	QSPI_FRAME_MAX			0xfff

Frame max is 4096, 0x1000, right ?

> +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_vdbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf));
> +		*rxbuf += 1;
> +		break;
> +	case 16:
> +		**rxbuf = readw(qspi->base + reg);
> +		dev_vdbg(qspi->dev, "rx done, read %04x\n", *(*rxbuf));
> +		*rxbuf += 2;
> +		break;
> +	case 32:
> +		**rxbuf = readl(qspi->base + reg);
> +		dev_vdbg(qspi->dev, "rx done, read %04x\n", *(*rxbuf));

%08x, this was commented before.

> +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;
> +	}
> +
> +	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;
> +	}
> +
> +	clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG);
> +
> +	clk_ctrl_reg &= ~QSPI_CLK_EN;
> +
> +	if (spi->master->busy) {
> +		dev_dbg(qspi->dev, "master busy doing other trasnfers\n");
> +		return -EBUSY;
> +	}

this check can be done before pm_runtime_get_sync(), you're also leaking
pm_runtime reference here.

> +	/* disable SCLK */
> +	ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG);
> +
> +	if (clk_div < 0) {
> +		dev_dbg(qspi->dev, "%s: clock divider < 0, using /1 divider\n",
> +				__func__);
> +		pm_runtime_put_sync(qspi->dev);
> +		return -EINVAL;
> +	}
> +
> +	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);
> +		pm_runtime_put_sync(qspi->dev);
> +		return -EINVAL;
> +	}

why don't you move all checks to clk_div before pm_runtime_get_sync()
call ?

> +static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> +{
> +	const u8 *txbuf;
> +	int wlen, count, ret;
> +
> +	count = t->len;
> +	txbuf = t->tx_buf;
> +	wlen = t->bits_per_word;
> +
> +	while (count--) {

you're decrementing count by one, but in some cases you write 4 bytes or
2 bytes... This will blow up very soon. I can already see overflows
happening...

> +static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> +{
> +	u8 *rxbuf;
> +	int wlen, count, ret;
> +
> +	count = t->len;
> +	rxbuf = t->rx_buf;
> +	wlen = t->bits_per_word;
> +
> +	while (count--) {

ditto

> +static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> +{
> +	int ret;
> +
> +	if (t->tx_buf) {
> +		ret = qspi_write_msg(qspi, t);
> +		if (ret) {
> +			dev_dbg(qspi->dev, "Error while writing\n");
> +			return -EINVAL;

why do you change the return value from qspi_write_msg() ?

> +		}
> +	}
> +
> +	if (t->rx_buf) {
> +		ret = qspi_read_msg(qspi, t);
> +		if (ret) {
> +			dev_dbg(qspi->dev, "Error while writing\n");
> +			return -EINVAL;

why do you change the return value from qspi_read_msg() ?

> +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;
> +
> +	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;
> +
> +	ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
> +
> +	list_for_each_entry(t, &m->transfers, transfer_list) {

no locking around list traversal ?

> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
> +{
> +	struct ti_qspi *qspi = dev_id;
> +	u16 stat;
> +
> +	irqreturn_t ret = IRQ_HANDLED;
> +
> +	spin_lock(&qspi->lock);
> +
> +	stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);
> +
> +	if (!stat) {
> +		dev_dbg(qspi->dev, "No IRQ triggered\n");
> +		return IRQ_NONE;

leaving lock held.

> +static irqreturn_t ti_qspi_threaded_isr(int this_irq, void *dev_id)
> +{
> +	struct ti_qspi *qspi = dev_id;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&qspi->lock, flags);
> +
> +	complete(&qspi->transfer_complete);

you need to check if your word completion is actually set. Don't assume
it's set because we might want to change the code later.

> +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->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;
> +
> +	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);
> +
> +	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, 0,
> +			dev_name(&pdev->dev), qspi);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
> +				irq);
> +		goto free_master;
> +	}
> +
> +	qspi->fclk = devm_clk_get(&pdev->dev, "fck");
> +	if (IS_ERR(qspi->fclk)) {
> +		ret = PTR_ERR(qspi->fclk);
> +		dev_err(&pdev->dev, "could not get clk: %d\n", ret);
> +	}
> +
> +	init_completion(&qspi->transfer_complete);
> +
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, QSPI_AUTOSUSPEND_TIMEOUT);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	if (!of_property_read_u32(np, "spi-max-frequency", &max_freq))
> +		qspi->spi_max_frequency = max_freq;
> +
> +	ret = spi_register_master(master);
> +	if (ret)
> +		goto free_master;
> +
> +	return ret;

you only get here with success, so return 0 is alright.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]