Hi Stephen, On 26 January 2015 at 10:44, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 01/24/2015 06:24 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 23 January 2015 at 23:35, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> >>> On 01/23/2015 09:37 PM, Bin Meng wrote: >>>> >>>> Hi Stephen, >>>> >>>> On Sat, Jan 24, 2015 at 12:49 AM, Stephen Warren <swar...@wwwdotorg.org> >>>> wrote: >>>>> >>>>> On 01/23/2015 03:19 AM, Thierry Reding wrote: >>>>>> >>>>>> >>>>>> On Thu, Jan 22, 2015 at 12:04:06AM +0800, Bin Meng wrote: >>>>>>> >>>>>>> >>>>>>> Hi Thierry, >>>>>>> >>>>>>> On Wed, Jan 21, 2015 at 5:40 PM, Thierry Reding <tred...@nvidia.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Jan 21, 2015 at 05:15:42PM +0800, Bin Meng wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi Thierry, >>>>>>>>> >>>>>>>>> On Wed, Jan 21, 2015 at 4:24 PM, Thierry Reding >>>>>>>>> <tred...@nvidia.com> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Jan 21, 2015 at 10:37:07AM +0800, Bin Meng wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> On Wed, Jan 21, 2015 at 3:05 AM, Simon Glass <s...@chromium.org> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hi Sjoerd, >>>>>>>>>>>> >>>>>>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>>>>>>> <sjoerd.sim...@collabora.co.uk> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> commit a62e84d7b1824a202dd incorrectly changed the tegra pci >>>>>>>>>>>>> code >>>>>>>>>>>>> to the >>>>>>>>>>>>> new fdtdec pci helpers. To get the device index of the root >>>>>>>>>>>>> port, >>>>>>>>>>>>> the >>>>>>>>>>>>> "reg" property should be parsed from the dtb (as was previously >>>>>>>>>>>>> the >>>>>>>>>>>>> case). >>>>>>>>>>>>> >>>>>>>>>>>>> With this patch i can successfully network boot my jetson tk1 >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Sjoerd Simons <sjoerd.sim...@collabora.co.uk> >>>>>>>>>>>>> --- >>>>>>>>>>>>> drivers/pci/pci_tegra.c | 5 ++--- >>>>>>>>>>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Can you also please take a look at this patch? >>>>>>>>>>>> >>>>>>>>>>>> http://patchwork.ozlabs.org/patch/430815/ >>>>>>>>>>>> >>>>>>>>>>>> It tries to support both options. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Although I still don't see how the Tegra's dts is written, I feel >>>>>>>>>>> this >>>>>>>>>>> patch is doing correctly. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> It's in the U-Boot tree, look at arch/arm/dts/tegra124.dtsi for an >>>>>>>>>> example. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Got it. I see: >>>>>>>>> >>>>>>>>> pci@1,0 { >>>>>>>>> device_type = "pci"; >>>>>>>>> assigned-addresses = <0x82000800 0 >>>>>>>>> 0x01000000 >>>>>>>>> 0 0x1000>; >>>>>>>>> reg = <0x000800 0 0 0 0>; >>>>>>>>> status = "disabled"; >>>>>>>>> >>>>>>>>> #address-cells = <3>; >>>>>>>>> #size-cells = <2>; >>>>>>>>> ranges; >>>>>>>>> >>>>>>>>> nvidia,num-lanes = <2>; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> So I would read this 'reg = <0x000800 0 0 0 0>' as this is a >>>>>>>>> downstream port with device number 1 of the root complex. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Correct. Note that these root ports don't appear on the bus using >>>>>>>> the >>>>>>>> regular configuration space accesses, so the definition here is >>>>>>>> arbitrary, though in a way to mirror what PCI would typically look >>>>>>>> like >>>>>>>> (host bridge 00:00.0, root ports 00:01.0..00:0N.0). >>>>>>>> >>>>>>>> The Linux kernel driver (and the U-Boot driver for that matter) rely >>>>>>>> on >>>>>>>> this numbering, though, for some aspects of configuration of the >>>>>>>> root >>>>>>>> ports. >>>>>>>> >>>>>>>>>>>>> diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c >>>>>>>>>>>>> index f9e05ad..67b5fdf 100644 >>>>>>>>>>>>> --- a/drivers/pci/pci_tegra.c >>>>>>>>>>>>> +++ b/drivers/pci/pci_tegra.c >>>>>>>>>>>>> @@ -459,7 +459,6 @@ static int tegra_pcie_parse_port_info(const >>>>>>>>>>>>> void *fdt, int node, >>>>>>>>>>>>> unsigned int *lanes) >>>>>>>>>>>>> { >>>>>>>>>>>>> struct fdt_pci_addr addr; >>>>>>>>>>>>> - pci_dev_t bdf; >>>>>>>>>>>>> int err; >>>>>>>>>>>>> >>>>>>>>>>>>> err = fdtdec_get_int(fdt, node, "nvidia,num-lanes", >>>>>>>>>>>>> 0); >>>>>>>>>>>>> @@ -470,13 +469,13 @@ static int >>>>>>>>>>>>> tegra_pcie_parse_port_info(const >>>>>>>>>>>>> void *fdt, int node, >>>>>>>>>>>>> >>>>>>>>>>>>> *lanes = err; >>>>>>>>>>>>> >>>>>>>>>>>>> - err = fdtdec_get_pci_bdf(fdt, node, &addr, &bdf); >>>>>>>>>>>>> + err = fdtdec_get_pci_addr(fdt, node, 0, "reg", &addr); >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I suggest replace 0 to FDT_PCI_SPACE_CONFIG. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I do like how 0 actually transports the meaning of "don't care" >>>>>>>>>> here. >>>>>>>>>> The reg property encodes only the BDF, whereas the configuration >>>>>>>>>> space >>>>>>>>>> region for the root ports is encoded in the assigned-addresses >>>>>>>>>> property. >>>>>>>>>> >>>>>>>>>> Looking at the fdtdec_get_pci_addr() implementation I notice that >>>>>>>>>> it >>>>>>>>>> uses the type parameter to match on the type of region. Devices >>>>>>>>>> can >>>>>>>>>> have >>>>>>>>>> more than one region of the same type. How is that supposed to >>>>>>>>>> work >>>>>>>>>> with >>>>>>>>>> this function. Perhaps it's nothing we care about for the fdtdec >>>>>>>>>> API >>>>>>>>>> since we don't access those regions anyway from FDT code? >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Ah, yes, some devices may have multiple regions of the same type. >>>>>>>>> Perhaps we need another parameter bar_index for this api? So far >>>>>>>>> this >>>>>>>>> API is not used by FDT codes. It is used by the ns16550 driver >>>>>>>>> where >>>>>>>>> pci ns16550 normally has two bars, one memory and one i/o. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Why not use the BARs directly in the ns16550 driver rather than >>>>>>>> looking >>>>>>>> it up from the device tree? I assume the device will have to be >>>>>>>> enumerated anyway to make it work properly, at which point addresses >>>>>>>> should've been assigned to the memory and I/O BARs. >>>>>>>> >>>>>>> >>>>>>> It is because we cannot predict which bar to look up if we hardcod >>>>>>> that in the generic ns16550 driver. Normally PCI ns16550 registers >>>>>>> can >>>>>>> be memory-mapped or I/O mapped and it could use any of the 6 BARs. >>>>>>> What's more, on x86 for memory-mapped and I/O mapped they use >>>>>>> different instructions to access the registers, and there is one >>>>>>> build >>>>>>> time macro (CONFIG_SYS_NS16550_PORT_MAPPED) to control this. >>>>> >>>>> >>>>> >>>>> Surely the vendor/device ID (or perhaps subvendor/device ID) directly >>>>> imply >>>>> which BAR is used for this purpose? It's really part of the >>>>> specification of >>>>> the interface to HW, so a particular bit of HW shouldn't be randomly >>>>> deciding to use a different BAR register each power-on. >>>>> >>>> >>>> Yes, the vendor/device ID should be unique to identify a device. >>>> However, if we solely rely on this pair to select which BAR to use in >>>> the pci device driver, we may end up with a big table in the driver >>>> codes to map the vendor/device ID to a BAR number. Each time we >>>> support a new device, say pci ns16550, we have to update the driver to >>>> add the vendor/device ID mapping. This will become unmaintainable as >>>> time goes by. The FDT provides us a way to have such configuration >>>> data removed from driver codes and that's why we all are in favor of >>>> FDT. >>>> >>>> The other benefit I could think of to use FDT to specify a BAR is that >>>> we can avoid using the erroneous BAR and ask the driver to use what we >>>> know the funtional BAR. For example, for pci ns16550 if we hardcod the >>>> BAR selection in the ns16550 driver to use the memory-mapped BAR, on >>>> some chipset there are possibilities that only i/o-mapped BAR could >>>> work correctly. We can easily do such thing via FDT but if we move >>>> such logic into the driver, we may have to create vendor/device ID >>>> test logic. >>> >>> >>> In general however, there is no DT. So you'll always have to support a >>> table in the driver. If you support the table at all, you may as well >>> always just use the table to avoid the complexity associated with >>> multiple methods of providing the same data. I don't agree that such a >>> table would be unmaintainable. >> >> >> In ns16550 we are using device tree to store this information and >> there is no table in the driver. > > > So boards that wish to use the ns16550 driver must convert to DT to use it? > That seems like quite an imposition.
If the default settings don't work, and they want to use PCI then I think that's best. If we find that this creates a lot of pain we can change it, but so far it seems pretty sensible. Also this driver is a mess so I'm looking forward to cleaning it up when it can support fewer use cases. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot