| Message ID | 20210224061132.26526-7-jianjun.wang@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: mediatek: Add new generation controller support | expand |
Hi Jianjun, > Add suspend_noirq and resume_noirq callback functions to implement > PM system suspend hooks for MediaTek Gen3 PCIe controller. So, "systems suspend" and "resume" hooks, correct? > When system suspend, trigger the PCIe link to L2 state and pull down It probably would be "the system suspends". [...] > When system resum, the PCIe link should be re-established and the > related control register values should be restored. Similarly to the above: "the system resumes". [...] > + if (err) { > + dev_err(port->dev, "can not enter L2 state\n"); > + return err; > + } Most likely you want "cannot" or "can't" in the above error message. > + /* Pull down the PERST# pin */ > + val = readl_relaxed(port->base + PCIE_RST_CTRL_REG); > + val |= PCIE_PE_RSTB; > + writel_relaxed(val, port->base + PCIE_RST_CTRL_REG); > + > + dev_dbg(port->dev, "enter L2 state success"); Just a nitpick. What about "entered L2 states successfully"? [...] > + if (err) { > + dev_err(port->dev, "resume failed\n"); > + return err; > + } This error message does not quite convey that the mtk_pcie_startup_port() was the function that failed, which is only a part of what you have to do to successfully resume. > + dev_dbg(port->dev, "resume done\n"); A nitpick. Probably not needed, as lack of error message would mean that the device resumed successfully after being suspended. Krzysztof
Hi Krzysztof, Thanks for your review, On Wed, 2021-02-24 at 15:10 +0100, Krzysztof Wilczyński wrote: > Hi Jianjun, > > > Add suspend_noirq and resume_noirq callback functions to implement > > PM system suspend hooks for MediaTek Gen3 PCIe controller. > > So, "systems suspend" and "resume" hooks, correct? The callback functions is suspend_noirq and resume_noirq, should I use "systems suspend" and "resume" in the commit message? > > > When system suspend, trigger the PCIe link to L2 state and pull down > > It probably would be "the system suspends". > > [...] > > When system resum, the PCIe link should be re-established and the > > related control register values should be restored. > > Similarly to the above: "the system resumes". > > [...] > > + if (err) { > > + dev_err(port->dev, "can not enter L2 state\n"); > > + return err; > > + } > > Most likely you want "cannot" or "can't" in the above error message. > > > + /* Pull down the PERST# pin */ > > + val = readl_relaxed(port->base + PCIE_RST_CTRL_REG); > > + val |= PCIE_PE_RSTB; > > + writel_relaxed(val, port->base + PCIE_RST_CTRL_REG); > > + > > + dev_dbg(port->dev, "enter L2 state success"); > > Just a nitpick. What about "entered L2 states successfully"? > > [...] > > + if (err) { > > + dev_err(port->dev, "resume failed\n"); > > + return err; > > + } > > This error message does not quite convey that the mtk_pcie_startup_port() > was the function that failed, which is only a part of what you have to do > to successfully resume. > > > + dev_dbg(port->dev, "resume done\n"); > > A nitpick. Probably not needed, as lack of error message would mean > that the device resumed successfully after being suspended. > > Krzysztof Thanks.
Hi Jianjun, [...] > Thanks for your review, Thank YOU for all the work here! [...] > > > Add suspend_noirq and resume_noirq callback functions to implement > > > PM system suspend hooks for MediaTek Gen3 PCIe controller. > > > > So, "systems suspend" and "resume" hooks, correct? > > The callback functions is suspend_noirq and resume_noirq, should I use > "systems suspend" and "resume" in the commit message? [...] What I meant was something along these lines: Add suspend_noirq and resume_noirq callback functions to implement PM system suspend and resume hooks for the MediaTek Gen3 PCIe controller. When the system suspends, trigger the PCIe link to enter the L2 state and pull down the PERST# pin, gating the clocks of the MAC layer, and then power-off the physical layer to provide power-saving. When the system resumes, the PCIe link should be re-established and the related control register values should be restored. The above is just a suggestion, thus feel tree to ignore it completely, and it's heavily based on your original commit message. Krzysztof
Hi Krzysztof, Thanks for your suggestion, I will fix it in the next version. On Thu, 2021-02-25 at 23:00 +0100, Krzysztof Wilczyński wrote: > Hi Jianjun, > > [...] > > Thanks for your review, > > Thank YOU for all the work here! > > [...] > > > > Add suspend_noirq and resume_noirq callback functions to implement > > > > PM system suspend hooks for MediaTek Gen3 PCIe controller. > > > > > > So, "systems suspend" and "resume" hooks, correct? > > > > The callback functions is suspend_noirq and resume_noirq, should I use > > "systems suspend" and "resume" in the commit message? > [...] > > > What I meant was something along these lines: > > Add suspend_noirq and resume_noirq callback functions to implement PM > system suspend and resume hooks for the MediaTek Gen3 PCIe controller. > > When the system suspends, trigger the PCIe link to enter the L2 state > and pull down the PERST# pin, gating the clocks of the MAC layer, and > then power-off the physical layer to provide power-saving. > > When the system resumes, the PCIe link should be re-established and the > related control register values should be restored. > > The above is just a suggestion, thus feel tree to ignore it completely, > and it's heavily based on your original commit message. > > Krzysztof Thanks.
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c index fde9de591888..fd13540d37fe 100644 --- a/drivers/pci/controller/pcie-mediatek-gen3.c +++ b/drivers/pci/controller/pcie-mediatek-gen3.c @@ -45,6 +45,9 @@ #define PCIE_PE_RSTB BIT(3) #define PCIE_LTSSM_STATUS_REG 0x150 +#define PCIE_LTSSM_STATE_MASK GENMASK(28, 24) +#define PCIE_LTSSM_STATE(val) ((val & PCIE_LTSSM_STATE_MASK) >> 24) +#define PCIE_LTSSM_STATE_L2_IDLE 0x14 #define PCIE_LINK_STATUS_REG 0x154 #define PCIE_PORT_LINKUP BIT(8) @@ -73,6 +76,9 @@ #define PCIE_MSI_SET_ADDR_HI_BASE 0xc80 #define PCIE_MSI_SET_ADDR_HI_OFFSET 0x04 +#define PCIE_ICMD_PM_REG 0x198 +#define PCIE_TURN_OFF_LINK BIT(4) + #define PCIE_TRANS_TABLE_BASE_REG 0x800 #define PCIE_ATR_SRC_ADDR_MSB_OFFSET 0x4 #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET 0x8 @@ -892,6 +898,83 @@ static int mtk_pcie_remove(struct platform_device *pdev) return 0; } +static int __maybe_unused mtk_pcie_turn_off_link(struct mtk_pcie_port *port) +{ + u32 val; + + val = readl_relaxed(port->base + PCIE_ICMD_PM_REG); + val |= PCIE_TURN_OFF_LINK; + writel_relaxed(val, port->base + PCIE_ICMD_PM_REG); + + /* Check the link is L2 */ + return readl_poll_timeout(port->base + PCIE_LTSSM_STATUS_REG, val, + (PCIE_LTSSM_STATE(val) == + PCIE_LTSSM_STATE_L2_IDLE), 20, + 50 * USEC_PER_MSEC); +} + +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev) +{ + struct mtk_pcie_port *port = dev_get_drvdata(dev); + int err; + u32 val; + + /* Trigger link to L2 state */ + err = mtk_pcie_turn_off_link(port); + if (err) { + dev_err(port->dev, "can not enter L2 state\n"); + return err; + } + + /* Pull down the PERST# pin */ + val = readl_relaxed(port->base + PCIE_RST_CTRL_REG); + val |= PCIE_PE_RSTB; + writel_relaxed(val, port->base + PCIE_RST_CTRL_REG); + + dev_dbg(port->dev, "enter L2 state success"); + + clk_bulk_disable_unprepare(port->num_clks, port->clks); + + reset_control_assert(port->mac_reset); + + phy_power_off(port->phy); + reset_control_assert(port->phy_reset); + + return 0; +} + +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev) +{ + struct mtk_pcie_port *port = dev_get_drvdata(dev); + int err; + + reset_control_deassert(port->phy_reset); + phy_power_on(port->phy); + + reset_control_deassert(port->mac_reset); + + err = clk_bulk_prepare_enable(port->num_clks, port->clks); + if (err) { + dev_dbg(dev, "failed to enable PCIe clocks\n"); + return err; + } + + err = mtk_pcie_startup_port(port); + if (err) { + dev_err(port->dev, "resume failed\n"); + return err; + } + + dev_dbg(port->dev, "resume done\n"); + + return 0; +} + +static const struct dev_pm_ops mtk_pcie_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq, + mtk_pcie_resume_noirq) +}; + static const struct of_device_id mtk_pcie_of_match[] = { { .compatible = "mediatek,mt8192-pcie" }, {}, @@ -903,6 +986,7 @@ static struct platform_driver mtk_pcie_driver = { .driver = { .name = "mtk-pcie", .of_match_table = mtk_pcie_of_match, + .pm = &mtk_pcie_pm_ops, }, };