Re: [PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

From: Huang Shijie <shijie.huang@intel.com>
To: Brian Norris <computersforpeace@gmail.com>
Cc: Huang Shijie <b32955@freescale.com>,
	marex@denx.de, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-spi@vger.kernel.org,
	linux-mtd@lists.infradead.org, dwmw2@infradead.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 03/10] mtd: spi-nor: add DDR quad read support
Date: Wed, 30 Jul 2014 14:44:13 +0800	[thread overview]
Message-ID: <20140730064413.GA15547@shldeISGChi005.sh.intel.com> (raw)
In-Reply-To: <20140730050843.GE11952@brian-ubuntu>

On Tue, Jul 29, 2014 at 10:08:43PM -0700, Brian Norris wrote:
> 
> On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote:
> > This patch adds the DDR quad read support by the following:
> 
> To Mark / linux-spi:
> 
> Are DDR modes in the scope of drivers/spi/ at all, so that we could
> someday wire it up through m25p80.c? Or is 'DDR' such a distortion of
> the meaning of 'SPI' such that it will be restricted only to SPI NOR
> dedicated controllers?

IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.
The SPI can only handles the byte streams. But the DDR mode may need to
handle the cycles(such as the dummy cycles could be 7 cycles) which is not byte.

So the DDR mode is handled by the SPI NOR controller now.

Please correct me if I am wrong. :)

> 
> >   [1] add SPI_NOR_DDR_QUAD read mode.
> > 
> >   [2] add DDR Quad read opcodes:
> >        SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D
> > 
> >   [3] add set_ddr_quad_mode() to initialize for the DDR quad read.
> >       Currently it only works for Spansion NOR.
> > 
> >   [3] about the dummy cycles.
> >       We set the dummy with 8 for DDR quad read by default.
> 
> Why? That seems wrong. You need to know for sure how many cycles should
> be used, not just guess a default.

Do you mean that if people do not set the DT node for dummy, we should
return an -EINVAL immediately?

> 
> >       The m25p80.c can not support the DDR quad read, but the SPI NOR controller
> >       can set the dummy value in its child DT node, and the SPI NOR framework
> >       can parse it out.
> 
> Why does the dummy value belong in device tree? I think this can be
> handled in software. You might, however, want a few other hardware
> description parameters in device tree to help you.
> 
> So I think spi-nor.c needs to know a few things:
> 
>  1. Does the hardware/driver support DDR clocking?
>  2. What granularity of dummy cycles are supported? So m25p80.c needs to
>     communicate that it only supports dummy cycles of multiples of 8,
>     and fsl-quadspi supports single clock cycles.

I think you can send patches for these features. I does not clear about:
  for what does the spi-nor needs to know the above things.

> And spi-nor.c should be able to do the following:
> 
>  3. Set how many dummy cycles should be used.
where can we get the number of the cycles? 

>  4. Write to the configuration register, to choose a Latency Code
>     according to what the flash supports. I see modes that support 3, 6,
>     7, or 8. We'd probably just go for the fastest mode, which requires
>     8, right?
not right.

The DDR mode can not work if we set a wrong dummy cycles for the flash.

for some chips, the fastest mode may need 6 cycles, not 8. 

> 
> So far, none of this seems to require a DT binding, unless there's
> something I'm missing about your fsl-quadspi controller.
> 
> > Test this patch for Spansion s25fl128s NOR flash.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> > +	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
> > +	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {
> 
> Hmm, I think I should probably take another look at the design of
> spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The
> driver should be communicating which (multiple) modes it supports, not
> selecting a single mode. spi-nor.c is the only one which knows what the
> *flash* supports, so it should be combining knowledge from the
> controller driver with its own knowledge of the flash.

It is okay for me to add multiples modes for the spi_nor_scan().
I added the single mode for spi_nor_scan is because that the fsl-quadspi
does not want to support the low speed modes. (Of course, the fsl-quadspi
controller does support the low speed modes.)

> 
> > +		ret = set_ddr_quad_mode(nor, info->jedec_id);
> > +		if (ret) {
> > +			dev_err(dev, "DDR quad mode not supported\n");
> > +			return ret;
> 
> A ramification of my comment above is that we should not be returning an
> error in a situation like this; we should be able to fall back to
> another known-supported mode, like SDR QUAD, SDR DUAL, or just plain
> SPI -- if they're supported by the driver -- and spi-nor.c doesn't
> currently have enough information to do this.
ok.
> 
> > +		}
> > +		nor->flash_read = SPI_NOR_DDR_QUAD;
> >  
> >  /**
> 
> So, I'll have to take another hard look at spi-nor.c soon. I think we
> may need to work on the abstractions here a bit more before I merge any
> new features like this.

okay.  no problem.

> 
> Regards,
> Brian
> 
> P.S. Is there a good reason we've defined a whole read_xfer/write_xfer
> API that is completely unused?
These hooks are designed for other SPI NOR drivers, the fsl-quadspi does
not use them.

thanks
Huang Shijie