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 ?
> + }
> +}
> +
> +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
> + } else
this needs to have curly braces too, per CodingStyle
> + 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 ?
> + 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 ?
> + 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
> + 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.
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.
> + 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->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
--
balbi
Attachment:
signature.asc
Description: Digital signature
