[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:52:16 UTC 2018
Hi,
> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Wednesday, May 09, 2018 4:47 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:44 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 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.
>
> and you agree to write driver on spi side right? but you still using flash
> commands.
Sorry, That’s not true, my understanding was that we agreed to move to driver/mtd/spi in future inorder to have other
controller specific functionality like dual flash, but for now and for single its ok to be here.
Anyway, let me try to address your comment of not using any flash specific stuff here in driver
at drivers/spi/ and will send next version. Hope this works for you.
Thanks,
Siva
>
> #define QUAD_OUT_READ_CMD 0x6B
> #define QUAD_PAGE_PROGRAM_CMD 0x32
> #define DUAL_OUTPUT_FASTRD_CMD 0x3B
>
> Jagan.
More information about the U-Boot
mailing list