| Message ID | 20210329174928.18816-1-henning.schild@siemens.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
Guys, sorry for the delay. This one did in fact not change too much after all. The biggest points that are still kind of open are the naming of the LEDs. If what is proposed here is acceptable it is not open from my side. The other big point was "using a generic gpio" driver as a basis. My current understanding of that point is, that such a driver does not yet exist. Meaning an introduction of the abstractions can and probably should wait for a second user. Without the second user it is just hard to test and find the right abstraction, plus we will end up with more code meaning more work for everyone. regards, Henning Am Mon, 29 Mar 2021 19:49:24 +0200 schrieb Henning Schild <henning.schild@siemens.com>: > 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 | 57 +++-- > drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++ > drivers/watchdog/Kconfig | 11 + > drivers/watchdog/Makefile | 1 + > drivers/watchdog/simatic-ipc-wdt.c | 215 > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h | > 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++ > 14 files changed, 769 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 >
Hi, On 3/29/21 7:49 PM, Henning Schild wrote: > 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. IT seems there still is significant discussion surrounding the LED and watchdog drivers which use patch 1/4 as parent-driver. I'm going to hold of on merging 1/4 and 4/4 until there is more consensus surrounding this series. Regards, Hans > > 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 | 57 +++-- > drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++ > drivers/watchdog/Kconfig | 11 + > drivers/watchdog/Makefile | 1 + > drivers/watchdog/simatic-ipc-wdt.c | 215 ++++++++++++++++++ > .../platform_data/x86/simatic-ipc-base.h | 29 +++ > include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++ > 14 files changed, 769 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 >
Am Wed, 7 Apr 2021 13:36:40 +0200 schrieb Hans de Goede <hdegoede@redhat.com>: > Hi, > > On 3/29/21 7:49 PM, Henning Schild wrote: > > 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. > > IT seems there still is significant discussion surrounding the LED > and watchdog drivers which use patch 1/4 as parent-driver. > > I'm going to hold of on merging 1/4 and 4/4 until there is more > consensus surrounding this series. Yes. Whithout 2 and 3, 1 would be way too big. Henning > Regards, > > Hans > > > > > > 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 | 57 +++-- > > drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++ > > drivers/watchdog/Kconfig | 11 + > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/simatic-ipc-wdt.c | 215 > > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h | > > 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++ > > 14 files changed, 769 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 >
This series is basically stuck because people rightfully want me to use the GPIO subsystem for the LEDs and the watchdog bits that are connected to GPIO. Problem is that the GPIO subsystem does not initialize on the machines in question. It is a combination of hidden P2SB and missing ACPI table entries. The GPIO subsystem (intel pinctrl) needs either P2SB or ACPI do come up ... Andy proposed some patches for initializing the intel pinctrl stuff for one of the machines by falling back to SoC detection in case there is no ACPI or visible P2SB. While that works it would need to be done for any Intel SoC to be consistent and discussions seem to go nowhere. I would be willing to port over to "intel pintctl" and help with testing, but not so much with actual coding. Andy is that moving at all? Since my drivers do reserve the mmio regions properly and the intel pinctrl will never come up anyways, i do not see a conflict merging my proposed drivers in the current codebase. The wish to use the pinctrl infrastructure can not be fulfilled if that infra is not in place. Once intel pinctrl works, we can change those drivers to work with that. I do not want to take shortcuts ... but also do not want to get stuck here. So maybe one way to serialize the merge is to allow my changes like proposed and rebase on intel pinctrl once that subsystem actually initializes on these machines. We could even have two code paths ... if region can not be reserved, try gpio ... or the other way around. regards, Henning Am Wed, 7 Apr 2021 13:36:40 +0200 schrieb Hans de Goede <hdegoede@redhat.com>: > Hi, > > On 3/29/21 7:49 PM, Henning Schild wrote: > > 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. > > IT seems there still is significant discussion surrounding the LED > and watchdog drivers which use patch 1/4 as parent-driver. > > I'm going to hold of on merging 1/4 and 4/4 until there is more > consensus surrounding this series. > > Regards, > > Hans > > > > > > 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 | 57 +++-- > > drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++ > > drivers/watchdog/Kconfig | 11 + > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/simatic-ipc-wdt.c | 215 > > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h | > > 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++ > > 14 files changed, 769 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 Mon, Jul 12, 2021 at 2:35 PM Henning Schild <henning.schild@siemens.com> wrote: > > This series is basically stuck because people rightfully want me to use > the GPIO subsystem for the LEDs and the watchdog bits that are > connected to GPIO. > > Problem is that the GPIO subsystem does not initialize on the machines > in question. It is a combination of hidden P2SB and missing ACPI table > entries. The GPIO subsystem (intel pinctrl) needs either P2SB or ACPI do > come up ... > > Andy proposed some patches for initializing the intel pinctrl stuff for > one of the machines by falling back to SoC detection in case there is > no ACPI or visible P2SB. While that works it would need to be done for > any Intel SoC to be consistent and discussions seem to go nowhere. > > I would be willing to port over to "intel pintctl" and help with > testing, but not so much with actual coding. Andy is that moving at all? > > Since my drivers do reserve the mmio regions properly and the intel > pinctrl will never come up anyways, i do not see a conflict merging my > proposed drivers in the current codebase. The wish to use the pinctrl > infrastructure can not be fulfilled if that infra is not in place. Once > intel pinctrl works, we can change those drivers to work with that. > > I do not want to take shortcuts ... but also do not want to get stuck > here. So maybe one way to serialize the merge is to allow my changes > like proposed and rebase on intel pinctrl once that subsystem actually > initializes on these machines. We could even have two code paths ... if > region can not be reserved, try gpio ... or the other way around. Bjorn suggested exercising the IORESOURCE_PCI_FIXED on top of the early PCI quirk that unhides P2SB for the entire run time. But I have had no time to actually patch the kernel this way. Have tried the proposed approach on your side?
Am Mon, 12 Jul 2021 15:09:04 +0300 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Mon, Jul 12, 2021 at 2:35 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > This series is basically stuck because people rightfully want me to > > use the GPIO subsystem for the LEDs and the watchdog bits that are > > connected to GPIO. > > > > Problem is that the GPIO subsystem does not initialize on the > > machines in question. It is a combination of hidden P2SB and > > missing ACPI table entries. The GPIO subsystem (intel pinctrl) > > needs either P2SB or ACPI do come up ... > > > > Andy proposed some patches for initializing the intel pinctrl stuff > > for one of the machines by falling back to SoC detection in case > > there is no ACPI or visible P2SB. While that works it would need to > > be done for any Intel SoC to be consistent and discussions seem to > > go nowhere. > > > > I would be willing to port over to "intel pintctl" and help with > > testing, but not so much with actual coding. Andy is that moving at > > all? > > > > Since my drivers do reserve the mmio regions properly and the intel > > pinctrl will never come up anyways, i do not see a conflict merging > > my proposed drivers in the current codebase. The wish to use the > > pinctrl infrastructure can not be fulfilled if that infra is not in > > place. Once intel pinctrl works, we can change those drivers to > > work with that. > > > > I do not want to take shortcuts ... but also do not want to get > > stuck here. So maybe one way to serialize the merge is to allow my > > changes like proposed and rebase on intel pinctrl once that > > subsystem actually initializes on these machines. We could even > > have two code paths ... if region can not be reserved, try gpio ... > > or the other way around. > > Bjorn suggested exercising the IORESOURCE_PCI_FIXED on top of the > early PCI quirk that unhides P2SB for the entire run time. But I have > had no time to actually patch the kernel this way. Have tried the > proposed approach on your side? Unhiding the P2SB (even if permanent and fixed) alone will not trigger pinctrl to initialize. One would still need something along the lines of "mfd: lpc_ich: Add support for pinctrl in non-ACPI system" for all SoCs. I guess it could be an improvement to your series, but i honestly do not see all fitting together too soon. Since your p2sb series still initializes the GPIO with two different names (depending on whether it was PCI or ACPI) and only for one SoC, while this series would need two ... and a consistent solution many more. Henning
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 | 57 +++-- drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/simatic-ipc-wdt.c | 215 ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h | 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++ 14 files changed, 769 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