diff mbox

ARM: fix debug prints relevant to PCI devices

Message ID 1401909447-8434-1-git-send-email-vidyas@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vidya Sagar June 4, 2014, 7:17 p.m. UTC
As per PCIe spec, fast back-to-back transactions feature
is not applicable to PCIe devices. Hence, do not print
that fast back-to-back trasactions are disabled when
there is a PCIe device found on the bus

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 arch/arm/kernel/bios32.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Stephen Warren June 4, 2014, 7:29 p.m. UTC | #1
On 06/04/2014 01:17 PM, Vidya Sagar wrote:
> As per PCIe spec, fast back-to-back transactions feature
> is not applicable to PCIe devices. Hence, do not print
> that fast back-to-back trasactions are disabled when
> there is a PCIe device found on the bus

> @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		u16 status;
>  
> +		if (!has_pcie_dev)
> +			has_pcie_dev = pci_pcie_cap(dev);

This sets the flag if any PCIe device is detected, even if regular PCI
devices are also detected. I assume the two can be mixed on a bus if
there's a bridge (although perhaps that would be separate buses, and
child buses don't get traversed by this function?)

>  	/*
>  	 * Report what we did for this bus
> +	 * (only if the bus doesn't have even one PCIe device)
>  	 */
> -	printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n",
> -		bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis");
> +	if (!has_pcie_dev)
> +		printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n",
> +			bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis");

So, this skips printing the message if any PCIe device was found.

Given that the message is relevant to PCI devices, more than being not
relevant to PCIe devices, perhaps the logic should be inverted, so that
the message is only printed if a PCI/non-PCIe device /is/ found, i.e.:

...
has_pci_device |= !pci_pcie_cap(dev);
...
if (has_pci_device)
	printk(...);
Sergei Shtylyov June 4, 2014, 7:32 p.m. UTC | #2
Hello.

On 06/04/2014 11:17 PM, Vidya Sagar wrote:

> As per PCIe spec, fast back-to-back transactions feature
> is not applicable to PCIe devices. Hence, do not print
> that fast back-to-back trasactions are disabled when
> there is a PCIe device found on the bus

> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>   arch/arm/kernel/bios32.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..95ad3fb 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
[...]
> @@ -354,9 +357,11 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>
>   	/*
>   	 * Report what we did for this bus
> +	 * (only if the bus doesn't have even one PCIe device)
>   	 */
> -	printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n",
> -		bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis");
> +	if (!has_pcie_dev)
> +		printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n",

    Time to change this to pr_info()...

> +			bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis");
>   }
>   EXPORT_SYMBOL(pcibios_fixup_bus);

WBR, Sergei
Jason Gunthorpe June 4, 2014, 9:04 p.m. UTC | #3
On Wed, Jun 04, 2014 at 01:29:14PM -0600, Stephen Warren wrote:
> On 06/04/2014 01:17 PM, Vidya Sagar wrote:
> > As per PCIe spec, fast back-to-back transactions feature
> > is not applicable to PCIe devices. Hence, do not print
> > that fast back-to-back trasactions are disabled when
> > there is a PCIe device found on the bus
> 
> > @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> >  	list_for_each_entry(dev, &bus->devices, bus_list) {
> >  		u16 status;
> >  
> > +		if (!has_pcie_dev)
> > +			has_pcie_dev = pci_pcie_cap(dev);
> 
> This sets the flag if any PCIe device is detected, even if regular PCI
> devices are also detected. I assume the two can be mixed on a bus if
> there's a bridge (although perhaps that would be separate buses, and
> child buses don't get traversed by this function?)

I like the concept, and the logic looks OK as is (though pci_is_pcie
is a better choice). The function is called on a bus number basis, and
it isn't physically possible to mix technology on a single bus number.

Jason
Russell King - ARM Linux June 4, 2014, 11:46 p.m. UTC | #4
On Wed, Jun 04, 2014 at 01:29:14PM -0600, Stephen Warren wrote:
> On 06/04/2014 01:17 PM, Vidya Sagar wrote:
> > As per PCIe spec, fast back-to-back transactions feature
> > is not applicable to PCIe devices. Hence, do not print
> > that fast back-to-back trasactions are disabled when
> > there is a PCIe device found on the bus
> 
> > @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> >  	list_for_each_entry(dev, &bus->devices, bus_list) {
> >  		u16 status;
> >  
> > +		if (!has_pcie_dev)
> > +			has_pcie_dev = pci_pcie_cap(dev);
> 
> This sets the flag if any PCIe device is detected, even if regular PCI
> devices are also detected. I assume the two can be mixed on a bus if
> there's a bridge (although perhaps that would be separate buses, and
> child buses don't get traversed by this function?)

This function gets caller per PCI bus - if there's a bridge between
(which there should be between PCIe and PCI) then they are by definition
separate buses.  (I'm guessing that there aren't transparent bridges
between PCI and PCIe.)
diff mbox

Patch

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c1..95ad3fb 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -290,6 +290,7 @@  void pcibios_fixup_bus(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
 	u16 features = PCI_COMMAND_SERR | PCI_COMMAND_PARITY | PCI_COMMAND_FAST_BACK;
+	int has_pcie_dev = 0;
 
 	/*
 	 * Walk the devices on this bus, working out what we can
@@ -298,6 +299,8 @@  void pcibios_fixup_bus(struct pci_bus *bus)
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		u16 status;
 
+		if (!has_pcie_dev)
+			has_pcie_dev = pci_pcie_cap(dev);
 		pci_read_config_word(dev, PCI_STATUS, &status);
 
 		/*
@@ -354,9 +357,11 @@  void pcibios_fixup_bus(struct pci_bus *bus)
 
 	/*
 	 * Report what we did for this bus
+	 * (only if the bus doesn't have even one PCIe device)
 	 */
-	printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n",
-		bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis");
+	if (!has_pcie_dev)
+		printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n",
+			bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis");
 }
 EXPORT_SYMBOL(pcibios_fixup_bus);