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

[Kamal Dasu <kamal.dasu@xxxxxxxxxxxx>]

Mark,

I have  implemented most of your suggestions.  A few of them, actually
just a couple,  I wanted to discuss further. More importantly related
to the spi_flash_read(). Please see my questions inline.

On Tue, Jun 14, 2016 at 12:58 PM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx> 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?

This bit has no effect and  is not connected actually although
software is manipulating it. The real CS is the one we use via the dt
cs_reg. I will fix the comments/code to reflect this accordingly.

>
>> >> +             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.

I was looking at spi_flash_read() caled from m25p80_read()  and spi
core implementation. I have a case where when we have very small reads
or  unaligned buffers and addresses passed then I have to fall back to
using standard MSPI reads. How best to handle this ?. One of the
options without having to replicate code  involves a minor change to
m25p80 driver as shown below.
---
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9d68544..e98b4fa 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -149,8 +149,11 @@ static int m25p80_read(struct spi_nor *nor,
loff_t from, size_t len,
                msg.data_nbits = m25p80_rx_nbits(nor);

                ret = spi_flash_read(spi, &msg);
-               *retlen = msg.retlen;
-               return ret;
+               /* check we need to fallback to normal spi read */
+               if (ret != -EAGAIN) {
+                       *retlen = msg.retlen;
+                       return ret;
+               }
        }

        spi_message_init(&m);
---

I am open to any other suggestion that I can implement in my driver.
In the previous version of using transfer_one_message() I could
fallback in such cases.

>> >> +     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.

Are you suggesting to use something like spi_read_then_write() in
cases where I want to set the addressing and single/quad modes ?.

Thanks
Kamal
--
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