[U-Boot] [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes

[Vikas MANOCHA]

Hi Stefan,

> -----Original Message-----
> From: Vikas MANOCHA
> Sent: Monday, June 22, 2015 4:31 PM
> To: 'Stefan Roese'
> Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com;
> dinguyen at opensource.altera.com; jteki at openedev.com
> Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect
> rd-writes
> 
> Thanks Stefan,
> 
> > -----Original Message-----
> > From: Stefan Roese [mailto:sr at denx.de]
> > Sent: Monday, June 22, 2015 1:35 AM
> > To: Vikas MANOCHA
> > Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com;
> > dinguyen at opensource.altera.com; jteki at openedev.com
> > Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix
> > indirect rd-writes
> >
> > Hi Vikas,
> >
> > On 19.06.2015 23:38, Vikas MANOCHA wrote:
> > >>> - git bisect or cherry-pick to find out which patch is breaking the
> > >>>    read functionality.
> > >>
> > >> This one is the first introducing this breakage:
> > >>
> > >> spi: cadence_qspi: fix base trigger address & transfer start
> > >> address
> > >>
> > >
> > > Ok, can you confirm applying patches upto (& including)
> > > "spi: cadence_qspi: fix indirect read/write start address" , read/write
> > >   to flash are working fine.
> >
> > Please note that with this patch the code does not compile:
> >
> > drivers/spi/cadence_qspi_apb.c: In function 'qpsi_write_sram_fifo_push':
> > drivers/spi/cadence_qspi_apb.c:227:32: error: 'struct
> cadence_spi_platdata'
> > has no member named 'trigger_base'
> >    unsigned int *dest_addr = plat->trigger_base;
> >
> > I've manually fixed this trivial change (trigger_base -> ahbbase) and
> > tests with these patches applied show some problems with "sf"
> > stability
> > (bit-flips):
> 
> Sorry for this dumb mistake which happened during rebase :-(,  I will correct
> in next patch series.
> 
> >
> > => sf update 400000 100000 100000
> > 1048576 bytes written, 0 bytes skipped in 34.196s, speed 31395 B/s =>
> > sf read
> > 500000 100000 100000
> > SF: 1048576 bytes @ 0x100000 Read: OK
> > => cmp.b 400000 500000 100000
> > byte at 0x0040001e (0x9f) != byte at 0x0050001e (0x8f) Total of 30
> > byte(s) were the same
> >
> > This is new - removing all your patches seems to solve this issue again.
> 
> Thanks again Stefan for these tests. It is not easy for me to figure out this
> instability without the board but I don 't see any logical error in the patches.
> Off-course code is more optimized & fast with these patches. Access timing
> in the log provided by you also confirms it.
> 
> I can suggest following checks on the hardware:
> 
> - Figure out the issue is with read or write, which means comparing the flash
> with ram after read/write.
> - Put some random microsecond delays in the read/write to confirm it is
> timing issue.

Any update on it ?

Rgds,
Vikas

> 
> > So there seems to be something wrong with the previous patches as well.
> > here the output with the patches reverted:
> >
> > => sf probe
> > SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total
> > 32 MiB
> > SF: Warning - Only lower 16MiB accessible, Full access #define
> > CONFIG_SPI_FLASH_BAR => sf update 400000 100000 100000
> > 1048576 bytes written, 0 bytes skipped in 35.872s, speed 29929 B/s =>
> > sf read
> > 500000 100000 100000
> > SF: 1048576 bytes @ 0x100000 Read: OK
> > => cmp.b 400000 500000 100000
> > Total of 1048576 byte(s) were the same
> >
> > > The point is if after applying above mentioned patch (...: fix
> > > indirect read/write start address), Read/write are working fine,
> > > then trigger_base value of 0xFFA00_0000 should also work fine.
> > > Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000
> > > in Socfpga.dtsi & check.
> > > If it works, it would mean both (socfpga & stv0991) are behaving same.
> >
> > No. With this change, the "sf read" command crashes / hangs on the
> > SoCFPGA board.
> 
> Ok, I don't know why socfpga is not working with trigger_base to be
> 0xFFA0_0000.
> Normally it should work, Graham also thinks the same, Let's wait for his
> discussion with the Altera designers.
> 
> Rgds,
> Vikas
> 
> >
> > Thanks,
> > Stefan