[PATCH 0/4] platform: allow ATOM PMC code to be optional

[PATCH 0/4] platform: allow ATOM PMC code to be optional

[Paul Gortmaker]

A few months back I was doing a test build for "defconfig-like" COTS
hardware and happened to notice some pmc-atom stuff being built.  Fine,
I thought - the defconfig isn't minimal - we all know that - what Kconfig
do I use to turn that off?  Well, imagine my surprise when I found out
you couldn't turn it [Atom Power Management Controller] code off!

Normally we all agree to not use "default y" unless unavoidable, but
somehow this was added as "def_bool y" which is even worse ; you can
see the Kconfig setting but there is *no* way you can turn it off.

After investigating, I found no reason why the atom code couldn't be
opt-out with just minor changes that the original addition overlooked.

And so this series addreses that.  I tried to be sensitive to not
break any existing configs in the process, but the defconfig will
now intentionally be different and exclude this atom specific code.

Using a defconfig on today's linux-next, here is the delta summary:

$ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove
add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659)
add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848)
add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155)

$ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total
Total: Before=13626994, After=13579335, chg -0.35%
Total: Before=3572137, After=3565289, chg -0.19%
Total: Before=1235335, After=1225180, chg -0.82%

It is hard to reclaim anything against the inevitable growth in
footprint, so I'd say we should be glad to take whatever we can.

Boot tested defconfig on today's linux-next on older non-atom COTS.

Paul.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Aubrey Li <aubrey.li@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Cc: Mark Gross <markgross@kernel.org>
Cc: platform-driver-x86@vger.kernel.org
Cc: "Rafael J. Wysocki" <rafael@kernel.org>

---

Paul Gortmaker (4):
  platform/x86: pmc_atom: remove unused pmc_atom_write()
  ACPI: LPSS: make the Kconfig dependency on PMC_ATOM explicit
  platform/x86: pmc_atom: dont export pmc_atom_read - no modular users
  platform/x86: pmc_atom: make the PMC driver actually unselectable

 arch/x86/Kconfig                           |  1 +
 drivers/platform/x86/Kconfig               | 11 ++++++++---
 drivers/platform/x86/pmc_atom.c            | 13 -------------
 include/linux/platform_data/x86/pmc_atom.h |  1 -
 4 files changed, 9 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 1/4] platform/x86: pmc_atom: remove unused pmc_atom_write()

[Paul Gortmaker]

This function isn't used anywhere in the driver or anywhere in tree.
So remove it.  It can always be re-added if/when a use arises.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Aubrey Li <aubrey.li@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mark Gross <markgross@kernel.org>
Cc: platform-driver-x86@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/pmc_atom.c            | 12 ------------
 include/linux/platform_data/x86/pmc_atom.h |  1 -
 2 files changed, 13 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index a40fae6edc84..31cf25d25d66 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -223,18 +223,6 @@ int pmc_atom_read(int offset, u32 *value)
 }
 EXPORT_SYMBOL_GPL(pmc_atom_read);
 
-int pmc_atom_write(int offset, u32 value)
-{
-	struct pmc_dev *pmc = &pmc_device;
-
-	if (!pmc->init)
-		return -ENODEV;
-
-	pmc_reg_write(pmc, offset, value);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pmc_atom_write);
-
 static void pmc_power_off(void)
 {
 	u16	pm1_cnt_port;
diff --git a/include/linux/platform_data/x86/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h
index 022bcea9edec..6807839c718b 100644
--- a/include/linux/platform_data/x86/pmc_atom.h
+++ b/include/linux/platform_data/x86/pmc_atom.h
@@ -144,6 +144,5 @@
 #define	SLEEP_ENABLE		0x2000
 
 extern int pmc_atom_read(int offset, u32 *value);
-extern int pmc_atom_write(int offset, u32 value);
 
 #endif /* PMC_ATOM_H */
-- 
2.17.1

[PATCH 2/4] ACPI: LPSS: make the Kconfig dependency on PMC_ATOM explicit

[Paul Gortmaker]

The code in acpi_lpss.c has been unconditionally using pmc_atom_read()
for about the past six years.  This hasn't been a problem since you
currently can't disable PMC_ATOM short of disabling PCI entirely.

But it doesn't need to be that way, and so that we can change Kconfigs
in a subsequent commit, we make sure LPSS selects PMC_ATOM in advance,
so that existing .config files can live on with "make oldconfig".

In theory, one could make LPSS build w/o PMC_ATOM, similar to what it
did six years ago, but I doubt there is any demand for that now.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Aubrey Li <aubrey.li@linux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 arch/x86/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2ee26f10a814..163c198ec8ec 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -654,6 +654,7 @@ config X86_INTEL_LPSS
 	select COMMON_CLK
 	select PINCTRL
 	select IOSF_MBI
+	select PMC_ATOM
 	help
 	  Select to build support for Intel Low Power Subsystem such as
 	  found on Intel Lynxpoint PCH. Selecting this option enables
-- 
2.17.1

[PATCH 3/4] platform/x86: pmc_atom: dont export pmc_atom_read - no modular users

[Paul Gortmaker]

There is only one user of pmc_atom_read in tree, and that is in
drivers/acpi/acpi_lpss.c -- which can't be anything but built-in.

As such there is no point in adding this function to the global symbol
list exported to modules.

Note that there is no <linux/export.h> include removal since the code
was getting that header implicitly.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Aubrey Li <aubrey.li@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mark Gross <markgross@kernel.org>
Cc: platform-driver-x86@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/pmc_atom.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index 31cf25d25d66..b8b1ed1406de 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -221,7 +221,6 @@ int pmc_atom_read(int offset, u32 *value)
 	*value = pmc_reg_read(pmc, offset);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(pmc_atom_read);
 
 static void pmc_power_off(void)
 {
-- 
2.17.1

[PATCH 4/4] platform/x86: pmc_atom: make the PMC driver actually unselectable

[Paul Gortmaker]

This caught my eye when I saw it was def_bool and hence largely
pointless to have a Kconfig for it at all.

Yet there is no reason why you shouldn't be able to opt out of Atom
platform support if you only care about desktop and server class CPUs.

It was introduced as def_bool, but there is no obvious reason as to why
it was forcibly built-in for everyone, other than LPSS implicitly
relying on it (which is now fixed).  So here we fix up the Kconfig and
open the door for people to opt out.

Since putting "default y" on anything that isn't absolutely essential is
generally frowned upon, I made the default be CONFIG_MATOM.  People who
use "make oldconfig" or similar won't notice any difference.

The two "unchanged" lines for PCI and COMMON_CLK appear in the diff from
fixing a whitespace issue that somehow managed to live on despite being
moved between two different Kconfig files since its introduction.

Cc: Aubrey Li <aubrey.li@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mark Gross <markgross@kernel.org>
Cc: platform-driver-x86@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/Kconfig | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f08ad85683cb..86459e99d831 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1163,6 +1163,11 @@ config WINMATE_FM07_KEYS
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
-       def_bool y
-       depends on PCI
-       select COMMON_CLK
+	bool "Intel Atom SOC Power Management Controller driver"
+	default MATOM
+	depends on PCI
+	select COMMON_CLK
+	help
+	  This enables support for the Atom SOC Power Management Controller
+	  driver, and associated platform clocks.  If you intend to boot this
+	  kernel on platforms with an intel Atom CPU, say Y here.
-- 
2.17.1

Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional

[Andy Shevchenko]

On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote:
> A few months back I was doing a test build for "defconfig-like" COTS
> hardware and happened to notice some pmc-atom stuff being built.  Fine,
> I thought - the defconfig isn't minimal - we all know that - what Kconfig
> do I use to turn that off?  Well, imagine my surprise when I found out
> you couldn't turn it [Atom Power Management Controller] code off!

Turning it off on BayTrail and CherryTrail devices will be devastating
to the users' experience. And plenty of cheap tablets are exactly made
on that SoCs.

> Normally we all agree to not use "default y" unless unavoidable, but
> somehow this was added as "def_bool y" which is even worse ; you can
> see the Kconfig setting but there is *no* way you can turn it off.
> 
> After investigating, I found no reason why the atom code couldn't be
> opt-out with just minor changes that the original addition overlooked.
> 
> And so this series addreses that.  I tried to be sensitive to not
> break any existing configs in the process, but the defconfig will
> now intentionally be different and exclude this atom specific code.
> 
> Using a defconfig on today's linux-next, here is the delta summary:
> 
> $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove
> add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659)
> add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848)
> add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155)
> 
> $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total
> Total: Before=13626994, After=13579335, chg -0.35%
> Total: Before=3572137, After=3565289, chg -0.19%
> Total: Before=1235335, After=1225180, chg -0.82%
> 
> It is hard to reclaim anything against the inevitable growth in
> footprint, so I'd say we should be glad to take whatever we can.
> 
> Boot tested defconfig on today's linux-next on older non-atom COTS.

I believe this needs an extensive test done by Hans who possesses a lot of
problematic devices that require that module to be present.

Note to all your patches, please, use --cc option instead of putting noisy
lines in the each of the commit messages. For myself I created a "smart"
script [1] to avoid bothering with that. Feel free to use, modify, send PRs
back to improve.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] platform/x86: pmc_atom: make the PMC driver actually unselectable

[Andy Shevchenko]

On Thu, Apr 28, 2022 at 02:24:30AM -0400, Paul Gortmaker wrote:
> This caught my eye when I saw it was def_bool and hence largely
> pointless to have a Kconfig for it at all.
> 
> Yet there is no reason why you shouldn't be able to opt out of Atom
> platform support if you only care about desktop and server class CPUs.
> 
> It was introduced as def_bool, but there is no obvious reason as to why
> it was forcibly built-in for everyone, other than LPSS implicitly
> relying on it (which is now fixed).  So here we fix up the Kconfig and
> open the door for people to opt out.
> 
> Since putting "default y" on anything that isn't absolutely essential is
> generally frowned upon, I made the default be CONFIG_MATOM.  People who
> use "make oldconfig" or similar won't notice any difference.
> 
> The two "unchanged" lines for PCI and COMMON_CLK appear in the diff from
> fixing a whitespace issue that somehow managed to live on despite being
> moved between two different Kconfig files since its introduction.

>  config PMC_ATOM
> -       def_bool y
> -       depends on PCI
> -       select COMMON_CLK
> +	bool "Intel Atom SOC Power Management Controller driver"

s/SOC/SoC/ here and everywhere else in this patch.

> +	default MATOM
> +	depends on PCI
> +	select COMMON_CLK
> +	help
> +	  This enables support for the Atom SOC Power Management Controller
> +	  driver, and associated platform clocks.  If you intend to boot this

One space is enough.

> +	  kernel on platforms with an intel Atom CPU, say Y here.

Please, list the Atoms that really need this, for example, Broxton doesn't.

I believe the list should include (but not limited to):

	Intel Bay Trail
	Intel Braswell
	Intel Cherry Trail

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] platform/x86: pmc_atom: remove unused pmc_atom_write()

[Andy Shevchenko]

On Thu, Apr 28, 2022 at 02:24:27AM -0400, Paul Gortmaker wrote:
> This function isn't used anywhere in the driver or anywhere in tree.
> So remove it.  It can always be re-added if/when a use arises.

Can we first move the driver under intel subfolder and then update?

I have no strong opinion, but personally I would keep it (even unexported) just
for the sake of consistency. Up to Mark and Hans.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] ACPI: LPSS: make the Kconfig dependency on PMC_ATOM explicit

[Andy Shevchenko]

On Thu, Apr 28, 2022 at 02:24:28AM -0400, Paul Gortmaker wrote:
> The code in acpi_lpss.c has been unconditionally using pmc_atom_read()
> for about the past six years.  This hasn't been a problem since you
> currently can't disable PMC_ATOM short of disabling PCI entirely.
> 
> But it doesn't need to be that way, and so that we can change Kconfigs
> in a subsequent commit, we make sure LPSS selects PMC_ATOM in advance,
> so that existing .config files can live on with "make oldconfig".
> 
> In theory, one could make LPSS build w/o PMC_ATOM, similar to what it
> did six years ago, but I doubt there is any demand for that now.

I'm wondering if without LPSS we may use those devices to some extend.
If it's the case, this patch is half-baked since it's missed the network
and audio drivers to also enable it (see PMC clock driver registration
and usage).

That said, I'm not sure it's beneficial to spread this selection over
the drivers that may be used widely on non-Intel-Atom platforms.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional

[Paul Gortmaker]

[Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional] On 28/04/2022 (Thu 13:12) Andy Shevchenko wrote:

> On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote:
> > A few months back I was doing a test build for "defconfig-like" COTS
> > hardware and happened to notice some pmc-atom stuff being built.  Fine,
> > I thought - the defconfig isn't minimal - we all know that - what Kconfig
> > do I use to turn that off?  Well, imagine my surprise when I found out
> > you couldn't turn it [Atom Power Management Controller] code off!
> 
> Turning it off on BayTrail and CherryTrail devices will be devastating
> to the users' experience. And plenty of cheap tablets are exactly made
> on that SoCs.

Sure, but I could say the same thing for DRM_I915 and millions of
desktop PC users - yet we still give all the other non i915 users the
option to be able to turn it off.  Pick any other Kconfig value you like
and the same thing holds true.

Just so we are on the same page - I want to give the option to let
people opt out, and at the same time not break existing users. If you
think the defconfig default of being off is too risky, then I am OK with
that and we can just start by exposing the option with "default y".

So, to that end - are you OK with exposing the Kconfig so people can
opt out, or are you 100% against exposing the Kconfig at all?  That
obviously has the most impact on what I do or don't do next.

> > Normally we all agree to not use "default y" unless unavoidable, but
> > somehow this was added as "def_bool y" which is even worse ; you can
> > see the Kconfig setting but there is *no* way you can turn it off.
> > 
> > After investigating, I found no reason why the atom code couldn't be
> > opt-out with just minor changes that the original addition overlooked.
> > 
> > And so this series addreses that.  I tried to be sensitive to not
> > break any existing configs in the process, but the defconfig will
> > now intentionally be different and exclude this atom specific code.
> > 
> > Using a defconfig on today's linux-next, here is the delta summary:
> > 
> > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove
> > add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659)
> > add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848)
> > add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155)
> > 
> > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total
> > Total: Before=13626994, After=13579335, chg -0.35%
> > Total: Before=3572137, After=3565289, chg -0.19%
> > Total: Before=1235335, After=1225180, chg -0.82%
> > 
> > It is hard to reclaim anything against the inevitable growth in
> > footprint, so I'd say we should be glad to take whatever we can.
> > 
> > Boot tested defconfig on today's linux-next on older non-atom COTS.
> 
> I believe this needs an extensive test done by Hans who possesses a lot of
> problematic devices that require that module to be present.

Input from Hans is 100% welcome - but maybe again if we just consider
using "default y" even though it isn't typical - then your concerns are
not as extensive?

> Note to all your patches, please, use --cc option instead of putting noisy
> lines in the each of the commit messages. For myself I created a "smart"
> script [1] to avoid bothering with that. Feel free to use, modify, send PRs
> back to improve.

Thanks - I'm used to explicitly capturing who was looped into the
discussion.  But I hadn't considered how things have evolved so that in
certain circumstances that might be considered just noise with the wider
audience we see in the typical patch sets of today.

Assuming you are OK with exposing the option as a choice, I'll make
things "default y" in v2 to ensure we've minimized risk to existing
users who rely on it, but wait a while for others like Hans to put in
their input.  I'll probably follow up in the individual patches to ask
for clarification if I'm not clear on what you had in mind.

Thanks,
Paul.
--

> 
> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional

[Andy Shevchenko]

On Thu, Apr 28, 2022 at 02:11:31PM -0400, Paul Gortmaker wrote:
> [Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional] On 28/04/2022 (Thu 13:12) Andy Shevchenko wrote:
> > On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote:
> > > A few months back I was doing a test build for "defconfig-like" COTS
> > > hardware and happened to notice some pmc-atom stuff being built.  Fine,
> > > I thought - the defconfig isn't minimal - we all know that - what Kconfig
> > > do I use to turn that off?  Well, imagine my surprise when I found out
> > > you couldn't turn it [Atom Power Management Controller] code off!
> > 
> > Turning it off on BayTrail and CherryTrail devices will be devastating
> > to the users' experience. And plenty of cheap tablets are exactly made
> > on that SoCs.
> 
> Sure, but I could say the same thing for DRM_I915 and millions of
> desktop PC users - yet we still give all the other non i915 users the
> option to be able to turn it off.  Pick any other Kconfig value you like
> and the same thing holds true.
> 
> Just so we are on the same page - I want to give the option to let
> people opt out, and at the same time not break existing users. If you
> think the defconfig default of being off is too risky, then I am OK with
> that and we can just start by exposing the option with "default y".
> 
> So, to that end - are you OK with exposing the Kconfig so people can
> opt out, or are you 100% against exposing the Kconfig at all?  That
> obviously has the most impact on what I do or don't do next.
> 
> > > Normally we all agree to not use "default y" unless unavoidable, but
> > > somehow this was added as "def_bool y" which is even worse ; you can
> > > see the Kconfig setting but there is *no* way you can turn it off.
> > > 
> > > After investigating, I found no reason why the atom code couldn't be
> > > opt-out with just minor changes that the original addition overlooked.
> > > 
> > > And so this series addreses that.  I tried to be sensitive to not
> > > break any existing configs in the process, but the defconfig will
> > > now intentionally be different and exclude this atom specific code.
> > > 
> > > Using a defconfig on today's linux-next, here is the delta summary:
> > > 
> > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove
> > > add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659)
> > > add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848)
> > > add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155)
> > > 
> > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total
> > > Total: Before=13626994, After=13579335, chg -0.35%
> > > Total: Before=3572137, After=3565289, chg -0.19%
> > > Total: Before=1235335, After=1225180, chg -0.82%
> > > 
> > > It is hard to reclaim anything against the inevitable growth in
> > > footprint, so I'd say we should be glad to take whatever we can.
> > > 
> > > Boot tested defconfig on today's linux-next on older non-atom COTS.
> > 
> > I believe this needs an extensive test done by Hans who possesses a lot of
> > problematic devices that require that module to be present.
> 
> Input from Hans is 100% welcome - but maybe again if we just consider
> using "default y" even though it isn't typical - then your concerns are
> not as extensive?
> 
> > Note to all your patches, please, use --cc option instead of putting noisy
> > lines in the each of the commit messages. For myself I created a "smart"
> > script [1] to avoid bothering with that. Feel free to use, modify, send PRs
> > back to improve.
> 
> Thanks - I'm used to explicitly capturing who was looped into the
> discussion.  But I hadn't considered how things have evolved so that in
> certain circumstances that might be considered just noise with the wider
> audience we see in the typical patch sets of today.
> 
> Assuming you are OK with exposing the option as a choice, I'll make
> things "default y" in v2 to ensure we've minimized risk to existing
> users who rely on it, but wait a while for others like Hans to put in
> their input.  I'll probably follow up in the individual patches to ask
> for clarification if I'm not clear on what you had in mind.

My main concern is the existing users' experience. Opting-out the option is a
good to have, I'm not against it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional

[Hans de Goede]

Hi,

On 4/28/22 20:11, Paul Gortmaker wrote:
> [Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional] On 28/04/2022 (Thu 13:12) Andy Shevchenko wrote:
> 
>> On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote:
>>> A few months back I was doing a test build for "defconfig-like" COTS
>>> hardware and happened to notice some pmc-atom stuff being built.  Fine,
>>> I thought - the defconfig isn't minimal - we all know that - what Kconfig
>>> do I use to turn that off?  Well, imagine my surprise when I found out
>>> you couldn't turn it [Atom Power Management Controller] code off!
>>
>> Turning it off on BayTrail and CherryTrail devices will be devastating
>> to the users' experience. And plenty of cheap tablets are exactly made
>> on that SoCs.
> 
> Sure, but I could say the same thing for DRM_I915 and millions of
> desktop PC users - yet we still give all the other non i915 users the
> option to be able to turn it off.  Pick any other Kconfig value you like
> and the same thing holds true.
> 
> Just so we are on the same page - I want to give the option to let
> people opt out, and at the same time not break existing users. If you
> think the defconfig default of being off is too risky, then I am OK with
> that and we can just start by exposing the option with "default y".
> 
> So, to that end - are you OK with exposing the Kconfig so people can
> opt out, or are you 100% against exposing the Kconfig at all?  That
> obviously has the most impact on what I do or don't do next.
> 
>>> Normally we all agree to not use "default y" unless unavoidable, but
>>> somehow this was added as "def_bool y" which is even worse ; you can
>>> see the Kconfig setting but there is *no* way you can turn it off.
>>>
>>> After investigating, I found no reason why the atom code couldn't be
>>> opt-out with just minor changes that the original addition overlooked.
>>>
>>> And so this series addreses that.  I tried to be sensitive to not
>>> break any existing configs in the process, but the defconfig will
>>> now intentionally be different and exclude this atom specific code.
>>>
>>> Using a defconfig on today's linux-next, here is the delta summary:
>>>
>>> $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove
>>> add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659)
>>> add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848)
>>> add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155)
>>>
>>> $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total
>>> Total: Before=13626994, After=13579335, chg -0.35%
>>> Total: Before=3572137, After=3565289, chg -0.19%
>>> Total: Before=1235335, After=1225180, chg -0.82%
>>>
>>> It is hard to reclaim anything against the inevitable growth in
>>> footprint, so I'd say we should be glad to take whatever we can.
>>>
>>> Boot tested defconfig on today's linux-next on older non-atom COTS.
>>
>> I believe this needs an extensive test done by Hans who possesses a lot of
>> problematic devices that require that module to be present.
> 
> Input from Hans is 100% welcome - but maybe again if we just consider
> using "default y" even though it isn't typical - then your concerns are
> not as extensive?

I have no objection against allowing disabling the PMC_ATOM Kconfig option.

As for users breaking support for BYT/CHT setups because they forget
to enable this, without X86_INTEL_LPSS being enabled BYT/CHT are pretty
much broken anyways and since patch 2/4 adds a "select PMC_ATOM" to the
X86_INTEL_LPSS Kconfig option I'm not really worried about that.

I'm afraid this patch-set might break some randomconfig builds though,
but I cannot see anything obviously causing such breakage here, so
I think it would be fine to just merge this series as is and then
see if we get any breakage reports.

Andy, are you ok with me moving ahead and merging this series as is?

Regards,

Hans

Re: [PATCH 4/4] platform/x86: pmc_atom: make the PMC driver actually unselectable

[Hans de Goede]

Hi,

On 4/28/22 08:24, Paul Gortmaker wrote:
> This caught my eye when I saw it was def_bool and hence largely
> pointless to have a Kconfig for it at all.
> 
> Yet there is no reason why you shouldn't be able to opt out of Atom
> platform support if you only care about desktop and server class CPUs.
> 
> It was introduced as def_bool, but there is no obvious reason as to why
> it was forcibly built-in for everyone, other than LPSS implicitly
> relying on it (which is now fixed).  So here we fix up the Kconfig and
> open the door for people to opt out.
> 
> Since putting "default y" on anything that isn't absolutely essential is
> generally frowned upon, I made the default be CONFIG_MATOM.  People who
> use "make oldconfig" or similar won't notice any difference.
> 
> The two "unchanged" lines for PCI and COMMON_CLK appear in the diff from
> fixing a whitespace issue that somehow managed to live on despite being
> moved between two different Kconfig files since its introduction.
> 
> Cc: Aubrey Li <aubrey.li@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Mark Gross <markgross@kernel.org>
> Cc: platform-driver-x86@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  drivers/platform/x86/Kconfig | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f08ad85683cb..86459e99d831 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1163,6 +1163,11 @@ config WINMATE_FM07_KEYS
>  endif # X86_PLATFORM_DEVICES
>  
>  config PMC_ATOM
> -       def_bool y
> -       depends on PCI
> -       select COMMON_CLK
> +	bool "Intel Atom SOC Power Management Controller driver"
> +	default MATOM

Besides the remarks from Andy, this does seem like a weird default,
MATOM means that gcc is passed -march=atom nothing more and nothing
less. For a distro kernel which may e.g. set 
CONFIG_GENERIC_CPU we still want this enabled. But as said it is
brought in by CONFIG_X86_INTEL_LPSS when that is set. 

Thinking more about this I think it might be best to just do this
instead, replacing patch 2 + 4 of this set:

diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile
index 1244c4e568ff..c2088b3c4081 100644
--- a/drivers/clk/x86/Makefile
+++ b/drivers/clk/x86/Makefile
@@ -1,6 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_PMC_ATOM)		+= clk-pmc-atom.o
 obj-$(CONFIG_X86_AMD_PLATFORM_DEVICE)	+= clk-fch.o
-clk-x86-lpss-y			:= clk-lpss-atom.o
-obj-$(CONFIG_X86_INTEL_LPSS)	+= clk-x86-lpss.o
+obj-$(CONFIG_X86_INTEL_LPSS)	+= clk-lpss-atom.o clk-pmc-atom.o
 obj-$(CONFIG_CLK_LGM_CGU)	+= clk-cgu.o clk-cgu-pll.o clk-lgm.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f08ad85683cb..85c396a43048 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1161,8 +1161,3 @@ config WINMATE_FM07_KEYS
 	  that delivers key events when these buttons are pressed.
 
 endif # X86_PLATFORM_DEVICES
-
-config PMC_ATOM
-       def_bool y
-       depends on PCI
-       select COMMON_CLK
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 4a59f47a46e2..cc2a74713313 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -126,7 +126,7 @@ obj-$(CONFIG_INTEL_SCU_PCI)		+= intel_scu_pcidrv.o
 obj-$(CONFIG_INTEL_SCU_PLATFORM)	+= intel_scu_pltdrv.o
 obj-$(CONFIG_INTEL_SCU_WDT)		+= intel_scu_wdt.o
 obj-$(CONFIG_INTEL_SCU_IPC_UTIL)	+= intel_scu_ipcutil.o
-obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
+obj-$(CONFIG_X86_INTEL_LPSS)		+= pmc_atom.o
 
 # Siemens Simatic Industrial PCs
 obj-$(CONFIG_SIEMENS_SIMATIC_IPC)	+= simatic-ipc.o


This just folds the enabling of the pmc_atom code into the same
Kconfig option as the one used the the LPSS code, so this actually
simplify things; while at the same time making it possible to
not build the pmc_atom code by unselecting the CONFIG_X86_INTEL_LPSS
option.

Regards,

Hans

Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional

[Andy Shevchenko]

On Mon, May 02, 2022 at 04:30:57PM +0200, Hans de Goede wrote:
> On 4/28/22 20:11, Paul Gortmaker wrote:

...

> As for users breaking support for BYT/CHT setups because they forget
> to enable this, without X86_INTEL_LPSS being enabled BYT/CHT are pretty
> much broken anyways and since patch 2/4 adds a "select PMC_ATOM" to the
> X86_INTEL_LPSS Kconfig option I'm not really worried about that.
> 
> I'm afraid this patch-set might break some randomconfig builds though,
> but I cannot see anything obviously causing such breakage here, so
> I think it would be fine to just merge this series as is and then
> see if we get any breakage reports.
> 
> Andy, are you ok with me moving ahead and merging this series as is?

It seems as is can't be fulfilled due to your own comment, but in general I'm
not objecting the idea. So, go ahead if you feel it's ready.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional

[Hans de Goede]

Hi,

On 5/2/22 18:05, Andy Shevchenko wrote:
> On Mon, May 02, 2022 at 04:30:57PM +0200, Hans de Goede wrote:
>> On 4/28/22 20:11, Paul Gortmaker wrote:
> 
> ...
> 
>> As for users breaking support for BYT/CHT setups because they forget
>> to enable this, without X86_INTEL_LPSS being enabled BYT/CHT are pretty
>> much broken anyways and since patch 2/4 adds a "select PMC_ATOM" to the
>> X86_INTEL_LPSS Kconfig option I'm not really worried about that.
>>
>> I'm afraid this patch-set might break some randomconfig builds though,
>> but I cannot see anything obviously causing such breakage here, so
>> I think it would be fine to just merge this series as is and then
>> see if we get any breakage reports.
>>
>> Andy, are you ok with me moving ahead and merging this series as is?
> 
> It seems as is can't be fulfilled due to your own comment, but in general I'm
> not objecting the idea. So, go ahead if you feel it's ready.

Right, my later comment to just replace PMC_ATOM with X86_INTEL_LPSS
supersedes this.

I'll send out a patch with that approach so that this can get some
comments / review.

Regards,

Hans

  

Re: [PATCH 2/4] ACPI: LPSS: make the Kconfig dependency on PMC_ATOM explicit

[Rafael J. Wysocki]

On Thu, Apr 28, 2022 at 8:25 AM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> The code in acpi_lpss.c has been unconditionally using pmc_atom_read()
> for about the past six years.  This hasn't been a problem since you
> currently can't disable PMC_ATOM short of disabling PCI entirely.
>
> But it doesn't need to be that way, and so that we can change Kconfigs
> in a subsequent commit, we make sure LPSS selects PMC_ATOM in advance,
> so that existing .config files can live on with "make oldconfig".
>
> In theory, one could make LPSS build w/o PMC_ATOM, similar to what it
> did six years ago, but I doubt there is any demand for that now.

You probably are right and it will get some more build test coverage
with respect to the other option, so

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Aubrey Li <aubrey.li@linux.intel.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  arch/x86/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2ee26f10a814..163c198ec8ec 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -654,6 +654,7 @@ config X86_INTEL_LPSS
>         select COMMON_CLK
>         select PINCTRL
>         select IOSF_MBI
> +       select PMC_ATOM
>         help
>           Select to build support for Intel Low Power Subsystem such as
>           found on Intel Lynxpoint PCH. Selecting this option enables
> --
> 2.17.1
>

Re: [PATCH 1/4] platform/x86: pmc_atom: remove unused pmc_atom_write()

[Hans de Goede]

Hi,

On 4/28/22 08:24, Paul Gortmaker wrote:
> This function isn't used anywhere in the driver or anywhere in tree.
> So remove it.  It can always be re-added if/when a use arises.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Aubrey Li <aubrey.li@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Mark Gross <markgross@kernel.org>
> Cc: platform-driver-x86@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/platform/x86/pmc_atom.c            | 12 ------------
>  include/linux/platform_data/x86/pmc_atom.h |  1 -
>  2 files changed, 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
> index a40fae6edc84..31cf25d25d66 100644
> --- a/drivers/platform/x86/pmc_atom.c
> +++ b/drivers/platform/x86/pmc_atom.c
> @@ -223,18 +223,6 @@ int pmc_atom_read(int offset, u32 *value)
>  }
>  EXPORT_SYMBOL_GPL(pmc_atom_read);
>  
> -int pmc_atom_write(int offset, u32 value)
> -{
> -	struct pmc_dev *pmc = &pmc_device;
> -
> -	if (!pmc->init)
> -		return -ENODEV;
> -
> -	pmc_reg_write(pmc, offset, value);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(pmc_atom_write);
> -
>  static void pmc_power_off(void)
>  {
>  	u16	pm1_cnt_port;
> diff --git a/include/linux/platform_data/x86/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h
> index 022bcea9edec..6807839c718b 100644
> --- a/include/linux/platform_data/x86/pmc_atom.h
> +++ b/include/linux/platform_data/x86/pmc_atom.h
> @@ -144,6 +144,5 @@
>  #define	SLEEP_ENABLE		0x2000
>  
>  extern int pmc_atom_read(int offset, u32 *value);
> -extern int pmc_atom_write(int offset, u32 value);
>  
>  #endif /* PMC_ATOM_H */

Re: [PATCH 3/4] platform/x86: pmc_atom: dont export pmc_atom_read - no modular users

[Hans de Goede]

Hi,

On 4/28/22 08:24, Paul Gortmaker wrote:
> There is only one user of pmc_atom_read in tree, and that is in
> drivers/acpi/acpi_lpss.c -- which can't be anything but built-in.
> 
> As such there is no point in adding this function to the global symbol
> list exported to modules.
> 
> Note that there is no <linux/export.h> include removal since the code
> was getting that header implicitly.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Aubrey Li <aubrey.li@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Mark Gross <markgross@kernel.org>
> Cc: platform-driver-x86@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/platform/x86/pmc_atom.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
> index 31cf25d25d66..b8b1ed1406de 100644
> --- a/drivers/platform/x86/pmc_atom.c
> +++ b/drivers/platform/x86/pmc_atom.c
> @@ -221,7 +221,6 @@ int pmc_atom_read(int offset, u32 *value)
>  	*value = pmc_reg_read(pmc, offset);
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(pmc_atom_read);
>  
>  static void pmc_power_off(void)
>  {