Re: [V3, 2/4] spi: bcm-qspi: Add SPI flash and MSPI driver

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2628 bytes --]

On Tue, Jun 14, 2016 at 11:43:56AM -0400, Kamal Dasu wrote:
> On Mon, Jun 13, 2016 at 6:13 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Fri, Jun 10, 2016 at 04:06:09PM -0400, Kamal Dasu wrote:

> >> +     /* deassert CS on the final byte */
> >> +     if (fnb & FNB_BREAK_DESELECT) {
> >> +             mspi_cdram = read_cdram_slot(qspi, slot - 1) &
> >> +                     ~MSPI_CDRAM_CONT_BIT;
> >> +             write_cdram_slot(qspi, slot - 1, mspi_cdram);
> >> +     }

> > Let the core handle asserting and deasserting chip select.

> Its actually a peripheral CS (PCS) bit in the MSPI block. Peripheral
> chip selects are used to select an external device for serial data
> transfer. PCS[i] corresponds to pin SS[i].

That doesn't appear to address the issue, what you're describing sounds
like the normal function of the chip select?

> >> +             switch (command) {
> >> +             case SPINOR_OP_READ4_FAST:
> >> +                     if (!bcm_qspi_is_4_byte_mode(qspi))

> > No, this is not a good idea. You should not be attempting to parse
> > the data stream.  If your controller has flash support it should
> > implement the flash interfaces, otherwise it should just pass the data
> > streams through.

> This is only for read command we need to setup both tx and rx at the
> same time for improved performance.

No, to repeat what I said if you want to support accelerated flash reads
implement the standard accelerated flash read operation we have
(spi_flash_read()).  This is a fairly common feature and trying to open
code this in individual drivers would at the very least lead to a lot of
duplicated code and is the source of most of the problems in the code.

> >> +     spi_message_init(&m);
> >> +     m.spi = &spi;

> > I don't know why your driver is creating and trying to do SPI transfers
> > but that is completely inappropriate for a SPI controller.  Whatever
> > functionality this is implementing should be in a separate SPI device
> > driver.

> Currently not used. But I did not see a good way to be able to send
> commands, like finding the ID, setting 3/4 byte addressing mode , quad
> mode, on a PM resume(). Are you suggesting I make a driver similar to
> m25p80  ?.

I am suggesting you implement the standard flash interfaces and as a
result directly use m25p80.

> >> +MODULE_DEVICE_TABLE(of, bcm_qspi_of_match);

> > This is adding new DT bindings but there is no documentation,
> > documentation is required for all DT bindings.

> This was part of Patch 1/4 as pointed to by Florian

Please use standard formatting when posting patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]