[U-Boot] [PATCH v3] spi: ich: Configure SPI BIOS parameters

[Bin Meng]

Hi Stefan,

On Fri, Feb 10, 2017 at 6:17 PM, Stefan Roese <sr at denx.de> wrote:
> Hi Bin,
>
> On 10.02.2017 06:45, Bin Meng wrote:
>
> <snip>
>
>
>>> @@ -127,6 +120,44 @@ struct spi_trans {
>>>  #define SPI_OPCODE_WREN                0x06
>>>  #define SPI_OPCODE_FAST_READ   0x0b
>>>
>>> +#define SPI_OPCODE_TYPE_READ_NO_ADDRESS                0
>>> +#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS       1
>>> +#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS      2
>>> +#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS     3
>>> +
>>
>>
>> Thanks for changing this.
>>
>>> +#define SPI_OPMENU_0   0x01 /* WRSR: Write Status Register */
>>> +#define SPI_OPTYPE_0   SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
>>> +
>>> +#define SPI_OPMENU_1   0x02 /* BYPR: Byte Program */
>>> +#define SPI_OPTYPE_1   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_2   0x03 /* READ: Read Data */
>>> +#define SPI_OPTYPE_2   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_3   0x05 /* RDSR: Read Status Register */
>>> +#define SPI_OPTYPE_3   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>>> +
>>> +#define SPI_OPMENU_4   0x20 /* SE20: Sector Erase 0x20 */
>>> +#define SPI_OPTYPE_4   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_5   0x9f /* RDID: Read ID */
>>> +#define SPI_OPTYPE_5   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>>> +
>>> +#define SPI_OPMENU_6   0xd8 /* BED8: Block Erase 0xd8 */
>>> +#define SPI_OPTYPE_6   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_7   0x0b /* FAST: Fast Read */
>>> +#define SPI_OPTYPE_7   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>>> +
>>
>>
>> These are SPI flash commands. Should we name them similarly like
>> SPI_OPCODE_WREN and SPI_OPCODE_FAST_READ?
>
>
> I've thought about this as well but neglected doing this (for now).
> If you think its "better" that way, I'll send a v4 shortly. Just
> let me know.
>

OK, let's leave it for now.

>>> +#define SPI_OPTYPE ((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \
>>> +                   (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 <<  8) | \
>>> +                   (SPI_OPTYPE_3 <<  6) | (SPI_OPTYPE_2 <<  4) | \
>>> +                   (SPI_OPTYPE_1 <<  2) | (SPI_OPTYPE_0 <<  0))
>>> +#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) |
>>> \
>>> +                         (SPI_OPMENU_5 <<  8) | (SPI_OPMENU_4 <<  0))
>>> +#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) |
>>> \
>>> +                         (SPI_OPMENU_1 <<  8) | (SPI_OPMENU_0 <<  0))
>>> +
>>
>>
>> What if the board mounts a flash with a different SPI flash command
>> set? Will this work?
>
>
> Frankly, I can't tell for sure but its very likely. As you might
> have guessed, these defines are taken from coreboot where they are
> usually board specific and written in the last stage before booting
> into the OS (IIRC). It would definitely make sense to add a mechanism
> to configure these BIOS parameters in a board-specific way. So that
> boards can choose to (optionally) provide different values.
>
> An easy way to do this, would be to make the newly created function
> spi_controller_config() a wear default. This way, board can always
> overwrite these default values (and / or do other stuff) in their
> board specific code. This could also be added in a follow up patch
> once this is needed though. I still have to see such a case of an
> "incompatible" SPI flash on x86, and IIRC all values in coreboot
> were identical.

I am OK with the weak implementation, but I know Simon dislikes weak
:) Maybe he has a better idea.

Let's hear what others think.

Regards,
Bin