Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Pratyush Yadav <me@yadavpratyush.com>
To: Tudor.Ambarus@microchip.com
Cc: alexandre.belloni@bootlin.com, vigneshr@ti.com, richard@nod.at,
	nsekhar@ti.com, Nicolas.Ferre@microchip.com,
	boris.brezillon@collabora.com, michal.simek@xilinx.com,
	Ludovic.Desroches@microchip.com, broonie@kernel.org,
	linux-mtd@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, miquel.raynal@bootlin.com,
	matthias.bgg@gmail.com, linux-mediatek@lists.infradead.org,
	linux-spi@vger.kernel.org, p.yadav@ti.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table
Date: Mon, 20 Jul 2020 22:08:02 +0530
Message-ID: <20200720163802.veql43cmal7sunit@yadavpratyush.com> (raw)
In-Reply-To: <1450d8c8-cda4-51e3-9f57-0b2f00825f11@microchip.com>

Hi Tudor,

On 08/07/20 04:01PM, Tudor.Ambarus@microchip.com wrote:
> On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > This table is indication that the flash is xSPI compliant and hence
> > supports octal DTR mode. Extract information like the fast read opcode,
> > dummy cycles, the number of dummy cycles needed for a Read Status
> > Register command, and the number of address bytes needed for a Read
> > Status Register command.
> > 
> > We don't know what speed the controller is running at. Find the fast
> > read dummy cycles for the fastest frequency the flash can run at to be
> > sure we are never short of dummy cycles. If nothing is available,
> > default to 20. Flashes that use a different value should update it in
> > their fixup hooks.
> > 
> > Since we want to set read settings, expose spi_nor_set_read_settings()
> > in core.h.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/mtd/spi-nor/core.c |  2 +-
> >  drivers/mtd/spi-nor/core.h | 10 ++++
> >  drivers/mtd/spi-nor/sfdp.c | 98 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 109 insertions(+), 1 deletion(-)
> > 
[...]
> > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> > index 3f709de5ea67..d5a24e61813c 100644
> > --- a/drivers/mtd/spi-nor/sfdp.c
> > +++ b/drivers/mtd/spi-nor/sfdp.c
[...]
> > @@ -66,6 +70,16 @@ struct sfdp_bfpt_erase {
> >         u32                     shift;
> >  };
> > 
> > +/* xSPI Profile 1.0 table (from JESD216D.01). */
> > +#define PROFILE1_DWORD1_RD_FAST_CMD            GENMASK(15, 8)
> > +#define PROFILE1_DWORD1_RDSR_DUMMY             BIT(28)
> > +#define PROFILE1_DWORD1_RDSR_ADDR_BYTES                BIT(29)
> > +#define PROFILE1_DWORD4_DUMMY_200MHZ           GENMASK(11, 7)
> > +#define PROFILE1_DWORD5_DUMMY_166MHZ           GENMASK(31, 27)
> > +#define PROFILE1_DWORD5_DUMMY_133MHZ           GENMASK(21, 17)
> > +#define PROFILE1_DWORD5_DUMMY_100MHZ           GENMASK(11, 7)
> 
> we should order these macros in a consistent way. I see that previous macros
> are declared in order starting from MSB to LSB.
> 
> > +#define PROFILE1_DUMMY_DEFAULT                 20
> 
> we need to explain why the default dummy value is 20.

No reason other than the fact that it is the default for the first flash 
that uses Profile 1.0 parsing (S28HS512T). AFAIK a similar reasoning is 
followed for the default being 8 for 1-1-4 or 1-1-8 modes.

I can't think of any reasonable way of deciding on a default value since 
it varies from flash to flash.
 
> How about declaring all these macros immediately above of spi_nor_parse_profile1()?
> 
> > +
> >  #define SMPT_CMD_ADDRESS_LEN_MASK              GENMASK(23, 22)
> >  #define SMPT_CMD_ADDRESS_LEN_0                 (0x0UL << 22)
> >  #define SMPT_CMD_ADDRESS_LEN_3                 (0x1UL << 22)
> > @@ -1106,6 +1120,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
> >         return ret;
> >  }
> > 
> > +/**
> > + * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table
> > + * @nor:               pointer to a 'struct spi_nor'
> > + * @param_header:      pointer to the 'struct sfdp_parameter_header' describing
> > + *                     the 4-Byte Address Instruction Table length and version.
> > + * @params:            pointer to the 'struct spi_nor_flash_parameter' to be.
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int spi_nor_parse_profile1(struct spi_nor *nor,
> > +                                 const struct sfdp_parameter_header *profile1_header,
> > +                                 struct spi_nor_flash_parameter *params)
> > +{
> > +       u32 *table, opcode, addr;
> 
> s/table/dwords?
> 
> u8 opcode?
> 
> > +       size_t len;
> > +       int ret, i;
> > +       u8 dummy;
> > +
> > +       len = profile1_header->length * sizeof(*table);
> > +       table = kmalloc(len, GFP_KERNEL);
> > +       if (!table)
> > +               return -ENOMEM;
> > +
> > +       addr = SFDP_PARAM_HEADER_PTP(profile1_header);
> > +       ret = spi_nor_read_sfdp(nor, addr, len, table);
> > +       if (ret)
> > +               goto out;
> > +
> > +       /* Fix endianness of the table DWORDs. */
> > +       for (i = 0; i < profile1_header->length; i++)
> > +               table[i] = le32_to_cpu(table[i]);
> 
> le32_to_cpu_array(table, profile1_header->length);
> 
> > +
> > +       /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> > +       opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> > +
> > +       /*
> > +        * We don't know what speed the controller is running at. Find the
> > +        * dummy cycles for the fastest frequency the flash can run at to be
> > +        * sure we are never short of dummy cycles. A value of 0 means the
> > +        * frequency is not supported.
> > +        *
> > +        * Default to PROFILE1_DUMMY_DEFAULT if we don't find anything, and let
> > +        * flashes set the correct value if needed in their fixup hooks.
> > +        */
> > +       dummy = FIELD_GET(PROFILE1_DWORD4_DUMMY_200MHZ, table[3]);
> > +       if (!dummy)
> > +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, table[4]);
> > +       if (!dummy)
> > +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_133MHZ, table[4]);
> > +       if (!dummy)
> > +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_100MHZ, table[4]);
> > +       if (!dummy)
> > +               dummy = PROFILE1_DUMMY_DEFAULT;
> > +
> > +       /* Round up to an even value to avoid tripping controllers up. */
> > +       dummy = ROUND_UP_TO(dummy, 2);
> > +
[...]

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
2020-07-08 16:22   ` Tudor.Ambarus
2020-07-13  3:55   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension Pratyush Yadav
2020-07-13  6:15   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops Pratyush Yadav
2020-07-13  6:19   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 04/17] spi: spi-mtk-nor: " Pratyush Yadav
2020-07-13  6:24   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol Pratyush Yadav
2020-07-07 17:37   ` Tudor.Ambarus
2020-07-21 11:29     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT Pratyush Yadav
2020-07-07 17:53   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table Pratyush Yadav
2020-07-08 16:01   ` Tudor.Ambarus
2020-07-20 16:38     ` Pratyush Yadav [this message]
2020-06-23 18:30 ` [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP Pratyush Yadav
2020-07-08 16:03   ` Tudor.Ambarus
2020-07-20 16:24     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 09/17] mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 10/17] mtd: spi-nor: core: enable octal DTR mode when possible Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal Pratyush Yadav
2020-07-13  9:33   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT Pratyush Yadav
2020-07-08 16:08   ` Tudor.Ambarus
2020-07-20 16:21     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown Pratyush Yadav
2020-07-08 16:10   ` Tudor.Ambarus
2020-07-20 16:11     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 14/17] mtd: spi-nor: core: disable Octal DTR mode on suspend Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 15/17] mtd: spi-nor: core: expose spi_nor_default_setup() in core.h Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 16/17] mtd: spi-nor: spansion: add support for Cypress Semper flash Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 17/17] mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode Pratyush Yadav
2020-07-13  6:34 ` [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Tudor.Ambarus
2020-07-14 19:19   ` Mark Brown
2020-07-15  3:40     ` Tudor.Ambarus
2020-07-14 16:40 ` 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=20200720163802.veql43cmal7sunit@yadavpratyush.com \
    --to=me@yadavpratyush.com \
    --cc=Ludovic.Desroches@microchip.com \
    --cc=Nicolas.Ferre@microchip.com \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=boris.brezillon@collabora.com \
    --cc=broonie@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=matthias.bgg@gmail.com \
    --cc=michal.simek@xilinx.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=nsekhar@ti.com \
    --cc=p.yadav@ti.com \
    --cc=richard@nod.at \
    --cc=vigneshr@ti.com \
    /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-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/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-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


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