[U-Boot] [PATCH 2/2] sf: Preserve QE bit when clearing BP# bits for Macronix flash

[Bin Meng]

Hi Jagan,

On Mon, Aug 14, 2017 at 12:58 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Bin,
>
> On Mon, Aug 14, 2017 at 8:07 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Jagan,
>>
>> On Mon, Aug 14, 2017 at 1:22 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Wed, Aug 2, 2017 at 3:56 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Jagan,
>>>>
>>>> On Wed, Aug 2, 2017 at 12:01 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>> On Sun, Jul 23, 2017 at 8:14 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>> On some flash (like Macronix), QE (quad enable) bit is in the same
>>>>>> status register as BP# bits, and we need preserve its original value
>>>>>> during a reboot cycle as this is required by some platforms (like
>>>>>> Intel ICH SPI controller working under descriptor mode).
>>>>>>
>>>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>>> ---
>>>>>>
>>>>>>  drivers/mtd/spi/spi_flash.c | 17 +++++++++++++++--
>>>>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>>>>> index 0034a28..7d8c660 100644
>>>>>> --- a/drivers/mtd/spi/spi_flash.c
>>>>>> +++ b/drivers/mtd/spi/spi_flash.c
>>>>>> @@ -947,11 +947,24 @@ int spi_flash_scan(struct spi_flash *flash)
>>>>>>         if (IS_ERR_OR_NULL(info))
>>>>>>                 return -ENOENT;
>>>>>>
>>>>>> -       /* Flash powers up read-only, so clear BP# bits */
>>>>>> +       /*
>>>>>> +        * Flash powers up read-only, so clear BP# bits.
>>>>>> +        *
>>>>>> +        * Note on some flash (like Macronix), QE (quad enable) bit is in the
>>>>>> +        * same status register as BP# bits, and we need preserve its original
>>>>>> +        * value during a reboot cycle as this is required by some platforms
>>>>>> +        * (like Intel ICH SPI controller working under descriptor mode).
>>>>>> +        */
>>>>>>         if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_ATMEL ||
>>>>>> -           JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_MACRONIX ||
>>>>>>             JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_SST)
>>>>>>                 write_sr(flash, 0);
>>>>>> +       if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_MACRONIX) {
>>>>>> +               u8 sr = 0;
>>>>>> +
>>>>>> +               read_sr(flash, &sr);
>>>>>> +               sr &= STATUS_QEB_MXIC;
>>>>>> +               write_sr(flash, sr);
>
> Better assign sr with QEB for macronix and call write_sr once.

For these Macronix flashes that does not support quard RW, QEB bit is
reserved. Writing 1 to a reserved bit is not a good practice.

>
>>>>>> +       }
>>>>>
>>>>> It doesn't make sense to have one(specific) controller fix to be
>>>>> generic to all macronix chips, think about alternative.
>>>>>
>>>>
>>>> This is no way to fix at the controller level. Actually this is
>>>> nothing related to controller level. It's just the bootstrap settings
>>>> (QE bit in this case) that cannot be overwritten by someone else
>>>> (although it's seen on Intel, it might happen on some other
>>>> architecture). The logic in the codes are having issues. Its comment
>>>> says: clear BP# bits, but it clears QE bit for Macronix flash as well,
>>>> which is wrong. The update was just to make sure the codes do as what
>>>> its comment says.
>>>
>>> I believe QEB is same position for all Macronix chips, checked few
>>> parts true? what if the supported chips from id tables doesn't have
>>> QEB at-all means specific chip support upto dual?
>>>
>>
>> Correct, QEB is in the same position (bit6) for all Macronix chips. If
>> a chipset that does not support QEB, that bit (bit6) is reserved, and
>> current patch still works.
>>
>> So current patch can correctly handle both situations. The issue here
>> is that what we do here for the status register does NOT conform the
>> comment. We only wanted to clear the #BP bits. We should NOT clear the
>> QEB bit at all.
>
> OK, thanks.

Regards,
Bin