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

quoted

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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help