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@xxxxxxxxxxxxx>
> > ---
> > + /* 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
