Re: [PATCH 15/19 v2] tg3: remove unnecessary read of PCI_CAP_ID_EXP

["Matt Carlson" <mcarlson@broadcom.com>, "Jon Mason" <jdmason@kudzu.us>]

On Mon, Jun 27, 2011 at 03:56:50PM -0700, Jon Mason wrote:
> The PCIE capability offset is saved during PCI bus walking.  Use the
> value from pci_dev instead of checking in the driver and saving it off
> the the driver specific structure.  It will remove an unnecessary search
> in the PCI configuration space if this value is referenced instead of
> reacquiring it.
> 
> v2 of the patch re-adds the PCI_EXPRESS flag and adds comments
> describing why it is necessary.
> 
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
> ---
>  drivers/net/tg3.c |   25 ++++++++++++++-----------
>  drivers/net/tg3.h |    5 +----
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 97cd02d..a555efd 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -2679,11 +2679,11 @@ static int tg3_power_down_prepare(struct tg3 *tp)
>  		u16 lnkctl;
>  
>  		pci_read_config_word(tp->pdev,
> -				     tp->pcie_cap + PCI_EXP_LNKCTL,
> +				     tp->pdev->pcie_cap + PCI_EXP_LNKCTL,

Sorry to be a stickler, but can we convert all occurances of
'tp->pdev->pcie_cap' to pci_pcie_cap(tp->pdev)?  If the PCI layer is
taking control of that variable, the driver shouldn't be accessing it
directly if it can help it.

>  				     &lnkctl);
>  		lnkctl |= PCI_EXP_LNKCTL_CLKREQ_EN;
>  		pci_write_config_word(tp->pdev,
> -				      tp->pcie_cap + PCI_EXP_LNKCTL,
> +				      tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
>  				      lnkctl);
>  	}
>  
> @@ -3485,7 +3485,7 @@ relink:
>  		u16 oldlnkctl, newlnkctl;
>  
>  		pci_read_config_word(tp->pdev,
> -				     tp->pcie_cap + PCI_EXP_LNKCTL,
> +				     tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
>  				     &oldlnkctl);
>  		if (tp->link_config.active_speed == SPEED_100 ||
>  		    tp->link_config.active_speed == SPEED_10)
> @@ -3494,7 +3494,7 @@ relink:
>  			newlnkctl = oldlnkctl | PCI_EXP_LNKCTL_CLKREQ_EN;
>  		if (newlnkctl != oldlnkctl)
>  			pci_write_config_word(tp->pdev,
> -					      tp->pcie_cap + PCI_EXP_LNKCTL,
> +					      tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
>  					      newlnkctl);
>  	}
>  
> @@ -7226,7 +7226,7 @@ static int tg3_chip_reset(struct tg3 *tp)
>  
>  	udelay(120);
>  
> -	if (tg3_flag(tp, PCI_EXPRESS) && tp->pcie_cap) {
> +	if (tg3_flag(tp, PCI_EXPRESS) && pci_pcie_cap(tp->pdev)) {
>  		u16 val16;
>  
>  		if (tp->pci_chip_rev_id == CHIPREV_ID_5750_A0) {
> @@ -7244,7 +7244,7 @@ static int tg3_chip_reset(struct tg3 *tp)
>  
>  		/* Clear the "no snoop" and "relaxed ordering" bits. */
>  		pci_read_config_word(tp->pdev,
> -				     tp->pcie_cap + PCI_EXP_DEVCTL,
> +				     tp->pdev->pcie_cap + PCI_EXP_DEVCTL,
>  				     &val16);
>  		val16 &= ~(PCI_EXP_DEVCTL_RELAX_EN |
>  			   PCI_EXP_DEVCTL_NOSNOOP_EN);
> @@ -7255,14 +7255,14 @@ static int tg3_chip_reset(struct tg3 *tp)
>  		if (!tg3_flag(tp, CPMU_PRESENT))
>  			val16 &= ~PCI_EXP_DEVCTL_PAYLOAD;
>  		pci_write_config_word(tp->pdev,
> -				      tp->pcie_cap + PCI_EXP_DEVCTL,
> +				      tp->pdev->pcie_cap + PCI_EXP_DEVCTL,
>  				      val16);
>  
>  		pcie_set_readrq(tp->pdev, tp->pcie_readrq);
>  
>  		/* Clear error status */
>  		pci_write_config_word(tp->pdev,
> -				      tp->pcie_cap + PCI_EXP_DEVSTA,
> +				      tp->pdev->pcie_cap + PCI_EXP_DEVSTA,
>  				      PCI_EXP_DEVSTA_CED |
>  				      PCI_EXP_DEVSTA_NFED |
>  				      PCI_EXP_DEVSTA_FED |
> @@ -13777,8 +13777,7 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
>  	pci_read_config_dword(tp->pdev, TG3PCI_PCISTATE,
>  			      &pci_state_reg);
>  
> -	tp->pcie_cap = pci_find_capability(tp->pdev, PCI_CAP_ID_EXP);
> -	if (tp->pcie_cap != 0) {
> +	if (pci_is_pcie(tp->pdev)) {
>  		u16 lnkctl;
>  
>  		tg3_flag_set(tp, PCI_EXPRESS);
> @@ -13791,7 +13790,7 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
>  		pcie_set_readrq(tp->pdev, tp->pcie_readrq);
>  
>  		pci_read_config_word(tp->pdev,
> -				     tp->pcie_cap + PCI_EXP_LNKCTL,
> +				     tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
>  				     &lnkctl);
>  		if (lnkctl & PCI_EXP_LNKCTL_CLKREQ_EN) {
>  			if (GET_ASIC_REV(tp->pci_chip_rev_id) ==
> @@ -13808,6 +13807,10 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
>  			tg3_flag_set(tp, L1PLLPD_EN);
>  		}
>  	} else if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5785) {
> +		/* BCM5785 devices are effectively PCIe devices, and should
> +		 * follow PCIe codepaths, but do not have a PCIe capabilities
> +		 * section.
> +		*/
>  		tg3_flag_set(tp, PCI_EXPRESS);
>  	} else if (!tg3_flag(tp, 5705_PLUS) ||
>  		   tg3_flag(tp, 5780_CLASS)) {
> diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
> index bedc3b4..5f250ae 100644
> --- a/drivers/net/tg3.h
> +++ b/drivers/net/tg3.h
> @@ -2857,7 +2857,7 @@ enum TG3_FLAGS {
>  	TG3_FLAG_IS_5788,
>  	TG3_FLAG_MAX_RXPEND_64,
>  	TG3_FLAG_TSO_CAPABLE,
> -	TG3_FLAG_PCI_EXPRESS,
> +	TG3_FLAG_PCI_EXPRESS, /* BCM5785 + pci_is_pcie() */
>  	TG3_FLAG_ASF_NEW_HANDSHAKE,
>  	TG3_FLAG_HW_AUTONEG,
>  	TG3_FLAG_IS_NIC,
> @@ -3022,10 +3022,7 @@ struct tg3 {
>  
>  	int				pm_cap;
>  	int				msi_cap;
> -	union {
>  	int				pcix_cap;
> -	int				pcie_cap;
> -	};
>  	int				pcie_readrq;
>  
>  	struct mii_bus			*mdio_bus;
> -- 
> 1.7.5.4
> 
>