[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Simon Glass]

Hi Bin,

On 12 September 2017 at 07:53, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Sep 6, 2017 at 9:39 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 26 August 2017 at 18:12, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Sun, Aug 27, 2017 at 6:40 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Bin,
>>>>
>>>> On 26 August 2017 at 07:58, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg at chromium.org> wrote:
>>>>>> Hi Bin,
>>>>>>
>>>>>> On 15 August 2017 at 23:38, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>>>> it's bootloader's responsibility to configure the SPI controller's
>>>>>>> opcode registers properly otherwise SPI controller driver doesn't
>>>>>>> know how to communicate with the SPI flash device.
>>>>>>>
>>>>>>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>>>>>>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>>>>>>> before the lock-down.
>>>>>>>
>>>>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  arch/x86/Kconfig              |  9 +++++++++
>>>>>>>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>>>>>>  2 files changed, 33 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>>>>>> index c26710b..5373082 100644
>>>>>>> --- a/arch/x86/Kconfig
>>>>>>> +++ b/arch/x86/Kconfig
>>>>>>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>>>>>>           do not overwrite the important boot service data which is used by
>>>>>>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>>>>>>
>>>>>>> +config FSP_LOCKDOWN_SPI
>>>>>>> +       bool
>>>>>>> +       depends on HAVE_FSP
>>>>>>> +       help
>>>>>>> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>>>>>>> +         for such FSP and U-Boot will configure the SPI opcode registers
>>>>>>> +         before the lock-down.
>>>>>>> +
>>>>>>>  config ENABLE_MRC_CACHE
>>>>>>>         bool "Enable MRC cache"
>>>>>>>         depends on !EFI && !SYS_COREBOOT
>>>>>>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>>>>>>> index 3397bb8..320d87d 100644
>>>>>>> --- a/arch/x86/lib/fsp/fsp_common.c
>>>>>>> +++ b/arch/x86/lib/fsp/fsp_common.c
>>>>>>> @@ -19,6 +19,8 @@
>>>>>>>
>>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>>
>>>>>>> +extern void ich_spi_config_opcode(struct udevice *dev);
>>>>>>> +
>>>>>>>  int checkcpu(void)
>>>>>>>  {
>>>>>>>         return 0;
>>>>>>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>>>>>>  {
>>>>>>>         u32 status;
>>>>>>>
>>>>>>> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>>>>>>> +       struct udevice *dev;
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>>>> +        * it's bootloader's responsibility to configure the SPI controller's
>>>>>>> +        * opcode registers properly otherwise SPI controller driver doesn't
>>>>>>> +        * know how to communicate with the SPI flash device.
>>>>>>> +        *
>>>>>>> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
>>>>>>> +        * controller driver's probe() routine), becasue:
>>>>>>> +        *
>>>>>>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>>>>>>> +        * 2). Any SPI transfer will corrupt the contents of these registers
>>>>>>> +        *
>>>>>>> +        * Hence we have to do it right here before SPI lock-down bit is set.
>>>>>>> +        */
>>>>>>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>>>>>>> +               ich_spi_config_opcode(dev);
>>>>>>
>>>>>> I wonder if we could do this by using an operation instead of directly
>>>>>> calling a function in the driver?
>>>>>>
>>>>>
>>>>> Do you mean adding one operation to dm_spi_ops?
>>>>
>>>> Yes I think that would be better.
>>>>
>>>
>>> Since this is x86-specific, I would hesitate to add one.
>>>
>>
>> Yes I understand that. Still I don't think we should call directly
>> into drivers. What do you suggest?
>>
>> - add some sort of DM event system to which drivers can attach for notifications
>> - add an ioctl() method to the SPI uclass where we can put random
>> calls like this
>> - set up a new MISC device as a child of SPI which includes an ioctl
>> for this operation
>> - something else?
>>
>
> These are maybe too complicated to solve this little specific issue.
> I've thought about this, and looks we can add a simple DTS property
> "intel,spi-lock-down" and let the driver call this function.

OK, sounds good so long as it knows when to call it.

Regards,
Simon