Re: [PATCHv3 2/3] drivers: spi: Add qspi flash controller

[Felipe Balbi]

Hi,

On Mon, Jul 08, 2013 at 03:33:30PM -0500, Nishanth Menon wrote:
> > +static inline unsigned long dra7xxx_readl_data(struct dra7xxx_qspi *qspi,
> > +		unsigned long reg, int wlen)
> > +{
> > +	switch (wlen) {
> > +	case 8:
> > +		return readw(qspi->base + reg);
> > +		break;
> > +	case 16:
> > +		return readb(qspi->base + reg);
> > +		break;
> > +	case 32:
> > +		return readl(qspi->base + reg);
> > +		break;
> > +	default:
> > +		return -1;
> > +	}
> > +}
> > +
> > +static inline void dra7xxx_writel_data(struct dra7xxx_qspi *qspi,
> > +		unsigned long val, unsigned long reg, int wlen)
> > +{
> > +	switch (wlen) {
> > +	case 8:
> > +		writew(val, qspi->base + reg);
> > +		break;
> > +	case 16:
> > +		writeb(val, qspi->base + reg);
> > +		break;
> > +	case 32:
> > +		writeb(val, qspi->base + reg);
> > +		break;
> > +	default:
> > +		dev_dbg(qspi->dev, "word lenght out of range");
> > +		break;
> > +	}
> > +}
> Looks like a case to use regmap?
> Dumb q: why cant we use regmap_spi? worst case, you should be able to

read regmap-spi and you'll see why it can't be used in this case.

regmap-spi is for SPI clients who want to read their register map
through SPI commands. This is a driver for the SPI master which has its
registers memory mapped.

> > +static int dra7xxx_qspi_start_transfer_one(struct spi_master *master,
> > +		struct spi_message *m)
> > +{
> > +	struct dra7xxx_qspi *qspi = spi_master_get_devdata(master);
> > +	struct spi_device *spi = m->spi;
> > +	struct spi_transfer *t;
> > +	int status = 0;
> > +	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 = DIV_ROUND_UP(m->frame_length * spi->bits_per_word,
> > +				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);
> > +
> how does one ensure pm runtime has not disabled clocks in
> background? e.g. long latency between transfers.

because pm_runtime_put*() has not been called ?

There's no way clocks will be gated until we kick the pm autosuspend
timer, which is only done when the transfer is finished.

> > +static irqreturn_t dra7xxx_qspi_isr(int irq, void *dev_id)
> > +{
> > +	struct dra7xxx_qspi *qspi = dev_id;
> > +	u16 mask, stat;
> > +
> > +	irqreturn_t ret = IRQ_HANDLED;
> > +
> > +	spin_lock(&qspi->lock);
> > +
> what if autosuspend has triggered here? before ISR was scheduled?

how ?

the pm timer hasn't been kicked yet

> > +	stat = dra7xxx_readl(qspi, QSPI_SPI_STATUS_REG);
> > +	mask = dra7xxx_readl(qspi, QSPI_SPI_CMD_REG);
> > +
> > +	if ((stat & QSPI_WC) && (mask & QSPI_WC_CMD_INT_EN))
> > +		ret = IRQ_WAKE_THREAD;
> > +
> > +	spin_unlock(&qspi->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t dra7xxx_qspi_threaded_isr(int this_irq, void *dev_id)
> > +{
> > +	struct dra7xxx_qspi *qspi = dev_id;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&qspi->lock, flags);
> > +
> what if autosuspend has triggered here? before ISR was scheduled?

how ?

pm timer hasn't been kicked yet

> > +	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> > +	if (IS_ERR(qspi->base)) {
> > +		ret = -ENOMEM;
> > +		goto free_master;
> > +	}
> why not use devm_request_and_ioremap? Lock that region down so that no
> two drivers can manage the same region?

read devm_ioremap_resource() and look at the git log for all the
numerous drivers which were converted to devm_ioremap_resource() to find
the reason.

> > +static struct platform_driver dra7xxx_qspi_driver = {
> > +	.probe	= dra7xxx_qspi_probe,
> > +	.remove	= dra7xxx_qspi_remove,
> > +	.driver = {
> > +		.name	= "ti,dra7xxx-qspi",
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = dra7xxx_qspi_match,
> > +	}
> no need for pm_ops?

+1

> > +};
> > +
> > +module_platform_driver(dra7xxx_qspi_driver);
> > +
> > +MODULE_LICENSE("GPL");
> GPL V2?

+1

> > +MODULE_DESCRIPTION("TI QSPI controller driver");
> MODULE_AUTHOR?

+1

-- 
balbi

Attachment: signature.asc
Description: Digital signature