2019-07-31 18:02:38

by Andrey Smirnov

[permalink] [raw]
Subject: [PATCH] ARM: imx: Drop imx_anatop_init()

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 <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peter Chen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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


2019-07-31 20:42:50

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH] ARM: imx: Drop imx_anatop_init()

Hi Andrey,

On Wed, Jul 31, 2019 at 3:01 PM Andrey Smirnov <[email protected]> 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 <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peter Chen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]

Reviewed-by: Fabio Estevam <[email protected]>

2019-08-04 03:55:04

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH] ARM: imx: Drop imx_anatop_init()

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 <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peter Chen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]

Applied, thanks.

2019-08-22 17:38:23

by Leonard Crestez

[permalink] [raw]
Subject: Re: [PATCH] ARM: imx: Drop imx_anatop_init()

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

2019-08-23 08:00:44

by Andrey Smirnov

[permalink] [raw]
Subject: Re: [PATCH] ARM: imx: Drop imx_anatop_init()

On Thu, Aug 22, 2019 at 10:33 AM Leonard Crestez
<[email protected]> 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

2019-08-23 09:04:47

by Leonard Crestez

[permalink] [raw]
Subject: Re: [PATCH] ARM: imx: Drop imx_anatop_init()

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

2019-08-24 18:32:30

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH] ARM: imx: Drop imx_anatop_init()

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

2019-08-26 20:33:28

by Andrey Smirnov

[permalink] [raw]
Subject: Re: [PATCH] ARM: imx: Drop imx_anatop_init()

On Sat, Aug 24, 2019 at 11:31 AM Shawn Guo <[email protected]> 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