changes since v3:
- fix io access width and region reservations
- fix style in p1
changes since v2:
- remove "simatic-ipc" prefix from LED names
- fix style issues found in v2, mainly LED driver
- fix OEM specific dmi code, and remove magic numbers
- more "simatic_ipc" name prefixing
- improved pmc quirk code using callbacks
changes since v1:
- fixed lots of style issues found in v1
- (debug) printing
- header ordering
- fixed license issues GPLv2 and SPDX in all files
- module_platform_driver instead of __init __exit
- wdt simplifications cleanup
- lots of fixes in wdt driver, all that was found in v1
- fixed dmi length in dmi helper
- changed LED names to allowed ones
- move led driver to simple/
- switched pmc_atom to dmi callback with global variable
This series adds support for watchdogs and leds of several x86 devices
from Siemens.
It is structured with a platform driver that mainly does identification
of the machines. It might trigger loading of the actual device drivers
by attaching devices to the platform bus.
The identification is vendor specific, parsing a special binary DMI
entry. The implementation of that platform identification is applied on
pmc_atom clock quirks in the final patch.
It is all structured in a way that we can easily add more devices and
more platform drivers later. Internally we have some more code for
hardware monitoring, more leds, watchdogs etc. This will follow some
day.
Henning Schild (4):
platform/x86: simatic-ipc: add main driver for Siemens devices
leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
platform/x86: pmc_atom: improve critclk_systems matching for Siemens
PCs
drivers/leds/Kconfig | 3 +
drivers/leds/Makefile | 3 +
drivers/leds/simple/Kconfig | 11 +
drivers/leds/simple/Makefile | 2 +
drivers/leds/simple/simatic-ipc-leds.c | 202 ++++++++++++++++
drivers/platform/x86/Kconfig | 12 +
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/pmc_atom.c | 54 +++--
drivers/platform/x86/simatic-ipc.c | 168 +++++++++++++
drivers/watchdog/Kconfig | 11 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/simatic-ipc-wdt.c | 228 ++++++++++++++++++
.../platform_data/x86/simatic-ipc-base.h | 29 +++
include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
14 files changed, 778 insertions(+), 21 deletions(-)
create mode 100644 drivers/leds/simple/Kconfig
create mode 100644 drivers/leds/simple/Makefile
create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
create mode 100644 drivers/platform/x86/simatic-ipc.c
create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
--
2.32.0
Siemens industrial PCs unfortunately can not always be properly
identified the way we used to. An earlier commit introduced code that
allows proper identification without looking at DMI strings that could
differ based on product branding.
Switch over to that proper way and revert commits that used to collect
the machines based on unstable strings.
Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Fixes: e8796c6c69d1 ("platform/x86: pmc_atom: Add Siemens CONNECT ...")
Fixes: f110d252ae79 ("platform/x86: pmc_atom: Add Siemens SIMATIC ...")
Fixes: ad0d315b4d4e ("platform/x86: pmc_atom: Add Siemens SIMATIC ...")
Tested-by: Michael Haener <[email protected]>
Signed-off-by: Henning Schild <[email protected]>
---
drivers/platform/x86/pmc_atom.c | 54 ++++++++++++++++++++-------------
1 file changed, 33 insertions(+), 21 deletions(-)
diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index a9d2a4b98e57..a40fae6edc84 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -13,6 +13,7 @@
#include <linux/io.h>
#include <linux/platform_data/x86/clk-pmc-atom.h>
#include <linux/platform_data/x86/pmc_atom.h>
+#include <linux/platform_data/x86/simatic-ipc.h>
#include <linux/platform_device.h>
#include <linux/pci.h>
#include <linux/seq_file.h>
@@ -362,6 +363,30 @@ static void pmc_dbgfs_register(struct pmc_dev *pmc)
}
#endif /* CONFIG_DEBUG_FS */
+static bool pmc_clk_is_critical = true;
+
+static int dmi_callback(const struct dmi_system_id *d)
+{
+ pr_info("%s critclks quirk enabled\n", d->ident);
+
+ return 1;
+}
+
+static int dmi_callback_siemens(const struct dmi_system_id *d)
+{
+ u32 st_id;
+
+ if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &st_id))
+ goto out;
+
+ if (st_id == SIMATIC_IPC_IPC227E || st_id == SIMATIC_IPC_IPC277E)
+ return dmi_callback(d);
+
+out:
+ pmc_clk_is_critical = false;
+ return 1;
+}
+
/*
* Some systems need one or more of their pmc_plt_clks to be
* marked as critical.
@@ -370,6 +395,7 @@ static const struct dmi_system_id critclk_systems[] = {
{
/* pmc_plt_clk0 is used for an external HSIC USB HUB */
.ident = "MPL CEC1x",
+ .callback = dmi_callback,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "MPL AG"),
DMI_MATCH(DMI_PRODUCT_NAME, "CEC10 Family"),
@@ -378,6 +404,7 @@ static const struct dmi_system_id critclk_systems[] = {
{
/* pmc_plt_clk0 - 3 are used for the 4 ethernet controllers */
.ident = "Lex 3I380D",
+ .callback = dmi_callback,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Lex BayTrail"),
DMI_MATCH(DMI_PRODUCT_NAME, "3I380D"),
@@ -386,6 +413,7 @@ static const struct dmi_system_id critclk_systems[] = {
{
/* pmc_plt_clk* - are used for ethernet controllers */
.ident = "Lex 2I385SW",
+ .callback = dmi_callback,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Lex BayTrail"),
DMI_MATCH(DMI_PRODUCT_NAME, "2I385SW"),
@@ -394,30 +422,17 @@ static const struct dmi_system_id critclk_systems[] = {
{
/* pmc_plt_clk* - are used for ethernet controllers */
.ident = "Beckhoff Baytrail",
+ .callback = dmi_callback,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
DMI_MATCH(DMI_PRODUCT_FAMILY, "CBxx63"),
},
},
{
- .ident = "SIMATIC IPC227E",
+ .ident = "SIEMENS AG",
+ .callback = dmi_callback_siemens,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
- DMI_MATCH(DMI_PRODUCT_VERSION, "6ES7647-8B"),
- },
- },
- {
- .ident = "SIMATIC IPC277E",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
- DMI_MATCH(DMI_PRODUCT_VERSION, "6AV7882-0"),
- },
- },
- {
- .ident = "CONNECT X300",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
- DMI_MATCH(DMI_PRODUCT_VERSION, "A5E45074588"),
},
},
@@ -429,7 +444,6 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
{
struct platform_device *clkdev;
struct pmc_clk_data *clk_data;
- const struct dmi_system_id *d = dmi_first_match(critclk_systems);
clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
if (!clk_data)
@@ -437,10 +451,8 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
clk_data->base = pmc_regmap; /* offset is added by client */
clk_data->clks = pmc_data->clks;
- if (d) {
- clk_data->critical = true;
- pr_info("%s critclks quirk enabled\n", d->ident);
- }
+ if (dmi_check_system(critclk_systems))
+ clk_data->critical = pmc_clk_is_critical;
clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom",
PLATFORM_DEVID_NONE,
--
2.32.0
On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
<[email protected]> wrote:
>
> Siemens industrial PCs unfortunately can not always be properly
> identified the way we used to. An earlier commit introduced code that
> allows proper identification without looking at DMI strings that could
> differ based on product branding.
> Switch over to that proper way and revert commits that used to collect
> the machines based on unstable strings.
Usually we start a series with fixes, but I guess it's fine here since
this can be taken separately, right?
...
> +#include <linux/platform_data/x86/simatic-ipc.h>
Seems not. Question is then, what Fixes tags would mean in this case?
--
With Best Regards,
Andy Shevchenko
Am Fri, 26 Nov 2021 16:51:00 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
> <[email protected]> wrote:
> >
> > Siemens industrial PCs unfortunately can not always be properly
> > identified the way we used to. An earlier commit introduced code
> > that allows proper identification without looking at DMI strings
> > that could differ based on product branding.
> > Switch over to that proper way and revert commits that used to
> > collect the machines based on unstable strings.
>
> Usually we start a series with fixes, but I guess it's fine here since
> this can be taken separately, right?
>
> ...
It can not be taken because it needs p1 to work. And p1 is mainly here
for p2 and p3 really. Splitting the patches up into
p1.1
p4
p1.2
p2
p3
would be possible but a lot of work for just that ordering topic i
guess.
> > +#include <linux/platform_data/x86/simatic-ipc.h>
>
> Seems not. Question is then, what Fixes tags would mean in this
> case?
>
Which of the several tags confuses you? Maybe i just need to drop a few.
the original problem is
Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
which introduced the need for several quirks
Fixes: e8796c6c69d1 ("platform/x86: pmc_atom: Add Siemens CONNECT...")
Fixes: f110d252ae79 ("platform/x86: pmc_atom: Add Siemens SIMATIC...")
Fixes: ad0d315b4d4e ("platform/x86: pmc_atom: Add Siemens SIMATIC...")
These quirks use unstable dmi information. Unstable because the
DMI_PRODUCT_VERSION can be branded. Yes weird ... we do not allow new
ACPI entries but do allow custom dmi ...
p1 introduces the use of stable dmi and p4 brings that into the quirk
table ... fixing all the quirks based on customizable
DMI_PRODUCT_VERSION
Henning
Hi all,
in p3 not too much was left open so i hope this might be the last and
might be quick.
The two points that have been open where:
1 wish to split wdt driver into two
2 wish to use pinctrl for LEDs/WDT
1 was ignored for now. We can split later when we add more devices. It
remains unclear if splitting is the way to go when more devices come.
Could also be that it would become too many and spreading code over
many files could make maint harder.
Plus a split of wdt alone would make things inconsistent with LEDs. And
if we did split that we would end up with even more files/drivers.
2 can not be done because pinctrl does not probe on any of the PCs.
There are patches floating around but still under review/construction.
There is no technical conflict it is more an ordering issue. So i
kindly ask to let these drivers in and not have them stuck behind the
P2SB patches. I once offered to wait, but its been many months.
regards,
Henning
Am Fri, 26 Nov 2021 15:10:23 +0100
schrieb Henning Schild <[email protected]>:
> changes since v3:
>
> - fix io access width and region reservations
> - fix style in p1
>
> changes since v2:
>
> - remove "simatic-ipc" prefix from LED names
> - fix style issues found in v2, mainly LED driver
> - fix OEM specific dmi code, and remove magic numbers
> - more "simatic_ipc" name prefixing
> - improved pmc quirk code using callbacks
>
> changes since v1:
>
> - fixed lots of style issues found in v1
> - (debug) printing
> - header ordering
> - fixed license issues GPLv2 and SPDX in all files
> - module_platform_driver instead of __init __exit
> - wdt simplifications cleanup
> - lots of fixes in wdt driver, all that was found in v1
> - fixed dmi length in dmi helper
> - changed LED names to allowed ones
> - move led driver to simple/
> - switched pmc_atom to dmi callback with global variable
>
>
> This series adds support for watchdogs and leds of several x86 devices
> from Siemens.
>
> It is structured with a platform driver that mainly does
> identification of the machines. It might trigger loading of the
> actual device drivers by attaching devices to the platform bus.
>
> The identification is vendor specific, parsing a special binary DMI
> entry. The implementation of that platform identification is applied
> on pmc_atom clock quirks in the final patch.
>
> It is all structured in a way that we can easily add more devices and
> more platform drivers later. Internally we have some more code for
> hardware monitoring, more leds, watchdogs etc. This will follow some
> day.
>
> Henning Schild (4):
> platform/x86: simatic-ipc: add main driver for Siemens devices
> leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
> platform/x86: pmc_atom: improve critclk_systems matching for Siemens
> PCs
>
> drivers/leds/Kconfig | 3 +
> drivers/leds/Makefile | 3 +
> drivers/leds/simple/Kconfig | 11 +
> drivers/leds/simple/Makefile | 2 +
> drivers/leds/simple/simatic-ipc-leds.c | 202 ++++++++++++++++
> drivers/platform/x86/Kconfig | 12 +
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/pmc_atom.c | 54 +++--
> drivers/platform/x86/simatic-ipc.c | 168 +++++++++++++
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 228
> ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h |
> 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
> 14 files changed, 778 insertions(+), 21 deletions(-)
> create mode 100644 drivers/leds/simple/Kconfig
> create mode 100644 drivers/leds/simple/Makefile
> create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> create mode 100644 drivers/platform/x86/simatic-ipc.c
> create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
> create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
>
On 11/26/21 7:34 AM, Henning Schild wrote:
> Hi all,
>
> in p3 not too much was left open so i hope this might be the last and
> might be quick.
>
> The two points that have been open where:
> 1 wish to split wdt driver into two
> 2 wish to use pinctrl for LEDs/WDT
>
> 1 was ignored for now. We can split later when we add more devices. It
> remains unclear if splitting is the way to go when more devices come.
The code is already quite messy, in part because memory regions are
declared locally and not passed through the parent device as they should.
I don't see how splitting the driver into multiple drivers would improve
the situation. The platform code claims to be inspired by the lpc_ich
driver. Maybe it should take a real example from that and pass version
or variant specific details through platform data instead of maintaining
it in the watchdog driver.
Guenter
Am Fri, 26 Nov 2021 08:02:11 -0800
schrieb Guenter Roeck <[email protected]>:
> On 11/26/21 7:34 AM, Henning Schild wrote:
> > Hi all,
> >
> > in p3 not too much was left open so i hope this might be the last
> > and might be quick.
> >
> > The two points that have been open where:
> > 1 wish to split wdt driver into two
> > 2 wish to use pinctrl for LEDs/WDT
> >
> > 1 was ignored for now. We can split later when we add more devices.
> > It remains unclear if splitting is the way to go when more devices
> > come.
>
> The code is already quite messy, in part because memory regions are
> declared locally and not passed through the parent device as they
> should.
Oh is it, can you please be more precise when being so "harsh". The
parent driver is just a detector which devices in which variant are
available on a given box. It does not drive and should not claim
resources i guess.
> I don't see how splitting the driver into multiple drivers
> would improve the situation.
In fact that already is a split i would say.
> The platform code claims to be inspired by the lpc_ich driver.
Only the P2SB bit, getting the memory base of the pinctrl memory.
> Maybe it should take a real example from that
> and pass version or variant specific details through platform data
> instead of maintaining it in the watchdog driver.
It does that ... or i misunderstand you. The driver from p1 can find
these three watchdog types
SIMATIC_IPC_DEVICE_NONE
SIMATIC_IPC_DEVICE_227E
SIMATIC_IPC_DEVICE_427E
Which one to use is determined in the platform driver that will
platform_device_register_data(NULL, KBUILD_MODNAME "_wdt" ...
&platform_data)
So we tell the watchdog driver about a watchdog and tell it which one
...
If we ever split we could
register(KBUILD_MODNAME "_wdt_227e"
or
register(KBUILD_MODNAME "_wdt_427e"
with no platform data needed anymore.
But as i said, i do not want to split just yet. Just want to defend
that code when someone calls it "pretty messy" so late in the process.
Henning
> Guenter
Hi all,
to me it currently looks like a v5 will only differ in terms of
documentation and could be acceptable to all people who have raised
concerns on v4.
Will send that v5 soon. And invite anyone to please raise further
concerns, or review again.
regards,
Henning
Am Fri, 26 Nov 2021 15:10:23 +0100
schrieb Henning Schild <[email protected]>:
> changes since v3:
>
> - fix io access width and region reservations
> - fix style in p1
>
> changes since v2:
>
> - remove "simatic-ipc" prefix from LED names
> - fix style issues found in v2, mainly LED driver
> - fix OEM specific dmi code, and remove magic numbers
> - more "simatic_ipc" name prefixing
> - improved pmc quirk code using callbacks
>
> changes since v1:
>
> - fixed lots of style issues found in v1
> - (debug) printing
> - header ordering
> - fixed license issues GPLv2 and SPDX in all files
> - module_platform_driver instead of __init __exit
> - wdt simplifications cleanup
> - lots of fixes in wdt driver, all that was found in v1
> - fixed dmi length in dmi helper
> - changed LED names to allowed ones
> - move led driver to simple/
> - switched pmc_atom to dmi callback with global variable
>
>
> This series adds support for watchdogs and leds of several x86 devices
> from Siemens.
>
> It is structured with a platform driver that mainly does
> identification of the machines. It might trigger loading of the
> actual device drivers by attaching devices to the platform bus.
>
> The identification is vendor specific, parsing a special binary DMI
> entry. The implementation of that platform identification is applied
> on pmc_atom clock quirks in the final patch.
>
> It is all structured in a way that we can easily add more devices and
> more platform drivers later. Internally we have some more code for
> hardware monitoring, more leds, watchdogs etc. This will follow some
> day.
>
> Henning Schild (4):
> platform/x86: simatic-ipc: add main driver for Siemens devices
> leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
> platform/x86: pmc_atom: improve critclk_systems matching for Siemens
> PCs
>
> drivers/leds/Kconfig | 3 +
> drivers/leds/Makefile | 3 +
> drivers/leds/simple/Kconfig | 11 +
> drivers/leds/simple/Makefile | 2 +
> drivers/leds/simple/simatic-ipc-leds.c | 202 ++++++++++++++++
> drivers/platform/x86/Kconfig | 12 +
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/pmc_atom.c | 54 +++--
> drivers/platform/x86/simatic-ipc.c | 168 +++++++++++++
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 228
> ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h |
> 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
> 14 files changed, 778 insertions(+), 21 deletions(-)
> create mode 100644 drivers/leds/simple/Kconfig
> create mode 100644 drivers/leds/simple/Makefile
> create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> create mode 100644 drivers/platform/x86/simatic-ipc.c
> create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
> create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
>