Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754993AbbHXQmz (ORCPT ); Mon, 24 Aug 2015 12:42:55 -0400 Received: from eusmtp01.atmel.com ([212.144.249.242]:54897 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753993AbbHXQmx (ORCPT ); Mon, 24 Aug 2015 12:42:53 -0400 Message-ID: <55DB4986.1000207@atmel.com> Date: Mon, 24 Aug 2015 18:42:46 +0200 From: Cyrille Pitchen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Marek Vasut CC: , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles References: <201508241248.17466.marex@denx.de> In-Reply-To: <201508241248.17466.marex@denx.de> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.161.30.18] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5715 Lines: 193 Hi Marek, Le 24/08/2015 12:48, Marek Vasut a ?crit : > On Monday, August 24, 2015 at 12:13:58 PM, Cyrille Pitchen wrote: >> The number of dummy cycles used during Fast Read commands can be reduced >> to improve transfer performances. Each manufacturer has a dedicated set of >> registers to provide the memory with the exact number of dummy cycles it >> should expect. Both the memory and the (Q)SPI controller must agree on >> this number of dummy cycles. >> >> The number of dummy cycles can be found into the memory datasheet and >> mostly depends on the SPI clock frequency, the Fast Read op code and the >> Single/Dual Data Rate mode. >> >> Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would >> only provide the driver with a high enough number of dummy cycles for each >> Fast Read command to be used for all clock frequencies: this solution >> would not be optimized. >> >> Signed-off-by: Cyrille Pitchen > > Hi! > >> drivers/mtd/spi-nor/spi-nor.c | 97 >> ++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h >> | 2 + >> 2 files changed, 80 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index e2a6029dc056..869e098a6841 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor) >> } >> >> /* >> - * Dummy Cycle calculation for different type of read. >> - * It can be used to support more commands with >> - * different dummy cycle requirements. >> - */ >> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) >> -{ >> - switch (nor->flash_read) { >> - case SPI_NOR_FAST: >> - case SPI_NOR_DUAL: >> - case SPI_NOR_QUAD: >> - return 8; >> - case SPI_NOR_NORMAL: >> - return 0; >> - } >> - return 0; >> -} > > You can probably just soup up this function so that it sets the > nor->read_dummy, no ? > Actually, this is what the patch does: spi_nor_read_dummy_cycles() was reused and enhanced few lines below where you've pointed out the "switch (nor->flash_read)" block should be move after the else block. I think when I wrote the code I've chosen to move the definition of this function instead of adding forward declarations of functions such as read_cr() or write_sr_cr(), which are now called by micron_set_dummy_cycles(). >> -/* >> * Write status register 1 byte >> * Returns negative if error occurred. >> */ >> @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct >> flash_info *info) } >> } >> >> +static int micron_set_dummy_cycles(struct spi_nor *nor) >> +{ >> + int ret; >> + u8 val, mask; >> + >> + /* read the Volatile Configuration Register (VCR) */ > > NIT: If this is a sentence, start it with capital letter and end it with > fullstop :) > done for the next version >> + ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1); >> + if (ret < 0) { >> + dev_err(nor->dev, "error %d reading VCR\n", ret); >> + return ret; >> + } >> + >> + write_enable(nor); >> + >> + /* update the number of dummy into the VCR */ > > DTTO > done for the next version >> + mask = GENMASK(7, 4); >> + val &= ~mask; >> + val |= (nor->read_dummy << 4) & mask; >> + ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0); >> + if (ret < 0) { >> + dev_err(nor->dev, "error while writing VCR register\n"); >> + return ret; >> + } >> + >> + ret = spi_nor_wait_till_ready(nor); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +/* >> + * Dummy Cycle calculation for different type of read. >> + * It can be used to support more commands with >> + * different dummy cycle requirements. >> + */ >> +static int spi_nor_read_dummy_cycles(struct spi_nor *nor, >> + const struct flash_info *info) >> +{ >> + struct device_node *np = nor->dev->of_node; >> + u32 num_dummy_cycles; >> + >> + if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles", >> + &num_dummy_cycles)) { >> + nor->read_dummy = num_dummy_cycles; >> + >> + /* >> + * This switch block might be moved after the if...then...else >> + * statement but it was not tested with all Spansion or Micron >> + * memories. >> + * Now the "m25p,num-dummy-cycles" property needs to be >> + * explicitly set in the device tree so the switch statement is >> + * executed. This should avoid unwanted side effects and keep >> + * backward compatibility. >> + */ >> + switch (JEDEC_MFR(info)) { >> + case CFI_MFR_ST: >> + return micron_set_dummy_cycles(nor); >> + default: > > If you do have m25p,num-dummy-cycles set for non-micron flash, you have a > problem here I believe. > >> + break; >> + } >> + } else { > > The solution would be to drop this else {} bit here, so that if you fail in > the DT-based configuration, you fall back to this old behavior. What do you > think please ? :) > Good idea! I also add a trace for the default case of "switch (JEDEC_MFR(info))": dev_warn(dev, "can't set the number of dummy cycles\n"); So the user is notified that the driver could not use the value of "m25p,num-dummy-cycles" from the DT before falling back to the legacy code. >> + switch (nor->flash_read) { >> + case SPI_NOR_FAST: >> + case SPI_NOR_DUAL: >> + case SPI_NOR_QUAD: >> + nor->read_dummy = 8; >> + case SPI_NOR_NORMAL: >> + nor->read_dummy = 0; >> + } >> + } >> + >> + return 0; >> +} > > [...] > thanks for the review! Best regards, Cyrille -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/