[PATCH 0/4] pci: Extend 'pci' and 'pci regions' commands

[PATCH 0/4] pci: Extend 'pci' and 'pci regions' commands

[Pali Rohár]

Allow to call 'pci regions' with non-zero bus number and allow to call
'pci' and 'pci regions' commands with bus number '*' which process all
buses. This would be also new default action if no bus is specified.

Pali Rohár (4):
  pci: Fix setting controller's last_busno
  pci: Extend 'pci regions' command with bus number
  pci: Add checks for valid cmdline arguments
  pci: Extend 'pci' command with bus option '*'

 cmd/pci.c                | 56 +++++++++++++++++++++++++++++++---------
 drivers/pci/pci-uclass.c |  2 ++
 2 files changed, 46 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH 1/4] pci: Fix setting controller's last_busno

[Pali Rohár]

Initially it is set to dev_seq but update to the last bus number is
missing. Fix it.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/pci-uclass.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 8c4d75bdb685..374b22601818 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -562,6 +562,8 @@ int pci_auto_config_devices(struct udevice *bus)
 		if (pplat->class == (PCI_CLASS_DISPLAY_VGA << 8))
 			set_vga_bridge_bits(dev);
 	}
+	if (hose->last_busno < sub_bus)
+		hose->last_busno = sub_bus;
 	debug("%s: done\n", __func__);
 
 	return log_msg_ret("sub", sub_bus);
-- 
2.20.1

[PATCH 2/4] pci: Extend 'pci regions' command with bus number

[Pali Rohár]

'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>
---
 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);
+			}
 		}
 		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"
-- 
2.20.1

[PATCH 3/4] pci: Add checks for valid cmdline arguments

[Pali Rohár]

Currently pci command ignores invalid cmdline arguments and do something.
Add checks that all passed arguments were processed.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 cmd/pci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cmd/pci.c b/cmd/pci.c
index 53edf0d90010..8d2c0c4b43dd 100644
--- a/cmd/pci.c
+++ b/cmd/pci.c
@@ -523,7 +523,12 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 			}
 			if (argc > 2 || (argc > 1 && cmd != 'r' && argv[1][0] != 's')) {
 				busnum = hextoul(argv[argc - 1], NULL);
+				argc--;
 			}
+			if (cmd == 'r' && argc > 2)
+				goto usage;
+			else if (cmd != 'r' && (argc > 2 || (argc == 2 && argv[1][0] != 's')))
+				goto usage;
 		}
 		ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, &bus);
 		if (ret) {
-- 
2.20.1

[PATCH 4/4] pci: Extend 'pci' command with bus option '*'

[Pali Rohár]

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>
---
 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"
-- 
2.20.1

Re: [PATCH 1/4] pci: Fix setting controller's last_busno

[Stefan Roese]

On 1/17/22 16:38, Pali Rohár wrote:
> Initially it is set to dev_seq but update to the last bus number is
> missing. Fix it.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-uclass.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 8c4d75bdb685..374b22601818 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -562,6 +562,8 @@ int pci_auto_config_devices(struct udevice *bus)
>   		if (pplat->class == (PCI_CLASS_DISPLAY_VGA << 8))
>   			set_vga_bridge_bits(dev);
>   	}
> +	if (hose->last_busno < sub_bus)
> +		hose->last_busno = sub_bus;
>   	debug("%s: done\n", __func__);
>   
>   	return log_msg_ret("sub", sub_bus);

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 2/4] pci: Extend 'pci regions' command with bus number

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 3/4] pci: Add checks for valid cmdline arguments

[Stefan Roese]

On 1/17/22 16:38, Pali Rohár wrote:
> Currently pci command ignores invalid cmdline arguments and do something.
> Add checks that all passed arguments were processed.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   cmd/pci.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/cmd/pci.c b/cmd/pci.c
> index 53edf0d90010..8d2c0c4b43dd 100644
> --- a/cmd/pci.c
> +++ b/cmd/pci.c
> @@ -523,7 +523,12 @@ static int do_pci(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   			}
>   			if (argc > 2 || (argc > 1 && cmd != 'r' && argv[1][0] != 's')) {
>   				busnum = hextoul(argv[argc - 1], NULL);
> +				argc--;
>   			}
> +			if (cmd == 'r' && argc > 2)
> +				goto usage;
> +			else if (cmd != 'r' && (argc > 2 || (argc == 2 && argv[1][0] != 's')))
> +				goto usage;
>   		}
>   		ret = uclass_get_device_by_seq(UCLASS_PCI, busnum, &bus);
>   		if (ret) {

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 4/4] pci: Extend 'pci' command with bus option '*'

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 2/4] pci: Extend 'pci regions' command with bus number

[Pali Rohár]

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.

> >   		}
> >   		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
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 2/4] pci: Extend 'pci regions' command with bus number

[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

Re: [PATCH 1/4] pci: Fix setting controller's last_busno

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

On Mon, Jan 17, 2022 at 04:38:37PM +0100, Pali Rohár wrote:

> Initially it is set to dev_seq but update to the last bus number is
> missing. Fix it.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/4] pci: Extend 'pci regions' command with bus number

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

On Mon, Jan 17, 2022 at 04:38:38PM +0100, 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>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/4] pci: Add checks for valid cmdline arguments

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

On Mon, Jan 17, 2022 at 04:38:39PM +0100, Pali Rohár wrote:

> Currently pci command ignores invalid cmdline arguments and do something.
> Add checks that all passed arguments were processed.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 4/4] pci: Extend 'pci' command with bus option '*'

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Mon, Jan 17, 2022 at 04:38:40PM +0100, 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>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]