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

[Jagan Teki]

On Wed, May 9, 2018 at 1:31 PM, Siva Durga Prasad Paladugu
<sivadur at xilinx.com> wrote:
> 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.

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?

Jagan.