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

[Stephen Warren]

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.