| Message ID | 20180108102855.GA32635@b29396-OptiPlex-7040 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Montag, den 08.01.2018, 18:28 +0800 schrieb Dong Aisheng: > On Sun, Jan 07, 2018 at 02:49:05PM +0100, Stefan Agner wrote: > > If power domain information are missing in the device tree, no > > power domains get initialized. However, imx_gpc_remove tries to > > remove power domains always in the old DT binding case. Only > > remove power domains when imx_gpc_probe initialized them in > > first place. > > > > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC > > driver") > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > --- > > drivers/soc/imx/gpc.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c > > index 53f7275d6cbd..62bb724726d9 100644 > > --- a/drivers/soc/imx/gpc.c > > +++ b/drivers/soc/imx/gpc.c > > @@ -470,13 +470,21 @@ static int imx_gpc_probe(struct > > platform_device *pdev) > > > > static int imx_gpc_remove(struct platform_device *pdev) > > { > > What's the original purpose of imx_gpc_remove? > ARM power domain can't be removed. Why? As long as it stays powered on there is not reason why we wouldn't be able to remove the driver. > And why current imx_gpc_remove only remove domains for old DT but not > for new ones? With the new binding the power domains will be removed by the sub- drivers for the domains. > How about make it un-removable? > e.g. I don't see why this would be a good idea. Once more device-dependency handling is in place we might need to unbind the power domains when the regulator driver for the domain is unbound. Do you intend to make them non-removable, too? Regards, Lucas > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c > index 47e7aa9..7fc6737 100644 > --- a/drivers/soc/imx/gpc.c > +++ b/drivers/soc/imx/gpc.c > @@ -454,36 +454,17 @@ static int imx_gpc_probe(struct platform_device > *pdev) > return 0; > } > > -static int imx_gpc_remove(struct platform_device *pdev) > -{ > - int ret; > - > - /* > - * If the old DT binding is used the toplevel driver needs to > - * de-register the power domains > - */ > - if (!of_get_child_by_name(pdev->dev.of_node, "pgc")) { > - of_genpd_del_provider(pdev->dev.of_node); > - > - ret = > pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base); > - if (ret) > - return ret; > - imx_pgc_put_clocks(&imx_gpc_domains[GPC_PGC_DOMAIN_PU > ]); > - > - ret = > pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_ARM].base); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > static struct platform_driver imx_gpc_driver = { > .driver = { > .name = "imx-gpc", > .of_match_table = imx_gpc_dt_ids, > + /* > + * We can't forcibly eject devices form power > domain, > + * so we can't really remove power domains once they > + * were added. > + */ > + .suppress_bind_attrs = true, > }, > .probe = imx_gpc_probe, > - .remove = imx_gpc_remove, > }; > builtin_platform_driver(imx_gpc_driver) > > Regards > Dong Aisheng > > > + struct device_node *pgc_node; > > int ret; > > > > + pgc_node = of_get_child_by_name(pdev->dev.of_node, "pgc"); > > + > > + /* bail out if DT too old and doesn't provide the > > necessary info */ > > + if (!of_property_read_bool(pdev->dev.of_node, "#power- > > domain-cells") && > > + !pgc_node) > > + return 0; > > + > > /* > > * If the old DT binding is used the toplevel driver needs > > to > > * de-register the power domains > > */ > > - if (!of_get_child_by_name(pdev->dev.of_node, "pgc")) { > > + if (!pgc_node) { > > of_genpd_del_provider(pdev->dev.of_node); > > > > ret = > > pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base); > > -- > > 2.15.1 > >
On 2018-01-08 11:51, Lucas Stach wrote: > Am Montag, den 08.01.2018, 18:28 +0800 schrieb Dong Aisheng: >> On Sun, Jan 07, 2018 at 02:49:05PM +0100, Stefan Agner wrote: >> > If power domain information are missing in the device tree, no >> > power domains get initialized. However, imx_gpc_remove tries to >> > remove power domains always in the old DT binding case. Only >> > remove power domains when imx_gpc_probe initialized them in >> > first place. >> > >> > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC >> > driver") >> > Cc: Lucas Stach <l.stach@pengutronix.de> >> > Signed-off-by: Stefan Agner <stefan@agner.ch> >> > --- >> > drivers/soc/imx/gpc.c | 10 +++++++++- >> > 1 file changed, 9 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c >> > index 53f7275d6cbd..62bb724726d9 100644 >> > --- a/drivers/soc/imx/gpc.c >> > +++ b/drivers/soc/imx/gpc.c >> > @@ -470,13 +470,21 @@ static int imx_gpc_probe(struct >> > platform_device *pdev) >> > >> > static int imx_gpc_remove(struct platform_device *pdev) >> > { >> >> What's the original purpose of imx_gpc_remove? >> ARM power domain can't be removed. > > Why? As long as it stays powered on there is not reason why we wouldn't > be able to remove the driver. > Is it really safe to make assumptions of the hardware state when drivers get removed? At least some drivers disable the hardware on remove (e.g. i.MX SPI driver). >> And why current imx_gpc_remove only remove domains for old DT but not >> for new ones? > > With the new binding the power domains will be removed by the sub- > drivers for the domains. > >> How about make it un-removable? >> e.g. > > I don't see why this would be a good idea. Once more device-dependency > handling is in place we might need to unbind the power domains when the > regulator driver for the domain is unbound. Do you intend to make them > non-removable, too? I think it would be preferable to keep the ability to remote the driver. However, I noticed that even with this fix, with device trees which do use the power domains capabilities (e.g. i.MX6DL) it leads to a stack trace when using DEBUG_TEST_DRIVER_REMOVE=y, see: https://marc.info/?l=linux-arm-kernel&m=151544599904423&w=4 -- Stefan > > Regards, > Lucas > >> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c >> index 47e7aa9..7fc6737 100644 >> --- a/drivers/soc/imx/gpc.c >> +++ b/drivers/soc/imx/gpc.c >> @@ -454,36 +454,17 @@ static int imx_gpc_probe(struct platform_device >> *pdev) >> return 0; >> } >> >> -static int imx_gpc_remove(struct platform_device *pdev) >> -{ >> - int ret; >> - >> - /* >> - * If the old DT binding is used the toplevel driver needs to >> - * de-register the power domains >> - */ >> - if (!of_get_child_by_name(pdev->dev.of_node, "pgc")) { >> - of_genpd_del_provider(pdev->dev.of_node); >> - >> - ret = >> pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base); >> - if (ret) >> - return ret; >> - imx_pgc_put_clocks(&imx_gpc_domains[GPC_PGC_DOMAIN_PU >> ]); >> - >> - ret = >> pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_ARM].base); >> - if (ret) >> - return ret; >> - } >> - >> - return 0; >> -} >> - >> static struct platform_driver imx_gpc_driver = { >> .driver = { >> .name = "imx-gpc", >> .of_match_table = imx_gpc_dt_ids, >> + /* >> + * We can't forcibly eject devices form power >> domain, >> + * so we can't really remove power domains once they >> + * were added. >> + */ >> + .suppress_bind_attrs = true, >> }, >> .probe = imx_gpc_probe, >> - .remove = imx_gpc_remove, >> }; >> builtin_platform_driver(imx_gpc_driver) >> >> Regards >> Dong Aisheng >> >> > + struct device_node *pgc_node; >> > int ret; >> > >> > + pgc_node = of_get_child_by_name(pdev->dev.of_node, "pgc"); >> > + >> > + /* bail out if DT too old and doesn't provide the >> > necessary info */ >> > + if (!of_property_read_bool(pdev->dev.of_node, "#power- >> > domain-cells") && >> > + !pgc_node) >> > + return 0; >> > + >> > /* >> > * If the old DT binding is used the toplevel driver needs >> > to >> > * de-register the power domains >> > */ >> > - if (!of_get_child_by_name(pdev->dev.of_node, "pgc")) { >> > + if (!pgc_node) { >> > of_genpd_del_provider(pdev->dev.of_node); >> > >> > ret = >> > pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base); >> > -- >> > 2.15.1 >> >
Am Montag, den 08.01.2018, 22:17 +0100 schrieb Stefan Agner: > On 2018-01-08 11:51, Lucas Stach wrote: > > Am Montag, den 08.01.2018, 18:28 +0800 schrieb Dong Aisheng: > > > On Sun, Jan 07, 2018 at 02:49:05PM +0100, Stefan Agner wrote: > > > > If power domain information are missing in the device tree, no > > > > power domains get initialized. However, imx_gpc_remove tries to > > > > remove power domains always in the old DT binding case. Only > > > > remove power domains when imx_gpc_probe initialized them in > > > > first place. > > > > > > > > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC > > > > driver") > > > > > > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > > > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > > > --- > > > > drivers/soc/imx/gpc.c | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c > > > > index 53f7275d6cbd..62bb724726d9 100644 > > > > --- a/drivers/soc/imx/gpc.c > > > > +++ b/drivers/soc/imx/gpc.c > > > > @@ -470,13 +470,21 @@ static int imx_gpc_probe(struct > > > > platform_device *pdev) > > > > > > > > static int imx_gpc_remove(struct platform_device *pdev) > > > > { > > > > > > What's the original purpose of imx_gpc_remove? > > > ARM power domain can't be removed. > > > > Why? As long as it stays powered on there is not reason why we wouldn't > > be able to remove the driver. > > > > Is it really safe to make assumptions of the hardware state when drivers > get removed? At least some drivers disable the hardware on remove (e.g. > i.MX SPI driver). You are completely right that we should do something more sensible on remove. Like making sure the domain is powered on or (preferably for the non ARM domains) unbinding the consumers. > > > And why current imx_gpc_remove only remove domains for old DT but not > > > for new ones? > > > > With the new binding the power domains will be removed by the sub- > > drivers for the domains. > > > > > How about make it un-removable? > > > e.g. > > > > I don't see why this would be a good idea. Once more device-dependency > > handling is in place we might need to unbind the power domains when the > > regulator driver for the domain is unbound. Do you intend to make them > > non-removable, too? > > I think it would be preferable to keep the ability to remote the driver. > > However, I noticed that even with this fix, with device trees which do > use the power domains capabilities (e.g. i.MX6DL) it leads to a stack > trace when using DEBUG_TEST_DRIVER_REMOVE=y, see: > https://marc.info/?l=linux-arm-kernel&m=151544599904423&w=4 Urgh. Yes, we should fix this. This is really missing a device dependency between the core GPC driver and the PM domain drivers. With this dependency in place we can make sure to unbind the domain driver before the core driver goes away. Regards, Lucas
diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c index 47e7aa9..7fc6737 100644 --- a/drivers/soc/imx/gpc.c +++ b/drivers/soc/imx/gpc.c @@ -454,36 +454,17 @@ static int imx_gpc_probe(struct platform_device *pdev) return 0; } -static int imx_gpc_remove(struct platform_device *pdev) -{ - int ret; - - /* - * If the old DT binding is used the toplevel driver needs to - * de-register the power domains - */ - if (!of_get_child_by_name(pdev->dev.of_node, "pgc")) { - of_genpd_del_provider(pdev->dev.of_node); - - ret = pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base); - if (ret) - return ret; - imx_pgc_put_clocks(&imx_gpc_domains[GPC_PGC_DOMAIN_PU]); - - ret = pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_ARM].base); - if (ret) - return ret; - } - - return 0; -} - static struct platform_driver imx_gpc_driver = { .driver = { .name = "imx-gpc", .of_match_table = imx_gpc_dt_ids, + /* + * We can't forcibly eject devices form power domain, + * so we can't really remove power domains once they + * were added. + */ + .suppress_bind_attrs = true, }, .probe = imx_gpc_probe, - .remove = imx_gpc_remove, }; builtin_platform_driver(imx_gpc_driver)