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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Remove pciehp_set_attention_status() and use pciehp_set_indicators()
instead, since the code is mostly the same.
Signed-off-by: Denis Efremov <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Lukas Wunner <[email protected]>
Signed-off-by: Denis Efremov <[email protected]>
---
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
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
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(-)
>
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 <[email protected]>
> ---
> 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
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?
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 <[email protected]>
> Reviewed-by: Lukas Wunner <[email protected]>
> Signed-off-by: Denis Efremov <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
> 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
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
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
On Tue, Aug 20, 2019 at 2:17 AM Denis Efremov <[email protected]> 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
On Tue, Aug 20, 2019 at 2:07 AM Denis Efremov <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Tue, Aug 20, 2019 at 2:09 AM Denis Efremov <[email protected]> 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 <[email protected]>
> ---
> 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.
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 <[email protected]>, <[email protected]>"
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