From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 03 May 2016 12:46:22 +0200 Subject: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code In-Reply-To: <20160503104250.GD6917@amd> References: <1461796606-9254-1-git-send-email-marex@denx.de> <20160503104250.GD6917@amd> Message-ID: <5728817E.7050507@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 05/03/2016 12:42 PM, Pavel Machek wrote: > Hi! > >> This patch replaces the whole unmaintainable indirect write implementation >> with the one from upcoming Linux CQSPI driver, which went through multiple >> rounds of thorough review and testing. While this makes the patch look >> terrifying and violates all best-practices of software development, >> all > > Could we get something less terifying and less violating? :-). > >> the patch does is it plucks out duplicate ad-hoc code distributed across >> the driver and replaces it with more compact code doing exactly the same >> thing. > > So this one is just a cleanup, and no behaviour change yet? Apparently Stefan discovered one behavior change, which I need to look into. Apparently, u-boot can pass unaligned buffer to the driver, which can cause problems. > Pavel > >> --- >> drivers/spi/cadence_qspi_apb.c | 122 +++++++++-------------------------------- >> 1 file changed, 26 insertions(+), 96 deletions(-) >> >> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c >> index 7786dd6..00a50cb 100644 >> --- a/drivers/spi/cadence_qspi_apb.c >> +++ b/drivers/spi/cadence_qspi_apb.c >> @@ -28,6 +28,7 @@ >> #include >> #include >> #include >> +#include >> #include "cadence_qspi.h" >> >> #define CQSPI_REG_POLL_US (1) /* 1us */ >> @@ -214,32 +215,6 @@ static void cadence_qspi_apb_read_fifo_data(void *dest, >> return; >> } >> >> -static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr, >> - const void *src, unsigned int bytes) >> -{ >> - unsigned int temp = 0; >> - int i; >> - int remaining = bytes; >> - unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr; >> - unsigned int *src_ptr = (unsigned int *)src; >> - >> - while (remaining >= CQSPI_FIFO_WIDTH) { >> - for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--) >> - writel(*(src_ptr+i), dest_ptr+i); >> - src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr); >> - remaining -= CQSPI_FIFO_WIDTH; >> - } >> - if (remaining) { >> - /* dangling bytes */ >> - i = remaining/sizeof(dest_ptr); >> - memcpy(&temp, src_ptr+i, remaining % sizeof(dest_ptr)); >> - writel(temp, dest_ptr+i); >> - for (--i; i >= 0; i--) >> - writel(*(src_ptr+i), dest_ptr+i); >> - } >> - return; >> -} >> - >> /* Read from SRAM FIFO with polling SRAM fill level. */ >> static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr, >> const void *src_addr, unsigned int num_bytes) >> @@ -276,44 +251,6 @@ static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr, >> return 0; >> } >> >> -/* Write to SRAM FIFO with polling SRAM fill level. */ >> -static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat, >> - const void *src_addr, unsigned int num_bytes) >> -{ >> - const void *reg_base = plat->regbase; >> - void *dest_addr = plat->ahbbase; >> - unsigned int retry = CQSPI_REG_RETRY; >> - unsigned int sram_level; >> - unsigned int wr_bytes; >> - unsigned char *src = (unsigned char *)src_addr; >> - int remaining = num_bytes; >> - unsigned int page_size = plat->page_size; >> - unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS; >> - >> - while (remaining > 0) { >> - retry = CQSPI_REG_RETRY; >> - while (retry--) { >> - sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base); >> - if (sram_level <= sram_threshold_words) >> - break; >> - } >> - if (!retry) { >> - printf("QSPI: SRAM fill level (0x%08x) not hit lower expected level (0x%08x)", >> - sram_level, sram_threshold_words); >> - return -1; >> - } >> - /* Write a page or remaining bytes. */ >> - wr_bytes = (remaining > page_size) ? >> - page_size : remaining; >> - >> - cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes); >> - src += wr_bytes; >> - remaining -= wr_bytes; >> - } >> - >> - return 0; >> -} >> - >> void cadence_qspi_apb_controller_enable(void *reg_base) >> { >> unsigned int reg; >> @@ -810,48 +747,41 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat, >> } >> >> int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat, >> - unsigned int txlen, const u8 *txbuf) >> + unsigned int n_tx, const u8 *txbuf) >> { >> - unsigned int reg = 0; >> - unsigned int retry; >> + unsigned int page_size = plat->page_size; >> + unsigned int remaining = n_tx; >> + unsigned int write_bytes; >> + int ret; >> >> /* Configure the indirect read transfer bytes */ >> - writel(txlen, plat->regbase + CQSPI_REG_INDIRECTWRBYTES); >> + writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES); >> >> /* Start the indirect write transfer */ >> writel(CQSPI_REG_INDIRECTWR_START_MASK, >> plat->regbase + CQSPI_REG_INDIRECTWR); >> >> - if (qpsi_write_sram_fifo_push(plat, (const void *)txbuf, txlen)) >> - goto failwr; >> - >> - /* Wait until last write is completed (FIFO empty) */ >> - retry = CQSPI_REG_RETRY; >> - while (retry--) { >> - reg = CQSPI_GET_WR_SRAM_LEVEL(plat->regbase); >> - if (reg == 0) >> - break; >> - >> - udelay(1); >> - } >> - >> - if (reg != 0) { >> - printf("QSPI: timeout for indirect write\n"); >> - goto failwr; >> - } >> + while (remaining > 0) { >> + write_bytes = remaining > page_size ? page_size : remaining; >> + writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4)); >> + >> + ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL, >> + CQSPI_REG_SDRAMLEVEL_WR_MASK << >> + CQSPI_REG_SDRAMLEVEL_WR_LSB, 0, 10, 0); >> + if (ret) { >> + printf("Indirect write timed out (%i)\n", ret); >> + goto failwr; >> + } >> >> - /* Check flash indirect controller status */ >> - retry = CQSPI_REG_RETRY; >> - while (retry--) { >> - reg = readl(plat->regbase + CQSPI_REG_INDIRECTWR); >> - if (reg & CQSPI_REG_INDIRECTWR_DONE_MASK) >> - break; >> - udelay(1); >> + txbuf += write_bytes; >> + remaining -= write_bytes; >> } >> >> - if (!(reg & CQSPI_REG_INDIRECTWR_DONE_MASK)) { >> - printf("QSPI: indirect completion status error with reg 0x%08x\n", >> - reg); >> + /* Check indirect done status */ >> + ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTWR, >> + CQSPI_REG_INDIRECTWR_DONE_MASK, 1, 10, 0); >> + if (ret) { >> + printf("Indirect write completion error (%i)\n", ret); >> goto failwr; >> } >> >> @@ -864,7 +794,7 @@ failwr: >> /* Cancel the indirect write */ >> writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK, >> plat->regbase + CQSPI_REG_INDIRECTWR); >> - return -1; >> + return ret; >> } >> >> void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy) > -- Best regards, Marek Vasut