[U-Boot] [PATCH v3 1/2] spi: zynqmp_gqspi: Add support for ZynqMP qspi driver
Siva Durga Prasad Paladugu
sivadur at xilinx.com
Wed May 9 11:14:06 UTC 2018
Hi Jagan,
> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Wednesday, May 09, 2018 4:18 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 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
> +
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.
More information about the U-Boot
mailing list