From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 26 Jan 2015 10:44:36 -0700 Subject: [U-Boot] [PATCH] pci: tegra: Fix port information parsing In-Reply-To: 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> <54C27B98.5050205@wwwdotorg.org> <54C33D1B.2030301@wwwdotorg.org> Message-ID: <54C67D04.1060608@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/24/2015 06:24 AM, Simon Glass wrote: > Hi Stephen, > > On 23 January 2015 at 23:35, Stephen Warren wrote: >> On 01/23/2015 09:37 PM, Bin Meng wrote: >>> Hi Stephen, >>> >>> On Sat, Jan 24, 2015 at 12:49 AM, Stephen Warren 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 >>>>>> 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. >>>> >>> >>> 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.