* [U-Boot] [PATCH] pci: tegra: Fix port information parsing @ 2015-01-20 17:06 Sjoerd Simons 2015-01-20 19:05 ` Simon Glass 2015-01-21 9:50 ` Thierry Reding 0 siblings, 2 replies; 24+ messages in thread From: Sjoerd Simons @ 2015-01-20 17:06 UTC (permalink / raw) To: u-boot 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@collabora.co.uk> --- drivers/pci/pci_tegra.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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); if (err < 0) { error("failed to parse \"reg\" property"); return err; } - *index = PCI_DEV(bdf) - 1; + *index = PCI_DEV(addr.phys_hi) - 1; return 0; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-20 17:06 [U-Boot] [PATCH] pci: tegra: Fix port information parsing Sjoerd Simons @ 2015-01-20 19:05 ` Simon Glass 2015-01-21 2:37 ` Bin Meng 2015-01-21 8:59 ` Thierry Reding 2015-01-21 9:50 ` Thierry Reding 1 sibling, 2 replies; 24+ messages in thread From: Simon Glass @ 2015-01-20 19:05 UTC (permalink / raw) To: u-boot Hi Sjoerd, On 20 January 2015 at 10:06, Sjoerd Simons <sjoerd.simons@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@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. > > 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); > if (err < 0) { > error("failed to parse \"reg\" property"); > return err; > } > > - *index = PCI_DEV(bdf) - 1; > + *index = PCI_DEV(addr.phys_hi) - 1; > > return 0; > } > -- > 2.1.4 > Regards, Simon ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-20 19:05 ` Simon Glass @ 2015-01-21 2:37 ` Bin Meng 2015-01-21 8:24 ` Thierry Reding 2015-01-21 8:59 ` Thierry Reding 1 sibling, 1 reply; 24+ messages in thread From: Bin Meng @ 2015-01-21 2:37 UTC (permalink / raw) To: u-boot Hi, On Wed, Jan 21, 2015 at 3:05 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Sjoerd, > > On 20 January 2015 at 10:06, Sjoerd Simons > <sjoerd.simons@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@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. >> >> 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. >> if (err < 0) { >> error("failed to parse \"reg\" property"); >> return err; >> } >> >> - *index = PCI_DEV(bdf) - 1; >> + *index = PCI_DEV(addr.phys_hi) - 1; >> >> return 0; >> } Based on this patch, I think Tegra's dts just uses "reg" to encode the device number into fdt_pci_addr.phys_hi. The bus number and function number are not there. Regards, Bin ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 2:37 ` Bin Meng @ 2015-01-21 8:24 ` Thierry Reding 2015-01-21 9:15 ` Bin Meng 2015-01-21 9:19 ` Sjoerd Simons 0 siblings, 2 replies; 24+ messages in thread From: Thierry Reding @ 2015-01-21 8:24 UTC (permalink / raw) To: u-boot 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@chromium.org> wrote: > > Hi Sjoerd, > > > > On 20 January 2015 at 10:06, Sjoerd Simons > > <sjoerd.simons@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@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. > >> 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? > >> if (err < 0) { > >> error("failed to parse \"reg\" property"); > >> return err; > >> } > >> > >> - *index = PCI_DEV(bdf) - 1; > >> + *index = PCI_DEV(addr.phys_hi) - 1; > >> > >> return 0; > >> } > > Based on this patch, I think Tegra's dts just uses "reg" to encode the > device number into fdt_pci_addr.phys_hi. The bus number and function > number are not there. Both the bus and function numbers are 0. These reg properties describe the root ports, which are single-function devices and always on bus 0 by definition. 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/20150121/77240639/attachment.pgp> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 8:24 ` Thierry Reding @ 2015-01-21 9:15 ` Bin Meng 2015-01-21 9:40 ` Thierry Reding 2015-01-21 9:19 ` Sjoerd Simons 1 sibling, 1 reply; 24+ messages in thread From: Bin Meng @ 2015-01-21 9:15 UTC (permalink / raw) To: u-boot Hi Thierry, On Wed, Jan 21, 2015 at 4:24 PM, Thierry Reding <treding@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@chromium.org> wrote: >> > Hi Sjoerd, >> > >> > On 20 January 2015 at 10:06, Sjoerd Simons >> > <sjoerd.simons@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@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. >> >> 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. >> >> if (err < 0) { >> >> error("failed to parse \"reg\" property"); >> >> return err; >> >> } >> >> >> >> - *index = PCI_DEV(bdf) - 1; >> >> + *index = PCI_DEV(addr.phys_hi) - 1; >> >> >> >> return 0; >> >> } >> >> Based on this patch, I think Tegra's dts just uses "reg" to encode the >> device number into fdt_pci_addr.phys_hi. The bus number and function >> number are not there. > > Both the bus and function numbers are 0. These reg properties describe > the root ports, which are single-function devices and always on bus 0 by > definition. > > Thierry Regards, Bin ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 9:15 ` Bin Meng @ 2015-01-21 9:40 ` Thierry Reding 2015-01-21 16:04 ` Bin Meng 0 siblings, 1 reply; 24+ messages in thread From: Thierry Reding @ 2015-01-21 9:40 UTC (permalink / raw) To: u-boot 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@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@chromium.org> wrote: > >> > Hi Sjoerd, > >> > > >> > On 20 January 2015 at 10:06, Sjoerd Simons > >> > <sjoerd.simons@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@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. 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/20150121/33f66dff/attachment.pgp> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 9:40 ` Thierry Reding @ 2015-01-21 16:04 ` Bin Meng 2015-01-23 10:19 ` Thierry Reding 0 siblings, 1 reply; 24+ messages in thread From: Bin Meng @ 2015-01-21 16:04 UTC (permalink / raw) To: u-boot Hi Thierry, On Wed, Jan 21, 2015 at 5:40 PM, Thierry Reding <treding@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@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@chromium.org> wrote: >> >> > Hi Sjoerd, >> >> > >> >> > On 20 January 2015 at 10:06, Sjoerd Simons >> >> > <sjoerd.simons@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@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. Regards, Bin ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 16:04 ` Bin Meng @ 2015-01-23 10:19 ` Thierry Reding 2015-01-23 16:49 ` Stephen Warren 2015-01-24 4:19 ` Bin Meng 0 siblings, 2 replies; 24+ messages in thread From: Thierry Reding @ 2015-01-23 10:19 UTC (permalink / raw) To: u-boot 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@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@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@chromium.org> wrote: > >> >> > Hi Sjoerd, > >> >> > > >> >> > On 20 January 2015 at 10:06, Sjoerd Simons > >> >> > <sjoerd.simons@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@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. 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 -------------- 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/20150123/acabea77/attachment.pgp> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-23 10:19 ` Thierry Reding @ 2015-01-23 16:49 ` Stephen Warren 2015-01-24 4:37 ` Bin Meng 2015-01-24 4:19 ` Bin Meng 1 sibling, 1 reply; 24+ messages in thread From: Stephen Warren @ 2015-01-23 16:49 UTC (permalink / raw) To: u-boot 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@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@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@chromium.org> wrote: >>>>>>> Hi Sjoerd, >>>>>>> >>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>> <sjoerd.simons@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@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. > 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-23 16:49 ` Stephen Warren @ 2015-01-24 4:37 ` Bin Meng 2015-01-24 6:35 ` Stephen Warren 0 siblings, 1 reply; 24+ messages in thread From: Bin Meng @ 2015-01-24 4:37 UTC (permalink / raw) To: u-boot Hi Stephen, On Sat, Jan 24, 2015 at 12:49 AM, Stephen Warren <swarren@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@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@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@chromium.org> >>>>>>> wrote: >>>>>>>> >>>>>>>> Hi Sjoerd, >>>>>>>> >>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>>> <sjoerd.simons@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@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. Regards, Bin ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-24 4:37 ` Bin Meng @ 2015-01-24 6:35 ` Stephen Warren 2015-01-24 13:24 ` Simon Glass 0 siblings, 1 reply; 24+ messages in thread From: Stephen Warren @ 2015-01-24 6:35 UTC (permalink / raw) To: u-boot On 01/23/2015 09:37 PM, Bin Meng wrote: > Hi Stephen, > > On Sat, Jan 24, 2015 at 12:49 AM, Stephen Warren <swarren@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@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@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@chromium.org> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Hi Sjoerd, >>>>>>>>> >>>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>>>> <sjoerd.simons@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@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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-24 6:35 ` Stephen Warren @ 2015-01-24 13:24 ` Simon Glass 2015-01-26 17:44 ` Stephen Warren 0 siblings, 1 reply; 24+ messages in thread From: Simon Glass @ 2015-01-24 13:24 UTC (permalink / raw) To: u-boot Hi Stephen, On 23 January 2015 at 23:35, Stephen Warren <swarren@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@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@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@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@chromium.org> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Hi Sjoerd, >>>>>>>>>> >>>>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>>>>> <sjoerd.simons@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@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. Given that we have device tree, there should be so need for a such a table. We have things like looking up the device class on PCI to avoid tabulating each device that a board might use, but that doesn't tell you about the BAR setup. It seems reasonable to me to use device tree for this sort of thing. It stores the information with the board rather than spread throughout U-Boot. But I think this is a design decision, I'm sure you can argue it either way. Also if PCI had enough information for the driver then probing might be enough. Then we get into the discussion about why you would probe what you already know... Regards, Simon ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-24 13:24 ` Simon Glass @ 2015-01-26 17:44 ` Stephen Warren 2015-01-26 18:18 ` Simon Glass 0 siblings, 1 reply; 24+ messages in thread From: Stephen Warren @ 2015-01-26 17:44 UTC (permalink / raw) To: u-boot On 01/24/2015 06:24 AM, Simon Glass wrote: > Hi Stephen, > > On 23 January 2015 at 23:35, Stephen Warren <swarren@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@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@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@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@chromium.org> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Sjoerd, >>>>>>>>>>> >>>>>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>>>>>> <sjoerd.simons@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@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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-26 17:44 ` Stephen Warren @ 2015-01-26 18:18 ` Simon Glass 0 siblings, 0 replies; 24+ messages in thread From: Simon Glass @ 2015-01-26 18:18 UTC (permalink / raw) To: u-boot Hi Stephen, On 26 January 2015 at 10:44, Stephen Warren <swarren@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@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@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@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@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@chromium.org> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hi Sjoerd, >>>>>>>>>>>> >>>>>>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons >>>>>>>>>>>> <sjoerd.simons@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@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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-23 10:19 ` Thierry Reding 2015-01-23 16:49 ` Stephen Warren @ 2015-01-24 4:19 ` Bin Meng 2015-01-30 12:24 ` Thierry Reding 1 sibling, 1 reply; 24+ messages in thread From: Bin Meng @ 2015-01-24 4:19 UTC (permalink / raw) To: u-boot Hi Thierry, On Fri, Jan 23, 2015 at 6:19 PM, Thierry Reding <treding@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@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@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@chromium.org> wrote: >> >> >> > Hi Sjoerd, >> >> >> > >> >> >> > On 20 January 2015 at 10:06, Sjoerd Simons >> >> >> > <sjoerd.simons@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@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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-24 4:19 ` Bin Meng @ 2015-01-30 12:24 ` Thierry Reding 2015-01-30 16:29 ` Simon Glass 2015-01-31 14:41 ` Bin Meng 0 siblings, 2 replies; 24+ messages in thread From: Thierry Reding @ 2015-01-30 12:24 UTC (permalink / raw) To: u-boot 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@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@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@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@chromium.org> wrote: > >> >> >> > Hi Sjoerd, > >> >> >> > > >> >> >> > On 20 January 2015 at 10:06, Sjoerd Simons > >> >> >> > <sjoerd.simons@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@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> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-30 12:24 ` Thierry Reding @ 2015-01-30 16:29 ` Simon Glass 2015-01-31 14:41 ` Bin Meng 1 sibling, 0 replies; 24+ messages in thread From: Simon Glass @ 2015-01-30 16:29 UTC (permalink / raw) To: u-boot Hi Thierry, On 30 January 2015 at 05:24, Thierry Reding <treding@nvidia.com> wrote: > 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@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@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@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@chromium.org> wrote: >> >> >> >> > Hi Sjoerd, >> >> >> >> > >> >> >> >> > On 20 January 2015 at 10:06, Sjoerd Simons >> >> >> >> > <sjoerd.simons@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@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? We would adjust the driver. Actually I'd like to get rid of the #ifdefs, but as I said in the comment we haven't seen such a platform yet. Regards, Simon ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-30 12:24 ` Thierry Reding 2015-01-30 16:29 ` Simon Glass @ 2015-01-31 14:41 ` Bin Meng 1 sibling, 0 replies; 24+ messages in thread From: Bin Meng @ 2015-01-31 14:41 UTC (permalink / raw) To: u-boot Hi Thierry, On Fri, Jan 30, 2015 at 8:24 PM, Thierry Reding <treding@nvidia.com> wrote: > 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@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@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@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@chromium.org> wrote: >> >> >> >> > Hi Sjoerd, >> >> >> >> > >> >> >> >> > On 20 January 2015 at 10:06, Sjoerd Simons >> >> >> >> > <sjoerd.simons@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@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? > I don't see there is a problem as we can only specify one UART as the U-Boot serial console. Regards, Bin ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 8:24 ` Thierry Reding 2015-01-21 9:15 ` Bin Meng @ 2015-01-21 9:19 ` Sjoerd Simons 1 sibling, 0 replies; 24+ messages in thread From: Sjoerd Simons @ 2015-01-21 9:19 UTC (permalink / raw) To: u-boot On Wed, 2015-01-21 at 09:24 +0100, 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 <sjg@chromium.org> wrote: > > > Hi Sjoerd, > > > > > > On 20 January 2015 at 10:06, Sjoerd Simons > > > <sjoerd.simons@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@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. I don't think that patch makes sense to fix this particular issue. More below > > 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. > > > >> 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); for fdtdec_get_pci_bdf addr is an *in* parameter, not an out parameter. So using it like this makes very little sense and will crash badly if the dt does define a compatibility. Given this is tegra specific and the nvidia,tegra20-pcie.txt[0] device-tree binding from linux defines the reg property as required just checking that should be enough in all cases (as in potentailly also using the compatibility string doesn't really add value) > > > > 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? The intention indeed was to have a "don't care", i guess a potential cleanup might be to have add FDT_PCI_SPACE_ANY to the enum? However in that case fdtdec_get_pci_addr should probably be adapted to check more carefully if the space match, given with the current code specifying _SPACE_CONFIG can give you the address of any space... > > >> if (err < 0) { > > >> error("failed to parse \"reg\" property"); > > >> return err; > > >> } > > >> > > >> - *index = PCI_DEV(bdf) - 1; > > >> + *index = PCI_DEV(addr.phys_hi) - 1; > > >> > > >> return 0; > > >> } > > > > Based on this patch, I think Tegra's dts just uses "reg" to encode the > > device number into fdt_pci_addr.phys_hi. The bus number and function > > number are not there. > > Both the bus and function numbers are 0. These reg properties describe > the root ports, which are single-function devices and always on bus 0 by > definition. > > Thierry 0: http://free-electrons.com/kerneldoc/latest/devicetree/bindings/pci/nvidia,tegra20-pcie.txt -- Sjoerd Simons <sjoerd.simons@collabora.co.uk> Collabora Ltd. -------------- next part -------------- A non-text attachment was scrubbed... Name: smime.p7s Type: application/x-pkcs7-signature Size: 6170 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150121/43c5b44e/attachment.bin> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-20 19:05 ` Simon Glass 2015-01-21 2:37 ` Bin Meng @ 2015-01-21 8:59 ` Thierry Reding 1 sibling, 0 replies; 24+ messages in thread From: Thierry Reding @ 2015-01-21 8:59 UTC (permalink / raw) To: u-boot On Tue, Jan 20, 2015 at 12:05:40PM -0700, Simon Glass wrote: > Hi Sjoerd, > > On 20 January 2015 at 10:06, Sjoerd Simons > <sjoerd.simons@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@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. Only that patch isn't sufficient because tegra_pcie_parse_port_info() then never calls fdtdec_get_pci_addr() so the index returned will be undefined (as in whatever happened to be on the stack - 1). I use the below patch to fix networking on Jetson TK1. It's kind of a mix between both 430815 and this one, so I didn't feel like making it a proper patch because I assume you'll want to fix this in fdtdec and the Tegra bits in a follow-up patch? Thierry -------------- next part -------------- diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c index d344673cc549..0ad959e598c6 100644 --- a/drivers/pci/pci_tegra.c +++ b/drivers/pci/pci_tegra.c @@ -479,6 +479,12 @@ static int tegra_pcie_parse_port_info(const void *fdt, int node, pci_dev_t bdf; int err; + err = fdtdec_get_pci_addr(fdt, node, 0, "reg", &addr); + if (err < 0) { + error("failed to parse \"reg\" property"); + return err; + } + err = fdtdec_get_int(fdt, node, "nvidia,num-lanes", 0); if (err < 0) { error("failed to parse \"nvidia,num-lanes\" property"); diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 487122eebcf6..77e694d6f53f 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -224,13 +224,13 @@ int fdtdec_get_pci_bdf(const void *blob, int node, u16 dt_vendor, dt_device, vendor, device; int ret; + /* extract the bdf from fdt_pci_addr */ + *bdf = addr->phys_hi & 0xffff00; + /* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device); if (ret) - return ret; - - /* extract the bdf from fdt_pci_addr */ - *bdf = addr->phys_hi & 0xffff00; + return 0; /* read vendor id & device id based on bdf */ pci_read_config_word(*bdf, PCI_VENDOR_ID, &vendor); -------------- 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/20150121/12a77553/attachment.pgp> ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-20 17:06 [U-Boot] [PATCH] pci: tegra: Fix port information parsing Sjoerd Simons 2015-01-20 19:05 ` Simon Glass @ 2015-01-21 9:50 ` Thierry Reding 2015-01-21 16:07 ` Bin Meng 1 sibling, 1 reply; 24+ messages in thread From: Thierry Reding @ 2015-01-21 9:50 UTC (permalink / raw) To: u-boot On Tue, Jan 20, 2015 at 06:06:53PM +0100, 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 <sjoerd.simons@collabora.co.uk> > --- > drivers/pci/pci_tegra.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Given the discussion here and in http://patchwork.ozlabs.org/patch/430815/ I agree with Bin that this is a more appropriate fix. The documentation of the fdtdec_get_pci_bdf() function could be improved, in my opinion, to mention how the compatible property is involved. That should clarify that any value in "reg" can be overridden by looking up the allocated bus number after enumeration. So this patch: Tested-by: Thierry Reding <treding@nvidia.com> Acked-by: Thierry Reding <treding@nvidia.com> -------------- 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/20150121/1cb228bc/attachment.pgp> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 9:50 ` Thierry Reding @ 2015-01-21 16:07 ` Bin Meng 2015-01-22 16:37 ` Simon Glass 0 siblings, 1 reply; 24+ messages in thread From: Bin Meng @ 2015-01-21 16:07 UTC (permalink / raw) To: u-boot Hi Thierry, On Wed, Jan 21, 2015 at 5:50 PM, Thierry Reding <treding@nvidia.com> wrote: > On Tue, Jan 20, 2015 at 06:06:53PM +0100, 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 <sjoerd.simons@collabora.co.uk> >> --- >> drivers/pci/pci_tegra.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) > > Given the discussion here and in > > http://patchwork.ozlabs.org/patch/430815/ > > I agree with Bin that this is a more appropriate fix. The documentation > of the fdtdec_get_pci_bdf() function could be improved, in my opinion, > to mention how the compatible property is involved. That should clarify > that any value in "reg" can be overridden by looking up the allocated > bus number after enumeration. I can prepare a patch to improve the doc. > > So this patch: > > Tested-by: Thierry Reding <treding@nvidia.com> > Acked-by: Thierry Reding <treding@nvidia.com> Regards, Bin ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-21 16:07 ` Bin Meng @ 2015-01-22 16:37 ` Simon Glass 2015-01-24 3:33 ` Simon Glass 0 siblings, 1 reply; 24+ messages in thread From: Simon Glass @ 2015-01-22 16:37 UTC (permalink / raw) To: u-boot Hi, On 21 January 2015 at 09:07, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Thierry, > > On Wed, Jan 21, 2015 at 5:50 PM, Thierry Reding <treding@nvidia.com> wrote: >> On Tue, Jan 20, 2015 at 06:06:53PM +0100, 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 <sjoerd.simons@collabora.co.uk> >>> --- >>> drivers/pci/pci_tegra.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> Given the discussion here and in >> >> http://patchwork.ozlabs.org/patch/430815/ >> >> I agree with Bin that this is a more appropriate fix. The documentation >> of the fdtdec_get_pci_bdf() function could be improved, in my opinion, >> to mention how the compatible property is involved. That should clarify >> that any value in "reg" can be overridden by looking up the allocated >> bus number after enumeration. > > I can prepare a patch to improve the doc. > >> >> So this patch: >> >> Tested-by: Thierry Reding <treding@nvidia.com> >> Acked-by: Thierry Reding <treding@nvidia.com> Since this patch seems OK, I'd like to pick it up for the x86 tree (where the breakage happens). Any objections? Regards, Simon ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH] pci: tegra: Fix port information parsing 2015-01-22 16:37 ` Simon Glass @ 2015-01-24 3:33 ` Simon Glass 0 siblings, 0 replies; 24+ messages in thread From: Simon Glass @ 2015-01-24 3:33 UTC (permalink / raw) To: u-boot On 22 January 2015 at 09:37, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 21 January 2015 at 09:07, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Thierry, >> >> On Wed, Jan 21, 2015 at 5:50 PM, Thierry Reding <treding@nvidia.com> wrote: >>> On Tue, Jan 20, 2015 at 06:06:53PM +0100, 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 <sjoerd.simons@collabora.co.uk> >>>> --- >>>> drivers/pci/pci_tegra.c | 5 ++--- >>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> Given the discussion here and in >>> >>> http://patchwork.ozlabs.org/patch/430815/ >>> >>> I agree with Bin that this is a more appropriate fix. The documentation >>> of the fdtdec_get_pci_bdf() function could be improved, in my opinion, >>> to mention how the compatible property is involved. That should clarify >>> that any value in "reg" can be overridden by looking up the allocated >>> bus number after enumeration. >> >> I can prepare a patch to improve the doc. >> >>> >>> So this patch: >>> >>> Tested-by: Thierry Reding <treding@nvidia.com> >>> Acked-by: Thierry Reding <treding@nvidia.com> > > Since this patch seems OK, I'd like to pick it up for the x86 tree > (where the breakage happens). Any objections? Applied to u-boot-x86, thanks! ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-01-31 14:41 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-20 17:06 [U-Boot] [PATCH] pci: tegra: Fix port information parsing Sjoerd Simons 2015-01-20 19:05 ` Simon Glass 2015-01-21 2:37 ` Bin Meng 2015-01-21 8:24 ` Thierry Reding 2015-01-21 9:15 ` Bin Meng 2015-01-21 9:40 ` Thierry Reding 2015-01-21 16:04 ` Bin Meng 2015-01-23 10:19 ` Thierry Reding 2015-01-23 16:49 ` Stephen Warren 2015-01-24 4:37 ` Bin Meng 2015-01-24 6:35 ` Stephen Warren 2015-01-24 13:24 ` Simon Glass 2015-01-26 17:44 ` Stephen Warren 2015-01-26 18:18 ` Simon Glass 2015-01-24 4:19 ` Bin Meng 2015-01-30 12:24 ` Thierry Reding 2015-01-30 16:29 ` Simon Glass 2015-01-31 14:41 ` Bin Meng 2015-01-21 9:19 ` Sjoerd Simons 2015-01-21 8:59 ` Thierry Reding 2015-01-21 9:50 ` Thierry Reding 2015-01-21 16:07 ` Bin Meng 2015-01-22 16:37 ` Simon Glass 2015-01-24 3:33 ` Simon Glass
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox