[U-Boot] [PATCH v2 5/7] x86: pci: Tidy up the generic x86 PCI driver

[Simon Glass]

Hi Bin,

On 29 June 2015 at 03:21, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass <sjg at chromium.org> wrote:
>> This driver should use the x86 PCI configuration functions. Also adjust its
>> compatible string to something generic (i.e. without a vendor name).
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>> Changes in v2:
>> - Rename the ops and ids arrays for consistency
>> - Drop the coreboot PCI driver which is no-longer needed
>>
>>  arch/x86/cpu/coreboot/pci.c | 21 ---------------------
>>  drivers/pci/pci_x86.c       | 13 ++++++++-----
>>  2 files changed, 8 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/x86/cpu/coreboot/pci.c b/arch/x86/cpu/coreboot/pci.c
>> index 67eb14c..af9f391 100644
>> --- a/arch/x86/cpu/coreboot/pci.c
>> +++ b/arch/x86/cpu/coreboot/pci.c
>> @@ -11,30 +11,9 @@
>>
>>  #include <common.h>
>>  #include <dm.h>
>> -#include <errno.h>
>>  #include <pci.h>
>> -#include <asm/io.h>
>>  #include <asm/pci.h>
>
> Should <pci.h> and <asm/pci.h> be removed too?

OK.

>
>>
>> -DECLARE_GLOBAL_DATA_PTR;
>> -
>> -static const struct dm_pci_ops pci_x86_ops = {
>> -       .read_config    = pci_x86_read_config,
>> -       .write_config   = pci_x86_write_config,
>> -};
>> -
>> -static const struct udevice_id pci_x86_ids[] = {
>> -       { .compatible = "pci-x86" },
>> -       { }
>> -};
>> -
>> -U_BOOT_DRIVER(pci_x86_drv) = {
>> -       .name           = "pci_x86",
>> -       .id             = UCLASS_PCI,
>> -       .of_match       = pci_x86_ids,
>> -       .ops            = &pci_x86_ops,
>> -};
>> -
>>  static const struct udevice_id generic_pch_ids[] = {
>>         { .compatible = "intel,pch" },
>>         { }
>
> What's this? Is this required by some board?

Yes, it is needed for ivybridge (when booted from coreboot). The PCH
contains the SPI and LPC peripherals so we need a driver to make those
visible.

>
>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>> index 901bdca..89e8c11 100644
>> --- a/drivers/pci/pci_x86.c
>> +++ b/drivers/pci/pci_x86.c
>> @@ -7,18 +7,21 @@
>>  #include <common.h>
>>  #include <dm.h>
>>  #include <pci.h>
>> +#include <asm/pci.h>
>>
>> -static const struct dm_pci_ops x86_pci_ops = {
>> +static const struct dm_pci_ops pci_x86_ops = {
>> +       .read_config    = pci_x86_read_config,
>> +       .write_config   = pci_x86_write_config,
>>  };
>
> As I mentioned in previous discussion, I still think it's better to
> move the implementation of pci_x86_read_config() and
> pci_x86_write_config() from arch/x86/cpu/pci.c to this file
> (drivers/pci/pci_x86.c). I don't think calling
> arch/x86/cpu/ivybridge/pci.c would be a problem. The
> arch/x86/cpu/pci.c provides generic x86 pci config access methods
> which can be utilized by some variants of x86 pci controller (like
> ivybridge). And I also looked the arch/x86/cpu/ivybridge/pci.c driver
> and just wonder why should we create that ivybridge-specific driver?
> Can we move pci_ivybridge_probe() from that file to
> arch/x86/cpu/ivybridge/bd82x6x.c? This way, we no longer need the
> ivybridge-specific pci driver as x86 pci controller should be generic
> enough.

But pci_ivybridge_probe() calls the code in bd82x6x.c.

With ivybridge we attach all the device init to the PCI probe
function. It should be possible to move more of this to driver model,
but not in this patch.

>
>>
>> -static const struct udevice_id x86_pci_ids[] = {
>> -       { .compatible = "x86,pci" },
>> +static const struct udevice_id pci_x86_ids[] = {
>> +       { .compatible = "pci-x86" },
>>         { }
>>  };
>>
>>  U_BOOT_DRIVER(pci_x86) = {
>>         .name   = "pci_x86",
>>         .id     = UCLASS_PCI,
>> -       .of_match = x86_pci_ids,
>> -       .ops    = &x86_pci_ops,
>> +       .of_match = pci_x86_ids,
>> +       .ops    = &pci_x86_ops,
>>  };
>> --

Regards,
Simon