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

[PATCH v2 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.

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: Replace pciehp_set_attention_status()
  PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()

 drivers/pci/hotplug/pciehp.h      | 30 +++++++++++--
 drivers/pci/hotplug/pciehp_ctrl.c | 14 +++---
 drivers/pci/hotplug/pciehp_hpc.c  | 74 +++++++++++--------------------
 include/uapi/linux/pci_regs.h     |  2 +
 4 files changed, 58 insertions(+), 62 deletions(-)

-- 
2.21.0

[PATCH v2 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.
enum pciehp_indicator introduced to switch between the indicators statuses.

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

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 8c51a04b8083..17305a6f01f1 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -150,6 +150,17 @@ struct controller {
 #define NO_CMD_CMPL(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
 #define PSN(ctrl)		(((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)
 
+enum pciehp_indicator {
+	ATTN_NONE  = -1,
+	ATTN_ON    =  1,
+	ATTN_BLINK =  2,
+	ATTN_OFF   =  3,
+	PWR_NONE   = -1,
+	PWR_ON     =  1,
+	PWR_BLINK  =  2,
+	PWR_OFF    =  3,
+};
+
 void pciehp_request(struct controller *ctrl, int action);
 void pciehp_handle_button_press(struct controller *ctrl);
 void pciehp_handle_disable_request(struct controller *ctrl);
@@ -167,6 +178,9 @@ 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,
+			   enum pciehp_indicator pwr,
+			   enum pciehp_indicator 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..5a690b1579ec 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -443,6 +443,44 @@ 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,
+			   enum pciehp_indicator pwr,
+			   enum pciehp_indicator attn)
+{
+	u16 cmd = 0, mask = 0;
+
+	if ((!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
+	    (!ATTN_LED(ctrl) || attn == ATTN_NONE))
+		return;
+
+	switch (pwr) {
+	case PWR_ON:
+	case PWR_BLINK:
+	case PWR_OFF:
+		cmd = pwr << PCI_EXP_SLTCTL_PWR_IND_OFFSET;
+		mask = PCI_EXP_SLTCTL_PIC;
+		break;
+	default:
+		break;
+	}
+
+	switch (attn) {
+	case ATTN_ON:
+	case ATTN_BLINK:
+	case ATTN_OFF:
+		cmd |= attn << PCI_EXP_SLTCTL_ATTN_IND_OFFSET;
+		mask |= PCI_EXP_SLTCTL_AIC;
+		break;
+	default:
+		break;
+	}
+
+	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..18722d1f54a0 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_OFFSET 0x6   /* Attention Indicator Offset */
 #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_OFFSET 0x8    /* Power Indicator Offset */
 #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 v2 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: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 14 +++++---------
 drivers/pci/hotplug/pciehp_hpc.c  |  3 +--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 631ced0ab28a..258a4060466d 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -42,8 +42,7 @@ 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, PWR_OFF, ATTN_ON);
 }
 
 /**
@@ -90,8 +89,7 @@ static int board_added(struct controller *ctrl)
 		}
 	}
 
-	pciehp_green_led_on(ctrl);
-	pciehp_set_attention_status(ctrl, 0);
+	pciehp_set_indicators(ctrl, PWR_ON, ATTN_OFF);
 	return 0;
 
 err_exit:
@@ -172,8 +170,7 @@ 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, PWR_BLINK, ATTN_OFF);
 		schedule_delayed_work(&ctrl->button_work, 5 * HZ);
 		break;
 	case BLINKINGOFF_STATE:
@@ -187,12 +184,11 @@ 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, PWR_ON, ATTN_OFF);
 		} else {
 			ctrl->state = OFF_STATE;
-			pciehp_green_led_off(ctrl);
+			pciehp_set_indicators(ctrl, PWR_OFF, ATTN_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 5a690b1579ec..68b880bc30db 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -676,8 +676,7 @@ 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, PWR_OFF, ATTN_ON);
 	}
 
 	/*
-- 
2.21.0

[PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status()

[Denis Efremov]

This patch replaces pciehp_set_attention_status() with
pciehp_set_indicators().

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h     |  4 +++-
 drivers/pci/hotplug/pciehp_hpc.c | 25 -------------------------
 2 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 17305a6f01f1..9a2a2d0db9d2 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -181,7 +181,6 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status);
 void pciehp_set_indicators(struct controller *ctrl,
 			   enum pciehp_indicator pwr,
 			   enum pciehp_indicator 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);
@@ -200,6 +199,9 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status);
 int pciehp_set_raw_indicator_status(struct hotplug_slot *h_slot, u8 status);
 int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
 
+#define pciehp_set_attention_status(ctrl, status) \
+	pciehp_set_indicators(ctrl, PWR_NONE, (status == 0 ? ATTN_OFF : status))
+
 static inline const char *slot_name(struct controller *ctrl)
 {
 	return hotplug_slot_name(&ctrl->hotplug_slot);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 68b880bc30db..fb4bea16063a 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,
 			   enum pciehp_indicator pwr,
 			   enum pciehp_indicator attn)
-- 
2.21.0

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

[Denis Efremov]

This patch replaces pciehp_green_led_{on,off,blink}() with
pciehp_set_indicators().

Reviewed-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/hotplug/pciehp.h     | 12 ++++++++---
 drivers/pci/hotplug/pciehp_hpc.c | 36 --------------------------------
 2 files changed, 9 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 9a2a2d0db9d2..6cdcb1ca561f 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -183,9 +183,6 @@ void pciehp_set_indicators(struct controller *ctrl,
 			   enum pciehp_indicator 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);
@@ -202,6 +199,15 @@ int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
 #define pciehp_set_attention_status(ctrl, status) \
 	pciehp_set_indicators(ctrl, PWR_NONE, (status == 0 ? ATTN_OFF : status))
 
+#define pciehp_green_led_on(ctrl) \
+	pciehp_set_indicators(ctrl, PWR_ON, ATTN_NONE)
+
+#define pciehp_green_led_off(ctrl) \
+	pciehp_set_indicators(ctrl, PWR_OFF, ATTN_NONE)
+
+#define pciehp_green_led_blink(ctrl) \
+	pciehp_set_indicators(ctrl, PWR_BLINK, ATTN_NONE)
+
 static inline const char *slot_name(struct controller *ctrl)
 {
 	return hotplug_slot_name(&ctrl->hotplug_slot);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index fb4bea16063a..8362d24405fd 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -456,42 +456,6 @@ void pciehp_set_indicators(struct controller *ctrl,
 		 cmd);
 }
 
-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 v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Lukas Wunner]

On Sun, Aug 11, 2019 at 10:59:41PM +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.
> enum pciehp_indicator introduced to switch between the indicators statuses.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Re: [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status()

[Lukas Wunner]

On Sun, Aug 11, 2019 at 10:59:43PM +0300, Denis Efremov wrote:
> +#define pciehp_set_attention_status(ctrl, status) \
> +	pciehp_set_indicators(ctrl, PWR_NONE, (status == 0 ? ATTN_OFF : status))

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Good catch regarding the translation of the "off" value that's needed here.

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

[sathyanarayanan kuppuswamy]

Hi,

On 8/11/19 12:59 PM, 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.
> enum pciehp_indicator introduced to switch between the indicators statuses.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>   drivers/pci/hotplug/pciehp.h     | 14 ++++++++++++
>   drivers/pci/hotplug/pciehp_hpc.c | 38 ++++++++++++++++++++++++++++++++
>   include/uapi/linux/pci_regs.h    |  2 ++
>   3 files changed, 54 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 8c51a04b8083..17305a6f01f1 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -150,6 +150,17 @@ struct controller {
>   #define NO_CMD_CMPL(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
>   #define PSN(ctrl)		(((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)
>   
> +enum pciehp_indicator {
> +	ATTN_NONE  = -1,
> +	ATTN_ON    =  1,
> +	ATTN_BLINK =  2,
> +	ATTN_OFF   =  3,
> +	PWR_NONE   = -1,
> +	PWR_ON     =  1,
> +	PWR_BLINK  =  2,
> +	PWR_OFF    =  3,
> +};
> +
>   void pciehp_request(struct controller *ctrl, int action);
>   void pciehp_handle_button_press(struct controller *ctrl);
>   void pciehp_handle_disable_request(struct controller *ctrl);
> @@ -167,6 +178,9 @@ 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,
> +			   enum pciehp_indicator pwr,
> +			   enum pciehp_indicator 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..5a690b1579ec 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -443,6 +443,44 @@ 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,
> +			   enum pciehp_indicator pwr,
> +			   enum pciehp_indicator attn)
> +{
> +	u16 cmd = 0, mask = 0;
> +
> +	if ((!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
> +	    (!ATTN_LED(ctrl) || attn == ATTN_NONE))
> +		return;
> +
> +	switch (pwr) {
> +	case PWR_ON:
> +	case PWR_BLINK:
> +	case PWR_OFF:
> +		cmd = pwr << PCI_EXP_SLTCTL_PWR_IND_OFFSET;
> +		mask = PCI_EXP_SLTCTL_PIC;
> +		break;
> +	default:
> +		break;
> +	}
Do we need to switch case here ? if (pwr > 0) {} should work right ?
> +
> +	switch (attn) {
> +	case ATTN_ON:
> +	case ATTN_BLINK:
> +	case ATTN_OFF:
> +		cmd |= attn << PCI_EXP_SLTCTL_ATTN_IND_OFFSET;
> +		mask |= PCI_EXP_SLTCTL_AIC;
> +		break;
> +	default:
> +		break;
> +	}
Same here. if (attn > 0) {}
> +
> +	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..18722d1f54a0 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_OFFSET 0x6   /* Attention Indicator Offset */
>   #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_OFFSET 0x8    /* Power Indicator Offset */
>   #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 */

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

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

[sathyanarayanan kuppuswamy]

Hi,

On 8/11/19 12:59 PM, 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: 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 | 14 +++++---------
>   drivers/pci/hotplug/pciehp_hpc.c  |  3 +--
>   2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 631ced0ab28a..258a4060466d 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -42,8 +42,7 @@ 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, PWR_OFF, ATTN_ON);
>   }
>   
>   /**
> @@ -90,8 +89,7 @@ static int board_added(struct controller *ctrl)
>   		}
>   	}
>   
> -	pciehp_green_led_on(ctrl);
> -	pciehp_set_attention_status(ctrl, 0);
> +	pciehp_set_indicators(ctrl, PWR_ON, ATTN_OFF);
>   	return 0;
>   
>   err_exit:
> @@ -172,8 +170,7 @@ 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, PWR_BLINK, ATTN_OFF);
>   		schedule_delayed_work(&ctrl->button_work, 5 * HZ);
>   		break;
>   	case BLINKINGOFF_STATE:
> @@ -187,12 +184,11 @@ 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, PWR_ON, ATTN_OFF);
>   		} else {
>   			ctrl->state = OFF_STATE;
> -			pciehp_green_led_off(ctrl);
> +			pciehp_set_indicators(ctrl, PWR_OFF, ATTN_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 5a690b1579ec..68b880bc30db 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -676,8 +676,7 @@ 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, PWR_OFF, ATTN_ON);
>   	}
>   
>   	/*

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status()

[sathyanarayanan kuppuswamy]

Hi,

On 8/11/19 12:59 PM, Denis Efremov wrote:
> This patch replaces pciehp_set_attention_status() with
> pciehp_set_indicators().
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>   drivers/pci/hotplug/pciehp.h     |  4 +++-
>   drivers/pci/hotplug/pciehp_hpc.c | 25 -------------------------
>   2 files changed, 3 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 17305a6f01f1..9a2a2d0db9d2 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -181,7 +181,6 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status);
>   void pciehp_set_indicators(struct controller *ctrl,
>   			   enum pciehp_indicator pwr,
>   			   enum pciehp_indicator 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);
> @@ -200,6 +199,9 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status);
>   int pciehp_set_raw_indicator_status(struct hotplug_slot *h_slot, u8 status);
>   int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
>   
> +#define pciehp_set_attention_status(ctrl, status) \
> +	pciehp_set_indicators(ctrl, PWR_NONE, (status == 0 ? ATTN_OFF : status))
> +
>   static inline const char *slot_name(struct controller *ctrl)
>   {
>   	return hotplug_slot_name(&ctrl->hotplug_slot);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 68b880bc30db..fb4bea16063a 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,
>   			   enum pciehp_indicator pwr,
>   			   enum pciehp_indicator attn)

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

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

[sathyanarayanan kuppuswamy]

Hi,

On 8/11/19 12:59 PM, Denis Efremov wrote:
> This patch replaces pciehp_green_led_{on,off,blink}() with
> pciehp_set_indicators().
>
> 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.h     | 12 ++++++++---
>   drivers/pci/hotplug/pciehp_hpc.c | 36 --------------------------------
>   2 files changed, 9 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 9a2a2d0db9d2..6cdcb1ca561f 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -183,9 +183,6 @@ void pciehp_set_indicators(struct controller *ctrl,
>   			   enum pciehp_indicator 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);
> @@ -202,6 +199,15 @@ int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
>   #define pciehp_set_attention_status(ctrl, status) \
>   	pciehp_set_indicators(ctrl, PWR_NONE, (status == 0 ? ATTN_OFF : status))
>   
> +#define pciehp_green_led_on(ctrl) \
> +	pciehp_set_indicators(ctrl, PWR_ON, ATTN_NONE)
> +
> +#define pciehp_green_led_off(ctrl) \
> +	pciehp_set_indicators(ctrl, PWR_OFF, ATTN_NONE)
> +
> +#define pciehp_green_led_blink(ctrl) \
> +	pciehp_set_indicators(ctrl, PWR_BLINK, ATTN_NONE)
> +
>   static inline const char *slot_name(struct controller *ctrl)
>   {
>   	return hotplug_slot_name(&ctrl->hotplug_slot);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index fb4bea16063a..8362d24405fd 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -456,42 +456,6 @@ void pciehp_set_indicators(struct controller *ctrl,
>   		 cmd);
>   }
>   
> -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);

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

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

[sathyanarayanan kuppuswamy]

Hi,

On 8/12/19 11:25 AM, sathyanarayanan kuppuswamy wrote:
> Hi,
>
> On 8/11/19 12:59 PM, 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.
>> enum pciehp_indicator introduced to switch between the indicators 
>> statuses.
>>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> ---
>>   drivers/pci/hotplug/pciehp.h     | 14 ++++++++++++
>>   drivers/pci/hotplug/pciehp_hpc.c | 38 ++++++++++++++++++++++++++++++++
>>   include/uapi/linux/pci_regs.h    |  2 ++
>>   3 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
>> index 8c51a04b8083..17305a6f01f1 100644
>> --- a/drivers/pci/hotplug/pciehp.h
>> +++ b/drivers/pci/hotplug/pciehp.h
>> @@ -150,6 +150,17 @@ struct controller {
>>   #define NO_CMD_CMPL(ctrl)    ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
>>   #define PSN(ctrl)        (((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) 
>> >> 19)
>>   +enum pciehp_indicator {
>> +    ATTN_NONE  = -1,
>> +    ATTN_ON    =  1,
>> +    ATTN_BLINK =  2,
>> +    ATTN_OFF   =  3,
>> +    PWR_NONE   = -1,
>> +    PWR_ON     =  1,
>> +    PWR_BLINK  =  2,
>> +    PWR_OFF    =  3,
>> +};
>> +
>>   void pciehp_request(struct controller *ctrl, int action);
>>   void pciehp_handle_button_press(struct controller *ctrl);
>>   void pciehp_handle_disable_request(struct controller *ctrl);
>> @@ -167,6 +178,9 @@ 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,
>> +               enum pciehp_indicator pwr,
>> +               enum pciehp_indicator 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..5a690b1579ec 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -443,6 +443,44 @@ 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,
>> +               enum pciehp_indicator pwr,
>> +               enum pciehp_indicator attn)
>> +{
>> +    u16 cmd = 0, mask = 0;
>> +
>> +    if ((!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
>> +        (!ATTN_LED(ctrl) || attn == ATTN_NONE))
>> +        return;

Also I think this condition needs to expand to handle the case whether 
pwr != PWR_NONE and !PWR_LED(ctrl) is true.

you need to return for case, pwr = PWR_ON, !PWR_LED(ctrl)=true , 
!ATTN_LED(ctrl)=false, attn=on

>> +
>> +    switch (pwr) {
>> +    case PWR_ON:
>> +    case PWR_BLINK:
>> +    case PWR_OFF:
>> +        cmd = pwr << PCI_EXP_SLTCTL_PWR_IND_OFFSET;
>> +        mask = PCI_EXP_SLTCTL_PIC;
>> +        break;
>> +    default:
>> +        break;
>> +    }
> Do we need to switch case here ? if (pwr > 0) {} should work right ?
>> +
>> +    switch (attn) {
>> +    case ATTN_ON:
>> +    case ATTN_BLINK:
>> +    case ATTN_OFF:
>> +        cmd |= attn << PCI_EXP_SLTCTL_ATTN_IND_OFFSET;
>> +        mask |= PCI_EXP_SLTCTL_AIC;
>> +        break;
>> +    default:
>> +        break;
>> +    }
> Same here. if (attn > 0) {}
>> +
>> +    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..18722d1f54a0 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_OFFSET 0x6   /* Attention Indicator 
>> Offset */
>>   #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_OFFSET 0x8    /* Power Indicator 
>> Offset */
>>   #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 */
>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

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

[Bjorn Helgaas]

On Sun, Aug 11, 2019 at 10:59:40PM +0300, 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.
> 
> 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: Replace pciehp_set_attention_status()
>   PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
> 
>  drivers/pci/hotplug/pciehp.h      | 30 +++++++++++--
>  drivers/pci/hotplug/pciehp_ctrl.c | 14 +++---
>  drivers/pci/hotplug/pciehp_hpc.c  | 74 +++++++++++--------------------
>  include/uapi/linux/pci_regs.h     |  2 +
>  4 files changed, 58 insertions(+), 62 deletions(-)

I really like these; thanks a lot for doing them.  A few comments
later...

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

[Bjorn Helgaas]

On Sun, Aug 11, 2019 at 10:59:41PM +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.
> enum pciehp_indicator introduced to switch between the indicators statuses.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  drivers/pci/hotplug/pciehp.h     | 14 ++++++++++++
>  drivers/pci/hotplug/pciehp_hpc.c | 38 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/pci_regs.h    |  2 ++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 8c51a04b8083..17305a6f01f1 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -150,6 +150,17 @@ struct controller {
>  #define NO_CMD_CMPL(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
>  #define PSN(ctrl)		(((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)
>  
> +enum pciehp_indicator {
> +	ATTN_NONE  = -1,
> +	ATTN_ON    =  1,
> +	ATTN_BLINK =  2,
> +	ATTN_OFF   =  3,
> +	PWR_NONE   = -1,
> +	PWR_ON     =  1,
> +	PWR_BLINK  =  2,
> +	PWR_OFF    =  3,
> +};
> +
>  void pciehp_request(struct controller *ctrl, int action);
>  void pciehp_handle_button_press(struct controller *ctrl);
>  void pciehp_handle_disable_request(struct controller *ctrl);
> @@ -167,6 +178,9 @@ 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,
> +			   enum pciehp_indicator pwr,
> +			   enum pciehp_indicator 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..5a690b1579ec 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -443,6 +443,44 @@ 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,
> +			   enum pciehp_indicator pwr,
> +			   enum pciehp_indicator attn)
> +{
> +	u16 cmd = 0, mask = 0;
> +
> +	if ((!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
> +	    (!ATTN_LED(ctrl) || attn == ATTN_NONE))
> +		return;
> +
> +	switch (pwr) {
> +	case PWR_ON:
> +	case PWR_BLINK:
> +	case PWR_OFF:
> +		cmd = pwr << PCI_EXP_SLTCTL_PWR_IND_OFFSET;
> +		mask = PCI_EXP_SLTCTL_PIC;

Since you initialized cmd and mask above, I would use "|=" here so PWR
and ATTN are handled identically.

One thing I don't like about this is the implicit connection between
the pciehp_indicator enum values and the PCI_EXP_SLTCTL_ATTN_IND_ON
definitions and the fact that grepping for PCI_EXP_SLTCTL_ATTN_IND_ON
is now useless.  It was *mostly* useless before, but here we might
have a chance to make it useful.

What would it look like if you had the caller pass in those values
directly, e.g.,

  pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
                              PCI_EXP_SLTCTL_ATTN_IND_ON);

I guess you'd still have to define a "NONE" value to mean "leave the
indicator unchanged", but you wouldn't need to define the shift, and
it would make it one step easier to find places that turn on an
indicator.

> +		break;
> +	default:
> +		break;

The "default:" case is superfluous.

> +	}
> +
> +	switch (attn) {
> +	case ATTN_ON:
> +	case ATTN_BLINK:
> +	case ATTN_OFF:
> +		cmd |= attn << PCI_EXP_SLTCTL_ATTN_IND_OFFSET;
> +		mask |= PCI_EXP_SLTCTL_AIC;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	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..18722d1f54a0 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_OFFSET 0x6   /* Attention Indicator Offset */

This is typically named *_SHIFT and expressed in decimal.

>  #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_OFFSET 0x8    /* Power Indicator Offset */
>  #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 v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()

[Bjorn Helgaas]

On Sun, Aug 11, 2019 at 10:59:44PM +0300, Denis Efremov wrote:
> This patch replaces pciehp_green_led_{on,off,blink}() with
> pciehp_set_indicators().
> 
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  drivers/pci/hotplug/pciehp.h     | 12 ++++++++---
>  drivers/pci/hotplug/pciehp_hpc.c | 36 --------------------------------
>  2 files changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 9a2a2d0db9d2..6cdcb1ca561f 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -183,9 +183,6 @@ void pciehp_set_indicators(struct controller *ctrl,
>  			   enum pciehp_indicator 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);
> @@ -202,6 +199,15 @@ int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
>  #define pciehp_set_attention_status(ctrl, status) \
>  	pciehp_set_indicators(ctrl, PWR_NONE, (status == 0 ? ATTN_OFF : status))
>  
> +#define pciehp_green_led_on(ctrl) \
> +	pciehp_set_indicators(ctrl, PWR_ON, ATTN_NONE)
> +
> +#define pciehp_green_led_off(ctrl) \
> +	pciehp_set_indicators(ctrl, PWR_OFF, ATTN_NONE)
> +
> +#define pciehp_green_led_blink(ctrl) \
> +	pciehp_set_indicators(ctrl, PWR_BLINK, ATTN_NONE)

You must have a reason, but why didn't you completely remove
pciehp_green_led_on(), etc, and change the callers to use
pciehp_set_indicators() instead?

>  static inline const char *slot_name(struct controller *ctrl)
>  {
>  	return hotplug_slot_name(&ctrl->hotplug_slot);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index fb4bea16063a..8362d24405fd 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -456,42 +456,6 @@ void pciehp_set_indicators(struct controller *ctrl,
>  		 cmd);
>  }
>  
> -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 v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators

[Lukas Wunner]

On Mon, Aug 12, 2019 at 11:49:23AM -0700, sathyanarayanan kuppuswamy wrote:
> > On 8/11/19 12:59 PM, Denis Efremov wrote:
> > > +    if ((!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
> > > +        (!ATTN_LED(ctrl) || attn == ATTN_NONE))
> > > +        return;
> 
> Also I think this condition needs to expand to handle the case whether pwr
> != PWR_NONE and !PWR_LED(ctrl) is true.
> 
> you need to return for case, pwr = PWR_ON, !PWR_LED(ctrl)=true ,
> !ATTN_LED(ctrl)=false, attn=on

Why should we return in that case?  We need to update the Attention
Indicator Control to On.

Thanks,

Lukas

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

[sathyanarayanan kuppuswamy]

On 8/12/19 1:40 PM, Lukas Wunner wrote:
> On Mon, Aug 12, 2019 at 11:49:23AM -0700, sathyanarayanan kuppuswamy wrote:
>>> On 8/11/19 12:59 PM, Denis Efremov wrote:
>>>> +    if ((!PWR_LED(ctrl)  || pwr  == PWR_NONE) &&
>>>> +        (!ATTN_LED(ctrl) || attn == ATTN_NONE))
>>>> +        return;
>> Also I think this condition needs to expand to handle the case whether pwr
>> != PWR_NONE and !PWR_LED(ctrl) is true.
>>
>> you need to return for case, pwr = PWR_ON, !PWR_LED(ctrl)=true ,
>> !ATTN_LED(ctrl)=false, attn=on
> Why should we return in that case?  We need to update the Attention
> Indicator Control to On.

Attempting to PWR_ON when !PWR_LED(ctrl) is true is incorrect right ? 
Even if you don't want to return (to handle ATTN part of the function), 
may be you should skip updating PWR mask and cmd for this case.

>
> Thanks,
>
> Lukas
>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

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

[Denis Efremov]

> You must have a reason, but why didn't you completely remove
> pciehp_green_led_on(), etc, and change the callers to use
> pciehp_set_indicators() instead?

Well, I don't have the exact reason here. I thought that it would be nice to preserve
an existing interface and to hide some implementation details (e.g., status of the
second indicator). I could completely remove pciehp_green_led_{on,off,blink}() and
pciehp_set_attention_status() in v3 if you prefer.

Thanks, 
Denis

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

[Bjorn Helgaas]

On Tue, Aug 13, 2019 at 12:14:08AM +0300, Denis Efremov wrote:
> > You must have a reason, but why didn't you completely remove
> > pciehp_green_led_on(), etc, and change the callers to use
> > pciehp_set_indicators() instead?
> 
> Well, I don't have the exact reason here. I thought that it would be nice to preserve
> an existing interface and to hide some implementation details (e.g., status of the
> second indicator). I could completely remove pciehp_green_led_{on,off,blink}() and
> pciehp_set_attention_status() in v3 if you prefer.

I might be missing something, but I do think I would prefer to
completely remove pciehp_green_led_{on,off,blink}() and
pciehp_set_attention_status().  Then we'd have exactly one interface
to change indicator state.