| Message ID | 20240102-j7200-pcie-s2r-v1-14-84e55da52400@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Add suspend and resume support for rc mode. Same comments as for earlier patches. Since it's wide, please, check the whole series for the same issues and address them. ... > + if (pcie->reset_gpio) Dup, why? > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); ... > + if (pcie->reset_gpio) { > + usleep_range(100, 200); fsleep() ? Btw, why is it needed here, perhaps a comment? > + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > + } ... > + ret = cdns_pcie_host_setup(rc, false); > + if (ret < 0) { > + clk_disable_unprepare(pcie->refclk); > + return -ENODEV; Why is the error code being shadowed? > + } ... > +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) Is container_of.h included in this file? ... > @@ -381,7 +383,6 @@ struct cdns_pcie_ep { > unsigned int quirk_disable_flr:1; > }; > > - > /* Register access */ > static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) > { Stray change.
Hello Andy, On 1/15/24 21:13, Andy Shevchenko wrote: > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> >> From: Théo Lebrun <theo.lebrun@bootlin.com> >> >> Add suspend and resume support for rc mode. > > Same comments as for earlier patches. > Since it's wide, please, check the whole series for the same issues > and address them. > > ... > >> + if (pcie->reset_gpio) > > Dup, why? This pcie->reset_gpio corresponds to PERST# of PCIe endpoints. I assert it during suspend, because I have to deassert it (with a delay) during resume stage [1]. > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > > ... > >> + if (pcie->reset_gpio) { >> + usleep_range(100, 200); > > fsleep() ? > Btw, why is it needed here, perhaps a comment? The comment should be the same than in the probe [1]. Should I copy it? Or should I just add a reference to the probe? [1] https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pci-j721e.c#L535 > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); >> + } > > ... > >> + ret = cdns_pcie_host_setup(rc, false); >> + if (ret < 0) { >> + clk_disable_unprepare(pcie->refclk); >> + return -ENODEV; > > Why is the error code being shadowed? Yes I should return ret instead. > >> + } > > ... > >> +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) > > Is container_of.h included in this file? linux/container_of.h is included in linux/kernel.h. And linux/kernel.h is included in pcie-cadence.h (https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pcie-cadence.h#L9). Regards,
On Mon, Jan 22, 2024 at 5:30 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 1/15/24 21:13, Andy Shevchenko wrote: > > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: ... > >> + if (pcie->reset_gpio) > > > > Dup, why? > > This pcie->reset_gpio corresponds to PERST# of PCIe endpoints. > I assert it during suspend, because I have to deassert it (with a delay) > during resume stage [1]. Ah, sorry for being unclear, I meant that gpiod_set_value*() already has that check, you don't need it here. > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); ... > >> + if (pcie->reset_gpio) { > >> + usleep_range(100, 200); > > > > fsleep() ? > > Btw, why is it needed here, perhaps a comment? > > The comment should be the same than in the probe [1]. > Should I copy it? Or should I just add a reference to the probe? > > [1] > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pci-j721e.c#L535 Either way works for me. > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > >> + } ... > >> +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) > > > > Is container_of.h included in this file? > > linux/container_of.h is included in linux/kernel.h. > And linux/kernel.h is included in pcie-cadence.h > (https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pcie-cadence.h#L9). Okay, so, try to clean up pcie-cadence.h so it won't use "proxy" headers. There is an IWYU (include what you use) principle, please follow it.
On 1/22/24 22:36, Andy Shevchenko wrote: > On Mon, Jan 22, 2024 at 5:30 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> On 1/15/24 21:13, Andy Shevchenko wrote: >>> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard >>> <thomas.richard@bootlin.com> wrote: > > ... > >>>> + if (pcie->reset_gpio) >>> >>> Dup, why? >> >> This pcie->reset_gpio corresponds to PERST# of PCIe endpoints. >> I assert it during suspend, because I have to deassert it (with a delay) >> during resume stage [1]. > > Ah, sorry for being unclear, I meant that gpiod_set_value*() already > has that check, you don't need it here. ok understood, check is useless. > >>>> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > > ... > >>>> + if (pcie->reset_gpio) { >>>> + usleep_range(100, 200); >>> >>> fsleep() ? >>> Btw, why is it needed here, perhaps a comment? >> >> The comment should be the same than in the probe [1]. >> Should I copy it? Or should I just add a reference to the probe? >> >> [1] >> https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pci-j721e.c#L535 > > Either way works for me. > >>>> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); >>>> + } > > ... > >>>> +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) >>> >>> Is container_of.h included in this file? >> >> linux/container_of.h is included in linux/kernel.h. >> And linux/kernel.h is included in pcie-cadence.h >> (https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pcie-cadence.h#L9). > > Okay, so, try to clean up pcie-cadence.h so it won't use "proxy" headers. > There is an IWYU (include what you use) principle, please follow it. In fact, as cdns_pcie_to_rc is only used in pci-j721e.c, no need to have it in a header file. I prefer to move cdns_pcie_to_rc definition in pci-j721e.c. As I don't modify pcie-cadence.h, the cleanup to not use proxy headers is something that it can be done outside this series.
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c index 477275d72257..51867a3f2499 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> @@ -554,6 +555,76 @@ static void j721e_pcie_remove(struct platform_device *pdev) pm_runtime_disable(dev); } +#ifdef CONFIG_PM +static int j721e_pcie_suspend_noirq(struct device *dev) +{ + struct j721e_pcie *pcie = dev_get_drvdata(dev); + + if (pcie->mode == PCI_MODE_RC) { + if (pcie->reset_gpio) + 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; + } + + if (pcie->reset_gpio) { + usleep_range(100, 200); + gpiod_set_value_cansleep(pcie->reset_gpio, 1); + } + + ret = cdns_pcie_host_setup(rc, false); + if (ret < 0) { + clk_disable_unprepare(pcie->refclk); + return -ENODEV; + } + } + + return 0; +} + +static const struct dev_pm_ops j721e_pcie_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(j721e_pcie_suspend_noirq, j721e_pcie_resume_noirq) +}; +#endif + static struct platform_driver j721e_pcie_driver = { .probe = j721e_pcie_probe, .remove_new = j721e_pcie_remove, @@ -561,6 +632,7 @@ static struct platform_driver j721e_pcie_driver = { .name = "j721e-pcie", .of_match_table = of_j721e_pcie_match, .suppress_bind_attrs = true, + .pm = pm_ptr(&j721e_pcie_pm_ops), }, }; builtin_platform_driver(j721e_pcie_driver); diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h index 3b0da889ed64..05d4b96fc71d 100644 --- a/drivers/pci/controller/cadence/pcie-cadence.h +++ b/drivers/pci/controller/cadence/pcie-cadence.h @@ -331,6 +331,8 @@ struct cdns_pcie_rc { unsigned int quirk_detect_quiet_flag:1; }; +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) + /** * struct cdns_pcie_epf - Structure to hold info about endpoint function * @epf: Info about virtual functions attached to the physical function @@ -381,7 +383,6 @@ struct cdns_pcie_ep { unsigned int quirk_disable_flr:1; }; - /* Register access */ static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) {