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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130709065143.GC5552@arwen.pp.htv.fi>
Date:	Tue, 9 Jul 2013 09:51:43 +0300
From:	Felipe Balbi <balbi@...com>
To:	Nishanth Menon <nm@...com>
CC:	Sourav Poddar <sourav.poddar@...com>, <broonie@...nel.org>,
	<spi-devel-general@...ts.sourceforge.net>,
	<grant.likely@...aro.org>, <balbi@...com>, <rnayak@...com>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv3 2/3] drivers: spi: Add qspi flash controller

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

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)