[U-Boot] [PATCH v3] spi: ich: Configure SPI BIOS parameters
Simon Glass
sjg at chromium.org
Wed Feb 22 03:59:24 UTC 2017
Hi,
On 13 February 2017 at 01:45, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.
Well how about crossing that bridge when we come to it? For now,
perhaps this is good enough. You are right, I'm not keen on weak
functions as I see them as ad-hoc APIs. Better, I believe, to think
about it and define a real API. For example in this case I wonder if
the remove() method of the SPI driver could do something? Or perhaps
add another method like finalise()?
Regards,
Simon
More information about the U-Boot
mailing list