From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 25 Jan 2016 10:11:44 -0700 Subject: [U-Boot] [PATCH] pci: restore initialization for DM_PCI In-Reply-To: References: <1453419333-3840-1-git-send-email-swarren@wwwdotorg.org> <56A2852B.7050902@wwwdotorg.org> <56A2A39F.9010109@wwwdotorg.org> <56A2A5EE.8080005@wwwdotorg.org> Message-ID: <56A65750.8040101@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 01/22/2016 03:00 PM, Simon Glass wrote: > Hi Stephen, > > On 22 January 2016 at 14:58, Stephen Warren wrote: >> On 01/22/2016 02:50 PM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 22 January 2016 at 14:48, Stephen Warren wrote: >>>> >>>> On 01/22/2016 01:32 PM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Stephen, >>>>> >>>>> On 22 January 2016 at 12:38, Stephen Warren >>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 01/21/2016 09:03 PM, Simon Glass wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 21 January 2016 at 20:53, Bin Meng wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Jan 22, 2016 at 11:36 AM, Simon Glass >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 21 January 2016 at 18:39, Bin Meng wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Hi Stephen, >>>>>>>>>> >>>>>>>>>> On Fri, Jan 22, 2016 at 7:35 AM, Stephen Warren >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> From: Stephen Warren >>>>>>>>>>> >>>>>>>>>>> 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 >>>>>>>>>>> --- >>>>>>>>>>> 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.