[U-Boot] [PATCH v2 15/33] x86: Refactor PCI to permit alternate init

[Simon Glass]

Hi Bin,

On 11 November 2014 18:50, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Nov 12, 2014 at 9:02 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 11 November 2014 07:14, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Tue, Nov 11, 2014 at 9:00 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> We want access PCI earlier in the init sequence, so refactor the code so
>>>> that it does not require use of a BSS variable to work. This will allow us
>>>> to use early malloc() to store information about a PCI hose.
>>>>
>>>> Common PCI code moves to arch/x86/cpu/pci.c and a new
>>>> board_pci_setup_hose() function is provided by boards to set up the (single)
>>>> hose used by that board.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>>
>>>>  arch/x86/cpu/Makefile       |  1 +
>>>>  arch/x86/cpu/coreboot/pci.c | 22 ++++++++--------------
>>>>  arch/x86/cpu/pci.c          | 26 ++++++++++++++++++++++++++
>>>>  arch/x86/include/asm/pci.h  | 11 +++++++++++
>>>>  4 files changed, 46 insertions(+), 14 deletions(-)
>>>>  create mode 100644 arch/x86/cpu/pci.c
>>>>
>>>> diff --git a/arch/x86/cpu/Makefile b/arch/x86/cpu/Makefile
>>>> index 9d38ef7..97f36d5 100644
>>>> --- a/arch/x86/cpu/Makefile
>>>> +++ b/arch/x86/cpu/Makefile
>>>> @@ -11,3 +11,4 @@
>>>>  extra-y        = start.o
>>>>  obj-$(CONFIG_X86_RESET_VECTOR) += resetvec.o start16.o
>>>>  obj-y  += interrupts.o cpu.o call64.o
>>>> +obj-$(CONFIG_PCI) += pci.o
>>>> diff --git a/arch/x86/cpu/coreboot/pci.c b/arch/x86/cpu/coreboot/pci.c
>>>> index 33f16a3..130fd88 100644
>>>> --- a/arch/x86/cpu/coreboot/pci.c
>>>> +++ b/arch/x86/cpu/coreboot/pci.c
>>>> @@ -13,8 +13,6 @@
>>>>  #include <pci.h>
>>>>  #include <asm/pci.h>
>>>>
>>>> -static struct pci_controller coreboot_hose;
>>>> -
>>>>  static void config_pci_bridge(struct pci_controller *hose, pci_dev_t dev,
>>>>                               struct pci_config_table *table)
>>>>  {
>>>> @@ -31,19 +29,15 @@ static struct pci_config_table pci_coreboot_config_table[] = {
>>>>         {}
>>>>  };
>>>>
>>>> -void pci_init_board(void)
>>>> +void board_pci_setup_hose(struct pci_controller *hose)
>>>>  {
>>>> -       coreboot_hose.config_table = pci_coreboot_config_table;
>>>> -       coreboot_hose.first_busno = 0;
>>>> -       coreboot_hose.last_busno = 0;
>>>> -
>>>> -       pci_set_region(coreboot_hose.regions + 0, 0x0, 0x0, 0xffffffff,
>>>> -               PCI_REGION_MEM);
>>>> -       coreboot_hose.region_count = 1;
>>>> -
>>>> -       pci_setup_type1(&coreboot_hose);
>>>> +       hose->config_table = pci_coreboot_config_table;
>>>> +       hose->first_busno = 0;
>>>> +       hose->last_busno = 0;
>>>>
>>>> -       pci_register_hose(&coreboot_hose);
>>>> +       pci_set_region(hose->regions + 0, 0x0, 0x0, 0xffffffff,
>>>> +                      PCI_REGION_MEM);
>>>> +       hose->region_count = 1;
>>>>
>>>> -       pci_hose_scan(&coreboot_hose);
>>>> +       pci_setup_type1(hose);
>>>
>>> Should pci_setup_type1(hose) be moved to arch/x86/cpu/pci.c?
>>
>> It seems happy enough in lib/ - why do you think it should move?
>
> Sorry I should have said it more clearly. I mean the call to
> pci_setup_type1() should be moved to
> arch/x86/cpu/pci.c::pci_init_board(), right after the call to
> board_pci_setup_hose().

OK I see. But I think this might be board-specific code. Some boards
might have different functions for access.

>
>>>
>>>>  }
>>>> diff --git a/arch/x86/cpu/pci.c b/arch/x86/cpu/pci.c
>>>> new file mode 100644
>>>> index 0000000..030cbbc
>>>> --- /dev/null
>>>> +++ b/arch/x86/cpu/pci.c
>>>> @@ -0,0 +1,26 @@
>>>> +/*
>>>> + * Copyright (c) 2011 The Chromium OS Authors.
>>>> + * (C) Copyright 2008,2009
>>>> + * Graeme Russ, <graeme.russ at gmail.com>
>>>> + *
>>>> + * (C) Copyright 2002
>>>> + * Daniel Engström, Omicron Ceti AB, <daniel at omicron.se>
>>>> + *
>>>> + * SPDX-License-Identifier:    GPL-2.0+
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <pci.h>
>>>> +#include <asm/pci.h>
>>>> +
>>>> +static struct pci_controller x86_hose;
>>>> +
>>>> +void pci_init_board(void)
>>>> +{
>>>> +       struct pci_controller *hose = &x86_hose;
>>>> +
>>>> +       board_pci_setup_hose(hose);
>>>> +       pci_register_hose(hose);
>>>> +
>>>> +       pci_hose_scan(hose);
>>>
>>> Should we save the return value of pci_hose_scan(hose) to
>>> hose->last_busno? I noticed that coreboot/pci.c is doing this in the
>>> config_device callback, but that callback causes infinite loop when I
>>> tested that logic on the Crown Bay board.
>>
>> I expect so, but perhaps that is for you to decide out when you send
>> your patches? My objective is to get ivybridge into a reasonable
>> state, but I and certainly expecting that you will want to change some
>> things.
>>
>> When you do that I will be able to test and may comments so we can
>> keep both working.
>
> Yep, I will send patches after your patches are in the mainline.

OK.

Regards,
Simon