[PATCH] ARM: imx: Drop imx_anatop_init()

[PATCH] ARM: imx: Drop imx_anatop_init()

[Andrey Smirnov]

With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
detect in mxs_phy_hw_init()") in tree all of the necessary charger
setup is done by the USB PHY driver which covers all of the affected
i.MX6 SoCs.

NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
datasheet it appears to have a different USB PHY IP block, so
executing i.MX6 charger disable configuration seems unnecessary.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Peter Chen <peter.chen@nxp.com>
Cc: linux-imx@nxp.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/mach-imx/anatop.c      | 25 -------------------------
 arch/arm/mach-imx/common.h      |  1 -
 arch/arm/mach-imx/mach-imx6q.c  |  1 -
 arch/arm/mach-imx/mach-imx6sl.c |  1 -
 arch/arm/mach-imx/mach-imx6sx.c |  1 -
 arch/arm/mach-imx/mach-imx6ul.c |  1 -
 arch/arm/mach-imx/mach-imx7d.c  |  1 -
 7 files changed, 31 deletions(-)

diff --git a/arch/arm/mach-imx/anatop.c b/arch/arm/mach-imx/anatop.c
index 777d8c255501..f2c9fe14198a 100644
--- a/arch/arm/mach-imx/anatop.c
+++ b/arch/arm/mach-imx/anatop.c
@@ -19,8 +19,6 @@
 #define ANADIG_REG_2P5		0x130
 #define ANADIG_REG_CORE		0x140
 #define ANADIG_ANA_MISC0	0x150
-#define ANADIG_USB1_CHRG_DETECT	0x1b0
-#define ANADIG_USB2_CHRG_DETECT	0x210
 #define ANADIG_DIGPROG		0x260
 #define ANADIG_DIGPROG_IMX6SL	0x280
 #define ANADIG_DIGPROG_IMX7D	0x800
@@ -33,8 +31,6 @@
 #define BM_ANADIG_ANA_MISC0_STOP_MODE_CONFIG	0x1000
 /* Below MISC0_DISCON_HIGH_SNVS is only for i.MX6SL */
 #define BM_ANADIG_ANA_MISC0_DISCON_HIGH_SNVS	0x2000
-#define BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B	0x80000
-#define BM_ANADIG_USB_CHRG_DETECT_EN_B		0x100000
 
 static struct regmap *anatop;
 
@@ -96,16 +92,6 @@ void imx_anatop_post_resume(void)
 
 }
 
-static void imx_anatop_usb_chrg_detect_disable(void)
-{
-	regmap_write(anatop, ANADIG_USB1_CHRG_DETECT,
-		BM_ANADIG_USB_CHRG_DETECT_EN_B
-		| BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B);
-	regmap_write(anatop, ANADIG_USB2_CHRG_DETECT,
-		BM_ANADIG_USB_CHRG_DETECT_EN_B |
-		BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B);
-}
-
 void __init imx_init_revision_from_anatop(void)
 {
 	struct device_node *np;
@@ -167,14 +153,3 @@ void __init imx_init_revision_from_anatop(void)
 	mxc_set_cpu_type(digprog >> 16 & 0xff);
 	imx_set_soc_revision(revision);
 }
-
-void __init imx_anatop_init(void)
-{
-	anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
-	if (IS_ERR(anatop)) {
-		pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
-		return;
-	}
-
-	imx_anatop_usb_chrg_detect_disable();
-}
diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
index 912aeceb4ff8..debeda48fb98 100644
--- a/arch/arm/mach-imx/common.h
+++ b/arch/arm/mach-imx/common.h
@@ -96,7 +96,6 @@ void imx_gpc_mask_all(void);
 void imx_gpc_restore_all(void);
 void imx_gpc_hwirq_mask(unsigned int hwirq);
 void imx_gpc_hwirq_unmask(unsigned int hwirq);
-void imx_anatop_init(void);
 void imx_anatop_pre_suspend(void);
 void imx_anatop_post_resume(void);
 int imx6_set_lpm(enum mxc_cpu_pwr_mode mode);
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index edd26e0ffeec..b4c2b99192c5 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -274,7 +274,6 @@ static void __init imx6q_init_machine(void)
 
 	of_platform_default_populate(NULL, NULL, parent);
 
-	imx_anatop_init();
 	cpu_is_imx6q() ?  imx6q_pm_init() : imx6dl_pm_init();
 	imx6q_1588_init();
 	imx6q_axi_init();
diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c
index e00818abe54d..1f3092be03fd 100644
--- a/arch/arm/mach-imx/mach-imx6sl.c
+++ b/arch/arm/mach-imx/mach-imx6sl.c
@@ -56,7 +56,6 @@ static void __init imx6sl_init_machine(void)
 
 	if (cpu_is_imx6sl())
 		imx6sl_fec_init();
-	imx_anatop_init();
 	imx6sl_pm_init();
 }
 
diff --git a/arch/arm/mach-imx/mach-imx6sx.c b/arch/arm/mach-imx/mach-imx6sx.c
index d5310bf307ff..0f93c2e023c3 100644
--- a/arch/arm/mach-imx/mach-imx6sx.c
+++ b/arch/arm/mach-imx/mach-imx6sx.c
@@ -72,7 +72,6 @@ static void __init imx6sx_init_machine(void)
 	of_platform_default_populate(NULL, NULL, parent);
 
 	imx6sx_enet_init();
-	imx_anatop_init();
 	imx6sx_pm_init();
 }
 
diff --git a/arch/arm/mach-imx/mach-imx6ul.c b/arch/arm/mach-imx/mach-imx6ul.c
index 311f5e4ff723..d063e3b6e5da 100644
--- a/arch/arm/mach-imx/mach-imx6ul.c
+++ b/arch/arm/mach-imx/mach-imx6ul.c
@@ -64,7 +64,6 @@ static void __init imx6ul_init_machine(void)
 
 	of_platform_default_populate(NULL, NULL, parent);
 	imx6ul_enet_init();
-	imx_anatop_init();
 	imx6ul_pm_init();
 }
 
diff --git a/arch/arm/mach-imx/mach-imx7d.c b/arch/arm/mach-imx/mach-imx7d.c
index 95713450591a..dede6004bfc8 100644
--- a/arch/arm/mach-imx/mach-imx7d.c
+++ b/arch/arm/mach-imx/mach-imx7d.c
@@ -90,7 +90,6 @@ static void __init imx7d_init_machine(void)
 	if (parent == NULL)
 		pr_warn("failed to initialize soc device\n");
 
-	imx_anatop_init();
 	imx7d_enet_init();
 }
 
-- 
2.21.0

Re: [PATCH] ARM: imx: Drop imx_anatop_init()

[Fabio Estevam]

Hi Andrey,

On Wed, Jul 31, 2019 at 3:01 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
> detect in mxs_phy_hw_init()") in tree all of the necessary charger
> setup is done by the USB PHY driver which covers all of the affected
> i.MX6 SoCs.
>
> NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
> datasheet it appears to have a different USB PHY IP block, so
> executing i.MX6 charger disable configuration seems unnecessary.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Peter Chen <peter.chen@nxp.com>
> Cc: linux-imx@nxp.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH] ARM: imx: Drop imx_anatop_init()

[Shawn Guo]

On Wed, Jul 31, 2019 at 11:01:31AM -0700, Andrey Smirnov wrote:
> With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
> detect in mxs_phy_hw_init()") in tree all of the necessary charger
> setup is done by the USB PHY driver which covers all of the affected
> i.MX6 SoCs.
> 
> NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
> datasheet it appears to have a different USB PHY IP block, so
> executing i.MX6 charger disable configuration seems unnecessary.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Peter Chen <peter.chen@nxp.com>
> Cc: linux-imx@nxp.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org

Applied, thanks.

Re: [PATCH] ARM: imx: Drop imx_anatop_init()

[Leonard Crestez]

On 31.07.2019 21:01, Andrey Smirnov wrote:
> With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
> detect in mxs_phy_hw_init()") in tree all of the necessary charger
> setup is done by the USB PHY driver which covers all of the affected
> i.MX6 SoCs.
> 
> NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
> datasheet it appears to have a different USB PHY IP block, so
> executing i.MX6 charger disable configuration seems unnecessary.
> 
> -void __init imx_anatop_init(void)
> -{
> -	anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
> -	if (IS_ERR(anatop)) {
> -		pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
> -		return;
> -	}

This patch breaks suspend on imx6 in linux-next because the "anatop" 
regmap is no longer initialized. This was found via bisect but 
no_console_suspend prints a helpful stack anyway:

(regmap_read) from [<c01226e4>] (imx_anatop_enable_weak2p5+0x28/0x70)
(imx_anatop_enable_weak2p5) from [<c0122744>] 
(imx_anatop_pre_suspend+0x18/0x64)
(imx_anatop_pre_suspend) from [<c0124434>] (imx6q_pm_enter+0x60/0x16c)
(imx6q_pm_enter) from [<c018c8a4>] (suspend_devices_and_enter+0x7d4/0xcbc)
(suspend_devices_and_enter) from [<c018d544>] (pm_suspend+0x7b8/0x904)
(pm_suspend) from [<c018b1b4>] (state_store+0x68/0xc8)

Minimal fix looks like this:

--- arch/arm/mach-imx/anatop.c
+++ arch/arm/mach-imx/anatop.c
@@ -111,6 +111,12 @@ void __init imx_init_revision_from_anatop(void)
          digprog = readl_relaxed(anatop_base + offset);
          iounmap(anatop_base);

+       anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
+       if (IS_ERR(anatop)) {
+               pr_err("failed to find imx6q-anatop regmap!\n");
+               return;
+       }

Since all SOCs that called imx_anatop_init also call 
imx_init_revision_from_anatop this might be an acceptable solution, 
unless there is some limitation preventing early regmap lookup.

--
Regards,
Leonard

Re: [PATCH] ARM: imx: Drop imx_anatop_init()

[Andrey Smirnov]

On Thu, Aug 22, 2019 at 10:33 AM Leonard Crestez
<leonard.crestez@nxp.com> wrote:
>
> On 31.07.2019 21:01, Andrey Smirnov wrote:
> > With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
> > detect in mxs_phy_hw_init()") in tree all of the necessary charger
> > setup is done by the USB PHY driver which covers all of the affected
> > i.MX6 SoCs.
> >
> > NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
> > datasheet it appears to have a different USB PHY IP block, so
> > executing i.MX6 charger disable configuration seems unnecessary.
> >
> > -void __init imx_anatop_init(void)
> > -{
> > -     anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
> > -     if (IS_ERR(anatop)) {
> > -             pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
> > -             return;
> > -     }
>
> This patch breaks suspend on imx6 in linux-next because the "anatop"
> regmap is no longer initialized. This was found via bisect but
> no_console_suspend prints a helpful stack anyway:
>
> (regmap_read) from [<c01226e4>] (imx_anatop_enable_weak2p5+0x28/0x70)
> (imx_anatop_enable_weak2p5) from [<c0122744>]
> (imx_anatop_pre_suspend+0x18/0x64)
> (imx_anatop_pre_suspend) from [<c0124434>] (imx6q_pm_enter+0x60/0x16c)
> (imx6q_pm_enter) from [<c018c8a4>] (suspend_devices_and_enter+0x7d4/0xcbc)
> (suspend_devices_and_enter) from [<c018d544>] (pm_suspend+0x7b8/0x904)
> (pm_suspend) from [<c018b1b4>] (state_store+0x68/0xc8)
>

My bad, completely missed that fact that anatop was a global variable
in  imx_anatop_init(). Sorry about that.

> Minimal fix looks like this:
>
> --- arch/arm/mach-imx/anatop.c
> +++ arch/arm/mach-imx/anatop.c
> @@ -111,6 +111,12 @@ void __init imx_init_revision_from_anatop(void)
>           digprog = readl_relaxed(anatop_base + offset);
>           iounmap(anatop_base);
>
> +       anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
> +       if (IS_ERR(anatop)) {
> +               pr_err("failed to find imx6q-anatop regmap!\n");
> +               return;
> +       }
>
> Since all SOCs that called imx_anatop_init also call
> imx_init_revision_from_anatop this might be an acceptable solution,
> unless there is some limitation preventing early regmap lookup.
>

Would making every function that uses anatop explicitly request it via
syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop") be too much of
a code duplication? This way we won't need to worry if
imx_init_revision_from_anatop() was called before any of them are
used.

Thanks,
Andrey Smirnov

Re: [PATCH] ARM: imx: Drop imx_anatop_init()

[Leonard Crestez]

On 22.08.2019 23:06, Andrey Smirnov wrote:
>> On 31.07.2019 21:01, Andrey Smirnov wrote:
>>> With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
>>> detect in mxs_phy_hw_init()") in tree all of the necessary charger
>>> setup is done by the USB PHY driver which covers all of the affected
>>> i.MX6 SoCs.
>>>
>>> NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
>>> datasheet it appears to have a different USB PHY IP block, so
>>> executing i.MX6 charger disable configuration seems unnecessary.
>>>
>>> -void __init imx_anatop_init(void)
>>> -{
>>> -     anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
>>> -     if (IS_ERR(anatop)) {
>>> -             pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
>>> -             return;
>>> -     }
>>
>> This patch breaks suspend on imx6 in linux-next because the "anatop"
>> regmap is no longer initialized. This was found via bisect but
>> no_console_suspend prints a helpful stack anyway:
>>
>> (regmap_read) from [<c01226e4>] (imx_anatop_enable_weak2p5+0x28/0x70)
>> (imx_anatop_enable_weak2p5) from [<c0122744>]
>> (imx_anatop_pre_suspend+0x18/0x64)
>> (imx_anatop_pre_suspend) from [<c0124434>] (imx6q_pm_enter+0x60/0x16c)
>> (imx6q_pm_enter) from [<c018c8a4>] (suspend_devices_and_enter+0x7d4/0xcbc)
>> (suspend_devices_and_enter) from [<c018d544>] (pm_suspend+0x7b8/0x904)
>> (pm_suspend) from [<c018b1b4>] (state_store+0x68/0xc8)
>>
> 
> My bad, completely missed that fact that anatop was a global variable
> in  imx_anatop_init(). Sorry about that.
> 
>> Minimal fix looks like this:
>>
>> --- arch/arm/mach-imx/anatop.c
>> +++ arch/arm/mach-imx/anatop.c
>> @@ -111,6 +111,12 @@ void __init imx_init_revision_from_anatop(void)
>>            digprog = readl_relaxed(anatop_base + offset);
>>            iounmap(anatop_base);
>>
>> +       anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
>> +       if (IS_ERR(anatop)) {
>> +               pr_err("failed to find imx6q-anatop regmap!\n");
>> +               return;
>> +       }
>>
>> Since all SOCs that called imx_anatop_init also call
>> imx_init_revision_from_anatop this might be an acceptable solution,
>> unless there is some limitation preventing early regmap lookup.
>>
> 
> Would making every function that uses anatop explicitly request it via
> syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop") be too much of
> a code duplication? This way we won't need to worry if
> imx_init_revision_from_anatop() was called before any of them are
> used.

It's only used from pre_suspend and post_suspend, everything else in 
arch/arm/mach-imx/anatop.c is static. Doing a lookup every time would be 
silly, it's fine to let this be global.

I was wondering if maybe imx_init_revision could somehow end up getting 
called before syscon/regmap core init.

--
Regards,
Leonard

Re: [PATCH] ARM: imx: Drop imx_anatop_init()

[Shawn Guo]

On Thu, Aug 22, 2019 at 05:33:13PM +0000, Leonard Crestez wrote:
> On 31.07.2019 21:01, Andrey Smirnov wrote:
> > With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
> > detect in mxs_phy_hw_init()") in tree all of the necessary charger
> > setup is done by the USB PHY driver which covers all of the affected
> > i.MX6 SoCs.
> > 
> > NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
> > datasheet it appears to have a different USB PHY IP block, so
> > executing i.MX6 charger disable configuration seems unnecessary.
> > 
> > -void __init imx_anatop_init(void)
> > -{
> > -	anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
> > -	if (IS_ERR(anatop)) {
> > -		pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
> > -		return;
> > -	}
> 
> This patch breaks suspend on imx6 in linux-next because the "anatop" 
> regmap is no longer initialized. This was found via bisect but 
> no_console_suspend prints a helpful stack anyway:
> 
> (regmap_read) from [<c01226e4>] (imx_anatop_enable_weak2p5+0x28/0x70)
> (imx_anatop_enable_weak2p5) from [<c0122744>] 
> (imx_anatop_pre_suspend+0x18/0x64)
> (imx_anatop_pre_suspend) from [<c0124434>] (imx6q_pm_enter+0x60/0x16c)
> (imx6q_pm_enter) from [<c018c8a4>] (suspend_devices_and_enter+0x7d4/0xcbc)
> (suspend_devices_and_enter) from [<c018d544>] (pm_suspend+0x7b8/0x904)
> (pm_suspend) from [<c018b1b4>] (state_store+0x68/0xc8)

I dropped it from my branch for now.  Thanks for reporting!

Shawn

Re: [PATCH] ARM: imx: Drop imx_anatop_init()

[Andrey Smirnov]

On Sat, Aug 24, 2019 at 11:31 AM Shawn Guo <shawnguo@kernel.org> wrote:
>
> On Thu, Aug 22, 2019 at 05:33:13PM +0000, Leonard Crestez wrote:
> > On 31.07.2019 21:01, Andrey Smirnov wrote:
> > > With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
> > > detect in mxs_phy_hw_init()") in tree all of the necessary charger
> > > setup is done by the USB PHY driver which covers all of the affected
> > > i.MX6 SoCs.
> > >
> > > NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
> > > datasheet it appears to have a different USB PHY IP block, so
> > > executing i.MX6 charger disable configuration seems unnecessary.
> > >
> > > -void __init imx_anatop_init(void)
> > > -{
> > > -   anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
> > > -   if (IS_ERR(anatop)) {
> > > -           pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
> > > -           return;
> > > -   }
> >
> > This patch breaks suspend on imx6 in linux-next because the "anatop"
> > regmap is no longer initialized. This was found via bisect but
> > no_console_suspend prints a helpful stack anyway:
> >
> > (regmap_read) from [<c01226e4>] (imx_anatop_enable_weak2p5+0x28/0x70)
> > (imx_anatop_enable_weak2p5) from [<c0122744>]
> > (imx_anatop_pre_suspend+0x18/0x64)
> > (imx_anatop_pre_suspend) from [<c0124434>] (imx6q_pm_enter+0x60/0x16c)
> > (imx6q_pm_enter) from [<c018c8a4>] (suspend_devices_and_enter+0x7d4/0xcbc)
> > (suspend_devices_and_enter) from [<c018d544>] (pm_suspend+0x7b8/0x904)
> > (pm_suspend) from [<c018b1b4>] (state_store+0x68/0xc8)
>
> I dropped it from my branch for now.  Thanks for reporting!
>

OK, it sounds like I can submit a v2 that only removes
imx_anatop_usb_chrg_detect_disable() and keeps imx_anatop_init()
intact.

Thanks,
Andrey Smirnov