| 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
| ||
|
Date: Tue, 9 Jul 2013 12:59:26 +0530
From: Sourav Poddar <sourav.poddar@...com>
To: <balbi@...com>
CC: <broonie@...nel.org>, <spi-devel-general@...ts.sourceforge.net>,
<grant.likely@...aro.org>, <rnayak@...com>,
<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv3 2/3] drivers: spi: Add qspi flash controller
On Monday 08 July 2013 08:02 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jul 08, 2013 at 07:12:59PM +0530, Sourav Poddar wrote:
>> +static inline unsigned long dra7xxx_readl(struct dra7xxx_qspi *qspi,
>> + unsigned long reg)
>> +{
>> + return readl(qspi->base + reg);
>> +}
>> +
>> +static inline void dra7xxx_writel(struct dra7xxx_qspi *qspi,
>> + unsigned long val, unsigned long reg)
>> +{
>> + writel(val, qspi->base + reg);
>> +}
>> +
>> +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;
> return -EINVAL ? or some other error code ?
>
Ok.will change.
>> + }
>> +}
>> +
>> +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;
>> + }
>> +}
>> +
>> +static int dra7xxx_qspi_setup(struct spi_device *spi)
>> +{
>> + struct dra7xxx_qspi *qspi = spi_master_get_devdata(spi->master);
>> + int clk_div = 0;
>> + u32 clk_ctrl_reg, clk_rate;
>> +
>> + clk_rate = clk_get_rate(qspi->fclk);
>> +
>> + if (!qspi->spi_max_frequency) {
>> + dev_err(qspi->dev, "spi max frequency not defined\n");
>> + return -1;
> same here
>
Ok.
>> + } else
> this needs to have curly braces too, per CodingStyle
>
hmm..will change.
>> + clk_div = (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);
>> +
>> + pm_runtime_get_sync(qspi->dev);
>> +
>> + clk_ctrl_reg = dra7xxx_readl(qspi, QSPI_SPI_CLOCK_CNTRL_REG);
>> +
>> + clk_ctrl_reg&= ~QSPI_CLK_EN;
>> +
>> + /* disable SCLK */
>> + dra7xxx_writel(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__);
>> + clk_div = 1;
>> + }
>> +
>> + 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);
>> + clk_div = QSPI_CLK_DIV_MAX;
>> + }
>> +
>> + /* enable SCLK */
>> + dra7xxx_writel(qspi, QSPI_CLK_EN | clk_div, QSPI_SPI_CLOCK_CNTRL_REG);
>> +
>> + pm_runtime_mark_last_busy(qspi->dev);
>> + pm_runtime_put_autosuspend(qspi->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int dra7xxx_qspi_prepare_xfer(struct spi_master *master)
>> +{
>> + struct dra7xxx_qspi *qspi = spi_master_get_devdata(master);
>> +
>> + pm_runtime_get_sync(qspi->dev);
> not going to check return value ?
>
Will add.
>> + return 0;
>> +}
>> +
>> +static int dra7xxx_qspi_unprepare_xfer(struct spi_master *master)
>> +{
>> + struct dra7xxx_qspi *qspi = spi_master_get_devdata(master);
>> +
>> + pm_runtime_mark_last_busy(qspi->dev);
>> + pm_runtime_put_autosuspend(qspi->dev);
> what about on these two ?
>
Yes, will add error checking.
>> + return 0;
>> +}
>> +
>> +static int qspi_write_msg(struct dra7xxx_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);
>> + dra7xxx_writel(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
> you should enable the interrupt as the last step. Also, why aren't you
> using frame interrupt ?
>
>> + dra7xxx_writel_data(qspi, *txbuf++, QSPI_SPI_DATA_REG, wlen);
>> + dra7xxx_writel(qspi, qspi->dc, QSPI_SPI_DC_REG);
>> + dra7xxx_writel(qspi, qspi->cmd | QSPI_WR_SNGL,
>> + QSPI_SPI_CMD_REG);
>> + wait_for_completion(&qspi->word_complete);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qspi_read_msg(struct dra7xxx_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);
>> + dra7xxx_writel(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
> ditto
>
hmm. will move this down.
>> + dra7xxx_writel(qspi, qspi->dc, QSPI_SPI_DC_REG);
>> + dra7xxx_writel(qspi, qspi->cmd | QSPI_RD_SNGL,
>> + QSPI_SPI_CMD_REG);
>> + wait_for_completion(&qspi->word_complete);
>> + *rxbuf++ = dra7xxx_readl_data(qspi, QSPI_SPI_DATA_REG, wlen);
>> + dev_dbg(qspi->dev, "rx done, read %02x\n", *(rxbuf-1));
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qspi_transfer_msg(struct dra7xxx_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 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,
> why this multiplication ? Frame is not counted in bits but in number of
> words. Which means that m->framelength / spi->bits_per_word should be
> enough.
Yes, got confused here. Will change.
> Also, this actually brings up a mistake I made, if I read the TRM
> correclty this time, frame length is nothing but the t->len which
> renders patch 1 in this series unnecessary, my mistake.
>
If flen is t->len, then
1.) yes patch 1 is useles.
2.) frame interrupt will make more sense then.
Till now, we were using flen as sum of all transfers, because of
which
I was forced to check for word complete.
I will check on "flen" and will get back on frame interrupt with
experimental results.
>> + 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);
> right, right, so far so good...
>
>> + list_for_each_entry(t,&m->transfers, transfer_list) {
>> + qspi->cmd |= QSPI_WLEN(t->bits_per_word);
> sure about this ? according to TRM a write of 0 means 1 bit, 1, means 2
> bits, 2 means 3 bits, etc... So this should be t->bits_per_word - 1.
>
QSPI_WLEN is taking care of subtrating it by -1.
>> + qspi->cmd |= QSPI_WC_CMD_INT_EN;
> ... but why are you still interrupting on every word ????
>
> as I said before, we want to interrupt on every frame. one spi_transfer
> is one frame, if that frame has a length of 1MiB, I want to be
> interrupted after 1MiB.
>> + qspi_transfer_msg(qspi, t);
>> + m->actual_length += t->len;
>> +
>> + if (list_is_last(&t->transfer_list,&m->transfers))
>> + goto out;
>> + }
>> +
>> +out:
>> + m->status = status;
>> + spi_finalize_current_message(master);
>> +
>> + dra7xxx_writel(qspi, qspi->cmd | QSPI_INVAL, QSPI_SPI_CMD_REG);
>> +
>> + return status;
>> +}
>> +
>> +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);
>> +
>> + 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))
> this is wrong, everytime you want to handle another IRQ bit, you will
> have to update this line.
>
> if (stat& mask)
>
> sounds like it's enough
>
Yes, correct will change.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists