[U-Boot] [PATCH] pci: tegra: Fix port information parsing
Thierry Reding
treding at nvidia.com
Fri Jan 30 13:24:26 CET 2015
On Sat, Jan 24, 2015 at 12:19:56PM +0800, Bin Meng wrote:
> 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.
So what if you have two UARTs, one using memory-mapped registers and
another using I/O-mapped registers?
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150130/7d472480/attachment.sig>
More information about the U-Boot
mailing list