From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754286AbeAHVRy (ORCPT + 1 other); Mon, 8 Jan 2018 16:17:54 -0500 Received: from mail.kmu-office.ch ([178.209.48.109]:44392 "EHLO mail.kmu-office.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750983AbeAHVRx (ORCPT ); Mon, 8 Jan 2018 16:17:53 -0500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Date: Mon, 08 Jan 2018 22:17:34 +0100 From: Stefan Agner To: Lucas Stach Cc: Dong Aisheng , shawnguo@kernel.org, kernel@pengutronix.de, fabio.estevam@nxp.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-imx@nxp.com Subject: Re: [PATCH] soc: imx: gpc: de-register power domains only if initialized In-Reply-To: <1515408692.12538.10.camel@pengutronix.de> References: <20180107134905.15624-1-stefan@agner.ch> <20180108102855.GA32635@b29396-OptiPlex-7040> <1515408692.12538.10.camel@pengutronix.de> Message-ID: User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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 >> > Signed-off-by: Stefan Agner >> > --- >> >  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 >> >