diff mbox series

[v5,09/19] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table

Message ID 20200519142642.24131-10-p.yadav@ti.com
State New
Headers show
Series mtd: spi-nor: add xSPI Octal DTR support | expand

Commit Message

Pratyush Yadav May 19, 2020, 2:26 p.m. UTC
This table is indication that the flash is xSPI compliant and hence
supports octal DTR mode. Extract information like the fast read opcode,
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.

The default dummy cycles for a fast octal DTR read are set to 20. Since
there is no simple way of determining the dummy cycles needed for the
fast read command, flashes that use a different value should update it
in their flash-specific 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 | 73 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 1 deletion(-)

Comments

Mason Yang May 20, 2020, 7:59 a.m. UTC | #1
Hi Pratyush, 

> +/**
> + * 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;
> +   size_t len;
> +   int ret, i;
> +
> +   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]);
> +
> +   /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> +   opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> +
> +   /*
> +    * Update the fast read settings. We set the default dummy cycles to 
20
> +    * here. Flashes can change this value if they need to when enabling
> +    * octal mode.
> +    */
> +   spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> +              0, 20, opcode,
> +              SNOR_PROTO_8_8_8_DTR);
> +


I thought we have a agreement that only do parse here, no other read 
parameters setting.

Driver should get dummy cycles used for various frequencies 
from 4th and 5th DWORD of xSPI table.[1]
 
[1] 
https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-3-git-send-email-masonccyang@mxic.com.tw/ 


In addition, 20 dummy cycles is for 200MHz but not for 100MHz, 133MHz and 
166MHz
in case of read performance concern.

Given a correct dummy cycles for a specific device. [2] 

[2] 
https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-5-git-send-email-masonccyang@mxic.com.tw/ 



> +   /*
> +    * Set the Read Status Register dummy cycles and dummy address 
bytes.
> +    */
> +   if (table[0] & PROFILE1_DWORD1_RDSR_DUMMY)
> +      params->rdsr_dummy = 8;
> +   else
> +      params->rdsr_dummy = 4;
> +
> +   if (table[0] & PROFILE1_DWORD1_RDSR_ADDR_BYTES)
> +      params->rdsr_addr_nbytes = 4;
> +   else
> +      params->rdsr_addr_nbytes = 0;
> +
> +out:
> +   kfree(table);
> +   return ret;
> +}
> +

thanks & best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Pratyush Yadav May 20, 2020, 8:55 a.m. UTC | #2
Hi Mason,

On 20/05/20 03:59PM, masonccyang@mxic.com.tw wrote:
> 
> Hi Pratyush, 
> 
> > +/**
> > + * 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;
> > +   size_t len;
> > +   int ret, i;
> > +
> > +   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]);
> > +
> > +   /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> > +   opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> > +
> > +   /*
> > +    * Update the fast read settings. We set the default dummy cycles to 
> 20
> > +    * here. Flashes can change this value if they need to when enabling
> > +    * octal mode.
> > +    */
> > +   spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > +              0, 20, opcode,
> > +              SNOR_PROTO_8_8_8_DTR);
> > +
> 
> 
> I thought we have a agreement that only do parse here, no other read 
> parameters setting.

Yes, and I considered it. But it didn't make much sense to me to 
introduce an extra member in struct spi_nor just to make this call in 
some other function later.

Why exactly do you think doing this here is bad? The way I see it, we 
avoid carrying around an extra member in spi_nor and this also allows 
flashes to change the read settings easily in a post-sfdp hook. The 
4bait parsing function does something similar.

What are the benefits of doing it otherwise?

Note that I did remove HWCAPS selection from here, which did seem like a 
sane idea.
 
> Driver should get dummy cycles used for various frequencies 
> from 4th and 5th DWORD of xSPI table.[1]
>  
> [1] 
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-3-git-send-email-masonccyang@mxic.com.tw/ 
> 
> 
> In addition, 20 dummy cycles is for 200MHz but not for 100MHz, 133MHz and 
> 166MHz
> in case of read performance concern.
> 
> Given a correct dummy cycles for a specific device. [2] 
> 
> [2] 
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-5-git-send-email-masonccyang@mxic.com.tw/ 

The problem is that we don't know what speed the controller is driving 
the flash at, and whether it is using Data Strobe. BFPT tells us the 
maximum speed of the flash based on if Data Strobe is being used. The 
controller can also drive it slower than the maximum. And it can drive 
it with or without DS.

So, we have to be conservative and just use the dummy cycles for the 
maximum speed so we can at least make sure the flash works, albeit at 
slightly less efficiency. I hard-coded it to 20 but I suppose we can 
find it out from the Profile 1.0 table and use that (though we'd have to 
round it to an even value to avoid tripping up controllers). Will fix in 
next version (or, Tudor if you're fine with fixup! patches, I can send 
that too because I suspect it will be a small change).
 
> 
> > +   /*
> > +    * Set the Read Status Register dummy cycles and dummy address 
> bytes.
> > +    */
> > +   if (table[0] & PROFILE1_DWORD1_RDSR_DUMMY)
> > +      params->rdsr_dummy = 8;
> > +   else
> > +      params->rdsr_dummy = 4;
> > +
> > +   if (table[0] & PROFILE1_DWORD1_RDSR_ADDR_BYTES)
> > +      params->rdsr_addr_nbytes = 4;
> > +   else
> > +      params->rdsr_addr_nbytes = 0;
> > +
> > +out:
> > +   kfree(table);
> > +   return ret;
> > +}
> > +
>
Mason Yang May 20, 2020, 9:40 a.m. UTC | #3
Hi Pratyush, 
 
> > > +/**
> > > + * 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;
> > > +   size_t len;
> > > +   int ret, i;
> > > +
> > > +   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]);
> > > +
> > > +   /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> > > +   opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> > > +
> > > +   /*
> > > +    * Update the fast read settings. We set the default dummy 
cycles to 
> > 20
> > > +    * here. Flashes can change this value if they need to when 
enabling
> > > +    * octal mode.
> > > +    */
> > > + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > > +              0, 20, opcode,
> > > +              SNOR_PROTO_8_8_8_DTR);
> > > +
> > 
> > 
> > I thought we have a agreement that only do parse here, no other read 
> > parameters setting.
> 
> Yes, and I considered it. But it didn't make much sense to me to 
> introduce an extra member in struct spi_nor just to make this call in 
> some other function later.
> 
> Why exactly do you think doing this here is bad? The way I see it, we 
> avoid carrying around an extra member in spi_nor and this also allows 
> flashes to change the read settings easily in a post-sfdp hook. The 
> 4bait parsing function does something similar.

I think it's not a question for good or bad. 

4bait parsing function parse the 4-Byte Address Instruction Table
and set up read/pp parameters there for sure.

Here we give the function name spi_nor_parse_profile1() but also 
do others setting that has nothing to do with it, 
it seems not good for SW module design. 
oh, it's my humble opinion.


> 
> What are the benefits of doing it otherwise?

For other Octal Flash like mx25*

> 
> Note that I did remove HWCAPS selection from here, which did seem like a 

> sane idea.
> 
> > Driver should get dummy cycles used for various frequencies 
> > from 4th and 5th DWORD of xSPI table.[1]
> > 
> > [1] 
> > 
https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-3-git-

> send-email-masonccyang@mxic.com.tw/ 
> > 
> > 
> > In addition, 20 dummy cycles is for 200MHz but not for 100MHz, 133MHz 
and 
> > 166MHz
> > in case of read performance concern.
> > 
> > Given a correct dummy cycles for a specific device. [2] 
> > 
> > [2] 
> > 
https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-5-git-

> send-email-masonccyang@mxic.com.tw/ 
> 
> The problem is that we don't know what speed the controller is driving 
> the flash at, and whether it is using Data Strobe. BFPT tells us the 
> maximum speed of the flash based on if Data Strobe is being used. The 
> controller can also drive it slower than the maximum. And it can drive 
> it with or without DS.

This is for flash, not every Octal flash could work in 200MHz,
The Max operation speeds for other Octal Flash is 100, 133 , or 166MHz.

If a specific Octal Flash could work in 166MHz(Max), and driver setup the
correct 16 dummy cycles for it rather than 20 dummy cycles.
it's for performance concern.

> 
> So, we have to be conservative and just use the dummy cycles for the 
> maximum speed so we can at least make sure the flash works, albeit at 
> slightly less efficiency. I hard-coded it to 20 but I suppose we can 
> find it out from the Profile 1.0 table and use that (though we'd have to 

> round it to an even value to avoid tripping up controllers). Will fix in 

> next version (or, Tudor if you're fine with fixup! patches, I can send 
> that too because I suspect it will be a small change).
> 
> > 

thanks & best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Pratyush Yadav May 20, 2020, 10:37 a.m. UTC | #4
On 20/05/20 05:40PM, masonccyang@mxic.com.tw wrote:
> 
> Hi Pratyush, 
>  
> > > > +/**
> > > > + * 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;
> > > > +   size_t len;
> > > > +   int ret, i;
> > > > +
> > > > +   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]);
> > > > +
> > > > +   /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> > > > +   opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> > > > +
> > > > +   /*
> > > > +    * Update the fast read settings. We set the default dummy 
> cycles to 
> > > 20
> > > > +    * here. Flashes can change this value if they need to when 
> enabling
> > > > +    * octal mode.
> > > > +    */
> > > > + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > > > +              0, 20, opcode,
> > > > +              SNOR_PROTO_8_8_8_DTR);
> > > > +
> > > 
> > > 
> > > I thought we have a agreement that only do parse here, no other read 
> > > parameters setting.
> > 
> > Yes, and I considered it. But it didn't make much sense to me to 
> > introduce an extra member in struct spi_nor just to make this call in 
> > some other function later.
> > 
> > Why exactly do you think doing this here is bad? The way I see it, we 
> > avoid carrying around an extra member in spi_nor and this also allows 
> > flashes to change the read settings easily in a post-sfdp hook. The 
> > 4bait parsing function does something similar.
> 
> I think it's not a question for good or bad. 
> 
> 4bait parsing function parse the 4-Byte Address Instruction Table
> and set up read/pp parameters there for sure.
> 
> Here we give the function name spi_nor_parse_profile1() but also 

But the function that parses 4bait table is also called 
spi_nor_parse_4bait(). 

> do others setting that has nothing to do with it, 

Why has setting read opcode and dummy cycles got nothing to do with it? 
The purpose of the Profile 1.0 table is to tell us the Read Fast command 
and dummy cycles, among other things. I think it _does_ have something 
to do with it.

Just like the 4bait table tells us the 4-byte opcodes and we set them up 
in our data structures, the profile 1.0 table tells us the 8D read 
opcode and dummy cycles, and we set them up in our data structures.

> it seems not good for SW module design. 
> oh, it's my humble opinion.
> 
> > 
> > What are the benefits of doing it otherwise?
> 
> For other Octal Flash like mx25*

I mean from a design perspective. How does it make the code better, or 
the job of people who need to read/change it easier?

> > 
> > Note that I did remove HWCAPS selection from here, which did seem like a 
> 
> > sane idea.
> > 
> > > Driver should get dummy cycles used for various frequencies 
> > > from 4th and 5th DWORD of xSPI table.[1]
> > > 
> > > [1] 
> > > 
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-3-git-
> 
> > send-email-masonccyang@mxic.com.tw/ 
> > > 
> > > 
> > > In addition, 20 dummy cycles is for 200MHz but not for 100MHz, 133MHz 
> and 
> > > 166MHz
> > > in case of read performance concern.
> > > 
> > > Given a correct dummy cycles for a specific device. [2] 
> > > 
> > > [2] 
> > > 
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1587451187-6889-5-git-
> 
> > send-email-masonccyang@mxic.com.tw/ 
> > 
> > The problem is that we don't know what speed the controller is driving 
> > the flash at, and whether it is using Data Strobe. BFPT tells us the 
> > maximum speed of the flash based on if Data Strobe is being used. The 
> > controller can also drive it slower than the maximum. And it can drive 
> > it with or without DS.
> 
> This is for flash, not every Octal flash could work in 200MHz,
> The Max operation speeds for other Octal Flash is 100, 133 , or 166MHz.
> 
> If a specific Octal Flash could work in 166MHz(Max), and driver setup the
> correct 16 dummy cycles for it rather than 20 dummy cycles.
> it's for performance concern.

Agreed. Like I mentioned in the next paragraph, will fix.
 
> > 
> > So, we have to be conservative and just use the dummy cycles for the 
> > maximum speed so we can at least make sure the flash works, albeit at 
> > slightly less efficiency. I hard-coded it to 20 but I suppose we can 
> > find it out from the Profile 1.0 table and use that (though we'd have to 
> 
> > round it to an even value to avoid tripping up controllers). Will fix in 
> 
> > next version (or, Tudor if you're fine with fixup! patches, I can send 
> > that too because I suspect it will be a small change).
> >
Mason Yang May 21, 2020, 8:09 a.m. UTC | #5
Hi Pratyush, 

> > 
> > > > > +/**
> > > > > + * 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;
> > > > > +   size_t len;
> > > > > +   int ret, i;
> > > > > +
> > > > > +   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]);
> > > > > +
> > > > > +   /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> > > > > +   opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> > > > > +
> > > > > +   /*
> > > > > +    * Update the fast read settings. We set the default dummy 
> > cycles to 
> > > > 20
> > > > > +    * here. Flashes can change this value if they need to when 
> > enabling
> > > > > +    * octal mode.
> > > > > +    */
> > > > > + 
spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > > > > +              0, 20, opcode,
> > > > > +              SNOR_PROTO_8_8_8_DTR);
> > > > > +
> > > > 
> > > > 
> > > > I thought we have a agreement that only do parse here, no other 
read 
> > > > parameters setting.
> > > 
> > > Yes, and I considered it. But it didn't make much sense to me to 
> > > introduce an extra member in struct spi_nor just to make this call 
in 
> > > some other function later.
> > > 
> > > Why exactly do you think doing this here is bad? The way I see it, 
we 
> > > avoid carrying around an extra member in spi_nor and this also 
allows 
> > > flashes to change the read settings easily in a post-sfdp hook. The 
> > > 4bait parsing function does something similar.
> > 
> > I think it's not a question for good or bad. 
> > 
> > 4bait parsing function parse the 4-Byte Address Instruction Table
> > and set up read/pp parameters there for sure.
> > 
> > Here we give the function name spi_nor_parse_profile1() but also 
> 
> But the function that parses 4bait table is also called 
> spi_nor_parse_4bait(). 
> 
> > do others setting that has nothing to do with it, 
> 
> Why has setting read opcode and dummy cycles got nothing to do with it? 
> The purpose of the Profile 1.0 table is to tell us the Read Fast command 

> and dummy cycles, among other things. I think it _does_ have something 
> to do with it.

As you know I mean this function just do parse parameter of profile 1 
table
and keep these value data for later usage.

A device supports xSPI profile table could work in either 8S-8S-8S or 
8D-8D-8D mode.
It seems to setup these parameters somewhere out here is betters.

> 
> Just like the 4bait table tells us the 4-byte opcodes and we set them up 

> in our data structures, the profile 1.0 table tells us the 8D read 
> opcode and dummy cycles, and we set them up in our data structures.
> 
> > it seems not good for SW module design. 
> > oh, it's my humble opinion.
> > 
> > > 
> > > What are the benefits of doing it otherwise?
> > 
> > For other Octal Flash like mx25*
> 
> I mean from a design perspective. How does it make the code better, or 
> the job of people who need to read/change it easier?

yes, agreed.
I also need to patch for 8S-8S-8S mode, not only 8D-8D-8D mode.
That's why we have some discussions.

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Pratyush Yadav May 21, 2020, 9:14 a.m. UTC | #6
On 21/05/20 04:09PM, masonccyang@mxic.com.tw wrote:
> 
> Hi Pratyush, 
> 
> > > > > > +   /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> > > > > > +   opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> > > > > > +
> > > > > > +   /*
> > > > > > +    * Update the fast read settings. We set the default dummy 
> > > cycles to 
> > > > > 20
> > > > > > +    * here. Flashes can change this value if they need to when 
> > > enabling
> > > > > > +    * octal mode.
> > > > > > +    */
> > > > > > + 
> spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > > > > > +              0, 20, opcode,
> > > > > > +              SNOR_PROTO_8_8_8_DTR);
> > > > > > +
> > > > > 
> > > > > 
> > > > > I thought we have a agreement that only do parse here, no other 
> read 
> > > > > parameters setting.
> > > > 
> > > > Yes, and I considered it. But it didn't make much sense to me to 
> > > > introduce an extra member in struct spi_nor just to make this call 
> in 
> > > > some other function later.
> > > > 
> > > > Why exactly do you think doing this here is bad? The way I see it, 
> we 
> > > > avoid carrying around an extra member in spi_nor and this also 
> allows 
> > > > flashes to change the read settings easily in a post-sfdp hook. The 
> > > > 4bait parsing function does something similar.
> > > 
> > > I think it's not a question for good or bad. 
> > > 
> > > 4bait parsing function parse the 4-Byte Address Instruction Table
> > > and set up read/pp parameters there for sure.
> > > 
> > > Here we give the function name spi_nor_parse_profile1() but also 
> > 
> > But the function that parses 4bait table is also called 
> > spi_nor_parse_4bait(). 
> > 
> > > do others setting that has nothing to do with it, 
> > 
> > Why has setting read opcode and dummy cycles got nothing to do with it? 
> > The purpose of the Profile 1.0 table is to tell us the Read Fast 
> > command and dummy cycles, among other things. I think it _does_ have 
> > something to do with it.
> 
> As you know I mean this function just do parse parameter of profile 1 
> table
> and keep these value data for later usage.
> 
> A device supports xSPI profile table could work in either 8S-8S-8S or 
> 8D-8D-8D mode.
> It seems to setup these parameters somewhere out here is betters.

As far as I know, the Profile 1.0 table only describes 8D-8D-8D mode. I 
see no mention of 8S-8S-8S in JESD251 or JESD216D.01. No field in the 
table describes anything related to 8S. In fact, searching for "8S" in 
the JESD251 spec yields 0 results. 

Anyway, you should set up 8S parameters in SNOR_CMD_READ_8_8_8, not 
SNOR_CMD_READ_8_8_8_DTR. 8D configuration is independent of 8S 
configuration.

PS: If you have any more comments, please send them now. The merge 
window is getting close, and I'd like to see this make it in.
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 388e695e763f..642e3c07acf9 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2355,7 +2355,7 @@  static int spi_nor_check(struct spi_nor *nor)
 	return 0;
 }
 
-static void
+void
 spi_nor_set_read_settings(struct spi_nor_read_command *read,
 			  u8 num_mode_clocks,
 			  u8 num_wait_states,
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index de1e3917889f..7e6df8322da0 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -192,6 +192,9 @@  struct spi_nor_locking_ops {
  *
  * @size:		the flash memory density in bytes.
  * @page_size:		the page size of the SPI NOR flash memory.
+ * @rdsr_dummy:		dummy cycles needed for Read Status Register command.
+ * @rdsr_addr_nbytes:	dummy address bytes needed for Read Status Register
+ *			command.
  * @hwcaps:		describes the read and page program hardware
  *			capabilities.
  * @reads:		read capabilities ordered by priority: the higher index
@@ -214,6 +217,8 @@  struct spi_nor_locking_ops {
 struct spi_nor_flash_parameter {
 	u64				size;
 	u32				page_size;
+	u8				rdsr_dummy;
+	u8				rdsr_addr_nbytes;
 
 	struct spi_nor_hwcaps		hwcaps;
 	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
@@ -424,6 +429,11 @@  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 
 int spi_nor_hwcaps_read2cmd(u32 hwcaps);
 u8 spi_nor_convert_3to4_read(u8 opcode);
+void spi_nor_set_read_settings(struct spi_nor_read_command *read,
+			      u8 num_mode_clocks,
+			      u8 num_wait_states,
+			      u8 opcode,
+			      enum spi_nor_protocol proto);
 void spi_nor_set_pp_settings(struct spi_nor_pp_command *pp, u8 opcode,
 			     enum spi_nor_protocol proto);
 
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index ab086aa4746f..4e5e0eabe2d9 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -4,6 +4,7 @@ 
  * Copyright (C) 2014, Freescale Semiconductor, Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/slab.h>
 #include <linux/sort.h>
 #include <linux/mtd/spi-nor.h>
@@ -19,12 +20,14 @@ 
 #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
 #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
 #define SFDP_4BAIT_ID		0xff84  /* 4-byte Address Instruction Table */
+#define SFDP_PROFILE1_ID	0xff05	/* xSPI Profile 1.0 table. */
 
 #define SFDP_SIGNATURE		0x50444653U
 #define SFDP_JESD216_MAJOR	1
 #define SFDP_JESD216_MINOR	0
 #define SFDP_JESD216A_MINOR	5
 #define SFDP_JESD216B_MINOR	6
+#define SFDP_JESD216D_MINOR	8
 
 struct sfdp_header {
 	u32		signature; /* Ox50444653U <=> "SFDP" */
@@ -70,6 +73,11 @@  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 SMPT_CMD_ADDRESS_LEN_MASK		GENMASK(23, 22)
 #define SMPT_CMD_ADDRESS_LEN_0			(0x0UL << 22)
 #define SMPT_CMD_ADDRESS_LEN_3			(0x1UL << 22)
@@ -1110,6 +1118,67 @@  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;
+	size_t len;
+	int ret, i;
+
+	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]);
+
+	/* Get 8D-8D-8D fast read opcode and dummy cycles. */
+	opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
+
+	/*
+	 * Update the fast read settings. We set the default dummy cycles to 20
+	 * here. Flashes can change this value if they need to when enabling
+	 * octal mode.
+	 */
+	spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
+				  0, 20, opcode,
+				  SNOR_PROTO_8_8_8_DTR);
+
+	/*
+	 * Set the Read Status Register dummy cycles and dummy address bytes.
+	 */
+	if (table[0] & PROFILE1_DWORD1_RDSR_DUMMY)
+		params->rdsr_dummy = 8;
+	else
+		params->rdsr_dummy = 4;
+
+	if (table[0] & PROFILE1_DWORD1_RDSR_ADDR_BYTES)
+		params->rdsr_addr_nbytes = 4;
+	else
+		params->rdsr_addr_nbytes = 0;
+
+out:
+	kfree(table);
+	return ret;
+}
+
 /**
  * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
  * @nor:		pointer to a 'struct spi_nor'
@@ -1211,6 +1280,10 @@  int spi_nor_parse_sfdp(struct spi_nor *nor,
 			err = spi_nor_parse_4bait(nor, param_header, params);
 			break;
 
+		case SFDP_PROFILE1_ID:
+			err = spi_nor_parse_profile1(nor, param_header, params);
+			break;
+
 		default:
 			break;
 		}