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

[Bin Meng]

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.

Regards,
Bin