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 linux-spi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html