Re: [RFC PATCH 0/5] RFC for Zynq QSPI

[naga suresh kumar <nagasureshkumarrelli@xxxxxxxxx>]

Hi Boris,

On Tue, Sep 18, 2018 at 12:47 PM Boris Brezillon
<boris.brezillon@xxxxxxxxxxx> wrote:
>
> Hi Naga,
>
> On Tue, 18 Sep 2018 11:26:05 +0530
> naga suresh kumar <nagasureshkumarrelli@xxxxxxxxx> wrote:
>
> > Hi,
> >
> > Please see my reply inline by prefacing my name, currently i am facing
> > issues with my outlook. the reply looks not good but please don't mind.
>
> I do mind. Please find a way to fix that, 'cause I'm pretty sure others
> will soon complain about that too.
>
>
> > > > As i said, it needs tweaking the mtd->size and spi read/write addresses.
> > >
> > > Can you elaborate a bit on why you think this is needed? Did you look
> > > at [1]? Maybe it will prevent us from exposing the flash size at the
> > > spi-mem level.
> > > [naga]: some example snippet below
> > >
> > > spi_nor_read () {
> > >
> > >    if (nor->isparallel == 1)
> > >
> > >    {
> > >
> > >       offset /= 2; //byte stripping will happen here
> > >
> > >        nor->spi->master->flags |= SPI_DATA_STRIPE;
> > >
> > >        //By setting this flag even bits of data word located in lower
> > > memory and odd are in upper memory
> > >
> > >    }
> > >
> > >    if (nor->isstacked == 1) {
> > >
> > >      stack_shift = 1;
> > >
> > >      if (offset >= (mtd->size / 2)) {
> > >
> > >      offset = offset - (mtd->size / 2);
> > >
> > >      nor->spi->master->flags |= SPI_MASTER_U_PAGE;
> > >
> > >      //We can access Upper memory or lower memory, by setting this flag in
> > > controller.
> > >
> > >   } else {
> > >
> > >         nor->spi->master->flags &= ~SPI_MASTER_U_PAGE;
> > >
> > >   }
> > >
> > >  }
> > >
> > > same for spi_nor_write() also.
>
> Let's put the stacked and parallel support on the side for now and
> focus on single die/chip support. Will find a clean way to handle that
> afterwards.
>
> > >
> > > Also in both dual parallel and stacked mode mtd->size should be sum of
> > > both flash device sizes
>
> Yes, but this information should stay at the spi-nor/mtd level.
>
> > >
> > > also status register read will change, it needs status from both devices.
> > > like that some changes
> > >
> > > will be needed in core.
>
> Which is why this needs to be handled in spi-nor.c. The spi-nor core
> needs to know about the parallel/stack setup and handle it differently
> (read both status bytes, read both IDs, ...).
>
> > > > Can somebody share your thoughts on this(Adding Zynq QSPI Dual parallel
> > > and
> > > > stacked)?
> > >
> > > I always have a hard time with this naming. I guess stacked is when you
> > > have 2 chips sharing the same I/O bus, and parallel is when you have 2
> > > chips with one taking all of the I/O and the other taking the other
> > > half. Is that correct?
> > > [naga]: Yes, you are correct, i have attached the diagrams for these mode.
> > >
> > > In parallel mode, with the help of controller STRIPE feature, even bits of
> > >
> > > data words are located in lower memory and odd bits are located in upper
> > > memory.
> > >
> > > Data management will be taken care by controller in both modes
>
> Ok. I need to think about it a bit more. In the meantime, I recommend
> that you submit a version of the driver that does not support parallel
> and stacked modes.

As you suggested previously, i have added the support for Zynq QSPI
controller to support single chip.
Currently i am planning to add support for Parallel and Stacked for the same.
Could you please provide your implementation thoughts on this Parallel
and Stacked mode?

>From my side, i am thinking like below.
Will take two config entries CONFIG_SPI_XLNX_PARALLEL and
CONFIG_SPI_XLNX_STACKED
and
#ifdef CONFIG_SPI_XILINX_PARALLEL
   #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
   ------
  .sector_size = (_sector_size) << 1, \
  .n_sectors = (_n_sectors), \
  .page_size = (256 << 1), \
  .flags = (_flags | SPI_NOR_XLNX_PARALLEL),
#elif CONFIG_SPI_XILINX_STACKED
    #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
    -------
  .sector_size = (_sector_size), \
  .n_sectors = (_n_sectors << 1), \
  .page_size = 256, \
  .flags = (_flags | SPI_NOR_XLNX_STACKED),
#else
  #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
   ------
 .sector_size = (_sector_size), \
  .n_sectors = (_n_sectors), \
  .page_size = 256, \
 .flags = (_flags),
 #endif
>

And in the spi_nor_scan()
if (info->flags & (SPI_NOR_XLNX_PARALLEL || SPI_NOR_XLNX_STACKED)) {
        mtd->size = (params.size << 1);
        if (info->flags & SPI_NOR_XLNX_PARALLEL) {
            nor->spi->controller->flags |= (SPI_CONTROLLER_BOTH_CS |
SPI_CONTROLLER_DATA_STRIPE);
            nor->flags |= SNOR_F_XLNX_PARALLEL;
        } else if (info->flags & SPI_NOR_XLNX_STACKED) {
            nor->flags |= SNOR_F_XLNX_STACKED;
       }
}
Based on these flags, will alter the address to write/read, in
spi_nor_write() and spi_nor_read().
i will send an RFC patch with this update just to give show case this feature.

So could you please provide a way to implement the same?
i have also seen, that NXP flex controller is also supporting Parallel
flash mode, but not sure how
they are handling the chip select and mtd->size, page_size etc...
Sorry to bother you, but your suggestions helps a lot.

Thanks,
Naga Sureshkumar Relli

> Regards,
>
> Boris