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.st...@pengutronix.de> >> > Signed-off-by: Stefan Agner <ste...@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 >> >