From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Fri, 23 Jan 2015 09:49:28 -0700 Subject: [U-Boot] [PATCH] pci: tegra: Fix port information parsing In-Reply-To: <20150123101947.GC11479@ulmo.nvidia.com> References: <1421773613-25138-1-git-send-email-sjoerd.simons@collabora.co.uk> <20150121082447.GC26240@ulmo.nvidia.com> <20150121094031.GC26340@ulmo.nvidia.com> <20150123101947.GC11479@ulmo.nvidia.com> Message-ID: <54C27B98.5050205@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/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 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 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 wrote: >>>>>>> Hi Sjoerd, >>>>>>> >>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>> 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 >>>>>>>> --- >>>>>>>> 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 at 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. > That sounds like pretty arbitrary restrictions. For one you can detect > invalid BARs, so selecting the right one should be easy. Also it seems > like adding support for both I/O and memory BARs in the same binary > should be relatively easy to do and not add a whole lot of code to the > driver. But even if you only want one variant you can still select the > correct port and not rely on the region defined in DTB. > > Like you said, bus number and addresses are assigned at runtime, so if > you rely on DTB you also need code to make sure the DTB matches reality > to prevent breakage. That sounds a lot more complicated than simply > using what's there in the PCI configuration space already. > > Thierry > > > > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >