[v4,1/3] PCI: pciehp: Add support for disabling in-band presence
diff mbox series

Message ID 20191025190047.38130-2-stuart.w.hayes@gmail.com
State New
Delegated to: Bjorn Helgaas
Headers show
Series
  • PCI: pciehp: Do not turn off slot if presence comes up after link
Related show

Commit Message

Stuart Hayes Oct. 25, 2019, 7 p.m. UTC
From: Alexandru Gagniuc <mr.nuke.me@gmail.com>

The presence detect state (PDS) is normally a logical or of in-band and
out-of-band presence. As of PCIe 4.0, there is the option to disable
in-band presence so that the PDS bit always reflects the state of the
out-of-band presence.

The recommendation of the PCIe spec is to disable in-band presence
whenever supported.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/hotplug/pciehp.h     | 1 +
 drivers/pci/hotplug/pciehp_hpc.c | 9 ++++++++-
 include/uapi/linux/pci_regs.h    | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Oct. 25, 2019, 7:15 p.m. UTC | #1
On Fri, Oct 25, 2019 at 10:00 PM Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>
> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>
> The presence detect state (PDS) is normally a logical or of in-band and
> out-of-band presence. As of PCIe 4.0, there is the option to disable
> in-band presence so that the PDS bit always reflects the state of the
> out-of-band presence.
>
> The recommendation of the PCIe spec is to disable in-band presence
> whenever supported.
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  drivers/pci/hotplug/pciehp.h     | 1 +
>  drivers/pci/hotplug/pciehp_hpc.c | 9 ++++++++-
>  include/uapi/linux/pci_regs.h    | 2 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 654c972b8ea0..27e4cd6529b0 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -83,6 +83,7 @@ struct controller {
>         struct pcie_device *pcie;
>
>         u32 slot_cap;                           /* capabilities and quirks */
> +       unsigned int inband_presence_disabled:1;
>
>         u16 slot_ctrl;                          /* control register access */
>         struct mutex ctrl_lock;
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 1a522c1c4177..dc109d521f30 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
>  struct controller *pcie_init(struct pcie_device *dev)
>  {
>         struct controller *ctrl;
> -       u32 slot_cap, link_cap;
> +       u32 slot_cap, slot_cap2, link_cap;
>         u8 poweron;
>         struct pci_dev *pdev = dev->port;
>         struct pci_bus *subordinate = pdev->subordinate;
> @@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
>                 FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
>                 pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
>
> +       pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, &slot_cap2);
> +       if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
> +               pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
> +                                     PCI_EXP_SLTCTL_IBPD_DISABLE);
> +               ctrl->inband_presence_disabled = 1;
> +       }
> +
>         /*
>          * If empty slot's power status is on, turn power off.  The IRQ isn't
>          * requested yet, so avoid triggering a notification with this command.
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 29d6e93fd15e..ea1cf9546e4d 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -604,6 +604,7 @@
>  #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
>  #define  PCI_EXP_SLTCTL_EIC    0x0800  /* Electromechanical Interlock Control */
>  #define  PCI_EXP_SLTCTL_DLLSCE 0x1000  /* Data Link Layer State Changed Enable */
> +#define  PCI_EXP_SLTCTL_IBPD_DISABLE   0x4000 /* In-band PD disable */
>  #define PCI_EXP_SLTSTA         26      /* Slot Status */
>  #define  PCI_EXP_SLTSTA_ABP    0x0001  /* Attention Button Pressed */
>  #define  PCI_EXP_SLTSTA_PFD    0x0002  /* Power Fault Detected */
> @@ -676,6 +677,7 @@
>  #define PCI_EXP_LNKSTA2                50      /* Link Status 2 */
>  #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 52      /* v2 endpoints with link end here */
>  #define PCI_EXP_SLTCAP2                52      /* Slot Capabilities 2 */
> +#define  PCI_EXP_SLTCAP2_IBPD  0x0001  /* In-band PD Disable Supported */
>  #define PCI_EXP_SLTCTL2                56      /* Slot Control 2 */
>  #define PCI_EXP_SLTSTA2                58      /* Slot Status 2 */
>
> --
> 2.18.1
>
Bjorn Helgaas Nov. 27, 2019, 1:36 a.m. UTC | #2
On Fri, Oct 25, 2019 at 03:00:45PM -0400, Stuart Hayes wrote:
> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> 
> The presence detect state (PDS) is normally a logical or of in-band and
> out-of-band presence. As of PCIe 4.0, there is the option to disable
> in-band presence so that the PDS bit always reflects the state of the
> out-of-band presence.
> 
> The recommendation of the PCIe spec is to disable in-band presence
> whenever supported.

I think I'm fine with this patch, but I would like to include the
specific reference for this recommendation.  If you have it handy, I
can just insert it.

> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  drivers/pci/hotplug/pciehp.h     | 1 +
>  drivers/pci/hotplug/pciehp_hpc.c | 9 ++++++++-
>  include/uapi/linux/pci_regs.h    | 2 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 654c972b8ea0..27e4cd6529b0 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -83,6 +83,7 @@ struct controller {
>  	struct pcie_device *pcie;
>  
>  	u32 slot_cap;				/* capabilities and quirks */
> +	unsigned int inband_presence_disabled:1;
>  
>  	u16 slot_ctrl;				/* control register access */
>  	struct mutex ctrl_lock;
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 1a522c1c4177..dc109d521f30 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
>  struct controller *pcie_init(struct pcie_device *dev)
>  {
>  	struct controller *ctrl;
> -	u32 slot_cap, link_cap;
> +	u32 slot_cap, slot_cap2, link_cap;
>  	u8 poweron;
>  	struct pci_dev *pdev = dev->port;
>  	struct pci_bus *subordinate = pdev->subordinate;
> @@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
>  		FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
>  		pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
>  
> +	pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, &slot_cap2);
> +	if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
> +		pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
> +				      PCI_EXP_SLTCTL_IBPD_DISABLE);
> +		ctrl->inband_presence_disabled = 1;
> +	}
> +
>  	/*
>  	 * If empty slot's power status is on, turn power off.  The IRQ isn't
>  	 * requested yet, so avoid triggering a notification with this command.
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 29d6e93fd15e..ea1cf9546e4d 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -604,6 +604,7 @@
>  #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
>  #define  PCI_EXP_SLTCTL_EIC	0x0800	/* Electromechanical Interlock Control */
>  #define  PCI_EXP_SLTCTL_DLLSCE	0x1000	/* Data Link Layer State Changed Enable */
> +#define  PCI_EXP_SLTCTL_IBPD_DISABLE	0x4000 /* In-band PD disable */
>  #define PCI_EXP_SLTSTA		26	/* Slot Status */
>  #define  PCI_EXP_SLTSTA_ABP	0x0001	/* Attention Button Pressed */
>  #define  PCI_EXP_SLTSTA_PFD	0x0002	/* Power Fault Detected */
> @@ -676,6 +677,7 @@
>  #define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
>  #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	52	/* v2 endpoints with link end here */
>  #define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */
> +#define  PCI_EXP_SLTCAP2_IBPD	0x0001	/* In-band PD Disable Supported */
>  #define PCI_EXP_SLTCTL2		56	/* Slot Control 2 */
>  #define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */
>  
> -- 
> 2.18.1
>
Stuart Hayes Nov. 27, 2019, 2:19 a.m. UTC | #3
On Tue, Nov 26, 2019 at 7:36 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Oct 25, 2019 at 03:00:45PM -0400, Stuart Hayes wrote:
> > From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >
> > The presence detect state (PDS) is normally a logical or of in-band and
> > out-of-band presence. As of PCIe 4.0, there is the option to disable
> > in-band presence so that the PDS bit always reflects the state of the
> > out-of-band presence.
> >
> > The recommendation of the PCIe spec is to disable in-band presence
> > whenever supported.
>
> I think I'm fine with this patch, but I would like to include the
> specific reference for this recommendation.  If you have it handy, I
> can just insert it.
>

The PCI Express Base Specification Revision 5.0, Version 1.0, in the
implementation note under Appendix I ("Async Hot-Plug Reference
Model"), it says "If OOB PD is being used and the associated DSP
supports In-Band PD Disable, it is recommended that the In-Band PD
Disable bit be Set, ..."


> > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > ---
> >  drivers/pci/hotplug/pciehp.h     | 1 +
> >  drivers/pci/hotplug/pciehp_hpc.c | 9 ++++++++-
> >  include/uapi/linux/pci_regs.h    | 2 ++
> >  3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> > index 654c972b8ea0..27e4cd6529b0 100644
> > --- a/drivers/pci/hotplug/pciehp.h
> > +++ b/drivers/pci/hotplug/pciehp.h
> > @@ -83,6 +83,7 @@ struct controller {
> >       struct pcie_device *pcie;
> >
> >       u32 slot_cap;                           /* capabilities and quirks */
> > +     unsigned int inband_presence_disabled:1;
> >
> >       u16 slot_ctrl;                          /* control register access */
> >       struct mutex ctrl_lock;
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> > index 1a522c1c4177..dc109d521f30 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -811,7 +811,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
> >  struct controller *pcie_init(struct pcie_device *dev)
> >  {
> >       struct controller *ctrl;
> > -     u32 slot_cap, link_cap;
> > +     u32 slot_cap, slot_cap2, link_cap;
> >       u8 poweron;
> >       struct pci_dev *pdev = dev->port;
> >       struct pci_bus *subordinate = pdev->subordinate;
> > @@ -869,6 +869,13 @@ struct controller *pcie_init(struct pcie_device *dev)
> >               FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
> >               pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
> >
> > +     pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, &slot_cap2);
> > +     if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
> > +             pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
> > +                                   PCI_EXP_SLTCTL_IBPD_DISABLE);
> > +             ctrl->inband_presence_disabled = 1;
> > +     }
> > +
> >       /*
> >        * If empty slot's power status is on, turn power off.  The IRQ isn't
> >        * requested yet, so avoid triggering a notification with this command.
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index 29d6e93fd15e..ea1cf9546e4d 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -604,6 +604,7 @@
> >  #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
> >  #define  PCI_EXP_SLTCTL_EIC  0x0800  /* Electromechanical Interlock Control */
> >  #define  PCI_EXP_SLTCTL_DLLSCE       0x1000  /* Data Link Layer State Changed Enable */
> > +#define  PCI_EXP_SLTCTL_IBPD_DISABLE 0x4000 /* In-band PD disable */
> >  #define PCI_EXP_SLTSTA               26      /* Slot Status */
> >  #define  PCI_EXP_SLTSTA_ABP  0x0001  /* Attention Button Pressed */
> >  #define  PCI_EXP_SLTSTA_PFD  0x0002  /* Power Fault Detected */
> > @@ -676,6 +677,7 @@
> >  #define PCI_EXP_LNKSTA2              50      /* Link Status 2 */
> >  #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2       52      /* v2 endpoints with link end here */
> >  #define PCI_EXP_SLTCAP2              52      /* Slot Capabilities 2 */
> > +#define  PCI_EXP_SLTCAP2_IBPD        0x0001  /* In-band PD Disable Supported */
> >  #define PCI_EXP_SLTCTL2              56      /* Slot Control 2 */
> >  #define PCI_EXP_SLTSTA2              58      /* Slot Status 2 */
> >
> > --
> > 2.18.1
> >

Patch
diff mbox series

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 654c972b8ea0..27e4cd6529b0 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -83,6 +83,7 @@  struct controller {
 	struct pcie_device *pcie;
 
 	u32 slot_cap;				/* capabilities and quirks */
+	unsigned int inband_presence_disabled:1;
 
 	u16 slot_ctrl;				/* control register access */
 	struct mutex ctrl_lock;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1a522c1c4177..dc109d521f30 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -811,7 +811,7 @@  static inline void dbg_ctrl(struct controller *ctrl)
 struct controller *pcie_init(struct pcie_device *dev)
 {
 	struct controller *ctrl;
-	u32 slot_cap, link_cap;
+	u32 slot_cap, slot_cap2, link_cap;
 	u8 poweron;
 	struct pci_dev *pdev = dev->port;
 	struct pci_bus *subordinate = pdev->subordinate;
@@ -869,6 +869,13 @@  struct controller *pcie_init(struct pcie_device *dev)
 		FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
 		pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
 
+	pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, &slot_cap2);
+	if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
+		pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
+				      PCI_EXP_SLTCTL_IBPD_DISABLE);
+		ctrl->inband_presence_disabled = 1;
+	}
+
 	/*
 	 * If empty slot's power status is on, turn power off.  The IRQ isn't
 	 * requested yet, so avoid triggering a notification with this command.
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 29d6e93fd15e..ea1cf9546e4d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -604,6 +604,7 @@ 
 #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC	0x0800	/* Electromechanical Interlock Control */
 #define  PCI_EXP_SLTCTL_DLLSCE	0x1000	/* Data Link Layer State Changed Enable */
+#define  PCI_EXP_SLTCTL_IBPD_DISABLE	0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA		26	/* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP	0x0001	/* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD	0x0002	/* Power Fault Detected */
@@ -676,6 +677,7 @@ 
 #define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	52	/* v2 endpoints with link end here */
 #define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */
+#define  PCI_EXP_SLTCAP2_IBPD	0x0001	/* In-band PD Disable Supported */
 #define PCI_EXP_SLTCTL2		56	/* Slot Control 2 */
 #define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */