Skip to contentSkip to conversation

pcie-rcar: try setting PCIe speed to 5 GT/s at boot

PATCH Superseded Sergei Shtylyov 3 participants 4 messages Sep 7, 2016 – Sep 20, 2016

Subsystems (2)
PCI Arm/RISC-V/Renesas architecture

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Sergei Shtylyov

author
#0 lore raw

PATCH 1/1 pcie-rcar: try setting PCIe speed to 5 GT/s at boot

From: Grigory Kletsko <[email protected]>

Initially, the PCIe link speed is set up only at 2.5 GT/s. For better performance, we're trying to increase link speed to 5 GT/s.

[Sergei Shtylyov: indented the macro definitions with tabs, renamed the SPCHG register bits for consistency, renamed the link speed field/values, fixed too long lines, fixed redundancy in clearing the MACSR register bits, fixed grammar/typos in the comments/messages, removed unrelated/useless changes, fixed bugs in rcar_rwm32() calls done to set the bits, removed unneeded braces, removed non-informative comment, reworded the patch summary/description.]

Signed-off-by Grigory Kletsko <[email protected]>
Signed-off-by Sergei Shtylyov <[email protected]>

---
The patch is against the 'next' branch of Bjorn Helgaas' 'pci.git' repo.

 drivers/pci/host/pcie-rcar.c |  103 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)
drivers/pci/host/pcie-rcar.c +103 -0
Index: pci/drivers/pci/host/pcie-rcar.c
===================================================================
@@ -48,6 +48,10 @@
 #define  CFINIT			1
 #define PCIETSTR		0x02004
 #define  DATA_LINK_ACTIVE	1
+#define PCIEINTR		0x02008
+#define  PCIEINTMAC		(1 << 13)
+#define PCIEINTER		0x0200C
+#define  PCIEINTMACE		(1 << 13)
 #define PCIEERRFR		0x02020
 #define  UNSUPPORTED_REQUEST	(1 << 4)
 #define PCIEMSIFR		0x02044

@@ -84,8 +88,21 @@
 #define IDSETR1			0x011004
 #define TLCTLR			0x011048
 #define MACSR			0x011054
+#define  SPCHG			(1 << 5)
+#define  SPCHGFIN		(1 << 4)
+#define  SPCHGSUC		(1 << 7)
+#define  SPCHGFAIL		(1 << 6)
+#define  LINK_SPEED		(0xf << 16)
+#define  LINK_SPEED_2_5GTS	(1 << 16)
+#define  LINK_SPEED_5_0GTS	(2 << 16)
 #define MACCTLR			0x011058
+#define  SPEED_CHANGE		(1 << 24)
 #define  SCRAMBLE_DISABLE	(1 << 27)
+#define MACINTENR		0x01106C
+#define  SPCHGFINE		(1 << 4)
+#define MACS2R			0x011078
+#define MACCGSPSETR		0x011084
+#define  SPCNGRSN		(1 << 31)
 
 /* R-Car H1 PHY */
 #define H1_PCIEPHYADRR		0x04000c

@@ -385,6 +402,51 @@ static int rcar_pcie_setup(struct list_h
 	return 1;
 }
 
+void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
+{
+	u32 macsr;
+
+	dev_info(pcie->dev, "Trying speed up to 5 GT/s\n");
+
+	if ((rcar_pci_read_reg(pcie, MACSR) & SPCHGFIN) ||
+	    (rcar_pci_read_reg(pcie, MACCTLR) & SPEED_CHANGE)) {
+		dev_err(pcie->dev, "Speed changing is in progress\n");
+		return;
+	}
+
+	if ((rcar_pci_read_reg(pcie, MACSR) & LINK_SPEED) ==
+	    LINK_SPEED_5_0GTS) {
+		dev_err(pcie->dev, "Current link speed is already 5 GT/s\n");
+		return;
+	}
+
+	if ((rcar_pci_read_reg(pcie, MACS2R) & LINK_SPEED) !=
+	    LINK_SPEED_5_0GTS) {
+		dev_err(pcie->dev,
+			"Current max support link speed not 5 GT/s\n");
+		return;
+	}
+
+	/* Set target link speed to 5.0 GT/s */
+	rcar_rmw32(pcie, EXPCAP(12), PCI_EXP_LNKSTA_CLS,
+		   PCI_EXP_LNKSTA_CLS_5_0GB);
+
+	/* Set speed change reason as intentional factor */
+	rcar_rmw32(pcie, MACCGSPSETR, SPCNGRSN, 0);
+
+	/* Clear SPCHGFIN, SPCHGSUC, and SPCHGFAIL */
+	macsr = rcar_pci_read_reg(pcie, MACSR);
+	if (macsr & (SPCHGFIN | SPCHGSUC | SPCHGFAIL))
+		rcar_pci_write_reg(pcie, macsr, MACSR);
+
+	/* Enable interrupt */
+	rcar_rmw32(pcie, MACINTENR, SPCHGFINE, SPCHGFINE);
+	rcar_rmw32(pcie, PCIEINTER, PCIEINTMACE, PCIEINTMACE);
+
+	/* Start link speed change */
+	rcar_rmw32(pcie, MACCTLR, SPEED_CHANGE, SPEED_CHANGE);
+}
+
 static int rcar_pcie_enable(struct rcar_pcie *pcie)
 {
 	struct pci_bus *bus, *child;

@@ -416,6 +478,9 @@ static int rcar_pcie_enable(struct rcar_
 
 	pci_bus_add_devices(bus);
 
+	/* Try setting 5 GT/s link speed */
+	rcar_pcie_force_speedup(pcie);
+
 	return 0;
 }
 

@@ -621,6 +686,44 @@ static irqreturn_t rcar_pcie_msi_irq(int
 	struct rcar_msi *msi = &pcie->msi;
 	unsigned long reg;
 
+	if (rcar_pci_read_reg(pcie, PCIEINTR) & PCIEINTMAC) {
+		dev_dbg(pcie->dev, "MAC interrupt received\n");
+
+		rcar_rmw32(pcie, MACSR, SPCHGFIN, SPCHGFIN);
+
+		/* Disable this interrupt */
+		rcar_rmw32(pcie, MACINTENR, SPCHGFINE, 0);
+		rcar_rmw32(pcie, PCIEINTER, PCIEINTMACE, 0);
+
+		if (rcar_pci_read_reg(pcie, MACSR) & SPCHGFAIL) {
+			dev_err(pcie->dev, "Speed change failed\n");
+
+			rcar_rmw32(pcie, MACSR, SPCHGFAIL, SPCHGFAIL);
+			/*
+			 * TODO: if speed change failed we need to enable
+			 * "L0 enter" interrupt and set "speed change disabled"
+			 * state. After L0 interrupt rising, we must clear it,
+			 * wait for 200 ms and set "speed change enabled" state
+			 * according to the R-Car Series, 2nd Generation User's
+			 * Manual, section 50.3.9.
+			 */
+			return IRQ_HANDLED;
+		}
+
+		if (rcar_pci_read_reg(pcie, MACSR) & SPCHGSUC)
+			rcar_rmw32(pcie, MACSR, SPCHGSUC, SPCHGSUC);
+
+		/* Check speed */
+		if ((rcar_pci_read_reg(pcie, MACSR) & LINK_SPEED) ==
+		    LINK_SPEED_5_0GTS)
+			dev_info(pcie->dev, "Current link speed now 5 GT/s\n");
+		else
+			dev_info(pcie->dev,
+				 "Current link speed now 2.5 GT/s\n");
+
+		return IRQ_HANDLED;
+	}
+
 	reg = rcar_pci_read_reg(pcie, PCIEMSIFR);
 
 	/* MSI & INTx share an interrupt - we only handle MSI here */
Acked-by on v1 1/1
#1 lore raw
Quoted 13 lines · Sergei Shtylyov

From: Grigory Kletsko <[email protected]>

Initially, the PCIe link speed is set up only at 2.5 GT/s. For better performance, we're trying to increase link speed to 5 GT/s.

[Sergei Shtylyov: indented the macro definitions with tabs, renamed the SPCHG register bits for consistency, renamed the link speed field/values, fixed too long lines, fixed redundancy in clearing the MACSR register bits, fixed grammar/typos in the comments/messages, removed unrelated/useless changes, fixed bugs in rcar_rwm32() calls done to set the bits, removed unneeded braces, removed non-informative comment, reworded the patch summary/description.]

Quoted 2 lines
Signed-off-by Grigory Kletsko <[email protected]>
Signed-off-by Sergei Shtylyov <[email protected]>
Acked-by Simon Horman <[email protected]>
#2 lore raw
In reply to Sergei Shtylyov
Quoted 62 lines · Sergei Shtylyov

From: Grigory Kletsko <[email protected]>

Initially, the PCIe link speed is set up only at 2.5 GT/s. For better performance, we're trying to increase link speed to 5 GT/s.

[Sergei Shtylyov: indented the macro definitions with tabs, renamed the SPCHG register bits for consistency, renamed the link speed field/values, fixed too long lines, fixed redundancy in clearing the MACSR register bits, fixed grammar/typos in the comments/messages, removed unrelated/useless changes, fixed bugs in rcar_rwm32() calls done to set the bits, removed unneeded braces, removed non-informative comment, reworded the patch summary/description.]

Signed-off-by Grigory Kletsko <[email protected]>
Signed-off-by Sergei Shtylyov <[email protected]>

---
The patch is against the 'next' branch of Bjorn Helgaas' 'pci.git' repo.

 drivers/pci/host/pcie-rcar.c |  103 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

Index: pci/drivers/pci/host/pcie-rcar.c
===================================================================

drivers/pci/host/pcie-rcar.c +17 -0
@@ -48,6 +48,10 @@
 #define  CFINIT			1
 #define PCIETSTR		0x02004
 #define  DATA_LINK_ACTIVE	1
+#define PCIEINTR		0x02008
+#define  PCIEINTMAC		(1 << 13)
+#define PCIEINTER		0x0200C
+#define  PCIEINTMACE		(1 << 13)
 #define PCIEERRFR		0x02020
 #define  UNSUPPORTED_REQUEST	(1 << 4)
 #define PCIEMSIFR		0x02044

@@ -84,8 +88,21 @@
 #define IDSETR1			0x011004
 #define TLCTLR			0x011048
 #define MACSR			0x011054
+#define  SPCHG			(1 << 5)
+#define  SPCHGFIN		(1 << 4)
+#define  SPCHGSUC		(1 << 7)
+#define  SPCHGFAIL		(1 << 6)
+#define  LINK_SPEED		(0xf << 16)
+#define  LINK_SPEED_2_5GTS	(1 << 16)
+#define  LINK_SPEED_5_0GTS	(2 << 16)
 #define MACCTLR			0x011058
+#define  SPEED_CHANGE		(1 << 24)
 #define  SCRAMBLE_DISABLE	(1 << 27)
+#define MACINTENR		0x01106C
+#define  SPCHGFINE		(1 << 4)
+#define MACS2R			0x011078
+#define MACCGSPSETR		0x011084
+#define  SPCNGRSN		(1 << 31)
 
 /* R-Car H1 PHY */
 #define H1_PCIEPHYADRR		0x04000c

@@ -385,6 +402,51 @@ static int rcar_pcie_setup(struct list_h
 	return 1;
 }

drivers/pci/host/pcie-rcar.c static int rcar_pcie_setup(struct list_h · line 413

Quoted 10 lines
drivers/pci/host/pcie-rcar.c +9 -0
 
+void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
+{
+	u32 macsr;
+
+	dev_info(pcie->dev, "Trying speed up to 5 GT/s\n");
+
+	if ((rcar_pci_read_reg(pcie, MACSR) & SPCHGFIN) ||
+	    (rcar_pci_read_reg(pcie, MACCTLR) & SPEED_CHANGE)) {
+		dev_err(pcie->dev, "Speed changing is in progress\n");

Are these messages all really errors?

Quoted 40 lines
drivers/pci/host/pcie-rcar.c +36 -0
+		return;
+	}
+
+	if ((rcar_pci_read_reg(pcie, MACSR) & LINK_SPEED) ==
+	    LINK_SPEED_5_0GTS) {
+		dev_err(pcie->dev, "Current link speed is already 5 GT/s\n");
+		return;
+	}
+
+	if ((rcar_pci_read_reg(pcie, MACS2R) & LINK_SPEED) !=
+	    LINK_SPEED_5_0GTS) {
+		dev_err(pcie->dev,
+			"Current max support link speed not 5 GT/s\n");
+		return;
+	}
+
+	/* Set target link speed to 5.0 GT/s */
+	rcar_rmw32(pcie, EXPCAP(12), PCI_EXP_LNKSTA_CLS,
+		   PCI_EXP_LNKSTA_CLS_5_0GB);
+
+	/* Set speed change reason as intentional factor */
+	rcar_rmw32(pcie, MACCGSPSETR, SPCNGRSN, 0);
+
+	/* Clear SPCHGFIN, SPCHGSUC, and SPCHGFAIL */
+	macsr = rcar_pci_read_reg(pcie, MACSR);
+	if (macsr & (SPCHGFIN | SPCHGSUC | SPCHGFAIL))
+		rcar_pci_write_reg(pcie, macsr, MACSR);
+
+	/* Enable interrupt */
+	rcar_rmw32(pcie, MACINTENR, SPCHGFINE, SPCHGFINE);
+	rcar_rmw32(pcie, PCIEINTER, PCIEINTMACE, PCIEINTMACE);
+
+	/* Start link speed change */
+	rcar_rmw32(pcie, MACCTLR, SPEED_CHANGE, SPEED_CHANGE);
+}
+
 static int rcar_pcie_enable(struct rcar_pcie *pcie)
 {
 	struct pci_bus *bus, *child;

drivers/pci/host/pcie-rcar.c static int rcar_pcie_enable(struct rcar_ · line 482

Quoted 5 lines
drivers/pci/host/pcie-rcar.c +2 -0
@@ -416,6 +478,9 @@ static int rcar_pcie_enable(struct rcar_
 
 	pci_bus_add_devices(bus);
 
+	/* Try setting 5 GT/s link speed */
+	rcar_pcie_force_speedup(pcie);

I assume it's safe to change the link speed even while the downstream devices are in use? As soon as we call pci_bus_add_devices(), drivers can claim the devices and start using them.

drivers/pci/host/pcie-rcar.c static irqreturn_t rcar_pcie_msi_irq(int · line 690

Quoted 9 lines
drivers/pci/host/pcie-rcar.c +2 -0
 	return 0;
 }
 

@@ -621,6 +686,44 @@ static irqreturn_t rcar_pcie_msi_irq(int
 	struct rcar_msi *msi = &pcie->msi;
 	unsigned long reg;
 
+	if (rcar_pci_read_reg(pcie, PCIEINTR) & PCIEINTMAC) {
+		dev_dbg(pcie->dev, "MAC interrupt received\n");

I guess you don't need to write PCIEINTR or any other register to acknowledge the interrupt?

Quoted 29 lines
drivers/pci/host/pcie-rcar.c +29 -0
+
+		rcar_rmw32(pcie, MACSR, SPCHGFIN, SPCHGFIN);
+
+		/* Disable this interrupt */
+		rcar_rmw32(pcie, MACINTENR, SPCHGFINE, 0);
+		rcar_rmw32(pcie, PCIEINTER, PCIEINTMACE, 0);
+
+		if (rcar_pci_read_reg(pcie, MACSR) & SPCHGFAIL) {
+			dev_err(pcie->dev, "Speed change failed\n");
+
+			rcar_rmw32(pcie, MACSR, SPCHGFAIL, SPCHGFAIL);
+			/*
+			 * TODO: if speed change failed we need to enable
+			 * "L0 enter" interrupt and set "speed change disabled"
+			 * state. After L0 interrupt rising, we must clear it,
+			 * wait for 200 ms and set "speed change enabled" state
+			 * according to the R-Car Series, 2nd Generation User's
+			 * Manual, section 50.3.9.
+			 */
+			return IRQ_HANDLED;
+		}
+
+		if (rcar_pci_read_reg(pcie, MACSR) & SPCHGSUC)
+			rcar_rmw32(pcie, MACSR, SPCHGSUC, SPCHGSUC);
+
+		/* Check speed */
+		if ((rcar_pci_read_reg(pcie, MACSR) & LINK_SPEED) ==
+		    LINK_SPEED_5_0GTS)
+			dev_info(pcie->dev, "Current link speed now 5 GT/s\n");

drivers/pci/host/pcie-rcar.c static irqreturn_t rcar_pcie_msi_irq(int · line 729

Quoted 10 lines
drivers/pci/host/pcie-rcar.c +7 -0
+		else
+			dev_info(pcie->dev,
+				 "Current link speed now 2.5 GT/s\n");
+
+		return IRQ_HANDLED;
+	}
+
 	reg = rcar_pci_read_reg(pcie, PCIEMSIFR);
 
 	/* MSI & INTx share an interrupt - we only handle MSI here */

Is this patch adding handling for INTx events? If so, it looks like this comment is now out of date, and possibly the code should be along these lines in case both MSI and INTx signal an interrupt simultaneously:

  irqreturn_t handled = IRQ_NONE;

  reg = rcar_pci_read_reg(pcie, PCIEINTR) & PCIEINTMAC;
  if (reg) {
    handled = IRQ_HANDLED;
    ...
  }

  reg = rcar_pci_read_reg(pcie, PCIEMSIFR);
  while (reg) {
    handled = IRQ_HANDLED;
    ...
  }

return handled;

author
#3 lore raw

Hello.

Quoted 26 lines · Bjorn Helgaas
Quoted 26 lines

From: Grigory Kletsko <[email protected]>

Initially, the PCIe link speed is set up only at 2.5 GT/s. For better performance, we're trying to increase link speed to 5 GT/s.

[Sergei Shtylyov: indented the macro definitions with tabs, renamed the SPCHG register bits for consistency, renamed the link speed field/values, fixed too long lines, fixed redundancy in clearing the MACSR register bits, fixed grammar/typos in the comments/messages, removed unrelated/useless changes, fixed bugs in rcar_rwm32() calls done to set the bits, removed unneeded braces, removed non-informative comment, reworded the patch summary/description.]

Signed-off-by Grigory Kletsko <[email protected]>
Signed-off-by Sergei Shtylyov <[email protected]>

---
The patch is against the 'next' branch of Bjorn Helgaas' 'pci.git' repo.

 drivers/pci/host/pcie-rcar.c |  103 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

Index: pci/drivers/pci/host/pcie-rcar.c
===================================================================

drivers/pci/host/pcie-rcar.c +0 -0

[...]

Quoted 4 lines
Quoted 4 lines
drivers/pci/host/pcie-rcar.c +0 -0
@@ -385,6 +402,51 @@ static int rcar_pcie_setup(struct list_h
 	return 1;
 }
Quoted 11 lines

drivers/pci/host/pcie-rcar.c static int rcar_pcie_setup(struct list_h · line 413

Quoted 9 lines
drivers/pci/host/pcie-rcar.c +9 -0
+void rcar_pcie_force_speedup(struct rcar_pcie *pcie)
+{
+	u32 macsr;
+
+	dev_info(pcie->dev, "Trying speed up to 5 GT/s\n");
+
+	if ((rcar_pci_read_reg(pcie, MACSR) & SPCHGFIN) ||
+	    (rcar_pci_read_reg(pcie, MACCTLR) & SPEED_CHANGE)) {
+		dev_err(pcie->dev, "Speed changing is in progress\n");

Are these messages all really errors?

I guess not. :-)

Quoted 15 lines
Quoted 15 lines
drivers/pci/host/pcie-rcar.c +15 -0
+		return;
+	}
+
+	if ((rcar_pci_read_reg(pcie, MACSR) & LINK_SPEED) ==
+	    LINK_SPEED_5_0GTS) {
+		dev_err(pcie->dev, "Current link speed is already 5 GT/s\n");
+		return;
+	}
+
+	if ((rcar_pci_read_reg(pcie, MACS2R) & LINK_SPEED) !=
+	    LINK_SPEED_5_0GTS) {
+		dev_err(pcie->dev,
+			"Current max support link speed not 5 GT/s\n");
+		return;
+	}

[...]

Quoted 10 lines

drivers/pci/host/pcie-rcar.c static int rcar_pcie_enable(struct rcar_ · line 482

Quoted 6 lines
drivers/pci/host/pcie-rcar.c +2 -0
@@ -416,6 +478,9 @@ static int rcar_pcie_enable(struct rcar_
 
 	pci_bus_add_devices(bus);
 
+	/* Try setting 5 GT/s link speed */
+	rcar_pcie_force_speedup(pcie);

I assume it's safe to change the link speed even while the downstream devices are in use? As soon as we call pci_bus_add_devices(), drivers can claim the devices and start using them.

Not sure. OK, I'm going to move this call before pci_bus_add_devices() and
try to make it synchronous, without using interrupt.

Quoted 7 lines
Quoted 7 lines
drivers/pci/host/pcie-rcar.c +0 -0
 	return 0;
 }
 

@@ -621,6 +686,44 @@ static irqreturn_t rcar_pcie_msi_irq(int
 	struct rcar_msi *msi = &pcie->msi;
 	unsigned long reg;
Quoted 5 lines

drivers/pci/host/pcie-rcar.c static irqreturn_t rcar_pcie_msi_irq(int · line 690

Quoted 2 lines
drivers/pci/host/pcie-rcar.c +2 -0
+	if (rcar_pci_read_reg(pcie, PCIEINTR) & PCIEINTMAC) {
+		dev_dbg(pcie->dev, "MAC interrupt received\n");

I guess you don't need to write PCIEINTR or any other register to acknowledge the interrupt?

You need to write MACSR -- which the code below does. However, it only
clears the SPCHGFIN interrupt (which is only ever enabled).

Quoted 7 lines
Quoted 6 lines
drivers/pci/host/pcie-rcar.c +6 -0
+
+		rcar_rmw32(pcie, MACSR, SPCHGFIN, SPCHGFIN);
+
+		/* Disable this interrupt */
+		rcar_rmw32(pcie, MACINTENR, SPCHGFINE, 0);
+		rcar_rmw32(pcie, PCIEINTER, PCIEINTMACE, 0);
Quoted 5 lines

drivers/pci/host/pcie-rcar.c static irqreturn_t rcar_pcie_msi_irq(int · line 729

Quoted 3 lines
drivers/pci/host/pcie-rcar.c +0 -0
 	reg = rcar_pci_read_reg(pcie, PCIEMSIFR);
 
 	/* MSI & INTx share an interrupt - we only handle MSI here */

Is this patch adding handling for INTx events?

    No.

MBR, Sergei