Hi Jagan, > -----Original Message----- > From: Jagan Teki [mailto:jagannadh.t...@gmail.com] > Sent: Wednesday, May 09, 2018 4:18 PM > To: Siva Durga Prasad Paladugu <siva...@xilinx.com> > Cc: Jagan Teki <ja...@amarulasolutions.com>; U-Boot-Denx <u- > b...@lists.denx.de>; michal.si...@xilinx.com > Subject: Re: [U-Boot] [PATCH v3 1/2] spi: zynqmp_gqspi: Add support for > ZynqMP qspi driver > > On Wed, May 9, 2018 at 4:08 PM, Jagan Teki <jagannadh.t...@gmail.com> > wrote: > > On Wed, May 9, 2018 at 1:31 PM, Siva Durga Prasad Paladugu > > <siva...@xilinx.com> wrote: > >> Hi Jagan, > >> > >>> -----Original Message----- > >>> From: Jagan Teki [mailto:jagannadh.t...@gmail.com] > >>> Sent: Wednesday, May 09, 2018 1:22 PM > >>> To: Siva Durga Prasad Paladugu <siva...@xilinx.com> > >>> Cc: Jagan Teki <ja...@amarulasolutions.com>; U-Boot-Denx <u- > >>> b...@lists.denx.de>; michal.si...@xilinx.com > >>> Subject: Re: [U-Boot] [PATCH v3 1/2] spi: zynqmp_gqspi: Add support > >>> for ZynqMP qspi driver > >>> > >>> On Wed, May 9, 2018 at 1:20 PM, Siva Durga Prasad Paladugu > >>> <siva...@xilinx.com> wrote: > >>> > > >>> > Hi, > >>> > > >>> >> -----Original Message----- > >>> >> From: Jagan Teki [mailto:jagannadh.t...@gmail.com] > >>> >> Sent: Wednesday, May 09, 2018 1:12 PM > >>> >> To: Siva Durga Prasad Paladugu <siva...@xilinx.com> > >>> >> Cc: Jagan Teki <ja...@amarulasolutions.com>; U-Boot-Denx <u- > >>> >> b...@lists.denx.de>; michal.si...@xilinx.com > >>> >> Subject: Re: [U-Boot] [PATCH v3 1/2] spi: zynqmp_gqspi: Add > >>> >> support for ZynqMP qspi driver > >>> >> > >>> >> On Wed, May 9, 2018 at 12:33 PM, Siva Durga Prasad Paladugu > >>> >> <siva...@xilinx.com> wrote: > >>> >> > Hi Jagan, > >>> >> > > >>> >> >> -----Original Message----- > >>> >> >> From: Jagan Teki [mailto:ja...@amarulasolutions.com] > >>> >> >> Sent: Wednesday, May 09, 2018 12:01 PM > >>> >> >> To: Siva Durga Prasad Paladugu <siva...@xilinx.com> > >>> >> >> Cc: U-Boot-Denx <u-boot@lists.denx.de>; > >>> >> >> michal.si...@xilinx.com > >>> >> >> Subject: Re: [PATCH v3 1/2] spi: zynqmp_gqspi: Add support for > >>> >> ZynqMP > >>> >> >> qspi driver > >>> >> >> > >>> >> >> On Tue, May 8, 2018 at 3:43 PM, Siva Durga Prasad Paladugu > >>> >> >> <siva.durga.palad...@xilinx.com> wrote: > >>> >> >> > This patch adds qspi driver support for ZynqMP SoC. This > >>> >> >> > driver is responsible for communicating with qspi flash devices. > >>> >> >> > > >>> >> >> > Signed-off-by: Siva Durga Prasad Paladugu > >>> >> >> > <siva.durga.palad...@xilinx.com> > >>> >> >> > --- > >>> >> >> > Changes for v3: > >>> >> >> > - Renamed all macros, functions, files and configs as per > >>> >> >> > comment > >>> >> >> > - Used wait_for_bit wherever required > >>> >> >> > - Removed unnecessary header inclusion > >>> >> >> > > >>> >> >> > Changes for v2: > >>> >> >> > - Rebased on top of latest master > >>> >> >> > - Moved macro definitions to .h file as per comment > >>> >> >> > - Fixed magic values with macros as per comment > >>> >> >> > --- > >>> >> >> > arch/arm/include/asm/arch-zynqmp/zynqmp_gqspi.h | 154 > >>> ++++++ > >>> >> >> > drivers/spi/Kconfig | 7 + > >>> >> >> > drivers/spi/Makefile | 1 + > >>> >> >> > drivers/spi/zynqmp_gqspi.c | 670 > >>> >> >> ++++++++++++++++++++++++ > >>> >> >> > 4 files changed, 832 insertions(+) create mode 100644 > >>> >> >> > arch/arm/include/asm/arch- > >>> >> >> zynqmp/zynqmp_gqspi.h > >>> >> >> > create mode 100644 drivers/spi/zynqmp_gqspi.c > >>> >> >> > > >>> >> >> > diff --git a/arch/arm/include/asm/arch- > zynqmp/zynqmp_gqspi.h > >>> >> >> > b/arch/arm/include/asm/arch-zynqmp/zynqmp_gqspi.h > >>> >> >> > >>> >> >> already asked you to move this header code in driver .c file > >>> >> > > >>> >> > You might have missed my reply to your earlier comment on this. > >>> >> > These were moved to .h based on comment from Lukasz in v1. > >>> >> > I don’t have any issue in having them anywhere. Let me know > >>> >> > your > >>> choice. > >>> >> > >>> >> I'm trying to align Linux code, better to move like that and make > >>> >> sure to use similar macros. > >>> >> > >>> >> > > >>> > > >>> > [snip] > >>> > > >>> >> >> > +static void zynqmp_qspi_genfifo_cmd(struct > zynqmp_qspi_priv > >>> >> *priv) { > >>> >> >> > + u8 command = 1; > >>> >> >> > + u32 gen_fifo_cmd; > >>> >> >> > + u32 bytecount = 0; > >>> >> >> > + > >>> >> >> > + while (priv->len) { > >>> >> >> > + gen_fifo_cmd = zynqmp_qspi_bus_select(priv); > >>> >> >> > + gen_fifo_cmd |= GQSPI_GFIFO_TX; > >>> >> >> > + > >>> >> >> > + if (command) { > >>> >> >> > + command = 0; > >>> >> >> > + last_cmd = *(u8 *)priv->tx_buf; > >>> >> >> > + } > >>> >> >> > >>> >> >> don't understand this code can you explain? command assigned > 1 > >>> >> >> it will not updated anywhere? > >>> >> > > >>> >> > I want to store last command sent. As the first byte in loop > >>> >> > only contains command, it ensures it fills only for one time > >>> >> > and next time it > >>> >> may contain data to be sent along with command. > >>> >> > Command initialized to 1 while declaring it above(u8 command = > 1). > >>> >> > >>> >> Ok the first TX buf has command and reused in tx and rx fifo, how > >>> >> come to use use same in rx fifo? why is is last_cmd it is first_cmd? > >>> > > >>> > This holds the command that was sent last to the device before we > >>> > use in > >>> tx/rx fill, hence used this name. > >>> > >>> ie. what I wonder, usually the spi transfer start with cmd + data in > >>> that case it would be the first command, did gqspi work reverse way? > >> > >> It's also same as others :-), the last_cmd holds the recent command that > was sent to the device. > >> Its just name. To avoid confusion, I can use other names like > >> "cmd_sent or cmd_recent". Hope this is ok for you or suggest which one > would be appropriate. > > > > Let's park the naming aside. > > > > u8 command = 0; > > > > while (priv->len) { > > gen_fifo_cmd = zynqmp_qspi_bus_select(priv); > > gen_fifo_cmd |= GQSPI_GFIFO_TX; > > > > if (command) { > > command = 0; > > last_cmd = *(u8 *)priv->tx_buf; > > } > > ...... > > priv->len--; > > } > > > > Why the last_cmd assignment is in loop? say priv->len has non-zero the > > command is 1 last_cmd assigned and command reassigned to 0 there is > no > > way execute the if for next while isn't it? > > The main issue with this driver, which I told/commented several times > about using flash specific stuff. > > + > +#define QUAD_OUT_READ_CMD 0x6B > +#define QUAD_PAGE_PROGRAM_CMD 0x32 > +#define DUAL_OUTPUT_FASTRD_CMD 0x3B > +
Yeah I know, that’s where our discussion stopped last time and would like to conclude using this series. https://patchwork.ozlabs.org/patch/829856/ We discussed this sometime back and I mentioned that this controller is not just targeted for spi-nor but even though none tested/used with devices other than spi-nor AFAIK. Thanks, Siva > > Jagan. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
