Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Pratyush Yadav <p.yadav@ti.com>
Cc: zhengxunli <zhengxunli@mxic.com.tw>, <vigneshr@ti.com>,
	<juliensu@mxic.com.tw>, <ycllin@mxic.com.tw>,
	<linux-spi@vger.kernel.org>, <broonie@kernel.org>,
	<linux-mtd@lists.infradead.org>
Subject: Re: [PATCH v2 1/2] mtd: spi-nor: macronix: add support for Macronix octaflash
Date: Tue, 23 Feb 2021 14:13:44 +0100
Message-ID: <20210223141344.7ad25831@xps13> (raw)
In-Reply-To: <20210205133404.esqqeokhlp4askpq@ti.com>

Hi Pratyush,

Pratyush Yadav <p.yadav@ti.com> wrote on Fri, 5 Feb 2021 19:04:04 +0530:

> On 05/02/21 10:47AM, Miquel Raynal wrote:
> > Hello,
> > 
> > zhengxunli <zhengxunli@mxic.com.tw> wrote on Fri,  5 Feb 2021 17:36:47
> > +0800:
> >   
> > > The ocatflash is an xSPI compliant octal DTR flash. Add support
> > > for using it in octal DTR mode.
> > > 
> > > Enable Octal DTR mode with 20 dummy cycles to allow running at the
> > > maximum supported frequency of 200Mhz.
> > > 
> > > Try to verify the flash ID to check whether the flash memory in octal
> > > DTR mode is correct. When reading ID in OCTAL DTR mode, ID will appear
> > > in a repeated manner. ex: ID[0] = 0xc2, ID[1] = 0xc2, ID[2] = 0x94,
> > > ID[3] = 0x94... Rearrange the order so that the ID can pass.
> > > 
> > > Signed-off-by: zhengxunli <zhengxunli@mxic.com.tw>
> > > ---
> > >  drivers/mtd/spi-nor/macronix.c | 121 +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 121 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> > > index 9203aba..7498978 100644
> > > --- a/drivers/mtd/spi-nor/macronix.c
> > > +++ b/drivers/mtd/spi-nor/macronix.c
> > > @@ -8,6 +8,16 @@
> > >  
> > >  #include "core.h"
> > >  
> > > +#define SPINOR_OP_RD_CR2		0x71		/* Read configuration register 2 */
> > > +#define SPINOR_OP_WR_CR2		0x72		/* Write configuration register 2 */
> > > +#define SPINOR_OP_MXIC_DTR_RD		0xee		/* Fast Read opcode in DTR mode */
> > > +#define SPINOR_REG_MXIC_CR2_MODE	0x00000000	/* For setting octal DTR mode */
> > > +#define SPINOR_REG_MXIC_OPI_DTR_EN	0x2		/* Enable Octal DTR */
> > > +#define SPINOR_REG_MXIC_OPI_DTR_DIS	0x1		/* Disable Octal DTR */
> > > +#define SPINOR_REG_MXIC_CR2_DC		0x00000300	/* For setting dummy cycles */
> > > +#define SPINOR_REG_MXIC_DC_20		0x0		/* Setting dummy cycles to 20 */
> > > +#define MXIC_MAX_DC			20		/* Maximum value of dummy cycles */
> > > +
> > >  static int
> > >  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
> > >  			    const struct sfdp_parameter_header *bfpt_header,
> > > @@ -33,6 +43,113 @@
> > >  	.post_bfpt = mx25l25635_post_bfpt_fixups,
> > >  };
> > >  
> > > +/**
> > > + * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix flashes.
> > > + * @nor:		pointer to a 'struct spi_nor'
> > > + * @enable:		whether to enable or disable Octal DTR
> > > + *
> > > + * This also sets the memory access dummy cycles to 20 to allow the flash to
> > > + * run at up to 200MHz.
> > > + *
> > > + * Return: 0 on success, -errno otherwise.
> > > + */
> > > +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool enable)
> > > +{
> > > +	struct spi_mem_op op;
> > > +	u8 *buf = nor->bouncebuf, i;
> > > +	int ret;
> > > +
> > > +	if (enable) {
> > > +		/* Use 20 dummy cycles for memory array reads. */
> > > +		ret = spi_nor_write_enable(nor);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		*buf = SPINOR_REG_MXIC_DC_20;
> > > +		op = (struct spi_mem_op)
> > > +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
> > > +				   SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_DC, 1),
> > > +				   SPI_MEM_OP_NO_DUMMY,
> > > +				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
> > > +
> > > +		ret = spi_mem_exec_op(nor->spimem, &op);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = spi_nor_wait_till_ready(nor);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		nor->read_dummy = MXIC_MAX_DC;  
> > 
> > I am still not convinced by this constant value.  
> 
> I think a constant value is fine. This dummy cycle value reflects how 
> many cycles the master clock would go through before the flash starts 
> emitting the data. If the master (aka the controller) is running at a 
> lower frequency then those cycles go through slower, but the flash still 
> waits for them to finish before emitting data. And since the master is 
> driving the clock and the flash is just "reading" it, both remain in 
> sync.
> 
> The dummy cycles need to be set for the worst case scenario [0]. The 
> flash usually needs a minimum amount of time before it is ready to emit 
> the data. So for example if the master is at 25 MHz, the clock period is 
> longer so 8 clock cycles [1] might be long enough to exceed that minimum 
> time. But when the master is running at 200 MHz, the clock period is 
> smaller so 8 cycles might not give the flash enough time to prepare. So 
> we need to to wait at least 20 cycles [1] before emitting data.
> 
> This is what my patches do for the Cypress S28 flash. I have tested it 
> on both 25 MHz and 166 MHz with 22 dummy cycles. It is not the most 
> efficient at 25 MHz since 5 dummy cycles is all that is needed for that 
> speed, but its the best we can do right now.
> 
> [0] Since SPI NOR has no way of knowing what speed the controller is 
> running at, assume the fastest speed the flash can run at.

Ok, I am not entirely clear about what is available/not available from
the SPI core.

If this is true then I guess we can't do better with the current code
base and this can be improved in the future if needed. So I'm fine with
the current implementation.

> [1] Hypothetical example. Don't know the actual values for this flash.
>  
> > The rest looks good to me.  
> 

Thanks,
Miquèl

  parent reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-05  9:36 [PATCH v2 0/2] Add octal DTR support for Macronix flash zhengxunli
2021-02-05  9:36 ` [PATCH v2 1/2] mtd: spi-nor: macronix: add support for Macronix octaflash zhengxunli
2021-02-05  9:47   ` Miquel Raynal
2021-02-05 13:34     ` Pratyush Yadav
2021-02-05 13:50       ` Miquel Raynal
2021-02-23 13:13       ` Miquel Raynal [this message]
2021-02-23 13:36         ` Mark Brown
2021-02-23 15:25           ` Pratyush Yadav
2021-02-23 18:07             ` Geert Uytterhoeven
2021-02-23 18:14               ` Mark Brown
     [not found]                 ` <OF5890F10B.27BC3B4B-ON482586A7.001FE05B-482586A7.0024305D@mxic.com.tw>
2021-04-05  4:41                   ` Tudor.Ambarus
2021-02-05  9:36 ` [PATCH v2 2/2] spi: mxic: patch for octal DTR mode support zhengxunli
2021-02-19  5:58   ` zhengxunli
2021-02-23 13:28     ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210223141344.7ad25831@xps13 \
    --to=miquel.raynal@bootlin.com \
    --cc=broonie@kernel.org \
    --cc=juliensu@mxic.com.tw \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=p.yadav@ti.com \
    --cc=vigneshr@ti.com \
    --cc=ycllin@mxic.com.tw \
    --cc=zhengxunli@mxic.com.tw \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-SPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-spi/0 linux-spi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-spi linux-spi/ https://lore.kernel.org/linux-spi \
		linux-spi@vger.kernel.org
	public-inbox-index linux-spi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-spi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git