[U-Boot] [PATCH v3 1/2] spi: zynqmp_gqspi: Add support for ZynqMP qspi driver

[Siva Durga Prasad Paladugu]

Hi Jagan,

> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Wednesday, May 09, 2018 1:22 PM
> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
> Cc: Jagan Teki <jagan at amarulasolutions.com>; U-Boot-Denx <u-
> boot at lists.denx.de>; michal.simek at 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
> <sivadur at xilinx.com> wrote:
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> >> Sent: Wednesday, May 09, 2018 1:12 PM
> >> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
> >> Cc: Jagan Teki <jagan at amarulasolutions.com>; U-Boot-Denx <u-
> >> boot at lists.denx.de>; michal.simek at 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
> >> <sivadur at xilinx.com> wrote:
> >> > Hi Jagan,
> >> >
> >> >> -----Original Message-----
> >> >> From: Jagan Teki [mailto:jagan at amarulasolutions.com]
> >> >> Sent: Wednesday, May 09, 2018 12:01 PM
> >> >> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
> >> >> Cc: U-Boot-Denx <u-boot at lists.denx.de>; michal.simek at 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.paladugu at 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.paladugu at 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.


Thanks,
Siva

> 
> Jagan.