diff mbox series

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

Message ID 20190811195944.23765-2-efremov@linux.com (mailing list archive)
State Superseded, archived
Headers show
Series Simplify PCIe hotplug indicator control | expand

Commit Message

Denis Efremov Aug. 11, 2019, 7:59 p.m. UTC 
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(+)

Comments

Lukas Wunner Aug. 12, 2019, 6:30 a.m. UTC | #1 
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>
Kuppuswamy Sathyanarayanan Aug. 12, 2019, 6:25 p.m. UTC | #2 
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 */
Kuppuswamy Sathyanarayanan Aug. 12, 2019, 6:49 p.m. UTC | #3 
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 */
>
Bjorn Helgaas Aug. 12, 2019, 8:03 p.m. UTC | #4 
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
>
Lukas Wunner Aug. 12, 2019, 8:40 p.m. UTC | #5 
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
Kuppuswamy Sathyanarayanan Aug. 12, 2019, 9:03 p.m. UTC | #6 
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
>
diff mbox series

Patch

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 */