[U-Boot] [PATCH v3 1/9] sf: Update SST flash params

[Bin Meng]

Hi Jagan,

On Wed, Apr 22, 2015 at 5:15 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Bin,
>
> On 22 April 2015 at 14:13, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Jagan,
>>
>> On Wed, Apr 22, 2015 at 4:06 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On 22 April 2015 at 12:44, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Jagan,
>>>>
>>>> On Wed, Apr 22, 2015 at 3:03 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On 22 April 2015 at 12:14, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>> Hi Jagan,
>>>>>>
>>>>>> On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>>>> Hi Bin,
>>>>>>>
>>>>>>> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>>>> Hi Jagan,
>>>>>>>>
>>>>>>>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>>>>>> Hi Bin,
>>>>>>>>>
>>>>>>>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>>>>>> Hi Jagan,
>>>>>>>>>>
>>>>>>>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>>>>>>>> Hi Bin,
>>>>>>>>>>>
>>>>>>>>>>> I think you have a different interpretation of sector size here-
>>>>>>>>>>>
>>>>>>>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't
>>>>>>>>>>>  * necessarily called a "sector" by the vendor.
>>>>>>>>>>>  */
>>>>>>>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is
>>>>>>>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or
>>>>>>>>>>> 64K sector erase through flags.
>>>>>>>>>>>
>>>>>>>>>>> Linux does follow the same-
>>>>>>>>>>>         /* SST -- large erase sizes are "overlays", "sectors" are 4K */
>>>>>>>>>>>         { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8, SECT_4K |
>>>>>>>>>>> SST_WRITE) },
>>>>>>>>>>>         { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K |
>>>>>>>>>>> SST_WRITE) },
>>>>>>>>>>>         { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K |
>>>>>>>>>>> SST_WRITE) },
>>>>>>>>>>>         { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K |
>>>>>>>>>>> SST_WRITE) },
>>>>>>>>>>>
>>>>>>>>>>> Please check it.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yes, I know this pretty well. And I want to change this behavior, as
>>>>>>>>>> my cover letter says.
>>>>>>>>>>
>>>>>>>>>> Currently the 'sf erase' command operates on a 64KB granularity, while
>>>>>>>>>> the actual erase command is 4KB granularity, which is inconsistent and
>>>>>>>>>> causes confusion.
>>>>>>>>>
>>>>>>>>> It never related to 'sf erase' instead based on the 'params->flags'
>>>>>>>>> sf_probe will decide which erase_cmd with erase_size will use.
>>>>>>>>
>>>>>>>> No, it is related. See cmd_sf.c:
>>>>>>>
>>>>>>> I'm not getting your point- how could it erase use 64K sector size
>>>>>>> instead of 4K.
>>>>>>
>>>>>> It indeed erases 64K sector size. You need check the logic in
>>>>>> spi_flash_validate_params().
>>>>>
>>>>> We're assigning erase_size to sector_size only when SECT_4K and SECT_32K
>>>>> and for these erase_size becomes direct values, please check this.
>>>>
>>>> You previous email already said: the 'sf erase' command depends on
>>>> *flash->sector_size*
>>>>
>>>>>        /* Compute erase sector and command */
>>>>>         if (params->flags & SECT_4K) {
>>>>>                 flash->erase_cmd = CMD_ERASE_4K;
>>>>>                 flash->erase_size = 4096;
>>>>>         } else if (params->flags & SECT_32K) {
>>>>>                 flash->erase_cmd = CMD_ERASE_32K;
>>>>>                 flash->erase_size = 32768;
>>>>>         } else {
>>>>>                 flash->erase_cmd = CMD_ERASE_64K;
>>>>>                 flash->erase_size = flash->sector_size;
>>>>>         }
>>>>
>>>> Here the codes says: *flash->erase_size*
>>>>
>>>> So when I give a 'sf erase 0 100' it erase 64KB even if you have SECT_4K.
>>>
>>> Example:
>>> "SST25WF080",     0xbf2505, 0x0,       64 * 1024,    16, RD_NORM,
>>>     SECT_4K | SST_WR},
>>>
>>> sf probe gives
>>> sector_size = 64 * 1024 and erase_size = 4096
>>>
>>> sf erase 0 100
>>> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns
>>> "SF: Erase offset/length not multiple of erase size"
>>
>> sf erase 0 +100. Sorry for the typo. But looks like you are not really
>> reading the codes.
>>
>
> Worked on too-many overclocked issue, sorry for that.
>
> So, something fixed in sf_probe.c will fix this I guess.

Good, you finally got it! So you prefer fixing this inconsistency in
sf_probe.c? I guess by overriding flash->sector_size and
flash->nr_sectors if SECT_4K?

>> => sf probe
>> SF: Detected SST25VF016B with page size 256 Bytes, erase size 4 KiB,
>> total 2 MiB, mapped at ffe00000
>>
>> => sf erase 0 +100
>> SF: 65536 bytes @ 0x0 Erased: OK
>>
>> Tested on two boards, and both shows 64K was erased.
>>
>>> Example:
>>> "SST25WF080",     0xbf2505, 0x0,       64 * 1024,    16, RD_NORM,
>>>     SST_WR},
>>>
>>> sf probe gives
>>> sector_size = 64 * 1024 and erase_size = 64 * 1024
>>>
>>> sf erase 0 100
>>> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns
>>> "SF: Erase offset/length not multiple of erase size"
>>>
>>> Still have any concerns, please come to IRC for more discussion
>

As you see I have rebased this patch once for v2/v3 and lots of effort
were spent on this series. I remember you said this patch series needs
some testing on your side, but this comment shows that you may want to
do it in another way. I really hope such comments could be sent months
ago. Today I can't even remember all of the details in this series.
Luckily I don't lose interest to get this upstream so I kept asking
for an update.

Regards,
Bin