[U-Boot] [PATCH] pci: restore initialization for DM_PCI

Tue Jan 26 01:57:59 CET 2016

Hi Stephen,

On 25 January 2016 at 10:11, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 01/22/2016 03:00 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 22 January 2016 at 14:58, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> On 01/22/2016 02:50 PM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 22 January 2016 at 14:48, Stephen Warren <swarren at wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 01/22/2016 01:32 PM, Simon Glass wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Stephen,
>>>>>>
>>>>>> On 22 January 2016 at 12:38, Stephen Warren <swarren at wwwdotorg.org>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 01/21/2016 09:03 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Bin,
>>>>>>>>
>>>>>>>> On 21 January 2016 at 20:53, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Fri, Jan 22, 2016 at 11:36 AM, Simon Glass <sjg at chromium.org>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 21 January 2016 at 18:39, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hi Stephen,
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Jan 22, 2016 at 7:35 AM, Stephen Warren
>>>>>>>>>>> <swarren at wwwdotorg.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>>>>>>>>>
>>>>>>>>>>>> PCI controllers should be enumerated at startup so that PCI
>>>>>>>>>>>> devices
>>>>>>>>>>>> such as Ethernet controllers are available at startup. Fix
>>>>>>>>>>>> board_init_r()
>>>>>>>>>>>> not to skip calling pci_init() when CONFIG_DM_PCI is defined,
>>>>>>>>>>>> and
>>>>>>>>>>>> provide
>>>>>>>>>>>> an implementation of pci_init() for the DM case.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> What exact issue are you trying to fix? I posted the same
>>>>>>>>>>> question
>>>>>>>>>>> on
>>>>>>>>>>> Simon's patch [1] before. Does your patch and Simon's fix the
>>>>>>>>>>> same
>>>>>>>>>>> issue?
>>>>>>>>>>>
>>>>>>>>>>> Note I submitted a similar patch [2] last year for x86 only, to
>>>>>>>>>>> explicitly trigger the PCI enueration. But it was not accepted.
>>>>>>>>>>>
>>>>>>>>>>>> Fixes: 96350f729c42 ("dm: tegra: net: Convert tegra boards to
>>>>>>>>>>>> driver
>>>>>>>>>>>> model
>>>>>>>>>>>> for Ethernet")
>>>>>>>>>>>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> I'm not sure if relying on the side-effects of calling
>>>>>>>>>>>> uclass_{first,ext}_device is the correct approach; is there a
>>>>>>>>>>>> more
>>>>>>>>>>>> explicit
>>>>>>>>>>>> way to probe all PCI controllers?
>>>>>>>>>>>>
>>>>>>>>>>>> Arguably, perhaps we should introduce a "pci start" command
>>>>>>>>>>>> instead
>>>>>>>>>>>> of
>>>>>>>>>>>> this change to be consistent with e.g. USB. However, that would
>>>>>>>>>>>> be
>>>>>>>>>>>> a
>>>>>>>>>>>> regression relative to earlier versions of U-Boot.
>>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [1] http://patchwork.ozlabs.org/patch/569323/
>>>>>>>>>>> [2] http://patchwork.ozlabs.org/patch/500246/
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Bin
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This does go against the driver-model philosophy of lazy init. I
>>>>>>>>>> wonder if we should add this patch with a Kconfig option to enable
>>>>>>>>>> it?
>>>>>>>>>> Then it can be enabled only for boards that need it.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I suspect the issue is somewhere else. On Intel Galileo with a PCI
>>>>>>>>> ethernet, it works fine without such explicit pci init. Which PCI
>>>>>>>>> ethernet driver does not work on Tegra?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It could be because that board probes PCI to get its serial to work.
>>>>>>>>
>>>>>>>> This could be fixed on Tegra by adding an Ethernet node to the
>>>>>>>> device
>>>>>>>> tree to cause it to be probed. But I don't think that should be a
>>>>>>>> requirement.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Since PCI devices are automatically probed via standard bus
>>>>>>> protocols,
>>>>>>> I
>>>>>>> believe we shouldn't have to add them to the DT.
>>>>>>>
>>>>>>> However, I could accept that we should add the PCI controller to DT
>>>>>>> in
>>>>>>> order for the controller to be probed, and when that happens, the bus
>>>>>>> should
>>>>>>> be enumerated thus causing all the Ethernet devices to be probed.
>>>>>>> However,
>>>>>>> the PCI controller is already in DT, and this process isn't being
>>>>>>> kicked
>>>>>>> off, so if that's the way it's supposed to work, there's a bug there.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> So what do you think about a Kconfig option that tells U-Boot that PCI
>>>>>> must be probed after relocation?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I'm puzzled why anyone would turn off that option.
>>>>>
>>>>> If PCI is to be used at all, it needs to be probed. The only way I'm
>>>>> aware
>>>>> of that it can be probed today is by accidental side-effect of some
>>>>> board
>>>>> forcibly probing some device that happens to be on a PCI bus (i.e.
>>>>> Bin's
>>>>> case). In other case, PCI doesn't get automatically probed at boot, and
>>>>> I
>>>>> don't see any command that allows it to be probed manually. Don't we
>>>>> need
>>>>> this patch (or your patch linked above) in order to make PCI usable at
>>>>> all?
>>>>> If so, why would anyone turn this off?
>>>>
>>>>
>>>>
>>>> We try to probe devices only when used. E.g. on beaver, if you are not
>>>> net-booting, you actually don't need PCI.
>>>
>>>
>>>
>>> Are you saying that the first network command I execute in U-Boot should
>>> probe Ethernet device and hence PCI devices right now in the current
>>> U-Boot
>>> code?
>>>
>>> That doesn't work right now. If it did work, that would probably be OK.
>>
>>
>> Two options:
>> - Add a 'pci start' command to your script
>> - Enable the PCI auto-probe Kconfig option
>>
>> Boards can then choose which they prefer.
>
>
> I see there's already a "pci enum" command that can be enabled:
>
> #ifdef CONFIG_SYS_LONGHELP
> static char pci_help_text[] =
>         "[bus] [long]\n"
>         "    - short or long list of PCI devices on bus 'bus'\n"
> #ifdef CONFIG_CMD_PCI_ENUM
>         "pci enum\n"
>         "    - re-enumerate PCI buses\n"
> #endif
>
> I'm tempted to re-enable that for DM, by making the implementation of
> pci_init() exist even with CONFIG_DM_PCI, then enhance
> config_distro_bootcmd.h to have it run "pci enum" any time before it might
> touch a PCI device, similar to the existing USB and SCSI enumeration
> support.
>
> The only wart is that the command help text currently says "*re-*enumerate
> PCI buses" rather than just "enumerate PCI buses", which implies the
> semantics would always cause an enumeration even on the second/...
> execution, whereas it'd be simpler to have it work like "usb start", i.e.
> perform enumeration the first time but be a no-op any second/... time. Am I
> reading too much into the help text? If not, should I just rename the
> command to "pci start" for the DM case? I'm not sure if PCI hotplug is
> something we'll need to deal with; that'd be the only time the different
> semantics were particular useful.

I'd suggest making 'pci enum' do the enumeration in the driver model
case, and it will only do this once. You can update the help. I don't
believe we found anyone interested in the current functionality so it
can die when things move to DM.

On the other hand, if we do want to implement it, it would involve
device_remove() and device_unbind() calls on the PCI controllers,
which would recursively cause them all to be deleted. This is
problematic since we'd then need to re-bind the controllers so that
that device tree PCI devices are bound in pci_uclass_post_bind(). So
IMO it is more complexity than it is worth.

The message in the code is ready for your action:

printf("This command is not yet supported with driver model\n");

Regards,
Simon