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

[masonccyang@mxic.com.tw]

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.

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


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