[PATCH v3 0/4] Simplify PCIe hotplug indicator control

[PATCH v3 0/4] Simplify PCIe hotplug indicator control

[Denis Efremov]

PCIe defines two optional hotplug indicators: a Power indicator and an
Attention indicator. Both are controlled by the same register, and each
can be on, off or blinking. The current interfaces
(pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
non-uniform and require two register writes in many cases where we could
do one.

This patchset introduces the new function pciehp_set_indicators(). It
allows one to set two indicators with a single register write. All
calls to previous interfaces (pciehp_green_led_* and
pciehp_set_attention_status()) are replaced with a new one. Thus,
the amount of duplicated code for setting indicators is reduced.

Changes in v3:
  - Changed pciehp_set_indicators() to work with existing
    PCI_EXP_SLTCTL_* macros
  - Reworked the inputs validation in pciehp_set_indicators()
  - Removed pciehp_set_attention_status() and pciehp_green_led_*()
    completely

Denis Efremov (4):
  PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
  PCI: pciehp: Switch LED indicators with a single write
  PCI: pciehp: Remove pciehp_set_attention_status()
  PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()

 drivers/pci/hotplug/pciehp.h      |  5 +-
 drivers/pci/hotplug/pciehp_core.c |  7 ++-
 drivers/pci/hotplug/pciehp_ctrl.c | 31 +++++++-----
 drivers/pci/hotplug/pciehp_hpc.c  | 82 ++++++++++---------------------
 include/uapi/linux/pci_regs.h     |  3 ++
 5 files changed, 54 insertions(+), 74 deletions(-)

-- 
2.21.0

[PATCH v3 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Denis Efremov]

Add pciehp_set_indicators() to set power and attention indicators with a
single register write. Thus, avoiding waiting twice for Command Complete.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h     |  1 +
 drivers/pci/hotplug/pciehp_hpc.c | 29 +++++++++++++++++++++++++++++
 include/uapi/linux/pci_regs.h    |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 8c51a04b8083..0e272bf3deb4 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -167,6 +167,7 @@ int pciehp_power_on_slot(struct controller *ctrl);
 void pciehp_power_off_slot(struct controller *ctrl);
 void pciehp_get_power_status(struct controller *ctrl, u8 *status);
 
+void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
 void pciehp_set_attention_status(struct controller *ctrl, u8 status);
 void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
 int pciehp_query_power_fault(struct controller *ctrl);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bd990e3371e3..5474b9854a7f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -443,6 +443,35 @@ void pciehp_set_attention_status(struct controller *ctrl, u8 value)
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
 }
 
+void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
+{
+	u16 cmd = 0, mask = 0;
+
+	if (PWR_LED(ctrl))
+		switch (pwr) {
+		case PCI_EXP_SLTCTL_PWR_IND_ON:
+		case PCI_EXP_SLTCTL_PWR_IND_BLINK:
+		case PCI_EXP_SLTCTL_PWR_IND_OFF:
+			cmd |= pwr;
+			mask |= PCI_EXP_SLTCTL_PIC;
+		}
+
+	if (ATTN_LED(ctrl))
+		switch (attn) {
+		case PCI_EXP_SLTCTL_ATTN_IND_ON:
+		case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
+		case PCI_EXP_SLTCTL_ATTN_IND_OFF:
+			cmd |= attn;
+			mask |= PCI_EXP_SLTCTL_AIC;
+		}
+
+	if (cmd) {
+		pcie_write_cmd_nowait(ctrl, cmd, mask);
+		ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+			 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
+	}
+}
+
 void pciehp_green_led_on(struct controller *ctrl)
 {
 	if (!PWR_LED(ctrl))
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index f28e562d7ca8..291788b58f3a 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -591,10 +591,12 @@
 #define  PCI_EXP_SLTCTL_CCIE	0x0010	/* Command Completed Interrupt Enable */
 #define  PCI_EXP_SLTCTL_HPIE	0x0020	/* Hot-Plug Interrupt Enable */
 #define  PCI_EXP_SLTCTL_AIC	0x00c0	/* Attention Indicator Control */
+#define  PCI_EXP_SLTCTL_ATTN_IND_NONE  0x0    /* Attention Indicator noop */
 #define  PCI_EXP_SLTCTL_ATTN_IND_ON    0x0040 /* Attention Indicator on */
 #define  PCI_EXP_SLTCTL_ATTN_IND_BLINK 0x0080 /* Attention Indicator blinking */
 #define  PCI_EXP_SLTCTL_ATTN_IND_OFF   0x00c0 /* Attention Indicator off */
 #define  PCI_EXP_SLTCTL_PIC	0x0300	/* Power Indicator Control */
+#define  PCI_EXP_SLTCTL_PWR_IND_NONE   0x0    /* Power Indicator noop */
 #define  PCI_EXP_SLTCTL_PWR_IND_ON     0x0100 /* Power Indicator on */
 #define  PCI_EXP_SLTCTL_PWR_IND_BLINK  0x0200 /* Power Indicator blinking */
 #define  PCI_EXP_SLTCTL_PWR_IND_OFF    0x0300 /* Power Indicator off */
-- 
2.21.0

[PATCH v3 2/4] PCI: pciehp: Switch LED indicators with a single write

[Denis Efremov]

This patch replaces all consecutive switches of power and attention
indicators with pciehp_set_indicators() call. Thus, performing only
single write to a register.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 19 ++++++++++---------
 drivers/pci/hotplug/pciehp_hpc.c  |  4 ++--
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 631ced0ab28a..232f7bfcfce9 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -42,8 +42,8 @@ static void set_slot_off(struct controller *ctrl)
 		msleep(1000);
 	}
 
-	pciehp_green_led_off(ctrl);
-	pciehp_set_attention_status(ctrl, 1);
+	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
+			      PCI_EXP_SLTCTL_ATTN_IND_ON);
 }
 
 /**
@@ -90,8 +90,8 @@ static int board_added(struct controller *ctrl)
 		}
 	}
 
-	pciehp_green_led_on(ctrl);
-	pciehp_set_attention_status(ctrl, 0);
+	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
+			      PCI_EXP_SLTCTL_ATTN_IND_OFF);
 	return 0;
 
 err_exit:
@@ -172,8 +172,8 @@ void pciehp_handle_button_press(struct controller *ctrl)
 				  slot_name(ctrl));
 		}
 		/* blink green LED and turn off amber */
-		pciehp_green_led_blink(ctrl);
-		pciehp_set_attention_status(ctrl, 0);
+		pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
+				      PCI_EXP_SLTCTL_ATTN_IND_OFF);
 		schedule_delayed_work(&ctrl->button_work, 5 * HZ);
 		break;
 	case BLINKINGOFF_STATE:
@@ -187,12 +187,13 @@ void pciehp_handle_button_press(struct controller *ctrl)
 		cancel_delayed_work(&ctrl->button_work);
 		if (ctrl->state == BLINKINGOFF_STATE) {
 			ctrl->state = ON_STATE;
-			pciehp_green_led_on(ctrl);
+			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
+					      PCI_EXP_SLTCTL_ATTN_IND_OFF);
 		} else {
 			ctrl->state = OFF_STATE;
-			pciehp_green_led_off(ctrl);
+			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
+					      PCI_EXP_SLTCTL_ATTN_IND_OFF);
 		}
-		pciehp_set_attention_status(ctrl, 0);
 		ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
 			  slot_name(ctrl));
 		break;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 5474b9854a7f..aa4252d11be2 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -667,8 +667,8 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
 		ctrl->power_fault_detected = 1;
 		ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(ctrl));
-		pciehp_set_attention_status(ctrl, 1);
-		pciehp_green_led_off(ctrl);
+		pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
+				      PCI_EXP_SLTCTL_ATTN_IND_ON);
 	}
 
 	/*
-- 
2.21.0

[PATCH v3 3/4] PCI: pciehp: Remove pciehp_set_attention_status()

[Denis Efremov]

Remove pciehp_set_attention_status() and use pciehp_set_indicators()
instead, since the code is mostly the same.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h      |  1 -
 drivers/pci/hotplug/pciehp_core.c |  7 ++++++-
 drivers/pci/hotplug/pciehp_hpc.c  | 25 -------------------------
 include/uapi/linux/pci_regs.h     |  1 +
 4 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 0e272bf3deb4..acda513f37d7 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -168,7 +168,6 @@ void pciehp_power_off_slot(struct controller *ctrl);
 void pciehp_get_power_status(struct controller *ctrl, u8 *status);
 
 void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
-void pciehp_set_attention_status(struct controller *ctrl, u8 status);
 void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
 int pciehp_query_power_fault(struct controller *ctrl);
 void pciehp_green_led_on(struct controller *ctrl);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 6ad0d86762cb..7a86ea90ed94 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -102,8 +102,13 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
 	struct controller *ctrl = to_ctrl(hotplug_slot);
 	struct pci_dev *pdev = ctrl->pcie->port;
 
+	if (status)
+		status <<= PCI_EXP_SLTCTL_ATTN_IND_SHIFT;
+	else
+		status = PCI_EXP_SLTCTL_ATTN_IND_OFF;
+
 	pci_config_pm_runtime_get(pdev);
-	pciehp_set_attention_status(ctrl, status);
+	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_NONE, status);
 	pci_config_pm_runtime_put(pdev);
 	return 0;
 }
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index aa4252d11be2..8f894fd5cd27 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -418,31 +418,6 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot,
 	return 0;
 }
 
-void pciehp_set_attention_status(struct controller *ctrl, u8 value)
-{
-	u16 slot_cmd;
-
-	if (!ATTN_LED(ctrl))
-		return;
-
-	switch (value) {
-	case 0:		/* turn off */
-		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_OFF;
-		break;
-	case 1:		/* turn on */
-		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_ON;
-		break;
-	case 2:		/* turn blink */
-		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_BLINK;
-		break;
-	default:
-		return;
-	}
-	pcie_write_cmd_nowait(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
-}
-
 void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
 {
 	u16 cmd = 0, mask = 0;
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 291788b58f3a..27d9f5bc1812 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -591,6 +591,7 @@
 #define  PCI_EXP_SLTCTL_CCIE	0x0010	/* Command Completed Interrupt Enable */
 #define  PCI_EXP_SLTCTL_HPIE	0x0020	/* Hot-Plug Interrupt Enable */
 #define  PCI_EXP_SLTCTL_AIC	0x00c0	/* Attention Indicator Control */
+#define  PCI_EXP_SLTCTL_ATTN_IND_SHIFT 6      /* Attention Indicator shift */
 #define  PCI_EXP_SLTCTL_ATTN_IND_NONE  0x0    /* Attention Indicator noop */
 #define  PCI_EXP_SLTCTL_ATTN_IND_ON    0x0040 /* Attention Indicator on */
 #define  PCI_EXP_SLTCTL_ATTN_IND_BLINK 0x0080 /* Attention Indicator blinking */
-- 
2.21.0

[PATCH v3 4/4] PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()

[Denis Efremov]

Remove pciehp_green_led_{on,off,blink}() and use pciehp_set_indicators()
instead, since the code is mostly the same.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h      |  3 ---
 drivers/pci/hotplug/pciehp_ctrl.c | 12 ++++++++---
 drivers/pci/hotplug/pciehp_hpc.c  | 36 -------------------------------
 3 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index acda513f37d7..da429345cf70 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -170,9 +170,6 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status);
 void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
 void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
 int pciehp_query_power_fault(struct controller *ctrl);
-void pciehp_green_led_on(struct controller *ctrl);
-void pciehp_green_led_off(struct controller *ctrl);
-void pciehp_green_led_blink(struct controller *ctrl);
 bool pciehp_card_present(struct controller *ctrl);
 bool pciehp_card_present_or_link_active(struct controller *ctrl);
 int pciehp_check_link_status(struct controller *ctrl);
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 232f7bfcfce9..862fe86e87cc 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -65,7 +65,9 @@ static int board_added(struct controller *ctrl)
 			return retval;
 	}
 
-	pciehp_green_led_blink(ctrl);
+	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
+			      PCI_EXP_SLTCTL_ATTN_IND_NONE);
+
 
 	/* Check link training status */
 	retval = pciehp_check_link_status(ctrl);
@@ -124,7 +126,9 @@ static void remove_board(struct controller *ctrl, bool safe_removal)
 	}
 
 	/* turn off Green LED */
-	pciehp_green_led_off(ctrl);
+	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
+			      PCI_EXP_SLTCTL_ATTN_IND_NONE);
+
 }
 
 static int pciehp_enable_slot(struct controller *ctrl);
@@ -311,7 +315,9 @@ static int pciehp_enable_slot(struct controller *ctrl)
 	pm_runtime_get_sync(&ctrl->pcie->port->dev);
 	ret = __pciehp_enable_slot(ctrl);
 	if (ret && ATTN_BUTTN(ctrl))
-		pciehp_green_led_off(ctrl); /* may be blinking */
+		/* may be blinking */
+		pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
+				      PCI_EXP_SLTCTL_ATTN_IND_NONE);
 	pm_runtime_put(&ctrl->pcie->port->dev);
 
 	mutex_lock(&ctrl->state_lock);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 8f894fd5cd27..9dc1ecd703b9 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -447,42 +447,6 @@ void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
 	}
 }
 
-void pciehp_green_led_on(struct controller *ctrl)
-{
-	if (!PWR_LED(ctrl))
-		return;
-
-	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
-			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_ON);
-}
-
-void pciehp_green_led_off(struct controller *ctrl)
-{
-	if (!PWR_LED(ctrl))
-		return;
-
-	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
-			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_OFF);
-}
-
-void pciehp_green_led_blink(struct controller *ctrl)
-{
-	if (!PWR_LED(ctrl))
-		return;
-
-	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
-			      PCI_EXP_SLTCTL_PIC);
-	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
-		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
-		 PCI_EXP_SLTCTL_PWR_IND_BLINK);
-}
-
 int pciehp_power_on_slot(struct controller *ctrl)
 {
 	struct pci_dev *pdev = ctrl_dev(ctrl);
-- 
2.21.0

Re: [PATCH v3 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Denis Efremov]

Hi,

On 8/19/19 7:06 PM, Denis Efremov wrote:
> +		switch (pwr) {
> +		case PCI_EXP_SLTCTL_PWR_IND_ON:
> +		case PCI_EXP_SLTCTL_PWR_IND_BLINK:
> +		case PCI_EXP_SLTCTL_PWR_IND_OFF:
> +			cmd |= pwr;
> +			mask |= PCI_EXP_SLTCTL_PIC;
> +		}
> +

On 8/12/19 11:25 AM, sathyanarayanan kuppuswamy wrote:
> Do we need to switch case here ? if (pwr > 0) {} should work right ? 

I saved the switch here from v2. I think switch makes the inputs check more
precise and filters-out all non-valid values. Maybe this check is too strict?

We could use mask here ON|OFF|BLINK for the check, but I don't know how hardware
will handle a case, for example, pwr == ON|BLINK.

Thanks,
Denis

Re: [PATCH v3 0/4] Simplify PCIe hotplug indicator control

[Denis Efremov]

On 8/19/19 7:06 PM, Denis Efremov wrote:
> PCIe defines two optional hotplug indicators: a Power indicator and an
> Attention indicator. Both are controlled by the same register, and each
> can be on, off or blinking. The current interfaces
> (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> non-uniform and require two register writes in many cases where we could
> do one.
> 
> This patchset introduces the new function pciehp_set_indicators(). It
> allows one to set two indicators with a single register write. All
> calls to previous interfaces (pciehp_green_led_* and
> pciehp_set_attention_status()) are replaced with a new one. Thus,
> the amount of duplicated code for setting indicators is reduced.
> 
> Changes in v3:
>   - Changed pciehp_set_indicators() to work with existing
>     PCI_EXP_SLTCTL_* macros
>   - Reworked the inputs validation in pciehp_set_indicators()
>   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
>     completely
> 
> Denis Efremov (4):
>   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
>   PCI: pciehp: Switch LED indicators with a single write
>   PCI: pciehp: Remove pciehp_set_attention_status()
>   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()

Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
The changes in the last 2 patches were significant.

> 
>  drivers/pci/hotplug/pciehp.h      |  5 +-
>  drivers/pci/hotplug/pciehp_core.c |  7 ++-
>  drivers/pci/hotplug/pciehp_ctrl.c | 31 +++++++-----
>  drivers/pci/hotplug/pciehp_hpc.c  | 82 ++++++++++---------------------
>  include/uapi/linux/pci_regs.h     |  3 ++
>  5 files changed, 54 insertions(+), 74 deletions(-)
> 

Re: [PATCH v3 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Kuppuswamy Sathyanarayanan]

On Mon, Aug 19, 2019 at 07:06:40PM +0300, Denis Efremov wrote:
> Add pciehp_set_indicators() to set power and attention indicators with a
> single register write. Thus, avoiding waiting twice for Command Complete.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  drivers/pci/hotplug/pciehp.h     |  1 +
>  drivers/pci/hotplug/pciehp_hpc.c | 29 +++++++++++++++++++++++++++++
>  include/uapi/linux/pci_regs.h    |  2 ++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 8c51a04b8083..0e272bf3deb4 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -167,6 +167,7 @@ int pciehp_power_on_slot(struct controller *ctrl);
>  void pciehp_power_off_slot(struct controller *ctrl);
>  void pciehp_get_power_status(struct controller *ctrl, u8 *status);
>  
> +void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
>  void pciehp_set_attention_status(struct controller *ctrl, u8 status);
>  void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
>  int pciehp_query_power_fault(struct controller *ctrl);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bd990e3371e3..5474b9854a7f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -443,6 +443,35 @@ void pciehp_set_attention_status(struct controller *ctrl, u8 value)
>  		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
>  }
>  
> +void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
> +{
> +	u16 cmd = 0, mask = 0;
> +
> +	if (PWR_LED(ctrl))
> +		switch (pwr) {
> +		case PCI_EXP_SLTCTL_PWR_IND_ON:
> +		case PCI_EXP_SLTCTL_PWR_IND_BLINK:
> +		case PCI_EXP_SLTCTL_PWR_IND_OFF:
> +			cmd |= pwr;
> +			mask |= PCI_EXP_SLTCTL_PIC;
> +		}
> +
> +	if (ATTN_LED(ctrl))
> +		switch (attn) {
> +		case PCI_EXP_SLTCTL_ATTN_IND_ON:
> +		case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
> +		case PCI_EXP_SLTCTL_ATTN_IND_OFF:
> +			cmd |= attn;
> +			mask |= PCI_EXP_SLTCTL_AIC;
> +		}
> +
> +	if (cmd) {
> +		pcie_write_cmd_nowait(ctrl, cmd, mask);
> +		ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
> +			 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
> +	}
> +}
> +
>  void pciehp_green_led_on(struct controller *ctrl)
>  {
>  	if (!PWR_LED(ctrl))
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index f28e562d7ca8..291788b58f3a 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -591,10 +591,12 @@
>  #define  PCI_EXP_SLTCTL_CCIE	0x0010	/* Command Completed Interrupt Enable */
>  #define  PCI_EXP_SLTCTL_HPIE	0x0020	/* Hot-Plug Interrupt Enable */
>  #define  PCI_EXP_SLTCTL_AIC	0x00c0	/* Attention Indicator Control */
> +#define  PCI_EXP_SLTCTL_ATTN_IND_NONE  0x0    /* Attention Indicator noop */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_ON    0x0040 /* Attention Indicator on */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_BLINK 0x0080 /* Attention Indicator blinking */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_OFF   0x00c0 /* Attention Indicator off */
>  #define  PCI_EXP_SLTCTL_PIC	0x0300	/* Power Indicator Control */
> +#define  PCI_EXP_SLTCTL_PWR_IND_NONE   0x0    /* Power Indicator noop */
Nitpick: Since the current patch does not use it, May be you can create a
new patch for these #defines ? or you could add some info about its
usage in comment section.
>  #define  PCI_EXP_SLTCTL_PWR_IND_ON     0x0100 /* Power Indicator on */
>  #define  PCI_EXP_SLTCTL_PWR_IND_BLINK  0x0200 /* Power Indicator blinking */
>  #define  PCI_EXP_SLTCTL_PWR_IND_OFF    0x0300 /* Power Indicator off */
> -- 
> 2.21.0
> 

-- 
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v3 0/4] Simplify PCIe hotplug indicator control

[Bjorn Helgaas]

On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote:
> On 8/19/19 7:06 PM, Denis Efremov wrote:
> > PCIe defines two optional hotplug indicators: a Power indicator and an
> > Attention indicator. Both are controlled by the same register, and each
> > can be on, off or blinking. The current interfaces
> > (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> > non-uniform and require two register writes in many cases where we could
> > do one.
> > 
> > This patchset introduces the new function pciehp_set_indicators(). It
> > allows one to set two indicators with a single register write. All
> > calls to previous interfaces (pciehp_green_led_* and
> > pciehp_set_attention_status()) are replaced with a new one. Thus,
> > the amount of duplicated code for setting indicators is reduced.
> > 
> > Changes in v3:
> >   - Changed pciehp_set_indicators() to work with existing
> >     PCI_EXP_SLTCTL_* macros
> >   - Reworked the inputs validation in pciehp_set_indicators()
> >   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
> >     completely
> > 
> > Denis Efremov (4):
> >   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
> >   PCI: pciehp: Switch LED indicators with a single write
> >   PCI: pciehp: Remove pciehp_set_attention_status()
> >   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()
> 
> Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
> The changes in the last 2 patches were significant.

Anybody want to review these?

Re: [PATCH v3 2/4] PCI: pciehp: Switch LED indicators with a single write

[Kuppuswamy Sathyanarayanan]

On Mon, Aug 19, 2019 at 07:06:41PM +0300, Denis Efremov wrote:
> This patch replaces all consecutive switches of power and attention
> indicators with pciehp_set_indicators() call. Thus, performing only
> single write to a register.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Denis Efremov <efremov@linux.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 19 ++++++++++---------
>  drivers/pci/hotplug/pciehp_hpc.c  |  4 ++--
>  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 631ced0ab28a..232f7bfcfce9 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -42,8 +42,8 @@ static void set_slot_off(struct controller *ctrl)
>  		msleep(1000);
>  	}
>  
> -	pciehp_green_led_off(ctrl);
> -	pciehp_set_attention_status(ctrl, 1);
> +	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> +			      PCI_EXP_SLTCTL_ATTN_IND_ON);
>  }
>  
>  /**
> @@ -90,8 +90,8 @@ static int board_added(struct controller *ctrl)
>  		}
>  	}
>  
> -	pciehp_green_led_on(ctrl);
> -	pciehp_set_attention_status(ctrl, 0);
> +	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
> +			      PCI_EXP_SLTCTL_ATTN_IND_OFF);
>  	return 0;
>  
>  err_exit:
> @@ -172,8 +172,8 @@ void pciehp_handle_button_press(struct controller *ctrl)
>  				  slot_name(ctrl));
>  		}
>  		/* blink green LED and turn off amber */
> -		pciehp_green_led_blink(ctrl);
> -		pciehp_set_attention_status(ctrl, 0);
> +		pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
> +				      PCI_EXP_SLTCTL_ATTN_IND_OFF);
>  		schedule_delayed_work(&ctrl->button_work, 5 * HZ);
>  		break;
>  	case BLINKINGOFF_STATE:
> @@ -187,12 +187,13 @@ void pciehp_handle_button_press(struct controller *ctrl)
>  		cancel_delayed_work(&ctrl->button_work);
>  		if (ctrl->state == BLINKINGOFF_STATE) {
>  			ctrl->state = ON_STATE;
> -			pciehp_green_led_on(ctrl);
> +			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
> +					      PCI_EXP_SLTCTL_ATTN_IND_OFF);
>  		} else {
>  			ctrl->state = OFF_STATE;
> -			pciehp_green_led_off(ctrl);
> +			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> +					      PCI_EXP_SLTCTL_ATTN_IND_OFF);
>  		}
> -		pciehp_set_attention_status(ctrl, 0);
>  		ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
>  			  slot_name(ctrl));
>  		break;
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 5474b9854a7f..aa4252d11be2 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -667,8 +667,8 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
>  	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
>  		ctrl->power_fault_detected = 1;
>  		ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(ctrl));
> -		pciehp_set_attention_status(ctrl, 1);
> -		pciehp_green_led_off(ctrl);
> +		pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> +				      PCI_EXP_SLTCTL_ATTN_IND_ON);
>  	}
>  
>  	/*
> -- 
> 2.21.0
> 

-- 
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v3 3/4] PCI: pciehp: Remove pciehp_set_attention_status()

[Kuppuswamy Sathyanarayanan]

On Mon, Aug 19, 2019 at 07:06:42PM +0300, Denis Efremov wrote:
> Remove pciehp_set_attention_status() and use pciehp_set_indicators()
> instead, since the code is mostly the same.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp.h      |  1 -
>  drivers/pci/hotplug/pciehp_core.c |  7 ++++++-
>  drivers/pci/hotplug/pciehp_hpc.c  | 25 -------------------------
>  include/uapi/linux/pci_regs.h     |  1 +
>  4 files changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 0e272bf3deb4..acda513f37d7 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -168,7 +168,6 @@ void pciehp_power_off_slot(struct controller *ctrl);
>  void pciehp_get_power_status(struct controller *ctrl, u8 *status);
>  
>  void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
> -void pciehp_set_attention_status(struct controller *ctrl, u8 status);
>  void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
>  int pciehp_query_power_fault(struct controller *ctrl);
>  void pciehp_green_led_on(struct controller *ctrl);
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 6ad0d86762cb..7a86ea90ed94 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -102,8 +102,13 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
>  	struct controller *ctrl = to_ctrl(hotplug_slot);
>  	struct pci_dev *pdev = ctrl->pcie->port;
>  
> +	if (status)
> +		status <<= PCI_EXP_SLTCTL_ATTN_IND_SHIFT;
> +	else
> +		status = PCI_EXP_SLTCTL_ATTN_IND_OFF;
> +
>  	pci_config_pm_runtime_get(pdev);
> -	pciehp_set_attention_status(ctrl, status);
> +	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_NONE, status);
>  	pci_config_pm_runtime_put(pdev);
>  	return 0;
>  }
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index aa4252d11be2..8f894fd5cd27 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -418,31 +418,6 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot,
>  	return 0;
>  }
>  
> -void pciehp_set_attention_status(struct controller *ctrl, u8 value)
> -{
> -	u16 slot_cmd;
> -
> -	if (!ATTN_LED(ctrl))
> -		return;
> -
> -	switch (value) {
> -	case 0:		/* turn off */
> -		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_OFF;
> -		break;
> -	case 1:		/* turn on */
> -		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_ON;
> -		break;
> -	case 2:		/* turn blink */
> -		slot_cmd = PCI_EXP_SLTCTL_ATTN_IND_BLINK;
> -		break;
> -	default:
> -		return;
> -	}
> -	pcie_write_cmd_nowait(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
> -	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
> -		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
> -}
> -
>  void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
>  {
>  	u16 cmd = 0, mask = 0;
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 291788b58f3a..27d9f5bc1812 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -591,6 +591,7 @@
>  #define  PCI_EXP_SLTCTL_CCIE	0x0010	/* Command Completed Interrupt Enable */
>  #define  PCI_EXP_SLTCTL_HPIE	0x0020	/* Hot-Plug Interrupt Enable */
>  #define  PCI_EXP_SLTCTL_AIC	0x00c0	/* Attention Indicator Control */
> +#define  PCI_EXP_SLTCTL_ATTN_IND_SHIFT 6      /* Attention Indicator shift */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_NONE  0x0    /* Attention Indicator noop */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_ON    0x0040 /* Attention Indicator on */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_BLINK 0x0080 /* Attention Indicator blinking */
> -- 
> 2.21.0
> 

-- 
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v3 4/4] PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()

[Kuppuswamy Sathyanarayanan]

On Mon, Aug 19, 2019 at 07:06:43PM +0300, Denis Efremov wrote:
> Remove pciehp_green_led_{on,off,blink}() and use pciehp_set_indicators()
> instead, since the code is mostly the same.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp.h      |  3 ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 12 ++++++++---
>  drivers/pci/hotplug/pciehp_hpc.c  | 36 -------------------------------
>  3 files changed, 9 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index acda513f37d7..da429345cf70 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -170,9 +170,6 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status);
>  void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
>  void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
>  int pciehp_query_power_fault(struct controller *ctrl);
> -void pciehp_green_led_on(struct controller *ctrl);
> -void pciehp_green_led_off(struct controller *ctrl);
> -void pciehp_green_led_blink(struct controller *ctrl);
>  bool pciehp_card_present(struct controller *ctrl);
>  bool pciehp_card_present_or_link_active(struct controller *ctrl);
>  int pciehp_check_link_status(struct controller *ctrl);
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 232f7bfcfce9..862fe86e87cc 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -65,7 +65,9 @@ static int board_added(struct controller *ctrl)
>  			return retval;
>  	}
>  
> -	pciehp_green_led_blink(ctrl);
> +	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
> +			      PCI_EXP_SLTCTL_ATTN_IND_NONE);
> +
>  
>  	/* Check link training status */
>  	retval = pciehp_check_link_status(ctrl);
> @@ -124,7 +126,9 @@ static void remove_board(struct controller *ctrl, bool safe_removal)
>  	}
>  
>  	/* turn off Green LED */
> -	pciehp_green_led_off(ctrl);
> +	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> +			      PCI_EXP_SLTCTL_ATTN_IND_NONE);
> +
>  }
>  
>  static int pciehp_enable_slot(struct controller *ctrl);
> @@ -311,7 +315,9 @@ static int pciehp_enable_slot(struct controller *ctrl)
>  	pm_runtime_get_sync(&ctrl->pcie->port->dev);
>  	ret = __pciehp_enable_slot(ctrl);
>  	if (ret && ATTN_BUTTN(ctrl))
> -		pciehp_green_led_off(ctrl); /* may be blinking */
> +		/* may be blinking */
> +		pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> +				      PCI_EXP_SLTCTL_ATTN_IND_NONE);
>  	pm_runtime_put(&ctrl->pcie->port->dev);
>  
>  	mutex_lock(&ctrl->state_lock);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 8f894fd5cd27..9dc1ecd703b9 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -447,42 +447,6 @@ void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
>  	}
>  }
>  
> -void pciehp_green_led_on(struct controller *ctrl)
> -{
> -	if (!PWR_LED(ctrl))
> -		return;
> -
> -	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
> -			      PCI_EXP_SLTCTL_PIC);
> -	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
> -		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
> -		 PCI_EXP_SLTCTL_PWR_IND_ON);
> -}
> -
> -void pciehp_green_led_off(struct controller *ctrl)
> -{
> -	if (!PWR_LED(ctrl))
> -		return;
> -
> -	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> -			      PCI_EXP_SLTCTL_PIC);
> -	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
> -		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
> -		 PCI_EXP_SLTCTL_PWR_IND_OFF);
> -}
> -
> -void pciehp_green_led_blink(struct controller *ctrl)
> -{
> -	if (!PWR_LED(ctrl))
> -		return;
> -
> -	pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
> -			      PCI_EXP_SLTCTL_PIC);
> -	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
> -		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
> -		 PCI_EXP_SLTCTL_PWR_IND_BLINK);
> -}
> -
>  int pciehp_power_on_slot(struct controller *ctrl)
>  {
>  	struct pci_dev *pdev = ctrl_dev(ctrl);
> -- 
> 2.21.0
> 

-- 
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v3 0/4] Simplify PCIe hotplug indicator control

[Kuppuswamy Sathyanarayanan]

On Tue, Aug 27, 2019 at 05:32:54PM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote:
> > On 8/19/19 7:06 PM, Denis Efremov wrote:
> > > PCIe defines two optional hotplug indicators: a Power indicator and an
> > > Attention indicator. Both are controlled by the same register, and each
> > > can be on, off or blinking. The current interfaces
> > > (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> > > non-uniform and require two register writes in many cases where we could
> > > do one.
> > > 
> > > This patchset introduces the new function pciehp_set_indicators(). It
> > > allows one to set two indicators with a single register write. All
> > > calls to previous interfaces (pciehp_green_led_* and
> > > pciehp_set_attention_status()) are replaced with a new one. Thus,
> > > the amount of duplicated code for setting indicators is reduced.
> > > 
> > > Changes in v3:
> > >   - Changed pciehp_set_indicators() to work with existing
> > >     PCI_EXP_SLTCTL_* macros
> > >   - Reworked the inputs validation in pciehp_set_indicators()
> > >   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
> > >     completely
> > > 
> > > Denis Efremov (4):
> > >   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
> > >   PCI: pciehp: Switch LED indicators with a single write
> > >   PCI: pciehp: Remove pciehp_set_attention_status()
> > >   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()
> > 
> > Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
> > The changes in the last 2 patches were significant.
> 
> Anybody want to review these?

Except for one nitpick in [PATCH v3 1/4], rest seems fine to me.

-- 
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v3 0/4] Simplify PCIe hotplug indicator control

[Bjorn Helgaas]

On Tue, Aug 27, 2019 at 05:32:54PM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote:
> > On 8/19/19 7:06 PM, Denis Efremov wrote:
> > > PCIe defines two optional hotplug indicators: a Power indicator and an
> > > Attention indicator. Both are controlled by the same register, and each
> > > can be on, off or blinking. The current interfaces
> > > (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> > > non-uniform and require two register writes in many cases where we could
> > > do one.
> > > 
> > > This patchset introduces the new function pciehp_set_indicators(). It
> > > allows one to set two indicators with a single register write. All
> > > calls to previous interfaces (pciehp_green_led_* and
> > > pciehp_set_attention_status()) are replaced with a new one. Thus,
> > > the amount of duplicated code for setting indicators is reduced.
> > > 
> > > Changes in v3:
> > >   - Changed pciehp_set_indicators() to work with existing
> > >     PCI_EXP_SLTCTL_* macros
> > >   - Reworked the inputs validation in pciehp_set_indicators()
> > >   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
> > >     completely
> > > 
> > > Denis Efremov (4):
> > >   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
> > >   PCI: pciehp: Switch LED indicators with a single write
> > >   PCI: pciehp: Remove pciehp_set_attention_status()
> > >   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()
> > 
> > Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
> > The changes in the last 2 patches were significant.
> 
> Anybody want to review these?

Unrelated, but if anybody is looking at pciehp, is there value in
having pciehp split across five files?

  drivers/pci/hotplug/pciehp.h
  drivers/pci/hotplug/pciehp_core.c
  drivers/pci/hotplug/pciehp_ctrl.c
  drivers/pci/hotplug/pciehp_hpc.c
  drivers/pci/hotplug/pciehp_pci.c

To me, it just makes things harder because when I'm browsing for
something in pciehp and I don't know the exact name of it, I have to
guess which file it's in, and I'm invariably wrong.

It seems like it would be much simpler if everything were in a single
pciehp.c file.  Then we could also get rid of the header and make
several more things static.

Bjorn

Re: [PATCH v3 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Oliver O'Halloran]

On Tue, Aug 20, 2019 at 2:17 AM Denis Efremov <efremov@linux.com> wrote:
>
> Hi,
>
> On 8/19/19 7:06 PM, Denis Efremov wrote:
> On 8/12/19 11:25 AM, sathyanarayanan kuppuswamy wrote:
> > Do we need to switch case here ? if (pwr > 0) {} should work right ?
>
> I saved the switch here from v2. I think switch makes the inputs check more
> precise and filters-out all non-valid values. Maybe this check is too strict?

Sounds like you're overthinking it tbh. If want to catch programming
errors then a WARN_ON_ONCE() in the default case would be better than
silently ignoring invalid values, but it's pretty hard to care.

> We could use mask here ON|OFF|BLINK for the check, but I don't know how hardware
> will handle a case, for example, pwr == ON|BLINK.

ON|BLINK is the same as OFF

Re: [PATCH v3 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Oliver O'Halloran]

On Tue, Aug 20, 2019 at 2:07 AM Denis Efremov <efremov@linux.com> wrote:
>
> Add pciehp_set_indicators() to set power and attention indicators with a
> single register write. Thus, avoiding waiting twice for Command Complete.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  drivers/pci/hotplug/pciehp.h     |  1 +
>  drivers/pci/hotplug/pciehp_hpc.c | 29 +++++++++++++++++++++++++++++
>  include/uapi/linux/pci_regs.h    |  2 ++
>  3 files changed, 32 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 8c51a04b8083..0e272bf3deb4 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -167,6 +167,7 @@ int pciehp_power_on_slot(struct controller *ctrl);
>  void pciehp_power_off_slot(struct controller *ctrl);
>  void pciehp_get_power_status(struct controller *ctrl, u8 *status);
>
> +void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
>  void pciehp_set_attention_status(struct controller *ctrl, u8 status);
>  void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
>  int pciehp_query_power_fault(struct controller *ctrl);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bd990e3371e3..5474b9854a7f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -443,6 +443,35 @@ void pciehp_set_attention_status(struct controller *ctrl, u8 value)
>                  pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
>  }
>
> +void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
> +{
> +       u16 cmd = 0, mask = 0;
> +
> +       if (PWR_LED(ctrl))
> +               switch (pwr) {
> +               case PCI_EXP_SLTCTL_PWR_IND_ON:
> +               case PCI_EXP_SLTCTL_PWR_IND_BLINK:
> +               case PCI_EXP_SLTCTL_PWR_IND_OFF:

Do you need an explicit /* fallthrough */ comment? I thought the
fallthrough warning was enabled by default now.

> +                       cmd |= pwr;
> +                       mask |= PCI_EXP_SLTCTL_PIC;
> +               }
> +
> +       if (ATTN_LED(ctrl))
> +               switch (attn) {
> +               case PCI_EXP_SLTCTL_ATTN_IND_ON:
> +               case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
> +               case PCI_EXP_SLTCTL_ATTN_IND_OFF:
> +                       cmd |= attn;
> +                       mask |= PCI_EXP_SLTCTL_AIC;
> +               }
> +
> +       if (cmd) {
> +               pcie_write_cmd_nowait(ctrl, cmd, mask);
> +               ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
> +                        pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
> +       }
> +}
> +
>  void pciehp_green_led_on(struct controller *ctrl)
>  {
>         if (!PWR_LED(ctrl))
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index f28e562d7ca8..291788b58f3a 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -591,10 +591,12 @@
>  #define  PCI_EXP_SLTCTL_CCIE   0x0010  /* Command Completed Interrupt Enable */
>  #define  PCI_EXP_SLTCTL_HPIE   0x0020  /* Hot-Plug Interrupt Enable */
>  #define  PCI_EXP_SLTCTL_AIC    0x00c0  /* Attention Indicator Control */

> +#define  PCI_EXP_SLTCTL_ATTN_IND_NONE  0x0    /* Attention Indicator noop */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_ON    0x0040 /* Attention Indicator on */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_BLINK 0x0080 /* Attention Indicator blinking */
>  #define  PCI_EXP_SLTCTL_ATTN_IND_OFF   0x00c0 /* Attention Indicator off */
>  #define  PCI_EXP_SLTCTL_PIC    0x0300  /* Power Indicator Control */
> +#define  PCI_EXP_SLTCTL_PWR_IND_NONE   0x0    /* Power Indicator noop */

I don't think pci_regs.h is the right place for this. The contents of
pci_regs.h is essentially a transcription of bit definitions from the
various PCI standards documents. The PCIe Base spec says the the 0b00
combination for the two-bit Power and Attn indicator fields is
reserved and there's nothing suggesting that writing a zero to those
fields should preserve the current value. Basicly, we now have a
implementation quirk of pcie_set_indicators() masquerading as part of
the standard.

>  #define  PCI_EXP_SLTCTL_PWR_IND_ON     0x0100 /* Power Indicator on */
>  #define  PCI_EXP_SLTCTL_PWR_IND_BLINK  0x0200 /* Power Indicator blinking */
>  #define  PCI_EXP_SLTCTL_PWR_IND_OFF    0x0300 /* Power Indicator off */
> --
> 2.21.0
>

Re: [PATCH v3 4/4] PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()

[Oliver O'Halloran]

On Tue, Aug 20, 2019 at 2:09 AM Denis Efremov <efremov@linux.com> wrote:
>
> Remove pciehp_green_led_{on,off,blink}() and use pciehp_set_indicators()
> instead, since the code is mostly the same.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  drivers/pci/hotplug/pciehp.h      |  3 ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 12 ++++++++---
>  drivers/pci/hotplug/pciehp_hpc.c  | 36 -------------------------------
>  3 files changed, 9 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index acda513f37d7..da429345cf70 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -170,9 +170,6 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status);
>  void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn);
>  void pciehp_get_latch_status(struct controller *ctrl, u8 *status);
>  int pciehp_query_power_fault(struct controller *ctrl);
> -void pciehp_green_led_on(struct controller *ctrl);
> -void pciehp_green_led_off(struct controller *ctrl);
> -void pciehp_green_led_blink(struct controller *ctrl);
>  bool pciehp_card_present(struct controller *ctrl);
>  bool pciehp_card_present_or_link_active(struct controller *ctrl);
>  int pciehp_check_link_status(struct controller *ctrl);
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 232f7bfcfce9..862fe86e87cc 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -65,7 +65,9 @@ static int board_added(struct controller *ctrl)
>                         return retval;
>         }
>
> -       pciehp_green_led_blink(ctrl);
> +       pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
> +                             PCI_EXP_SLTCTL_ATTN_IND_NONE);

I think it woud be good if you provided a helper macro for setting one
of the indicators rather than open coding the _NONE constant. e.g.

#define set_power_indicator(ctrl, x) pciehp_set_indicators(ctrl, (x),
PCI_EXP_SLTCTL_ATTN_IND_NONE);

then you can do:

set_power_indicator(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK);

which is a bit nicer to read.

Re: [PATCH v3 0/4] Simplify PCIe hotplug indicator control

[Lukas Wunner]

On Tue, Aug 27, 2019 at 05:53:19PM -0500, Bjorn Helgaas wrote:
> Unrelated, but if anybody is looking at pciehp, is there value in
> having pciehp split across five files?
> 
>   drivers/pci/hotplug/pciehp.h
>   drivers/pci/hotplug/pciehp_core.c
>   drivers/pci/hotplug/pciehp_ctrl.c
>   drivers/pci/hotplug/pciehp_hpc.c
>   drivers/pci/hotplug/pciehp_pci.c
> 
> To me, it just makes things harder because when I'm browsing for
> something in pciehp and I don't know the exact name of it, I have to
> guess which file it's in, and I'm invariably wrong.
> 
> It seems like it would be much simpler if everything were in a single
> pciehp.c file.  Then we could also get rid of the header and make
> several more things static.

I wouldn't go so far as to say there's *value* in the split.

It's a historic artefact, it's been like that since pciehp was
introduced back in 2004:
https://git.kernel.org/tglx/history/c/c16b4b14d980

I was hesitant to change it so far, in particular because it makes
it harder to backport stable patches.

That said, I did think about rearranging things to reduce the number
of files and non-static declarations or to give the files better names.
I wasn't thinking of squashing everything into one file, it would
probably be quite large and not make things simpler.

The general logic is that everything touching the registers is in
pciehp_hpc.c.  You won't (or shouldn't) find register access in any
of the other files.

pciehp_ctrl.c is the control logic, reaction to events, etc.
Though portions of the control logic are also in pciehp_hpc.c,
in particular the interrupt servicing.

pciehp_core.c contains the interface to the PCI hotplug core and
the probe/remove/PM routines.

pciehp_pci.c contains bringup/teardown of the slot.

One thing that I think deserves changing is this comment at the
top of each file:

"Send feedback to <greg@kroah.com>, <kristen.c.accardi@intel.com>"

We now use MAINTAINERS to do this.  Kristen took over maintainership
in 2005 with commit 8cf4c19523b7 but by 2009 she had stepped down per
commit be3652b8a253.  So providing her address is no longer accurate.
And I imagine Greg is no longer as familiar with the driver as it has
changed considerably since 2004.  He'd probably have to defer to others
who have done work on it recently (such as me).

Thanks,

Lukas