Hi Bin,
On 01.04.2017 04:02, Bin Meng wrote:
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.
Thanks.
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()?
Okay, lets think about this in more detail, once its necessary.
What's next step for this patch? I see Stefan has done some patches on dm core.
Correct, and this one would fit perfectly into this new DM removal / finalization framework. Additionally, I've just noticed yesterday, that this SPI patch is enough for the Linux Intel SPI NOR driver to correctly detect the NOR chip. But writing is not possible, since the SPI NOR seem to be protected. I still need to investigate here to fix this.
So I suggest to postpone this SPI patch until I have a newer, better version, perhaps already based on this DM remove framework.
Thanks, Stefan