* [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
2019-08-11 19:59 [PATCH v2 0/4] Simplify PCIe hotplug indicator control Denis Efremov
@ 2019-08-11 19:59 ` Denis Efremov
2019-08-12 6:30 ` Lukas Wunner
` (2 more replies)
2019-08-11 19:59 ` [PATCH v2 2/4] PCI: pciehp: Switch LED indicators with a single write Denis Efremov
` (3 subsequent siblings)
4 siblings, 3 replies; 19+ messages in thread
From: Denis Efremov @ 2019-08-11 19:59 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
2019-08-11 19:59 ` [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
@ 2019-08-12 6:30 ` Lukas Wunner
2019-08-12 18:25 ` sathyanarayanan kuppuswamy
2019-08-12 20:03 ` Bjorn Helgaas
2 siblings, 0 replies; 19+ messages in thread
From: Lukas Wunner @ 2019-08-12 6:30 UTC (permalink / raw)
To: Denis Efremov; +Cc: Bjorn Helgaas, linux-pci, linux-kernel
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
2019-08-11 19:59 ` [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
2019-08-12 6:30 ` Lukas Wunner
@ 2019-08-12 18:25 ` sathyanarayanan kuppuswamy
2019-08-12 18:49 ` sathyanarayanan kuppuswamy
2019-08-12 20:03 ` Bjorn Helgaas
2 siblings, 1 reply; 19+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-12 18:25 UTC (permalink / raw)
To: Denis Efremov, Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
2019-08-12 18:25 ` sathyanarayanan kuppuswamy
@ 2019-08-12 18:49 ` sathyanarayanan kuppuswamy
2019-08-12 20:40 ` Lukas Wunner
0 siblings, 1 reply; 19+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-12 18:49 UTC (permalink / raw)
To: Denis Efremov, Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
2019-08-12 18:49 ` sathyanarayanan kuppuswamy
@ 2019-08-12 20:40 ` Lukas Wunner
2019-08-12 21:03 ` sathyanarayanan kuppuswamy
0 siblings, 1 reply; 19+ messages in thread
From: Lukas Wunner @ 2019-08-12 20:40 UTC (permalink / raw)
To: sathyanarayanan kuppuswamy
Cc: Denis Efremov, Bjorn Helgaas, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
2019-08-12 20:40 ` Lukas Wunner
@ 2019-08-12 21:03 ` sathyanarayanan kuppuswamy
0 siblings, 0 replies; 19+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-12 21:03 UTC (permalink / raw)
To: Lukas Wunner; +Cc: Denis Efremov, Bjorn Helgaas, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
2019-08-11 19:59 ` [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
2019-08-12 6:30 ` Lukas Wunner
2019-08-12 18:25 ` sathyanarayanan kuppuswamy
@ 2019-08-12 20:03 ` Bjorn Helgaas
2 siblings, 0 replies; 19+ messages in thread
From: Bjorn Helgaas @ 2019-08-12 20:03 UTC (permalink / raw)
To: Denis Efremov; +Cc: Lukas Wunner, linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/4] PCI: pciehp: Switch LED indicators with a single write
2019-08-11 19:59 [PATCH v2 0/4] Simplify PCIe hotplug indicator control Denis Efremov
2019-08-11 19:59 ` [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
@ 2019-08-11 19:59 ` Denis Efremov
2019-08-12 18:27 ` sathyanarayanan kuppuswamy
2019-08-11 19:59 ` [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Denis Efremov @ 2019-08-11 19:59 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 2/4] PCI: pciehp: Switch LED indicators with a single write
2019-08-11 19:59 ` [PATCH v2 2/4] PCI: pciehp: Switch LED indicators with a single write Denis Efremov
@ 2019-08-12 18:27 ` sathyanarayanan kuppuswamy
0 siblings, 0 replies; 19+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-12 18:27 UTC (permalink / raw)
To: Denis Efremov, Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status()
2019-08-11 19:59 [PATCH v2 0/4] Simplify PCIe hotplug indicator control Denis Efremov
2019-08-11 19:59 ` [PATCH v2 1/4] PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators Denis Efremov
2019-08-11 19:59 ` [PATCH v2 2/4] PCI: pciehp: Switch LED indicators with a single write Denis Efremov
@ 2019-08-11 19:59 ` Denis Efremov
2019-08-12 6:32 ` Lukas Wunner
2019-08-12 18:28 ` sathyanarayanan kuppuswamy
2019-08-11 19:59 ` [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
2019-08-12 20:02 ` [PATCH v2 0/4] Simplify PCIe hotplug indicator control Bjorn Helgaas
4 siblings, 2 replies; 19+ messages in thread
From: Denis Efremov @ 2019-08-11 19:59 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status()
2019-08-11 19:59 ` [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
@ 2019-08-12 6:32 ` Lukas Wunner
2019-08-12 18:28 ` sathyanarayanan kuppuswamy
1 sibling, 0 replies; 19+ messages in thread
From: Lukas Wunner @ 2019-08-12 6:32 UTC (permalink / raw)
To: Denis Efremov; +Cc: Bjorn Helgaas, linux-pci, linux-kernel
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status()
2019-08-11 19:59 ` [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
2019-08-12 6:32 ` Lukas Wunner
@ 2019-08-12 18:28 ` sathyanarayanan kuppuswamy
1 sibling, 0 replies; 19+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-12 18:28 UTC (permalink / raw)
To: Denis Efremov, Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
2019-08-11 19:59 [PATCH v2 0/4] Simplify PCIe hotplug indicator control Denis Efremov
` (2 preceding siblings ...)
2019-08-11 19:59 ` [PATCH v2 3/4] PCI: pciehp: Replace pciehp_set_attention_status() Denis Efremov
@ 2019-08-11 19:59 ` Denis Efremov
2019-08-12 18:45 ` sathyanarayanan kuppuswamy
2019-08-12 20:03 ` Bjorn Helgaas
2019-08-12 20:02 ` [PATCH v2 0/4] Simplify PCIe hotplug indicator control Bjorn Helgaas
4 siblings, 2 replies; 19+ messages in thread
From: Denis Efremov @ 2019-08-11 19:59 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
2019-08-11 19:59 ` [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
@ 2019-08-12 18:45 ` sathyanarayanan kuppuswamy
2019-08-12 20:03 ` Bjorn Helgaas
1 sibling, 0 replies; 19+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-12 18:45 UTC (permalink / raw)
To: Denis Efremov, Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
2019-08-11 19:59 ` [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
2019-08-12 18:45 ` sathyanarayanan kuppuswamy
@ 2019-08-12 20:03 ` Bjorn Helgaas
2019-08-12 21:14 ` Denis Efremov
1 sibling, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2019-08-12 20:03 UTC (permalink / raw)
To: Denis Efremov; +Cc: Lukas Wunner, linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
2019-08-12 20:03 ` Bjorn Helgaas
@ 2019-08-12 21:14 ` Denis Efremov
2019-08-12 22:12 ` Bjorn Helgaas
0 siblings, 1 reply; 19+ messages in thread
From: Denis Efremov @ 2019-08-12 21:14 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lukas Wunner, linux-pci, linux-kernel
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}()
2019-08-12 21:14 ` Denis Efremov
@ 2019-08-12 22:12 ` Bjorn Helgaas
0 siblings, 0 replies; 19+ messages in thread
From: Bjorn Helgaas @ 2019-08-12 22:12 UTC (permalink / raw)
To: Denis Efremov; +Cc: Lukas Wunner, linux-pci, linux-kernel
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/4] Simplify PCIe hotplug indicator control
2019-08-11 19:59 [PATCH v2 0/4] Simplify PCIe hotplug indicator control Denis Efremov
` (3 preceding siblings ...)
2019-08-11 19:59 ` [PATCH v2 4/4] PCI: pciehp: Replace pciehp_green_led_{on,off,blink}() Denis Efremov
@ 2019-08-12 20:02 ` Bjorn Helgaas
4 siblings, 0 replies; 19+ messages in thread
From: Bjorn Helgaas @ 2019-08-12 20:02 UTC (permalink / raw)
To: Denis Efremov; +Cc: Lukas Wunner, linux-pci, linux-kernel
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...
^ permalink raw reply [flat|nested] 19+ messages in thread