[RFC 1/3] ksz884x: Use pci_set_power_state() for setting PM states

[RFC 1/3] ksz884x: Use pci_set_power_state() for setting PM states

[Jon Mason]

Use the existing infrastructure of pci_set_power_state() instead of
setting the relevant bits via PCI config read/write in the driver.
Also, use pci_pme_active() to set the PCI_PM_CTRL_PME_ENABLE bit in PCI
PM control register.  pci_set_power_state() and pci_pme_active() perform
the same operations as the driver did before, so there should be no
functional change.  That being said, this has not been tested.

Signed-off-by: Jon Mason <jdmason@kudzu.us>
Cc: Tristram Ha <Tristram.Ha@micrel.com>
---
 drivers/net/ethernet/micrel/ksz884x.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ksz884x.c b/drivers/net/ethernet/micrel/ksz884x.c
index ce84dc2..d658231 100644
--- a/drivers/net/ethernet/micrel/ksz884x.c
+++ b/drivers/net/ethernet/micrel/ksz884x.c
@@ -3524,17 +3524,17 @@ static void hw_cfg_wol_pme(struct ksz_hw *hw, int set)
 {
 	struct dev_info *hw_priv = container_of(hw, struct dev_info, hw);
 	struct pci_dev *pdev = hw_priv->pdev;
-	u16 data;
 
 	if (!pdev->pm_cap)
 		return;
-	pci_read_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, &data);
-	data &= ~PCI_PM_CTRL_STATE_MASK;
-	if (set)
-		data |= PCI_PM_CTRL_PME_ENABLE | PCI_D3hot;
-	else
-		data &= ~PCI_PM_CTRL_PME_ENABLE;
-	pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, data);
+
+	if (set) {
+		pci_pme_active(pdev, true);
+		pci_set_power_state(pdev, PCI_D3hot);
+	} else {
+		pci_pme_active(pdev, false);
+		pci_set_power_state(pdev, PCI_D0);
+	}
 }
 
 /**
-- 
1.8.3.2

[RFC 2/3] tg3: Use pci_set_power_state() for setting PM states

[Jon Mason]

Use the existing infrastructure of pci_set_power_state() instead of
setting the relevant bits via PCI config read/write in the driver.
Also, use pci_pme_active() to set the PCI_PM_CTRL_PME_ENABLE bit in PCI
PM control register.  pci_set_power_state() and pci_pme_active() perform
the same operations as the driver did before, so there should be no
functional change.  That being said, this has not been tested.

Signed-off-by: Jon Mason <jdmason@kudzu.us>
Cc: Nithin Nayak Sujir <nsujir@broadcom.com>
Cc: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 3167ed6..36c3fd9 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -16362,22 +16362,14 @@ static int tg3_get_invariants(struct tg3 *tp, const struct pci_device_id *ent)
 		 * for all chip writes not to mailbox registers.
 		 */
 		if (tg3_flag(tp, PCIX_MODE)) {
-			u32 pm_reg;
-
 			tg3_flag_set(tp, PCIX_TARGET_HWBUG);
 
 			/* The chip can have it's power management PCI config
 			 * space registers clobbered due to this bug.
 			 * So explicitly force the chip into D0 here.
 			 */
-			pci_read_config_dword(tp->pdev,
-					      tp->pdev->pm_cap + PCI_PM_CTRL,
-					      &pm_reg);
-			pm_reg &= ~PCI_PM_CTRL_STATE_MASK;
-			pm_reg |= PCI_PM_CTRL_PME_ENABLE | 0 /* D0 */;
-			pci_write_config_dword(tp->pdev,
-					       tp->pdev->pm_cap + PCI_PM_CTRL,
-					       pm_reg);
+			pci_set_power_state(tp->pdev, PCI_D0);
+			pci_pme_active(tp->pdev, true);
 
 			/* Also, force SERR#/PERR# in PCI command. */
 			pci_read_config_word(tp->pdev, PCI_COMMAND, &pci_cmd);
-- 
1.8.3.2

[RFC 3/3] bnx2x: Use pci_set_power_state() for setting PM states

[Jon Mason]

Use the existing infrastructure of pci_set_power_state() instead of
setting the relevant bits via PCI config read/write in the driver.
Also, use the d3_delay field to specify how long to msleep after putting
the device in the appropriate power management state.  Finally, use
pci_pme_active() to set the PCI_PM_CTRL_PME_ENABLE bit in PCI PM control
register.  pci_set_power_state() and pci_pme_active() perform the same
operations as the driver did before, so there should be no functional
change.  That being said, this has not been tested.

Signed-off-by: Jon Mason <jdmason@kudzu.us>
Cc: Ariel Elior <ariele@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 21 ++++-----------------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  3 +++
 2 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 66c0df7..3fee183 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -3054,25 +3054,16 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)
 
 int bnx2x_set_power_state(struct bnx2x *bp, pci_power_t state)
 {
-	u16 pmcsr;
-
 	/* If there is no power capability, silently succeed */
 	if (!bp->pdev->pm_cap) {
 		BNX2X_DEV_INFO("No power capability. Breaking.\n");
 		return 0;
 	}
 
-	pci_read_config_word(bp->pdev, bp->pdev->pm_cap + PCI_PM_CTRL, &pmcsr);
-
 	switch (state) {
 	case PCI_D0:
-		pci_write_config_word(bp->pdev, bp->pdev->pm_cap + PCI_PM_CTRL,
-				      ((pmcsr & ~PCI_PM_CTRL_STATE_MASK) |
-				       PCI_PM_CTRL_PME_STATUS));
-
-		if (pmcsr & PCI_PM_CTRL_STATE_MASK)
-			/* delay required during transition out of D3hot */
-			msleep(20);
+		pci_set_power_state(bp->pdev, PCI_D0);
+		pci_pme_active(bp->pdev, false);
 		break;
 
 	case PCI_D3hot:
@@ -3084,14 +3075,10 @@ int bnx2x_set_power_state(struct bnx2x *bp, pci_power_t state)
 		if (CHIP_REV_IS_SLOW(bp))
 			return 0;
 
-		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
-		pmcsr |= 3;
-
 		if (bp->wol)
-			pmcsr |= PCI_PM_CTRL_PME_ENABLE;
+			pci_pme_active(bp->pdev, true);
 
-		pci_write_config_word(bp->pdev, bp->pdev->pm_cap + PCI_PM_CTRL,
-				      pmcsr);
+		pci_set_power_state(bp->pdev, PCI_D3hot);
 
 		/* No more memory access after this point until
 		* device is brought back to D0.
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 7d43822..19f4b0a 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12898,6 +12898,9 @@ static int bnx2x_init_one(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, dev);
 
+	/* delay required during transition out of D3hot */
+	pdev->d3_delay = 20;
+
 	rc = bnx2x_init_dev(bp, pdev, dev, ent->driver_data);
 	if (rc < 0) {
 		free_netdev(dev);
-- 
1.8.3.2

Re: [RFC 2/3] tg3: Use pci_set_power_state() for setting PM states

[Michael Chan]

On Mon, 2014-02-24 at 15:34 -0700, Jon Mason wrote: 
> Use the existing infrastructure of pci_set_power_state() instead of
> setting the relevant bits via PCI config read/write in the driver.
> Also, use pci_pme_active() to set the PCI_PM_CTRL_PME_ENABLE bit in PCI
> PM control register.  pci_set_power_state() and pci_pme_active() perform
> the same operations as the driver did before, so there should be no
> functional change.  That being said, this has not been tested.
> 
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
> Cc: Nithin Nayak Sujir <nsujir@broadcom.com>
> Cc: Michael Chan <mchan@broadcom.com>

I've reviewed the code and this patch should be ok.  The original intent
of the code was to force the power state to D0 in case it was clobbered
by an MMIO write not intended for that register due to a hardware bug.
If it was clobbered like that, pci_set_power_state() may not set it to
D0 if it thinks that it is already in D0.  There is no MMIO write up
until this point so it is ok to call pci_set_power_state().

Acked-by: Michael Chan <mchan@broadcom.com>

> ---
>  drivers/net/ethernet/broadcom/tg3.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 3167ed6..36c3fd9 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -16362,22 +16362,14 @@ static int tg3_get_invariants(struct tg3 *tp, const struct pci_device_id *ent)
>  		 * for all chip writes not to mailbox registers.
>  		 */
>  		if (tg3_flag(tp, PCIX_MODE)) {
> -			u32 pm_reg;
> -
>  			tg3_flag_set(tp, PCIX_TARGET_HWBUG);
>  
>  			/* The chip can have it's power management PCI config
>  			 * space registers clobbered due to this bug.
>  			 * So explicitly force the chip into D0 here.
>  			 */
> -			pci_read_config_dword(tp->pdev,
> -					      tp->pdev->pm_cap + PCI_PM_CTRL,
> -					      &pm_reg);
> -			pm_reg &= ~PCI_PM_CTRL_STATE_MASK;
> -			pm_reg |= PCI_PM_CTRL_PME_ENABLE | 0 /* D0 */;
> -			pci_write_config_dword(tp->pdev,
> -					       tp->pdev->pm_cap + PCI_PM_CTRL,
> -					       pm_reg);
> +			pci_set_power_state(tp->pdev, PCI_D0);
> +			pci_pme_active(tp->pdev, true);
>  
>  			/* Also, force SERR#/PERR# in PCI command. */
>  			pci_read_config_word(tp->pdev, PCI_COMMAND, &pci_cmd);

RE: [RFC 3/3] bnx2x: Use pci_set_power_state() for setting PM states

[Yuval Mintz]

> Use the existing infrastructure of pci_set_power_state() instead of
> setting the relevant bits via PCI config read/write in the driver.
> Also, use the d3_delay field to specify how long to msleep after putting
> the device in the appropriate power management state.  Finally, use
> pci_pme_active() to set the PCI_PM_CTRL_PME_ENABLE bit in PCI PM control
> register.  pci_set_power_state() and pci_pme_active() perform the same
> operations as the driver did before, so there should be no functional
> change.  That being said, this has not been tested.
> 
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
> Cc: Ariel Elior <ariele@broadcom.com>

Acked-by: Yuval Mintz <yuvalmin@broadcom.com>