[U-Boot] [PATCH] pci: tegra: Fix port information parsing

[Bin Meng]

Hi Thierry,

On Fri, Jan 23, 2015 at 6:19 PM, Thierry Reding <treding at nvidia.com> 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.
>
> 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.

If the pci device only have two types of BARs, one for memory-mapped
register and the other one for i/o-mapped registers, we are good to
have the pci device driver to detect the BAR and determine which type
of BAR the driver wants to use. However, there are cases that a single
pci device may have multiple BARs mapped for different usage and the
driver does not know which BAR to use. For example, a ns16550 pci
device can have 4 BARs with the first 2 BARs used to map the classic
ns16550 registers to memory or i/o while the 3rd and 4th ones are used
to do some speicial purpose like gpio or dma or whatever the device
vendor wants to. For such case, there are two memory BARs and two i/o
BARs and the driver will be confused.

As for the specific ns16550 driver to support both memory-mapped and
i/o-mapped register access, I see there is a comment block in the
ns16550.c:

        /*
         * As far as we know it doesn't make sense to support selection of
         * these options at run-time, so use the existing CONFIG options.
         */
#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
        outb(value, (ulong)addr);
#elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
        out_le32(addr, value);
#elif defined(CONFIG_SYS_NS16550_MEM32) && defined(CONFIG_SYS_BIG_ENDIAN)
        out_be32(addr, value);
#elif defined(CONFIG_SYS_BIG_ENDIAN)
        writeb(value, addr + (1 << plat->reg_shift) - 1);
#else
        writeb(value, addr);
#endif

Looks that the intention was to have the build-time determination
instead of run-time.

> 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

Regards,
Bin