| Message ID | 20240102-j7200-pcie-s2r-v3-18-5c2e4a3fac1f@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Thu, Feb 15, 2024 at 04:18:03PM +0100, Thomas Richard wrote: > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Add suspend and resume support. Only the rc mode is supported. > > During the suspend stage PERST# is asserted, then deasserted during the > resume stage. ... > +#include <linux/clk-provider.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/gpio/consumer.h> > @@ -18,10 +19,13 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/regmap.h> > +#include <linux/container_of.h> Unordered. ... > + ret = j721e_pcie_ctrl_init(pcie); > + if (ret < 0) { > + dev_err(dev, "j721e_pcie_ctrl_init failed\n"); Is there any guarantee this won't spam logs? > + return ret; > + } ... > + /* > + * This is not called explicitly in the probe, it is called by > + * cdns_pcie_init_phy. cdns_pcie_init_phy() > + */ > + ret = cdns_pcie_enable_phy(pcie->cdns_pcie); > + if (ret < 0) { > + dev_err(dev, "cdns_pcie_enable_phy failed\n"); > + return -ENODEV; A potential log spammer? > + } > + if (pcie->mode == PCI_MODE_RC) { > + struct cdns_pcie_rc *rc = cdns_pcie_to_rc(cdns_pcie); > + > + ret = clk_prepare_enable(pcie->refclk); > + if (ret < 0) { > + dev_err(dev, "clk_prepare_enable failed\n"); Ditto. > + return -ENODEV; Why is the error code shadowed? > + } ... > + if (pcie->reset_gpio) { > + usleep_range(100, 200); fsleep() > + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > + } > + ret = cdns_pcie_host_link_setup(rc); > + if (ret < 0) { > + clk_disable_unprepare(pcie->refclk); > + return ret; > + } > + > + /* > + * Reset internal status of BARs to force reinitialization in > + * cdns_pcie_host_init(). > + */ > + for (enum cdns_pcie_rp_bar bar = RP_BAR0; bar <= RP_NO_BAR; bar++) > + rc->avail_ib_bar[bar] = true; > + > + ret = cdns_pcie_host_init(rc); > + if (ret) No clock disabling? > + return ret; > + }
On 24/02/15 04:18PM, Thomas Richard wrote: > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Add suspend and resume support. Only the rc mode is supported. > > During the suspend stage PERST# is asserted, then deasserted during the > resume stage. Wouldn't this imply that the Endpoint device will be reset and therefore lose context? Or is it expected that the driver corresponding to the Endpoint Function in Linux will restore the state on resume, post reset? Regards, Siddharth.
Hello, On Fri Feb 16, 2024 at 11:48 AM CET, Siddharth Vadapalli wrote: > On 24/02/15 04:18PM, Thomas Richard wrote: > > From: Théo Lebrun <theo.lebrun@bootlin.com> > > > > Add suspend and resume support. Only the rc mode is supported. > > > > During the suspend stage PERST# is asserted, then deasserted during the > > resume stage. > > Wouldn't this imply that the Endpoint device will be reset and therefore > lose context? Or is it expected that the driver corresponding to the > Endpoint Function in Linux will restore the state on resume, post reset? This does imply exactly that. Endpoint driver must be able to restore context anyway, as system-wide suspend could mean lost power to PCI RC controller (eg suspend-to-RAM) or PCI rails (dependent on hardware). Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 24/02/16 12:09PM, Théo Lebrun wrote: > Hello, > > On Fri Feb 16, 2024 at 11:48 AM CET, Siddharth Vadapalli wrote: > > On 24/02/15 04:18PM, Thomas Richard wrote: > > > From: Théo Lebrun <theo.lebrun@bootlin.com> > > > > > > Add suspend and resume support. Only the rc mode is supported. > > > > > > During the suspend stage PERST# is asserted, then deasserted during the > > > resume stage. > > > > Wouldn't this imply that the Endpoint device will be reset and therefore > > lose context? Or is it expected that the driver corresponding to the > > Endpoint Function in Linux will restore the state on resume, post reset? > > This does imply exactly that. Endpoint driver must be able to restore > context anyway, as system-wide suspend could mean lost power to PCI RC > controller (eg suspend-to-RAM) or PCI rails (dependent on hardware). Thank you for confirming. Regards, Siddharth.
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c index 9c588e79f5ac..cfa7ba237e1a 100644 --- a/drivers/pci/controller/cadence/pci-j721e.c +++ b/drivers/pci/controller/cadence/pci-j721e.c @@ -6,6 +6,7 @@ * Author: Kishon Vijay Abraham I <kishon@ti.com> */ +#include <linux/clk-provider.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> @@ -18,10 +19,13 @@ #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> +#include <linux/container_of.h> #include "../../pci.h" #include "pcie-cadence.h" +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) + #define ENABLE_REG_SYS_2 0x108 #define STATUS_REG_SYS_2 0x508 #define STATUS_CLR_REG_SYS_2 0x708 @@ -554,6 +558,91 @@ static void j721e_pcie_remove(struct platform_device *pdev) pm_runtime_disable(dev); } +static int j721e_pcie_suspend_noirq(struct device *dev) +{ + struct j721e_pcie *pcie = dev_get_drvdata(dev); + + if (pcie->mode == PCI_MODE_RC) { + gpiod_set_value_cansleep(pcie->reset_gpio, 0); + clk_disable_unprepare(pcie->refclk); + } + + cdns_pcie_disable_phy(pcie->cdns_pcie); + + return 0; +} + +static int j721e_pcie_resume_noirq(struct device *dev) +{ + struct j721e_pcie *pcie = dev_get_drvdata(dev); + struct cdns_pcie *cdns_pcie = pcie->cdns_pcie; + int ret; + + ret = j721e_pcie_ctrl_init(pcie); + if (ret < 0) { + dev_err(dev, "j721e_pcie_ctrl_init failed\n"); + return ret; + } + + j721e_pcie_config_link_irq(pcie); + + /* + * This is not called explicitly in the probe, it is called by + * cdns_pcie_init_phy. + */ + ret = cdns_pcie_enable_phy(pcie->cdns_pcie); + if (ret < 0) { + dev_err(dev, "cdns_pcie_enable_phy failed\n"); + return -ENODEV; + } + + if (pcie->mode == PCI_MODE_RC) { + struct cdns_pcie_rc *rc = cdns_pcie_to_rc(cdns_pcie); + + ret = clk_prepare_enable(pcie->refclk); + if (ret < 0) { + dev_err(dev, "clk_prepare_enable failed\n"); + return -ENODEV; + } + + /* + * "Power Sequencing and Reset Signal Timings" table in + * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 3.0 + * indicates PERST# should be deasserted after minimum of 100us + * once REFCLK is stable. The REFCLK to the connector in RC + * mode is selected while enabling the PHY. So deassert PERST# + * after 100 us. + */ + if (pcie->reset_gpio) { + usleep_range(100, 200); + gpiod_set_value_cansleep(pcie->reset_gpio, 1); + } + + ret = cdns_pcie_host_link_setup(rc); + if (ret < 0) { + clk_disable_unprepare(pcie->refclk); + return ret; + } + + /* + * Reset internal status of BARs to force reinitialization in + * cdns_pcie_host_init(). + */ + for (enum cdns_pcie_rp_bar bar = RP_BAR0; bar <= RP_NO_BAR; bar++) + rc->avail_ib_bar[bar] = true; + + ret = cdns_pcie_host_init(rc); + if (ret) + return ret; + } + + return 0; +} + +static DEFINE_NOIRQ_DEV_PM_OPS(j721e_pcie_pm_ops, + j721e_pcie_suspend_noirq, + j721e_pcie_resume_noirq); + static struct platform_driver j721e_pcie_driver = { .probe = j721e_pcie_probe, .remove_new = j721e_pcie_remove, @@ -561,6 +650,7 @@ static struct platform_driver j721e_pcie_driver = { .name = "j721e-pcie", .of_match_table = of_j721e_pcie_match, .suppress_bind_attrs = true, + .pm = pm_sleep_ptr(&j721e_pcie_pm_ops), }, }; builtin_platform_driver(j721e_pcie_driver);