On 1/17/22 16:38, Pali Rohár wrote:

'pci regions' currently prints only region information from bus 0 which belongs to controller 0. Parser for 'pci regions' cmdline currently ignores any additional arguments and so U-Boot always uses bus 0.

Regions are stored in controller (not on the bus) and therefore to retrieve controller from the bus, it is needed to call pci_get_controller() which returns root bus. Because bus 0 is root bus, current code worked fine for controller 0.

Extend cmdline parser for 'pci regions' to allows specifying bus number, extend pci_show_regions() code to accept also non-zero bus number and print bus ranges for which is regions configuration assigned.

Signed-off-by: Pali Rohár pali@kernel.org

Nitpicking comment below. But still:

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

cmd/pci.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/cmd/pci.c b/cmd/pci.c index 3b1863f139c9..53edf0d90010 100644 --- a/cmd/pci.c +++ b/cmd/pci.c @@ -443,7 +443,7 @@ static const struct pci_flag_info {

static void pci_show_regions(struct udevice *bus) {

• struct pci_controller *hose = dev_get_uclass_priv(bus);

• struct pci_controller *hose = dev_get_uclass_priv(pci_get_controller(bus)); const struct pci_region *reg; int i, j;

@@ -452,6 +452,7 @@ static void pci_show_regions(struct udevice *bus) return; }

• printf("Buses %02x-%02x\n", hose->first_busno, hose->last_busno); printf("# %-18s %-18s %-18s %s\n", "Bus start", "Phys start", "Size", "Flags"); for (i = 0, reg = hose->regions; i < hose->region_count; i++, reg++) {

@@ -520,8 +521,9 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) value = 0; argc--; }

• 	if (argc > 1)
  

• 		busnum = hextoul(argv[1], NULL);
  

• 	if (argc > 2 || (argc > 1 && cmd != 'r' && argv[1][0] != 's')) {
  

• 		busnum = hextoul(argv[argc - 1], NULL);
  

• 	}
  

Nitpicking: Single line statements without parentheses is preferred.

}
ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, &bus);
if (ret) {

@@ -586,7 +588,7 @@ static char pci_help_text[] = " - show header of PCI device 'bus.device.function'\n" "pci bar b.d.f\n" " - show BARs base and size for device b.d.f'\n"

• "pci regions\n"

• "pci regions [bus]\n" " - show PCI regions\n" "pci display[.b, .w, .l] b.d.f [address] [# of objects]\n" " - display PCI configuration space (CFG)\n"

Viele Grüße, Stefan Roese

On 1/25/22 17:50, Pali Rohár wrote:

On Tuesday 25 January 2022 16:53:05 Stefan Roese wrote:

...

On 1/17/22 16:38, Pali Rohár wrote:

...

'pci regions' currently prints only region information from bus 0 which belongs to controller 0. Parser for 'pci regions' cmdline currently ignores any additional arguments and so U-Boot always uses bus 0.

Regions are stored in controller (not on the bus) and therefore to retrieve controller from the bus, it is needed to call pci_get_controller() which returns root bus. Because bus 0 is root bus, current code worked fine for controller 0.

Extend cmdline parser for 'pci regions' to allows specifying bus number, extend pci_show_regions() code to accept also non-zero bus number and print bus ranges for which is regions configuration assigned.

Signed-off-by: Pali Rohár pali@kernel.org

Nitpicking comment below. But still:

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

...

---

cmd/pci.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/cmd/pci.c b/cmd/pci.c index 3b1863f139c9..53edf0d90010 100644 --- a/cmd/pci.c +++ b/cmd/pci.c @@ -443,7 +443,7 @@ static const struct pci_flag_info { static void pci_show_regions(struct udevice *bus) {

• struct pci_controller *hose = dev_get_uclass_priv(bus);

• struct pci_controller *hose = dev_get_uclass_priv(pci_get_controller(bus)); const struct pci_region *reg; int i, j;

@@ -452,6 +452,7 @@ static void pci_show_regions(struct udevice *bus) return; }

• printf("Buses %02x-%02x\n", hose->first_busno, hose->last_busno); printf("# %-18s %-18s %-18s %s\n", "Bus start", "Phys start", "Size", "Flags"); for (i = 0, reg = hose->regions; i < hose->region_count; i++, reg++) {

@@ -520,8 +521,9 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) value = 0; argc--; }

• 	if (argc > 1)
  

• 		busnum = hextoul(argv[1], NULL);
  

• 	if (argc > 2 || (argc > 1 && cmd != 'r' && argv[1][0] != 's')) {
  

• 		busnum = hextoul(argv[argc - 1], NULL);
  

• 	}
  

Nitpicking: Single line statements without parentheses is preferred.

In next patches I'm adding more statements into this branch and I did not wanted to make next patches too noisy.

Thanks for the explanation. Fine with me. RB tag is already sent.

Thanks, Stefan

On 1/17/22 16:38, Pali Rohár wrote:

Allow to call 'pci' and 'pci regions' commands with bus option '*' which cause pci to process all buses.

PCIe is point-to-point HW and so on each bus is maximally one physical device. Therefore for PCIe it is common to have multiple buses.

This change allows to easily print all available PCIe devices in system.

Make '*' as default option when no bus argument is specified.

Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

cmd/pci.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/cmd/pci.c b/cmd/pci.c index 8d2c0c4b43dd..4415feb2225b 100644 --- a/cmd/pci.c +++ b/cmd/pci.c @@ -256,10 +256,8 @@ static void pci_header_show(struct udevice *dev) } }

-static void pciinfo_header(int busnum, bool short_listing) +static void pciinfo_header(bool short_listing) {

• printf("Scanning PCI devices on bus %d\n", busnum);
• if (short_listing) { printf("BusDevFun VendorId DeviceId Device Class Sub-Class\n"); printf("_____________________________________________________________\n");

@@ -288,11 +286,15 @@ static void pci_header_show_brief(struct udevice *dev) pci_class_str(class), subclass); }

-static void pciinfo(struct udevice *bus, bool short_listing) +static void pciinfo(struct udevice *bus, bool short_listing, bool multi) { struct udevice *dev;

• pciinfo_header(dev_seq(bus), short_listing);

• if (!multi)

• printf("Scanning PCI devices on bus %d\n", dev_seq(bus));
  

• if (!multi || dev_seq(bus) == 0)

• pciinfo_header(short_listing);
  
  

  for (device_find_first_child(bus, &dev); dev;

@@ -483,10 +485,11 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) ulong addr = 0, value = 0, cmd_size = 0; enum pci_size_t size = PCI_SIZE_32; struct udevice *dev, *bus;

• int busnum = 0;

• int busnum = -1; pci_dev_t bdf = 0; char cmd = 's'; int ret = 0;

• char *endp;

  if (argc > 1) cmd = argv[1][0];

@@ -522,7 +525,11 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) argc--; } if (argc > 2 || (argc > 1 && cmd != 'r' && argv[1][0] != 's')) {

• 		busnum = hextoul(argv[argc - 1], NULL);
  

• 		if (argv[argc - 1][0] != '*') {
  

• 			busnum = hextoul(argv[argc - 1], &endp);
  

• 			if (*endp)
  

• 				goto usage;
  

• 		}
  	argc--;
  }
  if (cmd == 'r' && argc > 2)
  

@@ -530,6 +537,24 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) else if (cmd != 'r' && (argc > 2 || (argc == 2 && argv[1][0] != 's'))) goto usage; }

• if (busnum == -1) {
  

• 	if (cmd != 'r') {
  

• 		for (busnum = 0;
  

• 		     uclass_get_device_by_seq(UCLASS_PCI, busnum, &bus) == 0;
  

• 		     busnum++)
  

• 			pciinfo(bus, value, true);
  

• 	} else {
  

• 		for (busnum = 0;
  

• 		     uclass_get_device_by_seq(UCLASS_PCI, busnum, &bus) == 0;
  

• 		     busnum++) {
  

• 			/* Regions are controller specific so skip non-root buses */
  

• 			if (device_is_on_pci_bus(bus))
  

• 				continue;
  

• 			pci_show_regions(bus);
  

• 		}
  

• 	}
  

• 	return 0;
  

• }
  

  ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, &bus); if (ret) { printf("No such bus\n");

@@ -538,7 +563,7 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (cmd == 'r') pci_show_regions(bus); else

• 	pciinfo(bus, value);
  

• 	pciinfo(bus, value, false);
  

  return 0; }

@@ -585,7 +610,7 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])

#ifdef CONFIG_SYS_LONGHELP static char pci_help_text[] =

• "[bus] [long]\n"

• "[bus|*] [long]\n" " - short or long list of PCI devices on bus 'bus'\n" "pci enum\n" " - Enumerate PCI buses\n"

@@ -593,7 +618,7 @@ static char pci_help_text[] = " - show header of PCI device 'bus.device.function'\n" "pci bar b.d.f\n" " - show BARs base and size for device b.d.f'\n"

• "pci regions [bus]\n"

• "pci regions [bus|*]\n" " - show PCI regions\n" "pci display[.b, .w, .l] b.d.f [address] [# of objects]\n" " - display PCI configuration space (CFG)\n"

Viele Grüße, Stefan Roese