| Message ID | 20190811132945.12426-2-efremov@linux.com |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Simplify PCIe hotplug indicator control | expand |
On Sun, Aug 11, 2019 at 04:29:42PM +0300, Denis Efremov wrote: > This commit adds pciehp_set_indicators() to set power and attention Nit: "This commit ..." is superfluous, just say "Add ...". > indicators with a single register write. enum pciehp_indicator > introduced to switch between the indicators statuses. Attention > indicator statuses are explicitly set with values in the enum to > transparently comply with pciehp_set_attention_status() from > pciehp_hpc.c and set_attention_status() from pciehp_core.c Please document the motivation of the change (the "why"). One motivation might be to avoid waiting twice for Command Complete. Another motivation might be to change both LEDs at the same time in a glitch-free manner, thereby achieving a smoother user experience. > --- a/drivers/pci/hotplug/pciehp.h > +++ b/drivers/pci/hotplug/pciehp.h > +enum pciehp_indicator { > + // Explicit values to match set_attention_status interface Kernel coding style is typically /* */, not //. > + ATTN_NONE = -1, > + ATTN_OFF = 0, > + ATTN_ON = 1, > + ATTN_BLINK = 2, > + PWR_NONE, > + PWR_OFF, > + PWR_ON, > + PWR_BLINK > +}; I'd suggest using the same values that are written to the register, i.e.: 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, }; Then you can just shift the values to the proper offset and don't need a translation between enum pciehp_indicator and register value. > +void pciehp_set_indicators(struct controller *ctrl, > + enum pciehp_indicator pwr, > + enum pciehp_indicator attn) > +{ > + u16 cmd = 0; > + bool pwr_none = (pwr == PWR_NONE); > + bool attn_none = (attn == ATTN_NONE); > + bool pwr_led = PWR_LED(ctrl); > + bool attn_led = ATTN_LED(ctrl); > + > + if ((!pwr_led && !attn_led) || (pwr_none && attn_none) || > + (!attn_led && pwr_none) || (!pwr_led && attn_none)) > + return; I'd suggest the following simpler construct: if (!PWR_LED(ctrl) || pwr == PWR_NONE) && !ATTN_LED(ctrl) || attn == ATTN_NONE)) return; > + switch (pwr) { > + case PWR_OFF: > + cmd = PCI_EXP_SLTCTL_PWR_IND_OFF; > + break; > + case PWR_ON: > + cmd = PCI_EXP_SLTCTL_PWR_IND_ON; > + break; > + case PWR_BLINK: > + cmd = PCI_EXP_SLTCTL_PWR_IND_BLINK; > + break; > + default: > + break; > + } If you follow my suggestion above to use the register value for "pwr", then you can just fold all three cases into one, i.e. case PWR_ON: case PWR_BLINK: case PWR_OFF: cmd = pwr << 8; mask |= PCI_EXP_SLTCTL_PIC; break; Feel free to add a PCI_EXP_SLTCTL_PWR_IND_OFFSET macro for the offset 8. Add a "u16 mask = 0" to the top of the function and pass "mask" to pcie_write_cmd_nowait(). Thanks, Lukas
On Sun, Aug 11, 2019 at 06:07:55PM +0200, Lukas Wunner wrote: > if (!PWR_LED(ctrl) || pwr == PWR_NONE) && > !ATTN_LED(ctrl) || attn == ATTN_NONE)) > return; Forgot an opening brace in two spots here, sorry.
Thank you for the review, I will send v2. On 11.08.2019 19:07, Lukas Wunner wrote: > On Sun, Aug 11, 2019 at 04:29:42PM +0300, Denis Efremov wrote: >> This commit adds pciehp_set_indicators() to set power and attention > > Nit: "This commit ..." is superfluous, just say "Add ...". > > >> indicators with a single register write. enum pciehp_indicator >> introduced to switch between the indicators statuses. Attention >> indicator statuses are explicitly set with values in the enum to >> transparently comply with pciehp_set_attention_status() from >> pciehp_hpc.c and set_attention_status() from pciehp_core.c > > Please document the motivation of the change (the "why"). > > One motivation might be to avoid waiting twice for Command Complete. > > Another motivation might be to change both LEDs at the same time > in a glitch-free manner, thereby achieving a smoother user experience. > > >> --- a/drivers/pci/hotplug/pciehp.h >> +++ b/drivers/pci/hotplug/pciehp.h >> +enum pciehp_indicator { >> + // Explicit values to match set_attention_status interface > > Kernel coding style is typically /* */, not //. > > >> + ATTN_NONE = -1, >> + ATTN_OFF = 0, >> + ATTN_ON = 1, >> + ATTN_BLINK = 2, >> + PWR_NONE, >> + PWR_OFF, >> + PWR_ON, >> + PWR_BLINK >> +}; > > I'd suggest using the same values that are written to the register, i.e.: > > 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, > }; > > Then you can just shift the values to the proper offset and don't need > a translation between enum pciehp_indicator and register value. > > >> +void pciehp_set_indicators(struct controller *ctrl, >> + enum pciehp_indicator pwr, >> + enum pciehp_indicator attn) >> +{ >> + u16 cmd = 0; >> + bool pwr_none = (pwr == PWR_NONE); >> + bool attn_none = (attn == ATTN_NONE); >> + bool pwr_led = PWR_LED(ctrl); >> + bool attn_led = ATTN_LED(ctrl); >> + >> + if ((!pwr_led && !attn_led) || (pwr_none && attn_none) || >> + (!attn_led && pwr_none) || (!pwr_led && attn_none)) >> + return; > > I'd suggest the following simpler construct: > > if (!PWR_LED(ctrl) || pwr == PWR_NONE) && > !ATTN_LED(ctrl) || attn == ATTN_NONE)) > return; > > >> + switch (pwr) { >> + case PWR_OFF: >> + cmd = PCI_EXP_SLTCTL_PWR_IND_OFF; >> + break; >> + case PWR_ON: >> + cmd = PCI_EXP_SLTCTL_PWR_IND_ON; >> + break; >> + case PWR_BLINK: >> + cmd = PCI_EXP_SLTCTL_PWR_IND_BLINK; >> + break; >> + default: >> + break; >> + } > > If you follow my suggestion above to use the register value for "pwr", > then you can just fold all three cases into one, i.e. > > case PWR_ON: > case PWR_BLINK: > case PWR_OFF: > cmd = pwr << 8; > mask |= PCI_EXP_SLTCTL_PIC; > break; > > Feel free to add a PCI_EXP_SLTCTL_PWR_IND_OFFSET macro for the offset 8. > Add a "u16 mask = 0" to the top of the function and pass "mask" to > pcie_write_cmd_nowait(). > > Thanks, > > Lukas >
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h index 8c51a04b8083..92ff65132711 100644 --- a/drivers/pci/hotplug/pciehp.h +++ b/drivers/pci/hotplug/pciehp.h @@ -150,6 +150,18 @@ 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 { + // Explicit values to match set_attention_status interface + ATTN_NONE = -1, + ATTN_OFF = 0, + ATTN_ON = 1, + ATTN_BLINK = 2, + PWR_NONE, + PWR_OFF, + PWR_ON, + PWR_BLINK +}; + 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 +179,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..027e3864f632 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -443,6 +443,55 @@ 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; + bool pwr_none = (pwr == PWR_NONE); + bool attn_none = (attn == ATTN_NONE); + bool pwr_led = PWR_LED(ctrl); + bool attn_led = ATTN_LED(ctrl); + + if ((!pwr_led && !attn_led) || (pwr_none && attn_none) || + (!attn_led && pwr_none) || (!pwr_led && attn_none)) + return; + + switch (pwr) { + case PWR_OFF: + cmd = PCI_EXP_SLTCTL_PWR_IND_OFF; + break; + case PWR_ON: + cmd = PCI_EXP_SLTCTL_PWR_IND_ON; + break; + case PWR_BLINK: + cmd = PCI_EXP_SLTCTL_PWR_IND_BLINK; + break; + default: + break; + } + + switch (attn) { + case ATTN_OFF: + cmd |= PCI_EXP_SLTCTL_ATTN_IND_OFF; + break; + case ATTN_ON: + cmd |= PCI_EXP_SLTCTL_ATTN_IND_ON; + break; + case ATTN_BLINK: + cmd |= PCI_EXP_SLTCTL_ATTN_IND_BLINK; + break; + default: + break; + } + + pcie_write_cmd_nowait(ctrl, cmd, + PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC); + 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))
This commit adds pciehp_set_indicators() to set power and attention indicators with a single register write. enum pciehp_indicator introduced to switch between the indicators statuses. Attention indicator statuses are explicitly set with values in the enum to transparently comply with pciehp_set_attention_status() from pciehp_hpc.c and set_attention_status() from pciehp_core.c Signed-off-by: Denis Efremov <efremov@linux.com> --- drivers/pci/hotplug/pciehp.h | 15 ++++++++++ drivers/pci/hotplug/pciehp_hpc.c | 49 ++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+)