[U-Boot] [PATCH] pci: tegra: Fix port information parsing
Simon Glass
sjg at chromium.org
Mon Jan 26 19:18:46 CET 2015
Hi Stephen,
On 26 January 2015 at 10:44, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 01/24/2015 06:24 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 23 January 2015 at 23:35, Stephen Warren <swarren at 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 <swarren at 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 <treding at 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
>>>>>>>>> <treding at 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 <sjg at chromium.org>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Sjoerd,
>>>>>>>>>>>>
>>>>>>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons
>>>>>>>>>>>> <sjoerd.simons at 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.simons at 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 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.
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
More information about the U-Boot
mailing list