| Message ID | 6896977.DKiraAXap6@wasted.cogentembedded.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Sep 07, 2016 at 11:22:14PM +0300, Sergei Shtylyov wrote: > From: Grigory Kletsko <grigory.kletsko@cogentembedded.com> > > 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 <grigory.kletsko@cogentembedded.com> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Acked-by: Simon Horman <horms+renesas@verge.net.au> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 07, 2016 at 11:22:14PM +0300, Sergei Shtylyov wrote: > From: Grigory Kletsko <grigory.kletsko@cogentembedded.com> > > 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 <grigory.kletsko@cogentembedded.com> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > 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 > =================================================================== > --- pci.orig/drivers/pci/host/pcie-rcar.c > +++ 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"); Are these messages all really errors? > + 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); 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. > 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? > + > + 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 */ 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; -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 09/14/2016 11:54 PM, Bjorn Helgaas wrote: >> From: Grigory Kletsko <grigory.kletsko@cogentembedded.com> >> >> 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 <grigory.kletsko@cogentembedded.com> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> 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 >> =================================================================== >> --- pci.orig/drivers/pci/host/pcie-rcar.c >> +++ pci/drivers/pci/host/pcie-rcar.c [...] >> @@ -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"); > > Are these messages all really errors? I guess not. :-) >> + 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; >> + } [...] >> @@ -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. >> 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? You need to write MACSR -- which the code below does. However, it only clears the SPCHGFIN interrupt (which is only ever enabled). >> + >> + rcar_rmw32(pcie, MACSR, SPCHGFIN, SPCHGFIN); >> + >> + /* Disable this interrupt */ >> + rcar_rmw32(pcie, MACINTENR, SPCHGFINE, 0); >> + rcar_rmw32(pcie, PCIEINTER, PCIEINTMACE, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: pci/drivers/pci/host/pcie-rcar.c =================================================================== --- pci.orig/drivers/pci/host/pcie-rcar.c +++ 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 */