On 21/05/20 04:09PM, masonccyang@mxic.com.tw wrote:

Hi Pratyush, 

quoted

quoted

quoted

quoted

quoted

+   /* 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 

quoted

quoted

20

quoted

+    * here. Flashes can change this value if they need to when 

enabling

quoted

quoted

quoted

+    * octal mode.
+    */
+ 

spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],

quoted

quoted

quoted

quoted

quoted

+              0, 20, opcode,
+              SNOR_PROTO_8_8_8_DTR);
+

I thought we have a agreement that only do parse here, no other 

read 

quoted

quoted

quoted

quoted

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 

quoted

quoted

quoted

some other function later.

Why exactly do you think doing this here is bad? The way I see it, 

we 

quoted

quoted

quoted

avoid carrying around an extra member in spi_nor and this also 

allows 

quoted

quoted

quoted

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(). 

quoted

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.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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