On Tue, Feb 04, 2025 at 05:23:05PM +0100, Christian Bruel wrote:
[...]
> > > +static int stm32_pcie_suspend_noirq(struct device *dev)
> > > +{
> >
> > Can you consider making use of dw_pcie_{suspend/resume}_noirq()?
>
> I considered this, but dw_pcie_suspend_noirq needs to be tweaked as it
> checks both the pme_turn_off hook and whether we are entering into L2, which
> we don't support.
>
> For the former, I can check the PCI_EXP_DEVSTAT_AUXPD capability before
> polling for L2 LTSSM. It looks like only the Layerscape platform uses this.
> I will need a Tested-by for this new dw_pcie_suspend_noirq.
>
> Do you advise keeping stm32_pcie_suspend_noirq or modifying
> dw_pcie_suspend_noirq to this effect?
>
Please modify dw_pcie_suspend_noirq() to fit your needs. We will review the
change.
For testing the change, you can CC the Layerscape maintainer and request for
testing.
> Thanks,
>
> >
> > > + struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> > > +
> > > + stm32_pcie_stop_link(&stm32_pcie->pci);
> > > +
> > > + stm32_pcie_assert_perst(stm32_pcie);
> > > +
> > > + clk_disable_unprepare(stm32_pcie->clk);
> > > +
> > > + if (!device_may_wakeup(dev))
> > > + phy_exit(stm32_pcie->phy);
> > > +
> > > + return pinctrl_pm_select_sleep_state(dev);
> > > +}
> > > +
> > > +static int stm32_pcie_resume_noirq(struct device *dev)
> > > +{
> > > + struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> > > + struct dw_pcie_rp *pp = &stm32_pcie->pci.pp;
> > > + int ret;
> > > +
> > > + /*
> > > + * The core clock is gated with CLKREQ# from the COMBOPHY REFCLK,
> > > + * thus if no device is present, must force it low with an init pinmux
> > > + * to be able to access the DBI registers.
> > > + */
> > > + if (!IS_ERR(dev->pins->init_state))
> > > + ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
> > > + else
> > > + ret = pinctrl_pm_select_default_state(dev);
> > > +
> > > + if (ret) {
> > > + dev_err(dev, "Failed to activate pinctrl pm state: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + if (!device_may_wakeup(dev)) {
> > > + ret = phy_init(stm32_pcie->phy);
> > > + if (ret) {
> > > + pinctrl_pm_select_default_state(dev);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + ret = clk_prepare_enable(stm32_pcie->clk);
> > > + if (ret)
> > > + goto err_phy_exit;
> > > +
> > > + stm32_pcie_deassert_perst(stm32_pcie);
> > > +
> > > + ret = dw_pcie_setup_rc(pp);
> > > + if (ret)
> > > + goto err_disable_clk;
> > > +
> > > + ret = stm32_pcie_start_link(&stm32_pcie->pci);
> > > + if (ret)
> > > + goto err_disable_clk;
> > > +
> > > + /* Ignore errors, the link may come up later */
> > > + dw_pcie_wait_for_link(&stm32_pcie->pci);
> >
> > These can be dropped when using dw_pcie_resume_noirq().
>
> OK for dw_pcie_resume_noirq if we can keep it balanced with
> dw_pcie_suspend_noirq
>
> >
> > > +
> > > + pinctrl_pm_select_default_state(dev);
> > > +
> > > + return 0;
> > > +
> > > +err_disable_clk:
> > > + stm32_pcie_assert_perst(stm32_pcie);
> > > + clk_disable_unprepare(stm32_pcie->clk);
> > > +
> > > +err_phy_exit:
> > > + phy_exit(stm32_pcie->phy);
> > > + pinctrl_pm_select_default_state(dev);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static const struct dev_pm_ops stm32_pcie_pm_ops = {
> > > + NOIRQ_SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend_noirq,
> > > + stm32_pcie_resume_noirq)
> > > + SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend, stm32_pcie_resume)
> > > +};
> > > +
> > > +static const struct dw_pcie_host_ops stm32_pcie_host_ops = {
> > > +};
> > > +
> > > +static const struct dw_pcie_ops dw_pcie_ops = {
> > > + .start_link = stm32_pcie_start_link,
> > > + .stop_link = stm32_pcie_stop_link
> > > +};
> > > +
> > > +static int stm32_add_pcie_port(struct stm32_pcie *stm32_pcie,
> > > + struct platform_device *pdev)
> > > +{
> > > + struct device *dev = stm32_pcie->pci.dev;
> > > + struct dw_pcie_rp *pp = &stm32_pcie->pci.pp;
> > > + int ret;
> > > +
> >
> > You need to assert PERST# before configuring the resources.
>
> It is already initialized to GPIOD_OUT_HIGH in gpiod_get, I can have an
> explicit stm32_pcie_assert_perst but is it necessary ?
>
Ok, not necessary. Although, I would recommend to keep a comment here so that if
someone refactors the code, they would notice it.
> >
> > > + ret = phy_set_mode(stm32_pcie->phy, PHY_MODE_PCIE);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = phy_init(stm32_pcie->phy);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> > > + STM32MP25_PCIECR_TYPE_MASK,
> > > + STM32MP25_PCIECR_RC);
> > > + if (ret)
> > > + goto err_phy_exit;
> > > +
> > > + reset_control_assert(stm32_pcie->rst);
> > > + reset_control_deassert(stm32_pcie->rst);
> > > +
> > > + ret = clk_prepare_enable(stm32_pcie->clk);
> > > + if (ret) {
> > > + dev_err(dev, "Core clock enable failed %d\n", ret);
> > > + goto err_phy_exit;
> > > + }
> > > +
> > > + stm32_pcie_deassert_perst(stm32_pcie);
> > > +
> > > + pp->ops = &stm32_pcie_host_ops;
> > > + ret = dw_pcie_host_init(pp);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to initialize host: %d\n", ret);
> > > + goto err_disable_clk;
> > > + }
> >
> > Technically, dw_pcie_host_init() is not related to root port. So please move it
> > to probe() instead.
>
> OK will move, thanks
>
> >
> > > +
> > > + return 0;
> > > +
> > > +err_disable_clk:
> > > + clk_disable_unprepare(stm32_pcie->clk);
> > > + stm32_pcie_assert_perst(stm32_pcie);
> > > +
> > > +err_phy_exit:
> > > + phy_exit(stm32_pcie->phy);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int stm32_pcie_parse_port(struct stm32_pcie *stm32_pcie)
> > > +{
> > > + struct device *dev = stm32_pcie->pci.dev;
> > > + struct device_node *root_port;
> > > +
> > > + root_port = of_get_next_available_child(dev->of_node, NULL);
> > > +
> > > + stm32_pcie->phy = devm_of_phy_get(dev, root_port, NULL);
> > > + if (IS_ERR(stm32_pcie->phy))
> > > + return dev_err_probe(dev, PTR_ERR(stm32_pcie->phy),
> > > + "Failed to get pcie-phy\n");
> >
> > OF refcount not decremented in both the error and success case.
>
> I don't understand your point, isn't devm_of_phy_get managed to decrement
> the phy resources ?
>
You should drop the refcount of the root_port using of_node_put().
> >
> > > +
> > > + stm32_pcie->perst_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(root_port),
> > > + "reset", GPIOD_OUT_HIGH, NULL);
> > > + if (IS_ERR(stm32_pcie->perst_gpio)) {
> > > + if (PTR_ERR(stm32_pcie->perst_gpio) != -ENOENT)
> > > + return dev_err_probe(dev, PTR_ERR(stm32_pcie->perst_gpio),
> > > + "Failed to get reset GPIO\n");
> > > + stm32_pcie->perst_gpio = NULL;
> > > + }
> > > +
> > > + if (device_property_read_bool(dev, "wakeup-source")) {
> >
> > As per the current logic, 'wakeup-source' is applicable even without WAKE# GPIO,
> > which doesn't make sense.
>
> Agree, wakeup-source is not needed
>
> >
> > > + stm32_pcie->wake_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(root_port),
> > > + "wake", GPIOD_IN, NULL);
> > > +
> > > + if (IS_ERR(stm32_pcie->wake_gpio)) {
> > > + if (PTR_ERR(stm32_pcie->wake_gpio) != -ENOENT)
> > > + return dev_err_probe(dev, PTR_ERR(stm32_pcie->wake_gpio),
> > > + "Failed to get wake GPIO\n");
> > > + stm32_pcie->wake_gpio = NULL;
> > > + }
> >
> > Hmm. I think we need to move WAKE# handling inside drivers/pci/pcie/portdrv.c
> > since that is responsible for the root port. While other root port properties
> > have some dependency with the RC (like PERST#, PHY etc...), WAKE# handling could
> > be moved safel >
> > And once done, it can benefit all platforms.
>
> OK I'll check if there is a convenient way to do this through a port_service
>
You can drop the WAKE# support altogether and add it in the subsequent patches
once this initial driver gets merged. It is up to you.
> >
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
[...]
> > You can just move these definitions inside the driver itself.
> >
>
> Some definitions will be duplicated with the ep driver, but on the other
> side this file is very small... is it OK to duplicate definitions instead of
> having the bitfields together ?
>
I didn't notice that these are reused by the ep driver. Fine with me.
- Mani
--
மணிவண்ணன் சதாசிவம்
