diff mbox

[4/8] soc: imx: gpc: fix imx6sl gpc power domain regression

Message ID 20170322192356.GC16264@b29396-OptiPlex-7040 (mailing list archive)
State New, archived
Headers show

Commit Message

Dong Aisheng March 22, 2017, 7:23 p.m. UTC 
On Mon, Mar 20, 2017 at 10:26:14AM +0100, Lucas Stach wrote:
> Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > broke the MX6SL GPC power domain support.
> > It always got the following error:
> > [    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
> > This patch adds back the legecy support.
> 
> This patch removes a safeguard against abusing the old binding for new
> SoCs. Please leave this check in place and instead loosen the check to
> also allow the old i.MX6SL binding.
> 

Thanks for the reminder.
I found beside the checking, we probably also need some extra fix that
the current imx_gpc_old_dt_init() only supports two domains while the
driver formerly supports three for MX6SL.

And because the num_domains is defined by driver itself, no need check
in imx_gpc_old_dt_init() anymore.

The patch would be as follows:

Do you think it's ok?

Regards
Dong Aisheng

> > 
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/soc/imx/gpc.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > index ba6e7ab..c1d0e67 100644
> > --- a/drivers/soc/imx/gpc.c
> > +++ b/drivers/soc/imx/gpc.c
> > @@ -385,12 +385,6 @@ static int imx_gpc_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	if (!pgc_node) {
> > -		/* old DT layout is only supported for mx6q aka 2 domains */
> > -		if (of_id_data->num_domains != 2) {
> > -			dev_err(&pdev->dev, "could not find pgc DT node\n");
> > -			return -ENODEV;
> > -		}
> > -
> >  		ret = imx_gpc_old_dt_init(&pdev->dev, regmap);
> >  		if (ret)
> >  			return ret;
> 
>

Comments

Lucas Stach March 22, 2017, 8:46 a.m. UTC | #1 
Am Donnerstag, den 23.03.2017, 03:23 +0800 schrieb Dong Aisheng:
> On Mon, Mar 20, 2017 at 10:26:14AM +0100, Lucas Stach wrote:
> > Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > > Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > > broke the MX6SL GPC power domain support.
> > > It always got the following error:
> > > [    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
> > > This patch adds back the legecy support.
> > 
> > This patch removes a safeguard against abusing the old binding for new
> > SoCs. Please leave this check in place and instead loosen the check to
> > also allow the old i.MX6SL binding.
> > 
> 
> Thanks for the reminder.
> I found beside the checking, we probably also need some extra fix that
> the current imx_gpc_old_dt_init() only supports two domains while the
> driver formerly supports three for MX6SL.
> 
> And because the num_domains is defined by driver itself, no need check
> in imx_gpc_old_dt_init() anymore.
> 
> The patch would be as follows:
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 4885ec2..6a806a1 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -303,10 +303,13 @@ static struct genpd_onecell_data imx_gpc_onecell_data = {
>  
>  static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
>  {
> +       const struct of_device_id *of_id =
> +                       of_match_device(imx_gpc_dt_ids, dev);
> +       const struct imx_gpc_dt_data *of_id_data = of_id->data;
>         struct imx_pm_domain *domain;
>         int i, ret;
>  
> -       for (i = 0; i < 2; i++) {
> +       for (i = 0; i < of_id_data->num_domains; i++) {
>                 domain = &imx_gpc_domains[i];
>                 domain->regmap = regmap;
>                 domain->ipg_rate_mhz = 66;
> @@ -324,7 +327,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
>                 }
>         }
>  
> -       for (i = 0; i < 2; i++)
> +       for (i = 0; i < of_id_data->num_domains; i++)
>                 pm_genpd_init(&imx_gpc_domains[i].base, NULL, false);
>  
>         if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
> @@ -337,7 +340,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
>         return 0;
>  
>  genpd_err:
> -       for (i = 0; i < 2; i++)
> +       for (i = 0; i < of_id_data->num_domains; i++)
>                 pm_genpd_remove(&imx_gpc_domains[i].base);
>         imx_pgc_put_clocks(&imx_gpc_domains[1]);
>  clk_err:
> 
> Do you think it's ok?
> 
I don't like the redundant of_match_id. Please just pass num_domains as
a parameter to imx_gpc_old_dt_init(). Otherwise this looks good.

Regards,
Lucas
Dong Aisheng March 23, 2017, 12:47 a.m. UTC | #2 
On Wed, Mar 22, 2017 at 09:46:10AM +0100, Lucas Stach wrote:
> Am Donnerstag, den 23.03.2017, 03:23 +0800 schrieb Dong Aisheng:
> > On Mon, Mar 20, 2017 at 10:26:14AM +0100, Lucas Stach wrote:
> > > Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng:
> > > > Commit 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver")
> > > > broke the MX6SL GPC power domain support.
> > > > It always got the following error:
> > > > [    1.248364] imx-gpc 20dc000.gpc: could not find pgc DT node
> > > > This patch adds back the legecy support.
> > > 
> > > This patch removes a safeguard against abusing the old binding for new
> > > SoCs. Please leave this check in place and instead loosen the check to
> > > also allow the old i.MX6SL binding.
> > > 
> > 
> > Thanks for the reminder.
> > I found beside the checking, we probably also need some extra fix that
> > the current imx_gpc_old_dt_init() only supports two domains while the
> > driver formerly supports three for MX6SL.
> > 
> > And because the num_domains is defined by driver itself, no need check
> > in imx_gpc_old_dt_init() anymore.
> > 
> > The patch would be as follows:
> > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> > index 4885ec2..6a806a1 100644
> > --- a/drivers/soc/imx/gpc.c
> > +++ b/drivers/soc/imx/gpc.c
> > @@ -303,10 +303,13 @@ static struct genpd_onecell_data imx_gpc_onecell_data = {
> >  
> >  static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
> >  {
> > +       const struct of_device_id *of_id =
> > +                       of_match_device(imx_gpc_dt_ids, dev);
> > +       const struct imx_gpc_dt_data *of_id_data = of_id->data;
> >         struct imx_pm_domain *domain;
> >         int i, ret;
> >  
> > -       for (i = 0; i < 2; i++) {
> > +       for (i = 0; i < of_id_data->num_domains; i++) {
> >                 domain = &imx_gpc_domains[i];
> >                 domain->regmap = regmap;
> >                 domain->ipg_rate_mhz = 66;
> > @@ -324,7 +327,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
> >                 }
> >         }
> >  
> > -       for (i = 0; i < 2; i++)
> > +       for (i = 0; i < of_id_data->num_domains; i++)
> >                 pm_genpd_init(&imx_gpc_domains[i].base, NULL, false);
> >  
> >         if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
> > @@ -337,7 +340,7 @@ static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
> >         return 0;
> >  
> >  genpd_err:
> > -       for (i = 0; i < 2; i++)
> > +       for (i = 0; i < of_id_data->num_domains; i++)
> >                 pm_genpd_remove(&imx_gpc_domains[i].base);
> >         imx_pgc_put_clocks(&imx_gpc_domains[1]);
> >  clk_err:
> > 
> > Do you think it's ok?
> > 
> I don't like the redundant of_match_id. Please just pass num_domains as
> a parameter to imx_gpc_old_dt_init(). Otherwise this looks good.
> 

Yes, of course, will update it.

Thanks for the suggestion.

Regards
Dong Aisheng

> Regards,
> Lucas
>
diff mbox

Patch

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 4885ec2..6a806a1 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -303,10 +303,13 @@  static struct genpd_onecell_data imx_gpc_onecell_data = {
 
 static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
 {
+       const struct of_device_id *of_id =
+                       of_match_device(imx_gpc_dt_ids, dev);
+       const struct imx_gpc_dt_data *of_id_data = of_id->data;
        struct imx_pm_domain *domain;
        int i, ret;
 
-       for (i = 0; i < 2; i++) {
+       for (i = 0; i < of_id_data->num_domains; i++) {
                domain = &imx_gpc_domains[i];
                domain->regmap = regmap;
                domain->ipg_rate_mhz = 66;
@@ -324,7 +327,7 @@  static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
                }
        }
 
-       for (i = 0; i < 2; i++)
+       for (i = 0; i < of_id_data->num_domains; i++)
                pm_genpd_init(&imx_gpc_domains[i].base, NULL, false);
 
        if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
@@ -337,7 +340,7 @@  static int imx_gpc_old_dt_init(struct device *dev, struct regmap *regmap)
        return 0;
 
 genpd_err:
-       for (i = 0; i < 2; i++)
+       for (i = 0; i < of_id_data->num_domains; i++)
                pm_genpd_remove(&imx_gpc_domains[i].base);
        imx_pgc_put_clocks(&imx_gpc_domains[1]);
 clk_err: