| Message ID | 20190731180131.8597-1-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: imx: Drop imx_anatop_init() | expand |
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>
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.
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
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
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
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
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
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(); }
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(-)