diff mbox series

[04/15] pciutils-pcilmr: Add functions for device checking and preparations before main margining processes

Message ID 20231208091734.12225-5-n.proshkin@yadro.com (mailing list archive)
State Superseded
Headers show
Series pciutils: Add utility for Lane Margining | expand

Commit Message

Nikita Proshkin Dec. 8, 2023, 9:17 a.m. UTC 
Follow the checklist from the section 4.2.13.3 "Receiver Margin Testing
Requirements" of the 5.0 spec:
* Verify the Link is at 16 GT/s or higher data rate, in DO PM state;
* Verify that Margining Ready bit of the device is set;
* Disable the ASPM and Autonomous Speed/Width features for the duration
  of the test.

Also verify that Upstream Port of the Link is Function 0 of a Device,
according to spec,only it must implement margining registers.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 Makefile            |  5 ++-
 lmr_lib/Makefile    | 10 ++++++
 lmr_lib/margin_hw.c | 85 +++++++++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin_hw.h | 39 +++++++++++++++++++++
 4 files changed, 138 insertions(+), 1 deletion(-)
 create mode 100644 lmr_lib/Makefile
 create mode 100644 lmr_lib/margin_hw.c
 create mode 100644 lmr_lib/margin_hw.h

Comments

Martin Mareš Dec. 8, 2023, 5:30 p.m. UTC | #1 
Hello!

> -all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS)
> +all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a

Is there any advantage with building LMR as a library instead of linking all
the object files with the margining utility?

> --- /dev/null
> +++ b/lmr_lib/margin_hw.c
> @@ -0,0 +1,85 @@
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>

Generally: Please add a comment to every source file, which explains the
purpose of the file and contains a copyright notice. See existing files
for the recommeneded format.

> +  uint8_t down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
> +  uint8_t down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
> +  uint8_t down_dir = (pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;

I would prefer using libpci types (u8, u32 etc.).

> +  if (!(down_sec == up_port->bus && down_type == 1 

Please avoid whitespace at the end of line.

> +bool margin_prep_dev(struct margin_dev *dev)
> +{
> +  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);

What if it doesn't exist?

> diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
> new file mode 100644
> index 0000000..a436d4b
> --- /dev/null
> +++ b/lmr_lib/margin_hw.h
> @@ -0,0 +1,39 @@
> +#ifndef _MARGIN_HW_H
> +#define _MARGIN_HW_H
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +#include "../lib/pci.h"

Please do not use relative paths to libpci header files.
Instead, supply proper include path to CC in the Makefile.

> +/*PCI Device wrapper for margining functions*/

Please surround "/*" and "*/" by spaces as in existing source files.

				Have a nice fortnight
Nikita Proshkin Dec. 13, 2023, 10:10 a.m. UTC | #2 
Hello Martin,
Thanks for the review!

On Fri, 8 Dec 2023 18:30:01 +0100
Martin Mareš <mj@ucw.cz> wrote:
 
> > -all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS)
> > +all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a
> 
> Is there any advantage with building LMR as a library instead of linking all
> the object files with the margining utility?

Actually, there are no advantages, I just thought that the Makefiles would
look more neat with this approach. I will redo the linking to make it 
consistent with the lspci building.

> > +bool margin_prep_dev(struct margin_dev *dev)
> > +{
> > +  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
> 
> What if it doesn't exist?

Nothing good at all. I will add more checks.

> > --- /dev/null
> > +++ b/lmr_lib/margin_hw.c
> > @@ -0,0 +1,85 @@
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> 
> Generally: Please add a comment to every source file, which explains the
> purpose of the file and contains a copyright notice. See existing files
> for the recommeneded format.
> 
> > +  uint8_t down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
> > +  uint8_t down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
> > +  uint8_t down_dir = (pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;
> 
> I would prefer using libpci types (u8, u32 etc.).
> 
> > +  if (!(down_sec == up_port->bus && down_type == 1
> 
> Please avoid whitespace at the end of line.
> 
> > diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
> > new file mode 100644
> > index 0000000..a436d4b
> > --- /dev/null
> > +++ b/lmr_lib/margin_hw.h
> > @@ -0,0 +1,39 @@
> > +#ifndef _MARGIN_HW_H
> > +#define _MARGIN_HW_H
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +
> > +#include "../lib/pci.h"
> 
> Please do not use relative paths to libpci header files.
> Instead, supply proper include path to CC in the Makefile.
> 
> > +/*PCI Device wrapper for margining functions*/
> 
> Please surround "/*" and "*/" by spaces as in existing source files.

Got it, I'll rework the code.

Best regards,
Nikita Proshkin
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 228cb56..bd636bd 100644
--- a/Makefile
+++ b/Makefile
@@ -67,11 +67,14 @@  PCIINC_INS=lib/config.h lib/header.h lib/pci.h lib/types.h
 
 export
 
-all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS)
+all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a
 
 lib/$(PCIIMPLIB): $(PCIINC) force
 	$(MAKE) -C lib all
 
+lmr_lib/liblmr.a: $(PCIINC) force
+	$(MAKE) -C lmr_lib all
+
 force:
 
 lib/config.h lib/config.mk:
diff --git a/lmr_lib/Makefile b/lmr_lib/Makefile
new file mode 100644
index 0000000..4f85a17
--- /dev/null
+++ b/lmr_lib/Makefile
@@ -0,0 +1,10 @@ 
+OBJS=margin_hw
+INCL=$(addsuffix .h,$(OBJS)) $(addprefix ../,$(PCIINC))
+
+$(addsuffix .o, $(OBJS)): %.o: %.c $(INCL)
+
+all: liblmr.a
+
+liblmr.a: $(addsuffix .o, $(OBJS))
+	rm -f $@
+	$(AR) rcs $@ $^
diff --git a/lmr_lib/margin_hw.c b/lmr_lib/margin_hw.c
new file mode 100644
index 0000000..dcc6593
--- /dev/null
+++ b/lmr_lib/margin_hw.c
@@ -0,0 +1,85 @@ 
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "margin_hw.h"
+
+bool margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port)
+{
+  struct pci_cap *cap = pci_find_cap(down_port, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+  if (!cap)
+    return false;
+  if ((pci_read_word(down_port, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED) < 4)
+    return false;
+  if ((pci_read_word(down_port, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED) > 5)
+    return false;
+
+  uint8_t down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
+  uint8_t down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
+  uint8_t down_dir = (pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;
+
+  // Verify that devices are linked, down_port is Root Port or Downstream Port of Switch,
+  // up_port is Function 0 of a Device
+  if (!(down_sec == up_port->bus && down_type == 1 
+      && (down_dir == 4 || down_dir == 6) && up_port->func == 0))
+    return false;
+
+  struct pci_cap *pm = pci_find_cap(up_port, PCI_CAP_ID_PM, PCI_CAP_NORMAL);
+  return !(pci_read_word(up_port, pm->addr + PCI_PM_CTRL) & PCI_PM_CTRL_STATE_MASK); // D0
+}
+
+bool margin_check_ready_bit(struct pci_dev *dev)
+{
+  struct pci_cap *lmr = pci_find_cap(dev, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED);
+  return lmr && (pci_read_word(dev, lmr->addr + PCI_LMR_PORT_STS) & PCI_LMR_PORT_STS_READY);
+}
+
+struct margin_dev margin_fill_wrapper(struct pci_dev *dev)
+{
+  struct pci_cap *cap = pci_find_cap(dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+  struct margin_dev res = {
+      .dev = dev,
+      .lmr_cap_addr = pci_find_cap(dev, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED)->addr,
+      .width = (pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_WIDTH) >> 4,
+      .retimers_n = ((pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA2) & PCI_EXP_LINKSTA2_RETIMER) > 0) +
+                    ((pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA2) & PCI_EXP_LINKSTA2_2RETIMERS) > 0),
+      .link_speed = (pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED)};
+  return res;
+}
+
+bool margin_prep_dev(struct margin_dev *dev)
+{
+  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+
+  uint16_t lnk_ctl = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL);
+  dev->aspm = lnk_ctl & PCI_EXP_LNKCTL_ASPM;
+  dev->hawd = lnk_ctl & PCI_EXP_LNKCTL_HWAUTWD;
+  lnk_ctl &= ~PCI_EXP_LNKCTL_ASPM;
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL, lnk_ctl);
+  if (pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM)
+    return false;
+
+  lnk_ctl |= PCI_EXP_LNKCTL_HWAUTWD;
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL, lnk_ctl);
+
+  uint16_t lnk_ctl2 = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2);
+  dev->hasd = lnk_ctl2 & PCI_EXP_LNKCTL2_SPEED_DIS;
+  lnk_ctl2 |= PCI_EXP_LNKCTL2_SPEED_DIS;
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2, lnk_ctl2);
+
+  return true;
+}
+
+void margin_restore_dev(struct margin_dev *dev)
+{
+  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+
+  uint16_t lnk_ctl = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL);
+  lnk_ctl |= dev->aspm;
+  lnk_ctl &= (~PCI_EXP_LNKCTL_HWAUTWD | dev->hawd);
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL, lnk_ctl);
+
+  uint16_t lnk_ctl2 = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2);
+  lnk_ctl2 &= (~PCI_EXP_LNKCTL2_SPEED_DIS | dev->hasd);
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2, lnk_ctl2);
+}
diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
new file mode 100644
index 0000000..a436d4b
--- /dev/null
+++ b/lmr_lib/margin_hw.h
@@ -0,0 +1,39 @@ 
+#ifndef _MARGIN_HW_H
+#define _MARGIN_HW_H
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#include "../lib/pci.h"
+
+/*PCI Device wrapper for margining functions*/
+struct margin_dev
+{
+  struct pci_dev *dev;
+  int lmr_cap_addr;
+  uint8_t width;
+  uint8_t retimers_n;
+  uint8_t link_speed;
+
+  /*Saved Device settings to restore after margining*/
+  uint16_t aspm;
+  uint16_t hasd; /*Hardware Autonomous Speed Disable*/
+  uint16_t hawd; /*Hardware Autonomous Width Disable*/
+};
+
+/*Verify that devices form the link at Gen 4 speed or higher*/
+bool margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port);
+
+/*Check Margining Ready bit from Margining Port Status Register*/
+bool margin_check_ready_bit(struct pci_dev *dev);
+
+/*Awaits device at Gen 4 or higher*/
+struct margin_dev margin_fill_wrapper(struct pci_dev *dev);
+
+/*Disable ASPM, set Hardware Autonomous Speed/Width Disable bits*/
+bool margin_prep_dev(struct margin_dev *dev);
+
+/*Restore Device ASPM, Hardware Autonomous Speed/Width settings*/
+void margin_restore_dev(struct margin_dev *dev);
+
+#endif