[PATCH] pinctrl: mediatek: convert to arch_initcall

[PATCH] pinctrl: mediatek: convert to arch_initcall

[Daniel Kurtz]

Move pinctrl initialization earlier in boot so that real devices can find
their pctldev without probe deferring.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/pinctrl/mediatek/pinctrl-mt6397.c | 2 +-
 drivers/pinctrl/mediatek/pinctrl-mt8127.c | 2 +-
 drivers/pinctrl/mediatek/pinctrl-mt8135.c | 2 +-
 drivers/pinctrl/mediatek/pinctrl-mt8173.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6397.c b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
index f9751ae..a3780d4 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt6397.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
@@ -70,7 +70,7 @@ static int __init mtk_pinctrl_init(void)
 	return platform_driver_register(&mtk_pinctrl_driver);
 }
 
-module_init(mtk_pinctrl_init);
+arch_initcall(mtk_pinctrl_init);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("MediaTek MT6397 Pinctrl Driver");
diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8127.c b/drivers/pinctrl/mediatek/pinctrl-mt8127.c
index b317b0b..98e0beb 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8127.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8127.c
@@ -351,7 +351,7 @@ static int __init mtk_pinctrl_init(void)
 	return platform_driver_register(&mtk_pinctrl_driver);
 }
 
-module_init(mtk_pinctrl_init);
+arch_initcall(mtk_pinctrl_init);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("MediaTek MT8127 Pinctrl Driver");
diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8135.c b/drivers/pinctrl/mediatek/pinctrl-mt8135.c
index 404f117..1c153b8 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8135.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8135.c
@@ -366,7 +366,7 @@ static int __init mtk_pinctrl_init(void)
 	return platform_driver_register(&mtk_pinctrl_driver);
 }
 
-module_init(mtk_pinctrl_init);
+arch_initcall(mtk_pinctrl_init);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("MediaTek Pinctrl Driver");
diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
index ad27184..a62514e 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
@@ -394,7 +394,7 @@ static int __init mtk_pinctrl_init(void)
 	return platform_driver_register(&mtk_pinctrl_driver);
 }
 
-module_init(mtk_pinctrl_init);
+arch_initcall(mtk_pinctrl_init);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("MediaTek Pinctrl Driver");
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Yingjoe Chen]

On Fri, 2015-12-18 at 12:21 +0800, Daniel Kurtz wrote:
> Move pinctrl initialization earlier in boot so that real devices can find
> their pctldev without probe deferring.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/pinctrl/mediatek/pinctrl-mt6397.c | 2 +-
>  drivers/pinctrl/mediatek/pinctrl-mt8127.c | 2 +-
>  drivers/pinctrl/mediatek/pinctrl-mt8135.c | 2 +-
>  drivers/pinctrl/mediatek/pinctrl-mt8173.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6397.c b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> index f9751ae..a3780d4 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> @@ -70,7 +70,7 @@ static int __init mtk_pinctrl_init(void)
>  	return platform_driver_register(&mtk_pinctrl_driver);
>  }
>  
> -module_init(mtk_pinctrl_init);
> +arch_initcall(mtk_pinctrl_init);


MT6397 is PMIC, which depends on pwrap on main AP to work. Since
pmic-wrap itself is module_platform_driver, I think it make sense to
keep this one as module_init. Maybe adding a comment to explain why it
is different from others will help.

Joe.C

>  
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("MediaTek MT6397 Pinctrl Driver");
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8127.c b/drivers/pinctrl/mediatek/pinctrl-mt8127.c
> index b317b0b..98e0beb 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8127.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8127.c
> @@ -351,7 +351,7 @@ static int __init mtk_pinctrl_init(void)
>  	return platform_driver_register(&mtk_pinctrl_driver);
>  }
>  
> -module_init(mtk_pinctrl_init);
> +arch_initcall(mtk_pinctrl_init);
>  
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("MediaTek MT8127 Pinctrl Driver");
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8135.c b/drivers/pinctrl/mediatek/pinctrl-mt8135.c
> index 404f117..1c153b8 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8135.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8135.c
> @@ -366,7 +366,7 @@ static int __init mtk_pinctrl_init(void)
>  	return platform_driver_register(&mtk_pinctrl_driver);
>  }
>  
> -module_init(mtk_pinctrl_init);
> +arch_initcall(mtk_pinctrl_init);
>  
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("MediaTek Pinctrl Driver");
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> index ad27184..a62514e 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
> @@ -394,7 +394,7 @@ static int __init mtk_pinctrl_init(void)
>  	return platform_driver_register(&mtk_pinctrl_driver);
>  }
>  
> -module_init(mtk_pinctrl_init);
> +arch_initcall(mtk_pinctrl_init);
>  
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("MediaTek Pinctrl Driver");

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Daniel Kurtz]

On Fri, Dec 18, 2015 at 11:06 PM, Yingjoe Chen
<yingjoe.chen@mediatek.com> wrote:
> On Fri, 2015-12-18 at 12:21 +0800, Daniel Kurtz wrote:
>> Move pinctrl initialization earlier in boot so that real devices can find
>> their pctldev without probe deferring.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/pinctrl/mediatek/pinctrl-mt6397.c | 2 +-
>>  drivers/pinctrl/mediatek/pinctrl-mt8127.c | 2 +-
>>  drivers/pinctrl/mediatek/pinctrl-mt8135.c | 2 +-
>>  drivers/pinctrl/mediatek/pinctrl-mt8173.c | 2 +-
>>  4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6397.c b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
>> index f9751ae..a3780d4 100644
>> --- a/drivers/pinctrl/mediatek/pinctrl-mt6397.c
>> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
>> @@ -70,7 +70,7 @@ static int __init mtk_pinctrl_init(void)
>>       return platform_driver_register(&mtk_pinctrl_driver);
>>  }
>>
>> -module_init(mtk_pinctrl_init);
>> +arch_initcall(mtk_pinctrl_init);
>
>
> MT6397 is PMIC, which depends on pwrap on main AP to work. Since
> pmic-wrap itself is module_platform_driver, I think it make sense to
> keep this one as module_init. Maybe adding a comment to explain why it
> is different from others will help.

I interpret this the other way - I think that since the PMIC wrapper
provides a bus required for the system PMIC it should also be a
builtin and use arch_initcall.

WDYT?

-Dan

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Yingjoe Chen]

On Mon, 2015-12-21 at 14:51 +0800, Daniel Kurtz wrote:
> On Fri, Dec 18, 2015 at 11:06 PM, Yingjoe Chen
> <yingjoe.chen@mediatek.com> wrote:
> > On Fri, 2015-12-18 at 12:21 +0800, Daniel Kurtz wrote:
> >> Move pinctrl initialization earlier in boot so that real devices can find
> >> their pctldev without probe deferring.
> >>
> >> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> >> ---
> >>  drivers/pinctrl/mediatek/pinctrl-mt6397.c | 2 +-
> >>  drivers/pinctrl/mediatek/pinctrl-mt8127.c | 2 +-
> >>  drivers/pinctrl/mediatek/pinctrl-mt8135.c | 2 +-
> >>  drivers/pinctrl/mediatek/pinctrl-mt8173.c | 2 +-
> >>  4 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6397.c b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> >> index f9751ae..a3780d4 100644
> >> --- a/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> >> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> >> @@ -70,7 +70,7 @@ static int __init mtk_pinctrl_init(void)
> >>       return platform_driver_register(&mtk_pinctrl_driver);
> >>  }
> >>
> >> -module_init(mtk_pinctrl_init);
> >> +arch_initcall(mtk_pinctrl_init);
> >
> >
> > MT6397 is PMIC, which depends on pwrap on main AP to work. Since
> > pmic-wrap itself is module_platform_driver, I think it make sense to
> > keep this one as module_init. Maybe adding a comment to explain why it
> > is different from others will help.
> 
> I interpret this the other way - I think that since the PMIC wrapper
> provides a bus required for the system PMIC it should also be a
> builtin and use arch_initcall.

We'll have to change mt8173 PMIC wrapper and mt6397 MFD core to
arch_initcall if we want to do it.

I think regulators on PMIC is more important than pinctrl. For all
mt8173 systems, few drivers depends on PMIC pinctrl to work but many
depends on the regulators. So if we adjust pinctrl to arch_initcall, we
should change mt6397 regulator as well.

Joe.C

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Daniel Kurtz]

On Mon, Dec 21, 2015 at 8:39 PM, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
>
> On Mon, 2015-12-21 at 14:51 +0800, Daniel Kurtz wrote:
> > On Fri, Dec 18, 2015 at 11:06 PM, Yingjoe Chen
> > <yingjoe.chen@mediatek.com> wrote:
> > > On Fri, 2015-12-18 at 12:21 +0800, Daniel Kurtz wrote:
> > >> Move pinctrl initialization earlier in boot so that real devices can find
> > >> their pctldev without probe deferring.
> > >>
> > >> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > >> ---
> > >>  drivers/pinctrl/mediatek/pinctrl-mt6397.c | 2 +-
> > >>  drivers/pinctrl/mediatek/pinctrl-mt8127.c | 2 +-
> > >>  drivers/pinctrl/mediatek/pinctrl-mt8135.c | 2 +-
> > >>  drivers/pinctrl/mediatek/pinctrl-mt8173.c | 2 +-
> > >>  4 files changed, 4 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6397.c b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> > >> index f9751ae..a3780d4 100644
> > >> --- a/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> > >> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6397.c
> > >> @@ -70,7 +70,7 @@ static int __init mtk_pinctrl_init(void)
> > >>       return platform_driver_register(&mtk_pinctrl_driver);
> > >>  }
> > >>
> > >> -module_init(mtk_pinctrl_init);
> > >> +arch_initcall(mtk_pinctrl_init);
> > >
> > >
> > > MT6397 is PMIC, which depends on pwrap on main AP to work. Since
> > > pmic-wrap itself is module_platform_driver, I think it make sense to
> > > keep this one as module_init. Maybe adding a comment to explain why it
> > > is different from others will help.
> >
> > I interpret this the other way - I think that since the PMIC wrapper
> > provides a bus required for the system PMIC it should also be a
> > builtin and use arch_initcall.
>
> We'll have to change mt8173 PMIC wrapper and mt6397 MFD core to
> arch_initcall if we want to do it.
>
> I think regulators on PMIC is more important than pinctrl. For all
> mt8173 systems, few drivers depends on PMIC pinctrl to work but many
> depends on the regulators. So if we adjust pinctrl to arch_initcall, we
> should change mt6397 regulator as well.

ACK to changing all of mt6397 to arch_initcall, especially the regulators.

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Linus Walleij]

On Fri, Dec 18, 2015 at 5:21 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:

> Move pinctrl initialization earlier in boot so that real devices can find
> their pctldev without probe deferring.
>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

I interpret the discussion as I should wait for a new version of this?

Or should it be applied?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Yingjoe Chen]

On Tue, 2015-12-22 at 11:23 +0100, Linus Walleij wrote:
> On Fri, Dec 18, 2015 at 5:21 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 
> > Move pinctrl initialization earlier in boot so that real devices can find
> > their pctldev without probe deferring.
> >
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> I interpret the discussion as I should wait for a new version of this?
> 
> Or should it be applied?

We'll have another patch to change init orders for pwrap, mt6397 mfd
core and mt6397 regulator. Before that, the mt6397 pinctrl changes in
this patch won't work as expect. Apply this now won't cause any problem
either, unless someone think we should stink to current orders.

So I think we could apply this now:

Acked-by: Yingjoe Chen <yingjoe.chen@mediatek.com>

Joe.C

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Daniel Kurtz]

On Tue, Dec 22, 2015 at 9:36 PM, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> On Tue, 2015-12-22 at 11:23 +0100, Linus Walleij wrote:
>> On Fri, Dec 18, 2015 at 5:21 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>
>> > Move pinctrl initialization earlier in boot so that real devices can find
>> > their pctldev without probe deferring.
>> >
>> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>>
>> I interpret the discussion as I should wait for a new version of this?
>>
>> Or should it be applied?
>
> We'll have another patch to change init orders for pwrap, mt6397 mfd
> core and mt6397 regulator. Before that, the mt6397 pinctrl changes in
> this patch won't work as expect. Apply this now won't cause any problem
> either, unless someone think we should stink to current orders.
>
> So I think we could apply this now:
>
> Acked-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
>
> Joe.C

Joe.C, Linus,

Thanks for the Ack.
I think it is cleaner if I just send a new patch without the mt6397 change.
So, I'll do that in a bit.

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3561 bytes --]

On Fri, Jan 01, 2016 at 09:56:01AM +0800, Daniel Kurtz wrote:
> On Fri, Jan 1, 2016 at 6:07 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Dec 31, 2015 at 09:45:51PM +0800, Daniel Kurtz wrote:

> >> Things are actively broken right now, in the sense that there are many
> >> needless probe deferrals on boot.

> > That's just noisy, everything does end up loading OK.

> It's not just noisy, it also adds to boot time.

If you've got meaningful boot time numbers from practical systems it'd
be good to share them, nobody was able to show any actual problem there.

> >> These are pinctrl drivers, which are required to load before every
> >> other driver that requests a gpio.
> >> AFAICT, the pinctrl is part of the platform "architecture", hence why
> >> I suggest we move this to arch_initcall().

> > This is exactly the sort of hacking that leads to problems

> What problems?
> More patches as people adjust / tune / optimize boot time, or something else?

There's that and also general fragility when multiple drivers interact -
it can get really problematic when different systems end up with
different requirements for the sasme driver and disrupt each other or
conflict with each other.

> > you can
> > also make the same argument for a bunch of other things like regulators
> > but then you find there's circular dependencies or extra devices with
> > different requirements on some systems that cause further issues and
> > need more special casing, or you find that some other device gets pushed
> > earlier so you have to go round tweaking everything it uses.

> For regulators, I think things are a bit more subtle.  Most regulators
> are external components that can be used on different boards for
> different purposes, so I can see an argument for treating them in a
> more generic way by using a later device_initcall.  The exception
> being architecture specific PMICs that only make sense when paired
> with a specific small set of CPUs - and if you look, there are many
> PMIC regulator drivers that are at earlier initcall levels, presumably
> because they are required by cpufreq drivers, and/or their initcall
> level is set as the same as the rest of the functions exposed by the
> same PMIC MFD.

The reason the regulator drivers are earlier is that cpufreq and a
couple of other things are broken and don't use the device model so we
have to use link order hacks.  I'm pushing any driver that doesn't
specifically have problems with those to just use normal initcall.

> >> arch_initcall() is also consistent with 39 other pinctrl drivers in
> >> drivers/pinctrl.
> >> 19 others use subsys_initcall(), core_initcall() or
> >> postcore_initcall(), any of which would also work.

> > It's fairly clear that there's at least a case for simplifying the
> > existing practice here, for example by moving everything into a single
> > (perhaps aliased) initcall rather than by randomly picking a level per
> > system or by actually fiddling with the link ordering if the case is
> > sufficiently clear that pinctrl in general ought to load earlier than it
> > does.

> Nothing above sounds like a reason not to merge this patch, however.
> Why should we block useful patches that use existing tools to fix real
> architecture-specific issues until new infrastructure is merged that
> solves general problems?

Some of the infrastructure here is really trivial if we are saying that
basically all drivers do this.  That way you don't end up so much with
things like wondering why different drivers made different decisions so
much.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On Fri, Jan 01, 2016 at 06:29:15PM +0100, Linus Walleij wrote:
> On Fri, Jan 1, 2016 at 3:27 PM, Matthias Brugger <matthias.bgg@gmail.com> wrote:

> > I think what Mark means is, that we define some pinctrl_initcall which
> > is a macro to subsys_initcall (or arch_initcall or similar). We apply this
> > to all pinctrl drivers including the one from Mediatek. This way at least
> > we have a common method and changing the behaviour in the future is
> > easier to handle.

> That would be pinctrl_soc_initcall() in that case. Just pinctrl_initcall()
> would assume it's for all drivers and there is a bunch of them that are just
> fine with simple device_initcall()s.

OTOH it might be easier to just have an initcall (or fiddle with the
link order) for everything to avoid having to think about which drivers
things apply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Linus Walleij]

On Fri, Jan 1, 2016 at 3:27 PM, Matthias Brugger <matthias.bgg@gmail.com> wrote:
> On January 1, 2016 3:56:01 AM EET, Daniel Kurtz <djkurtz@chromium.org> wrote:

>>> It's fairly clear that there's at least a case for simplifying the
>>> existing practice here, for example by moving everything into a
>>single
>>> (perhaps aliased) initcall rather than by randomly picking a level
>>per
>>> system or by actually fiddling with the link ordering if the case is
>>> sufficiently clear that pinctrl in general ought to load earlier than
>>it
>>> does.
>>
>>Nothing above sounds like a reason not to merge this patch, however.
>>Why should we block useful patches that use existing tools to fix real
>>architecture-specific issues until new infrastructure is merged that
>>solves general problems?
>
> I think what Mark means is, that we define some pinctrl_initcall which
> is a macro to subsys_initcall (or arch_initcall or similar). We apply this
> to all pinctrl drivers including the one from Mediatek. This way at least
> we have a common method and changing the behaviour in the future is
> easier to handle.

That would be pinctrl_soc_initcall() in that case. Just pinctrl_initcall()
would assume it's for all drivers and there is a bunch of them that are just
fine with simple device_initcall()s.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Matthias Brugger]

On January 1, 2016 3:56:01 AM EET, Daniel Kurtz <djkurtz@chromium.org> wrote:
>Hi Mark,
>
>Thanks for responding.
>
>On Fri, Jan 1, 2016 at 6:07 AM, Mark Brown <broonie@kernel.org> wrote:
>> On Thu, Dec 31, 2015 at 09:45:51PM +0800, Daniel Kurtz wrote:
>>> On Thu, Dec 31, 2015 at 1:22 AM, Mark Brown <broonie@kernel.org>
>wrote:
>>
>>> > I really don't think we should be applying this sort of stuff
>unless
>>> > things are actively broken right now.  It's a bit of a rabbit hole
>we
>>> > could spend a long time going down tweaking things for different
>>> > systems in the same way that tweaking the link order can be and it
>masks
>>> > the underlying issues.
>>
>>> Things are actively broken right now, in the sense that there are
>many
>>> needless probe deferrals on boot.
>>
>> That's just noisy, everything does end up loading OK.
>
>It's not just noisy, it also adds to boot time.
>
>>  If the noise is a
>> problem working on fixing the underlying problem with not being able
>to
>> figure out dependencies seems like a better thing.  When we discussed
>> this on the kernel summit list it wasn't clear everyone was convinced
>> this was even a problem (I think it is since it obscures real
>> information).  Actual breakage to me is something that never sorts
>> itself out.
>>
>>> These are pinctrl drivers, which are required to load before every
>>> other driver that requests a gpio.
>>> AFAICT, the pinctrl is part of the platform "architecture", hence
>why
>>> I suggest we move this to arch_initcall().
>>
>> This is exactly the sort of hacking that leads to problems
>
>What problems?
>More patches as people adjust / tune / optimize boot time, or something
>else?
>
>> you can
>> also make the same argument for a bunch of other things like
>regulators
>> but then you find there's circular dependencies or extra devices with
>> different requirements on some systems that cause further issues and
>> need more special casing, or you find that some other device gets
>pushed
>> earlier so you have to go round tweaking everything it uses.
>
>For regulators, I think things are a bit more subtle.  Most regulators
>are external components that can be used on different boards for
>different purposes, so I can see an argument for treating them in a
>more generic way by using a later device_initcall.  The exception
>being architecture specific PMICs that only make sense when paired
>with a specific small set of CPUs - and if you look, there are many
>PMIC regulator drivers that are at earlier initcall levels, presumably
>because they are required by cpufreq drivers, and/or their initcall
>level is set as the same as the rest of the functions exposed by the
>same PMIC MFD.
>
>> It's not
>> that the device is magic, it's that we don't understand how to handle
>> dependencies well enough.  Raphael did say he was going to work on
>> something for this, I'm not sure where it got to though.
>
>Glad to hear it is a well recognized problem, and that people plan to
>look into a fix.
>
>>> arch_initcall() is also consistent with 39 other pinctrl drivers in
>>> drivers/pinctrl.
>>> 19 others use subsys_initcall(), core_initcall() or
>>> postcore_initcall(), any of which would also work.
>>
>> It's fairly clear that there's at least a case for simplifying the
>> existing practice here, for example by moving everything into a
>single
>> (perhaps aliased) initcall rather than by randomly picking a level
>per
>> system or by actually fiddling with the link ordering if the case is
>> sufficiently clear that pinctrl in general ought to load earlier than
>it
>> does.
>
>Nothing above sounds like a reason not to merge this patch, however.
>Why should we block useful patches that use existing tools to fix real
>architecture-specific issues until new infrastructure is merged that
>solves general problems?
>

I think what Mark means is, that we define some pinctrl_initcall which is a macro to subsys_initcall (or arch_initcall or similar). We apply this to all pinctrl drivers including the one from Mediatek. This way at least we have a common method and changing the behaviour in the future is easier to handle.

Regards,
Matthias

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Daniel Kurtz]

Hi Mark,

Thanks for responding.

On Fri, Jan 1, 2016 at 6:07 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Dec 31, 2015 at 09:45:51PM +0800, Daniel Kurtz wrote:
>> On Thu, Dec 31, 2015 at 1:22 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > I really don't think we should be applying this sort of stuff unless
>> > things are actively broken right now.  It's a bit of a rabbit hole we
>> > could spend a long time going down tweaking things for different
>> > systems in the same way that tweaking the link order can be and it masks
>> > the underlying issues.
>
>> Things are actively broken right now, in the sense that there are many
>> needless probe deferrals on boot.
>
> That's just noisy, everything does end up loading OK.

It's not just noisy, it also adds to boot time.

>  If the noise is a
> problem working on fixing the underlying problem with not being able to
> figure out dependencies seems like a better thing.  When we discussed
> this on the kernel summit list it wasn't clear everyone was convinced
> this was even a problem (I think it is since it obscures real
> information).  Actual breakage to me is something that never sorts
> itself out.
>
>> These are pinctrl drivers, which are required to load before every
>> other driver that requests a gpio.
>> AFAICT, the pinctrl is part of the platform "architecture", hence why
>> I suggest we move this to arch_initcall().
>
> This is exactly the sort of hacking that leads to problems

What problems?
More patches as people adjust / tune / optimize boot time, or something else?

> you can
> also make the same argument for a bunch of other things like regulators
> but then you find there's circular dependencies or extra devices with
> different requirements on some systems that cause further issues and
> need more special casing, or you find that some other device gets pushed
> earlier so you have to go round tweaking everything it uses.

For regulators, I think things are a bit more subtle.  Most regulators
are external components that can be used on different boards for
different purposes, so I can see an argument for treating them in a
more generic way by using a later device_initcall.  The exception
being architecture specific PMICs that only make sense when paired
with a specific small set of CPUs - and if you look, there are many
PMIC regulator drivers that are at earlier initcall levels, presumably
because they are required by cpufreq drivers, and/or their initcall
level is set as the same as the rest of the functions exposed by the
same PMIC MFD.

> It's not
> that the device is magic, it's that we don't understand how to handle
> dependencies well enough.  Raphael did say he was going to work on
> something for this, I'm not sure where it got to though.

Glad to hear it is a well recognized problem, and that people plan to
look into a fix.

>> arch_initcall() is also consistent with 39 other pinctrl drivers in
>> drivers/pinctrl.
>> 19 others use subsys_initcall(), core_initcall() or
>> postcore_initcall(), any of which would also work.
>
> It's fairly clear that there's at least a case for simplifying the
> existing practice here, for example by moving everything into a single
> (perhaps aliased) initcall rather than by randomly picking a level per
> system or by actually fiddling with the link ordering if the case is
> sufficiently clear that pinctrl in general ought to load earlier than it
> does.

Nothing above sounds like a reason not to merge this patch, however.
Why should we block useful patches that use existing tools to fix real
architecture-specific issues until new infrastructure is merged that
solves general problems?

-Dan

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2392 bytes --]

On Thu, Dec 31, 2015 at 09:45:51PM +0800, Daniel Kurtz wrote:
> On Thu, Dec 31, 2015 at 1:22 AM, Mark Brown <broonie@kernel.org> wrote:

> > I really don't think we should be applying this sort of stuff unless
> > things are actively broken right now.  It's a bit of a rabbit hole we
> > could spend a long time going down tweaking things for different
> > systems in the same way that tweaking the link order can be and it masks
> > the underlying issues.

> Things are actively broken right now, in the sense that there are many
> needless probe deferrals on boot.

That's just noisy, everything does end up loading OK.  If the noise is a
problem working on fixing the underlying problem with not being able to
figure out dependencies seems like a better thing.  When we discussed
this on the kernel summit list it wasn't clear everyone was convinced
this was even a problem (I think it is since it obscures real
information).  Actual breakage to me is something that never sorts
itself out.

> These are pinctrl drivers, which are required to load before every
> other driver that requests a gpio.
> AFAICT, the pinctrl is part of the platform "architecture", hence why
> I suggest we move this to arch_initcall().

This is exactly the sort of hacking that leads to problems - you can
also make the same argument for a bunch of other things like regulators
but then you find there's circular dependencies or extra devices with
different requirements on some systems that cause further issues and
need more special casing, or you find that some other device gets pushed
earlier so you have to go round tweaking everything it uses.  It's not
that the device is magic, it's that we don't understand how to handle
dependencies well enough.  Raphael did say he was going to work on
something for this, I'm not sure where it got to though.

> arch_initcall() is also consistent with 39 other pinctrl drivers in
> drivers/pinctrl.
> 19 others use subsys_initcall(), core_initcall() or
> postcore_initcall(), any of which would also work.

It's fairly clear that there's at least a case for simplifying the
existing practice here, for example by moving everything into a single
(perhaps aliased) initcall rather than by randomly picking a level per
system or by actually fiddling with the link ordering if the case is
sufficiently clear that pinctrl in general ought to load earlier than it
does.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Daniel Kurtz]

On Thu, Dec 31, 2015 at 1:22 AM, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Dec 22, 2015 at 04:11:47PM +0100, Linus Walleij wrote:
> > On Tue, Dec 22, 2015 at 2:46 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>
> > > Move pinctrl initialization earlier in boot so that real devices can find
> > > their pctldev without probe deferring.
> > (...)
> > > -module_init(mtk_pinctrl_init);
> > > +arch_initcall(mtk_pinctrl_init);
>
> > So I see why you're doing this (because of wanting to avoid nasty boot
> > probe deferrals, right?) and I'm going to apply it, because the work
> > with probe ordering is still in the works, but I'd like some general
> > ARM people to come in with opinions.
>
> I really don't think we should be applying this sort of stuff unless
> things are actively broken right now.  It's a bit of a rabbit hole we
> could spend a long time going down tweaking things for different
> systems in the same way that tweaking the link order can be and it masks
> the underlying issues.

Things are actively broken right now, in the sense that there are many
needless probe deferrals on boot.
These are pinctrl drivers, which are required to load before every
other driver that requests a gpio.
AFAICT, the pinctrl is part of the platform "architecture", hence why
I suggest we move this to arch_initcall().
arch_initcall() is also consistent with 39 other pinctrl drivers in
drivers/pinctrl.
19 others use subsys_initcall(), core_initcall() or
postcore_initcall(), any of which would also work.

-Dan

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On Tue, Dec 22, 2015 at 04:11:47PM +0100, Linus Walleij wrote:
> On Tue, Dec 22, 2015 at 2:46 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:

> > Move pinctrl initialization earlier in boot so that real devices can find
> > their pctldev without probe deferring.
> (...)
> > -module_init(mtk_pinctrl_init);
> > +arch_initcall(mtk_pinctrl_init);

> So I see why you're doing this (because of wanting to avoid nasty boot
> probe deferrals, right?) and I'm going to apply it, because the work
> with probe ordering is still in the works, but I'd like some general
> ARM people to come in with opinions.

I really don't think we should be applying this sort of stuff unless
things are actively broken right now.  It's a bit of a rabbit hole we
could spend a long time going down tweaking things for different
systems in the same way that tweaking the link order can be and it masks
the underlying issues.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

[Linus Walleij]

On Tue, Dec 22, 2015 at 2:46 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:

> Move pinctrl initialization earlier in boot so that real devices can find
> their pctldev without probe deferring.
(...)
> -module_init(mtk_pinctrl_init);
> +arch_initcall(mtk_pinctrl_init);

So I see why you're doing this (because of wanting to avoid nasty boot
probe deferrals, right?) and I'm going to apply it, because the work
with probe ordering is still in the works, but I'd like some general
ARM people to come in with opinions.

Yours,
Linus Walleij

[PATCH] pinctrl: mediatek: convert to arch_initcall

[Daniel Kurtz]

Move pinctrl initialization earlier in boot so that real devices can find
their pctldev without probe deferring.

Note: We don't change mt6397 probe order in this patch, since MT6397 is mfd
PMIC, which depends on pwrap on main AP to work. Since pmic-wrap itself
is module_platform_driver, we keep it as module_init.  A later patch
will convert both pmic-wrap, and all functions of the MT6397 mfd to
arch_initcall.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/pinctrl/mediatek/pinctrl-mt8127.c | 2 +-
 drivers/pinctrl/mediatek/pinctrl-mt8135.c | 2 +-
 drivers/pinctrl/mediatek/pinctrl-mt8173.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8127.c b/drivers/pinctrl/mediatek/pinctrl-mt8127.c
index b317b0b..98e0beb 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8127.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8127.c
@@ -351,7 +351,7 @@ static int __init mtk_pinctrl_init(void)
 	return platform_driver_register(&mtk_pinctrl_driver);
 }
 
-module_init(mtk_pinctrl_init);
+arch_initcall(mtk_pinctrl_init);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("MediaTek MT8127 Pinctrl Driver");
diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8135.c b/drivers/pinctrl/mediatek/pinctrl-mt8135.c
index 404f117..1c153b8 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8135.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8135.c
@@ -366,7 +366,7 @@ static int __init mtk_pinctrl_init(void)
 	return platform_driver_register(&mtk_pinctrl_driver);
 }
 
-module_init(mtk_pinctrl_init);
+arch_initcall(mtk_pinctrl_init);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("MediaTek Pinctrl Driver");
diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8173.c b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
index ad27184..a62514e 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt8173.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt8173.c
@@ -394,7 +394,7 @@ static int __init mtk_pinctrl_init(void)
 	return platform_driver_register(&mtk_pinctrl_driver);
 }
 
-module_init(mtk_pinctrl_init);
+arch_initcall(mtk_pinctrl_init);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("MediaTek Pinctrl Driver");
-- 
2.6.0.rc2.230.g3dd15c0