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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Lukas Wunner <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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
>
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
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