[U-Boot] [PATCH v3 1/2] spi: zynqmp_gqspi: Add support for ZynqMP qspi driver
Jagan Teki
jagannadh.teki at gmail.com
Wed May 9 11:54:12 UTC 2018
On Wed, May 9, 2018 at 5:22 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 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.
Yes, please.
More information about the U-Boot
mailing list