Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

[Tony Lindgren]

* Nishanth Menon <nm@ti.com> [160601 15:51]:
> On 06/01/2016 05:31 PM, Arnd Bergmann wrote:
> > On Wednesday, June 1, 2016 4:31:54 PM CEST Nishanth Menon wrote:
> >>
> >> Hence the "KEYSTONE_TYPICAL" option is designed similar to commit 8d9166b519fd
> >> ("omap2/3/4: Add Kconfig option to compile in typical omap features")
> >> that can be enabled for most boards keystone platforms
> >> without needing to rediscover these in defconfig all over again -
> >> examples include multi_v7_defconfig base and optimizations done on top
> >> of them for keystone platform.
> > 
> > I'd rather remove the option for OMAP as well, it doesn't really fit in with
> > how we do things for other platforms, and selecting a lot of other Kconfig
> > symbols tends to cause circular dependencies.
> 
> Hmm, fair enough -> adding Tony as well if he sees an problem with
> dropping ARCH_OMAP2PLUS_TYPICAL.

Yes there's a problem. Removing that is going to make all the custom
.config files unbootable almost certainly. Just search around on
issues related to not having REGULATOR_FIXED_VOLTAGE selected.

I agree we should not force select things though. And the I2C selection
at least is wrong, we can have PMICs that are not on I2C bus.

Anybody got better ideas what we should do to make the configuration
options more intuitive?

Regards,

Tony

Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

[Arnd Bergmann]

On Monday, June 6, 2016 9:28:54 PM CEST Tony Lindgren wrote:
> * Nishanth Menon <nm@ti.com> [160601 15:51]:
> > On 06/01/2016 05:31 PM, Arnd Bergmann wrote:
> > > On Wednesday, June 1, 2016 4:31:54 PM CEST Nishanth Menon wrote:
> > >>
> > >> Hence the "KEYSTONE_TYPICAL" option is designed similar to commit 8d9166b519fd
> > >> ("omap2/3/4: Add Kconfig option to compile in typical omap features")
> > >> that can be enabled for most boards keystone platforms
> > >> without needing to rediscover these in defconfig all over again -
> > >> examples include multi_v7_defconfig base and optimizations done on top
> > >> of them for keystone platform.
> > > 
> > > I'd rather remove the option for OMAP as well, it doesn't really fit in with
> > > how we do things for other platforms, and selecting a lot of other Kconfig
> > > symbols tends to cause circular dependencies.
> > 
> > Hmm, fair enough -> adding Tony as well if he sees an problem with
> > dropping ARCH_OMAP2PLUS_TYPICAL.
> 
> Yes there's a problem. Removing that is going to make all the custom
> .config files unbootable almost certainly. Just search around on
> issues related to not having REGULATOR_FIXED_VOLTAGE selected.
> 
> I agree we should not force select things though. And the I2C selection
> at least is wrong, we can have PMICs that are not on I2C bus.
> 
> Anybody got better ideas what we should do to make the configuration
> options more intuitive?

Let's have a look at what we actually select:

config ARCH_OMAP2PLUS_TYPICAL
        bool "Typical OMAP configuration"
        default y
        select AEABI
        select HIGHMEM
        select I2C
        select I2C_OMAP
        select MENELAUS if ARCH_OMAP2
        select NEON if CPU_V7
        select PM
        select REGULATOR
        select REGULATOR_FIXED_VOLTAGE
        select TWL4030_CORE if ARCH_OMAP3 || ARCH_OMAP4
        select TWL4030_POWER if ARCH_OMAP3 || ARCH_OMAP4
        select VFP
        help
          Compile a kernel suitable for booting most boards

A lot of these options are typical for all ARMv7 machines: AEABI, I2C, NEON,
PM, REGULATOR and VFP are things that we probably want to default to enabled
whenever we build a MULTI_V7 kernel, some also for V5 or V6, we just
need to come up with a good way to do that while (probably for most of
them) still allowing them to be disabled with CONFIG_EXPERT. I would
feel more comfortable with having an ARCH_MULTIPLATFORM_TYPICAL
Kconfig symbol than with the ARCH_OMAP2PLUS_TYPICAL one that ends up
turning things on unconditionally whenever you add ARCH_OMAP2PLUS
to some other configuration.

For I2C_OMAP, MENELAUS and TWL4030, we could just add a
'default ARCH_OMAP3 || ARCH_OMAP4' or whatever is appropriate for
each of them. I assume we want to handle TWL6040 the same way, though
we currently don't do that. MENELAUS seems to be only needed for
omap2420-n8x0.

REGULATOR_FIXED_VOLTAGE is needed by a lot of platforms that usually
don't select it, so it might be good to add a 'default y' to its Kconfig
symbol, or we could add 'select REGULATOR_FIXED_VOLTAGE if REGULATOR'
to all platforms that need it. The driver is about 1.5KB in size, compared
to about 50KB for the regulator core.

HIGHMEM is a bit odd here, because there are a couple of boards that
really really want it to access all of their memory, while most other
boards (anything with <=768MB RAM) are better off not enabling it,
so I can't see a good strategy for when to enable it or not across
multiplatform builds. If we wanted to apply crude heuristics, we could
make it default-enabled for SMP configurations, and default-disabled
for UP, or we could default-enable it for ARMv7VE (Cortex-A7/A15/A17,
Krait-400, PJ4B-MP) and default-disable for ARMv7 (Cortex-A5/A8/A9,
Scorpion, Krait, PJ4, PJ4B) and older. This would get us the right
setting in most cases, but it would also be rather confusing as it gets
flipped based on options that it doesn't really have anything to
do with in the strict sense.

	Arnd

Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

[Mark Brown]

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

On Tue, Jun 07, 2016 at 10:09:46AM +0200, Arnd Bergmann wrote:

Please start new threads for things like this - there's no obvious
relevance to me in either the subject or even the first couple of pages
of the message so it very nearly got deleted unread.  It's very common
to get copied on irrelevant stuff so things with no obvious motivation.

> REGULATOR_FIXED_VOLTAGE is needed by a lot of platforms that usually
> don't select it, so it might be good to add a 'default y' to its Kconfig
> symbol, or we could add 'select REGULATOR_FIXED_VOLTAGE if REGULATOR'
> to all platforms that need it. The driver is about 1.5KB in size, compared
> to about 50KB for the regulator core.

The platforms should do this - I wouldn't say it's *that* common,
especially not as a build time dependency with things moving to DT.

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

Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

[Tony Lindgren]

Hi,

* Arnd Bergmann <arnd@arndb.de> [160607 01:11]:
> A lot of these options are typical for all ARMv7 machines: AEABI, I2C, NEON,
> PM, REGULATOR and VFP are things that we probably want to default to enabled
> whenever we build a MULTI_V7 kernel, some also for V5 or V6, we just
> need to come up with a good way to do that while (probably for most of
> them) still allowing them to be disabled with CONFIG_EXPERT. I would
> feel more comfortable with having an ARCH_MULTIPLATFORM_TYPICAL
> Kconfig symbol than with the ARCH_OMAP2PLUS_TYPICAL one that ends up
> turning things on unconditionally whenever you add ARCH_OMAP2PLUS
> to some other configuration.

We still need to be careful about not changing the Kconfig option
name for something like this as it will cause the custom .config files
to fail unless the default is y. But yeah I agree, it would be nice
to have some multiarch sane default selects, then also do the platform
specific selects as needed. Naturally this needs to be kept down to
minimum ideally limited to silent options.

So we could have ARCH_MULTIPLATFORM_TYPICAL, then have the old
ARCH_OMAP2PLUS_TYPICAL select that one. That should keep things
behaving just fine if the old .config had ARCH_OMAP2PLUS_TYPICAL.

> For I2C_OMAP, MENELAUS and TWL4030, we could just add a
> 'default ARCH_OMAP3 || ARCH_OMAP4' or whatever is appropriate for
> each of them. I assume we want to handle TWL6040 the same way, though
> we currently don't do that. MENELAUS seems to be only needed for
> omap2420-n8x0.

We could yeah. TWL4030 is omap2430 to omap3, I don't think we currently
have any omap3 based ones not using twl4030.. But there certainly
are tons of usable devices with the Motorola CPCAP PMIC instead.

Then REGULATOR_FIXED_VOLTAGE we can have platform select like Mark
seems to prefer. HIGHMEM too should be platform specific.

The original reason for ARCH_OMAP2PLUS_TYPICAL was that if omap2plus
is selected with that you actually get a bootable system.

Regards,

Tony

Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

[Arnd Bergmann]

On Wednesday, June 8, 2016 2:47:48 AM CEST Tony Lindgren wrote:
> 
> * Arnd Bergmann <arnd@arndb.de> [160607 01:11]:
> > A lot of these options are typical for all ARMv7 machines: AEABI, I2C, NEON,
> > PM, REGULATOR and VFP are things that we probably want to default to enabled
> > whenever we build a MULTI_V7 kernel, some also for V5 or V6, we just
> > need to come up with a good way to do that while (probably for most of
> > them) still allowing them to be disabled with CONFIG_EXPERT. I would
> > feel more comfortable with having an ARCH_MULTIPLATFORM_TYPICAL
> > Kconfig symbol than with the ARCH_OMAP2PLUS_TYPICAL one that ends up
> > turning things on unconditionally whenever you add ARCH_OMAP2PLUS
> > to some other configuration.
> 
> We still need to be careful about not changing the Kconfig option
> name for something like this as it will cause the custom .config files
> to fail unless the default is y. But yeah I agree, it would be nice
> to have some multiarch sane default selects, then also do the platform
> specific selects as needed. Naturally this needs to be kept down to
> minimum ideally limited to silent options.
> 
> So we could have ARCH_MULTIPLATFORM_TYPICAL, then have the old
> ARCH_OMAP2PLUS_TYPICAL select that one. That should keep things
> behaving just fine if the old .config had ARCH_OMAP2PLUS_TYPICAL.

If we make ARCH_MULTIPLATFORM_TYPICAL 'default y', it will be always
enabled in this case, and I think we can drop ARCH_OMAP2PLUS_TYPICAL.

The downside would be any users that want the options we enable there
turned off and have them silently enabled when upgrading the kernel.

> > For I2C_OMAP, MENELAUS and TWL4030, we could just add a
> > 'default ARCH_OMAP3 || ARCH_OMAP4' or whatever is appropriate for
> > each of them. I assume we want to handle TWL6040 the same way, though
> > we currently don't do that. MENELAUS seems to be only needed for
> > omap2420-n8x0.
> 
> We could yeah. TWL4030 is omap2430 to omap3, I don't think we currently
> have any omap3 based ones not using twl4030.. But there certainly
> are tons of usable devices with the Motorola CPCAP PMIC instead.

So should we enable both then?

> Then REGULATOR_FIXED_VOLTAGE we can have platform select like Mark
> seems to prefer. 

Ok

> HIGHMEM too should be platform specific.

I still don't see a good way to handle that, we should certainly not
just 'select' it, because we often want to leave it disabled.

> The original reason for ARCH_OMAP2PLUS_TYPICAL was that if omap2plus
> is selected with that you actually get a bootable system.

I understand. It's just not very scalable to have every platform do
this, in particular if they want to select conflicting options (none
of the ones you select should conflict, but we'd get there eventually).

	Arnd

Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [160608 09:06]:
> On Wednesday, June 8, 2016 2:47:48 AM CEST Tony Lindgren wrote:
> > 
> > So we could have ARCH_MULTIPLATFORM_TYPICAL, then have the old
> > ARCH_OMAP2PLUS_TYPICAL select that one. That should keep things
> > behaving just fine if the old .config had ARCH_OMAP2PLUS_TYPICAL.
> 
> If we make ARCH_MULTIPLATFORM_TYPICAL 'default y', it will be always
> enabled in this case, and I think we can drop ARCH_OMAP2PLUS_TYPICAL.

Having "default y" there seems safe enough for me.

> The downside would be any users that want the options we enable there
> turned off and have them silently enabled when upgrading the kernel.

I think the long term solution here might be to generate the .config
out of dts files..

> > > For I2C_OMAP, MENELAUS and TWL4030, we could just add a
> > > 'default ARCH_OMAP3 || ARCH_OMAP4' or whatever is appropriate for
> > > each of them. I assume we want to handle TWL6040 the same way, though
> > > we currently don't do that. MENELAUS seems to be only needed for
> > > omap2420-n8x0.
> > 
> > We could yeah. TWL4030 is omap2430 to omap3, I don't think we currently
> > have any omap3 based ones not using twl4030.. But there certainly
> > are tons of usable devices with the Motorola CPCAP PMIC instead.
> 
> So should we enable both then?

No CPCAP support currently in mainline kernel.. And with most of
initcall level issues out of the way, any future support should
not be added to the TYPICAL because of all the issues discussed.

> > HIGHMEM too should be platform specific.
> 
> I still don't see a good way to handle that, we should certainly not
> just 'select' it, because we often want to leave it disabled.

Right. Leaving it out produces a bootable kernel though that could
produce some warning if configured memory is > 768MB.

> > The original reason for ARCH_OMAP2PLUS_TYPICAL was that if omap2plus
> > is selected with that you actually get a bootable system.
> 
> I understand. It's just not very scalable to have every platform do
> this, in particular if they want to select conflicting options (none
> of the ones you select should conflict, but we'd get there eventually).

Yeah. Seems like generating the base .confing out of the dts file
is better in the long run.

Regards,

Tony

Re: [PATCH] ARM: Keystone: Introduce Kconfig option to compile in typical Keystone features

[Mark Brown]

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

On Wed, Jun 08, 2016 at 11:50:35PM -0700, Tony Lindgren wrote:
> * Arnd Bergmann <arnd@arndb.de> [160608 09:06]:

> > The downside would be any users that want the options we enable there
> > turned off and have them silently enabled when upgrading the kernel.

> I think the long term solution here might be to generate the .config
> out of dts files..

That would be cool!

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