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

[Simon Glass]

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?

Regards,
Simon