Re: [PATCHv2] drivers: spi: Add qspi flash controller

[Sourav Poddar]

On Tuesday 02 July 2013 04:01 PM, Felipe Balbi wrote:
> Hi,
>
> On Tue, Jul 02, 2013 at 03:53:49PM +0530, Sourav Poddar wrote:
> > On Tuesday 02 July 2013 03:46 PM, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Tue, Jul 02, 2013 at 03:30:42PM +0530, Sourav Poddar wrote:
> > > > > > +static int dra7xxx_qspi_setup(struct spi_device *spi)
> > > > > > +{
> > > > > > +	struct dra7xxx_qspi	*qspi =
> > > > > > +			spi_master_get_devdata(spi->master);
> > > > > > +
> > > > > > +	int clk_div;
> > > > > > +
> > > > > > +	if (!qspi->spi_max_frequency)
> > > > > > +		clk_div = 0;
> > > > > won't this generate division by zero ?
> > > > Yes, Probably only an error should be thrown here. ?
> > > > since min clk_div should be kept at 1.
> > > right, if spi_max_frequency isn't passed, this is a broken DT binding.
> > > Bail out.
> > >
> > > > > > +	pm_runtime_get_sync(qspi->dev);
> > > > > > +
> > > > > > +	/* disable SCLK */
> > > > > > +	dra7xxx_writel(qspi, dra7xxx_readl(qspi, QSPI_SPI_CLOCK_CNTRL_REG)
> > > > > > +			&    ~QSPI_CLK_EN, QSPI_SPI_CLOCK_CNTRL_REG);
> > > > > > +
> > > > > > +	if (clk_div<    0) {
> > > btw, add a space between clk_div and<
> > >
> > > > > > +			dra7xxx_writel(qspi, *txbuf++, QSPI_SPI_DATA_REG);
> > > > > > +			dra7xxx_writel(qspi, qspi->dc, QSPI_SPI_DC_REG);
> > > > > > +			dra7xxx_writel(qspi, qspi->cmd | QSPI_WR_SNGL,
> > > > > > +					QSPI_SPI_CMD_REG);
> > > > > > +			status = dra7xxx_readl(qspi, QSPI_SPI_STATUS_REG);
> > > > > > +			timeout = QSPI_TIMEOUT;
> > > > > > +			while ((status&    QSPI_WC_BUSY) != QSPI_XFER_DONE) {
> > > > > do you really need to poll ? No IRQ available ?
> > > > There is an interrupt available, I will try using that.
> > > look at how i2c-omap.c synchronizes interrupt with the transfer_msg
> > > code. It just uses a wait_for_completion().
> > Ok.
> > > > > > +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 flags = 0;
> > > > > > +
> > > > > > +	/* setup command reg */
> > > > > > +	qspi->cmd = 0;
> > > > > > +	qspi->cmd |= QSPI_WLEN(8);
> > > > > > +	qspi->cmd |= QSPI_EN_CS(0);
> > > > > > +	qspi->cmd |= 0xfff;
> > > > Since, we dont know the number of frame lenght that need to be
> > > > transferred and it comes from the spi framework, we keep the frame
> > > > lenght to maximum.
> > > > Then depending on the count value above in while loop, we terminate
> > > > our trasnfer.
> > > what ? seriously didn't get what you meant.
> > I mean, the lower 12 bits of cmd register is meant to be filled with
> > frame lenght.
> >
> > But the frame lenght is parsed when you iterate the list. So, what is
> which list ?
message list, from which we iterate through each transfers.
> > done here is that
> > the framelenght is kept to its maximum value.
> why ? That seems wrong. If you can get the actual frame length at some
> point, that's what you should use.
Ok.Then probably it makes sense to have frame count interrupt also to
signal the end of frame.
> > Then, to signal the the end of the frame, we use
> >
> > static int qspi_transfer_msg(struct dra7xxx_qspi *qspi, unsigned count,
> >                          const u8 *txbuf, u8 *rxbuf, bool flags)
> > {
> >          uint status;
> >          int timeout;
> >
> >          while (count--) {
> >                  if (txbuf) {
> >                          pr_debug("tx cmd %08x dc %08x data %02x\n",
> >                                  qspi->cmd | QSPI_WR_SNGL, qspi->dc,
> > *txbuf);
> >                          dra7xxx_writel(qspi, *txbuf++, QSPI_SPI_DATA_REG);
> >                          dra7xxx_writel(qspi, qspi->dc, QSPI_SPI_DC_REG);
> >                          dra7xxx_writel(qspi, qspi->cmd | QSPI_WR_SNGL,
> >                                          QSPI_SPI_CMD_REG);
> >                          status = dra7xxx_readl(qspi, QSPI_SPI_STATUS_REG);
> >                          timeout = QSPI_TIMEOUT;
> >                          while ((status&  QSPI_WC_BUSY) != QSPI_XFER_DONE) {
> >                                  if (--timeout<  0) {
> >                                          pr_debug("QSPI tx timed out\n");
> >                                          return
> >
> >       .............................
> >
> >                                     status, *(rxbuf-1));
> >                  }
> >          }
> >
> >          if (flags&  XFER_END)
> >                  dra7xxx_writel(qspi, qspi->cmd | QSPI_INVAL,
> > QSPI_SPI_CMD_REG);
> >
> > }
> > INVAL will terminate the current frame.
> nevermind that this value is "RESERVED" on the documentation. You should
> not rely on reserved features, they can go away at any point in time.
>
> That's probably there only for some IP debugging kinda thing.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/