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

[Jagan Teki]

On Wed, May 9, 2018 at 4:08 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> 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?

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
+

Jagan.