[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Simon Glass]

Hi Bin,

On 24 June 2015 at 09:15, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 23 June 2015 at 21:59, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Jun 24, 2015 at 11:54 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 23 June 2015 at 21:46, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>>>>>>> ---
>>>>>>>
>>>>>>>  drivers/pci/pci_x86.c | 5 ++++-
>>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>>>>>>> index 901bdca..9f842c3 100644
>>>>>>> --- a/drivers/pci/pci_x86.c
>>>>>>> +++ b/drivers/pci/pci_x86.c
>>>>>>> @@ -7,12 +7,15 @@
>>>>>>>  #include <common.h>
>>>>>>>  #include <dm.h>
>>>>>>>  #include <pci.h>
>>>>>>> +#include <asm/pci.h>
>>>>>>>
>>>>>>>  static const struct dm_pci_ops x86_pci_ops = {
>>>>>>
>>>>>> To keep the consistent naming to match the driver name, can we rename
>>>>>> this to pci_x86_ops?
>>>>>
>>>>> OK
>>>>>
>>>>>>
>>>>>>> +       .read_config    = pci_x86_read_config,
>>>>>>> +       .write_config   = pci_x86_write_config,
>>>>>>
>>>>>> Can we move pci_x86_read_config() and pci_x86_write_config() from
>>>>>> arch/x86/cpu/pci.c to this file to make it a complete driver file?
>>>>>> Also create a new header file pci_x86.h to declare these two so that
>>>>>> it can be used by ivybridge.
>>>>>
>>>>> I can certainly drop the ivybridge duplication. But I don't think it
>>>>> is right to call directly into a driver in drivers/...
>>>>>
>>>>> We should use driver model for this if we want to do it properly. I
>>>>> would like to continue the work to move x86 fully to driver model.
>>>>>
>>>>> In the meantime I think that directly called functions should be in arch/x86.
>>>>>
>>>>
>>>> Sorry I don't get it. I mean moving the implementation of
>>>> pci_x86_read_config() and pci_x86_write_config() to
>>>> drivers/pci/pci_x86.c. Do you have some concern about this?
>>>>
>>>> [snip]
>>>
>>> Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I
>>> don't like the 'call directly into driver' idea. If we could remove
>>> the coreboot case then it would be fine.
>>
>> Why not we drop this coreboot pci driver (arch/x86/cpu/coreboot/pci.c)
>> and use the new one (drivers/pci/pci_x86.c) directly?
>
> Yes let's do that. I can't see any reason not to.

I think the problem is that cpu/ivybridge/pci.c has its own driver and
wants to call the pci_x86_read/write_config() functions. So they
really need to be in a generic area. I was thinking about coreboot
yesterday, but the problem is ivybridge.

Regards,
Simon