| Message ID | 8018069bc772376caef29e76280e4d0facffdb68.1532455922.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] PCI: imx: Add multi-pd support | expand |
On 24 July 2018 at 20:17, Leonard Crestez <leonard.crestez@nxp.com> wrote: > On some chips the PCIE and PCIE_PHY blocks are in separate power domains > which can be power-gated independently. The driver needs to handle this > by keeping both domain active. > > This is intended for imx6sx where PCIE is in DISPMIX and PCIE_PHY in > it's own domain. Defining the DISPMIX domain requires a way for pcie to > keep it active or it will break when displays are off. > > The power-domains on imx6sx are meant to look like this: > power-domains = <&pd_disp>, <&pd_pci>; > power-domain-names = "pcie", "pcie_phy"; > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> See some comments below... > --- > > Right now if a device has a single power domain it will be activated > before probe but if it has multiple domains they need to be explicitly > "attached" to and controlled. Supporting both is a bit awkward, this > patch makes the distinction based on (dev->pm_domain != NULL). > > Maybe the PM core should make this distinction based on a flag in struct > device_driver instead of number of power-domains? So by default when a > device has multiple power domains they would would be activated > together and this patch would be unnecessary. Activation is deliberately left to be manged by each user, as the PM core/genpd can't know when powering on the PM domains make sense. The main reason to why genpd powers on the PM domain for the single PM domain case, is because of legacy behaviors in drivers. Some drivers don't call pm_runtime_get_sync() during ->probe(), but instead use pm_runtime_set_active(), to synchronize the state with the HW. This means the ->runtime_resume() callback doesn't get invoked for genpd, hence genpd must power on the PM domain already at attache to cover this case. > > This is marked as "RFC" mostly because I believe it should be handled > inside PM core, without driver code. Does this make sense to anyone else? > > This is independent of recent patches adding suspend/resume support to > imx pci, but supporting suspend with multi-pd requires adding device > links, not just activating pds at probe time. > > The device_link is marked as "STATELESS" because otherwise a warning is > triggered in device_links_driver_bound. This seems to happen because the > pd devices are always marked as "DL_DEV_NO_DRIVER". Maybe they should be > instead always be marked as DL_DEV_DRIVER_BOUND? Using STATELESS is correct, because the supplier devices, which are managed by genpd don't have any driver attached to them. [...] Kind regards Uffe
On Tue, 2018-07-31 at 10:32 +0200, Ulf Hansson wrote: > On 24 July 2018 at 20:17, Leonard Crestez <leonard.crestez@nxp.com> wrote: > > On some chips the PCIE and PCIE_PHY blocks are in separate power domains > > which can be power-gated independently. The driver needs to handle this > > by keeping both domain active. > > > > This is intended for imx6sx where PCIE is in DISPMIX and PCIE_PHY in > > it's own domain. Defining the DISPMIX domain requires a way for pcie to > > keep it active or it will break when displays are off. > > > > The power-domains on imx6sx are meant to look like this: > > power-domains = <&pd_disp>, <&pd_pci>; > > power-domain-names = "pcie", "pcie_phy"; > > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Thanks for taking the time to look at this, I will likely repost this as PATCH in a series later. > > Right now if a device has a single power domain it will be activated > > before probe but if it has multiple domains they need to be explicitly > > "attached" to and controlled. Supporting both is a bit awkward, this > > patch makes the distinction based on (dev->pm_domain != NULL). > > > > Maybe the PM core should make this distinction based on a flag in struct > > device_driver instead of number of power-domains? So by default when a > > device has multiple power domains they would would be activated > > together and this patch would be unnecessary. > > Activation is deliberately left to be manged by each user, as the PM > core/genpd can't know when powering on the PM domains make sense. By default the PM core could treat a list of pm domains just like it treats a domain now. My patch doesn't do anything pci-specific, it just attaches all domains and creates device links. Couldn't the core do this? > The main reason to why genpd powers on the PM domain for the single PM > domain case, is because of legacy behaviors in drivers. Wouldn't it be nicer to out-out of such legacy behaviors with a flag in struct driver instead of single-versus-multiple domains? > > The device_link is marked as "STATELESS" because otherwise a warning is > > triggered in device_links_driver_bound. This seems to happen because the > > pd devices are always marked as "DL_DEV_NO_DRIVER". Maybe they should be > > instead always be marked as DL_DEV_DRIVER_BOUND? > > Using STATELESS is correct, because the supplier devices, which are > managed by genpd don't have any driver attached to them. As far as I can tell these genpd_dev devices need to be manually unregistered in the consumer's remove function, right? If they were marked with DL_DEV_DRIVER_BOUND then you could use DL_FLAG_AUTOREMOVE_SUPPLIER on them. As far as I can tell the DL_DEV_* states are used to deal with probing order but since no driver will ever bind to these suppliers we could treat them as effectively always bounds. Perhaps this can be revisited later, imx-pci is not a very good usecase for this because it doesn't even support remove.
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index fc9529619469..7403918010e9 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -25,10 +25,12 @@ #include <linux/resource.h> #include <linux/signal.h> #include <linux/types.h> #include <linux/interrupt.h> #include <linux/reset.h> +#include <linux/pm_domain.h> +#include <linux/pm_runtime.h> #include "pcie-designware.h" #define to_imx6_pcie(x) dev_get_drvdata((x)->dev) @@ -57,10 +59,15 @@ struct imx6_pcie { u32 tx_deemph_gen2_6db; u32 tx_swing_full; u32 tx_swing_low; int link_gen; struct regulator *vpcie; + + /* power domain for pcie itself (dispmix) */ + struct device *pd_pcie; + /* power domain for pcie phy */ + struct device *pd_pcie_phy; }; /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ #define PHY_PLL_LOCK_WAIT_MAX_RETRIES 2000 #define PHY_PLL_LOCK_WAIT_USLEEP_MIN 50 @@ -805,10 +812,47 @@ static int imx6_pcie_resume_noirq(struct device *dev) static const struct dev_pm_ops imx6_pcie_pm_ops = { SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq, imx6_pcie_resume_noirq) }; +static int imx6_pcie_attach_pd(struct device *dev) +{ + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); + struct device_link *link; + + /* Do nothing when in a single power domain */ + if (dev->pm_domain) + return 0; + + imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie"); + if (IS_ERR(imx6_pcie->pd_pcie)) + return PTR_ERR(imx6_pcie->pd_pcie); + link = device_link_add(dev, imx6_pcie->pd_pcie, + DL_FLAG_STATELESS | + DL_FLAG_PM_RUNTIME | + DL_FLAG_RPM_ACTIVE); + if (IS_ERR(link)) { + dev_err(dev, "Failed to add device_link to pcie pd: %ld\n", PTR_ERR(link)); + return PTR_ERR(link); + } + + imx6_pcie->pd_pcie_phy = dev_pm_domain_attach_by_name(dev, "pcie_phy"); + if (IS_ERR(imx6_pcie->pd_pcie_phy)) + return PTR_ERR(imx6_pcie->pd_pcie_phy); + + device_link_add(dev, imx6_pcie->pd_pcie_phy, + DL_FLAG_STATELESS | + DL_FLAG_PM_RUNTIME | + DL_FLAG_RPM_ACTIVE); + if (IS_ERR(link)) { + dev_err(dev, "Failed to add device_link to pcie_phy pd: %ld\n", PTR_ERR(link)); + return PTR_ERR(link); + } + + return 0; +} + static int imx6_pcie_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct dw_pcie *pci; struct imx6_pcie *imx6_pcie; @@ -945,10 +989,14 @@ static int imx6_pcie_probe(struct platform_device *pdev) imx6_pcie->vpcie = NULL; } platform_set_drvdata(pdev, imx6_pcie); + ret = imx6_pcie_attach_pd(dev); + if (ret) + return ret; + ret = imx6_add_pcie_port(imx6_pcie, pdev); if (ret < 0) return ret; return 0;
On some chips the PCIE and PCIE_PHY blocks are in separate power domains which can be power-gated independently. The driver needs to handle this by keeping both domain active. This is intended for imx6sx where PCIE is in DISPMIX and PCIE_PHY in it's own domain. Defining the DISPMIX domain requires a way for pcie to keep it active or it will break when displays are off. The power-domains on imx6sx are meant to look like this: power-domains = <&pd_disp>, <&pd_pci>; power-domain-names = "pcie", "pcie_phy"; Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- Right now if a device has a single power domain it will be activated before probe but if it has multiple domains they need to be explicitly "attached" to and controlled. Supporting both is a bit awkward, this patch makes the distinction based on (dev->pm_domain != NULL). Maybe the PM core should make this distinction based on a flag in struct device_driver instead of number of power-domains? So by default when a device has multiple power domains they would would be activated together and this patch would be unnecessary. This is marked as "RFC" mostly because I believe it should be handled inside PM core, without driver code. Does this make sense to anyone else? This is independent of recent patches adding suspend/resume support to imx pci, but supporting suspend with multi-pd requires adding device links, not just activating pds at probe time. The device_link is marked as "STATELESS" because otherwise a warning is triggered in device_links_driver_bound. This seems to happen because the pd devices are always marked as "DL_DEV_NO_DRIVER". Maybe they should be instead always be marked as DL_DEV_DRIVER_BOUND? The imx pci driver doesn't support unbind or removal anyway so this is not handled for multi-pd either. Previously: https://lkml.org/lkml/2018/7/10/230 --- drivers/pci/controller/dwc/pci-imx6.c | 48 +++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)